Skip to content

Commit 87e94f1

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #235 from openshift-cherrypick-robot/cherry-pick-229-to-oadp-1.6
[oadp-1.6] OADP-8340: Error when modifying DPA-managed BSL via --cacert and --credential
2 parents 3a436a3 + c974e61 commit 87e94f1

3 files changed

Lines changed: 324 additions & 1 deletion

File tree

‎cmd/bsl_guard.go‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
/*
2+
Copyright 2025 The OADP CLI Contributors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package cmd
18+
19+
import (
20+
"context"
21+
"fmt"
22+
"strings"
23+
24+
"github.com/spf13/cobra"
25+
"github.com/spf13/pflag"
26+
velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
27+
clientcmd "github.com/vmware-tanzu/velero/pkg/client"
28+
kbclient "sigs.k8s.io/controller-runtime/pkg/client"
29+
)
30+
31+
// bslDPAManagedError returns an error if the BSL has an ownerReference pointing to a
32+
// DataProtectionApplication, indicating the location is managed by the DPA reconciler.
33+
// This is the testable core — it accepts a pre-built client so tests can pass a fake.
34+
func bslDPAManagedError(ctx context.Context, kbClient kbclient.Client, namespace, bslName string) error {
35+
location := &velerov1.BackupStorageLocation{}
36+
if err := kbClient.Get(ctx, kbclient.ObjectKey{
37+
Namespace: namespace,
38+
Name: bslName,
39+
}, location); err != nil {
40+
return err
41+
}
42+
43+
for _, ref := range location.OwnerReferences {
44+
if ref.Kind == "DataProtectionApplication" && strings.HasPrefix(ref.APIVersion, "oadp.openshift.io/") {
45+
return fmt.Errorf(
46+
"backup storage location %q is managed by DataProtectionApplication %q.\n"+
47+
"Direct modifications via 'oc oadp backup-location set' will be overwritten by the DPA reconciler.\n"+
48+
"To change these settings, update the DataProtectionApplication spec",
49+
bslName, ref.Name,
50+
)
51+
}
52+
}
53+
return nil
54+
}
55+
56+
// checkBSLNotDPAManaged is the factory-aware wrapper used by the CLI command's PreRunE.
57+
func checkBSLNotDPAManaged(ctx context.Context, f clientcmd.Factory, bslName string) error {
58+
kbClient, err := f.KubebuilderClient()
59+
if err != nil {
60+
return err
61+
}
62+
return bslDPAManagedError(ctx, kbClient, f.Namespace(), bslName)
63+
}
64+
65+
// onlyDefaultFlagChanged returns true if --default is the only flag that was changed.
66+
// This is the allowlist check: --default is the one flag the DPA reconciler preserves,
67+
// so it is safe to set on DPA-managed BSLs. Any other changed flag risks being overwritten.
68+
func onlyDefaultFlagChanged(c *cobra.Command) bool {
69+
onlyDefault := true
70+
c.Flags().Visit(func(f *pflag.Flag) {
71+
if f.Name != "default" {
72+
onlyDefault = false
73+
}
74+
})
75+
return onlyDefault
76+
}
77+
78+
// injectDPAManagedGuard wraps the "set" subcommand of the given backup-location command
79+
// with a PreRunE that rejects modifications to DPA-managed BSLs before the update is attempted.
80+
// Uses an allowlist: only --default is permitted on DPA-managed BSLs. Any other changed flag
81+
// triggers the guard, so future upstream flags are automatically protected without code changes.
82+
func injectDPAManagedGuard(bslCmd *cobra.Command, f clientcmd.Factory) {
83+
for _, sub := range bslCmd.Commands() {
84+
if strings.HasPrefix(sub.Use, "set ") {
85+
sub.PreRunE = wrapPreRunE(sub.PreRunE, func(c *cobra.Command, args []string) error {
86+
if len(args) == 0 {
87+
return nil
88+
}
89+
// Allow if no flags were changed or if --default is the only changed flag.
90+
if !c.Flags().HasFlags() || onlyDefaultFlagChanged(c) {
91+
return nil
92+
}
93+
return checkBSLNotDPAManaged(c.Context(), f, args[0])
94+
})
95+
return
96+
}
97+
}
98+
}

‎cmd/bsl_guard_test.go‎

Lines changed: 222 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,222 @@
1+
/*
2+
Copyright 2025 The OADP CLI Contributors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package cmd
18+
19+
import (
20+
"context"
21+
"strings"
22+
"testing"
23+
24+
"github.com/spf13/cobra"
25+
velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
26+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
27+
"k8s.io/apimachinery/pkg/runtime"
28+
kbfake "sigs.k8s.io/controller-runtime/pkg/client/fake"
29+
)
30+
31+
func TestBSLDPAManagedError(t *testing.T) {
32+
const ns = "openshift-adp"
33+
34+
scheme := runtime.NewScheme()
35+
if err := velerov1.AddToScheme(scheme); err != nil {
36+
t.Fatalf("failed to add velero scheme: %v", err)
37+
}
38+
39+
tests := []struct {
40+
name string
41+
bsl *velerov1.BackupStorageLocation
42+
bslName string
43+
wantErr bool
44+
errContains string
45+
}{
46+
{
47+
name: "standalone BSL without ownerReference is allowed",
48+
bslName: "standalone",
49+
bsl: &velerov1.BackupStorageLocation{
50+
ObjectMeta: metav1.ObjectMeta{
51+
Name: "standalone",
52+
Namespace: ns,
53+
},
54+
},
55+
wantErr: false,
56+
},
57+
{
58+
name: "BSL owned by DPA is rejected",
59+
bslName: "default",
60+
bsl: &velerov1.BackupStorageLocation{
61+
ObjectMeta: metav1.ObjectMeta{
62+
Name: "default",
63+
Namespace: ns,
64+
OwnerReferences: []metav1.OwnerReference{
65+
{
66+
APIVersion: "oadp.openshift.io/v1alpha1",
67+
Kind: "DataProtectionApplication",
68+
Name: "velero",
69+
},
70+
},
71+
},
72+
},
73+
wantErr: true,
74+
errContains: "managed by DataProtectionApplication",
75+
},
76+
{
77+
name: "error message names the DPA",
78+
bslName: "default",
79+
bsl: &velerov1.BackupStorageLocation{
80+
ObjectMeta: metav1.ObjectMeta{
81+
Name: "default",
82+
Namespace: ns,
83+
OwnerReferences: []metav1.OwnerReference{
84+
{
85+
APIVersion: "oadp.openshift.io/v1alpha1",
86+
Kind: "DataProtectionApplication",
87+
Name: "my-dpa",
88+
},
89+
},
90+
},
91+
},
92+
wantErr: true,
93+
errContains: "my-dpa",
94+
},
95+
{
96+
name: "DataProtectionApplication kind with wrong API group is allowed",
97+
bslName: "foreign-dpa",
98+
bsl: &velerov1.BackupStorageLocation{
99+
ObjectMeta: metav1.ObjectMeta{
100+
Name: "foreign-dpa",
101+
Namespace: ns,
102+
OwnerReferences: []metav1.OwnerReference{
103+
{
104+
APIVersion: "other.io/v1",
105+
Kind: "DataProtectionApplication",
106+
Name: "some-other-dpa",
107+
},
108+
},
109+
},
110+
},
111+
wantErr: false,
112+
},
113+
{
114+
name: "BSL with unrelated ownerReference is allowed",
115+
bslName: "other-owned",
116+
bsl: &velerov1.BackupStorageLocation{
117+
ObjectMeta: metav1.ObjectMeta{
118+
Name: "other-owned",
119+
Namespace: ns,
120+
OwnerReferences: []metav1.OwnerReference{
121+
{
122+
APIVersion: "v1",
123+
Kind: "ConfigMap",
124+
Name: "some-config",
125+
},
126+
},
127+
},
128+
},
129+
wantErr: false,
130+
},
131+
{
132+
name: "non-existent BSL returns not-found error",
133+
bslName: "does-not-exist",
134+
bsl: nil,
135+
wantErr: true,
136+
},
137+
}
138+
139+
for _, tt := range tests {
140+
t.Run(tt.name, func(t *testing.T) {
141+
var objs []runtime.Object
142+
if tt.bsl != nil {
143+
objs = append(objs, tt.bsl)
144+
}
145+
fakeClient := kbfake.NewClientBuilder().
146+
WithScheme(scheme).
147+
WithRuntimeObjects(objs...).
148+
Build()
149+
150+
err := bslDPAManagedError(context.Background(), fakeClient, ns, tt.bslName)
151+
if tt.wantErr && err == nil {
152+
t.Errorf("expected error but got nil")
153+
}
154+
if !tt.wantErr && err != nil {
155+
t.Errorf("expected no error but got: %v", err)
156+
}
157+
if tt.errContains != "" && err != nil && !strings.Contains(err.Error(), tt.errContains) {
158+
t.Errorf("error %q does not contain %q", err.Error(), tt.errContains)
159+
}
160+
})
161+
}
162+
}
163+
164+
func TestOnlyDefaultFlagChanged(t *testing.T) {
165+
tests := []struct {
166+
name string
167+
flags map[string]string
168+
want bool
169+
}{
170+
{
171+
name: "no flags changed",
172+
flags: map[string]string{},
173+
want: true,
174+
},
175+
{
176+
name: "only --default changed",
177+
flags: map[string]string{"default": "true"},
178+
want: true,
179+
},
180+
{
181+
name: "only --cacert changed",
182+
flags: map[string]string{"cacert": "/path/to/cert"},
183+
want: false,
184+
},
185+
{
186+
name: "only --credential changed",
187+
flags: map[string]string{"credential": "secret/key"},
188+
want: false,
189+
},
190+
{
191+
name: "--default and --cacert both changed",
192+
flags: map[string]string{"default": "true", "cacert": "/path/to/cert"},
193+
want: false,
194+
},
195+
{
196+
name: "unknown future flag changed",
197+
flags: map[string]string{"some-new-flag": "value"},
198+
want: false,
199+
},
200+
}
201+
202+
for _, tt := range tests {
203+
t.Run(tt.name, func(t *testing.T) {
204+
cmd := &cobra.Command{}
205+
cmd.Flags().Bool("default", false, "")
206+
cmd.Flags().String("cacert", "", "")
207+
cmd.Flags().String("credential", "", "")
208+
cmd.Flags().String("some-new-flag", "", "")
209+
210+
for name, val := range tt.flags {
211+
if err := cmd.Flags().Set(name, val); err != nil {
212+
t.Fatalf("failed to set flag %q: %v", name, err)
213+
}
214+
}
215+
216+
got := onlyDefaultFlagChanged(cmd)
217+
if got != tt.want {
218+
t.Errorf("onlyDefaultFlagChanged() = %v, want %v", got, tt.want)
219+
}
220+
})
221+
}
222+
}

‎cmd/root.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -441,6 +441,9 @@ func NewVeleroRootCommand(baseName string) *cobra.Command {
441441
// Nonadmin commands continue using GetCurrentNamespace() for security isolation.
442442
f.BindFlags(c.PersistentFlags())
443443

444+
bslCmd := backuplocation.NewCommand(f)
445+
injectDPAManagedGuard(bslCmd, f)
446+
444447
c.AddCommand(
445448
backup.NewCommand(f),
446449
schedule.NewCommand(f),
@@ -452,7 +455,7 @@ func NewVeleroRootCommand(baseName string) *cobra.Command {
452455
veldelete.NewCommand(f),
453456
cliclient.NewCommand(),
454457
completion.NewCommand(),
455-
backuplocation.NewCommand(f),
458+
bslCmd,
456459
snapshotlocation.NewCommand(f),
457460
debug.NewCommand(f),
458461
)

0 commit comments

Comments
 (0)