feat: Add collection key to differentiate namespaced policies #2337

joshuajorel · 2024-04-16T05:16:59Z

Currently, TracingPolicy and TracingPolicyNamespaced encounter conflicts whenever a policy is applied with the same name. A policy with the same name, even if it is in a different namespace does not get applied. This behavior should be allowed given the k8s context e.g. a policy in one namespace is a different policy from one in a different namespace even with the same name.

tetragon: allow namespaced and non-namespaced policies to have the same name

Fixes: #2299

lambdanis

Thank you for tackling this @joshuajorel! I see it's still WIP, but I left a few comments.

pkg/sensors/collection.go

pkg/sensors/handler.go

kkourt

Thanks!

Please find some comments below.

pkg/sensors/manager.go

netlify · 2024-04-19T08:56:49Z

✅ Deploy Preview for tetragon ready!

Name	Link
🔨 Latest commit	`340e184`
🔍 Latest deploy log	https://app.netlify.com/sites/tetragon/deploys/662f842cfaa54900083b5a56
😎 Deploy Preview	https://deploy-preview-2337--tetragon.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

joshuajorel · 2024-04-19T11:28:28Z

@kkourt @lambdanis - what is the usecase for enabling and disabling a Sensor? Since every interaction with Tetragon (as far I can see) goes through a TracingPolicy, where is this functionality used?

kkourt · 2024-04-29T07:02:38Z

@kkourt @lambdanis - what is the usecase for enabling and disabling a Sensor? Since every interaction with Tetragon (as far I can see) goes through a TracingPolicy, where is this functionality used?

Hi,

The sensor interface (and infrastructure) is indeed deprecated. There is an issue to remove it #1272, but we first need to provide means to disable a tracing policy as we do with sensors.

lambdanis · 2024-05-01T11:32:10Z

Hey @joshuajorel thank you for the updates.

Could you rewrite the commits so that changes are clearly grouped and documented? I think one commit introducing collectionKey, one introducing API changes, and one bringing everything together would be good. Or a single commit with all changes would be okay too. Please also keep the commit message titles no longer that 75 characters - otherwise the CI job fails.

lambdanis

Looks good to me, thank you @joshuajorel. Let's wait for @kkourt to confirm nothing is missing.

lambdanis · 2024-05-07T07:56:28Z

pkg/sensors/handler.go

