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

Add a good practices page for mutating webhook design #49626

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

shannonxtreme
Copy link
Contributor

Add a good practices page for designing and running mutating admission webhooks

Doc plan: https://docs.google.com/document/d/1pg80Qn3Uz2SupCHjuQ_CRhnh9m3q9a3KU_PGV3ecVFE/edit?pli=1&tab=t.0

Overview of changes

  • Create a new page based on the discussion in the doc plan

  • Move existing best practices from Dynamic Admission Control page

  • Deleted any existing practices that were significantly expanded on in the new page

  • Added guidance to compare types of admission control mechanisms and choose the best one

    I used an HTML table to do lists and to organize the info more clearly.

Outstanding questions and omitted sections

The following good practices didn't have enough information, so I omitted them from this iteration:

  • Webhooks vs. CI static yaml modifications and templating like kustomize? When to use what.
  • Scope what events the Pod should react to
  • Make sure to never double-mutate (check if object already has all modifications made)
  • If you're on older Kubernetes versions, they don't warn you when you provide an unknown or malformed field - be extra careful
  • Group endpoints into one or more binaries
  • Keep your mutating webhook up to date - make sure you update often as webhook can adopt new k8s API changes with every release. (TODO: is there any tooling that can help detect that it can be updated?)
  • Use of webhook abstraction tools such as Kyverno that should handle a lot of the best practices for you already

Co-authors/contributors

Closes: #47465

/sig docs
/language en

@k8s-ci-robot k8s-ci-robot added sig/docs Categorizes an issue or PR as relevant to SIG Docs. language/en Issues or PRs related to English language labels Feb 3, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign dipesh-rawat for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 3, 2025
@shannonxtreme
Copy link
Contributor Author

One question for reviewers: Some of the good practices that I moved seem relevant to validating webhooks as well. Should I keep the old content on the original page to avoid a loss of info?

Copy link

netlify bot commented Feb 3, 2025

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 58a1ab9
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/67b61061d2e6b5000848a0a1
😎 Deploy Preview https://deploy-preview-49626--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR. I recommend a copy editing round (see inline feedback), as well as a fact check.

Although this is a move of existing content, I recommend getting the styling right where we can.

It's not obligatory to fix any of the moved stuff. The net new text should follow our style guide where possible though.

Comment on lines +48 to +64
```shell
kubectl get mutatingwebhookconfigurations
```
The output lists any mutating admission controllers in the cluster.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used to support configuring these statically by passing a plugin configuration file to the kube-apiserver.

At technical review time or earlier, let's validate the the old method is completely omitted. Otherwise, this text isn't 100% accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sftim
Copy link
Contributor

sftim commented Feb 3, 2025

Outstanding questions and omitted sections

Once this merges, you are welcome - but not forced! - to file an issue about the remaining gaps.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might put this in the Cluster Administration section, and have https://kubernetes.io/docs/concepts/security/ also link there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do this before submission so that it doesn't muddle up the reviews

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect to LGTM this next review.

The quality bar is: does merging this make our docs overall better—provided that we didn't introduce anything that obviously misleads.

@guettli
Copy link
Contributor

guettli commented Feb 6, 2025

@shannonxtreme my best practice is to avoid mutating webhooks, because they confuse gitops tools. It's likely that ArgoCD will think that a resource is out of sync, and revert the changes made by the mutating webhooks.

We even avoid that gitops tool change the desired state stored in git. It's called the Rendered Manifests Pattern: https://akuity.io/blog/the-rendered-manifests-pattern

What about adding the RMP to the instruction section?

Additional note: some parts of the doc could link to the API conventions. The API conventions doc is the "single source of truth" about how to design or interact with Kubernetes APIs.

@SergeyKanzhelev
Copy link
Member

@shannonxtreme my best practice is to avoid mutating webhooks, because they confuse gitops tools. It's likely that ArgoCD will think that a resource is out of sync, and revert the changes made by the mutating webhooks.

We even avoid that gitops tool change the desired state stored in git. It's called the Rendered Manifests Pattern: https://akuity.io/blog/the-rendered-manifests-pattern

What about adding the RMP to the instruction section?

Additional note: some parts of the doc could link to the API conventions. The API conventions doc is the "single source of truth" about how to design or interact with Kubernetes APIs.

It will be a great addition to the article. Maybe we can start with merging this iteration. The article already stating some practices how to avoid using webhook, adding more is definitely beneficial.

@sftim
Copy link
Contributor

sftim commented Feb 7, 2025

You might find #46798 useful too @shannonxtreme; feel free to use elements of that PR (with appropriate credit!)

@SergeyKanzhelev
Copy link
Member

Also this one: https://github.com/kubernetes/website/pull/46798/files#r1946911968

It would be nice to merge this PR so we can iterate

@shannonxtreme
Copy link
Contributor Author

@SergeyKanzhelev I decided not to add the content from kubernetes/enhancements#5068 (comment) in this version of the doc as it would tie up the reviews a bit more. Let's merge this MVP and build on it!

shannonxtreme and others added 5 commits February 19, 2025 16:52
- Link to new page from dynamic admission control page
- Retain TODOs for info that'll be migration from existing page
…ices page

Moved content as-is (no text changes) for a more readable diff between commits.

The following sections werent moved:

* Idempotence main section (better content in new page)
* Intercepting all versions of an object (better content in new page)
* Guaranteeing the final state of an object is seen
* Avoiding operating in the kube-system namespace
- Remove h4 section headers for idempotence
- Wrap lines at 80 chars
- Update pod to Pod, deployment to Deployment
- Move the outcome of idempotency examples to the correct list item
- Update links to go to the correct places
Source PR: kubernetes#46798

Co-authored-by: Shaun Crampton <[email protected]>
Co-authored-by: Kat Cosgrove <[email protected]>
Co-authored-by: Tim Bannister <[email protected]>
@shannonxtreme shannonxtreme force-pushed the mutating-webhook-good-practices branch from 4776142 to 58a1ab9 Compare February 19, 2025 17:09
@shannonxtreme
Copy link
Contributor Author

@sftim @SergeyKanzhelev this is ready for another review. There are some unresolved comments in this PR that should either be a follow-up or need more info. I've listed them here:

  1. Rendered Manifest Patterns comment: follow-up
  2. Validation-only examples of idempotency comment: add now or follow up?
  3. Service load-balancing - no info in Service docs about it even though it's in the webhook best practices even today comment: remove the paragraph or keep?
  4. Static webhook configuration using file - is it omitted in the new page? comment: tech reviewer should confirm whether this is good to resolve

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2025
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document the best practices when implementing mutating webhook
5 participants