Skip to content

fix: address shubham's review feedback on DPT CACertRef handling #2

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
msfrucht merged 1 commit into from
Oct 5, 2026
Merged

fix: address shubham's review feedback on DPT CACertRef handling #2

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
37 changes: 27 additions & 10 deletions internal/controller/dataprotectiontest_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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))
Expand Down Expand Up @@ -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")
Expand All @@ -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)
Expand Down Expand Up @@ -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) {

@msfrucht msfrucht Oct 5, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The Azure/GCP plugins don't respect providing custom CAs anyways.

I don't see how this helps, anyone doing so is configuring the object incorrectly. But I don't see this hurting to force ignoring for anything other than aws.

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.
Expand Down
121 changes: 120 additions & 1 deletion internal/controller/dataprotectiontest_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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(),

@msfrucht msfrucht Oct 5, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Should be t.Context(). I'll submit a commit to fix this.

@kaovilai kaovilai Oct 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks

dpt: &oadpv1alpha1.DataProtectionTest{
Spec: oadpv1alpha1.DataProtectionTestSpec{SkipTLSVerify: tt.skipTLSVerify},
},
}

caData, err := reconciler.resolveCAData(context.Background(), tt.bsl)

@msfrucht msfrucht Oct 5, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Same here.

if tt.expectErr {
require.Error(t, err)
} else {
require.NoError(t, err)
}
require.Equal(t, tt.expectBytes, caData)
})
}
}
6 changes: 6 additions & 0 deletions tests/e2e/lib/dpt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@msfrucht msfrucht Oct 5, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

If there is ever a DPTv2 I would ask this to change.

One expects when the phase is "Completed" all parts were successful and otherwise either PartialFailed or PartialComplete or PartialSuccess or similar. With a condition array added to explain.

@kaovilai kaovilai Oct 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@@shubham-pampattiwar thoughts?

// 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
}