Add an option to disable L3/L4 network policy correlation of flows by mereta · Pull Request #37986 · cilium/cilium

mereta · 2025-03-04T14:43:51Z

Adding a config option to disable L3/L4 network policy correlation of Hubble flows.
This is enabled by default, only setting the flag to 'false' will cause a change in behavior.
the new flag can be set as such:
--set hubble.networkpolicycorrelation.enabled=false
or set in values.yaml :
.Values.hubble.networkpolicycorrelation.enabled

Fixes: #37528

Add an option to disable L3/L4 network policy correlation of Hubble flows

Manual verification in a cluster:

build feature images & update the cluster
edit config map cilium-config to add:
- hubble-network-policy-correlation-enabled: "false"
or helm upgrade with --set hubble.networkpolicycorrelation.enabled=false
restart cilium & hubble-relay pods
use node-shell to the cilium pod
- navigate to var/run/cilium/hubble
- check flows in events.log
- When flag is enabled the flow object will contain : "egress_allowed_by"/ "ingress_allowed_by" property with an array of evaluated policies.
- When the flag is disabled the flow object will not contain the above properties

maintainer-s-little-helper · 2025-03-04T14:43:53Z

Commit 070b3ad does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2025-03-04T14:48:35Z

Commits 070b3ad, 3f79446 do not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2025-03-04T15:19:05Z

Commit 4bb9c90 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2025-03-04T15:35:27Z

Commit ed6eac4 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2025-03-04T15:36:09Z

Commit ed6eac4 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

anubhabMajumdar

Can you add details about manual verification in a cluster? Thanks!

pkg/hubble/parser/threefour/parser.go

pkg/hubble/parser/options/options.go

pkg/hubble/parser/threefour/parser.go

pkg/hubble/parser/options/options.go

pkg/hubble/parser/threefour/parser_test.go

pkg/hubble/parser/threefour/parser.go

joestringer · 2025-03-05T17:50:16Z

Thanks for the PR! Small mechanical suggestion just to help reviewers: Could you rebase + squash all the commits back into the top one? We often review commit-by-commit in the Cilium project and we don't admit merge commits into the tree (to ensure every commit compiles and simplify bisecting bugs in future).

mereta · 2025-03-12T16:54:56Z

Thanks for the quick update @mereta 👍

Again, a couple of non-blocking changes request. If you do update the Redact() log line, then please rename it to WithReact() on the way (both in a separate commit) 🙏

Thanks for the comments. Updated the PR in a separate commit :)

kaworu

Almost there, I think this call has been missed in the renaming of WithRedact() so the build should fail.

On the way s/perser/parser/ in the second commit title, and prefix both commit titles with hubble: 🙏

maintainer-s-little-helper · 2025-03-12T17:12:44Z

Commit fe83438 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2025-03-12T17:13:27Z

Commit fe83438 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

mereta · 2025-03-12T17:16:31Z

Almost there, I think this call has been missed in the renaming of WithRedact() so the build should fail.

On the way s/perser/parser/ in the second commit title, and prefix both commit titles with hubble: 🙏

I need to remember to 'save all' 😅 Updated the commits now.

youngnick

👍 from sig-k8s

install/kubernetes/cilium/values.yaml.tmpl

gandro

In terms of implementation, this looks good to me. I would recommend rewording the user exposed flag a bit though.

install/kubernetes/cilium/values.yaml.tmpl

gandro

Awesome, thank you!

mereta · 2025-03-13T14:39:47Z

Almost there, I think this call has been missed in the renaming of WithRedact() so the build should fail.

On the way s/perser/parser/ in the second commit title, and prefix both commit titles with hubble: 🙏

@kaworu I made the changes in a separate commit now. Could you check :)

kaworu

Thanks @mereta the patch LGTM now! 🎉

kaworu · 2025-03-17T11:32:32Z

/test

gandro · 2025-03-19T16:19:27Z

Re-running tests. Please avoid from rebasing unless strictly needed, as it invalidates CI results. We can restart tests without having to rebase.

gandro · 2025-03-19T16:19:34Z

/test

mereta · 2025-03-19T16:50:21Z

Re-running tests. Please avoid from rebasing unless strictly needed, as it invalidates CI results. We can restart tests without having to rebase.

Gotcha. Is there anything I need to do now?

joestringer · 2025-03-19T19:25:33Z

The smoke tests are marked as a "Required" job to merge and there is a failure there that looks related to your PR: https://github.com/cilium/cilium/actions/runs/13951380856/job/39052000550?pr=37986#step:3:119

I would also suggest rebasing against the tip of main as I see one of the other jobs failed due to a temporary breakage earlier today.

squeed

For consistency's sake, we call this "policy correlation" rather than enrichment. Can you adjust the variable and flag naming accordingly?

mereta · 2025-03-21T10:53:10Z

For consistency's sake, we call this "policy correlation" rather than enrichment. Can you adjust the variable and flag naming accordingly?

Hey! Thanks for the review! I have updated the naming throughout :)

squeed · 2025-03-24T11:30:33Z

Looks good (though it needs a rebase). When things look good, comment with /test to launch the full test suite.

Signed-off-by: mereta <[email protected]>

mereta · 2025-03-24T12:11:54Z

Looks good (though it needs a rebase). When things look good, comment with /test to launch the full test suite.

Rebased. Will start full test suite when images build successfully. Thanks @squeed! ☘️

mereta · 2025-03-24T12:59:49Z

/test

dylandreimerink · 2025-03-24T14:12:22Z

/test

