Skip to content

Decouple conversion webhook registration from leader election #678

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

Merged
sergenyalcin merged 1 commit into from
Jun 22, 2026
Merged

Decouple conversion webhook registration from leader election #678

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
18 changes: 9 additions & 9 deletions pkg/pipeline/templates/controller.go.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,15 @@ import (
{{ .Imports }}
)

// SetupWebhookWithManager registers the conversion webhook for {{ .CRD.Kind }}.
func SetupWebhookWithManager(mgr ctrl.Manager) error {

@ulucinar ulucinar Jun 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please see the comment below. We need to document the newly generated webhook setup functions and ask the provider authors to call them. Not all providers will have this bug (not all of them will be using CRD-gating). What could happen is that although they set the controller.Options.StartWebhooks to True, their webhooks won't be started. The good news is that they will get a change to see the changes (especially the removal of the previously generated condition section in Setup function for the webhooks and the newly generated SetupWebhookWithManager function) when they run make generate, so in my opinion, not a huge deal. But we had better still explain the changes and instruct the provider authors how to call the newly generated SetupWebhookWithManager_<API group> function(s) in some release notes. What do you think?

@erhancagirici erhancagirici Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

just to double-check: AFAIU, in "gated" setups, when there are MRAPs disabling a particular Kind, this will register its webhook unconditionally, although the CRD won't exist in cluster.

I think this will still work with controller-runtime, but we should test it to make sure

@sergenyalcin sergenyalcin Jun 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll add an instruction to the Release Notes.

@erhancagirici, I've tested this and the webhook registration doesn't be an issue for the unregistered CRDs or controller-runtime.

if err := ctrl.NewWebhookManagedBy(mgr, &{{ .TypePackageAlias }}{{ .CRD.Kind }}{}).
Complete(); err != nil {
return errors.Wrap(err, "cannot register webhook for the kind {{ .TypePackageAlias }}{{ .CRD.Kind }}")

@ulucinar ulucinar Jun 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In case the same (version, kind) exists in a different group:

Suggested change
return errors.Wrap(err, "cannot register webhook for the kind {{ .TypePackageAlias }}{{ .CRD.Kind }}")
return errors.Wrapf(err, "cannot register webhook for the kind %s", {{ .TypePackageAlias }}{{ .CRD.Kind }}_GroupVersionKind.String())

}
return nil
}

// SetupGated adds a controller that reconciles {{ .CRD.Kind }} managed resources.
func SetupGated(mgr ctrl.Manager, o tjcontroller.Options) error {
o.Options.Gate.Register(func() {
Expand Down Expand Up @@ -122,15 +131,6 @@ func Setup(mgr ctrl.Manager, o tjcontroller.Options) error {
opts = append(opts, managed.WithMetricRecorder(o.MetricOptions.MRMetrics))
}

// register webhooks for the kind {{ .TypePackageAlias }}{{ .CRD.Kind }}
// if they're enabled.
if o.StartWebhooks {

@ulucinar ulucinar Jun 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One concern here is some providers out in the wild might still be relying on this condition (and using this template shipped with upjet). Maybe we need to document the change in the release notes.

@erhancagirici erhancagirici Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: newly introduced SetupWebhookWithManager(mgr ctrl.Manager) can accept o tjcontroller.Options, keeping the logic intact

@sergenyalcin sergenyalcin Jun 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll add a detailed section about this change to the Release Notes.

if err := ctrl.NewWebhookManagedBy(mgr, &{{ .TypePackageAlias }}{{ .CRD.Kind }}{}).
Complete(); err != nil {
return errors.Wrap(err, "cannot register webhook for the kind {{ .TypePackageAlias }}{{ .CRD.Kind }}")
}
}

if o.MetricOptions != nil && o.MetricOptions.MRStateMetrics != nil {
stateMetricsRecorder := statemetrics.NewMRStateRecorder(
mgr.GetClient(), o.Logger, o.MetricOptions.MRStateMetrics, &{{ .TypePackageAlias }}{{ .CRD.Kind }}List{}, o.MetricOptions.PollStateMetricInterval,
Expand Down
14 changes: 14 additions & 0 deletions pkg/pipeline/templates/setup.go.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,18 @@ func SetupGated{{ .Group }}(mgr ctrl.Manager, o controller.Options) error {
}
}
return nil
}

// SetupWebhookWithManager{{ .Group }} registers conversion webhooks for all resource kinds in the group.
func SetupWebhookWithManager{{ .Group }}(mgr ctrl.Manager) error {
for _, setup := range []func(ctrl.Manager) error{
{{- range $alias := .Aliases }}
{{ $alias }}SetupWebhookWithManager,

@erhancagirici erhancagirici Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: another alternative could be to add {{ $alias }}SetupWebhookWithManager, into Setup_<apiGroup> and SetupGated_<apiGroup> templates:
this would:

  • keep Setup_<apiGroup> logically the same
  • decouple Webhook registration for SetupGated_<apiGroup>, the actual setup is still gated, webhooks are registered without gate.
  • no extra call to SetupWebhookWithManager_<apiGroup> required from provider developers

On the other hand, the current approach has the advantage of keeping webhook registration clean and explicit.

@sergenyalcin sergenyalcin Jun 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, this was an option, but I preferred it because of the clear separation.

{{- end }}
} {
if err := setup(mgr); err != nil {
return err
}
}
return nil
}
Loading