Fix and Enhance E2E ACNP Baseline Isolation Test #6218
Conversation
test/e2e/antreapolicy_test.go
Outdated
| builder1.AddIngress(ProtocolTCP, &p80, nil, nil, nil, nil, nil, nil, nil, nil, nil, | ||
| nil, []metav1.LabelSelectorRequirement{podExpOtherThanC}, nil, nil, | ||
| nil, nil, crdv1beta1.RuleActionDrop, "", "", nil) |
There was a problem hiding this comment.
A util function with so many arguments is difficult to use and error prone, especially when adjacent parameters are of the same type.
We should consider refactoring it in a more clear and flexible way some day.
test/e2e/antreapolicy_test.go
Outdated
| testStep := []*TestStep{ | ||
| { | ||
| Name: "Port 80", | ||
| Name: "Two baseline ACNPs", |
There was a problem hiding this comment.
I'm not sure if the rewriting is necessary. It looks to me the real problem is the ACNP's effect regarding x->x has the same effect as the K8s policy's x->x, so the issue was not detected. Should we just remove the x->x part in K8s policy?
There was a problem hiding this comment.
This PR actually started from #6111 when adding e2e tests for NetworkPolicyEvaluation, otherwise it won't be detected. The purpose was to test that
- baseline ACNP works
- KNP enforced before baseline ACNP
- KNP default isolation also enforced before baseline ACNP
- baseline ACNP remains effective if not selected by KNP
So I will test each of these aspects returns correct rule result in NetworkPolicyEvaluation tests.
There was a problem hiding this comment.
Are 1 and 4 essentially the same? The only difference is 1 has no K8s policy while 4 has irrelevant K8s policy.
Regardless of whether 1 and 4 are duplicate, I think just changing the original test a little can still cover it:
- Keep the ACNP acnp-baseline-isolate-ns-x unchanged
- Make the K8s policy applied to x/a, allow y/a
Then:
only y/a can access x/a, proving 2 and 3 works
only x can access x/b and x/c, proving 1 and 4 works
There was a problem hiding this comment.
Sure that sounds good!
qiyueyao
changed the title
qiyueyao
force-pushed
the
fix-e2e-baseline
branch
from
b94c34c to
82db9e7
Compare
qiyueyao
force-pushed
the
fix-e2e-baseline
branch
from
82db9e7 to
b5e966f
Compare
test/e2e/antreapolicy_test.go
Outdated
| // create a K8s NetworkPolicy for the x/a Pod to allow ingress traffic from Pods in the same namespace, | ||
| // as well as from the y/a Pod. It should open up ingress from y/a and deny ingress from y/b y/c based on | ||
| // Kubernetes NetworkPolicy default isolation model, since it's evaluated before the baseline tier. |
There was a problem hiding this comment.
I think this won't validate the 3rd point: KNP default isolation also enforced before baseline ACNP.
Even if the implementation is the opposite, the test can still pass.
My suggestion was to remove ns-x from the rule, so only y/a is expected to access x/a, proving it's enforced before baseline ACNP.
qiyueyao
force-pushed
the
fix-e2e-baseline
branch
from
b5e966f to
c032804
Compare
In current baseline isolation e2e test, the nsSelector expression is missplaced as nodeSelector expression, yet the test case does not detect the error based on current test factors. This PR fixes the nsSelector expression issue, and also adds test factors in the baseline isolation test case to increase coverage. Signed-off-by: Qiyue Yao <[email protected]>
qiyueyao
force-pushed
the
fix-e2e-baseline
branch
from
c032804 to
e0ab7fb
Compare
|
/skip-all |
In current baseline isolation e2e test, the nsSelector expression is misplaced as nodeSelector expression, yet the test case does not detect the error based on current test factors.
This PR fixes the nsSelector expression issue, and also adds test factors in the baseline isolation test case to increase coverage.