🌱 validate group names in webhooks. #1144
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: guettli 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 |
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @guettli! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @guettli. Thanks for your PR. I'm waiting for a kubernetes-sigs 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/S
| @@ -448,6 +449,8 @@ func (g Generator) Generate(ctx *genall.GenerationContext) error { | |||
| var mutatingWebhookCfgs admissionregv1.MutatingWebhookConfiguration | |||
| var validatingWebhookCfgs admissionregv1.ValidatingWebhookConfiguration | |||
|
|
|||
| groupRegex := regexp.MustCompile(`^[a-z][-a-z0-9.]*[a-z0-9]$`) | |||
There was a problem hiding this comment.
maybe worth to have some positive and negative unit tests for this to ensure we (don't) error out on the right ones.
|
Which specific field within the mutating/validating webhook configuration does the group end up in? Have we compared this new validation with validation applied during the admission of those resources? |
|
This manifest got created, and it seems to get applied successfully: apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
metadata:
name: validating-webhook-configuration
webhooks:
- admissionReviewVersions:
- v1
- v1beta1
clientConfig:
service:
name: webhook-service
namespace: system
path: /validate-foo-example-com-v1beta1-foocluster
failurePolicy: Fail
name: validation.foocluster.foo.example.com
rules:
- apiGroups:
- foo.example.com/iiiiinvalid # <------------------------
apiVersions:
- v1beta1
operations:
- CREATE
- UPDATE
resources:
- fooclusters
sideEffects: NoneI am thinking about this again, and maybe the better fix is to disallow that in Kubernetes. In my envTests this gets applied successfully. Fixing that in Kubernetes would help more users. Is there a valid use-case for using an apiGroup which contains a slash? Which regex could get used to validate an apiGroup? |
|
So there isn't any validation in the admission of a VWC that checks for the pattern to match. But for a CRD, the group is validated to be a DNS1123Subdomain, so perhaps we can leverage that validation, combined with validation for the built-in groups? Maybe also worth checking with sig-apimachinery if they have any thoughts |
|
I asked in #api-machinery ... |
| @@ -493,6 +496,14 @@ func (g Generator) Generate(ctx *genall.GenerationContext) error { | |||
| if err != nil { | |||
| return err | |||
| } | |||
| for _, group := range cfg.Groups { | |||
| if group == "" { // aka "core" | |||
There was a problem hiding this comment.
Why would we continue? There must be a groupname
| if group == "" { // aka "core" | ||
| continue | ||
| } | ||
| if !groupRegex.MatchString(group) { |
There was a problem hiding this comment.
This error will be very frustrating for anyone that runs into it - Please include the string of the regex used for validation
|
Thank you for your feedback. I think Make ValidatingWebhookConfiguration validate rule.APIGroups makes more sense. Validating in controller-tools has the benefit that it fails earlier, but validating at the api server has the benefit that it fails for all ValidatingWebhookConfigurations (created via controller-gen or by hand). I am closing my PR. Feel free to re-open if you want to continue. |
This PR makes controller-gen fail, if there is an invalid group name in a
kubebuilder:webhookcomment.This helps to detect typos early.
TODO: I am unsure if the regex to validate an api group name is correct.
Related: #1143