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)
This commit is contained in:
Pujitha Paladugu
2026-09-21 03:15:56 -07:00
parent 54d5243923
commit b9047d1e27
2 changed files with 116 additions and 5 deletions
+5 -5
View File
@@ -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
}
+111
View File
@@ -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")
}