-
Notifications
You must be signed in to change notification settings - Fork 364
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 EgressNode field in Traceflow observation #5949
Add EgressNode field in Traceflow observation #5949
Conversation
375cd62
to
a910db0
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.
cc @antoninbas @heanlan @yuntanghsu for change to FlowExporter.
egressNode: | ||
type: string |
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 don't think we need to add this field for v1alpha1 version. We have promoted the version to v1beta1 half a year ago. @tnqn @antoninbas any thoughts?
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 didn't comment because it's harmless anyway. It's just about whether v1alpha1 client will see the added field, no compatibility impact.
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.
removed for v1alpha1.
a910db0
to
aa2bfa9
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.
Changes to the FlowExporter should probably be reverted (the extra return value can be ignored when calling GetEgress
in fillEgressInfo
). If we want to add the Egress Node to the exported IPFIX record, it should be done in a separate PR (note that the current version of this PR doesn't actually update the IPFIX record).
pkg/agent/flowexporter/types.go
Outdated
@@ -86,6 +86,7 @@ type Connection struct { | |||
EgressIP string | |||
AppProtocolName string | |||
HttpVals string | |||
EgressNode string |
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.
If you are not going to add this information to the exported IPFIX record, then I don't think the FlowExporter changes in this PR make sense.
And if you do want to add the information to the record, it should probably be done in a separate PR (a change to go-ipfix is also required), given that this one is specific to Traceflow.
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.
reverted, thanks.
@@ -641,14 +641,15 @@ func (exp *FlowExporter) findFlowType(conn flowexporter.Connection) uint8 { | |||
} | |||
|
|||
func (exp *FlowExporter) fillEgressInfo(conn *flowexporter.Connection) { | |||
egressName, egressIP, err := exp.egressQuerier.GetEgress(conn.SourcePodNamespace, conn.SourcePodName) | |||
egressName, egressIP, egressNode, err := exp.egressQuerier.GetEgress(conn.SourcePodNamespace, conn.SourcePodName) |
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.
To add a new field, please open a PR in go-ipfix repository to update the registry first.
reference PRs for adding egressName and egressIP fields: vmware/go-ipfix#308 #5088
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.
thanks.
Fixes antrea-io#5911 Signed-off-by: Kumar Atish <[email protected]>
aa2bfa9
to
307b586
Compare
/skip-all |
1 similar comment
/skip-all |
Fixes #5911