anubhabMajumdar · 2025-03-24T16:55:27Z

/test

rectified95 · 2025-03-24T18:11:34Z

/ci-gateway-api

joestringer · 2025-03-24T20:54:35Z

Merged thanks! @mereta I see that the test framework did not respond to your request to run the tests. Only organization members can do this, and we do this to protect the infrastructure against abuse from the wider GitHub community. If you continue to contribute to Cilium, you might consider becoming an organization member to gain this privilege. For now we'd be happy to help you by triggering the tests.

maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Mar 4, 2025

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Mar 4, 2025

mereta force-pushed the pr/l3l4-netpol-enrich-flag branch from 3f79446 to ae35264 Compare March 4, 2025 15:14

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 4, 2025

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 4, 2025

mereta force-pushed the pr/l3l4-netpol-enrich-flag branch from 4bb9c90 to e9f6753 Compare March 4, 2025 15:20

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 4, 2025

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 4, 2025

mereta force-pushed the pr/l3l4-netpol-enrich-flag branch from ed6eac4 to f0eeedf Compare March 4, 2025 15:36

mereta force-pushed the pr/l3l4-netpol-enrich-flag branch from f0eeedf to b9940e2 Compare March 4, 2025 15:36

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 4, 2025

anubhabMajumdar reviewed Mar 4, 2025

View reviewed changes

pkg/hubble/parser/threefour/parser.go Outdated Show resolved Hide resolved

pkg/hubble/parser/options/options.go Outdated Show resolved Hide resolved

pkg/hubble/parser/threefour/parser.go Outdated Show resolved Hide resolved

rectified95 reviewed Mar 4, 2025

View reviewed changes

pkg/hubble/parser/options/options.go Outdated Show resolved Hide resolved

rectified95 reviewed Mar 4, 2025

View reviewed changes

pkg/hubble/parser/threefour/parser_test.go Outdated Show resolved Hide resolved

rectified95 reviewed Mar 4, 2025

View reviewed changes

pkg/hubble/parser/threefour/parser.go Show resolved Hide resolved

mereta marked this pull request as ready for review March 5, 2025 15:26

mereta requested review from a team as code owners March 5, 2025 15:26

mereta requested review from kaworu, squeed and youngnick March 5, 2025 15:26

joestringer added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Mar 5, 2025

kaworu requested changes Mar 12, 2025

View reviewed changes

youngnick approved these changes Mar 13, 2025

View reviewed changes

gandro reviewed Mar 13, 2025

View reviewed changes

install/kubernetes/cilium/values.yaml.tmpl Outdated Show resolved Hide resolved

gandro reviewed Mar 13, 2025

View reviewed changes

install/kubernetes/cilium/values.yaml.tmpl Outdated Show resolved Hide resolved

gandro approved these changes Mar 13, 2025

View reviewed changes

kaworu approved these changes Mar 17, 2025

View reviewed changes

squeed requested changes Mar 19, 2025

View reviewed changes

kaworu mentioned this pull request Mar 21, 2025

feat(hubble): decouple the payloadparser from hubble control plane. #38368

Merged

8 tasks

kaworu approved these changes Mar 21, 2025

View reviewed changes

squeed approved these changes Mar 24, 2025

View reviewed changes

mereta added 2 commits March 24, 2025 12:08

hubble: Add an option for enabling L3/L4 network policy correlation

1a6db76

Signed-off-by: mereta <[email protected]>

hubble: Rename Hubble Redact options & update parser options test

b1a68d9

Signed-off-by: mereta <[email protected]>

Conversation

mereta commented Mar 4, 2025 • edited by joestringer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maintainer-s-little-helper bot commented Mar 4, 2025

Uh oh!

maintainer-s-little-helper bot commented Mar 4, 2025

Uh oh!

maintainer-s-little-helper bot commented Mar 4, 2025

Uh oh!

maintainer-s-little-helper bot commented Mar 4, 2025

Uh oh!

maintainer-s-little-helper bot commented Mar 4, 2025

Uh oh!

anubhabMajumdar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joestringer commented Mar 5, 2025

Uh oh!

mereta commented Mar 12, 2025

Uh oh!

kaworu left a comment

Choose a reason for hiding this comment

Uh oh!

maintainer-s-little-helper bot commented Mar 12, 2025

Uh oh!

maintainer-s-little-helper bot commented Mar 12, 2025

Uh oh!

mereta commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youngnick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gandro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gandro left a comment

Choose a reason for hiding this comment

Uh oh!

mereta commented Mar 13, 2025

Uh oh!

kaworu left a comment

Choose a reason for hiding this comment

Uh oh!

kaworu commented Mar 17, 2025

Uh oh!

gandro commented Mar 19, 2025

Uh oh!

gandro commented Mar 19, 2025

Uh oh!

mereta commented Mar 19, 2025

Uh oh!

joestringer commented Mar 19, 2025

Uh oh!

squeed left a comment

Choose a reason for hiding this comment

Uh oh!

mereta commented Mar 21, 2025

Uh oh!

squeed commented Mar 24, 2025

Uh oh!

mereta commented Mar 24, 2025

Uh oh!

mereta commented Mar 24, 2025

Uh oh!

dylandreimerink commented Mar 24, 2025

Uh oh!

anubhabMajumdar commented Mar 24, 2025

Uh oh!

rectified95 commented Mar 24, 2025

Uh oh!

mereta commented Mar 4, 2025 •

edited by joestringer

Loading

mereta commented Mar 12, 2025 •

edited

Loading