Skip to content

Conversation

@SHA65536
Copy link

I added an option for the webhook to only match certain namespaces instead of all of them
In GKE they complain when you have a webhook that can intercept requests for system namespaces (kube-system, kube-node-lease) so I needed a way to adhere to their guidelines
I tested this on our company infra, seems fine
Thanks!

@SHA65536
Copy link
Author

@skartikey @eatondustin1 can you take a look?
Sorry for the ping, saw you reviewed stuff recently

eatondustin1
eatondustin1 previously approved these changes Nov 24, 2025
Copy link
Contributor

@eatondustin1 eatondustin1 left a comment

Choose a reason for hiding this comment

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

@SHA65536 sorry for the delay, but this LGTM, thanks for the contribution 🙌

@skartikey
Copy link
Contributor

@SHA65536
Issues/Suggestions:

  1. Missing documentation in values.yaml - The new namespaceSelector option has no comment explaining its purpose or usage: namespaceSelector: {}
    Should include a comment like:
  # namespaceSelector allows restricting webhook to specific namespaces
  # See: https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#matching-requests-namespaceselector
  # Example to exclude system namespaces:
  # namespaceSelector:
  #   matchExpressions:
  #   - key: kubernetes.io/metadata.name
  #     operator: NotIn
  #     values: ["kube-system", "kube-node-lease"]
  namespaceSelector: {}
  1. README not updated - The chart README should document this new option for discoverability.
  2. Duplicate template logic - The same webhook configuration exists in both _helpers.tpl and mutatingwebhookconfiguration.yml. Consider if this duplication is intentional or could be consolidated.
  3. Template logic review - The scope: "Namespaced" is only set when namespaceSelector is not empty. This is correct behavior, but the namespaceSelector: field is always rendered (outputting namespaceSelector: {} when
    empty). This should work fine but could be wrapped in a conditional for cleaner output.

@eatondustin1 eatondustin1 dismissed their stale review November 24, 2025 13:23

I'm dismissing my approval for now based on @skartikey 's feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants