-
Notifications
You must be signed in to change notification settings - Fork 359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Action to NetworkPolicy Evaluation response #6112
Conversation
} | ||
assert.Equal(t, []string{"testName", "ns", "K8sNetworkPolicy", "10", "In"}, test.GetTableRow(32)) | ||
assert.Equal(t, []string{"testName", "ns", "K8sNetworkPolicy", "10", "In", "Allow"}, test.GetTableRow(32)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite misleading UT. K8s NP should not have action. We should use a testcase to test action "" and another at least to test outputting a real action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, but this also reminds me that for the manually inserted K8s default isolation rules, it's better to assign "Drop" to them for identification? Or perhaps an Index of max int is enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add "isolate"
c54831e
to
a317d3a
Compare
pkg/apis/crd/v1beta1/types.go
Outdated
// RuleActionIsolation indicates that the traffic matching the rule should be | ||
// affected by Kubernetes NetworkPolicy default isolation. | ||
RuleActionIsolation RuleAction = "Isolation" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to understand this. Is this a real action or for other purpose? I feel it sounds a bit confusing. The existing actions are all verbs clearly stating what action will be taken, while "Isolation" describes a state. And I don't get its difference from "Pass".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I did not mean this should be a real action. And I agree that it does not make sense to be added to the CRD action type here. @qiyueyao what we want is simply a "isolate" string in the networkpolicyevaluation
result if it is an isolation rule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, one question: if we want this "isolation" also in the API response, or just in the antctl response?
Currently added a step to identify default isolation rules in antctl processing, let me know if we also want it in API response (the place adding a real action, but a string instead of action constant).
Discussed offline: we don't want to add a fake action to API response, just process the response in antctl.
658e7e0
to
df954e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@tnqn @antoninbas Do you have additional comments for setting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I am fine with Allow / Isolate for K8s NPs
if r.Response.Rule.Action != nil { | ||
action = string(*r.Response.Rule.Action) | ||
} else if r.Response.RuleIndex == math.MaxInt32 { | ||
action = "Isolation" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @tnqn suggested Isolate
for this one (to stick to a verb), which I also prefer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I misunderstood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to "Isolate"
} | ||
|
||
func (r EvaluationResponse) GetTableRow(_ int) []string { | ||
if r.NetworkPolicyEvaluation != nil && r.Response != nil { | ||
action := "Allow" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add a quick comment here explaining that this handling is to account for K8s NPs (r.Response.Rule.Action == nil
corresponds to K8s NP "allow" and r.Response.RuleIndex == math.MaxInt32
corresponds to a drop action because of the "default isolation" model).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
As necessary for networkpolicyevaluation consumption, this PR adds the Action field to the above antctl command response. Signed-off-by: Qiyue Yao <yaoq@vmware.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after s/Isolation/Isolate/
/test-all |
As necessary for
networkpolicyevaluation
consumption, this PR adds theAction
field to the aboveantctl
command response.