-	if col, exists := h.collections[op.name]; exists && col.state != LoadErrorState {
-		return fmt.Errorf("failed to add tracing policy %s, a sensor collection with the name already exists", op.name)
+	if col, exists := h.collections[op.ck]; exists && col.state != LoadErrorState {
+		return fmt.Errorf("failed to add tracing policy %s, a sensor collection with the name already exists", op.ck)


Suggested change

return fmt.Errorf("failed to add tracing policy %s, a sensor collection with the name already exists", op.ck)

return fmt.Errorf("failed to add tracing policy %s, a sensor collection with this key already exists", op.ck)

or something similar (non-blocking)

Change wording to reference key instead of name

joshuajorel · 2024-05-13T01:47:14Z

Looks good to me, thank you @joshuajorel. Let's wait for @kkourt to confirm nothing is missing.

Thank you for taking the time to review @lambdanis ! Will wait for @kkourt 's review

kkourt

Thanks, changes LGTM. I left one comment about removing the unneeded name field.

One of the things we also need to do is update the CLI: https://github.com/cilium/tetragon/blob/main/cmd/tetra/tracingpolicy/tracingpolicy.go. Could you also do that?

pkg/sensors/manager.go

joshuajorel · 2024-05-13T15:34:54Z

Thanks, changes LGTM. I left one comment about removing the unneeded name field.

One of the things we also need to do is update the CLI: https://github.com/cilium/tetragon/blob/main/cmd/tetra/tracingpolicy/tracingpolicy.go. Could you also do that?

Sure. Let me do the changes and get back to you. Thank you very much!

kkourt · 2024-05-14T06:23:27Z

Thanks, changes LGTM. I left one comment about removing the unneeded name field.
One of the things we also need to do is update the CLI: https://github.com/cilium/tetragon/blob/main/cmd/tetra/tracingpolicy/tracingpolicy.go. Could you also do that?

Sure. Let me do the changes and get back to you. Thank you very much!

Thanks! I would suggest doing the CLI updates in a different patch to ease reviewing.

TracingPolicy and TracingPolicyNamespaced encounter conflicts whenever a policy is applied with the same name. A policy with the same name, even if it is in a different namespace does not get applied. This commit adds a collectionKey to differentiate policies with the same, but in different namespaces. Fixes: cilium#2299 Signed-off-by: Joshua Jorel Lee <joshua.jorel.lee@gmail.com>

cmd/tetra/tracingpolicy/tracingpolicy.go

By adding a collectionKey to differentiate namespaces, the CLI needs to be updated to support this update. This change adds a namespace flag to the CLI for the delete, enable, and disable commands. If the namespace flag is unset, it will reference the global TracingPolicy. Fixes: cilium#2299 Signed-off-by: Joshua Jorel Lee <joshua.jorel.lee@gmail.com>

kkourt

Thanks, LGTM!

kkourt · 2024-05-22T13:49:00Z

Everything's ✅ , so merging this. Many thanks @joshuajorel this is an awesome contribution.

joshuajorel mentioned this pull request Apr 16, 2024

Creating a TracingPolicyNamespaced with the same name for a different namespace does not get applied. #2299

Closed

lambdanis reviewed Apr 16, 2024

View reviewed changes

pkg/sensors/collection.go Outdated Show resolved Hide resolved

pkg/sensors/handler.go Outdated Show resolved Hide resolved

pkg/sensors/handler.go Outdated Show resolved Hide resolved

lambdanis added the release-note/bug This PR fixes an issue in a previous release of Tetragon. label Apr 16, 2024

kkourt requested changes Apr 18, 2024

View reviewed changes

joshuajorel force-pushed the pr/joshuajorel/add-k8s-namespace-segregation-for-policies branch from acd9375 to 340e184 Compare April 29, 2024 11:27

joshuajorel changed the title ~~[WIP] feat: Add collection key to differentiate namespaced policies~~ feat: Add collection key to differentiate namespaced policies Apr 29, 2024

joshuajorel marked this pull request as ready for review April 29, 2024 11:28

joshuajorel requested a review from kkourt April 29, 2024 11:28

joshuajorel requested a review from a team as a code owner April 29, 2024 11:28

lambdanis self-requested a review April 29, 2024 19:31

joshuajorel force-pushed the pr/joshuajorel/add-k8s-namespace-segregation-for-policies branch from 340e184 to 7d9918e Compare May 3, 2024 03:15

lambdanis approved these changes May 11, 2024

View reviewed changes

kkourt requested changes May 13, 2024

View reviewed changes

pkg/sensors/manager.go Show resolved Hide resolved

pkg/sensors/manager.go Show resolved Hide resolved

pkg/sensors/manager.go Show resolved Hide resolved

joshuajorel force-pushed the pr/joshuajorel/add-k8s-namespace-segregation-for-policies branch from 7d9918e to d5641fd Compare May 15, 2024 10:11

mtardy reviewed May 16, 2024

View reviewed changes

cmd/tetra/tracingpolicy/tracingpolicy.go Outdated Show resolved Hide resolved

joshuajorel force-pushed the pr/joshuajorel/add-k8s-namespace-segregation-for-policies branch from 0c7d2b1 to 6ec273f Compare May 20, 2024 09:59

joshuajorel requested a review from kkourt May 21, 2024 05:23

kkourt approved these changes May 21, 2024

View reviewed changes

kkourt merged commit d7619f9 into cilium:main May 22, 2024
41 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add collection key to differentiate namespaced policies #2337

feat: Add collection key to differentiate namespaced policies #2337

joshuajorel commented Apr 16, 2024 •

edited by kkourt

lambdanis left a comment

kkourt left a comment

netlify bot commented Apr 19, 2024 •

edited

joshuajorel commented Apr 19, 2024

kkourt commented Apr 29, 2024

lambdanis commented May 1, 2024

lambdanis left a comment

lambdanis May 7, 2024 •

edited

joshuajorel May 15, 2024

joshuajorel commented May 13, 2024

kkourt left a comment

joshuajorel commented May 13, 2024

kkourt commented May 14, 2024

kkourt left a comment

kkourt commented May 22, 2024

	return fmt.Errorf("failed to add tracing policy %s, a sensor collection with the name already exists", op.ck)
	return fmt.Errorf("failed to add tracing policy %s, a sensor collection with this key already exists", op.ck)

feat: Add collection key to differentiate namespaced policies #2337

feat: Add collection key to differentiate namespaced policies #2337

Conversation

joshuajorel commented Apr 16, 2024 • edited by kkourt

lambdanis left a comment

Choose a reason for hiding this comment

kkourt left a comment

Choose a reason for hiding this comment

netlify bot commented Apr 19, 2024 • edited

✅ Deploy Preview for tetragon ready!

joshuajorel commented Apr 19, 2024

kkourt commented Apr 29, 2024

lambdanis commented May 1, 2024

lambdanis left a comment

Choose a reason for hiding this comment

lambdanis May 7, 2024 • edited

Choose a reason for hiding this comment

joshuajorel May 15, 2024

Choose a reason for hiding this comment

joshuajorel commented May 13, 2024

kkourt left a comment

Choose a reason for hiding this comment

joshuajorel commented May 13, 2024

kkourt commented May 14, 2024

kkourt left a comment

Choose a reason for hiding this comment

kkourt commented May 22, 2024

joshuajorel commented Apr 16, 2024 •

edited by kkourt

netlify bot commented Apr 19, 2024 •

edited

lambdanis May 7, 2024 •

edited