From b9047d1e27efdc61c6c6a948246843d6abd68060 Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Mon, 21 Sep 2026 03:15:30 -0700 Subject: [PATCH 1/2] Propagate caller context in repository manager Forget/BatchForget Motivation: Forget and BatchForget in pkg/repository/manager/manager.go accept a caller-provided context.Context but ignore it, hardcoding context.Background() when calling into the repository provider. This means cancellation and timeouts set by callers (e.g. the backup deletion controller) are silently dropped during repository connection and snapshot deletion. Additionally, BatchForget returned a wrapped nil instead of the real connection error when prd.BoostRepoConnect failed, because it referenced an unrelated, already-nil err variable instead of connectErr. Approach: Pass the caller's ctx through to prd.BoostRepoConnect, prd.Forget, and prd.BatchForget in both Forget and BatchForget, instead of substituting context.Background(). Fix BatchForget's connection-failure branch to wrap and return connectErr instead of the stale err. Other methods on manager (InitRepo, ConnectToRepo, PrepareRepo, PruneRepo, UnlockRepo) don't accept a ctx parameter at all, so they are unaffected and out of scope for this change. Validation: - go build ./pkg/repository/... and go build ./... pass. - go vet ./pkg/repository/... is clean. - go test ./pkg/repository/... passes, including three new tests added to pkg/repository/manager/manager_test.go. - golangci-lint run ./pkg/repository/... is clean. - Confirmed the new tests reproduce both bugs: temporarily reverting only manager.go and re-running go test ./pkg/repository/manager/... made all three new tests fail (missing propagated context value and cancellation, and a nil error returned where the real connect error was expected); re-applying the fix makes them pass. This is a silent behavior bug (broken context propagation and a swallowed error), not a crash. Report: https://github.com/velero-io/velero/issues/10551 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code) --- pkg/repository/manager/manager.go | 10 +-- pkg/repository/manager/manager_test.go | 111 +++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 5 deletions(-) diff --git a/pkg/repository/manager/manager.go b/pkg/repository/manager/manager.go index f8b10db5e..f25a1cd29 100644 --- a/pkg/repository/manager/manager.go +++ b/pkg/repository/manager/manager.go @@ -231,11 +231,11 @@ func (m *manager) Forget(ctx context.Context, repo *velerov1api.BackupRepository return errors.WithStack(err) } - if err := prd.BoostRepoConnect(context.Background(), param); err != nil { + if err := prd.BoostRepoConnect(ctx, param); err != nil { return errors.WithStack(err) } - return prd.Forget(context.Background(), snapshot, param) + return prd.Forget(ctx, snapshot, param) } func (m *manager) BatchForget(ctx context.Context, repo *velerov1api.BackupRepository, snapshots []string) []error { @@ -254,15 +254,15 @@ func (m *manager) BatchForget(ctx context.Context, repo *velerov1api.BackupRepos // Disable FIPS-140 compliance check, because Kopia doesn't support FIPS-140 yet. var connectErr error fips140.WithoutEnforcement(func() { - connectErr = prd.BoostRepoConnect(context.Background(), param) + connectErr = prd.BoostRepoConnect(ctx, param) }) if connectErr != nil { - return []error{errors.WithStack(err)} + return []error{errors.WithStack(connectErr)} } forgetErr := make([]error, 0) fips140.WithoutEnforcement(func() { - forgetErr = prd.BatchForget(context.Background(), snapshots, param) + forgetErr = prd.BatchForget(ctx, snapshots, param) }) return forgetErr } diff --git a/pkg/repository/manager/manager_test.go b/pkg/repository/manager/manager_test.go index 88d8db481..585b7e860 100644 --- a/pkg/repository/manager/manager_test.go +++ b/pkg/repository/manager/manager_test.go @@ -17,15 +17,73 @@ limitations under the License. package repository import ( + "context" "testing" + "github.com/cockroachdb/errors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/runtime" kbclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + "github.com/vmware-tanzu/velero/pkg/repository" + "github.com/vmware-tanzu/velero/pkg/repository/provider" ) +// fakeProvider is a minimal provider.Provider implementation used to +// observe the context passed by the manager into the provider calls. +type fakeProvider struct { + provider.Provider + + gotConnectCtx context.Context + gotForgetCtx context.Context + + connectErr error + forgetErr error + forgetErrs []error +} + +func (f *fakeProvider) BoostRepoConnect(ctx context.Context, _ provider.RepoParam) error { + f.gotConnectCtx = ctx + return f.connectErr +} + +func (f *fakeProvider) Forget(ctx context.Context, _ string, _ provider.RepoParam) error { + f.gotForgetCtx = ctx + return f.forgetErr +} + +func (f *fakeProvider) BatchForget(ctx context.Context, _ []string, _ provider.RepoParam) []error { + f.gotForgetCtx = ctx + return f.forgetErrs +} + +func newTestManager(t *testing.T, prd provider.Provider) *manager { + t.Helper() + + scheme := runtime.NewScheme() + require.NoError(t, velerov1.AddToScheme(scheme)) + + bsl := &velerov1.BackupStorageLocation{} + bsl.Namespace = "velero" + bsl.Name = "fake-bsl" + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(bsl).Build() + + mgr := NewManager("velero", fakeClient, repository.NewRepoLocker(), nil, nil, nil).(*manager) + mgr.providers[velerov1.BackupRepositoryTypeKopia] = prd + return mgr +} + +func newTestRepo() *velerov1.BackupRepository { + repo := &velerov1.BackupRepository{} + repo.Spec.RepositoryType = velerov1.BackupRepositoryTypeKopia + repo.Spec.BackupStorageLocation = "fake-bsl" + return repo +} + func TestGetRepositoryProvider(t *testing.T) { var fakeClient kbclient.Client mgr := NewManager("", fakeClient, nil, nil, nil, nil).(*manager) @@ -62,3 +120,56 @@ func TestGetRepositoryConfigProvider(t *testing.T) { _, err = mgr.getRepositoryProvider("restic") require.Error(t, err) } + +func TestForgetPropagatesCallerContext(t *testing.T) { + prd := &fakeProvider{} + mgr := newTestManager(t, prd) + + type ctxKeyType string + key := ctxKeyType("test-key") + ctx, cancel := context.WithCancel(context.WithValue(context.Background(), key, "test-value")) + defer cancel() + + err := mgr.Forget(ctx, newTestRepo(), "snapshot-1") + require.NoError(t, err) + + require.NotNil(t, prd.gotConnectCtx) + require.NotNil(t, prd.gotForgetCtx) + assert.Equal(t, "test-value", prd.gotConnectCtx.Value(key)) + assert.Equal(t, "test-value", prd.gotForgetCtx.Value(key)) + + // canceling the caller's context must be observed by the provider calls, + // proving the manager no longer substitutes context.Background(). + cancel() + require.Error(t, prd.gotConnectCtx.Err()) + require.Error(t, prd.gotForgetCtx.Err()) +} + +func TestBatchForgetPropagatesCallerContext(t *testing.T) { + prd := &fakeProvider{forgetErrs: []error{}} + mgr := newTestManager(t, prd) + + type ctxKeyType string + key := ctxKeyType("test-key") + ctx, cancel := context.WithCancel(context.WithValue(context.Background(), key, "test-value")) + defer cancel() + + errs := mgr.BatchForget(ctx, newTestRepo(), []string{"snapshot-1", "snapshot-2"}) + require.Empty(t, errs) + + require.NotNil(t, prd.gotConnectCtx) + require.NotNil(t, prd.gotForgetCtx) + assert.Equal(t, "test-value", prd.gotConnectCtx.Value(key)) + assert.Equal(t, "test-value", prd.gotForgetCtx.Value(key)) +} + +func TestBatchForgetReturnsConnectError(t *testing.T) { + connectErr := errors.New("boom: connection refused") + prd := &fakeProvider{connectErr: connectErr} + mgr := newTestManager(t, prd) + + errs := mgr.BatchForget(context.Background(), newTestRepo(), []string{"snapshot-1"}) + + require.Len(t, errs, 1) + require.ErrorContains(t, errs[0], "boom: connection refused") +} From 8a2d9db687befd12df704896d096838799eb8d75 Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Mon, 21 Sep 2026 07:37:08 -0700 Subject: [PATCH 2/2] Add changelog for PR 10554 CI's changelog check requires a changelogs/unreleased/- file; this PR didn't have one since the PR number wasn't known until after it was opened. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> --- changelogs/unreleased/10554-pujitha24 | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/10554-pujitha24 diff --git a/changelogs/unreleased/10554-pujitha24 b/changelogs/unreleased/10554-pujitha24 new file mode 100644 index 000000000..cccfbd257 --- /dev/null +++ b/changelogs/unreleased/10554-pujitha24 @@ -0,0 +1 @@ +Propagate caller context in repository manager Forget/BatchForget, and fix BatchForget returning a nil error instead of the real connection error