[Followup] Fix action field in NetworkPolicyEvaluation for KNP #6217
Conversation
| @@ -190,7 +190,7 @@ func (r EvaluationResponse) GetTableRow(_ int) []string { | |||
| // Handle K8s NPs empty action field. "Allow" corresponds to a K8s NP | |||
| // allow action, and "Isolate" corresponds to a drop action because of the | |||
| // default isolation model. Otherwise, display the action field content. | |||
| action := "Allow" | |||
| action := "" | |||
There was a problem hiding this comment.
I don't get why it shouldn't be assigned with "Allow". Aren't all K8s explicit rules "allow" rules?
There was a problem hiding this comment.
I should have updated the comment as well.
For now, K8s explicit rules will go to the Action != nil case with their default allow action, and manual inserted isolation rules will go to math.MaxInt32. Based on current implementation, the only case left over is Action = nil, (which will never happen as long as Antrea controller is functioning, so it's not default value for K8s explicit rules), so perhaps defaulting to "" is more suitable.
There was a problem hiding this comment.
For the sake of our understanding, could you point to the code that ensures that the action is set to Allow K8s NP explicit rules? Do we have a unit test that validates this behavior server-side?
There was a problem hiding this comment.
The action for K8s rules are set in controller networkpolicy, then controller endpoint querier and evaluation. Also after discussion, we thought action field should not be set for isolation since it was not an actual rule (being changed in this PR).
It is covered by networkpolicy UT, but not endpoint_querier UT (changing in this PR)
| if r.Response.Rule.Action != nil { | ||
| action = string(*r.Response.Rule.Action) | ||
| } else if r.Response.RuleIndex == math.MaxInt32 { | ||
| // Handle empty action field from endpoint query, "Isolate" corresponds | ||
| // to a drop action because of the default isolation model of K8s NPs. | ||
| action = "Isolate" | ||
| } |
There was a problem hiding this comment.
what happens if r.Response.Rule.Action == nil and r.Response.RuleIndex != math.MaxInt32? If this is not a possible case, we should clarify why. Also, if this is not a possible case, I don't see the point of setting RuleIndex to math.MaxInt32 in the first place for isolate rules.
There was a problem hiding this comment.
Yes, that case will not happen as long as Antrea controller endpoint querier is responding correctly. I updated the comment.
I believe the intention of using math.MaxInt32 was because isolation is a manually constructed rule in querier, and does not correspond to any original rule, so math.MaxInt32 will not mess with rule index and give lowest priority in comparison.
| @@ -182,7 +182,7 @@ func TestEvaluationResponseTransform(t *testing.T) { | |||
| assert.Equal(t, []string{"NAME", "NAMESPACE", "POLICY-TYPE", "RULE-INDEX", "DIRECTION", "ACTION"}, test.GetTableHeader()) | |||
| assert.False(t, test.SortRows()) | |||
| assert.Equal(t, []string{"", "", "", "", "", ""}, test.GetTableRow(32)) | |||
| testDropAction := crdv1beta1.RuleActionDrop | |||
| testDropAction, testDefaultAction := crdv1beta1.RuleActionDrop, crdv1beta1.RuleActionAllow | |||
There was a problem hiding this comment.
why is the name of the variable testDefaultAction and not testAllowAction? which default behavior are we referring to?
There was a problem hiding this comment.
I was referring to defaultAction = secv1beta1.RuleActionAllow assigned to K8s NP but I've changed to testAllowAction.
| @@ -190,7 +190,7 @@ func (r EvaluationResponse) GetTableRow(_ int) []string { | |||
| // Handle K8s NPs empty action field. "Allow" corresponds to a K8s NP | |||
| // allow action, and "Isolate" corresponds to a drop action because of the | |||
| // default isolation model. Otherwise, display the action field content. | |||
| action := "Allow" | |||
| action := "" | |||
There was a problem hiding this comment.
For the sake of our understanding, could you point to the code that ensures that the action is set to Allow K8s NP explicit rules? Do we have a unit test that validates this behavior server-side?
|
@luolanzone This needs to go in v2.0, since the feature (NetworkPolicyEvaluation) is new in this release, and it is a bug fix. No need for a CHANGELOG entry however. |
When processing Kubernetes NetworkPolicy, a default action Allow is assigned in Antrea controller. This was not handled corretly in NetworkPolicyEvaluation action field, for default isolation model. As it will undeterminately display action. This PR fixes the issue, by indicating the rule as default isolation with nil action field, and then assigning Isolate to the response in NetworkPolicyEvaluation. Signed-off-by: Qiyue Yao <[email protected]>
|
/test-all |
When processing Kubernetes NetworkPolicy, a default action Allow is assigned in Antrea controller. This was not handled in NetworkPolicyEvaluation action field. This PR fixes it to differentiate from Isolate value in specific case of default isolation model.