-
Notifications
You must be signed in to change notification settings - Fork 366
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 NetworkPolicy rule name in Traceflow observation #5667
Conversation
4621040
to
b403390
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. Could you update e2e cases to verify this section?
@@ -1135,6 +1135,8 @@ type Observation struct { | |||
DstMAC string `json:"dstMAC,omitempty" yaml:"dstMAC,omitempty"` | |||
// NetworkPolicy is the combination of Namespace and NetworkPolicyName. | |||
NetworkPolicy string `json:"networkPolicy,omitempty" yaml:"networkPolicy,omitempty"` | |||
// NetworkPolicyRule is the name of an ingress or an egress rule in NetworkPolicy. | |||
NetworkPolicyRule string `json:"networkPolicyRule,omitempty" yaml:"networkPolicyRule,omitempty"` |
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.
Since we are introducing new attribute NetworkPolicyRule in "v1beta1" but not in "v1alpha1," it's essential to provide clear documentation about this difference to avoid confusion. Users of "v1alpha1" should be aware that the attribute is not available in that version, and they may need to upgrade their CRDs to "v1beta1" if they want to use the new feature.
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'll make it clear in the release notes. Is there any other documentation that you suggest updating?
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.
Upstream side, updating release note is required.
b403390
to
b22e485
Compare
updated, PTAL. |
b22e485
to
3cb61fd
Compare
3cb61fd
to
f39adb9
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 overall, one nit.
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.
Some small comments, otherwise LGTM
It would have been better to separate 1) the e2e tests changes to replace usage of v1alpha1
with v1beta1
, and 2) the addition of the new field to Traceflow (i.e., use 2 different PRs). I don't think there is a strong need to split that PR now, but something to keep in mind in the future.
Class: openflow15.OXM_CLASS_PACKET_REGS, | ||
Field: uint8(openflow.TFEgressConjIDField.GetRegID() / 2), | ||
Value: &openflow15.ByteArrayField{ | ||
Data: xreg0, |
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.
It's confusing to use xreg0
as the data source here. The name xreg0
comes from the fact that we use it (above) to set the value of the openflow15.NXM_NX_REG0
register.
I suggest that you use a new byte array for the conj ID registers. Maybe conjData
or some variable like that.
Name: "acnp-1", | ||
}, | ||
) | ||
npQuerier.EXPECT().GetRuleByFlowID(gomock.Any()).Return( |
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.
nit: I think that it is pretty easy to know the expected ID here, given that we craft the register data ourselves?
applies to expectations below as well
@@ -1135,6 +1135,8 @@ type Observation struct { | |||
DstMAC string `json:"dstMAC,omitempty" yaml:"dstMAC,omitempty"` | |||
// NetworkPolicy is the combination of Namespace and NetworkPolicyName. | |||
NetworkPolicy string `json:"networkPolicy,omitempty" yaml:"networkPolicy,omitempty"` | |||
// NetworkPolicyRule is the name of an ingress or an egress rule in NetworkPolicy. | |||
NetworkPolicyRule string `json:"networkPolicyRule,omitempty" yaml:"networkPolicyRule,omitempty"` |
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'll make it clear in the release notes. Is there any other documentation that you suggest updating?
test/e2e/traceflow_test.go
Outdated
Component: v1beta1.ComponentNetworkPolicy, | ||
ComponentInfo: "IngressMetric", | ||
Action: v1beta1.ActionDropped, | ||
NetworkPolicy: "AntreaNetworkPolicy:testtraceflow-.{8}/test-annp-deny-ingress", |
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.
Do we really need a regex here? We know what the name of the Namespace is by the time we declare the test cases.
test/e2e/traceflow_test.go
Outdated
ComponentInfo: "IngressMetric", | ||
Action: v1beta1.ActionDropped, | ||
NetworkPolicy: "AntreaNetworkPolicy:testtraceflow-.{8}/test-annp-deny-ingress", | ||
NetworkPolicyRule: "ingress-drop-.{7}", |
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.
A similar comment here: we can use hardcoded rule names when we create the policies, and use that name here. We don't need to let Antrea auto-generate a rule name when testing Traceflow.
f39adb9
to
f14e6b5
Compare
Update Traceflow e2e tests to verify "NetworkPolicy" and "NetworkPolicyRule" in Traceflow observation. Signed-off-by: Kumar Atish <atish.iaf@gmail.com>
f14e6b5
to
d2cc4a5
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
/test-all |
/test-conformance |
Add NetworkPolicy rule name in Traceflow observation.
Update Traceflow e2e tests to verify "NetworkPolicy" and "NetworkPolicyRule"
in Traceflow observation.
Signed-off-by: Kumar Atish atish.iaf@gmail.com