diff --git a/cmd/install/assets/hypershift_operator.go b/cmd/install/assets/hypershift_operator.go index d2af9802030..3163fdcbaed 100644 --- a/cmd/install/assets/hypershift_operator.go +++ b/cmd/install/assets/hypershift_operator.go @@ -858,6 +858,7 @@ func (o HyperShiftOperatorDeployment) addWebhookResources(args *[]string, volume VolumeSource: corev1.VolumeSource{ Secret: &corev1.SecretVolumeSource{ SecretName: "manager-serving-cert", + Optional: ptr.To(true), }, }, }) @@ -1163,10 +1164,11 @@ func (o ExternalDNSPodMonitor) Build() *prometheusoperatorv1.PodMonitor { "name": ExternalDNSDeploymentName, }, }, - PodMetricsEndpoints: []prometheusoperatorv1.PodMetricsEndpoint{{ - Port: ptr.To("metrics"), - Interval: "30s", - }, + PodMetricsEndpoints: []prometheusoperatorv1.PodMetricsEndpoint{ + { + Port: ptr.To("metrics"), + Interval: "30s", + }, }, }, } @@ -2050,7 +2052,6 @@ func (o HyperShiftReaderClusterRoleBinding) Build() *rbacv1.ClusterRoleBinding { type HyperShiftMutatingWebhookConfiguration struct { Namespace *corev1.Namespace EnableAuditLogPersistence bool - CABundle []byte } const ( @@ -2089,7 +2090,7 @@ func (o HyperShiftMutatingWebhookConfiguration) Build() *admissionregistrationv1 }, }, ClientConfig: admissionregistrationv1.WebhookClientConfig{ - CABundle: o.CABundle, + CABundle: nil, Service: &admissionregistrationv1.ServiceReference{ Namespace: "hypershift", Name: "operator", @@ -2116,7 +2117,7 @@ func (o HyperShiftMutatingWebhookConfiguration) Build() *admissionregistrationv1 }, }, ClientConfig: admissionregistrationv1.WebhookClientConfig{ - CABundle: o.CABundle, + CABundle: nil, Service: &admissionregistrationv1.ServiceReference{ Namespace: "hypershift", Name: "operator", @@ -2157,7 +2158,7 @@ func (o HyperShiftMutatingWebhookConfiguration) Build() *admissionregistrationv1 }, }, ClientConfig: admissionregistrationv1.WebhookClientConfig{ - CABundle: o.CABundle, + CABundle: nil, Service: &admissionregistrationv1.ServiceReference{ Namespace: "hypershift", Name: "operator", @@ -2187,7 +2188,7 @@ func (o HyperShiftMutatingWebhookConfiguration) Build() *admissionregistrationv1 }, }, ClientConfig: admissionregistrationv1.WebhookClientConfig{ - CABundle: o.CABundle, + CABundle: nil, Service: &admissionregistrationv1.ServiceReference{ Namespace: "hypershift", Name: "operator", @@ -2208,7 +2209,6 @@ func (o HyperShiftMutatingWebhookConfiguration) Build() *admissionregistrationv1 type HyperShiftValidatingWebhookConfiguration struct { Namespace string - CABundle []byte } func (o HyperShiftValidatingWebhookConfiguration) Build() *admissionregistrationv1.ValidatingWebhookConfiguration { @@ -2245,7 +2245,7 @@ func (o HyperShiftValidatingWebhookConfiguration) Build() *admissionregistration }, }, ClientConfig: admissionregistrationv1.WebhookClientConfig{ - CABundle: o.CABundle, + CABundle: nil, Service: &admissionregistrationv1.ServiceReference{ Namespace: "hypershift", Name: "operator", @@ -2274,7 +2274,7 @@ func (o HyperShiftValidatingWebhookConfiguration) Build() *admissionregistration }, }, ClientConfig: admissionregistrationv1.WebhookClientConfig{ - CABundle: o.CABundle, + CABundle: nil, Service: &admissionregistrationv1.ServiceReference{ Namespace: "hypershift", Name: "operator", diff --git a/cmd/install/install.go b/cmd/install/install.go index 2afbf660900..6d9589a2aef 100644 --- a/cmd/install/install.go +++ b/cmd/install/install.go @@ -30,7 +30,6 @@ import ( crdassets "github.com/openshift/hypershift/cmd/install/assets/crds" "github.com/openshift/hypershift/cmd/util" "github.com/openshift/hypershift/hypershift-operator/controllers/sharedingress" - "github.com/openshift/hypershift/hypershift-operator/controllers/webhookcerts" hyperapi "github.com/openshift/hypershift/support/api" "github.com/openshift/hypershift/support/config" "github.com/openshift/hypershift/support/metrics" @@ -756,22 +755,10 @@ func hyperShiftOperatorManifests(ctx context.Context, client crclient.Client, op operatorServiceAccount, rbacObjs := setupRBAC(opts, operatorNamespace) objects = append(objects, rbacObjs...) - // Generate self-managed webhook CA and serving cert when any webhook is enabled. - var webhookCABundle []byte - if opts.EnableDefaultingWebhook || opts.EnableConversionWebhook || opts.EnableValidatingWebhook || opts.EnableAuditLogPersistence { - caSecret, servingSecret, caBundle, err := webhookcerts.GenerateInitialWebhookCerts(operatorNamespace.Name, assets.HypershiftOperatorName) - if err != nil { - return nil, nil, fmt.Errorf("failed to generate webhook certs: %w", err) - } - objects = append(objects, caSecret, servingSecret) - webhookCABundle = caBundle - } - if opts.EnableDefaultingWebhook || opts.EnableAuditLogPersistence { mutatingWebhookConfiguration := assets.HyperShiftMutatingWebhookConfiguration{ Namespace: operatorNamespace, EnableAuditLogPersistence: opts.EnableAuditLogPersistence, - CABundle: webhookCABundle, }.Build() objects = append(objects, mutatingWebhookConfiguration) } @@ -779,7 +766,6 @@ func hyperShiftOperatorManifests(ctx context.Context, client crclient.Client, op if opts.EnableValidatingWebhook { validatingWebhookConfiguration := assets.HyperShiftValidatingWebhookConfiguration{ Namespace: operatorNamespace.Name, - CABundle: webhookCABundle, }.Build() objects = append(objects, validatingWebhookConfiguration) } @@ -831,7 +817,7 @@ func hyperShiftOperatorManifests(ctx context.Context, client crclient.Client, op objects = append(objects, sharedIngressObjs...) } - crds, err = setupCRDs(ctx, client, opts, operatorNamespace, operatorService, webhookCABundle) + crds, err = setupCRDs(ctx, client, opts, operatorNamespace, operatorService) if err != nil { return nil, nil, err } @@ -874,7 +860,7 @@ var ipamCRDNames = set.New( // related to etcd are excluded from the list. If the option EnableConversionWebhook is set to true, the CRDs related // to hypershift.openshift.io group are annotated with the necessary annotations to enable the conversion webhook. // If a client is provided, IPAM CRDs that already exist in the cluster are skipped to avoid conflicts. -func setupCRDs(ctx context.Context, client crclient.Client, opts Options, operatorNamespace *corev1.Namespace, operatorService *corev1.Service, webhookCABundle []byte) ([]crclient.Object, error) { +func setupCRDs(ctx context.Context, client crclient.Client, opts Options, operatorNamespace *corev1.Namespace, operatorService *corev1.Service) ([]crclient.Object, error) { // Build a set of existing IPAM CRDs if a client is available existingIPAMCRDs := set.New[string]() if client != nil { @@ -967,7 +953,7 @@ func setupCRDs(ctx context.Context, client crclient.Client, opts Options, operat Port: ptr.To[int32](443), Path: ptr.To("/convert"), }, - CABundle: webhookCABundle, + CABundle: nil, }, ConversionReviewVersions: []string{"v1beta1", "v1alpha1"}, }, diff --git a/cmd/install/install_test.go b/cmd/install/install_test.go index 33375544104..61052faf3a4 100644 --- a/cmd/install/install_test.go +++ b/cmd/install/install_test.go @@ -5,6 +5,7 @@ import ( "context" "io" "io/fs" + "os" "path/filepath" "strings" "testing" @@ -412,7 +413,7 @@ func TestSetupCRDs(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { g := NewGomegaWithT(t) - crds, err := setupCRDs(t.Context(), nil, tc.inputOptions, &corev1.Namespace{}, nil, nil) + crds, err := setupCRDs(t.Context(), nil, tc.inputOptions, &corev1.Namespace{}, nil) g.Expect(err).ToNot(HaveOccurred()) nodePoolCRDS := make([]crclient.Object, 0) var machineDeploymentCRD crclient.Object @@ -497,6 +498,10 @@ func TestSetupCRDs(t *testing.T) { } func TestRenderHyperShiftOperator_RenderSensitive(t *testing.T) { + g := NewGomegaWithT(t) + pullSecretFile := filepath.Join(t.TempDir(), "pull-secret.json") + g.Expect(os.WriteFile(pullSecretFile, []byte(`{"auths":{}}`), 0o600)).To(Succeed()) + tests := []struct { name string renderSensitive bool @@ -524,10 +529,12 @@ func TestRenderHyperShiftOperator_RenderSensitive(t *testing.T) { EnableDefaultingWebhook: true, EnableValidatingWebhook: true, EnableConversionWebhook: true, + PullSecretFile: pullSecretFile, RenderSensitive: tc.renderSensitive, Format: RenderFormatYaml, OutputTypes: string(OutputAll), } + err := RenderHyperShiftOperator(t.Context(), &buf, opts) g.Expect(err).ToNot(HaveOccurred()) @@ -552,6 +559,39 @@ func TestRenderHyperShiftOperator_RenderSensitive(t *testing.T) { } }) } + + t.Run("When webhooks are enabled it should not render webhook cert secrets", func(t *testing.T) { + g := NewGomegaWithT(t) + + var buf bytes.Buffer + opts := &Options{ + PrivatePlatform: string(hyperv1.NonePlatform), + PullSecretFile: pullSecretFile, + EnableDefaultingWebhook: true, + EnableValidatingWebhook: true, + EnableConversionWebhook: true, + RenderSensitive: true, + Format: RenderFormatYaml, + OutputTypes: string(OutputAll), + } + err := RenderHyperShiftOperator(t.Context(), &buf, opts) + g.Expect(err).ToNot(HaveOccurred()) + + var nonWebhookSecretCount int + for doc := range strings.SplitSeq(buf.String(), "\n---\n") { + if strings.TrimSpace(doc) == "" { + continue + } + obj, _, err := hyperapi.YamlSerializer.Decode([]byte(doc), nil, nil) + g.Expect(err).ToNot(HaveOccurred(), "failed to decode rendered manifest") + if secret, ok := obj.(*corev1.Secret); ok { + g.Expect(secret.Name).ToNot(Equal("webhook-serving-ca"), "webhook CA secret should not be rendered") + g.Expect(secret.Name).ToNot(Equal("manager-serving-cert"), "webhook serving cert should not be rendered") + nonWebhookSecretCount++ + } + } + g.Expect(nonWebhookSecretCount).To(BeNumerically(">", 0), "expected at least one non-webhook secret to be rendered") + }) } func TestHyperShiftOperatorManifests_SharedIngress(t *testing.T) { @@ -973,6 +1013,7 @@ func TestWaitForCAPIOperatorSync(t *testing.T) { }) } } + func TestApplyDefaults(t *testing.T) { tests := []struct { name string diff --git a/hypershift-operator/controllers/webhookcerts/webhookcerts_controller.go b/hypershift-operator/controllers/webhookcerts/webhookcerts_controller.go index b2d3c7652b4..79852656eec 100644 --- a/hypershift-operator/controllers/webhookcerts/webhookcerts_controller.go +++ b/hypershift-operator/controllers/webhookcerts/webhookcerts_controller.go @@ -33,6 +33,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/predicate" "github.com/go-logr/logr" @@ -364,3 +365,60 @@ func GenerateInitialWebhookCerts(namespace, serviceName string) (*corev1.Secret, caBundle := caSecret.Data[certs.CASignerCertMapKey] return caSecret, servingSecret, caBundle, nil } + +// EnsureWebhookCerts ensures that webhook cert secrets exist so the webhook +// server can start. If the serving cert secret already exists with valid data, +// this is a no-op (the volume mount handles file delivery). If the secret is +// missing or has empty data, new certs are generated and persisted as secrets. +func EnsureWebhookCerts(ctx context.Context, c client.Client, namespace, serviceName string) error { + log := ctrl.LoggerFrom(ctx).WithName("webhook-cert-bootstrap") + + if certsExist(ctx, c, namespace) { + log.Info("Webhook cert secrets already exist with valid data, skipping bootstrap") + return nil + } + + log.Info("Generating webhook certificates") + caSecret, servingSecret, _, err := GenerateInitialWebhookCerts(namespace, serviceName) + if err != nil { + return fmt.Errorf("failed to generate webhook certs: %w", err) + } + + caObj := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: CASecretName, Namespace: namespace}} + if _, err := controllerutil.CreateOrUpdate(ctx, c, caObj, func() error { + caObj.Type = corev1.SecretTypeOpaque + caObj.Data = caSecret.Data + return nil + }); err != nil { + return fmt.Errorf("failed to create or update CA secret: %w", err) + } + + servingObj := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: ServingCertSecretName, Namespace: namespace}} + if _, err := controllerutil.CreateOrUpdate(ctx, c, servingObj, func() error { + servingObj.Type = corev1.SecretTypeTLS + servingObj.Data = servingSecret.Data + return nil + }); err != nil { + return fmt.Errorf("failed to create or update serving cert secret: %w", err) + } + + log.Info("Webhook certificates bootstrapped") + return nil +} + +// certsExist returns true when both the CA and serving cert secrets exist with non-empty data. +func certsExist(ctx context.Context, c client.Client, namespace string) bool { + ca := &corev1.Secret{} + if err := c.Get(ctx, client.ObjectKey{Namespace: namespace, Name: CASecretName}, ca); err != nil { + return false + } + if len(ca.Data[certs.CASignerCertMapKey]) == 0 || len(ca.Data[certs.CASignerKeyMapKey]) == 0 { + return false + } + + serving := &corev1.Secret{} + if err := c.Get(ctx, client.ObjectKey{Namespace: namespace, Name: ServingCertSecretName}, serving); err != nil { + return false + } + return len(serving.Data[corev1.TLSCertKey]) > 0 && len(serving.Data[corev1.TLSPrivateKeyKey]) > 0 +} diff --git a/hypershift-operator/controllers/webhookcerts/webhookcerts_controller_test.go b/hypershift-operator/controllers/webhookcerts/webhookcerts_controller_test.go index ec96cf3dd5b..c125b5887a7 100644 --- a/hypershift-operator/controllers/webhookcerts/webhookcerts_controller_test.go +++ b/hypershift-operator/controllers/webhookcerts/webhookcerts_controller_test.go @@ -2,6 +2,8 @@ package webhookcerts import ( "context" + "crypto/x509" + "encoding/pem" "testing" "time" @@ -406,7 +408,110 @@ func TestGenerateInitialWebhookCerts(t *testing.T) { g.Expect(servingSecret.Type).To(Equal(corev1.SecretTypeTLS)) g.Expect(servingSecret.Data).To(HaveKey(corev1.TLSCertKey)) g.Expect(servingSecret.Data).To(HaveKey(corev1.TLSPrivateKeyKey)) + }) +} + +func TestEnsureWebhookCerts(t *testing.T) { + t.Run("When no secrets exist it should create secrets", func(t *testing.T) { + g := NewWithT(t) + + cl := fake.NewClientBuilder().WithScheme(newScheme(t)).Build() + + err := EnsureWebhookCerts(t.Context(), cl, "hypershift", "operator") + g.Expect(err).ToNot(HaveOccurred()) + + caSecret := &corev1.Secret{} + g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: CASecretName, Namespace: "hypershift"}, caSecret)).To(Succeed()) + g.Expect(caSecret.Data).To(HaveKey(certs.CASignerCertMapKey)) + + servingSecret := &corev1.Secret{} + g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: ServingCertSecretName, Namespace: "hypershift"}, servingSecret)).To(Succeed()) + g.Expect(servingSecret.Data).To(HaveKey(corev1.TLSCertKey)) + g.Expect(servingSecret.Data).To(HaveKey(corev1.TLSPrivateKeyKey)) + g.Expect(servingSecret.Type).To(Equal(corev1.SecretTypeTLS)) + }) + t.Run("When secrets already exist with valid data it should not modify them", func(t *testing.T) { + g := NewWithT(t) + + caSecret, servingSecret, _, err := GenerateInitialWebhookCerts("hypershift", "operator") + g.Expect(err).ToNot(HaveOccurred()) + + cl := fake.NewClientBuilder().WithScheme(newScheme(t)).WithObjects(caSecret, servingSecret).Build() + + err = EnsureWebhookCerts(t.Context(), cl, "hypershift", "operator") + g.Expect(err).ToNot(HaveOccurred()) + + updatedServingSecret := &corev1.Secret{} + g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: ServingCertSecretName, Namespace: "hypershift"}, updatedServingSecret)).To(Succeed()) + g.Expect(updatedServingSecret.Data[corev1.TLSCertKey]).To(Equal(servingSecret.Data[corev1.TLSCertKey])) + g.Expect(updatedServingSecret.Data[corev1.TLSPrivateKeyKey]).To(Equal(servingSecret.Data[corev1.TLSPrivateKeyKey])) + + updatedCASecret := &corev1.Secret{} + g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: CASecretName, Namespace: "hypershift"}, updatedCASecret)).To(Succeed()) + g.Expect(updatedCASecret.Data[certs.CASignerCertMapKey]).To(Equal(caSecret.Data[certs.CASignerCertMapKey])) + g.Expect(updatedCASecret.Data[certs.CASignerKeyMapKey]).To(Equal(caSecret.Data[certs.CASignerKeyMapKey])) + }) + + t.Run("When serving cert secret exists with empty data it should regenerate and update", func(t *testing.T) { + g := NewWithT(t) + + emptySecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: ServingCertSecretName, + Namespace: "hypershift", + }, + Type: corev1.SecretTypeTLS, + Data: map[string][]byte{}, + } + + cl := fake.NewClientBuilder().WithScheme(newScheme(t)).WithObjects(emptySecret).Build() + + err := EnsureWebhookCerts(t.Context(), cl, "hypershift", "operator") + g.Expect(err).ToNot(HaveOccurred()) + + updatedServingSecret := &corev1.Secret{} + g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: ServingCertSecretName, Namespace: "hypershift"}, updatedServingSecret)).To(Succeed()) + g.Expect(updatedServingSecret.Data[corev1.TLSCertKey]).ToNot(BeEmpty()) + g.Expect(updatedServingSecret.Data[corev1.TLSPrivateKeyKey]).ToNot(BeEmpty()) + + updatedCASecret := &corev1.Secret{} + g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: CASecretName, Namespace: "hypershift"}, updatedCASecret)).To(Succeed()) + g.Expect(updatedCASecret.Data[certs.CASignerCertMapKey]).ToNot(BeEmpty()) + g.Expect(updatedCASecret.Data[certs.CASignerKeyMapKey]).ToNot(BeEmpty()) + }) + + t.Run("When CA exists but serving cert is missing it should regenerate both secrets", func(t *testing.T) { + g := NewWithT(t) + + caSecret, _, _, err := GenerateInitialWebhookCerts("hypershift", "operator") + g.Expect(err).ToNot(HaveOccurred()) + + cl := fake.NewClientBuilder().WithScheme(newScheme(t)).WithObjects(caSecret).Build() + + err = EnsureWebhookCerts(t.Context(), cl, "hypershift", "operator") + g.Expect(err).ToNot(HaveOccurred()) + + // Both secrets should exist with valid data. + updatedCASecret := &corev1.Secret{} + g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: CASecretName, Namespace: "hypershift"}, updatedCASecret)).To(Succeed()) + g.Expect(updatedCASecret.Data[certs.CASignerCertMapKey]).ToNot(BeEmpty()) + g.Expect(updatedCASecret.Data[certs.CASignerKeyMapKey]).ToNot(BeEmpty()) + + servingSecret := &corev1.Secret{} + g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: ServingCertSecretName, Namespace: "hypershift"}, servingSecret)).To(Succeed()) + g.Expect(servingSecret.Data[corev1.TLSCertKey]).ToNot(BeEmpty()) + g.Expect(servingSecret.Data[corev1.TLSPrivateKeyKey]).ToNot(BeEmpty()) + + // Verify the serving cert was signed by the (possibly replaced) CA. + caPool := x509.NewCertPool() + g.Expect(caPool.AppendCertsFromPEM(updatedCASecret.Data[certs.CASignerCertMapKey])).To(BeTrue()) + block, _ := pem.Decode(servingSecret.Data[corev1.TLSCertKey]) + g.Expect(block).ToNot(BeNil()) + leaf, err := x509.ParseCertificate(block.Bytes) + g.Expect(err).ToNot(HaveOccurred()) + _, err = leaf.Verify(x509.VerifyOptions{Roots: caPool}) + g.Expect(err).ToNot(HaveOccurred()) }) } diff --git a/hypershift-operator/main.go b/hypershift-operator/main.go index 11e552f0a27..a663242bf0f 100644 --- a/hypershift-operator/main.go +++ b/hypershift-operator/main.go @@ -24,6 +24,7 @@ import ( hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" awsutil "github.com/openshift/hypershift/cmd/infra/aws/util" + "github.com/openshift/hypershift/cmd/install/assets" pkiconfig "github.com/openshift/hypershift/control-plane-pki-operator/config" etcdrecovery "github.com/openshift/hypershift/etcd-recovery" "github.com/openshift/hypershift/hypershift-operator/controllers/auditlogpersistence" @@ -274,6 +275,12 @@ func run(ctx context.Context, opts *StartOptions, log logr.Logger) error { return fmt.Errorf("failed to construct api reading client: %w", err) } + if opts.CertDir != "" { + if err := webhookcerts.EnsureWebhookCerts(ctx, apiReadingClient, opts.Namespace, assets.HypershiftOperatorName); err != nil { + return fmt.Errorf("failed to bootstrap webhook certs: %w", err) + } + } + if err := reconcileDeprecationValidatingAdmissionPolicy(ctx, apiReadingClient, mgmtClusterCaps, log); err != nil { return fmt.Errorf("failed to reconcile deprecation ValidatingAdmissionPolicy: %w", err) }