Skip to content

Commit

Permalink
Merge #138873 #139044
Browse files Browse the repository at this point in the history 
138873: cloudtestutils: refactor CheckExportStore, CheckListFiles, CheckNoPermissions r=RaduBerinde a=RaduBerinde

This PR is taking a first step towards removing `ExternalIODir` from `cluster.Settings`.

#### cloudtestutils: refactor CheckExportStore

This change adds a `cloudtestutils.StoreInfo` structure and refactors
`CheckExportStore` to use it, making the call sites simpler and more
readable. We now pass the external IO dir explicitly through
`StoreInfo` instead of getting it from the settings.

Epic: none
Release note: None

#### cloudtestutils: refactor CheckListFiles

Refactor `CheckListFiles` and `CheckListFilesCanonical` to use
`StoreInfo`.

Epic: none
Release note: None

#### cloudtestutils: refactor CheckNoPermissions

Use `StoreInfo` for `CheckNoPermissions.`

Epic: none
Release note: None


139044: build: update `rules_go` r=jlinder a=rickystewart

To pull in cockroachdb/rules_go#24

Epic: CRDB-41952
Release note: None

Co-authored-by: Radu Berinde <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
  • Loading branch information
@RaduBerinde @rickystewart
3 people committed Jan 14, 2025
3 parents 202b559 + f76f279 + 3aa2724 commit 4f224db
Show file tree
Hide file tree
Showing 10 changed files with 324 additions and 360 deletions.
8 changes: 4 additions & 4 deletions WORKSPACE
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
# Load go bazel tools. This gives us access to the go bazel SDK/toolchains.
http_archive(
name = "io_bazel_rules_go",
sha256 = "094b2d9b480eb508bd352914a085c5901d0f3c4520833a12b9197394a80619c7",
strip_prefix = "cockroachdb-rules_go-548c13b",
sha256 = "fc43f42615292c9c3ca6dc211b19ce45986562b0efbca3635e83a70eb011e293",
strip_prefix = "cockroachdb-rules_go-59db5f5",
urls = [
# cockroachdb/rules_go as of 548c13bf30e840418237a12cb5fbd0be391278bd
# cockroachdb/rules_go as of 59db5f5a4a65d6671dd62dd4aea42757b09a1fa3
# (upstream release-0.50 plus a few patches).
"https://storage.googleapis.com/public-bazel-artifacts/bazel/cockroachdb-rules_go-v0.27.0-529-g548c13b.tar.gz",
"https://storage.googleapis.com/public-bazel-artifacts/bazel/cockroachdb-rules_go-v0.27.0-530-g59db5f5.tar.gz",
],
)

