Adds unit tests for pkg/controller/networkpolicy/validate.go #4629
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4629 +/- ##
==========================================
- Coverage 72.33% 71.48% -0.86%
==========================================
Files 409 409
Lines 61181 63960 +2779
==========================================
+ Hits 44258 45719 +1461
- Misses 13979 15253 +1274
- Partials 2944 2988 +44
*This pull request uses carry forward flags. Click here to find out more. |
| report = crdv1alpha1.IGMPReportV1 | ||
| allowAction = crdv1alpha1.RuleActionAllow | ||
| passAction = crdv1alpha1.RuleActionPass | ||
| int32For80 = int32(80) |
There was a problem hiding this comment.
use some significant variable name
There was a problem hiding this comment.
These variables was already present in the test code
There was a problem hiding this comment.
You can refine the name if it's not proper, I would suggest to name it port80.
| tests := []struct { | ||
| name string | ||
| featureGates map[featuregate.Feature]bool | ||
| policy *crdv1alpha1.ClusterNetworkPolicy | ||
| oldPolicy *crdv1alpha1.ClusterNetworkPolicy | ||
| op admv1.Operation |
There was a problem hiding this comment.
instead of op keep it operation
| Tier: "non-existent-tier", | ||
| }, | ||
| }, | ||
| op: admv1.Update, |
There was a problem hiding this comment.
no expectedReason for this test case?
| }, | ||
| }, | ||
| op: admv1.Update, | ||
| }, |
There was a problem hiding this comment.
no test cases for delete operation?
There was a problem hiding this comment.
// Delete of Antrea Policies have no validation. This will be an empty for loop. So no test case for delete
| group *crdv1alpha3.Group | ||
| currGroup *crdv1alpha3.Group | ||
| oldGroup *crdv1alpha3.Group | ||
| op admv1.Operation |
| }, | ||
| }, | ||
| }, | ||
| op: admv1.Update, |
There was a problem hiding this comment.
no expected reason, and what about the cases including other two operations?
| Priority: 3, | ||
| }, | ||
| }, | ||
| op: admv1.Delete, |
There was a problem hiding this comment.
if there is no value for the expectedReason field then what's the point adding it to the structure?
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| _, c := newController() | ||
| v := NewNetworkPolicyValidator(c.NetworkPolicyController) |
There was a problem hiding this comment.
avoid using single character names, use some significant variable names.
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| var v *antreaPolicyValidator |
luolanzone
added
the
area/test
urharshitha
force-pushed
the
validateTesting
branch
2 times, most recently
from
9c174bf
to
9fee64d
Compare
| report = crdv1alpha1.IGMPReportV1 | ||
| allowAction = crdv1alpha1.RuleActionAllow | ||
| passAction = crdv1alpha1.RuleActionPass | ||
| int32For80 = int32(80) |
There was a problem hiding this comment.
You can refine the name if it's not proper, I would suggest to name it port80.
| @@ -1509,12 +1595,14 @@ func TestValidateAntreaClusterGroup(t *testing.T) { | |||
| func TestValidateAntreaGroup(t *testing.T) { | |||
| tests := []struct { | |||
| name string | |||
| group *crdv1alpha3.Group | |||
| currGroup *crdv1alpha3.Group | |||
There was a problem hiding this comment.
Usually we define it with perfix cur instead of curr.
| currGroup *crdv1alpha3.Group | |
| curGroup *crdv1alpha3.Group |
| operation: admv1.Update, | ||
| }, | ||
| { | ||
| name: "to-delete", |
There was a problem hiding this comment.
better to follow other test case name like anp-group-to-delete
| name: "to-delete", | ||
| oldGroup: &crdv1alpha3.Group{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "anp-group-set-with-podselector-specified-to-update", |
There was a problem hiding this comment.
The name is not proper, you can remove -to-update
| func TestValidateTier(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| currTier *crdv1alpha1.Tier |
| operation admv1.Operation | ||
| }{ | ||
| { | ||
| name: "create-validate-tier", |
There was a problem hiding this comment.
| name: "create-validate-tier", | |
| name: "create-tier-pass", |
| operation: admv1.Create, | ||
| }, | ||
| { | ||
| name: "delete-tier", |
There was a problem hiding this comment.
| name: "delete-tier", | |
| name: "delete-tier-pass", |
| if actualReason == "" { | ||
| assert.True(t, allowed) |
There was a problem hiding this comment.
The check doesn't make sense here, it will always be allowed when actualReason is empty, you should provide an expectedReason field in the test structure like other test functions. please add cases to cover failed validation cases.
urharshitha
force-pushed
the
validateTesting
branch
4 times, most recently
from
43bf67c
to
8c2dea3
Compare
urharshitha
force-pushed
the
validateTesting
branch
from
8c2dea3
to
b463e34
Compare
| }, | ||
| }, | ||
| }, | ||
| Tier: "non-existent-tier", |
There was a problem hiding this comment.
This case doesn't make sense. There could many updating tier scenarios, but updating from an non-existing one to an existing one would never happen as the old policy won't be created successfully.
| expectedReason: "At most one of podSelector, externalEntitySelector, serviceReference, ipBlocks or childGroups can be set for a Group", | ||
| }, | ||
| { | ||
| name: "anp-group-set-with-podselector-specified-to-update", |
There was a problem hiding this comment.
hard to understand the case from the name, you probably mean "update anp-group-set-with-podselector-specified"
| name: "anp-group-set-with-podselector-specified-to-update", | ||
| curGroup: &crdv1alpha3.Group{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "anp-group-set-with-podselector-specified-to-update", |
There was a problem hiding this comment.
another impossible update, object name can never change
| }, | ||
| Spec: crdv1alpha3.GroupSpec{ | ||
| PodSelector: &metav1.LabelSelector{ | ||
| MatchLabels: map[string]string{"foo=": "bar"}, |
There was a problem hiding this comment.
this is an disallowed label key constructed intentionally, I believe other tests use it to fail cases. If the new one passes, there must be something wrong in validateAntreaGroup
There was a problem hiding this comment.
In this case , the old group contains IPBlocks specified which results in error case At most one of podSelector, externalEntitySelector, serviceReference, ipBlocks or childGroups can be set for a Group so in updating the current group doesn't contain IPBlocks , then its passing .
| curTier: &crdv1alpha1.Tier{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "tier-priority-3", | ||
| Namespace: "x", |
There was a problem hiding this comment.
Tier is not a namespace scope resource
| operation: admv1.Create, | ||
| expectedReason: "tier tier-priority-251 priority 251 is reserved", | ||
| }, | ||
| { |
There was a problem hiding this comment.
Tier update is not allowed so no case for update
There was a problem hiding this comment.
- Tier update is allowed for some cases.
- Even if Tier update is not allowed at all, it's controlled by code, and it's the exactly point of unit test, to make sure updating Tier is not allowed.
| expectedReason: "tier tier-priority-251 priority 251 is reserved", | ||
| }, | ||
| { | ||
| name: "delete-tier-pass", |
There was a problem hiding this comment.
I believe there are more delete-tier failure cases worth to test.
There was a problem hiding this comment.
There are three cases in which tier delete is not allowed, there is no much coverage improvement to just add an allowed case.
| } | ||
| } | ||
|
|
||
| func TestValidateMulticastIGMP(t *testing.T) { |
There was a problem hiding this comment.
why creating a separate test just for validateMulticastIGMP? It looks not different from the other validation. Perhaps just add the cases to TestValidateAntreaPolicy.
There was a problem hiding this comment.
@jainpulkit22 Could you check and address a few old comments from @tnqn, I didn't see they are being addressed.
You may check https://github.com/antrea-io/antrea/pull/4629/files to get the leftover comments.
| egressRules: []crdv1alpha1.Rule{ | ||
| { | ||
| Protocols: []crdv1alpha1.NetworkPolicyProtocol{ | ||
| { | ||
| ICMP: &crdv1alpha1.ICMPProtocol{}, | ||
| IGMP: &crdv1alpha1.IGMPProtocol{ | ||
| IGMPType: &report, | ||
| GroupAddress: "225.1.2.3", | ||
| }, | ||
| }, | ||
| }, | ||
| Action: &allowAction, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
nit: no need to repeat the same invalid case in both ingress and egress rules. Use ingress rules for one invalid case and use egress rules for another invalid case so it makes sure both ingress and egress validation work.
| @@ -1434,18 +1485,19 @@ func TestValidateAntreaPolicy(t *testing.T) { | |||
| }, | |||
| }, | |||
| }, | |||
| operation: admv1.Create, | |||
There was a problem hiding this comment.
You don't need this operation field if all values are admv1.Create.
| tests := []struct { | ||
| name string | ||
| featureGates map[featuregate.Feature]bool | ||
| policy *crdv1alpha1.ClusterNetworkPolicy | ||
| oldPolicy *crdv1alpha1.ClusterNetworkPolicy |
There was a problem hiding this comment.
I didn't see this oldPolicy field being used.
jainpulkit22
force-pushed
the
validateTesting
branch
2 times, most recently
from
e1d142d
to
9234b72
Compare
jainpulkit22
force-pushed
the
validateTesting
branch
from
9234b72
to
ef951ef
Compare
There was a problem hiding this comment.
Can we calculate the previous and the current coverage? I doubt it has much improvement. If https://app.codecov.io/gh/antrea-io/antrea/blob/main/pkg/controller/networkpolicy/validate.go and https://app.codecov.io/gh/antrea-io/antrea/pull/4629/blob/pkg/controller/networkpolicy/validate.go#L164 calculates it correctly, it just improves it from 53.41% to 57.25% (the coverage of this file).
| operation: admv1.Create, | ||
| expectedReason: "tier tier-priority-251 priority 251 is reserved", | ||
| }, | ||
| { |
There was a problem hiding this comment.
- Tier update is allowed for some cases.
- Even if Tier update is not allowed at all, it's controlled by code, and it's the exactly point of unit test, to make sure updating Tier is not allowed.
| expectedReason: "tier tier-priority-251 priority 251 is reserved", | ||
| }, | ||
| { | ||
| name: "delete-tier-pass", |
There was a problem hiding this comment.
There are three cases in which tier delete is not allowed, there is no much coverage improvement to just add an allowed case.
| expectedReason: "At most one of podSelector, externalEntitySelector, serviceReference, ipBlocks or childGroups can be set for a Group", | ||
| }, | ||
| { |
There was a problem hiding this comment.
There are quite some disallowed cases that can be validated. Since this PR is dedicated to improve the test coverage, it should increase it with a good percent.
jainpulkit22
force-pushed
the
validateTesting
branch
from
ef951ef
to
66d5eb9
Compare
Signed-off-by: Harshitha U R <harshithaur1611@gmail.com> Signed-off-by: Pulkit Jain <jainpu@vmware.com>
jainpulkit22
force-pushed
the
validateTesting
branch
from
66d5eb9
to
d672df7
Compare
|
/skip-all |
…io#4629) Signed-off-by: Harshitha U R <harshithaur1611@gmail.com> Signed-off-by: Pulkit Jain <jainpu@vmware.com>
Adds unit test for pkg/controller/networkpolicy/validate.go
for #4142
Signed-off-by: Harshitha U R harshithaur1611@gmail.com