Skip to content

OCPBUGS-85580: Fix webhook TLS failure after service-ca to self-managed cert migration #8504

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
openshift-merge-bot merged 1 commit into from
May 14, 2026
+101 −9
Merged

OCPBUGS-85580: Fix webhook TLS failure after service-ca to self-managed cert migration #8504

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
49 changes: 45 additions & 4 deletions hypershift-operator/controllers/webhookcerts/webhookcerts_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ const (
// service-ca annotations placed on secrets it creates.
originatingServiceBetaAnnotation = "service.beta.openshift.io/originating-service-name"
originatingServiceAlphaAnnotation = "service.alpha.openshift.io/originating-service-name"

// service-ca annotation on webhook configs that triggers CA bundle injection.
injectCABundleAnnotation = "service.beta.openshift.io/inject-cabundle"

webhookConfigName = "hypershift.openshift.io"
)

// WebhookCertReconciler reconciles the self-managed webhook CA and serving cert.
Expand Down Expand Up @@ -172,14 +177,44 @@ func (r *WebhookCertReconciler) patchCRDsCABundle(ctx context.Context, caBundle
return nil
}

// removeInjectCABundleAnnotation removes the service-ca inject-cabundle annotation from
// webhook configurations so the service-ca operator stops overwriting our self-managed caBundle.
func (r *WebhookCertReconciler) removeInjectCABundleAnnotation(ctx context.Context, log logr.Logger) error {
mwc := &admissionregistrationv1.MutatingWebhookConfiguration{}
if err := r.Client.Get(ctx, client.ObjectKey{Name: webhookConfigName}, mwc); err != nil {
if !apierrors.IsNotFound(err) {
return fmt.Errorf("failed to get MutatingWebhookConfiguration: %w", err)
}
} else if _, ok := mwc.Annotations[injectCABundleAnnotation]; ok {
delete(mwc.Annotations, injectCABundleAnnotation)
if err := r.Client.Update(ctx, mwc); err != nil {
return fmt.Errorf("failed to remove %s annotation from MutatingWebhookConfiguration: %w", injectCABundleAnnotation, err)
}
log.Info("Removed inject-cabundle annotation from MutatingWebhookConfiguration")
}

vwc := &admissionregistrationv1.ValidatingWebhookConfiguration{}
if err := r.Client.Get(ctx, client.ObjectKey{Name: webhookConfigName}, vwc); err != nil {
if !apierrors.IsNotFound(err) {
return fmt.Errorf("failed to get ValidatingWebhookConfiguration: %w", err)
}
} else if _, ok := vwc.Annotations[injectCABundleAnnotation]; ok {
delete(vwc.Annotations, injectCABundleAnnotation)
if err := r.Client.Update(ctx, vwc); err != nil {
return fmt.Errorf("failed to remove %s annotation from ValidatingWebhookConfiguration: %w", injectCABundleAnnotation, err)
}
log.Info("Removed inject-cabundle annotation from ValidatingWebhookConfiguration")
}

return nil
}

