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

✨ Webhooks for external APIs #4176

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion pkg/plugins/golang/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ func (opts Options) UpdateResource(res *resource.Resource, c config.Config) {

if opts.DoDefaulting || opts.DoValidation || opts.DoConversion {
//nolint:staticcheck
res.Path = resource.APIPackagePath(c.GetRepository(), res.Group, res.Version, c.IsMultiGroup())
loadedRes, _ := c.GetResource(res.GVK)
res.Path = loadedRes.Path

res.Webhooks.WebhookVersion = "v1"
if opts.DoDefaulting {
Expand Down
31 changes: 31 additions & 0 deletions pkg/plugins/golang/v4/scaffolds/internal/templates/api/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ type Webhook struct { // nolint:maligned
// Define value for AdmissionReviewVersions marker
AdmissionReviewVersions string

// If the webhook is for an external resource. External resources need to be
// imported.
ExternalAPI bool

Force bool
}

Expand Down Expand Up @@ -97,16 +101,27 @@ import (
{{- if .Resource.HasValidationWebhook }}
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
{{- end }}
{{- if .ExternalAPI }}
{{ .Resource.ImportAlias }} "{{ .Resource.Path }}"
{{- end }}
)

// nolint:unused
// log is for logging in this package.
var {{ lower .Resource.Kind }}log = logf.Log.WithName("{{ lower .Resource.Kind }}-resource")

// SetupWebhookWithManager will setup the manager to manage the webhooks.
{{- if .ExternalAPI }}
func SetupWebhookFor{{ .Resource.Kind }}WithManager(mgr ctrl.Manager) error {
Copy link
Member

Choose a reason for hiding this comment

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

Those changes will not be required after : #4150

{{- else }}
func (r *{{ .Resource.Kind }}) SetupWebhookWithManager(mgr ctrl.Manager) error {
{{- end }}
return ctrl.NewWebhookManagedBy(mgr).
{{- if .ExternalAPI }}
For(&{{ .Resource.ImportAlias }}.{{ .Resource.Kind }}{}).
Copy link
Member

Choose a reason for hiding this comment

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

Here should be if the Resource.Path is not empty

{{- else }}
For(r).
{{- end }}
{{- if .Resource.HasValidationWebhook }}
WithValidator(&{{ .Resource.Kind }}CustomValidator{}).
{{- end }}
Expand Down Expand Up @@ -137,7 +152,11 @@ var _ webhook.CustomDefaulter = &{{ .Resource.Kind }}CustomDefaulter{}

// Default implements webhook.CustomDefaulter so a webhook will be registered for the Kind {{ .Resource.Kind }}.
func (d *{{ .Resource.Kind }}CustomDefaulter) Default(ctx context.Context, obj runtime.Object) error {
{{- if .ExternalAPI }}
{{ lower .Resource.Kind }}, ok := obj.(*{{ .Resource.ImportAlias }}.{{ .Resource.Kind }})
{{- else }}
{{ lower .Resource.Kind }}, ok := obj.(*{{ .Resource.Kind }})
{{- end }}
if !ok {
return fmt.Errorf("expected an {{ .Resource.Kind }} object but got %T", obj)
}
Expand Down Expand Up @@ -170,7 +189,11 @@ var _ webhook.CustomValidator = &{{ .Resource.Kind }}CustomValidator{}

// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type {{ .Resource.Kind }}.
func (v *{{ .Resource.Kind }}CustomValidator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
{{- if .ExternalAPI }}
{{ lower .Resource.Kind }}, ok := obj.(*{{ .Resource.ImportAlias }}.{{ .Resource.Kind }})
{{- else }}
{{ lower .Resource.Kind }}, ok := obj.(*{{ .Resource.Kind }})
{{- end }}
if !ok {
return nil, fmt.Errorf("expected a {{ .Resource.Kind }} object but got %T", obj)
}
Expand All @@ -183,7 +206,11 @@ func (v *{{ .Resource.Kind }}CustomValidator) ValidateCreate(ctx context.Context

// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type {{ .Resource.Kind }}.
func (v *{{ .Resource.Kind }}CustomValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
{{- if .ExternalAPI }}
{{ lower .Resource.Kind }}, ok := newObj.(*{{ .Resource.ImportAlias }}.{{ .Resource.Kind }})
{{- else }}
{{ lower .Resource.Kind }}, ok := newObj.(*{{ .Resource.Kind }})
{{- end }}
if !ok {
return nil, fmt.Errorf("expected a {{ .Resource.Kind }} object but got %T", newObj)
}
Expand All @@ -196,7 +223,11 @@ func (v *{{ .Resource.Kind }}CustomValidator) ValidateUpdate(ctx context.Context

// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type {{ .Resource.Kind }}.
func (v *{{ .Resource.Kind }}CustomValidator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
{{- if .ExternalAPI }}
{{ lower .Resource.Kind }}, ok := obj.(*{{ .Resource.ImportAlias }}.{{ .Resource.Kind }})
{{- else }}
{{ lower .Resource.Kind }}, ok := obj.(*{{ .Resource.Kind }})
{{- end }}
if !ok {
return nil, fmt.Errorf("expected a {{ .Resource.Kind }} object but got %T", obj)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ type WebhookSuite struct { //nolint:maligned

// BaseDirectoryRelativePath define the Path for the base directory when it is multigroup
BaseDirectoryRelativePath string

ExternalAPI bool
}

// SetTemplateDefaults implements file.Template
Expand Down Expand Up @@ -101,6 +103,11 @@ const (
addWebhookManagerCodeFragment = `err = (&%s{}).SetupWebhookWithManager(mgr)
Expect(err).NotTo(HaveOccurred())

`

addWebhookExternalManagerCodeFragment = `err = SetupWebhookFor%sWithManager(mgr)
Expect(err).NotTo(HaveOccurred())

`
)

Expand All @@ -117,7 +124,11 @@ func (f *WebhookSuite) GetCodeFragments() machinery.CodeFragmentsMap {

// Generate add webhookManager code fragments
addWebhookManager := make([]string, 0)
addWebhookManager = append(addWebhookManager, fmt.Sprintf(addWebhookManagerCodeFragment, f.Resource.Kind))
if f.ExternalAPI {
addWebhookManager = append(addWebhookManager, fmt.Sprintf(addWebhookExternalManagerCodeFragment, f.Resource.Kind))
} else {
addWebhookManager = append(addWebhookManager, fmt.Sprintf(addWebhookManagerCodeFragment, f.Resource.Kind))
}

// Only store code fragments in the map if the slices are non-empty
if len(addWebhookManager) != 0 {
Expand Down Expand Up @@ -159,6 +170,9 @@ import (
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/log/zap"
metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
{{- if .ExternalAPI }}
{{ .Resource.ImportAlias }} "{{ .Resource.Path }}"
{{- end }}
)

// These tests use Ginkgo (BDD-style Go testing framework). Refer to
Expand Down Expand Up @@ -208,7 +222,11 @@ var _ = BeforeSuite(func() {
Expect(cfg).NotTo(BeNil())

scheme := apimachineryruntime.NewScheme()
{{- if .ExternalAPI }}
err = {{- .Resource.ImportAlias }}.AddToScheme(scheme)
{{- else }}
err = AddToScheme(scheme)
{{- end }}
Expect(err).NotTo(HaveOccurred())

err = %s.AddToScheme(scheme)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ type WebhookTest struct { // nolint:maligned
machinery.BoilerplateMixin
machinery.ResourceMixin

ExternalAPI bool

Force bool
}

Expand Down Expand Up @@ -77,17 +79,28 @@ package {{ .Resource.Version }}
import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
{{- if .ExternalAPI }}
{{ .Resource.ImportAlias }} "{{ .Resource.Path }}"
{{- end }}

// TODO (user): Add any additional imports if needed
)

var _ = Describe("{{ .Resource.Kind }} Webhook", func() {
var (
{{- if .ExternalAPI }}
obj *{{ .Resource.ImportAlias }}.{{ .Resource.Kind }}
{{- else }}
obj *{{ .Resource.Kind }}
{{- end }}
)

BeforeEach(func() {
{{- if .ExternalAPI }}
obj = &{{ .Resource.ImportAlias }}.{{ .Resource.Kind }}{}
{{- else }}
obj = &{{ .Resource.Kind }}{}
{{- end }}
Expect(obj).NotTo(BeNil(), "Expected obj to be initialized")

// TODO (user): Add any setup logic common to all tests
Expand All @@ -106,7 +119,11 @@ Context("When creating {{ .Resource.Kind }} under Conversion Webhook", func() {
// TODO (user): Add logic to convert the object to the desired version and verify the conversion
// Example:
// It("Should convert the object correctly", func() {
{{- if .ExternalAPI }}
// convertedObj := &{{ .Resource.ImportAlias }}.{{ .Resource.Kind }}{}
{{- else }}
// convertedObj := &{{ .Resource.Kind }}{}
{{- end }}
// Expect(obj.ConvertTo(convertedObj)).To(Succeed())
// Expect(convertedObj).ToNot(BeNil())
// })
Expand Down
19 changes: 18 additions & 1 deletion pkg/plugins/golang/v4/scaffolds/internal/templates/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"path/filepath"

"sigs.k8s.io/kubebuilder/v4/pkg/machinery"
"sigs.k8s.io/kubebuilder/v4/pkg/model/resource"
)

const defaultMainPath = "cmd/main.go"
Expand Down Expand Up @@ -62,6 +63,7 @@ type MainUpdater struct { //nolint:maligned

// Flags to indicate which parts need to be included when updating the file
WireResource, WireController, WireWebhook bool
ExternalAPI bool
}

// GetPath implements file.Builder
Expand Down Expand Up @@ -122,6 +124,15 @@ const (
}
}
`

webhookExternalSetupCodeFragment = `// nolint:goconst
if os.Getenv("ENABLE_WEBHOOKS") != "false" {
if err = %s.SetupWebhookFor%sWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create webhook", "webhook", "%s")
os.Exit(1)
}
}
`
)

// GetCodeFragments implements file.Inserter
Expand Down Expand Up @@ -165,10 +176,16 @@ func (f *MainUpdater) GetCodeFragments() machinery.CodeFragmentsMap {
f.Resource.PackageName(), f.Resource.Kind, f.Resource.Kind))
}
}
if f.WireWebhook {
if f.WireWebhook && !f.ExternalAPI {
setup = append(setup, fmt.Sprintf(webhookSetupCodeFragment,
f.Resource.ImportAlias(), f.Resource.Kind, f.Resource.Kind))
}
if f.WireWebhook && f.ExternalAPI {
path := resource.APIPackagePath(f.Repo, f.Resource.Group, f.Resource.Version, f.MultiGroup)
imports = append(imports, fmt.Sprintf(apiImportCodeFragment, f.Resource.ImportAlias(), path))
setup = append(setup, fmt.Sprintf(webhookExternalSetupCodeFragment,
f.Resource.ImportAlias(), f.Resource.Kind, f.Resource.Kind))
}

// Only store code fragments in the map if the slices are non-empty
if len(imports) != 0 {
Expand Down
11 changes: 7 additions & 4 deletions pkg/plugins/golang/v4/scaffolds/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package scaffolds

import (
"fmt"
"strings"

log "github.com/sirupsen/logrus"
"github.com/spf13/afero"
Expand Down Expand Up @@ -85,11 +86,13 @@ func (s *webhookScaffolder) Scaffold() error {
return fmt.Errorf("error updating resource: %w", err)
}

externalAPI := !strings.HasPrefix(s.resource.Path, s.config.GetRepository())

if err := scaffold.Execute(
&api.Webhook{Force: s.force},
&api.Webhook{Force: s.force, ExternalAPI: externalAPI},
&e2e.WebhookTestUpdater{WireWebhook: true},
&templates.MainUpdater{WireWebhook: true},
&api.WebhookTest{Force: s.force},
&templates.MainUpdater{WireWebhook: true, ExternalAPI: externalAPI},
&api.WebhookTest{Force: s.force, ExternalAPI: externalAPI},
); err != nil {
return err
}
Expand All @@ -102,7 +105,7 @@ You need to implement the conversion.Hub and conversion.Convertible interfaces f
// TODO: Add test suite for conversion webhook after #1664 has been merged & conversion tests supported in envtest.
if doDefaulting || doValidation {
if err := scaffold.Execute(
&api.WebhookSuite{K8SVersion: EnvtestK8SVersion},
&api.WebhookSuite{K8SVersion: EnvtestK8SVersion, ExternalAPI: externalAPI},
); err != nil {
return err
}
Expand Down
13 changes: 9 additions & 4 deletions pkg/plugins/golang/v4/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,17 @@ func (p *createWebhookSubcommand) InjectResource(res *resource.Resource) error {
" --programmatic-validation and --conversion to be true", p.commandName)
}

// check if resource exist to create webhook
if r, err := p.config.GetResource(p.resource.GVK); err != nil {
return fmt.Errorf("%s create webhook requires a previously created API ", p.commandName)
} else if r.Webhooks != nil && !r.Webhooks.IsEmpty() && !p.force {
if p.force {
return nil
}
Comment on lines +99 to +101
Copy link
Member

Choose a reason for hiding this comment

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

It is not the same of what was implemented before.


r, err := p.config.GetResource(p.resource.GVK)
if err == nil && r.Webhooks != nil && !r.Webhooks.IsEmpty() {
return fmt.Errorf("webhook resource already exists")
}
if err == nil && r.API == nil || err != nil && res.Path == "" {
return fmt.Errorf("%s create webhook requires a previously created API or a core API", p.commandName)
}

return nil
}
Expand Down