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.

Sign up for GitHub

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

Jump to bottom

⚠ Add AdmissionReviewVersions support for webhook #474

Merged
merged 1 commit into from
Aug 25, 2020
Merged

⚠ Add AdmissionReviewVersions support for webhook #474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
86 changes: 41 additions & 45 deletions pkg/webhook/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ limitations under the License.
//
// The markers take the form:
//
// +kubebuilder:webhook:webhookVersions=<[]string>,failurePolicy=<string>,matchPolicy=<string>,groups=<[]string>,resources=<[]string>,verbs=<[]string>,versions=<[]string>,name=<string>,path=<string>,mutating=<bool>,sideEffects=<string>
// +kubebuilder:webhook:webhookVersions=<[]string>,failurePolicy=<string>,matchPolicy=<string>,groups=<[]string>,resources=<[]string>,verbs=<[]string>,versions=<[]string>,name=<string>,path=<string>,mutating=<bool>,sideEffects=<string>,admissionReviewVersions=<[]string>
package webhook

import (
Expand Down Expand Up @@ -107,6 +107,12 @@ type Config struct {
// WebhookVersions specifies the target API versions of the {Mutating,Validating}WebhookConfiguration objects
// itself to generate. Defaults to v1.
WebhookVersions []string `marker:"webhookVersions,optional"`

// AdmissionReviewVersions is an ordered list of preferred `AdmissionReview`
// versions the Webhook expects.
// For generating v1 {Mutating,Validating}WebhookConfiguration, this is mandatory.
// For generating v1beta1 {Mutating,Validating}WebhookConfiguration, this is optional, and default to v1beta1.
AdmissionReviewVersions []string `marker:"admissionReviewVersions,optional"`
}

// verbToAPIVariant converts a marker's verb to the proper value for the API.
Expand Down Expand Up @@ -140,15 +146,13 @@ func (c Config) ToMutatingWebhook() (admissionregv1.MutatingWebhook, error) {
}

return admissionregv1.MutatingWebhook{
Name: c.Name,
Rules: c.rules(),
FailurePolicy: c.failurePolicy(),
MatchPolicy: matchPolicy,
ClientConfig: c.clientConfig(),
SideEffects: c.sideEffects(),
// TODO(jiachengxu): AdmissionReviewVersions becomes required in admissionregistration/v1, here we default it
// to `v1` and `v1beta1`, and we should support to config the `AdmissionReviewVersions` as a marker.
AdmissionReviewVersions: []string{defaultWebhookVersion, "v1beta1"},
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if this might be nice to keep this as a default for v1, and have AdmissionReviewVersions: []string{"v1beta1"} as the default for v1beta1 or not default them (even though it is probably good practice to do so).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For v1beta1, I think we don't need to default it since it will be defaulted by kube-apiserver to []string{"v1beta1"} if it is not specified: https://github.com/kubernetes/api/blob/master/admissionregistration/v1beta1/types.go#L318-L320

For v1, I am not sure if we should default it. I would slightly prefer to ask the user to set AdmissionReviewVersions because it is required in v1, and don't really want to hide this to the user, defaulting it without notifying may cause some unknown issues in their environments/projects. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

On the flip side, breaking marker format for users using defaults isn’t great, although this is a requirement for v1 and they can pretty easily keep using v1beta1 if they choose. Is there a policy on breaking marker format @DirectXMan12 @vincepri?

Copy link
Member

Choose a reason for hiding this comment

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

This is definitely a breaking change as you outlined, if we're supporting both we should have defaults only when necessary. When using v1, we should make sure there is some sort of validation to not break users

Copy link
Contributor

Choose a reason for hiding this comment

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

If we’re defaulting only when necessary then we probably shouldn’t here, since these need to be set for v1 but controller-gen can’t know which admission review versions the webhook server is using.

Name: c.Name,
Rules: c.rules(),
FailurePolicy: c.failurePolicy(),
MatchPolicy: matchPolicy,
ClientConfig: c.clientConfig(),
SideEffects: c.sideEffects(),
AdmissionReviewVersions: c.AdmissionReviewVersions,
}, nil
}

Expand All @@ -164,15 +168,13 @@ func (c Config) ToValidatingWebhook() (admissionregv1.ValidatingWebhook, error)
}

return admissionregv1.ValidatingWebhook{
Name: c.Name,
Rules: c.rules(),
FailurePolicy: c.failurePolicy(),
MatchPolicy: matchPolicy,
ClientConfig: c.clientConfig(),
SideEffects: c.sideEffects(),
// TODO(jiachengxu): AdmissionReviewVersions becomes required in admissionregistration/v1, here we default it
// to `v1` and `v1beta1`, and we should support to config the `AdmissionReviewVersions` as a marker.
AdmissionReviewVersions: []string{defaultWebhookVersion, "v1beta1"},
Name: c.Name,
Rules: c.rules(),
FailurePolicy: c.failurePolicy(),
MatchPolicy: matchPolicy,
ClientConfig: c.clientConfig(),
SideEffects: c.sideEffects(),
AdmissionReviewVersions: c.AdmissionReviewVersions,
}, nil
}

