-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Document the best practices when implementing mutating webhook #47465
Comments
/triage accepted |
@jpbetz @shannonxtreme any chance you can help with this some time soon? We are getting reports about the sidecar feature incompatibility from time to time and hope to get the document to point to. |
I'll try to get some attention on this. |
/priority important-longterm due to relevance around sidecar containers turning into init containers |
@shannonxtreme or @jpbetz if you'd like to pair on this (or pair up async), I can try to make time |
Yeah I can pair up. What's the scope here? It sounds like a best practices page, and maybe should include a working example of a "correct" implementation? |
Not sure about scope. We could do a blog article that links to a section within https://kubernetes.io/docs/tutorials/configuration/pod-sidecar-containers/ perhaps? I'll message you via Slack |
Ideally we need something that we can share whenever we see this issue. If some webhook stripped out the field we can say "please upgrade to k8s API 1.28+ so this field will be known and change the design of your webhook to follow the best practices to avoid problems like this going forward". Scope-wise, maybe @jpbetz may also help to share some good practices. |
/assign |
I've written a quick and dirty skeleton for the structure https://docs.google.com/document/d/1pg80Qn3Uz2SupCHjuQ_CRhnh9m3q9a3KU_PGV3ecVFE/edit?usp=sharing @jpbetz @sftim can you two take a look and add your thoughts/suggestions? I'll check with the API machinery SIG in slack too |
This is great. I'll dropped in some comments. |
I've added a few small notes but looks really good so far! I can see this being very useful when complete! |
Alright, I've implemented a lot of the suggestions. I think that this is a good enough state for an initial draft - we can add more after publication as needed. @AverageMarcus @jpbetz @cici37 @SergeyKanzhelev @sftim can you please look at the doc one more time and change the LGTM dropdown for your name to LGTM if it's ok? I'll draft the content after that. |
lgtm from the content structure perspective. The doc promising to be very helpful! |
This is a Feature Request
What would you like to be added
This is a follow up from the #46825 (comment)
Specifically:
Why is this needed
KEPs like Sidecar Containers add new fields. We hit many issues when mutating webhook unintentionally removing the fields, they are not aware of. This highlights the issue that many mutating webhooks are implemented incorrectly and would benefit from following the best practices.
/assign @jpbetz
/sig api-machinery
The text was updated successfully, but these errors were encountered: