Skip to content

Improve hubble exporter configuration error handling and logging #35069

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
Merged
merged 6 commits into from
Oct 7, 2024
Merged

Improve hubble exporter configuration error handling and logging #35069

merged 6 commits into from
Oct 7, 2024

Conversation

chancez
Copy link
Contributor

@chancez chancez commented Sep 27, 2024 

Log errors when reloading hubble exporter configuration dynamically and do not attempt to close os.Stdout

@chancez chancez added sig/hubble needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch labels Sep 27, 2024
@chancez chancez self-assigned this Sep 27, 2024
@chancez chancez requested a review from a team as a code owner September 27, 2024 21:03
@chancez chancez requested a review from rolinh September 27, 2024 21:03
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 27, 2024
@chancez chancez force-pushed the pr/chancez/dynamic_exporter_config branch from c32c354 to 2d4e0b5 Compare September 27, 2024 21:22
@chancez chancez requested a review from a team as a code owner September 27, 2024 21:22
@chancez chancez requested a review from christarazi September 27, 2024 21:22
@chancez chancez force-pushed the pr/chancez/dynamic_exporter_config branch from 2d4e0b5 to 9c0ba61 Compare September 27, 2024 21:25
sdickhoven
sdickhoven approved these changes Sep 27, 2024
View reviewed changes
Copy link

@sdickhoven sdickhoven left a comment

Choose a reason for hiding this comment

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

that looks great! thanks!

sdickhoven
sdickhoven reviewed Sep 27, 2024
View reviewed changes
@chancez
Copy link
Contributor Author

chancez commented Sep 27, 2024

I'm noticing a few more possible issues, gonna iterate a bit and do a bit more manual testing as well.

christarazi
christarazi approved these changes Sep 30, 2024
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

ci-structure LGTM

@chancez
Copy link
Contributor Author

chancez commented Sep 30, 2024

I tested my additional changes. Mostly noticed it was possible the config could be unloaded before it validated the new config, the last commit fixes that.

@chancez chancez added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Sep 30, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 30, 2024
@chancez
Copy link
Contributor Author

chancez commented Sep 30, 2024

/test

rolinh
rolinh requested changes Oct 2, 2024
View reviewed changes
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

Changes lgtm overall.

chancez added 6 commits October 2, 2024 14:16
@chancez
hubble/exporter: Replace assertFlowLogConfig with new helper
261a22c 
Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez
hubble/exporter: Update exporter config tests
b57b005 
Add a source_label filter because a OSS user was reporting issues with
filters containing slashes that I can't reproduce.

Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez
hubble/exporter: Log error when reloading config fails
d6686e7 
Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez
hubble/exporter: Do not close os.Stdout
be32f45 
Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez
hubble/exporter: Fix logging exporter options
9f6cddf 
filters.FilterFuncs is an interface and when being logged by logrus
produces an error "Failed to obtain reader, failed to marshal fields to
JSON, json: unsupported type: filters.FilterFunc".

Fix this by storing the original flowpb.FlowFilters in the
exporteroption.Option and store the filters.FilterFuncs as unexported
fields that have accessor methods instead.

Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez
hubble/exporter: Do not update existing exporter if new config is inv…
b740f3f 
…alid

Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez chancez force-pushed the pr/chancez/dynamic_exporter_config branch from d28d0cd to b740f3f Compare October 2, 2024 21:17
@chancez chancez requested a review from rolinh October 2, 2024 21:17
@rolinh
Copy link
Member

rolinh commented Oct 3, 2024

/test

@rolinh rolinh enabled auto-merge October 3, 2024 07:19
@rolinh rolinh added this pull request to the merge queue Oct 7, 2024
Merged via the queue into main with commit 26331e7 Oct 7, 2024
263 of 265 checks passed
@rolinh rolinh deleted the pr/chancez/dynamic_exporter_config branch October 7, 2024 23:19
@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Oct 7, 2024
@giorio94 giorio94 mentioned this pull request Oct 9, 2024
Merged
5 tasks
@giorio94 giorio94 added backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. and removed needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch labels Oct 9, 2024
@chancez chancez mentioned this pull request Oct 11, 2024
Merged
@github-actions github-actions bot added backport-done/1.16 The backport for Cilium 1.16.x for this PR is done. and removed backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. labels Oct 16, 2024
@cilium-release-bot cilium-release-bot bot mentioned this pull request Nov 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rolinh rolinh rolinh approved these changes

@christarazi christarazi christarazi approved these changes

+1 more reviewer

@sdickhoven sdickhoven sdickhoven approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@chancez chancez

Labels
backport-done/1.16 The backport for Cilium 1.16.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chancez @rolinh @christarazi @sdickhoven @giorio94