Expand Down
2 changes: 1 addition & 1 deletion build/bazelutil/distdir_files.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1212,7 +1212,7 @@ DISTDIR_FILES = {
"https://storage.googleapis.com/public-bazel-artifacts/bazel/bmatcuk-doublestar-v4.0.1-0-gf7a8118.tar.gz": "d11c3b3a45574f89d6a6b2f50e53feea50df60407b35f36193bf5815d32c79d1",
"https://storage.googleapis.com/public-bazel-artifacts/bazel/cockroachdb-protobuf-3f5d91f.tar.gz": "6d4e7fe1cbd958dee69ce9becbf8892d567f082b6782d3973a118d0aa00807a8",
"https://storage.googleapis.com/public-bazel-artifacts/bazel/cockroachdb-rules_foreign_cc-8d34d77.tar.gz": "03afebfc3f173666a3820a29512265c710c3a08d0082ba77469779d3e3af5a11",
"https://storage.googleapis.com/public-bazel-artifacts/bazel/cockroachdb-rules_go-v0.27.0-529-g548c13b.tar.gz": "094b2d9b480eb508bd352914a085c5901d0f3c4520833a12b9197394a80619c7",
"https://storage.googleapis.com/public-bazel-artifacts/bazel/cockroachdb-rules_go-v0.27.0-530-g59db5f5.tar.gz": "fc43f42615292c9c3ca6dc211b19ce45986562b0efbca3635e83a70eb011e293",
"https://storage.googleapis.com/public-bazel-artifacts/bazel/google-starlark-go-e043a3d.tar.gz": "a35c6468e0e0921833a63290161ff903295eaaf5915200bbce272cbc8dfd1c1c",
"https://storage.googleapis.com/public-bazel-artifacts/bazel/googleapis-83c3605afb5a39952bf0a0809875d41cf2a558ca.zip": "ba694861340e792fd31cb77274eacaf6e4ca8bda97707898f41d8bebfd8a4984",
"https://storage.googleapis.com/public-bazel-artifacts/bazel/platforms-0.0.10.tar.gz": "218efe8ee736d26a3572663b374a253c012b716d8af0c07e842e82f238a0a7ee",
Expand Down
181 changes: 85 additions & 96 deletions pkg/cloud/amazon/s3_storage_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,14 +106,10 @@ func TestPutS3(t *testing.T) {
}
t.Run(testName, func(t *testing.T) {
t.Run("auth-empty-no-cred", func(t *testing.T) {
_, err := cloud.ExternalStorageFromURI(ctx, fmt.Sprintf("s3://%s/%s-%d", bucket,
"backup-test-default", testID), base.ExternalIODirConfig{}, testSettings,
blobs.TestEmptyBlobClientFactory, user,
nil, /* ie */
nil, /* ief */
nil, /* kvDB */
nil, /* limiters */
nil, /* metrics */
uri := fmt.Sprintf("s3://%s/%s-%d", bucket, "backup-test-default", testID)
_, err := cloud.ExternalStorageFromURI(
ctx, uri, base.ExternalIODirConfig{}, testSettings, blobs.TestEmptyBlobClientFactory, user,
nil /* ie */, nil /* ief */, nil /* kvDB */, nil /* limiters */, nil, /* metrics */
)
require.EqualError(t, err, fmt.Sprintf(
`%s is set to '%s', but %s is not set`,
Expand All @@ -124,55 +120,52 @@ func TestPutS3(t *testing.T) {
})
t.Run("auth-implicit", func(t *testing.T) {
skipIfNoDefaultConfig(t, ctx)
cloudtestutils.CheckExportStore(t, fmt.Sprintf(
"s3://%s/%s-%d?%s=%s",
bucket, "backup-test-default", testID,
cloud.AuthParam, cloud.AuthParamImplicit,
), false, user,
nil, /* db */
testSettings)
info := cloudtestutils.StoreInfo{
URI: fmt.Sprintf(
"s3://%s/%s-%d?%s=%s",
bucket, "backup-test-default", testID,
cloud.AuthParam, cloud.AuthParamImplicit,
),
User: user,
}
cloudtestutils.CheckExportStore(t, info)
})
t.Run("auth-specified", func(t *testing.T) {
uri := S3URI(bucket, fmt.Sprintf("backup-test-%d", testID),
&cloudpb.ExternalStorage_S3{AccessKey: envCreds.AccessKeyID, Secret: envCreds.SecretAccessKey, Region: "us-east-1"},
)
cloudtestutils.CheckExportStore(
t, uri, false, user, nil /* db */, testSettings,
)
cloudtestutils.CheckListFiles(
t, uri, user, nil /* db */, testSettings,
)
info := cloudtestutils.StoreInfo{
URI: S3URI(
bucket, fmt.Sprintf("backup-test-%d", testID),
&cloudpb.ExternalStorage_S3{AccessKey: envCreds.AccessKeyID, Secret: envCreds.SecretAccessKey, Region: "us-east-1"},
),
User: user,
}
cloudtestutils.CheckExportStore(t, info)
cloudtestutils.CheckListFiles(t, info)
})

// Tests that we can put an object with server side encryption specified.
t.Run("server-side-encryption", func(t *testing.T) {
skipIfNoDefaultConfig(t, ctx)
cloudtestutils.CheckExportStore(t, fmt.Sprintf(
"s3://%s/%s-%d?%s=%s&%s=%s",
bucket, "backup-test-sse-256", testID,
cloud.AuthParam, cloud.AuthParamImplicit, AWSServerSideEncryptionMode,
"AES256",
),
false,
user,
nil, /* db */
testSettings,
)

info := cloudtestutils.StoreInfo{
URI: fmt.Sprintf(
"s3://%s/%s-%d?%s=%s&%s=%s",
bucket, "backup-test-sse-256", testID,
cloud.AuthParam, cloud.AuthParamImplicit, AWSServerSideEncryptionMode,
"AES256",
),
User: user,
}
cloudtestutils.CheckExportStore(t, info)
v := os.Getenv("AWS_KMS_KEY_ARN")
if v == "" {
skip.IgnoreLint(t, "AWS_KMS_KEY_ARN env var must be set")
}
cloudtestutils.CheckExportStore(t, fmt.Sprintf(
info.URI = fmt.Sprintf(
"s3://%s/%s-%d?%s=%s&%s=%s&%s=%s",
bucket, "backup-test-sse-kms", testID,
cloud.AuthParam, cloud.AuthParamImplicit, AWSServerSideEncryptionMode,
"aws:kms", AWSServerSideEncryptionKMSID, v,
),
false,
user,
nil, /* db */
testSettings)
)
cloudtestutils.CheckExportStore(t, info)
})

t.Run("server-side-encryption-invalid-params", func(t *testing.T) {
Expand All @@ -197,7 +190,6 @@ func TestPutS3(t *testing.T) {
require.True(t, testutils.IsError(err, "AWS_SERVER_KMS_ID param must be set when using aws:kms server side encryption mode."))
})
})

}
}

Expand All @@ -218,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)

Expand All @@ -231,27 +222,25 @@ func TestPutS3AssumeRole(t *testing.T) {
ctx := context.Background()
t.Run("auth-implicit", func(t *testing.T) {
skipIfNoDefaultConfig(t, ctx)
uri := S3URI(bucket, testPath,
&cloudpb.ExternalStorage_S3{Auth: cloud.AuthParamImplicit, RoleARN: roleArn, Region: "us-east-1"},
)
cloudtestutils.CheckExportStore(
t, uri, false, user, nil /* db */, testSettings,
)
cloudtestutils.CheckListFiles(
t, uri, user, nil /* db */, testSettings,
)
info := cloudtestutils.StoreInfo{
URI: S3URI(bucket, testPath,
&cloudpb.ExternalStorage_S3{Auth: cloud.AuthParamImplicit, RoleARN: roleArn, Region: "us-east-1"},
),
User: user,
}
cloudtestutils.CheckExportStore(t, info)
cloudtestutils.CheckListFiles(t, info)
})

t.Run("auth-specified", func(t *testing.T) {
uri := S3URI(bucket, testPath,
&cloudpb.ExternalStorage_S3{Auth: cloud.AuthParamSpecified, RoleARN: roleArn, AccessKey: creds.AccessKeyID, Secret: creds.SecretAccessKey, Region: "us-east-1"},
)
cloudtestutils.CheckExportStore(
t, uri, false, user, nil /* db */, testSettings,
)
cloudtestutils.CheckListFiles(
t, uri, user, nil /* db */, testSettings,
)
info := cloudtestutils.StoreInfo{
URI: S3URI(bucket, testPath,
&cloudpb.ExternalStorage_S3{Auth: cloud.AuthParamSpecified, RoleARN: roleArn, AccessKey: creds.AccessKeyID, Secret: creds.SecretAccessKey, Region: "us-east-1"},
),
User: user,
}
cloudtestutils.CheckExportStore(t, info)
cloudtestutils.CheckListFiles(t, info)
})

t.Run("role-chaining-external-id", func(t *testing.T) {
Expand All @@ -273,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
Expand All @@ -295,22 +285,23 @@ func TestPutS3AssumeRole(t *testing.T) {
delegatesWithoutID = append(delegatesWithoutID, cloudpb.ExternalStorage_AssumeRoleProvider{Role: p.Role})
}

uri := S3URI(bucket, testPath,
&cloudpb.ExternalStorage_S3{
Auth: tc.auth,
AssumeRoleProvider: roleWithoutID,
DelegateRoleProviders: delegatesWithoutID,
AccessKey: tc.accessKey,
Secret: tc.secretKey,
Region: "us-east-1",
},
)
cloudtestutils.CheckNoPermission(
t, uri, user, nil /* db */, testSettings,
)
info := cloudtestutils.StoreInfo{
URI: S3URI(bucket, testPath,
&cloudpb.ExternalStorage_S3{
Auth: tc.auth,
AssumeRoleProvider: roleWithoutID,
DelegateRoleProviders: delegatesWithoutID,
AccessKey: tc.accessKey,
Secret: tc.secretKey,
Region: "us-east-1",
},
),
User: user,
}
cloudtestutils.CheckNoPermission(t, info)

// Finally, check that the chain of roles can be used to access the storage.
uri = S3URI(bucket, testPath,
info.URI = S3URI(bucket, testPath,
&cloudpb.ExternalStorage_S3{
Auth: tc.auth,
AssumeRoleProvider: providerChain[len(providerChain)-1],
Expand All @@ -321,9 +312,7 @@ func TestPutS3AssumeRole(t *testing.T) {
},
)

cloudtestutils.CheckExportStore(
t, uri, false, user, nil /* db */, testSettings,
)
cloudtestutils.CheckExportStore(t, info)
})
}
})
Expand Down Expand Up @@ -363,11 +352,11 @@ func TestPutS3Endpoint(t *testing.T) {
RawQuery: q.Encode(),
}

testSettings := cluster.MakeTestingClusterSettings()

cloudtestutils.CheckExportStore(
t, u.String(), false, user, nil /* db */, testSettings,
)
info := cloudtestutils.StoreInfo{
URI: u.String(),
User: user,
}
cloudtestutils.CheckExportStore(t, info)
})
t.Run("use-path-style", func(t *testing.T) {
// EngFlow machines have no internet access, and queries even to localhost will time out.
Expand Down
67 changes: 23 additions & 44 deletions pkg/cloud/azure/azure_storage_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,45 +104,29 @@ func TestAzure(t *testing.T) {
skip.IgnoreLint(t, "Test not configured for Azure")
return
}
testSettings := cluster.MakeTestingClusterSettings()
testID := cloudtestutils.NewTestID()
testPath := fmt.Sprintf("backup-test-%d", testID)
testListPath := fmt.Sprintf("listing-test-%d", testID)

cloudtestutils.CheckExportStore(t, cfg.filePath(testPath),
false, username.RootUserName(),
nil, /* db */
testSettings,
)
cloudtestutils.CheckListFiles(t, cfg.filePath(testListPath),
username.RootUserName(),
nil, /* db */
testSettings,
)
info := cloudtestutils.StoreInfo{
URI: cfg.filePath(testPath),
User: username.RootUserName(),
}
cloudtestutils.CheckExportStore(t, info)
info.URI = cfg.filePath(testListPath)
cloudtestutils.CheckListFiles(t, info)

// Client Secret auth
cloudtestutils.CheckExportStore(t, cfg.filePathClientAuth(testPath),
false, username.RootUserName(),
nil, /* db */
testSettings,
)
cloudtestutils.CheckListFiles(t, cfg.filePathClientAuth(testListPath),
username.RootUserName(),
nil, /* db */
testSettings,
)
info.URI = cfg.filePathClientAuth(testPath)
cloudtestutils.CheckExportStore(t, info)
info.URI = cfg.filePathClientAuth(testListPath)
cloudtestutils.CheckListFiles(t, info)

// Implicit auth
cloudtestutils.CheckExportStore(t, cfg.filePathImplicitAuth(testPath),
false, username.RootUserName(),
nil, /* db */
testSettings,
)
cloudtestutils.CheckListFiles(t, cfg.filePathImplicitAuth(testListPath),
username.RootUserName(),
nil, /* db */
testSettings,
)
info.URI = cfg.filePathImplicitAuth(testPath)
cloudtestutils.CheckExportStore(t, info)
info.URI = cfg.filePathImplicitAuth(testListPath)
cloudtestutils.CheckListFiles(t, info)
}

func TestAzureSchemes(t *testing.T) {
Expand Down Expand Up @@ -279,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", "")
Expand All @@ -288,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()
Expand All @@ -300,14 +286,7 @@ func TestAzureStorageFileImplicitAuth(t *testing.T) {
cleanup3 := envutil.TestSetEnv(t, "COCKROACH_AZURE_APPLICATION_CREDENTIALS_FILE", credFile)
defer cleanup3()

cloudtestutils.CheckExportStore(t, cfg.filePathImplicitAuth(testPath),
false, username.RootUserName(),
nil, /* db */
testSettings,
)
cloudtestutils.CheckListFiles(t, cfg.filePathImplicitAuth(testListPath),
username.RootUserName(),
nil, /* db */
testSettings,
)
cloudtestutils.CheckExportStore(t, info)
info.URI = cfg.filePathImplicitAuth(testListPath)
cloudtestutils.CheckListFiles(t, info)
}
Loading

0 comments on commit 4f224db

Please sign in to comment.