Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Always include podLabels in FlowAggregator IPFIX template #6386

Closed
antoninbas opened this issue May 31, 2024 · 3 comments · Fixed by #6418
Closed

Always include podLabels in FlowAggregator IPFIX template #6386

antoninbas opened this issue May 31, 2024 · 3 comments · Fixed by #6418
Assignees
@antoninbas
Labels
area/flow-visibility/aggregator Issues or PRs related to Flow Aggregator

Comments

@antoninbas
Copy link
Contributor

antoninbas commented May 31, 2024 •
edited
Loading

Whether it's better to remove the includePodLabels parameter while initiating the IPFIXExporter (just like other Exporters, to have general columns). - from @yuntanghsu

I think this is a good approach. We should always include podLabels in the template. I think there is no ambiguity because we can use "" as the field value when podLabels are not being filled, and "{}" when podLabels are being filled, but the Pod has no labels.

Originally posted by @antoninbas in #6384 (comment)

This should simplify the IPFIXExporter code in the FlowAggregator.
@antoninbas antoninbas added the area/flow-visibility/aggregator Issues or PRs related to Flow Aggregator label May 31, 2024
@antoninbas
Copy link
Contributor Author

cc @yuntanghsu

@antoninbas
Copy link
Contributor Author

@heanlan do you see any issue with this proposal?

@heanlan
Copy link
Contributor

heanlan commented Jun 4, 2024

do you see any issue with this proposal?

No, it looks good to me.

@antoninbas antoninbas self-assigned this Jun 4, 2024
antoninbas added a commit to antoninbas/antrea that referenced this issue Jun 6, 2024
@antoninbas
Always include Pod labels in FlowAggregator IPFIX template
cfe771c 
We always include the Pod labels IEs (for source and destination Pods),
regardless of the value of the recordContents.podLabels configuration
parameter. This simplifies the logic and the IPFIXExporter no longer
needs to be aware of this configuration. There will be a minor size
increase to the IPFIX records exported by the FlowAggregator when
recordContents.podLabels is false, as we will need to include empty
strings in the records for the 2 IEs.

We use an empty string when recordContents.podLabels is false, or when
the endpoint is not a Pod. We use an empty JSON dictionary ("{}"), when
the Pod has no labels.

Fixes antrea-io#6386

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
@antoninbas antoninbas mentioned this issue Jun 6, 2024
Merged
antoninbas added a commit that referenced this issue Jun 10, 2024
@antoninbas
Always include Pod labels in FlowAggregator IPFIX template (#6418)
76bb211 
We always include the Pod labels IEs (for source and destination Pods),
regardless of the value of the recordContents.podLabels configuration
parameter. This simplifies the logic and the IPFIXExporter no longer
needs to be aware of this configuration. There will be a minor size
increase to the IPFIX records exported by the FlowAggregator when
recordContents.podLabels is false, as we will need to include empty
strings in the records for the 2 IEs.

We use an empty string when recordContents.podLabels is false, or when
the endpoint is not a Pod. We use an empty JSON dictionary ("{}"), when
the Pod has no labels.

Fixes #6386

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@antoninbas antoninbas

Labels
area/flow-visibility/aggregator Issues or PRs related to Flow Aggregator
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Always include Pod labels in FlowAggregator IPFIX template antoninbas/antrea
2 participants
@antoninbas @heanlan