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

✨ Allow customizing generated webhook configuration's name #1002

Merged
merged 1 commit into from
Sep 12, 2024
Merged

✨ Allow customizing generated webhook configuration's name #1002

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 98 additions & 9 deletions pkg/webhook/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/sets"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-tools/pkg/genall"
"sigs.k8s.io/controller-tools/pkg/markers"
)
Expand All @@ -42,16 +43,31 @@ const (
)

var (
// ConfigDefinition s a marker for defining Webhook manifests.
// ConfigDefinition is a marker for defining Webhook manifests.
// Call ToWebhook on the value to get a Kubernetes Webhook.
ConfigDefinition = markers.Must(markers.MakeDefinition("kubebuilder:webhook", markers.DescribesPackage, Config{}))
// WebhookConfigDefinition is a marker for defining MutatingWebhookConfiguration or ValidatingWebhookConfiguration manifests.
WebhookConfigDefinition = markers.Must(markers.MakeDefinition("kubebuilder:webhookconfiguration", markers.DescribesPackage, WebhookConfig{}))
sbueringer marked this conversation as resolved.
Show resolved Hide resolved
)

// supportedWebhookVersions returns currently supported API version of {Mutating,Validating}WebhookConfiguration.
func supportedWebhookVersions() []string {
return []string{defaultWebhookVersion}
}

// +controllertools:marker:generateHelp

type WebhookConfig struct {
// Mutating marks this as a mutating webhook (it's validating only if false)
//
// Mutating webhooks are allowed to change the object in their response,
// and are called *before* all validating webhooks. Mutating webhooks may
// choose to reject an object, similarly to a validating webhook.
Mutating bool
// Name indicates the name of the K8s MutatingWebhookConfiguration or ValidatingWebhookConfiguration object.
Name string `marker:"name,optional"`
}

// +controllertools:marker:generateHelp:category=Webhook

// Config specifies how a webhook should be served.
Expand Down Expand Up @@ -154,6 +170,32 @@ func verbToAPIVariant(verbRaw string) admissionregv1.OperationType {
}
}

// ToMutatingWebhookConfiguration converts this WebhookConfig to its Kubernetes API form.
func (c WebhookConfig) ToMutatingWebhookConfiguration() (admissionregv1.MutatingWebhookConfiguration, error) {
if !c.Mutating {
return admissionregv1.MutatingWebhookConfiguration{}, fmt.Errorf("%s is a validating webhook", c.Name)
}

return admissionregv1.MutatingWebhookConfiguration{
ObjectMeta: metav1.ObjectMeta{
Name: c.Name,
},
}, nil
}

// ToValidatingWebhookConfiguration converts this WebhookConfig to its Kubernetes API form.
func (c WebhookConfig) ToValidatingWebhookConfiguration() (admissionregv1.ValidatingWebhookConfiguration, error) {
if c.Mutating {
return admissionregv1.ValidatingWebhookConfiguration{}, fmt.Errorf("%s is a mutating webhook", c.Name)
}

return admissionregv1.ValidatingWebhookConfiguration{
ObjectMeta: metav1.ObjectMeta{
Name: c.Name,
},
}, nil
}

