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

Jump to bottom

[hubble observe]: Add http header filter #1277

Merged
merged 2 commits into from Dec 1, 2023
Merged

[hubble observe]: Add http header filter #1277

merged 2 commits into from Dec 1, 2023

Conversation

ChrsMark
Copy link
Contributor

@ChrsMark ChrsMark commented Oct 28, 2023 •
edited

We are adding support for filtering on HTTP headers in cilium/cilium#28851. This PR adds this feature in the CLI by adding a new flag --http-header

Fixes: #776

How to test this manually

See cilium/cilium#28851

@ChrsMark ChrsMark requested a review from a team as a code owner October 28, 2023 16:21
@ChrsMark ChrsMark requested review from lambdanis and removed request for a team October 28, 2023 16:21
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label PR is blocked until the release note is set label Oct 28, 2023
@ChrsMark ChrsMark marked this pull request as draft October 28, 2023 16:21
@ChrsMark ChrsMark mentioned this pull request Oct 29, 2023
Merged
8 tasks
@rolinh
Copy link
Member

rolinh commented Nov 24, 2023

@ChrsMark Do you need help with this work?

@ChrsMark
Copy link
Contributor Author

Hey @rolinh , I just wait for a pre-lease version of cilium so as to update the dependency and be able to use the types introduced with cilium/cilium#28851.
If I miss sth here or there is another way to move this forward let me know.

@rolinh
Copy link
Member

rolinh commented Nov 27, 2023

Ah, I see. It's actually fine to upgrade the Cilium dep to whatever is main at the moment. Once Cilium v1.15 is branched (with v1.15.0-rc.0), we'll stick to vendoring the v1.15 branch until the next release of the CLI around Cilium v1.15.0.

@ChrsMark ChrsMark marked this pull request as ready for review November 27, 2023 13:13
@ChrsMark ChrsMark requested a review from a team as a code owner November 27, 2023 13:13
@ChrsMark ChrsMark requested review from tklauser and removed request for a team November 27, 2023 13:13
lambdanis
lambdanis requested changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@lambdanis lambdanis left a comment

Choose a reason for hiding this comment

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

The CI failure looks legit - TestTestHubbleObserve is failing due to different ordering of CLI args.

cmd/observe/flows_filter.go Outdated Show resolved Hide resolved
cmd/observe_help.txt Outdated Show resolved Hide resolved
@lambdanis lambdanis added release-note/minor This PR introduces functionality that users may find relevant to operating Hubble. and removed dont-merge/needs-release-note-label PR is blocked until the release note is set labels Nov 28, 2023
@ChrsMark ChrsMark force-pushed the add_http_header_filter branch 4 times, most recently from bfed0ca to 3e67bff Compare November 28, 2023 14:21
tklauser
tklauser requested changes Nov 29, 2023
View reviewed changes
cmd/observe/flows_filter.go Outdated
Comment on lines 554 to 555
hVal := h[len(h)-1]
key := strings.Join(h[:len(h)-1], ":")
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be the other way around. Everything before the first colon is the key and everything after is the value. I think strings.SplitN as mentioned by Anna would work as intended here. Another easier option might be strings.Cut:

key, hVal, _ := strings.Cut(h, ":")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! I doubled checked and only the values can contain :. Changed that.

tklauser
tklauser requested changes Nov 29, 2023
View reviewed changes
cmd/observe/flows_filter.go Outdated Show resolved Hide resolved
lambdanis
lambdanis approved these changes Nov 30, 2023
View reviewed changes
Copy link
Contributor

@lambdanis lambdanis left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for updates.

One detail: I would split the Cilium update into a separate commit.

@ChrsMark
Copy link
Contributor Author

That vendor update related CI failure looks weird to me 🤔 : https://github.com/cilium/hubble/actions/runs/7049712881/job/19188814888?pr=1277#step:9:71

Is this sth that we should double check?

@tklauser
Copy link
Member

That vendor update related CI failure looks weird to me 🤔 : https://github.com/cilium/hubble/actions/runs/7049712881/job/19188814888?pr=1277#step:9:71

Is this sth that we should double check?

This is an issue that was recently fixed upstream (ref. cilium/cilium#29481). Could you please update the vendored cilium version to latest main (go get github.com/cilium/cilium@main && go mod tidy && go mod vendor) in the commit bumping the dependency? Then this should be good to go!

@ChrsMark ChrsMark force-pushed the add_http_header_filter branch 2 times, most recently from f37b3a2 to cf4b8c6 Compare December 1, 2023 08:44
@ChrsMark
Update cilium version
5f1c6ac 
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
@ChrsMark
[hubble observe]: Add http header filter
cf4b8c6 
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
@ChrsMark
Copy link
Contributor Author

ChrsMark commented Dec 1, 2023

Thank's @tklauser, it should be fine now :)

tklauser
tklauser approved these changes Dec 1, 2023
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Thanks @ChrsMark!

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 1, 2023
@tklauser tklauser merged commit baac2ed into cilium:main Dec 1, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tklauser tklauser tklauser approved these changes

@lambdanis lambdanis lambdanis approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR introduces functionality that users may find relevant to operating Hubble.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for filtering on HTTP headers
4 participants
@ChrsMark @rolinh @tklauser @lambdanis