diff --git a/internal/controller/dataprotectiontest_controller.go b/internal/controller/dataprotectiontest_controller.go index 07e96f77ac3..76809fd60d8 100644 --- a/internal/controller/dataprotectiontest_controller.go +++ b/internal/controller/dataprotectiontest_controller.go @@ -142,8 +142,10 @@ func (r *DataProtectionTestReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, fmt.Errorf("resolved BackupLocationSpec is nil") } - // Retrieve the CAs if provided - caPEMData, err := r.retrieveCAData(ctx, resolvedBackupLocationSpec) + // Retrieve the CAs if provided. Resolved once here and reused for vendor + // detection and provider initialization so a reconcile uses a single CA + // value and performs a single Secret lookup. + caPEMData, err := r.resolveCAData(ctx, resolvedBackupLocationSpec) if err != nil { logger.Error(err, "failed to retrieve CA data from BackupLocation") r.updateDPTErrorStatus(ctx, fmt.Sprintf("failed to retrieve CA data from BackupLocation: %v", err)) @@ -161,7 +163,7 @@ func (r *DataProtectionTestReconciler) Reconcile(ctx context.Context, req ctrl.R if cfg := r.dpt.Spec.UploadSpeedTestConfig; cfg != nil { logger.Info("Initializing cloud provider for upload test...") - cp, err := r.initializeProvider(ctx, resolvedBackupLocationSpec) + cp, err := r.initializeProvider(ctx, resolvedBackupLocationSpec, caPEMData) if err != nil { logger.Error(err, "failed to initialize cloud provider") r.updateDPTErrorStatus(ctx, fmt.Sprintf("cloud provider init failed: %v", err)) @@ -287,8 +289,9 @@ func (r *DataProtectionTestReconciler) determineVendor(ctx context.Context, dpt // initializeProvider reads the BackupLocationSpec from the DPT CR, // retrieves the associated credentials from a Secret, and returns an initialized -// CloudProvider -func (r *DataProtectionTestReconciler) initializeProvider(ctx context.Context, backupLocationSpec *velerov1.BackupStorageLocationSpec) (cloudprovider.CloudProvider, error) { +// CloudProvider. caCertData is the already-resolved CA bundle (see +// resolveCAData); it is not looked up again here. +func (r *DataProtectionTestReconciler) initializeProvider(ctx context.Context, backupLocationSpec *velerov1.BackupStorageLocationSpec, caCertData []byte) (cloudprovider.CloudProvider, error) { if backupLocationSpec == nil { return nil, fmt.Errorf("backupLocationSpec is nil") @@ -297,11 +300,6 @@ func (r *DataProtectionTestReconciler) initializeProvider(ctx context.Context, b providerName := strings.ToLower(backupLocationSpec.Provider) //TODO handle credential when not specified - caCertData, err := r.retrieveCAData(ctx, backupLocationSpec) - if err != nil { - return nil, fmt.Errorf("cannot retrieve CA Certificate data: %s", err.Error()) - } - switch providerName { case AWSProvider: return r.initializeAWSProvider(ctx, backupLocationSpec, caCertData) @@ -727,6 +725,25 @@ func (r *DataProtectionTestReconciler) updateDPTStatusToComplete(ctx context.Con }) } +// resolveCAData returns the CA bundle the DPT should trust for this +// reconciliation, or nil when none applies: +// - skipTLSVerify=true: no CA is needed, so the CACertRef Secret is not read +// (a missing Secret must not fail a DPT that disables verification). +// - non-AWS providers: only the AWS/S3-compatible path configures client +// trust from a CA bundle today, so resolving it for GCP/Azure would add a +// Secret dependency without changing any TLS behavior. +// +// Otherwise it defers to retrieveCAData, which owns CA selection. +func (r *DataProtectionTestReconciler) resolveCAData(ctx context.Context, backupLocationSpec *velerov1.BackupStorageLocationSpec) ([]byte, error) { + if r.dpt != nil && r.dpt.Spec.SkipTLSVerify { + return nil, nil + } + if backupLocationSpec == nil || !strings.EqualFold(backupLocationSpec.Provider, AWSProvider) { + return nil, nil + } + return r.retrieveCAData(ctx, backupLocationSpec) +} + // retrieveCAData returns the PEM-encoded CA certificate bytes for the given // BackupStorageLocationSpec. CACertRef (a Secret reference) takes priority over // the inline CACert field, matching Velero's own resolution order. diff --git a/internal/controller/dataprotectiontest_controller_test.go b/internal/controller/dataprotectiontest_controller_test.go index c24f2bf242b..7f1e059f54d 100644 --- a/internal/controller/dataprotectiontest_controller_test.go +++ b/internal/controller/dataprotectiontest_controller_test.go @@ -347,7 +347,7 @@ aws_secret_access_key = test-secret }, } - cp, err := reconciler.initializeProvider(ctx, spec) + cp, err := reconciler.initializeProvider(ctx, spec, nil) if tt.expectError { require.Error(t, err) @@ -1391,6 +1391,32 @@ func TestRetrieveCAData(t *testing.T) { }, expectBytes: []byte("more bad ca data"), }, + { + name: "ca cert ref takes precedence over inline ca cert", + bsl: &velerov1.BackupStorageLocationSpec{ + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + CACert: []byte("inline ca data"), + CACertRef: &corev1.SecretKeySelector{ + Key: "ca", + LocalObjectReference: corev1.LocalObjectReference{ + Name: "casecret", + }, + }, + }, + }, + }, + startingSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "casecret", + Namespace: namespace, + }, + Data: map[string][]byte{ + "ca": []byte("secret ca data"), + }, + }, + expectBytes: []byte("secret ca data"), + }, { name: "missing ca cert ref secret", bsl: &velerov1.BackupStorageLocationSpec{ @@ -1489,3 +1515,96 @@ func TestRetrieveCAData(t *testing.T) { } } + +func TestResolveCAData(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, oadpv1alpha1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + namespace := "dpt-test" + + refBSL := func(provider string) *velerov1.BackupStorageLocationSpec { + return &velerov1.BackupStorageLocationSpec{ + Provider: provider, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + CACertRef: &corev1.SecretKeySelector{ + Key: "ca", + LocalObjectReference: corev1.LocalObjectReference{Name: "casecret"}, + }, + }, + }, + } + } + caSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "casecret", Namespace: namespace}, + Data: map[string][]byte{"ca": []byte("secret ca data")}, + } + + tests := []struct { + name string + skipTLSVerify bool + bsl *velerov1.BackupStorageLocationSpec + startingSecret *corev1.Secret + expectErr bool + expectBytes []byte + }{ + { + name: "aws resolves the referenced secret", + bsl: refBSL("aws"), + startingSecret: caSecret, + expectBytes: []byte("secret ca data"), + }, + { + name: "skipTLSVerify does not read a missing ca secret", + skipTLSVerify: true, + bsl: refBSL("aws"), + }, + { + name: "aws with a missing ca secret still errors when verification is on", + bsl: refBSL("aws"), + // no secret created + expectErr: true, + }, + { + name: "gcp does not add a secret dependency", + bsl: refBSL("gcp"), + // no secret created: must not be looked up + }, + { + name: "azure does not add a secret dependency", + bsl: refBSL("azure"), + }, + { + name: "nil spec", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + builder := fake.NewClientBuilder().WithScheme(scheme) + if tt.startingSecret != nil { + builder = builder.WithObjects(tt.startingSecret) + } + fakeClient := builder.Build() + + reconciler := &DataProtectionTestReconciler{ + Client: fakeClient, + Log: logr.Discard(), + NamespacedName: types.NamespacedName{Namespace: namespace, Name: "test-obj"}, + Context: context.Background(), + dpt: &oadpv1alpha1.DataProtectionTest{ + Spec: oadpv1alpha1.DataProtectionTestSpec{SkipTLSVerify: tt.skipTLSVerify}, + }, + } + + caData, err := reconciler.resolveCAData(context.Background(), tt.bsl) + if tt.expectErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + require.Equal(t, tt.expectBytes, caData) + }) + } +} diff --git a/tests/e2e/lib/dpt.go b/tests/e2e/lib/dpt.go index eccc45b57d9..7f37e0b2c57 100644 --- a/tests/e2e/lib/dpt.go +++ b/tests/e2e/lib/dpt.go @@ -86,5 +86,11 @@ func CreateDPTAndAssertComplete(c client.Client, namespace, bslName string) erro if dpt.Status.Phase != "Complete" { return fmt.Errorf("DataProtectionTest %s reached phase %q (error: %s)", dpt.Name, dpt.Status.Phase, dpt.Status.ErrorMessage) } + // The reconciler marks the DPT Complete even when the upload itself failed + // and records the outcome in status.uploadTest, so phase alone is not + // enough: a TLS failure reaching the bucket would otherwise pass here. + if !dpt.Status.UploadTest.Success { + return fmt.Errorf("DataProtectionTest %s completed but the upload test failed: %s", dpt.Name, dpt.Status.UploadTest.ErrorMessage) + } return nil }