Expand Down Expand Up @@ -344,29 +346,26 @@ func (Generator) Generate(ctx *genall.GenerationContext) error {
},
Webhooks: cfgs,
}
// SideEffects in required in admissionregistration/v1, if this is not set or set to `Some` or `Known`,
// we return an error
if version == defaultWebhookVersion {
for i := range objRaw.Webhooks {
// SideEffects is required in admissionregistration/v1, if this is not set or set to `Some` or `Known`,
// we return an error
if err := checkSideEffectsForV1(objRaw.Webhooks[i].SideEffects); err != nil {
return err
}
// AdmissionReviewVersions is required in admissionregistration/v1, if this is not set,
// we return an error
if len(objRaw.Webhooks[i].AdmissionReviewVersions) == 0 {
return fmt.Errorf("AdmissionReviewVersions is mandatory for v1 {Mutating,Validating}WebhookConfiguration")
}
}
}
// AdmissionReviewVersions is optional in admissionregistration/v1beta1, so let kubernetes to default it.
if version == "v1beta1" {
for i := range objRaw.Webhooks {
objRaw.Webhooks[i].AdmissionReviewVersions = nil
}
}
if version != defaultWebhookVersion {
versionedWebhooks[version] = append(versionedWebhooks[version], objRaw)
} else {
conv, err := MutatingWebhookConfigurationAsVersion(objRaw, schema.GroupVersion{Group: admissionregv1.SchemeGroupVersion.Group, Version: version})
versionedWebhooks[version] = append(versionedWebhooks[version], conv)
if err != nil {
return err
}
} else {
versionedWebhooks[version] = append(versionedWebhooks[version], objRaw)
versionedWebhooks[version] = append(versionedWebhooks[version], conv)
}
}

Expand All @@ -381,29 +380,26 @@ func (Generator) Generate(ctx *genall.GenerationContext) error {
},
Webhooks: cfgs,
}
// SideEffects in required in admissionregistration/v1, if this is not set or set to `Some` or `Known`,
// we return an error
if version == defaultWebhookVersion {
for i := range objRaw.Webhooks {
// SideEffects is required in admissionregistration/v1, if this is not set or set to `Some` or `Known`,
// we return an error
if err := checkSideEffectsForV1(objRaw.Webhooks[i].SideEffects); err != nil {
return err
}
// AdmissionReviewVersions is required in admissionregistration/v1, if this is not set,
// we return an error
if len(objRaw.Webhooks[i].AdmissionReviewVersions) == 0 {
return fmt.Errorf("AdmissionReviewVersions is mandatory for v1 {Mutating,Validating}WebhookConfiguration")
}
}
}
// AdmissionReviewVersions is optional in admissionregistration/v1beta1, so let kubernetes to default it.
if version == "v1beta1" {
for i := range objRaw.Webhooks {
objRaw.Webhooks[i].AdmissionReviewVersions = nil
}
}
if version != defaultWebhookVersion {
versionedWebhooks[version] = append(versionedWebhooks[version], objRaw)
} else {
conv, err := ValidatingWebhookConfigurationAsVersion(objRaw, schema.GroupVersion{Group: admissionregv1.SchemeGroupVersion.Group, Version: version})
versionedWebhooks[version] = append(versionedWebhooks[version], conv)
if err != nil {
return err
}
} else {
versionedWebhooks[version] = append(versionedWebhooks[version], objRaw)
versionedWebhooks[version] = append(versionedWebhooks[version], conv)
}
}
}
Expand Down
5 changes: 4 additions & 1 deletion pkg/webhook/testdata/manifests.v1beta1.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ metadata:
creationTimestamp: null
name: mutating-webhook-configuration
webhooks:
- clientConfig:
- admissionReviewVersions:
- v1
- v1beta1
clientConfig:
caBundle: Cg==
service:
name: webhook-service
Expand Down
4 changes: 2 additions & 2 deletions pkg/webhook/testdata/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ func (c *CronJob) SetupWebhookWithManager(mgr ctrl.Manager) error {
}

// +kubebuilder:webhook:webhookVersions=v1beta1,verbs=create;update,path=/validate-testdata-kubebuilder-io-v1-cronjob,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=testdata.kubebuiler.io,resources=cronjobs,versions=v1,name=validation.cronjob.testdata.kubebuilder.io,sideEffects=Some
// +kubebuilder:webhook:verbs=create;update,path=/validate-testdata-kubebuilder-io-v1-cronjob,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=testdata.kubebuiler.io,resources=cronjobs,versions=v1,name=validation.cronjob.testdata.kubebuilder.io,sideEffects=NoneOnDryRun
// +kubebuilder:webhook:webhookVersions=v1;v1beta1,verbs=create;update,path=/mutate-testdata-kubebuilder-io-v1-cronjob,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=testdata.kubebuiler.io,resources=cronjobs,versions=v1,name=default.cronjob.testdata.kubebuilder.io,sideEffects=None
// +kubebuilder:webhook:verbs=create;update,path=/validate-testdata-kubebuilder-io-v1-cronjob,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=testdata.kubebuiler.io,resources=cronjobs,versions=v1,name=validation.cronjob.testdata.kubebuilder.io,sideEffects=NoneOnDryRun,admissionReviewVersions=v1;v1beta1
// +kubebuilder:webhook:webhookVersions=v1;v1beta1,verbs=create;update,path=/mutate-testdata-kubebuilder-io-v1-cronjob,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=testdata.kubebuiler.io,resources=cronjobs,versions=v1,name=default.cronjob.testdata.kubebuilder.io,sideEffects=None,admissionReviewVersions=v1;v1beta1

var _ webhook.Defaulter = &CronJob{}
var _ webhook.Validator = &CronJob{}
Expand Down
4 changes: 4 additions & 0 deletions pkg/webhook/zz_generated.markerhelp.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.