cloudtestutils: refactor CheckNoPermissions

Use `StoreInfo` for `CheckNoPermissions.` Epic: none Release note: None
cockroachdb · Jan 11, 2025 · f76f279 · f76f279
1 parent 7f8c8eb
commit f76f279
Show file tree

Hide file tree

Showing 4 changed files with 59 additions and 74 deletions.
diff --git a/pkg/cloud/amazon/s3_storage_test.go b/pkg/cloud/amazon/s3_storage_test.go
@@ -210,7 +210,6 @@ func TestPutS3AssumeRole(t *testing.T) {
 		skip.IgnoreLint(t, "AWS_S3_BUCKET env var must be set")
 	}
 
-	testSettings := cluster.MakeTestingClusterSettings()
 	testID := cloudtestutils.NewTestID()
 	testPath := fmt.Sprintf("backup-test-%d", testID)
 
@@ -263,18 +262,19 @@ func TestPutS3AssumeRole(t *testing.T) {
 			t.Run(tc.auth, func(t *testing.T) {
 				// First verify that none of the individual roles in the chain can be used to access the storage.
 				for _, p := range providerChain {
-					roleURI := S3URI(bucket, testPath,
-						&cloudpb.ExternalStorage_S3{
-							Auth:               tc.auth,
-							AssumeRoleProvider: p,
-							AccessKey:          tc.accessKey,
-							Secret:             tc.secretKey,
-							Region:             "us-east-1",
-						},
-					)
-					cloudtestutils.CheckNoPermission(
-						t, roleURI, user, nil /* db */, testSettings,
-					)
+					info := cloudtestutils.StoreInfo{
+						URI: S3URI(bucket, testPath,
+							&cloudpb.ExternalStorage_S3{
+								Auth:               tc.auth,
+								AssumeRoleProvider: p,
+								AccessKey:          tc.accessKey,
+								Secret:             tc.secretKey,
+								Region:             "us-east-1",
+							},
+						),
+						User: user,
+					}
+					cloudtestutils.CheckNoPermission(t, info)
 				}
 
 				// Next check that the role chain without any external IDs cannot be used to
@@ -298,9 +298,7 @@ func TestPutS3AssumeRole(t *testing.T) {
 					),
 					User: user,
 				}
-				cloudtestutils.CheckNoPermission(
-					t, info.URI, user, nil /* db */, testSettings,
-				)
+				cloudtestutils.CheckNoPermission(t, info)
 
 				// Finally, check that the chain of roles can be used to access the storage.
 				info.URI = S3URI(bucket, testPath,

diff --git a/pkg/cloud/azure/azure_storage_test.go b/pkg/cloud/azure/azure_storage_test.go
@@ -263,7 +263,6 @@ func TestAzureStorageFileImplicitAuth(t *testing.T) {
 		skip.IgnoreLint(t, "Test not configured for Azure")
 		return
 	}
-	testSettings := cluster.MakeTestingClusterSettings()
 	testID := cloudtestutils.NewTestID()
 
 	cleanup := envutil.TestSetEnv(t, "AZURE_CLIENT_ID", "")
@@ -272,8 +271,11 @@ func TestAzureStorageFileImplicitAuth(t *testing.T) {
 	testPath := fmt.Sprintf("backup-test-%d", testID)
 	testListPath := fmt.Sprintf("listing-test-%d", testID)
 
-	cloudtestutils.CheckNoPermission(t, cfg.filePathImplicitAuth(testPath), username.RootUserName(),
-		nil /*db*/, testSettings)
+	info := cloudtestutils.StoreInfo{
+		URI:  cfg.filePathImplicitAuth(testPath),
+		User: username.RootUserName(),
+	}
+	cloudtestutils.CheckNoPermission(t, info)
 
 	tmpDir, cleanup2 := testutils.TempDir(t)
 	defer cleanup2()
@@ -284,10 +286,6 @@ func TestAzureStorageFileImplicitAuth(t *testing.T) {
 	cleanup3 := envutil.TestSetEnv(t, "COCKROACH_AZURE_APPLICATION_CREDENTIALS_FILE", credFile)
 	defer cleanup3()
 
-	info := cloudtestutils.StoreInfo{
-		URI:  cfg.filePathImplicitAuth(testPath),
-		User: username.RootUserName(),
-	}
 	cloudtestutils.CheckExportStore(t, info)
 	info.URI = cfg.filePathImplicitAuth(testListPath)
 	cloudtestutils.CheckListFiles(t, info)

diff --git a/pkg/cloud/cloudtestutils/cloud_test_helpers.go b/pkg/cloud/cloudtestutils/cloud_test_helpers.go
@@ -558,24 +558,19 @@ func CheckAntagonisticRead(
 
 // CheckNoPermission checks that we do not have permission to list the external
 // storage at storeURI.
-func CheckNoPermission(
-	t *testing.T,
-	storeURI string,
-	user username.SQLUsername,
-	db isql.DB,
-	testSettings *cluster.Settings,
-) {
+func CheckNoPermission(t *testing.T, info StoreInfo) {
 	ioConf := base.ExternalIODirConfig{}
 	ctx := context.Background()
 
-	conf, err := cloud.ExternalStorageConfFromURI(storeURI, user)
+	conf, err := cloud.ExternalStorageConfFromURI(info.URI, info.User)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	clientFactory := blobs.TestBlobServiceClient(testSettings.ExternalIODir)
+	testSettings := info.testSettings()
+	clientFactory := blobs.TestBlobServiceClient(info.ExternalIODir)
 	s, err := cloud.MakeExternalStorage(
-		ctx, conf, ioConf, testSettings, clientFactory, db, nil, cloud.NilMetrics,
+		ctx, conf, ioConf, testSettings, clientFactory, info.DB, nil, cloud.NilMetrics,
 	)
 	if err != nil {
 		t.Fatal(err)
@@ -584,7 +579,7 @@ func CheckNoPermission(
 
 	err = s.List(ctx, "", "", nil)
 	if err == nil {
-		t.Fatalf("expected error when listing %s with no permissions", storeURI)
+		t.Fatalf("expected error when listing %s with no permissions", info.URI)
 	}
 
 	require.Regexp(t, "(failed|unable) to list", err)

diff --git a/pkg/cloud/gcp/gcs_storage_test.go b/pkg/cloud/gcp/gcs_storage_test.go
@@ -147,7 +147,6 @@ func TestPutGoogleCloud(t *testing.T) {
 
 func TestGCSAssumeRole(t *testing.T) {
 	user := username.RootUserName()
-	testSettings := cluster.MakeTestingClusterSettings()
 
 	limitedBucket := os.Getenv("GOOGLE_LIMITED_BUCKET")
 	if limitedBucket == "" {
@@ -167,27 +166,25 @@ func TestGCSAssumeRole(t *testing.T) {
 		}
 		encoded := base64.StdEncoding.EncodeToString([]byte(credentials))
 
-		// Verify that specified permissions with the credentials do not give us
-		// access to the bucket.
-		cloudtestutils.CheckNoPermission(t, fmt.Sprintf("gs://%s/%s-%d?%s=%s", limitedBucket, "backup-test-assume-role", testID,
-			CredentialsParam, url.QueryEscape(encoded)), user,
-			nil, /* db */
-			testSettings,
-		)
-
 		info := cloudtestutils.StoreInfo{
-			URI: fmt.Sprintf("gs://%s/%s-%d?%s=%s&%s=%s&%s=%s",
-				limitedBucket,
-				"backup-test-assume-role",
-				testID,
-				cloud.AuthParam,
-				cloud.AuthParamSpecified,
-				AssumeRoleParam,
-				assumedAccount, CredentialsParam,
-				url.QueryEscape(encoded),
-			),
+			URI: fmt.Sprintf(
+				"gs://%s/%s-%d?%s=%s", limitedBucket, "backup-test-assume-role", testID,
+				CredentialsParam, url.QueryEscape(encoded)),
 			User: user,
 		}
+		// Verify that specified permissions with the credentials do not give us
+		// access to the bucket.
+		cloudtestutils.CheckNoPermission(t, info)
+		info.URI = fmt.Sprintf("gs://%s/%s-%d?%s=%s&%s=%s&%s=%s",
+			limitedBucket,
+			"backup-test-assume-role",
+			testID,
+			cloud.AuthParam,
+			cloud.AuthParamSpecified,
+			AssumeRoleParam,
+			assumedAccount, CredentialsParam,
+			url.QueryEscape(encoded),
+		)
 		cloudtestutils.CheckExportStore(t, info)
 		info = cloudtestutils.StoreInfo{
 			URI: fmt.Sprintf("gs://%s/%s-%d/%s?%s=%s&%s=%s&%s=%s",
@@ -212,21 +209,18 @@ func TestGCSAssumeRole(t *testing.T) {
 			skip.IgnoreLint(t, err)
 		}
 
-		// Verify that implicit permissions with the credentials do not give us
-		// access to the bucket.
-		cloudtestutils.CheckNoPermission(t, fmt.Sprintf("gs://%s/%s-%d?%s=%s", limitedBucket, "backup-test-assume-role", testID,
-			cloud.AuthParam, cloud.AuthParamImplicit), user,
-			nil, /* db */
-			testSettings,
-		)
-
 		info := cloudtestutils.StoreInfo{
 			URI: fmt.Sprintf(
-				"gs://%s/%s-%d?%s=%s&%s=%s", limitedBucket, "backup-test-assume-role", testID,
-				cloud.AuthParam, cloud.AuthParamImplicit, AssumeRoleParam, assumedAccount,
+				"gs://%s/%s-%d?%s=%s", limitedBucket, "backup-test-assume-role", testID,
+				cloud.AuthParam, cloud.AuthParamImplicit,
 			),
 			User: user,
 		}
+		cloudtestutils.CheckNoPermission(t, info)
+		info.URI = fmt.Sprintf(
+			"gs://%s/%s-%d?%s=%s&%s=%s", limitedBucket, "backup-test-assume-role", testID,
+			cloud.AuthParam, cloud.AuthParamImplicit, AssumeRoleParam, assumedAccount,
+		)
 		cloudtestutils.CheckExportStore(t, info)
 		info.URI = fmt.Sprintf("gs://%s/%s-%d/%s?%s=%s&%s=%s",
 			limitedBucket,
@@ -271,17 +265,17 @@ func TestGCSAssumeRole(t *testing.T) {
 				// to access the storage.
 				for _, role := range roleChain {
 					q.Set(AssumeRoleParam, role)
-					roleURI := fmt.Sprintf("gs://%s/%s-%d/%s?%s",
-						limitedBucket,
-						"backup-test-assume-role",
-						testID,
-						"listing-test",
-						q.Encode(),
-					)
-					cloudtestutils.CheckNoPermission(t, roleURI, user,
-						nil, /* db */
-						testSettings,
-					)
+					info := cloudtestutils.StoreInfo{
+						URI: fmt.Sprintf("gs://%s/%s-%d/%s?%s",
+							limitedBucket,
+							"backup-test-assume-role",
+							testID,
+							"listing-test",
+							q.Encode(),
+						),
+						User: user,
+					}
+					cloudtestutils.CheckNoPermission(t, info)
 				}
 
 				// Finally, check that the chain of roles can be used to access the storage.