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

Document the best practices when implementing mutating webhook #47465

Open
SergeyKanzhelev opened this issue Aug 12, 2024 · 14 comments
Open

Document the best practices when implementing mutating webhook #47465

SergeyKanzhelev opened this issue Aug 12, 2024 · 14 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. language/en Issues or PRs related to English language priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. triage/accepted Indicates an issue or PR is ready to be actively worked on. wg/api-expression Categorizes an issue or PR as relevant to WG API Expression.

Comments

@SergeyKanzhelev
Copy link
Member

This is a Feature Request

What would you like to be added

This is a follow up from the #46825 (comment)

  1. Document the best practices writing tools and mutating web hooks that will preserve unknown fields: Add tutorial about running Pods with sidecar containers #46825 (comment)

Specifically:

  • Use patch instead of update
  • Patch only what you know
  • Expect the new fields may be defined on existing objects that your webhook may not know about
  • Any other recommendations?

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

@SergeyKanzhelev SergeyKanzhelev added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 12, 2024
@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 12, 2024
@natalisucks
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 12, 2024
@SergeyKanzhelev
Copy link
Member Author

@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.

@jpbetz
Copy link
Contributor

jpbetz commented Sep 13, 2024

I'll try to get some attention on this.

@sftim
Copy link
Contributor

sftim commented Sep 27, 2024

/priority important-longterm

due to relevance around sidecar containers turning into init containers
/wg api-expression
/language en

@k8s-ci-robot k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. wg/api-expression Categorizes an issue or PR as relevant to WG API Expression. language/en Issues or PRs related to English language labels Sep 27, 2024
@sftim
Copy link
Contributor

sftim commented Sep 27, 2024

@shannonxtreme or @jpbetz if you'd like to pair on this (or pair up async), I can try to make time

@shannonxtreme
Copy link
Contributor

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?

@sftim
Copy link
Contributor

sftim commented Sep 28, 2024

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

@SergeyKanzhelev
Copy link
Member Author

Not sure about scope.

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.

@shannonxtreme
Copy link
Contributor

/assign

@shannonxtreme
Copy link
Contributor

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

@jpbetz
Copy link
Contributor

jpbetz commented Oct 4, 2024

This is great. I'll dropped in some comments.

@AverageMarcus
Copy link
Member

I've added a few small notes but looks really good so far! I can see this being very useful when complete!

@shannonxtreme
Copy link
Contributor

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.

@SergeyKanzhelev
Copy link
Member Author

lgtm from the content structure perspective. The doc promising to be very helpful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. language/en Issues or PRs related to English language priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. triage/accepted Indicates an issue or PR is ready to be actively worked on. wg/api-expression Categorizes an issue or PR as relevant to WG API Expression.
Projects
None yet
Development

No branches or pull requests

7 participants