OADP-8340: Error when modifying DPA-managed BSL via --cacert and --credential

cmd/bsl_guard.go

@@ -0,0 +1,98 @@
+/*
+Copyright 2025 The OADP CLI Contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package cmd
+
+import (
+	"context"
+	"fmt"
+	"strings"
+
+	"github.com/spf13/cobra"
+	"github.com/spf13/pflag"
+	velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
+	clientcmd "github.com/vmware-tanzu/velero/pkg/client"
+	kbclient "sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+// bslDPAManagedError returns an error if the BSL has an ownerReference pointing to a
+// DataProtectionApplication, indicating the location is managed by the DPA reconciler.
+// This is the testable core — it accepts a pre-built client so tests can pass a fake.
+func bslDPAManagedError(ctx context.Context, kbClient kbclient.Client, namespace, bslName string) error {
+	location := &velerov1.BackupStorageLocation{}
+	if err := kbClient.Get(ctx, kbclient.ObjectKey{
+		Namespace: namespace,
+		Name:      bslName,
+	}, location); err != nil {
+		return err
+	}
+
+	for _, ref := range location.OwnerReferences {
+		if ref.Kind == "DataProtectionApplication" && strings.HasPrefix(ref.APIVersion, "oadp.openshift.io/") {
+			return fmt.Errorf(
+				"backup storage location %q is managed by DataProtectionApplication %q.\n"+
+					"Direct modifications via 'oc oadp backup-location set' will be overwritten by the DPA reconciler.\n"+
+					"To change these settings, update the DataProtectionApplication spec",
+				bslName, ref.Name,
+			)
+		}
+	}
+	return nil
+}
+
+// checkBSLNotDPAManaged is the factory-aware wrapper used by the CLI command's PreRunE.
+func checkBSLNotDPAManaged(ctx context.Context, f clientcmd.Factory, bslName string) error {
+	kbClient, err := f.KubebuilderClient()
+	if err != nil {
+		return err
+	}
+	return bslDPAManagedError(ctx, kbClient, f.Namespace(), bslName)
+}
+
+// onlyDefaultFlagChanged returns true if --default is the only flag that was changed.
+// This is the allowlist check: --default is the one flag the DPA reconciler preserves,
+// so it is safe to set on DPA-managed BSLs. Any other changed flag risks being overwritten.
+func onlyDefaultFlagChanged(c *cobra.Command) bool {
+	onlyDefault := true
+	c.Flags().Visit(func(f *pflag.Flag) {
+		if f.Name != "default" {
+			onlyDefault = false
+		}
+	})
+	return onlyDefault
+}
+
+// injectDPAManagedGuard wraps the "set" subcommand of the given backup-location command
+// with a PreRunE that rejects modifications to DPA-managed BSLs before the update is attempted.
+// Uses an allowlist: only --default is permitted on DPA-managed BSLs. Any other changed flag
+// triggers the guard, so future upstream flags are automatically protected without code changes.
+func injectDPAManagedGuard(bslCmd *cobra.Command, f clientcmd.Factory) {
+	for _, sub := range bslCmd.Commands() {
+		if strings.HasPrefix(sub.Use, "set ") {
+			sub.PreRunE = wrapPreRunE(sub.PreRunE, func(c *cobra.Command, args []string) error {
+				if len(args) == 0 {
+					return nil
+				}
+				// Allow if no flags were changed or if --default is the only changed flag.
+				if !c.Flags().HasFlags() || onlyDefaultFlagChanged(c) {
+					return nil
+				}
+				return checkBSLNotDPAManaged(c.Context(), f, args[0])
+			})
+			return
+		}
+	}
+}

cmd/bsl_guard_test.go

@@ -0,0 +1,222 @@
+/*
+Copyright 2025 The OADP CLI Contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package cmd
+
+import (
+	"context"
+	"strings"
+	"testing"
+
+	"github.com/spf13/cobra"
+	velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	kbfake "sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+func TestBSLDPAManagedError(t *testing.T) {
+	const ns = "openshift-adp"
+
+	scheme := runtime.NewScheme()
+	if err := velerov1.AddToScheme(scheme); err != nil {
+		t.Fatalf("failed to add velero scheme: %v", err)
+	}
+
+	tests := []struct {
+		name        string
+		bsl         *velerov1.BackupStorageLocation
+		bslName     string
+		wantErr     bool
+		errContains string
+	}{
+		{
+			name:    "standalone BSL without ownerReference is allowed",
+			bslName: "standalone",
+			bsl: &velerov1.BackupStorageLocation{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "standalone",
+					Namespace: ns,
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name:    "BSL owned by DPA is rejected",
+			bslName: "default",
+			bsl: &velerov1.BackupStorageLocation{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "default",
+					Namespace: ns,
+					OwnerReferences: []metav1.OwnerReference{
+						{
+							APIVersion: "oadp.openshift.io/v1alpha1",
+							Kind:       "DataProtectionApplication",
+							Name:       "velero",
+						},
+					},
+				},
+			},
+			wantErr:     true,
+			errContains: "managed by DataProtectionApplication",
+		},
+		{
+			name:    "error message names the DPA",
+			bslName: "default",
+			bsl: &velerov1.BackupStorageLocation{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "default",
+					Namespace: ns,
+					OwnerReferences: []metav1.OwnerReference{
+						{
+							APIVersion: "oadp.openshift.io/v1alpha1",
+							Kind:       "DataProtectionApplication",
+							Name:       "my-dpa",
+						},
+					},
+				},
+			},
+			wantErr:     true,
+			errContains: "my-dpa",
+		},
+		{
+			name:    "DataProtectionApplication kind with wrong API group is allowed",
+			bslName: "foreign-dpa",
+			bsl: &velerov1.BackupStorageLocation{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "foreign-dpa",
+					Namespace: ns,
+					OwnerReferences: []metav1.OwnerReference{
+						{
+							APIVersion: "other.io/v1",
+							Kind:       "DataProtectionApplication",
+							Name:       "some-other-dpa",
+						},
+					},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name:    "BSL with unrelated ownerReference is allowed",
+			bslName: "other-owned",
+			bsl: &velerov1.BackupStorageLocation{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "other-owned",
+					Namespace: ns,
+					OwnerReferences: []metav1.OwnerReference{
+						{
+							APIVersion: "v1",
+							Kind:       "ConfigMap",
+							Name:       "some-config",
+						},
+					},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name:    "non-existent BSL returns not-found error",
+			bslName: "does-not-exist",
+			bsl:     nil,
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var objs []runtime.Object
+			if tt.bsl != nil {
+				objs = append(objs, tt.bsl)
+			}
+			fakeClient := kbfake.NewClientBuilder().
+				WithScheme(scheme).
+				WithRuntimeObjects(objs...).
+				Build()
+
+			err := bslDPAManagedError(context.Background(), fakeClient, ns, tt.bslName)
+			if tt.wantErr && err == nil {
+				t.Errorf("expected error but got nil")
+			}
+			if !tt.wantErr && err != nil {
+				t.Errorf("expected no error but got: %v", err)
+			}
+			if tt.errContains != "" && err != nil && !strings.Contains(err.Error(), tt.errContains) {
+				t.Errorf("error %q does not contain %q", err.Error(), tt.errContains)
+			}
+		})
+	}
+}
+
+func TestOnlyDefaultFlagChanged(t *testing.T) {
+	tests := []struct {
+		name  string
+		flags map[string]string
+		want  bool
+	}{
+		{
+			name:  "no flags changed",
+			flags: map[string]string{},
+			want:  true,
+		},
+		{
+			name:  "only --default changed",
+			flags: map[string]string{"default": "true"},
+			want:  true,
+		},
+		{
+			name:  "only --cacert changed",
+			flags: map[string]string{"cacert": "/path/to/cert"},
+			want:  false,
+		},
+		{
+			name:  "only --credential changed",
+			flags: map[string]string{"credential": "secret/key"},
+			want:  false,
+		},
+		{
+			name:  "--default and --cacert both changed",
+			flags: map[string]string{"default": "true", "cacert": "/path/to/cert"},
+			want:  false,
+		},
+		{
+			name:  "unknown future flag changed",
+			flags: map[string]string{"some-new-flag": "value"},
+			want:  false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cmd := &cobra.Command{}
+			cmd.Flags().Bool("default", false, "")
+			cmd.Flags().String("cacert", "", "")
+			cmd.Flags().String("credential", "", "")
+			cmd.Flags().String("some-new-flag", "", "")
+
+			for name, val := range tt.flags {
+				if err := cmd.Flags().Set(name, val); err != nil {
+					t.Fatalf("failed to set flag %q: %v", name, err)
+				}
+			}
+
+			got := onlyDefaultFlagChanged(cmd)
+			if got != tt.want {
+				t.Errorf("onlyDefaultFlagChanged() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

cmd/root.go

@@ -431,6 +431,9 @@ func NewVeleroRootCommand(baseName string) *cobra.Command {
 	// Nonadmin commands continue using GetCurrentNamespace() for security isolation.
 	f.BindFlags(c.PersistentFlags())
 
+	bslCmd := backuplocation.NewCommand(f)
+	injectDPAManagedGuard(bslCmd, f)
+
 	c.AddCommand(
 		backup.NewCommand(f),
 		schedule.NewCommand(f),
@@ -442,7 +445,7 @@ func NewVeleroRootCommand(baseName string) *cobra.Command {
 		veldelete.NewCommand(f),
 		cliclient.NewCommand(),
 		completion.NewCommand(),
-		backuplocation.NewCommand(f),
+		bslCmd,
 		snapshotlocation.NewCommand(f),
 		debug.NewCommand(f),
 	)