diff --git a/api/v1alpha1/dataprotectionapplication_types.go b/api/v1alpha1/dataprotectionapplication_types.go index 5f9797becb4..3046e981365 100644 --- a/api/v1alpha1/dataprotectionapplication_types.go +++ b/api/v1alpha1/dataprotectionapplication_types.go @@ -755,8 +755,19 @@ type CloudStorageLocation struct { Prefix string `json:"prefix,omitempty"` // CACert defines a CA bundle to use when verifying TLS connections to the provider. + // Deprecated: use CACertRef instead, matching Velero's own deprecation of inline CACert. + // Inline CACert requires updating this BSL to rotate the certificate; CACertRef lets + // Velero resolve the CA fresh from the Secret on each use, so rotation is just a Secret + // update. // +optional CACert []byte `json:"caCert,omitempty"` + + // CACertRef is a reference to a Secret containing the CA certificate bundle to use + // when verifying TLS connections to the provider. The Secret must be in the same + // namespace as the DataProtectionApplication. Prefer this over CACert for CA + // certificate rotation support. + // +optional + CACertRef *corev1.SecretKeySelector `json:"caCertRef,omitempty"` } // BackupLocation defines the configuration for the DPA backup storage @@ -794,8 +805,19 @@ type ObjectStorageLocation struct { Prefix string `json:"prefix,omitempty"` // CACert defines a CA bundle to use when verifying TLS connections to the provider. + // Deprecated: use CACertRef instead, matching Velero's own deprecation of inline CACert. + // Inline CACert requires updating this BSL to rotate the certificate; CACertRef lets + // Velero resolve the CA fresh from the Secret on each use, so rotation is just a Secret + // update. // +optional CACert []byte `json:"caCert,omitempty"` + + // CACertRef is a reference to a Secret containing the CA certificate bundle to use + // when verifying TLS connections to the provider. The Secret must be in the same + // namespace as the DataProtectionApplication. Prefer this over CACert for CA + // certificate rotation support. + // +optional + CACertRef *corev1.SecretKeySelector `json:"caCertRef,omitempty"` } // StorageType defines the enforced values for the Velero StorageType diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 02931129263..d813477256e 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -213,6 +213,11 @@ func (in *CloudStorageLocation) DeepCopyInto(out *CloudStorageLocation) { *out = make([]byte, len(*in)) copy(*out, *in) } + if in.CACertRef != nil { + in, out := &in.CACertRef, &out.CACertRef + *out = new(corev1.SecretKeySelector) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CloudStorageLocation. @@ -998,6 +1003,11 @@ func (in *ObjectStorageLocation) DeepCopyInto(out *ObjectStorageLocation) { *out = make([]byte, len(*in)) copy(*out, *in) } + if in.CACertRef != nil { + in, out := &in.CACertRef, &out.CACertRef + *out = new(corev1.SecretKeySelector) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ObjectStorageLocation. diff --git a/bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml b/bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml index 89566e160da..a043e3d9a8f 100644 --- a/bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml +++ b/bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml @@ -69,9 +69,40 @@ spec: nullable: true type: string caCert: - description: CACert defines a CA bundle to use when verifying TLS connections to the provider. + description: |- + CACert defines a CA bundle to use when verifying TLS connections to the provider. + Deprecated: use CACertRef instead, matching Velero's own deprecation of inline CACert. + Inline CACert requires updating this BSL to rotate the certificate; CACertRef lets + Velero resolve the CA fresh from the Secret on each use, so rotation is just a Secret + update. format: byte type: string + caCertRef: + description: |- + CACertRef is a reference to a Secret containing the CA certificate bundle to use + when verifying TLS connections to the provider. The Secret must be in the same + namespace as the DataProtectionApplication. Prefer this over CACert for CA + certificate rotation support. + properties: + key: + description: The key of the secret to select from. Must be a valid secret key. + type: string + name: + default: "" + description: |- + Name of the referent. + This field is effectively required, but due to backwards compatibility is + allowed to be empty. Instances of this type with an empty value here are + almost certainly wrong. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + type: string + optional: + description: Specify whether the Secret or its key must be defined + type: boolean + required: + - key + type: object + x-kubernetes-map-type: atomic cloudStorageRef: description: |- LocalObjectReference contains enough information to let you locate the @@ -1972,9 +2003,40 @@ spec: description: Bucket is the bucket to use for object storage. type: string caCert: - description: CACert defines a CA bundle to use when verifying TLS connections to the provider. + description: |- + CACert defines a CA bundle to use when verifying TLS connections to the provider. + Deprecated: use CACertRef instead, matching Velero's own deprecation of inline CACert. + Inline CACert requires updating this BSL to rotate the certificate; CACertRef lets + Velero resolve the CA fresh from the Secret on each use, so rotation is just a Secret + update. format: byte type: string + caCertRef: + description: |- + CACertRef is a reference to a Secret containing the CA certificate bundle to use + when verifying TLS connections to the provider. The Secret must be in the same + namespace as the DataProtectionApplication. Prefer this over CACert for CA + certificate rotation support. + properties: + key: + description: The key of the secret to select from. Must be a valid secret key. + type: string + name: + default: "" + description: |- + Name of the referent. + This field is effectively required, but due to backwards compatibility is + allowed to be empty. Instances of this type with an empty value here are + almost certainly wrong. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + type: string + optional: + description: Specify whether the Secret or its key must be defined + type: boolean + required: + - key + type: object + x-kubernetes-map-type: atomic prefix: description: Prefix is the path inside a bucket to use for Velero storage. Optional. type: string diff --git a/config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml b/config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml index 36ef7cfd5ee..da0f4c8f9d8 100644 --- a/config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml +++ b/config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml @@ -69,9 +69,40 @@ spec: nullable: true type: string caCert: - description: CACert defines a CA bundle to use when verifying TLS connections to the provider. + description: |- + CACert defines a CA bundle to use when verifying TLS connections to the provider. + Deprecated: use CACertRef instead, matching Velero's own deprecation of inline CACert. + Inline CACert requires updating this BSL to rotate the certificate; CACertRef lets + Velero resolve the CA fresh from the Secret on each use, so rotation is just a Secret + update. format: byte type: string + caCertRef: + description: |- + CACertRef is a reference to a Secret containing the CA certificate bundle to use + when verifying TLS connections to the provider. The Secret must be in the same + namespace as the DataProtectionApplication. Prefer this over CACert for CA + certificate rotation support. + properties: + key: + description: The key of the secret to select from. Must be a valid secret key. + type: string + name: + default: "" + description: |- + Name of the referent. + This field is effectively required, but due to backwards compatibility is + allowed to be empty. Instances of this type with an empty value here are + almost certainly wrong. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + type: string + optional: + description: Specify whether the Secret or its key must be defined + type: boolean + required: + - key + type: object + x-kubernetes-map-type: atomic cloudStorageRef: description: |- LocalObjectReference contains enough information to let you locate the @@ -1972,9 +2003,40 @@ spec: description: Bucket is the bucket to use for object storage. type: string caCert: - description: CACert defines a CA bundle to use when verifying TLS connections to the provider. + description: |- + CACert defines a CA bundle to use when verifying TLS connections to the provider. + Deprecated: use CACertRef instead, matching Velero's own deprecation of inline CACert. + Inline CACert requires updating this BSL to rotate the certificate; CACertRef lets + Velero resolve the CA fresh from the Secret on each use, so rotation is just a Secret + update. format: byte type: string + caCertRef: + description: |- + CACertRef is a reference to a Secret containing the CA certificate bundle to use + when verifying TLS connections to the provider. The Secret must be in the same + namespace as the DataProtectionApplication. Prefer this over CACert for CA + certificate rotation support. + properties: + key: + description: The key of the secret to select from. Must be a valid secret key. + type: string + name: + default: "" + description: |- + Name of the referent. + This field is effectively required, but due to backwards compatibility is + allowed to be empty. Instances of this type with an empty value here are + almost certainly wrong. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + type: string + optional: + description: Specify whether the Secret or its key must be defined + type: boolean + required: + - key + type: object + x-kubernetes-map-type: atomic prefix: description: Prefix is the path inside a bucket to use for Velero storage. Optional. type: string diff --git a/internal/controller/bsl.go b/internal/controller/bsl.go index c09054bd472..c0e651db1a3 100644 --- a/internal/controller/bsl.go +++ b/internal/controller/bsl.go @@ -307,10 +307,14 @@ func (r *DataProtectionApplicationReconciler) ReconcileBackupStorageLocations(lo // Preserve Velero's management of default bsl.Spec.Default = existingDefault } + caCertRef, err := r.reconcileCACertSecret(dpa.Namespace, bslName, bslSpec.CloudStorage.CACert, bslSpec.CloudStorage.CACertRef, dpa) + if err != nil { + return err + } bsl.Spec.ObjectStorage = &velerov1.ObjectStorageLocation{ - Bucket: bucket.Spec.Name, - Prefix: bslSpec.CloudStorage.Prefix, - CACert: bslSpec.CloudStorage.CACert, + Bucket: bucket.Spec.Name, + Prefix: bslSpec.CloudStorage.Prefix, + CACertRef: caCertRef, } switch bucket.Spec.Provider { case oadpv1alpha1.AWSBucketProvider: @@ -473,10 +477,14 @@ func (r *DataProtectionApplicationReconciler) populateBSLFromCloudStorage(bslSpe } // Set object storage configuration + caCertRef, err := r.reconcileCACertSecret(namespace, cloudStorage.Name, bslSpec.CloudStorage.CACert, bslSpec.CloudStorage.CACertRef, nil) + if err != nil { + return fmt.Errorf("failed to reconcile CACert secret for CloudStorage %s: %w", cloudStorage.Name, err) + } bslSpec.Velero.ObjectStorage = &velerov1.ObjectStorageLocation{ - Bucket: cloudStorage.Spec.Name, - Prefix: bslSpec.CloudStorage.Prefix, - CACert: bslSpec.CloudStorage.CACert, + Bucket: cloudStorage.Spec.Name, + Prefix: bslSpec.CloudStorage.Prefix, + CACertRef: caCertRef, } // Set config, merging CloudStorage config with BSL-specific config @@ -924,6 +932,135 @@ func (r *DataProtectionApplicationReconciler) patchAzureSecretWithResourceGroup( return nil } +const caCertSecretKey = "cacert" + +// caCertSecretName returns the name of the Secret OADP creates to hold a BSL's inline CACert bytes. +func caCertSecretName(name string) string { + return "oadp-" + name + "-cacert" +} + +// reconcileCACertSecret resolves a CACertRef for a BSL's object storage TLS verification. +// +// If the DPA spec already provides an explicit CACertRef, it is returned as-is, so the user +// can manage and rotate that Secret themselves. Any Secret OADP previously generated for this +// name is cleaned up, since it's no longer referenced by anything. +// +// If inline CACert bytes are provided instead, OADP creates/updates an owned Secret containing +// those bytes and returns a SecretKeySelector pointing at it. This gives users cert rotation +// support (updating the Secret directly) without requiring them to know about CACertRef, since +// Velero resolves CACertRef fresh on each use instead of caching CACert bytes on the BSL object. +// +// If neither is provided, any previously-created Secret for this name is removed and nil is returned. +// +// A Secret at the generated name is only ever created, updated, or deleted here if it already +// carries OADP's own label (i.e. OADP created it) — an unrelated, unmanaged Secret that happens +// to collide with the generated name is left untouched and an error is returned instead. +func (r *DataProtectionApplicationReconciler) reconcileCACertSecret(namespace, name string, caCert []byte, caCertRef *corev1.SecretKeySelector, owner client.Object) (*corev1.SecretKeySelector, error) { + secretName := caCertSecretName(name) + existing := &corev1.Secret{} + err := r.Get(r.Context, types.NamespacedName{Name: secretName, Namespace: namespace}, existing) + if err != nil && !k8serrors.IsNotFound(err) { + return nil, err + } + exists := err == nil + oadpOwned := exists && existing.Labels[oadpv1alpha1.OadpOperatorLabel] == "True" + if exists && !oadpOwned { + return nil, fmt.Errorf("secret %s/%s already exists and is not managed by OADP; refusing to overwrite or delete it", namespace, secretName) + } + + if caCertRef != nil { + if oadpOwned { + if err := r.Delete(r.Context, existing); err != nil && !k8serrors.IsNotFound(err) { + return nil, err + } + } + return caCertRef, nil + } + + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: namespace, + }, + } + + if len(caCert) == 0 { + if oadpOwned { + if err := r.Delete(r.Context, secret); err != nil && !k8serrors.IsNotFound(err) { + return nil, err + } + } + return nil, nil + } + + if err := validatePEMCertificate(caCert); err != nil { + // Log warning but continue processing (graceful degradation for testing), + // consistent with validation elsewhere in this file. + r.Log.Info("CA certificate validation failed, but continuing with processing", + "name", name, "error", err.Error()) + } + + op, err := controllerutil.CreateOrPatch(r.Context, r.Client, secret, func() error { + if secret.Labels == nil { + secret.Labels = map[string]string{} + } + secret.Labels[oadpv1alpha1.OadpOperatorLabel] = "True" + secret.Data = map[string][]byte{caCertSecretKey: caCert} + if owner == nil { + return nil + } + return controllerutil.SetControllerReference(owner, secret, r.Scheme) + }) + if err != nil { + return nil, err + } + if op == controllerutil.OperationResultCreated || op == controllerutil.OperationResultUpdated { + r.EventRecorder.Event(secret, + corev1.EventTypeNormal, + "CACertSecretReconciled", + fmt.Sprintf("performed %s on secret %s/%s", op, secret.Namespace, secret.Name), + ) + } + + return &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: secret.Name}, + Key: caCertSecretKey, + }, nil +} + +// resolveCACertBytes returns the raw CA certificate bytes for a caCertRef pointing at a Secret in +// the given namespace (e.g. one reconcileCACertSecret created, or one the user manages themselves), +// falling back to inline caCert only when caCertRef is nil. A required (non-optional) Secret or key +// that can't be resolved is a real error, not an empty CA bundle — otherwise a BSL that needs the CA +// cert for TLS verification would silently fail at backup time instead of failing reconciliation +// loudly. +func (r *DataProtectionApplicationReconciler) resolveCACertBytes(caCert []byte, caCertRef *corev1.SecretKeySelector, namespace string) ([]byte, error) { + // CACertRef takes precedence when both are set, matching reconcileCACertSecret's own + // precedence for the Velero BSL object itself — otherwise the two consumers of this + // DPA-level CACert/CACertRef pair (the BSL's ObjectStorage and this AWS_CA_BUNDLE + // aggregation) could end up trusting different CA material. + if caCertRef == nil { + return caCert, nil + } + optional := caCertRef.Optional != nil && *caCertRef.Optional + + secret := &corev1.Secret{} + if err := r.Get(r.Context, types.NamespacedName{Name: caCertRef.Name, Namespace: namespace}, secret); err != nil { + if k8serrors.IsNotFound(err) && optional { + return nil, nil + } + return nil, fmt.Errorf("failed to resolve CACertRef secret %s/%s: %w", namespace, caCertRef.Name, err) + } + data, ok := secret.Data[caCertRef.Key] + if !ok { + if optional { + return nil, nil + } + return nil, fmt.Errorf("CACertRef secret %s/%s has no key %q", namespace, caCertRef.Name, caCertRef.Key) + } + return data, nil +} + // processCACertForBSLs creates a ConfigMap containing CA certificates from BackupStorageLocations // Returns the ConfigMap name if certificates were found, empty string otherwise func (r *DataProtectionApplicationReconciler) processCACertForBSLs() (string, error) { @@ -944,8 +1081,12 @@ func (r *DataProtectionApplicationReconciler) processCACertForBSLs() (string, er // Determine provider and get CA certificate if bslSpec.Velero != nil { provider = bslSpec.Velero.Provider - if bslSpec.Velero.ObjectStorage != nil && bslSpec.Velero.ObjectStorage.CACert != nil { - caCert = bslSpec.Velero.ObjectStorage.CACert + if bslSpec.Velero.ObjectStorage != nil { + var err error + caCert, err = r.resolveCACertBytes(bslSpec.Velero.ObjectStorage.CACert, bslSpec.Velero.ObjectStorage.CACertRef, dpa.Namespace) + if err != nil { + return "", fmt.Errorf("BSL %s: %w", bslName, err) + } } } else if bslSpec.CloudStorage != nil { // For CloudStorage, determine provider from the CloudStorage resource @@ -961,8 +1102,12 @@ func (r *DataProtectionApplicationReconciler) processCACertForBSLs() (string, er provider = GCPProvider } } - if bslSpec.CloudStorage.CACert != nil { - caCert = bslSpec.CloudStorage.CACert + if bslSpec.CloudStorage.CACert != nil || bslSpec.CloudStorage.CACertRef != nil { + var err error + caCert, err = r.resolveCACertBytes(bslSpec.CloudStorage.CACert, bslSpec.CloudStorage.CACertRef, dpa.Namespace) + if err != nil { + return "", fmt.Errorf("BSL %s: %w", bslName, err) + } } } @@ -1020,8 +1165,14 @@ func (r *DataProtectionApplicationReconciler) processCACertForBSLs() (string, er } // Check for CA certificate in this BSL - if bsl.Spec.ObjectStorage != nil && bsl.Spec.ObjectStorage.CACert != nil { - caCert := bsl.Spec.ObjectStorage.CACert + if bsl.Spec.ObjectStorage != nil { + caCert, err := r.resolveCACertBytes(bsl.Spec.ObjectStorage.CACert, bsl.Spec.ObjectStorage.CACertRef, bsl.Namespace) + if err != nil { + // This BSL isn't part of the DPA spec (it's an extra one already in the + // cluster), so a resolution failure here shouldn't block the ConfigMap + // update for the BSLs the DPA spec actually declares. + r.Log.Error(err, "Failed to resolve CACertRef for BSL, skipping its CA cert", "bsl", bsl.Name) + } if len(caCert) > 0 { certStr := string(caCert) if !collectedCerts[certStr] { diff --git a/internal/controller/bsl_test.go b/internal/controller/bsl_test.go index 92e441553f8..ab42dce2503 100644 --- a/internal/controller/bsl_test.go +++ b/internal/controller/bsl_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/assert" velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" corev1 "k8s.io/api/core/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -3105,7 +3106,12 @@ func TestDPAReconciler_ReconcileBackupStorageLocations(t *testing.T) { ObjectStorage: &velerov1.ObjectStorageLocation{ Bucket: "test-bucket", Prefix: "test-prefix", - CACert: []byte("test-ca"), + CACertRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "oadp-test-dpa-1-cacert", + }, + Key: "cacert", + }, }, }, Credential: &corev1.SecretKeySelector{ @@ -4733,7 +4739,12 @@ func TestDPAReconciler_populateBSLFromCloudStorage(t *testing.T) { ObjectStorage: &velerov1.ObjectStorageLocation{ Bucket: "my-gcp-bucket", Prefix: "velero", - CACert: []byte("test-ca-cert"), + CACertRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "oadp-gcp-bucket-cacert", + }, + Key: "cacert", + }, }, }, Config: map[string]string{}, @@ -4947,10 +4958,11 @@ func TestDPAReconciler_populateBSLFromCloudStorage(t *testing.T) { fakeClient := getFakeClientFromObjectsForTest(t, objs...) r := &DataProtectionApplicationReconciler{ - Client: fakeClient, - Scheme: scheme, - Context: context.Background(), - Log: logr.Discard(), + Client: fakeClient, + Scheme: scheme, + Context: context.Background(), + Log: logr.Discard(), + EventRecorder: record.NewFakeRecorder(10), } err := r.populateBSLFromCloudStorage(tt.bslSpec, "test-ns") @@ -5948,3 +5960,225 @@ ddummyenddummyenddummyenddummyend }) } } + +func TestDPAReconciler_reconcileCACertSecret(t *testing.T) { + testScheme, err := getSchemeForFakeClient() + assert.NoError(t, err) + + dpa := &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-dpa", + Namespace: "test-ns", + }, + } + + t.Run("inline CACert creates an owned Secret and returns a CACertRef to it", func(t *testing.T) { + r := &DataProtectionApplicationReconciler{ + Client: getFakeClientFromObjectsForTest(t), + Scheme: testScheme, + Context: context.Background(), + Log: logr.Discard(), + EventRecorder: record.NewFakeRecorder(10), + } + + caCert := generateTestCACert("bsl-1") + ref, err := r.reconcileCACertSecret("test-ns", "bsl-1", caCert, nil, dpa) + assert.NoError(t, err) + if assert.NotNil(t, ref) { + assert.Equal(t, "oadp-bsl-1-cacert", ref.Name) + assert.Equal(t, "cacert", ref.Key) + } + + secret := &corev1.Secret{} + assert.NoError(t, r.Get(r.Context, types.NamespacedName{Name: "oadp-bsl-1-cacert", Namespace: "test-ns"}, secret)) + assert.Equal(t, caCert, secret.Data["cacert"]) + assert.Len(t, secret.OwnerReferences, 1) + assert.Equal(t, "test-dpa", secret.OwnerReferences[0].Name) + }) + + t.Run("explicit CACertRef is passed through and no Secret is created", func(t *testing.T) { + r := &DataProtectionApplicationReconciler{ + Client: getFakeClientFromObjectsForTest(t), + Scheme: testScheme, + Context: context.Background(), + Log: logr.Discard(), + EventRecorder: record.NewFakeRecorder(10), + } + + userRef := &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: "user-managed-secret"}, + Key: "ca.crt", + } + ref, err := r.reconcileCACertSecret("test-ns", "bsl-1", nil, userRef, dpa) + assert.NoError(t, err) + assert.Same(t, userRef, ref) + + secret := &corev1.Secret{} + assert.True(t, k8serrors.IsNotFound(r.Get(r.Context, types.NamespacedName{Name: "oadp-bsl-1-cacert", Namespace: "test-ns"}, secret))) + }) + + t.Run("removing CACert deletes a previously-created Secret", func(t *testing.T) { + r := &DataProtectionApplicationReconciler{ + Client: getFakeClientFromObjectsForTest(t), + Scheme: testScheme, + Context: context.Background(), + Log: logr.Discard(), + EventRecorder: record.NewFakeRecorder(10), + } + + caCert := generateTestCACert("bsl-1") + _, err := r.reconcileCACertSecret("test-ns", "bsl-1", caCert, nil, dpa) + assert.NoError(t, err) + + ref, err := r.reconcileCACertSecret("test-ns", "bsl-1", nil, nil, dpa) + assert.NoError(t, err) + assert.Nil(t, ref) + + secret := &corev1.Secret{} + assert.True(t, k8serrors.IsNotFound(r.Get(r.Context, types.NamespacedName{Name: "oadp-bsl-1-cacert", Namespace: "test-ns"}, secret))) + }) + + t.Run("switching to an explicit CACertRef cleans up the previously-created owned Secret", func(t *testing.T) { + r := &DataProtectionApplicationReconciler{ + Client: getFakeClientFromObjectsForTest(t), + Scheme: testScheme, + Context: context.Background(), + Log: logr.Discard(), + EventRecorder: record.NewFakeRecorder(10), + } + + caCert := generateTestCACert("bsl-1") + _, err := r.reconcileCACertSecret("test-ns", "bsl-1", caCert, nil, dpa) + assert.NoError(t, err) + + userRef := &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: "user-managed-secret"}, + Key: "ca.crt", + } + ref, err := r.reconcileCACertSecret("test-ns", "bsl-1", nil, userRef, dpa) + assert.NoError(t, err) + assert.Same(t, userRef, ref) + + secret := &corev1.Secret{} + assert.True(t, k8serrors.IsNotFound(r.Get(r.Context, types.NamespacedName{Name: "oadp-bsl-1-cacert", Namespace: "test-ns"}, secret))) + }) + + t.Run("a pre-existing Secret not owned by OADP is never touched", func(t *testing.T) { + foreignSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "oadp-bsl-1-cacert", + Namespace: "test-ns", + }, + Data: map[string][]byte{"unrelated": []byte("do-not-touch")}, + } + r := &DataProtectionApplicationReconciler{ + Client: getFakeClientFromObjectsForTest(t, foreignSecret), + Scheme: testScheme, + Context: context.Background(), + Log: logr.Discard(), + EventRecorder: record.NewFakeRecorder(10), + } + + caCert := generateTestCACert("bsl-1") + _, err := r.reconcileCACertSecret("test-ns", "bsl-1", caCert, nil, dpa) + assert.Error(t, err) + + secret := &corev1.Secret{} + assert.NoError(t, r.Get(r.Context, types.NamespacedName{Name: "oadp-bsl-1-cacert", Namespace: "test-ns"}, secret)) + assert.Equal(t, foreignSecret.Data, secret.Data) + + // Same guard applies on the delete path (no CACert/CACertRef) and the + // switch-to-CACertRef path. + _, err = r.reconcileCACertSecret("test-ns", "bsl-1", nil, nil, dpa) + assert.Error(t, err) + userRef := &corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "other"}, Key: "k"} + _, err = r.reconcileCACertSecret("test-ns", "bsl-1", nil, userRef, dpa) + assert.Error(t, err) + + assert.NoError(t, r.Get(r.Context, types.NamespacedName{Name: "oadp-bsl-1-cacert", Namespace: "test-ns"}, secret)) + assert.Equal(t, foreignSecret.Data, secret.Data) + }) +} + +func TestDPAReconciler_resolveCACertBytes(t *testing.T) { + testScheme, err := getSchemeForFakeClient() + assert.NoError(t, err) + + t.Run("CACertRef wins over inline CACert when both are set", func(t *testing.T) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "s", Namespace: "test-ns"}, + Data: map[string][]byte{"k": []byte("from-secret")}, + } + r := &DataProtectionApplicationReconciler{ + Client: getFakeClientFromObjectsForTest(t, secret), + Scheme: testScheme, + Context: context.Background(), + Log: logr.Discard(), + } + data, err := r.resolveCACertBytes([]byte("inline"), &corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "s"}, Key: "k"}, "test-ns") + assert.NoError(t, err) + assert.Equal(t, []byte("from-secret"), data) + }) + + t.Run("inline CACert used only when CACertRef is nil", func(t *testing.T) { + r := &DataProtectionApplicationReconciler{ + Client: getFakeClientFromObjectsForTest(t), + Scheme: testScheme, + Context: context.Background(), + Log: logr.Discard(), + } + data, err := r.resolveCACertBytes([]byte("inline"), nil, "test-ns") + assert.NoError(t, err) + assert.Equal(t, []byte("inline"), data) + }) + + t.Run("a required but unresolvable CACertRef errors even when inline CACert is also set", func(t *testing.T) { + r := &DataProtectionApplicationReconciler{ + Client: getFakeClientFromObjectsForTest(t), + Scheme: testScheme, + Context: context.Background(), + Log: logr.Discard(), + } + _, err := r.resolveCACertBytes([]byte("inline"), &corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "missing"}, Key: "k"}, "test-ns") + assert.Error(t, err) + }) + + t.Run("required CACertRef pointing at a missing Secret returns an error", func(t *testing.T) { + r := &DataProtectionApplicationReconciler{ + Client: getFakeClientFromObjectsForTest(t), + Scheme: testScheme, + Context: context.Background(), + Log: logr.Discard(), + } + _, err := r.resolveCACertBytes(nil, &corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "missing"}, Key: "k"}, "test-ns") + assert.Error(t, err) + }) + + t.Run("required CACertRef pointing at a Secret missing the key returns an error", func(t *testing.T) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "s", Namespace: "test-ns"}, + Data: map[string][]byte{"other-key": []byte("x")}, + } + r := &DataProtectionApplicationReconciler{ + Client: getFakeClientFromObjectsForTest(t, secret), + Scheme: testScheme, + Context: context.Background(), + Log: logr.Discard(), + } + _, err := r.resolveCACertBytes(nil, &corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "s"}, Key: "missing-key"}, "test-ns") + assert.Error(t, err) + }) + + t.Run("optional CACertRef pointing at a missing Secret returns no error", func(t *testing.T) { + r := &DataProtectionApplicationReconciler{ + Client: getFakeClientFromObjectsForTest(t), + Scheme: testScheme, + Context: context.Background(), + Log: logr.Discard(), + } + optional := true + data, err := r.resolveCACertBytes(nil, &corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "missing"}, Key: "k", Optional: &optional}, "test-ns") + assert.NoError(t, err) + assert.Nil(t, data) + }) +} diff --git a/tests/e2e/cacert_cloudstorage_suite_test.go b/tests/e2e/cacert_cloudstorage_suite_test.go new file mode 100644 index 00000000000..a4821fba59c --- /dev/null +++ b/tests/e2e/cacert_cloudstorage_suite_test.go @@ -0,0 +1,148 @@ +package e2e_test + +import ( + "context" + "time" + + "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" + velero "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" + + oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1" + "github.com/openshift/oadp-operator/tests/e2e/lib" +) + +// This suite covers OADP-8056: for a CloudStorage-backed BackupLocation, OADP must create an +// owned Secret from an inline `caCert` and point the resulting Velero BSL's `caCertRef` at it, +// instead of copying the CA bytes inline. It only exercises reconciliation and the resulting +// BSL/Secret state — it does not need a working backup, since the CloudStorage-backed BSL here +// points at the suite's real cloud bucket (a self-signed test CA would correctly fail TLS +// verification against that bucket's real public CA, so BSL Available is not asserted). +var _ = ginkgo.Describe("BSL cacert with CloudStorage-backed BSL", ginkgo.Ordered, ginkgo.Label("aws"), func() { + const ( + cloudStorageName = "ts-cacert-cloudstorage" + dpaName = "ts-cacert-cloudstorage" + bslName = "cs-cacert-bsl" + ) + + var ( + caPEM []byte + cloudStorageDpaCR *lib.DpaCustomResource + ) + + ginkgo.BeforeAll(func(ctx ginkgo.SpecContext) { + cloudStorageDpaCR = &lib.DpaCustomResource{ + Name: dpaName, + Namespace: namespace, + Client: runTimeClientForSuiteRun, + } + + var err error + caPEM, _, err = lib.GenerateSelfSignedCA() + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "generating self-signed CA") + + cloudStorage := &oadpv1alpha1.CloudStorage{ + ObjectMeta: metav1.ObjectMeta{Name: cloudStorageName, Namespace: namespace}, + Spec: oadpv1alpha1.CloudStorageSpec{ + Name: dpaCR.BSLBucket, + Provider: oadpv1alpha1.AWSBucketProvider, + Region: dpaCR.BSLConfig["region"], + CreationSecret: corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: bslSecretName}, + Key: "cloud", + }, + }, + } + err = runTimeClientForSuiteRun.Create(ctx, cloudStorage) + if err != nil && !apierrors.IsAlreadyExists(err) { + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "creating CloudStorage CR %s", cloudStorageName) + } + }) + + ginkgo.AfterAll(func(ctx ginkgo.SpecContext) { + cloudStorage := &oadpv1alpha1.CloudStorage{ObjectMeta: metav1.ObjectMeta{Name: cloudStorageName, Namespace: namespace}} + _ = runTimeClientForSuiteRun.Delete(ctx, cloudStorage) + }) + + ginkgo.AfterEach(func(ctx ginkgo.SpecContext) { + if !skipMustGather && ctx.SpecReport().Failed() { + _ = lib.RunMustGather(artifact_dir, cloudStorageDpaCR.Client) + } + gomega.Expect(cloudStorageDpaCR.Delete()).NotTo(gomega.HaveOccurred()) + gomega.Eventually(lib.VeleroIsDeleted(kubernetesClientForSuiteRun, namespace), 5*time.Minute, 5*time.Second).Should(gomega.BeTrue()) + }) + + buildCloudStorageDpaSpec := func(caCert []byte) *oadpv1alpha1.DataProtectionApplicationSpec { + return &oadpv1alpha1.DataProtectionApplicationSpec{ + Configuration: &oadpv1alpha1.ApplicationConfig{ + Velero: &oadpv1alpha1.VeleroConfig{ + DefaultPlugins: []oadpv1alpha1.DefaultPlugin{ + oadpv1alpha1.DefaultPluginOpenShift, + oadpv1alpha1.DefaultPluginAWS, + }, + }, + NodeAgent: &oadpv1alpha1.NodeAgentConfig{ + UploaderType: "kopia", + NodeAgentCommonFields: oadpv1alpha1.NodeAgentCommonFields{ + Enable: ptr.To(false), + }, + }, + }, + BackupLocations: []oadpv1alpha1.BackupLocation{ + { + Name: bslName, + CloudStorage: &oadpv1alpha1.CloudStorageLocation{ + CloudStorageRef: corev1.LocalObjectReference{Name: cloudStorageName}, + CACert: caCert, + Default: true, + // Prefix is required by DPA validation when backupImages is enabled. + Prefix: "cloudstorage", + }, + }, + }, + } + } + + ginkgo.It("creates an owned Secret and sets CACertRef instead of inline CACert for a CloudStorage-backed BSL", func(ctx ginkgo.SpecContext) { + gomega.Expect(cloudStorageDpaCR.CreateOrUpdate(buildCloudStorageDpaSpec(caPEM))).NotTo(gomega.HaveOccurred()) + gomega.Eventually(cloudStorageDpaCR.IsReconciledTrue(), 3*time.Minute, 5*time.Second).Should(gomega.BeTrue()) + + bsl := &velero.BackupStorageLocation{} + gomega.Expect(runTimeClientForSuiteRun.Get(ctx, types.NamespacedName{Namespace: namespace, Name: bslName}, bsl)).NotTo(gomega.HaveOccurred()) + gomega.Expect(bsl.Spec.ObjectStorage).NotTo(gomega.BeNil()) + gomega.Expect(bsl.Spec.ObjectStorage.CACert).To(gomega.BeEmpty(), "inline CACert should not be set on the Velero BSL") + gomega.Expect(bsl.Spec.ObjectStorage.CACertRef).NotTo(gomega.BeNil(), "CACertRef should be set on the Velero BSL") + + expectedSecretName := "oadp-" + bslName + "-cacert" + gomega.Expect(bsl.Spec.ObjectStorage.CACertRef.Name).To(gomega.Equal(expectedSecretName)) + gomega.Expect(bsl.Spec.ObjectStorage.CACertRef.Key).To(gomega.Equal("cacert")) + + secret, err := kubernetesClientForSuiteRun.CoreV1().Secrets(namespace).Get(context.Background(), expectedSecretName, metav1.GetOptions{}) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + gomega.Expect(secret.Data).To(gomega.HaveKeyWithValue("cacert", caPEM)) + gomega.Expect(secret.Labels[oadpv1alpha1.OadpOperatorLabel]).To(gomega.Equal("True")) + + dpa, err := cloudStorageDpaCR.Get() + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + ownedByDPA := false + for _, ref := range secret.OwnerReferences { + if ref.Kind == "DataProtectionApplication" && ref.Name == dpa.Name { + ownedByDPA = true + } + } + gomega.Expect(ownedByDPA).To(gomega.BeTrue(), "the generated CACert secret should be owned by the DPA") + + ginkgo.GinkgoWriter.Println("cacert: removing inline CACert should clean up the generated Secret") + gomega.Expect(cloudStorageDpaCR.CreateOrUpdate(buildCloudStorageDpaSpec(nil))).NotTo(gomega.HaveOccurred()) + gomega.Eventually(cloudStorageDpaCR.IsReconciledTrue(), 3*time.Minute, 5*time.Second).Should(gomega.BeTrue()) + gomega.Eventually(func() bool { + _, err := kubernetesClientForSuiteRun.CoreV1().Secrets(namespace).Get(context.Background(), expectedSecretName, metav1.GetOptions{}) + return apierrors.IsNotFound(err) + }, 2*time.Minute, 5*time.Second).Should(gomega.BeTrue(), "generated CACert secret should be deleted once inline CACert is removed") + }) +}) diff --git a/tests/e2e/cacert_suite_test.go b/tests/e2e/cacert_suite_test.go index 73538b1229a..6578602003c 100644 --- a/tests/e2e/cacert_suite_test.go +++ b/tests/e2e/cacert_suite_test.go @@ -25,7 +25,8 @@ var ( var _ = ginkgo.Describe("BSL cacert with in-cluster minio", ginkgo.Ordered, ginkgo.Label("aws"), func() { const ( minioBSLSecretName = "minio-bsl-creds" - testBackupName = "cacert-minio-backup" + minioCACertSecret = "minio-bsl-cacert" + minioCACertKey = "ca.crt" testNamespace = "cacert-minio-test-app" ) @@ -42,7 +43,8 @@ var _ = ginkgo.Describe("BSL cacert with in-cluster minio", ginkgo.Ordered, gink } // Clean up any resources left by a previously interrupted run. - _ = lib.DeleteBackup(runTimeClientForSuiteRun, namespace, testBackupName) + _ = lib.DeleteBackup(runTimeClientForSuiteRun, namespace, "cacert-minio-backup-legacy") + _ = lib.DeleteBackup(runTimeClientForSuiteRun, namespace, "cacert-minio-backup-ref") log.Println("cacert: generating self-signed CA and server certificate") var caKeyPEM []byte @@ -80,12 +82,20 @@ var _ = ginkgo.Describe("BSL cacert with in-cluster minio", ginkgo.Ordered, gink gomega.Expect(err).NotTo(gomega.HaveOccurred(), "creating BSL credentials secret %s", minioBSLSecretName) } + log.Println("cacert: creating a user-managed Secret holding the CA cert for CACertRef") + _, err = kubernetesClientForSuiteRun.CoreV1().Secrets(namespace).Create(ctx, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: minioCACertSecret, Namespace: namespace}, + Data: map[string][]byte{minioCACertKey: caPEM}, + }, metav1.CreateOptions{}) + if err != nil && !apierrors.IsAlreadyExists(err) { + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "creating CACertRef secret %s", minioCACertSecret) + } + // kubevirt/hypershift plugins lack arm64-compatible images in some environments. cacertDpaCR.BSLSecretName = minioBSLSecretName cacertDpaCR.BSLProvider = dpaCR.BSLProvider cacertDpaCR.BSLBucket = lib.MinioBucketName cacertDpaCR.BSLBucketPrefix = "e2e" - cacertDpaCR.BSLCacert = caPEM cacertDpaCR.BSLConfig = map[string]string{ "s3Url": minioURL, "s3ForcePathStyle": "true", @@ -103,17 +113,21 @@ var _ = ginkgo.Describe("BSL cacert with in-cluster minio", ginkgo.Ordered, gink ginkgo.AfterAll(func(ctx ginkgo.SpecContext) { lib.DeleteMinioResources(ctx, kubernetesClientForSuiteRun, namespace) _ = lib.DeleteSecret(kubernetesClientForSuiteRun, namespace, minioBSLSecretName) + _ = lib.DeleteSecret(kubernetesClientForSuiteRun, namespace, minioCACertSecret) }) ginkgo.AfterEach(func(ctx ginkgo.SpecContext) { if !skipMustGather && ctx.SpecReport().Failed() { _ = lib.RunMustGather(artifact_dir, cacertDpaCR.Client) } - // Happy path: the positive It block exercised deletion via DeleteBackupRequest. + // Happy path: the positive It blocks exercise deletion via DeleteBackupRequest. // Fallback: direct CR delete when the test failed before reaching that step. if ctx.SpecReport().Failed() { - if err := lib.DeleteBackup(runTimeClientForSuiteRun, namespace, testBackupName); err != nil { - log.Printf("cacert: warning: could not delete backup CR %s: %v", testBackupName, err) + if err := lib.DeleteBackup(runTimeClientForSuiteRun, namespace, "cacert-minio-backup-legacy"); err != nil { + log.Printf("cacert: warning: could not delete backup CR: %v", err) + } + if err := lib.DeleteBackup(runTimeClientForSuiteRun, namespace, "cacert-minio-backup-ref"); err != nil { + log.Printf("cacert: warning: could not delete backup CR: %v", err) } } // Clean up whichever DPA was created this run (positive or negative test). @@ -129,7 +143,10 @@ var _ = ginkgo.Describe("BSL cacert with in-cluster minio", ginkgo.Ordered, gink // ── Tests ────────────────────────────────────────────────────────────────── - ginkgo.It("BSL is Available and Velero gets AWS_CA_BUNDLE when using minio with custom TLS", func(ctx ginkgo.SpecContext) { + // verifyBSLWithCACertBackup drives the DPA through reconcile, asserts the CA cert was + // actually used (BSL Available, AWS_CA_BUNDLE set, ConfigMap populated), and proves it + // end-to-end with a real backup + delete against minio over the custom-CA TLS connection. + verifyBSLWithCACertBackup := func(ctx ginkgo.SpecContext, testBackupName string) { gomega.Expect(cacertDpaCR.CreateOrUpdate(cacertDpaCR.Build(lib.CSI))).NotTo(gomega.HaveOccurred()) gomega.Eventually(cacertDpaCR.IsReconciledTrue(), 3*time.Minute, 5*time.Second).Should(gomega.BeTrue()) gomega.Eventually(lib.VeleroPodIsRunning(kubernetesClientForSuiteRun, namespace), 3*time.Minute, 5*time.Second).Should(gomega.BeTrue()) @@ -171,6 +188,21 @@ var _ = ginkgo.Describe("BSL cacert with in-cluster minio", ginkgo.Ordered, gink runTimeClientForSuiteRun, kubernetesClientForSuiteRun, kubeConfig, namespace, testBackupName, "", )).NotTo(gomega.HaveOccurred()) + } + + ginkgo.It("BSL is Available and Velero gets AWS_CA_BUNDLE when using minio with custom TLS (legacy inline CACert)", func(ctx ginkgo.SpecContext) { + cacertDpaCR.BSLCacert = caPEM + cacertDpaCR.BSLCacertRef = nil + verifyBSLWithCACertBackup(ctx, "cacert-minio-backup-legacy") + }) + + ginkgo.It("BSL is Available and Velero gets AWS_CA_BUNDLE when using minio with custom TLS (CACertRef, default)", func(ctx ginkgo.SpecContext) { + cacertDpaCR.BSLCacert = nil + cacertDpaCR.BSLCacertRef = &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: minioCACertSecret}, + Key: minioCACertKey, + } + verifyBSLWithCACertBackup(ctx, "cacert-minio-backup-ref") }) ginkgo.It("BSL without CACert does not become Available against minio with self-signed TLS", func(ctx ginkgo.SpecContext) { diff --git a/tests/e2e/lib/dpa_helpers.go b/tests/e2e/lib/dpa_helpers.go index 72781d39647..d203f3e6e21 100644 --- a/tests/e2e/lib/dpa_helpers.go +++ b/tests/e2e/lib/dpa_helpers.go @@ -40,6 +40,7 @@ type DpaCustomResource struct { BSLProvider string BSLBucket string BSLCacert []byte + BSLCacertRef *corev1.SecretKeySelector BSLBucketPrefix string VeleroDefaultPlugins []oadpv1alpha1.DefaultPlugin SnapshotLocations []oadpv1alpha1.SnapshotLocation @@ -123,9 +124,10 @@ func (v *DpaCustomResource) BackupStorageLocationSpec() *velero.BackupStorageLoc }, StorageType: velero.StorageType{ ObjectStorage: &velero.ObjectStorageLocation{ - Bucket: v.BSLBucket, - Prefix: v.BSLBucketPrefix, - CACert: v.BSLCacert, + Bucket: v.BSLBucket, + Prefix: v.BSLBucketPrefix, + CACert: v.BSLCacert, + CACertRef: v.BSLCacertRef, }, }, }