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.

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

Always include Pod labels in FlowAggregator IPFIX template #6418

Conversation

antoninbas
Copy link
Contributor

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

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 requested review from tnqn, yuntanghsu and heanlan June 6, 2024 19:40
heanlan
heanlan previously approved these changes Jun 7, 2024
Copy link
Contributor

@heanlan heanlan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@antoninbas
Copy link
Contributor Author

/test-all

yuntanghsu
yuntanghsu previously approved these changes Jun 7, 2024
Copy link
Contributor

@yuntanghsu yuntanghsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one nit

err := fa.sendFlowKeyRecord(tc.flowKey, tc.flowRecord)
assert.NoError(t, err, "Error in sending flow key record: %v, key: %v, record: %v", err, tc.flowKey, tc.flowRecord)
err := fa.sendFlowKeyRecord(tc.flowKey, flowRecord)
assert.NoError(t, err, "Error in sending flow key record: %v, key: %v, record: %v", err, tc.flowKey, flowRecord)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert.NoError(t, err, "Error in sending flow key record: %v, key: %v, record: %v", err, tc.flowKey, flowRecord)
assert.NoError(t, err, "Error when sending flow key record: %v, key: %v, record: %v", err, tc.flowKey, flowRecord)

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
@antoninbas antoninbas dismissed stale reviews from yuntanghsu and heanlan via ea22cac June 7, 2024 21:52
@antoninbas antoninbas requested a review from yuntanghsu June 7, 2024 22:04
@antoninbas
Copy link
Contributor Author

/test-all

@antoninbas antoninbas merged commit 76bb211 into antrea-io:main Jun 10, 2024
52 of 55 checks passed
@antoninbas antoninbas deleted the always-include-pod-labels-in-ipfix-template branch June 10, 2024 20:16
@antoninbas antoninbas added action/release-note Indicates a PR that should be included in release notes. area/flow-visibility/aggregator Issues or PRs related to Flow Aggregator labels Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/release-note Indicates a PR that should be included in release notes. area/flow-visibility/aggregator Issues or PRs related to Flow Aggregator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Always include podLabels in FlowAggregator IPFIX template
3 participants