// patchWebhookConfigsCABundle patches the caBundle on MutatingWebhookConfiguration and ValidatingWebhookConfiguration
// resources named hypershift.openshift.io.
func (r *WebhookCertReconciler) patchWebhookConfigsCABundle(ctx context.Context, caBundle []byte) error {
webhookName := "hypershift.openshift.io"

// Patch MutatingWebhookConfiguration
mwc := &admissionregistrationv1.MutatingWebhookConfiguration{}
if err := r.Client.Get(ctx, client.ObjectKey{Name: webhookName}, mwc); err != nil {
if err := r.Client.Get(ctx, client.ObjectKey{Name: webhookConfigName}, mwc); err != nil {
if !apierrors.IsNotFound(err) {
return fmt.Errorf("failed to get MutatingWebhookConfiguration: %w", err)
}
Expand All @@ -200,7 +235,7 @@ func (r *WebhookCertReconciler) patchWebhookConfigsCABundle(ctx context.Context,

// Patch ValidatingWebhookConfiguration
vwc := &admissionregistrationv1.ValidatingWebhookConfiguration{}
if err := r.Client.Get(ctx, client.ObjectKey{Name: webhookName}, vwc); err != nil {
if err := r.Client.Get(ctx, client.ObjectKey{Name: webhookConfigName}, vwc); err != nil {
if !apierrors.IsNotFound(err) {
return fmt.Errorf("failed to get ValidatingWebhookConfiguration: %w", err)
}
Expand Down Expand Up @@ -247,6 +282,12 @@ func (r *WebhookCertReconciler) removeServiceCAResources(ctx context.Context, lo
}
}

// Remove the inject-cabundle annotation from webhook configurations so the
// service-ca operator stops overwriting the caBundle we manage ourselves.
if err := r.removeInjectCABundleAnnotation(ctx, log); err != nil {
return err
}

// If the existing serving cert secret was created by service-ca, delete it
// so we can recreate it signed by our self-managed CA.
existingSecret := &corev1.Secret{}
Expand Down
61 changes: 56 additions & 5 deletions hypershift-operator/controllers/webhookcerts/webhookcerts_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func TestReconcile(t *testing.T) {
crd := &apiextensionsv1.CustomResourceDefinition{
ObjectMeta: metav1.ObjectMeta{Name: "hostedclusters.hypershift.openshift.io"},
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
Group: "hypershift.openshift.io",
Group: webhookConfigName,
Names: apiextensionsv1.CustomResourceDefinitionNames{
Plural: "hostedclusters",
Singular: "hostedcluster",
Expand Down Expand Up @@ -181,7 +181,7 @@ func TestReconcile(t *testing.T) {
g := NewWithT(t)

mwc := &admissionregistrationv1.MutatingWebhookConfiguration{
ObjectMeta: metav1.ObjectMeta{Name: "hypershift.openshift.io"},
ObjectMeta: metav1.ObjectMeta{Name: webhookConfigName},
Webhooks: []admissionregistrationv1.MutatingWebhook{
{
Name: "defaulting.hypershift.openshift.io",
Expand All @@ -192,7 +192,7 @@ func TestReconcile(t *testing.T) {
},
}
vwc := &admissionregistrationv1.ValidatingWebhookConfiguration{
ObjectMeta: metav1.ObjectMeta{Name: "hypershift.openshift.io"},
ObjectMeta: metav1.ObjectMeta{Name: webhookConfigName},
Webhooks: []admissionregistrationv1.ValidatingWebhook{
{
Name: "validating.hypershift.openshift.io",
Expand All @@ -210,12 +210,12 @@ func TestReconcile(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())

updatedMWC := &admissionregistrationv1.MutatingWebhookConfiguration{}
g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: "hypershift.openshift.io"}, updatedMWC)).To(Succeed())
g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: webhookConfigName}, updatedMWC)).To(Succeed())
g.Expect(updatedMWC.Webhooks[0].ClientConfig.CABundle).ToNot(Equal([]byte("old")))
g.Expect(updatedMWC.Webhooks[0].ClientConfig.CABundle).ToNot(BeEmpty())

updatedVWC := &admissionregistrationv1.ValidatingWebhookConfiguration{}
g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: "hypershift.openshift.io"}, updatedVWC)).To(Succeed())
g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: webhookConfigName}, updatedVWC)).To(Succeed())
g.Expect(updatedVWC.Webhooks[0].ClientConfig.CABundle).ToNot(Equal([]byte("old")))
g.Expect(updatedVWC.Webhooks[0].ClientConfig.CABundle).ToNot(BeEmpty())
})
Expand Down Expand Up @@ -314,6 +314,57 @@ func TestReconcile(t *testing.T) {
g.Expect(caSecret.Data).To(HaveKey(certs.CASignerCertMapKey))
})

t.Run("When upgrading from service-ca it should remove inject-cabundle annotation from webhook configurations", func(t *testing.T) {
g := NewWithT(t)

mwc := &admissionregistrationv1.MutatingWebhookConfiguration{
ObjectMeta: metav1.ObjectMeta{
Name: webhookConfigName,
Copy link
Copy Markdown
Member

@cblecker cblecker May 13, 2026

Choose a reason for hiding this comment

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

nit: Since you introduced webhookConfigName, it might be worth updating the existing tests in this file to use it too — lines 184, 194, 213, and 218 still hardcode "hypershift.openshift.io". No big deal either way.

Copy link
Copy Markdown
Contributor Author

@joshbranham joshbranham May 13, 2026

Choose a reason for hiding this comment

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

Good call, silly robot missed that. Fixed, needs another lgtm if you are up for it.

Annotations: map[string]string{
injectCABundleAnnotation: "true",
},
},
Webhooks: []admissionregistrationv1.MutatingWebhook{
{
Name: "defaulting.hypershift.openshift.io",
ClientConfig: admissionregistrationv1.WebhookClientConfig{CABundle: []byte("old")},
SideEffects: sideEffectNone(),
AdmissionReviewVersions: []string{"v1"},
},
},
}
vwc := &admissionregistrationv1.ValidatingWebhookConfiguration{
ObjectMeta: metav1.ObjectMeta{
Name: webhookConfigName,
Annotations: map[string]string{
injectCABundleAnnotation: "true",
},
},
Webhooks: []admissionregistrationv1.ValidatingWebhook{
{
Name: "validating.hypershift.openshift.io",
ClientConfig: admissionregistrationv1.WebhookClientConfig{CABundle: []byte("old")},
SideEffects: sideEffectNone(),
AdmissionReviewVersions: []string{"v1"},
},
},
}

cl := fake.NewClientBuilder().WithScheme(newScheme(t)).WithObjects(mwc, vwc).Build()
r := newReconciler(cl)

_, err := r.Reconcile(t.Context(), caRequest())
g.Expect(err).ToNot(HaveOccurred())

updatedMWC := &admissionregistrationv1.MutatingWebhookConfiguration{}
g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: webhookConfigName}, updatedMWC)).To(Succeed())
g.Expect(updatedMWC.Annotations).ToNot(HaveKey(injectCABundleAnnotation))

updatedVWC := &admissionregistrationv1.ValidatingWebhookConfiguration{}
g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: webhookConfigName}, updatedVWC)).To(Succeed())
g.Expect(updatedVWC.Annotations).ToNot(HaveKey(injectCABundleAnnotation))
})

t.Run("When the serving cert was not created by service-ca it should not delete it", func(t *testing.T) {
g := NewWithT(t)

Expand Down