From: Tom Clegg Date: Tue, 4 May 2021 21:24:03 +0000 (-0400) Subject: 17590: Rename S3 volume config keys to match AWS terms. X-Git-Tag: 2.2.0~37^2~5 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/b44a05493cba8cc40c81fc487cbea5ba33662d3c 17590: Rename S3 volume config keys to match AWS terms. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- diff --git a/doc/install/configure-s3-object-storage.html.textile.liquid b/doc/install/configure-s3-object-storage.html.textile.liquid index 76a2f3ab57..6a3c396d60 100644 --- a/doc/install/configure-s3-object-storage.html.textile.liquid +++ b/doc/install/configure-s3-object-storage.html.textile.liquid @@ -43,8 +43,8 @@ Volumes are configured in the @Volumes@ section of the cluster configuration fil # If you are not using an IAM role for authentication, # specify access credentials here instead. - AccessKey: "" - SecretKey: "" + AccessKeyID: "" + SecretAccessKey: "" # Storage provider region. For Google Cloud Storage, use "" # or omit. diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml index ca627d07e8..42388abfdd 100644 --- a/lib/config/config.default.yml +++ b/lib/config/config.default.yml @@ -1194,8 +1194,8 @@ Clusters: # for s3 driver -- see # https://doc.arvados.org/install/configure-s3-object-storage.html IAMRole: aaaaa - AccessKey: aaaaa - SecretKey: aaaaa + AccessKeyID: aaaaa + SecretAccessKey: aaaaa Endpoint: "" Region: us-east-1a Bucket: aaaaa diff --git a/lib/config/deprecated.go b/lib/config/deprecated.go index 0552b66adb..acaf0c5da7 100644 --- a/lib/config/deprecated.go +++ b/lib/config/deprecated.go @@ -5,6 +5,7 @@ package config import ( + "encoding/json" "fmt" "io/ioutil" "net/url" @@ -119,6 +120,49 @@ func (ldr *Loader) applyDeprecatedConfig(cfg *arvados.Config) error { return nil } +func (ldr *Loader) applyDeprecatedVolumeDriverParameters(cfg *arvados.Config) error { + for clusterID, cluster := range cfg.Clusters { + for volID, vol := range cluster.Volumes { + if vol.Driver == "s3" { + var params struct { + AccessKey string `json:",omitempty"` + SecretKey string `json:",omitempty"` + AccessKeyID string + SecretAccessKey string + } + err := json.Unmarshal(vol.DriverParameters, ¶ms) + if err != nil { + return fmt.Errorf("error loading %s.Volumes.%s.DriverParameters: %w", clusterID, volID, err) + } + if params.AccessKey != "" || params.SecretKey != "" { + if params.AccessKeyID != "" || params.SecretAccessKey != "" { + ldr.Logger.Warnf("ignoring old config keys %s.Volumes.%s.DriverParameters.AccessKey/SecretKey because new keys AccessKeyID/SecretAccessKey are also present", clusterID, volID) + continue + } + var allparams map[string]interface{} + err = json.Unmarshal(vol.DriverParameters, &allparams) + if err != nil { + return fmt.Errorf("error loading %s.Volumes.%s.DriverParameters: %w", clusterID, volID, err) + } + for k := range allparams { + if lk := strings.ToLower(k); lk == "accesskey" || lk == "secretkey" { + delete(allparams, k) + } + } + allparams["AccessKeyID"] = params.AccessKey + allparams["SecretAccessKey"] = params.SecretKey + vol.DriverParameters, err = json.Marshal(allparams) + if err != nil { + return err + } + cluster.Volumes[volID] = vol + } + } + } + } + return nil +} + func applyDeprecatedNodeProfile(hostname string, ssi systemServiceInstance, svc *arvados.Service) { scheme := "https" if !ssi.TLS { diff --git a/lib/config/deprecated_keepstore.go b/lib/config/deprecated_keepstore.go index 186ffc3371..d9f4815fcf 100644 --- a/lib/config/deprecated_keepstore.go +++ b/lib/config/deprecated_keepstore.go @@ -324,8 +324,8 @@ func (ldr *Loader) translateOldKeepstoreVolume(oldvol oldKeepstoreVolume) (arvad StorageClasses: array2boolmap(oldvol.StorageClasses), } params = arvados.S3VolumeDriverParameters{ - AccessKey: string(bytes.TrimSpace(accesskeydata)), - SecretKey: string(bytes.TrimSpace(secretkeydata)), + AccessKeyID: string(bytes.TrimSpace(accesskeydata)), + SecretAccessKey: string(bytes.TrimSpace(secretkeydata)), Endpoint: oldvol.Endpoint, Region: oldvol.Region, Bucket: oldvol.Bucket, diff --git a/lib/config/deprecated_keepstore_test.go b/lib/config/deprecated_keepstore_test.go index dab308c9d1..ff1d8be7bd 100644 --- a/lib/config/deprecated_keepstore_test.go +++ b/lib/config/deprecated_keepstore_test.go @@ -225,8 +225,8 @@ Volumes: Driver: "S3", Replication: 4, }, &arvados.S3VolumeDriverParameters{ - AccessKey: "accesskeydata", - SecretKey: "secretkeydata", + AccessKeyID: "accesskeydata", + SecretAccessKey: "secretkeydata", Endpoint: "https://storage.googleapis.com", Region: "us-east-1z", Bucket: "testbucket", diff --git a/lib/config/deprecated_test.go b/lib/config/deprecated_test.go index 0dd03583d7..3fba765888 100644 --- a/lib/config/deprecated_test.go +++ b/lib/config/deprecated_test.go @@ -47,6 +47,34 @@ func testLoadLegacyConfig(content []byte, mungeFlag string, c *check.C) (*arvado return cluster, nil } +func (s *LoadSuite) TestLegacyVolumeDriverParameters(c *check.C) { + logs := checkEquivalent(c, ` +Clusters: + z1111: + Volumes: + z1111-nyw5e-aaaaaaaaaaaaaaa: + Driver: s3 + DriverParameters: + AccessKey: exampleaccesskey + SecretKey: examplesecretkey + Region: foobar + ReadTimeout: 1200s +`, ` +Clusters: + z1111: + Volumes: + z1111-nyw5e-aaaaaaaaaaaaaaa: + Driver: s3 + DriverParameters: + AccessKeyID: exampleaccesskey + SecretAccessKey: examplesecretkey + Region: foobar + ReadTimeout: 1200s +`) + c.Check(logs, check.Matches, `(?ms).*deprecated or unknown config entry: .*AccessKey.*`) + c.Check(logs, check.Matches, `(?ms).*deprecated or unknown config entry: .*SecretKey.*`) +} + func (s *LoadSuite) TestDeprecatedNodeProfilesToServices(c *check.C) { hostname, err := os.Hostname() c.Assert(err, check.IsNil) diff --git a/lib/config/generated_config.go b/lib/config/generated_config.go index 1032663973..d413a8006b 100644 --- a/lib/config/generated_config.go +++ b/lib/config/generated_config.go @@ -1200,8 +1200,8 @@ Clusters: # for s3 driver -- see # https://doc.arvados.org/install/configure-s3-object-storage.html IAMRole: aaaaa - AccessKey: aaaaa - SecretKey: aaaaa + AccessKeyID: aaaaa + SecretAccessKey: aaaaa Endpoint: "" Region: us-east-1a Bucket: aaaaa diff --git a/lib/config/load.go b/lib/config/load.go index 292e3f6d6f..cc26cdaecc 100644 --- a/lib/config/load.go +++ b/lib/config/load.go @@ -241,30 +241,33 @@ func (ldr *Loader) Load() (*arvados.Config, error) { return nil, fmt.Errorf("transcoding config data: %s", err) } + var loadFuncs []func(*arvados.Config) error if !ldr.SkipDeprecated { - err = ldr.applyDeprecatedConfig(&cfg) - if err != nil { - return nil, err - } + loadFuncs = append(loadFuncs, + ldr.applyDeprecatedConfig, + ldr.applyDeprecatedVolumeDriverParameters, + ) } if !ldr.SkipLegacy { // legacy file is required when either: // * a non-default location was specified // * no primary config was loaded, and this is the // legacy config file for the current component - for _, err := range []error{ - ldr.loadOldEnvironmentVariables(&cfg), - ldr.loadOldKeepstoreConfig(&cfg), - ldr.loadOldKeepWebConfig(&cfg), - ldr.loadOldCrunchDispatchSlurmConfig(&cfg), - ldr.loadOldWebsocketConfig(&cfg), - ldr.loadOldKeepproxyConfig(&cfg), - ldr.loadOldGitHttpdConfig(&cfg), - ldr.loadOldKeepBalanceConfig(&cfg), - } { - if err != nil { - return nil, err - } + loadFuncs = append(loadFuncs, + ldr.loadOldEnvironmentVariables, + ldr.loadOldKeepstoreConfig, + ldr.loadOldKeepWebConfig, + ldr.loadOldCrunchDispatchSlurmConfig, + ldr.loadOldWebsocketConfig, + ldr.loadOldKeepproxyConfig, + ldr.loadOldGitHttpdConfig, + ldr.loadOldKeepBalanceConfig, + ) + } + for _, f := range loadFuncs { + err = f(&cfg) + if err != nil { + return nil, err } } diff --git a/lib/config/load_test.go b/lib/config/load_test.go index 91bd6a7439..6c11ee7803 100644 --- a/lib/config/load_test.go +++ b/lib/config/load_test.go @@ -437,10 +437,12 @@ Clusters: `) } -func checkEquivalent(c *check.C, goty, expectedy string) { - gotldr := testLoader(c, goty, nil) +func checkEquivalent(c *check.C, goty, expectedy string) string { + var logbuf bytes.Buffer + gotldr := testLoader(c, goty, &logbuf) expectedldr := testLoader(c, expectedy, nil) checkEquivalentLoaders(c, gotldr, expectedldr) + return logbuf.String() } func checkEqualYAML(c *check.C, got, expected interface{}) { diff --git a/sdk/go/arvados/config.go b/sdk/go/arvados/config.go index c0170d1d7f..2c6db42d13 100644 --- a/sdk/go/arvados/config.go +++ b/sdk/go/arvados/config.go @@ -279,8 +279,8 @@ type Volume struct { type S3VolumeDriverParameters struct { IAMRole string - AccessKey string - SecretKey string + AccessKeyID string + SecretAccessKey string Endpoint string Region string Bucket string diff --git a/services/keepstore/command.go b/services/keepstore/command.go index 0927b18704..bf3bf1722c 100644 --- a/services/keepstore/command.go +++ b/services/keepstore/command.go @@ -72,8 +72,8 @@ func convertKeepstoreFlagsToServiceFlags(args []string, lgr logrus.FieldLogger) flags.String("s3-bucket-volume", "", "Volumes.*.DriverParameters.Bucket") flags.String("s3-region", "", "Volumes.*.DriverParameters.Region") flags.String("s3-endpoint", "", "Volumes.*.DriverParameters.Endpoint") - flags.String("s3-access-key-file", "", "Volumes.*.DriverParameters.AccessKey") - flags.String("s3-secret-key-file", "", "Volumes.*.DriverParameters.SecretKey") + flags.String("s3-access-key-file", "", "Volumes.*.DriverParameters.AccessKeyID") + flags.String("s3-secret-key-file", "", "Volumes.*.DriverParameters.SecretAccessKey") flags.String("s3-race-window", "", "Volumes.*.DriverParameters.RaceWindow") flags.String("s3-replication", "", "Volumes.*.Replication") flags.String("s3-unsafe-delete", "", "Volumes.*.DriverParameters.UnsafeDelete") diff --git a/services/keepstore/s3_volume.go b/services/keepstore/s3_volume.go index 07bb033c9f..fdc2ed56ad 100644 --- a/services/keepstore/s3_volume.go +++ b/services/keepstore/s3_volume.go @@ -148,9 +148,9 @@ func (v *S3Volume) GetDeviceID() string { } func (v *S3Volume) bootstrapIAMCredentials() error { - if v.AccessKey != "" || v.SecretKey != "" { + if v.AccessKeyID != "" || v.SecretAccessKey != "" { if v.IAMRole != "" { - return errors.New("invalid DriverParameters: AccessKey and SecretKey must be blank if IAMRole is specified") + return errors.New("invalid DriverParameters: AccessKeyID and SecretAccessKey must be blank if IAMRole is specified") } return nil } @@ -175,7 +175,7 @@ func (v *S3Volume) bootstrapIAMCredentials() error { } func (v *S3Volume) newS3Client() *s3.S3 { - auth := aws.NewAuth(v.AccessKey, v.SecretKey, v.AuthToken, v.AuthExpiration) + auth := aws.NewAuth(v.AccessKeyID, v.SecretAccessKey, v.AuthToken, v.AuthExpiration) client := s3.New(*auth, v.region) if !v.V2Signature { client.Signature = aws.V4Signature @@ -225,7 +225,7 @@ func (v *S3Volume) updateIAMCredentials() (time.Duration, error) { } defer resp.Body.Close() if resp.StatusCode == http.StatusNotFound { - return 0, fmt.Errorf("this instance does not have an IAM role assigned -- either assign a role, or configure AccessKey and SecretKey explicitly in DriverParameters (error getting %s: HTTP status %s)", url, resp.Status) + return 0, fmt.Errorf("this instance does not have an IAM role assigned -- either assign a role, or configure AccessKeyID and SecretAccessKey explicitly in DriverParameters (error getting %s: HTTP status %s)", url, resp.Status) } else if resp.StatusCode != http.StatusOK { return 0, fmt.Errorf("error getting %s: HTTP status %s", url, resp.Status) } @@ -260,7 +260,7 @@ func (v *S3Volume) updateIAMCredentials() (time.Duration, error) { if err != nil { return 0, fmt.Errorf("error decoding credentials from %s: %s", url, err) } - v.AccessKey, v.SecretKey, v.AuthToken, v.AuthExpiration = cred.AccessKeyID, cred.SecretAccessKey, cred.Token, cred.Expiration + v.AccessKeyID, v.SecretAccessKey, v.AuthToken, v.AuthExpiration = cred.AccessKeyID, cred.SecretAccessKey, cred.Token, cred.Expiration v.bucket.SetBucket(&s3.Bucket{ S3: v.newS3Client(), Name: v.Bucket, diff --git a/services/keepstore/s3_volume_test.go b/services/keepstore/s3_volume_test.go index 2736f00b74..cb08e44ac8 100644 --- a/services/keepstore/s3_volume_test.go +++ b/services/keepstore/s3_volume_test.go @@ -111,11 +111,11 @@ func (s *StubbedS3Suite) TestSignatureVersion(c *check.C) { // Default V4 signature vol := S3Volume{ S3VolumeDriverParameters: arvados.S3VolumeDriverParameters{ - AccessKey: "xxx", - SecretKey: "xxx", - Endpoint: stub.URL, - Region: "test-region-1", - Bucket: "test-bucket-name", + AccessKeyID: "xxx", + SecretAccessKey: "xxx", + Endpoint: stub.URL, + Region: "test-region-1", + Bucket: "test-bucket-name", }, cluster: s.cluster, logger: ctxlog.TestLogger(c), @@ -130,12 +130,12 @@ func (s *StubbedS3Suite) TestSignatureVersion(c *check.C) { // Force V2 signature vol = S3Volume{ S3VolumeDriverParameters: arvados.S3VolumeDriverParameters{ - AccessKey: "xxx", - SecretKey: "xxx", - Endpoint: stub.URL, - Region: "test-region-1", - Bucket: "test-bucket-name", - V2Signature: true, + AccessKeyID: "xxx", + SecretAccessKey: "xxx", + Endpoint: stub.URL, + Region: "test-region-1", + Bucket: "test-bucket-name", + V2Signature: true, }, cluster: s.cluster, logger: ctxlog.TestLogger(c), @@ -160,8 +160,8 @@ func (s *StubbedS3Suite) TestIAMRoleCredentials(c *check.C) { defer s.metadata.Close() v := s.newTestableVolume(c, s.cluster, arvados.Volume{Replication: 2}, newVolumeMetricsVecs(prometheus.NewRegistry()), 5*time.Minute) - c.Check(v.AccessKey, check.Equals, "ASIAIOSFODNN7EXAMPLE") - c.Check(v.SecretKey, check.Equals, "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY") + c.Check(v.AccessKeyID, check.Equals, "ASIAIOSFODNN7EXAMPLE") + c.Check(v.SecretAccessKey, check.Equals, "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY") c.Check(v.bucket.bucket.S3.Auth.AccessKey, check.Equals, "ASIAIOSFODNN7EXAMPLE") c.Check(v.bucket.bucket.S3.Auth.SecretKey, check.Equals, "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY") @@ -519,8 +519,8 @@ func (s *StubbedS3Suite) newTestableVolume(c *check.C, cluster *arvados.Cluster, S3Volume: &S3Volume{ S3VolumeDriverParameters: arvados.S3VolumeDriverParameters{ IAMRole: iamRole, - AccessKey: accessKey, - SecretKey: secretKey, + AccessKeyID: accessKey, + SecretAccessKey: secretKey, Bucket: TestBucketName, Endpoint: endpoint, Region: "test-region-1", diff --git a/services/keepstore/s3aws_volume.go b/services/keepstore/s3aws_volume.go index 8d999e7472..baded41163 100644 --- a/services/keepstore/s3aws_volume.go +++ b/services/keepstore/s3aws_volume.go @@ -192,7 +192,7 @@ func (v *S3AWSVolume) check(ec2metadataHostname string) error { creds := aws.NewChainProvider( []aws.CredentialsProvider{ - aws.NewStaticCredentialsProvider(v.AccessKey, v.SecretKey, v.AuthToken), + aws.NewStaticCredentialsProvider(v.AccessKeyID, v.SecretAccessKey, v.AuthToken), ec2rolecreds.New(ec2metadata.New(cfg)), }) diff --git a/services/keepstore/s3aws_volume_test.go b/services/keepstore/s3aws_volume_test.go index d9886c07f3..0387d52f18 100644 --- a/services/keepstore/s3aws_volume_test.go +++ b/services/keepstore/s3aws_volume_test.go @@ -123,11 +123,11 @@ func (s *StubbedS3AWSSuite) TestSignature(c *check.C) { // as of June 24, 2020. Cf. https://forums.aws.amazon.com/ann.jspa?annID=5816 vol := S3AWSVolume{ S3VolumeDriverParameters: arvados.S3VolumeDriverParameters{ - AccessKey: "xxx", - SecretKey: "xxx", - Endpoint: stub.URL, - Region: "test-region-1", - Bucket: "test-bucket-name", + AccessKeyID: "xxx", + SecretAccessKey: "xxx", + Endpoint: stub.URL, + Region: "test-region-1", + Bucket: "test-bucket-name", }, cluster: s.cluster, logger: ctxlog.TestLogger(c), @@ -567,8 +567,8 @@ func (s *StubbedS3AWSSuite) newTestableVolume(c *check.C, cluster *arvados.Clust S3AWSVolume: &S3AWSVolume{ S3VolumeDriverParameters: arvados.S3VolumeDriverParameters{ IAMRole: iamRole, - AccessKey: accessKey, - SecretKey: secretKey, + AccessKeyID: accessKey, + SecretAccessKey: secretKey, Bucket: S3AWSTestBucketName, Endpoint: endpoint, Region: "test-region-1",