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

hubble/recorder: Sanitize pcap filename #18612

Merged
merged 1 commit into from
Jan 26, 2022

Conversation

gandro
Copy link
Member

@gandro gandro commented Jan 25, 2022

This removes any special characters from the generated pcap filename.

This fixes a bug where we accidentally added a slash to the filename
when we added support for the clustername. This broke file creation, as
file names cannot contain slashes.

Fixes: 3203df9 ("hubble: Hubble node_name field should contain cluster name")

Signed-off-by: Sebastian Wicki sebastian@isovalent.com

@gandro gandro added release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.11 labels Jan 25, 2022
@gandro gandro requested review from a team and jrfastab January 25, 2022 13:59
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.11.2 Jan 25, 2022
@rolinh rolinh changed the title hubble/recorder: Sanatize pcap filename hubble/recorder: Sanitize pcap filename Jan 25, 2022
This removes any special characters from the generated pcap filename.

This fixes a bug where we accidentally added a slash to the filename
when we added support for the cluster name. This broke file creation, as
file names cannot contain slashes.

Fixes: 3203df9 ("hubble: Hubble node_name field should contain cluster name")

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
@gandro gandro force-pushed the pr/gandro/fix-pcap-recorder-slashes branch from ff4926f to a25a479 Compare January 25, 2022 15:06
@gandro
Copy link
Member Author

gandro commented Jan 25, 2022

/test

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sChaosTest Connectivity demo application Endpoint can still connect while Cilium is not running

Failure Output

FAIL: Cannot ping from "testclient-s2d65" to "10.132.2.119"

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-1.23-kernel-net-next' has 2 failures but they might be new flakes since it also hit 1 known flakes: #18566 (96.68)

@borkmann
Copy link
Member

/test-1.23-net-next

@gandro
Copy link
Member Author

gandro commented Jan 26, 2022

GKE test failure (linked above) is unrelated, as the code modified by this PR is not active in that test suite at all and doesn't affect connectivity either. Marking ready to merge.

@gandro gandro added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 26, 2022
@borkmann borkmann merged commit 4baf2da into cilium:master Jan 26, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.11.2 Jan 31, 2022
@joamaki joamaki added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Feb 8, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.11 in 1.11.2 Feb 8, 2022
@gandro
Copy link
Member Author

gandro commented Mar 11, 2022

Marking for backport to v1.10 too. Sanitizing the file path is something we should always do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done/1.11 The backport for Cilium 1.11.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
No open projects
1.11.2
Backport done to v1.11
Development

Successfully merging this pull request may close these issues.

None yet

7 participants