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

Correlate flows with CiliumNetworkPolicies #27854

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

chancez
Copy link
Contributor

@chancez chancez commented Aug 31, 2023

Using GetRealizedPolicyRuleLabelsForKey, lookup policy labels for a flow using the: direction, endpointIP, remoteIdentity, protocool and destination port. After getting the labels, reconstruct the policy name and namespace using the well-known policy labels.

Relates to cilium/hubble#1100
Closes #26438.

Reviewers note: The protobuf field numbers may look arbitrary, however, this is a port of a feature we maintained internally, so for compatibility, it would be ideal to reuse the existing protobuf field names and numbers.

Correlate flows with CiliumNetworkPolicies

@chancez chancez added kind/feature This introduces new functionality. area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. sig/hubble Impacts hubble server or relay labels Aug 31, 2023
@chancez chancez self-assigned this Aug 31, 2023
@chancez chancez requested review from a team as code owners August 31, 2023 16:08
@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 Aug 31, 2023
@chancez chancez requested a review from gandro August 31, 2023 16:16
@michi-covalent
Copy link
Contributor

/test

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

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

lgtm for the files owned by hubble team, just some minor suggestions / questions.

i'll add do-not-merge label. i'd like to get this reviewed by @gandro 🙏

pkg/hubble/parser/seven/parser.go Outdated Show resolved Hide resolved
@michi-covalent michi-covalent added the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Aug 31, 2023
@chancez chancez force-pushed the pr/chancez/hubble_policy_metadata_oss branch from bac754b to 0aee000 Compare August 31, 2023 22:39
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.

Overall looks good to me, thanks! One note regarding the L7 parser invocation

pkg/hubble/parser/seven/parser.go Outdated Show resolved Hide resolved
pkg/hubble/parser/seven/parser.go Outdated Show resolved Hide resolved
@chancez chancez force-pushed the pr/chancez/hubble_policy_metadata_oss branch from 0aee000 to f060504 Compare September 5, 2023 16:04
@chancez chancez requested a review from gandro September 5, 2023 16:05
@chancez
Copy link
Contributor Author

chancez commented Sep 5, 2023

/test

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.

Neat, thanks!

@michi-covalent michi-covalent removed the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Sep 6, 2023
Copy link
Member

@nathanjsweet nathanjsweet left a comment

Choose a reason for hiding this comment

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

A couple of nits, but otherwise this looks good.

api/v1/flow/flow.proto Show resolved Hide resolved
pkg/policy/correlation/correlation.go Show resolved Hide resolved
@chancez chancez force-pushed the pr/chancez/hubble_policy_metadata_oss branch from f060504 to 438e764 Compare September 7, 2023 16:02
Using GetRealizedPolicyRuleLabelsForKey, lookup policy labels for a flow
using the: direction, endpointIP, remoteIdentity, protocool and destination port.
After getting the labels, reconstruct the policy name and namespace
using the well-known policy labels.

Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez chancez force-pushed the pr/chancez/hubble_policy_metadata_oss branch from 438e764 to cd9b4d4 Compare September 7, 2023 16:06
@chancez
Copy link
Contributor Author

chancez commented Sep 7, 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 Sep 7, 2023
@youngnick youngnick merged commit 6fcad13 into main Sep 8, 2023
213 checks passed
@youngnick youngnick deleted the pr/chancez/hubble_policy_metadata_oss branch September 8, 2023 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. kind/feature This introduces new functionality. 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. sig/hubble Impacts hubble server or relay
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CFP: Correlate policy verdicts to network policies
6 participants