// ToMutatingWebhook converts this rule to its Kubernetes API form.
func (c Config) ToMutatingWebhook() (admissionregv1.MutatingWebhook, error) {
if !c.Mutating {
Expand Down Expand Up @@ -362,20 +404,55 @@ func (Generator) RegisterMarkers(into *markers.Registry) error {
if err := into.Register(ConfigDefinition); err != nil {
return err
}
if err := into.Register(WebhookConfigDefinition); err != nil {
return err
}
into.AddHelp(ConfigDefinition, Config{}.Help())
into.AddHelp(WebhookConfigDefinition, Config{}.Help())
return nil
}

func (g Generator) Generate(ctx *genall.GenerationContext) error {
supportedWebhookVersions := supportedWebhookVersions()
mutatingCfgs := make(map[string][]admissionregv1.MutatingWebhook, len(supportedWebhookVersions))
validatingCfgs := make(map[string][]admissionregv1.ValidatingWebhook, len(supportedWebhookVersions))
var mutatingWebhookCfgs admissionregv1.MutatingWebhookConfiguration
var validatingWebhookCfgs admissionregv1.ValidatingWebhookConfiguration

for _, root := range ctx.Roots {
markerSet, err := markers.PackageMarkers(ctx.Collector, root)
if err != nil {
root.AddError(err)
}

webhookCfgs := markerSet[WebhookConfigDefinition.Name]
sbueringer marked this conversation as resolved.
Show resolved Hide resolved
var hasValidatingWebhookConfig, hasMutatingWebhookConfig bool = false, false
for _, webhookCfg := range webhookCfgs {
webhookCfg := webhookCfg.(WebhookConfig)

if webhookCfg.Mutating {
if hasMutatingWebhookConfig {
return fmt.Errorf("duplicate mutating %s with name %s", WebhookConfigDefinition.Name, webhookCfg.Name)
}

if mutatingWebhookCfgs, err = webhookCfg.ToMutatingWebhookConfiguration(); err != nil {
return err
}

hasMutatingWebhookConfig = true
} else {
if hasValidatingWebhookConfig {
return fmt.Errorf("duplicate validating %s with name %s", WebhookConfigDefinition.Name, webhookCfg.Name)
}

if validatingWebhookCfgs, err = webhookCfg.ToValidatingWebhookConfiguration(); err != nil {
return err
}

hasValidatingWebhookConfig = true
}
}

cfgs := markerSet[ConfigDefinition.Name]
sort.SliceStable(cfgs, func(i, j int) bool {
return cfgs[i].(Config).Name < cfgs[j].(Config).Name
Expand Down Expand Up @@ -410,16 +487,22 @@ func (g Generator) Generate(ctx *genall.GenerationContext) error {
versionedWebhooks := make(map[string][]interface{}, len(supportedWebhookVersions))
for _, version := range supportedWebhookVersions {
if cfgs, ok := mutatingCfgs[version]; ok {
// The only possible version in supportedWebhookVersions is v1,
// so use it for all versioned types in this context.
objRaw := &admissionregv1.MutatingWebhookConfiguration{}
var objRaw *admissionregv1.MutatingWebhookConfiguration
if mutatingWebhookCfgs.Name != "" {
objRaw = &mutatingWebhookCfgs
} else {
// The only possible version in supportedWebhookVersions is v1,
// so use it for all versioned types in this context.
objRaw = &admissionregv1.MutatingWebhookConfiguration{}
objRaw.SetName("mutating-webhook-configuration")
}
Comment on lines +493 to +498
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 backwards-compat

objRaw.SetGroupVersionKind(schema.GroupVersionKind{
Group: admissionregv1.SchemeGroupVersion.Group,
Version: version,
Kind: "MutatingWebhookConfiguration",
})
objRaw.SetName("mutating-webhook-configuration")
objRaw.Webhooks = cfgs

for i := range objRaw.Webhooks {
// SideEffects is required in admissionregistration/v1, if this is not set or set to `Some` or `Known`,
// return an error
Expand All @@ -441,16 +524,22 @@ func (g Generator) Generate(ctx *genall.GenerationContext) error {
}

if cfgs, ok := validatingCfgs[version]; ok {
// The only possible version in supportedWebhookVersions is v1,
// so use it for all versioned types in this context.
objRaw := &admissionregv1.ValidatingWebhookConfiguration{}
var objRaw *admissionregv1.ValidatingWebhookConfiguration
if validatingWebhookCfgs.Name != "" {
objRaw = &validatingWebhookCfgs
} else {
// The only possible version in supportedWebhookVersions is v1,
// so use it for all versioned types in this context.
objRaw = &admissionregv1.ValidatingWebhookConfiguration{}
objRaw.SetName("validating-webhook-configuration")
}
Comment on lines +530 to +535
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 backwards-compat

objRaw.SetGroupVersionKind(schema.GroupVersionKind{
Group: admissionregv1.SchemeGroupVersion.Group,
Version: version,
Kind: "ValidatingWebhookConfiguration",
})
objRaw.SetName("validating-webhook-configuration")
objRaw.Webhooks = cfgs

for i := range objRaw.Webhooks {
// SideEffects is required in admissionregistration/v1, if this is not set or set to `Some` or `Known`,
// return an error
Expand Down
85 changes: 85 additions & 0 deletions pkg/webhook/parser_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
By("setting up the parser")
reg := &markers.Registry{}
Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())

By("requesting that the manifest be generated")
outputDir, err := os.MkdirTemp("", "webhook-integration-test")
Expand Down Expand Up @@ -85,6 +86,7 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
By("setting up the parser")
reg := &markers.Registry{}
Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())

By("requesting that the manifest be generated")
outputDir, err := os.MkdirTemp("", "webhook-integration-test")
Expand Down Expand Up @@ -116,6 +118,7 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
By("setting up the parser")
reg := &markers.Registry{}
Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())

By("requesting that the manifest be generated")
outputDir, err := os.MkdirTemp("", "webhook-integration-test")
Expand Down Expand Up @@ -145,6 +148,7 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
By("setting up the parser")
reg := &markers.Registry{}
Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())

By("requesting that the manifest be generated")
outputDir, err := os.MkdirTemp("", "webhook-integration-test")
Expand Down Expand Up @@ -174,6 +178,7 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
By("setting up the parser")
reg := &markers.Registry{}
Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())

By("requesting that the manifest be generated")
outputDir, err := os.MkdirTemp("", "webhook-integration-test")
Expand Down Expand Up @@ -219,6 +224,7 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
By("setting up the parser")
reg := &markers.Registry{}
Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())

By("requesting that the manifest be generated")
outputDir, err := os.MkdirTemp("", "webhook-integration-test")
Expand Down Expand Up @@ -268,6 +274,7 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
By("setting up the parser")
reg := &markers.Registry{}
Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())

By("requesting that the manifest be generated")
outputDir, err := os.MkdirTemp("", "webhook-integration-test")
Expand Down Expand Up @@ -313,6 +320,7 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
By("setting up the parser")
reg := &markers.Registry{}
Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())

By("requesting that the manifest be generated")
outputDir, err := os.MkdirTemp("", "webhook-integration-test")
Expand All @@ -326,6 +334,83 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
err = webhook.Generator{}.Generate(genCtx)
Expect(err).To(HaveOccurred())
})

It("should properly generate the webhook definition when a name is specified with the `kubebuilder:webhookconfiguration` marker", func() {
By("switching into testdata to appease go modules")
cwd, err := os.Getwd()
Expect(err).NotTo(HaveOccurred())
Expect(os.Chdir("./testdata/valid-custom-name")).To(Succeed()) // go modules are directory-sensitive
defer func() { Expect(os.Chdir(cwd)).To(Succeed()) }()

By("loading the roots")
pkgs, err := loader.LoadRoots(".")
Expect(err).NotTo(HaveOccurred())
Expect(pkgs).To(HaveLen(1))

By("setting up the parser")
reg := &markers.Registry{}
Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())

By("requesting that the manifest be generated")
outputDir, err := os.MkdirTemp("", "webhook-integration-test")
Expect(err).NotTo(HaveOccurred())
defer os.RemoveAll(outputDir)
genCtx := &genall.GenerationContext{
Collector: &markers.Collector{Registry: reg},
Roots: pkgs,
OutputRule: genall.OutputToDirectory(outputDir),
}
Expect(webhook.Generator{}.Generate(genCtx)).To(Succeed())
for _, r := range genCtx.Roots {
Expect(r.Errors).To(HaveLen(0))
}

By("loading the generated v1 YAML")
actualFile, err := os.ReadFile(path.Join(outputDir, "manifests.yaml"))
Expect(err).NotTo(HaveOccurred())
actualMutating, actualValidating := unmarshalBothV1(actualFile)

By("loading the desired v1 YAML")
expectedFile, err := os.ReadFile("manifests.yaml")
Expect(err).NotTo(HaveOccurred())
expectedMutating, expectedValidating := unmarshalBothV1(expectedFile)

By("comparing the two")
assertSame(actualMutating, expectedMutating)
assertSame(actualValidating, expectedValidating)
})

It("should fail to generate when there are multiple `kubebuilder:webhookconfiguration` markers of the same mutation type", func() {
By("switching into testdata to appease go modules")
cwd, err := os.Getwd()
Expect(err).NotTo(HaveOccurred())
Expect(os.Chdir("./testdata/invalid-multiple-webhookconfigurations")).To(Succeed()) // go modules are directory-sensitive
defer func() { Expect(os.Chdir(cwd)).To(Succeed()) }()

By("loading the roots")
pkgs, err := loader.LoadRoots(".")
Expect(err).NotTo(HaveOccurred())
Expect(pkgs).To(HaveLen(1))

By("setting up the parser")
reg := &markers.Registry{}
Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())

By("requesting that the manifest be generated")
outputDir, err := os.MkdirTemp("", "webhook-integration-test")
Expect(err).NotTo(HaveOccurred())
defer os.RemoveAll(outputDir)
genCtx := &genall.GenerationContext{
Collector: &markers.Collector{Registry: reg},
Roots: pkgs,
OutputRule: genall.OutputToDirectory(outputDir),
}
err = webhook.Generator{}.Generate(genCtx)
Expect(err).To(HaveOccurred())
})

})

func unmarshalBothV1(in []byte) (mutating admissionregv1.MutatingWebhookConfiguration, validating admissionregv1.ValidatingWebhookConfiguration) {
Expand Down
Loading
Loading