Merge pull request #10554 from pujitha24/auto/issue-10551

Propagate caller context in repository manager Forget/BatchForget

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

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
 }

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")
+}