diff --git a/cmd/install/assets/hypershift_operator.go b/cmd/install/assets/hypershift_operator.go index d2af98020306..099db103dcec 100644 --- a/cmd/install/assets/hypershift_operator.go +++ b/cmd/install/assets/hypershift_operator.go @@ -489,6 +489,7 @@ type HyperShiftOperatorDeployment struct { EnableCIDebugOutput bool EnableWebhook bool EnableValidatingWebhook bool + EnableWebhookCertReconciler bool PrivatePlatform string AWSPrivateSecret *corev1.Secret AWSPrivateSecretKey string @@ -865,6 +866,9 @@ func (o HyperShiftOperatorDeployment) addWebhookResources(args *[]string, volume if o.EnableValidatingWebhook { *args = append(*args, "--enable-validating-webhook=true") } + if !o.EnableWebhookCertReconciler { + *args = append(*args, "--enable-webhook-cert-reconciler=false") + } } func (o HyperShiftOperatorDeployment) addOIDCResources(args *[]string, volumeMounts *[]corev1.VolumeMount, volumes *[]corev1.Volume) { diff --git a/cmd/install/assets/hypershift_operator_test.go b/cmd/install/assets/hypershift_operator_test.go index 7dcca5ccf9a2..a0b9986e115e 100644 --- a/cmd/install/assets/hypershift_operator_test.go +++ b/cmd/install/assets/hypershift_operator_test.go @@ -1004,12 +1004,14 @@ func TestBuildEnvVars(t *testing.T) { func TestAddWebhookResources(t *testing.T) { tests := []struct { - name string - enableWebhook bool - enableValidatingWebhook bool - expectArgs []string - expectVolumeMountCount int - expectVolumeCount int + name string + enableWebhook bool + enableValidatingWebhook bool + enableWebhookCertReconciler bool + expectArgs []string + notExpectArgs []string + expectVolumeMountCount int + expectVolumeCount int }{ { name: "When webhook is disabled, it should not add any resources", @@ -1018,19 +1020,31 @@ func TestAddWebhookResources(t *testing.T) { expectVolumeCount: 0, }, { - name: "When webhook is enabled without validating webhook, it should add serving-cert resources and cert-dir arg", - enableWebhook: true, - expectArgs: []string{"--cert-dir=/var/run/secrets/serving-cert"}, - expectVolumeMountCount: 1, - expectVolumeCount: 1, + name: "When webhook is enabled without validating webhook, it should add serving-cert resources and cert-dir arg", + enableWebhook: true, + enableWebhookCertReconciler: true, + expectArgs: []string{"--cert-dir=/var/run/secrets/serving-cert"}, + notExpectArgs: []string{"--enable-webhook-cert-reconciler=false"}, + expectVolumeMountCount: 1, + expectVolumeCount: 1, }, { - name: "When webhook and validating webhook are both enabled, it should add cert-dir and enable-validating-webhook args", - enableWebhook: true, - enableValidatingWebhook: true, - expectArgs: []string{"--cert-dir=/var/run/secrets/serving-cert", "--enable-validating-webhook=true"}, - expectVolumeMountCount: 1, - expectVolumeCount: 1, + name: "When webhook and validating webhook are both enabled, it should add cert-dir and enable-validating-webhook args", + enableWebhook: true, + enableValidatingWebhook: true, + enableWebhookCertReconciler: true, + expectArgs: []string{"--cert-dir=/var/run/secrets/serving-cert", "--enable-validating-webhook=true"}, + notExpectArgs: []string{"--enable-webhook-cert-reconciler=false"}, + expectVolumeMountCount: 1, + expectVolumeCount: 1, + }, + { + name: "When webhook cert reconciler is disabled, it should add the flag to args", + enableWebhook: true, + enableWebhookCertReconciler: false, + expectArgs: []string{"--cert-dir=/var/run/secrets/serving-cert", "--enable-webhook-cert-reconciler=false"}, + expectVolumeMountCount: 1, + expectVolumeCount: 1, }, } @@ -1038,8 +1052,9 @@ func TestAddWebhookResources(t *testing.T) { t.Run(tc.name, func(t *testing.T) { g := NewGomegaWithT(t) d := HyperShiftOperatorDeployment{ - EnableWebhook: tc.enableWebhook, - EnableValidatingWebhook: tc.enableValidatingWebhook, + EnableWebhook: tc.enableWebhook, + EnableValidatingWebhook: tc.enableValidatingWebhook, + EnableWebhookCertReconciler: tc.enableWebhookCertReconciler, } var args []string var volumeMounts []corev1.VolumeMount @@ -1052,6 +1067,9 @@ func TestAddWebhookResources(t *testing.T) { for _, expected := range tc.expectArgs { g.Expect(args).To(ContainElement(expected)) } + for _, notExpected := range tc.notExpectArgs { + g.Expect(args).NotTo(ContainElement(notExpected)) + } }) } } diff --git a/cmd/install/install.go b/cmd/install/install.go index 2afbf6609000..07b8256e43d6 100644 --- a/cmd/install/install.go +++ b/cmd/install/install.go @@ -155,6 +155,7 @@ type Options struct { ScaleFromZeroCreds string ScaleFromZeroCredentialsSecret string ScaleFromZeroCredentialsSecretKey string + EnableWebhookCertReconciler bool RenderSensitive bool } @@ -377,6 +378,7 @@ func NewCommand() *cobra.Command { cmd.PersistentFlags().BoolVar(&opts.EnableDefaultingWebhook, "enable-defaulting-webhook", opts.EnableDefaultingWebhook, "Enable webhook for defaulting hypershift API types") cmd.PersistentFlags().BoolVar(&opts.EnableValidatingWebhook, "enable-validating-webhook", opts.EnableValidatingWebhook, "Enable webhook for validating hypershift API types") cmd.PersistentFlags().BoolVar(&opts.EnableConversionWebhook, "enable-conversion-webhook", opts.EnableConversionWebhook, "Enable webhook for converting hypershift API types") + cmd.PersistentFlags().BoolVar(&opts.EnableWebhookCertReconciler, "enable-webhook-cert-reconciler", opts.EnableWebhookCertReconciler, "If true, the operator manages webhook TLS certificates. Set to false when an external certificate manager (e.g. cert-manager) manages the serving cert") cmd.PersistentFlags().BoolVar(&opts.ExcludeEtcdManifests, "exclude-etcd", opts.ExcludeEtcdManifests, "Leave out etcd manifests") cmd.PersistentFlags().Var(&opts.PlatformMonitoring, "platform-monitoring", "Select an option for enabling platform cluster monitoring. Valid values are: None, OperatorOnly, All") cmd.PersistentFlags().BoolVar(&opts.EnableCIDebugOutput, "enable-ci-debug-output", opts.EnableCIDebugOutput, "If extra CI debug output should be enabled") @@ -536,6 +538,7 @@ func NewInstallOptionsWithDefaults() Options { opts.EnableConversionWebhook = true opts.EnableDedicatedRequestServingIsolation = true opts.EnableDefaultingWebhook = false + opts.EnableWebhookCertReconciler = true opts.EnableEtcdRecovery = true opts.EnableSizeTagging = false opts.EnableValidatingWebhook = false @@ -1259,6 +1262,7 @@ func setupOperatorResources(opts Options, userCABundleCM *corev1.ConfigMap, trus EnableCIDebugOutput: opts.EnableCIDebugOutput, EnableWebhook: opts.EnableDefaultingWebhook || opts.EnableConversionWebhook || opts.EnableValidatingWebhook || opts.EnableAuditLogPersistence, EnableValidatingWebhook: opts.EnableValidatingWebhook, + EnableWebhookCertReconciler: opts.EnableWebhookCertReconciler, PrivatePlatform: opts.PrivatePlatform, AWSPrivateRegion: opts.AWSPrivateRegion, GCPProject: opts.GCPProject, diff --git a/hypershift-operator/controllers/webhookcerts/webhookcerts_controller.go b/hypershift-operator/controllers/webhookcerts/webhookcerts_controller.go index b2d3c7652b4a..e0fa6a82098d 100644 --- a/hypershift-operator/controllers/webhookcerts/webhookcerts_controller.go +++ b/hypershift-operator/controllers/webhookcerts/webhookcerts_controller.go @@ -62,10 +62,15 @@ const ( // WebhookCertReconciler reconciles the self-managed webhook CA and serving cert. // It is used on non-OpenShift clusters where the service-ca operator is not available. +// +// When ManageCerts is false, the reconciler runs in caBundle-only mode: it reads the +// CA from the serving cert secret's ca.crt key and patches CRDs and webhook configurations, +// but does not create or manage the CA or serving cert secrets. type WebhookCertReconciler struct { Client client.Client Namespace string ServiceName string + ManageCerts bool createOrUpdate upsert.CreateOrUpdateFN } @@ -73,28 +78,57 @@ func (r *WebhookCertReconciler) SetupWithManager(mgr ctrl.Manager, createOrUpdat r.Client = mgr.GetClient() r.createOrUpdate = createOrUpdate.CreateOrUpdate + secretFilter := func(o client.Object) bool { + if o.GetNamespace() != r.Namespace { + return false + } + if r.ManageCerts { + return o.GetName() == CASecretName || o.GetName() == ServingCertSecretName + } + return o.GetName() == ServingCertSecretName + } + return ctrl.NewControllerManagedBy(mgr). Named("webhookcerts"). - For(&corev1.Secret{}, builder.WithPredicates(predicate.NewPredicateFuncs(func(o client.Object) bool { - return o.GetNamespace() == r.Namespace && - (o.GetName() == CASecretName || o.GetName() == ServingCertSecretName) - }))). + For(&corev1.Secret{}, builder.WithPredicates(predicate.NewPredicateFuncs(secretFilter))). Complete(r) } func (r *WebhookCertReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := ctrl.LoggerFrom(ctx) - // 0. Handle upgrade from service-ca managed certs. - // On existing OpenShift clusters, the service-ca operator may have created the - // serving cert secret and annotated the Service. We must remove these before - // reconciling our own certs, otherwise service-ca will keep overwriting the secret - // with a cert signed by a different CA than the one we inject into webhook configs. - if err := r.removeServiceCAResources(ctx, log); err != nil { + var ( + caBundle []byte + err error + ) + if r.ManageCerts { + caBundle, err = r.reconcileCerts(ctx, log) + } else { + caBundle, err = r.readCABundleFromServingCert(ctx) + } + if err != nil { return ctrl.Result{}, err } - // 1. Reconcile the self-signed CA. + if err := r.patchCRDsCABundle(ctx, caBundle); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to patch CRD caBundle: %w", err) + } + + if err := r.patchWebhookConfigsCABundle(ctx, caBundle); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to patch webhook config caBundle: %w", err) + } + + log.Info("Webhook certs reconciled", "manageCerts", r.ManageCerts, "requeueAfter", requeueInterval) + return ctrl.Result{RequeueAfter: requeueInterval}, nil +} + +func (r *WebhookCertReconciler) reconcileCerts(ctx context.Context, log logr.Logger) ([]byte, error) { + // Handle upgrade from service-ca managed certs. + if err := r.removeServiceCAResources(ctx, log); err != nil { + return nil, err + } + + // Reconcile the self-signed CA. caSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: CASecretName, @@ -105,10 +139,10 @@ func (r *WebhookCertReconciler) Reconcile(ctx context.Context, req ctrl.Request) caSecret.Type = corev1.SecretTypeOpaque return certs.ReconcileSelfSignedCA(caSecret, "hypershift-webhook-ca", "openshift") }); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to reconcile webhook CA secret: %w", err) + return nil, fmt.Errorf("failed to reconcile webhook CA secret: %w", err) } - // 2. Reconcile the serving cert signed by the CA. + // Reconcile the serving cert signed by the CA. dnsNames := webhookDNSNames(r.ServiceName, r.Namespace) servingSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -131,22 +165,24 @@ func (r *WebhookCertReconciler) Reconcile(ctx context.Context, req ctrl.Request) nil, ) }); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to reconcile webhook serving cert: %w", err) + return nil, fmt.Errorf("failed to reconcile webhook serving cert: %w", err) } - // 3. Patch caBundle on CRDs with conversion webhooks. - caBundle := caSecret.Data[certs.CASignerCertMapKey] - if err := r.patchCRDsCABundle(ctx, caBundle); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to patch CRD caBundle: %w", err) - } + return caSecret.Data[certs.CASignerCertMapKey], nil +} - // 4. Patch caBundle on webhook configurations. - if err := r.patchWebhookConfigsCABundle(ctx, caBundle); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to patch webhook config caBundle: %w", err) +// readCABundleFromServingCert reads the CA bundle from the serving cert secret's ca.crt key, +// as populated by an external certificate manager (e.g. cert-manager). +func (r *WebhookCertReconciler) readCABundleFromServingCert(ctx context.Context) ([]byte, error) { + servingSecret := &corev1.Secret{} + if err := r.Client.Get(ctx, client.ObjectKey{Namespace: r.Namespace, Name: ServingCertSecretName}, servingSecret); err != nil { + return nil, fmt.Errorf("failed to get serving cert secret: %w", err) } - - log.Info("Webhook certs reconciled", "requeueAfter", requeueInterval) - return ctrl.Result{RequeueAfter: requeueInterval}, nil + caBundle, ok := servingSecret.Data[certs.CASignerCertMapKey] + if !ok || len(caBundle) == 0 { + return nil, fmt.Errorf("serving cert secret %s/%s does not contain a %s key", r.Namespace, ServingCertSecretName, certs.CASignerCertMapKey) + } + return caBundle, nil } // patchCRDsCABundle patches the caBundle on all CRDs whose conversion webhook points to our service. diff --git a/hypershift-operator/controllers/webhookcerts/webhookcerts_controller_test.go b/hypershift-operator/controllers/webhookcerts/webhookcerts_controller_test.go index ec96cf3dd5b1..e5ad8caa4e71 100644 --- a/hypershift-operator/controllers/webhookcerts/webhookcerts_controller_test.go +++ b/hypershift-operator/controllers/webhookcerts/webhookcerts_controller_test.go @@ -36,12 +36,22 @@ func newReconciler(cl client.Client) *WebhookCertReconciler { Client: cl, Namespace: "hypershift", ServiceName: "operator", + ManageCerts: true, createOrUpdate: func(ctx context.Context, c client.Client, obj client.Object, f controllerutil.MutateFn) (controllerutil.OperationResult, error) { return controllerutil.CreateOrUpdate(ctx, c, obj, f) }, } } +func newCABundleOnlyReconciler(cl client.Client) *WebhookCertReconciler { + return &WebhookCertReconciler{ + Client: cl, + Namespace: "hypershift", + ServiceName: "operator", + ManageCerts: false, + } +} + func caRequest() ctrl.Request { return ctrl.Request{NamespacedName: client.ObjectKey{Name: CASecretName, Namespace: "hypershift"}} } @@ -422,6 +432,108 @@ func TestWebhookDNSNames(t *testing.T) { }) } +func TestReconcileCABundleOnly(t *testing.T) { + t.Run("When serving cert has ca.crt it should patch webhook configurations", func(t *testing.T) { + g := NewWithT(t) + + servingSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: ServingCertSecretName, + Namespace: "hypershift", + }, + Type: corev1.SecretTypeTLS, + Data: map[string][]byte{ + corev1.TLSCertKey: []byte("cert-data"), + corev1.TLSPrivateKeyKey: []byte("key-data"), + certs.CASignerCertMapKey: []byte("external-ca-bundle"), + }, + } + mwc := &admissionregistrationv1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: webhookConfigName}, + Webhooks: []admissionregistrationv1.MutatingWebhook{ + { + Name: "defaulting.hypershift.openshift.io", + ClientConfig: admissionregistrationv1.WebhookClientConfig{CABundle: []byte("old")}, + SideEffects: sideEffectNone(), + AdmissionReviewVersions: []string{"v1"}, + }, + }, + } + + cl := fake.NewClientBuilder().WithScheme(newScheme(t)).WithObjects(servingSecret, mwc).Build() + r := newCABundleOnlyReconciler(cl) + + result, err := r.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKey{Name: ServingCertSecretName, Namespace: "hypershift"}}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result.RequeueAfter).To(Equal(12 * time.Hour)) + + updatedMWC := &admissionregistrationv1.MutatingWebhookConfiguration{} + g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: webhookConfigName}, updatedMWC)).To(Succeed()) + g.Expect(updatedMWC.Webhooks[0].ClientConfig.CABundle).To(Equal([]byte("external-ca-bundle"))) + }) + + t.Run("When serving cert is missing it should return an error", func(t *testing.T) { + g := NewWithT(t) + + cl := fake.NewClientBuilder().WithScheme(newScheme(t)).Build() + r := newCABundleOnlyReconciler(cl) + + _, err := r.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKey{Name: ServingCertSecretName, Namespace: "hypershift"}}) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("failed to get serving cert secret")) + }) + + t.Run("When serving cert has no ca.crt key it should return an error", func(t *testing.T) { + g := NewWithT(t) + + servingSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: ServingCertSecretName, + Namespace: "hypershift", + }, + Type: corev1.SecretTypeTLS, + Data: map[string][]byte{ + corev1.TLSCertKey: []byte("cert-data"), + corev1.TLSPrivateKeyKey: []byte("key-data"), + }, + } + + cl := fake.NewClientBuilder().WithScheme(newScheme(t)).WithObjects(servingSecret).Build() + r := newCABundleOnlyReconciler(cl) + + _, err := r.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKey{Name: ServingCertSecretName, Namespace: "hypershift"}}) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("does not contain a ca.crt key")) + }) + + t.Run("When serving cert has ca.crt it should not create a CA secret", func(t *testing.T) { + g := NewWithT(t) + + servingSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: ServingCertSecretName, + Namespace: "hypershift", + }, + Type: corev1.SecretTypeTLS, + Data: map[string][]byte{ + corev1.TLSCertKey: []byte("cert-data"), + corev1.TLSPrivateKeyKey: []byte("key-data"), + certs.CASignerCertMapKey: []byte("external-ca-bundle"), + }, + } + + cl := fake.NewClientBuilder().WithScheme(newScheme(t)).WithObjects(servingSecret).Build() + r := newCABundleOnlyReconciler(cl) + + _, err := r.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKey{Name: ServingCertSecretName, Namespace: "hypershift"}}) + g.Expect(err).ToNot(HaveOccurred()) + + caSecret := &corev1.Secret{} + err = cl.Get(t.Context(), client.ObjectKey{Name: CASecretName, Namespace: "hypershift"}, caSecret) + g.Expect(err).To(HaveOccurred()) + }) +} + func sideEffectNone() *admissionregistrationv1.SideEffectClass { se := admissionregistrationv1.SideEffectClassNone return &se diff --git a/hypershift-operator/main.go b/hypershift-operator/main.go index 11e552f0a273..551326e5d76e 100644 --- a/hypershift-operator/main.go +++ b/hypershift-operator/main.go @@ -152,6 +152,7 @@ type StartOptions struct { EnableUWMTelemetryRemoteWrite bool EnableValidatingWebhook bool EnableDedicatedRequestServingIsolation bool + EnableWebhookCertReconciler bool ScaleFromZeroProvider string ScaleFromZeroCreds string EtcdBackupMaxCount int @@ -192,6 +193,7 @@ func NewStartCommand() *cobra.Command { cmd.Flags().BoolVar(&opts.EnableUWMTelemetryRemoteWrite, "enable-uwm-telemetry-remote-write", opts.EnableUWMTelemetryRemoteWrite, "If true, enables a controller that ensures user workload monitoring is enabled and that it is configured to remote write telemetry metrics from control planes") cmd.Flags().BoolVar(&opts.EnableValidatingWebhook, "enable-validating-webhook", false, "Enable webhook for validating hypershift API types") cmd.Flags().BoolVar(&opts.EnableDedicatedRequestServingIsolation, "enable-dedicated-request-serving-isolation", true, "If true, enables scheduling of request serving components to dedicated nodes") + cmd.Flags().BoolVar(&opts.EnableWebhookCertReconciler, "enable-webhook-cert-reconciler", true, "If true, the operator manages webhook TLS certificates. Set to false when an external certificate manager (e.g. cert-manager) manages the serving cert") cmd.Flags().StringVar(&opts.ScaleFromZeroProvider, "scale-from-zero-provider", opts.ScaleFromZeroProvider, "Platform type for scale-from-zero autoscaling (aws)") cmd.Flags().StringVar(&opts.ScaleFromZeroCreds, "scale-from-zero-creds", opts.ScaleFromZeroCreds, "Path to credentials file for scale-from-zero instance type queries") cmd.Flags().IntVar(&opts.EtcdBackupMaxCount, "etcd-backup-max-count", 5, "Maximum number of completed HCPEtcdBackup CRs to retain per HostedControlPlane") @@ -793,6 +795,7 @@ func setupSupportControllers(mgr ctrl.Manager, opts *StartOptions, mgmtClusterCa webhookCertReconciler := &webhookcerts.WebhookCertReconciler{ Namespace: opts.Namespace, ServiceName: "operator", + ManageCerts: opts.EnableWebhookCertReconciler, } if err := webhookCertReconciler.SetupWithManager(mgr, createOrUpdate); err != nil { return fmt.Errorf("unable to create webhook cert controller: %w", err)