-
Notifications
You must be signed in to change notification settings - Fork 376
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
oci-hook: allow users to set a list of namespace exceptions and define default #2404
Conversation
I think it makes sense to add @kkourt as a reviewer as he knows the most about the OCI hook implementation at this point. |
fc9726f
to
e523052
Compare
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM.
I do have a small suggestion, but I'm pre-approving since it's a trivial change.
# -- Comma-separated list of namespaces to allow Pod creation for, in case tetragon-oci-hook fails to reach Tetragon agent. | ||
failAllowNamespaces: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we document that if this is left empty, the namespace where tetragon is installed will be used by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved via 2424f81
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm changing my review to "Request changes", because of my later comment. It would be nice to have a way to always include the tetragon namespace in the failed namespaces list because otherwise Tetragon will not be able to start.
Thanks for the review! As per your last comment this is already accounted for, see this comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
One final request: Can you squash the changes from my feedback into a single commit? ( |
Prior to this commit `kube-system` was always added as an exception in order for Pods in that namespace to be created in the event that the OCI hook was not able to reach the Tetragon agent. This leads to a deadlock scenario when Tetragon itself is not installed in `kube-system` as the agent won't start and leave the cluster in an unhealthy state. Instead of always adding `kube-syste` this change will always add the namespace Tetragon is deployed in, to guarantee that the agent can always start. In addition to that a user can now define additional namespaces as further exceptions. For example to ensure Pods in business-critical namespaces can still be created even if the OCI hook fails to reach the Tetragon agent. Signed-off-by: Filip Nikolic <oss.filipn@gmail.com>
Done 👍 |
Prior to this commit
kube-system
was always added as an exception in order for Pods in that namespace to be created in the event that the OCI hook was not able to reach the Tetragon agent. This leads to a deadlock scenario when Tetragon itself is not installed inkube-system
.Instead of always adding
kube-syste
this change will always add the namespace Tetragon is deployed in. In addition to that a user can now define additional namespaces as further exception. For example to ensure Pods in business-critical namespaces can still be created even if the OCI hook fails to reach the Tetragon agent.Install Tetragon in a namespace other than
kube-system
:Fixes #2402
Install helm with the new
failAllowNamespaces
values:Uninstall Tetragon to trigger OCI hook error:
Create various namespaces:
Create deployments in namespaces:
As can be seen Pods in
business-critical-a
andbusiness-critical-b
were created and are Running whereasbusiness-critical-c
wasn't part of the list and therefore continues to fail:Fixes #2403