Skip to content

Conversation

ChrsMark
Copy link
Contributor

@ChrsMark ChrsMark commented Oct 28, 2023

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

Implements the server side changes for cilium/hubble#776

hubble: Add Support for filtering on HTTP headers

How to test this manually

  1. Deploy cilium from source with make kind-image && make kind-install-cilium (note the hubble-export-file-path should be set for some weird reason that is not super clear to me, this is how the code flow works at
    if option.Config.HubbleExportFilePath != "" {
    )
  2. Enable hubble with cilium hubble enable --chart-directory=/home/chrismark/go/src/github.com/cilium/cilium/install/kubernetes/cilium
  3. Apply a L7 example endpoint with kubectl apply -f https://raw.githubusercontent.com/cilium/cilium/1.13.3/examples/minikube/sw_l3_l4_l7_policy.yaml
  4. Port forward hubble relay svc with kubectl port-forward -n kube-system svc/hubble-relay --address 0.0.0.0 --address :: 4245:80
  5. Deploy the sample app with kubectl create -f https://raw.githubusercontent.com/cilium/cilium/1.13.3/examples/minikube/http-sw-app.yaml. From https://docs.cilium.io/en/stable/gettingstarted/demo/#starwars-demo

-> Build hubble CLI from cilium/hubble#1277 after replacing the cilium dependency in the go.mod file with replace github.com/cilium/cilium => /home/chrismark/go/src/github.com/cilium/cilium.

  1. Observe the flows with ./hubble observe --pod tiefighter --http-header Foo:bar -f
  2. Execute a matching curl request with kubectl exec tiefighter -- curl -H "Foo: bar" -s -XPOST "http://deathstar.default.svc.cluster.local/v1/request-landing"
  3. Verify that the flow is observed:
Oct 29 19:46:06.337: default/tiefighter:39348 (ID:49781) -> default/deathstar-7848d6c4d5-9z2wr:80 (ID:43688) http-request FORWARDED (HTTP/1.1 POST http://deathstar.default.svc.cluster.local/v1/request-landing)
  1. Execute a non-matching curl request with kubectl exec tiefighter -- curl -H "Foo: baz" -s -XPOST "http://deathstar.default.svc.cluster.local/v1/request-landing"
  2. Verify that the flow is not observed.

@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 Oct 28, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Oct 28, 2023
@ChrsMark ChrsMark force-pushed the http-headers-filter branch 7 times, most recently from 7397e9a to 5e78f23 Compare October 28, 2023 15:52
@ChrsMark ChrsMark marked this pull request as ready for review October 28, 2023 15:53
@ChrsMark ChrsMark requested review from a team as code owners October 28, 2023 15:53
@ChrsMark ChrsMark requested review from a user and gandro October 28, 2023 15:53
Copy link
Member

@gandro gandro 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!

@gandro
Copy link
Member

gandro commented Oct 30, 2023

/test

@dylandreimerink dylandreimerink added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Oct 30, 2023
@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 Oct 30, 2023
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thank you for tackling this @ChrsMark. The error msg needs to be updated, but otherwise looks good to me.

@ChrsMark ChrsMark force-pushed the http-headers-filter branch from 5e78f23 to 7e40874 Compare October 30, 2023 16:06
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

lgtm

@ghost
Copy link

ghost commented Oct 30, 2023

/test

@ghost ghost added the sig/hubble label Oct 30, 2023
@ChrsMark
Copy link
Contributor Author

ChrsMark commented Nov 2, 2023

Hey @lambdanis @gandro ! Anything else missing here?

@gandro
Copy link
Member

gandro commented Nov 2, 2023

Required pipeline ci-runtime seems to have timed out. I'm going to restart it.

@squeed
Copy link
Contributor

squeed commented Nov 8, 2023

@ChrsMark the failing job has has its timeout bumped. I'm rebasing your PR and hopefully it all passes :-)

Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Co-authored-by: rohithsai1904 <rohithsai1904@gmail.com>
@squeed squeed force-pushed the http-headers-filter branch from 7e40874 to 12de0eb Compare November 8, 2023 11:04
@gandro
Copy link
Member

gandro commented Nov 8, 2023

/test

@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 Nov 8, 2023
@squeed squeed merged commit f0eed15 into cilium:main Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants