-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Link configuration snippet to allow-snippet-annotations #10456
Conversation
|
✅ Deploy Preview for kubernetes-ingress-nginx ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This issue is currently awaiting triage. If Ingress contributors determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Welcome @StefanLobbenmeierObjego! |
Hi @StefanLobbenmeierObjego. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: StefanLobbenmeierObjego, strongjz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
For some reason the link worked fine in the preview but seems to be broken now in the documentation. Not sure if something else broke it or I messed up the syntax? |
I am guessing it was the extra /, removed it now. Sorry for the hassle. |
if you accept an inofficial answer - here is my take. allow-snippet-annotations is security relevant for multi tenant clusters Basically if you know everyone that changes deployments to the cluster or have an approval process for it, you should be good. If you have random people changing the deployments, then they can change more via the snippet annotation that you might want. Not sure how much we are talking about, but I would expect that they can change the whole config file via this annotation. in other words, it can give you more access to the cluster if you can already deploy and you can get more permissions. But for single tenant cluster if someone can deploy to your cluster that you do not trust you are pwned anyway. That being said, take a look at the snippet annotations you currently have. It might be easy to migrate them away from snippets by replacing it with dedicated annotations. If you have snippets that you cannot migrate away from, I suggest you open an issue with those snippets to get further support. (e.g. in our case we had this snippet
|
The reason we implemented this kubernetes/kubernetes#126811 |
What this PR does / why we need it:
Reduces confusion caused by breaking change in #10393
Types of changes
Which issue/s this PR fixes
How Has This Been Tested?
Checklist: