[revive] Use the configured AWS partition when constructing ARNs #1554
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ import ( | |
| const ( | ||
| keyAccountID = "account_id" | ||
| keyRegion = "region" | ||
| keyPartition = "partition" | ||
| localstackAccountID = "000000000" | ||
| ) | ||
|
|
||
|
|
@@ -38,6 +39,16 @@ type SetupConfig struct { | |
| Logger logging.Logger | ||
| } | ||
|
|
||
| // iamRegions holds the region used for signing IAM credentials for each AWS partition. | ||
| var iamRegions = map[string]string{ | ||
|
There was a problem hiding this comment. What's the source of truth for this table? Following is a more robust implementation for the signing region lookup keyed by the partition ID: // map from a partition to its IAM signing region
signingRegionMap := make(map[string]string)
for _, p := range endpoints.DefaultResolver().(endpoints.EnumPartitions).Partitions() {
for _, r := range p.Regions() {
re, err := p.EndpointFor("iam", r.ID(), func(opt *endpoints.Options) {
opt.ResolveUnknownService = true
})
if err != nil {
panic(err)
}
signingRegionMap[p.ID()] = re.SigningRegion
break
}
}It currently produces the following map: Please note the difference for the partition Please also see the discussion below regarding partition lookups using the Also please note that we have |
||
| "aws": "us-east-1", | ||
| "aws-us-gov": "us-gov-west-1", | ||
| "aws-cn": "cn-northeast-1", | ||
| "aws-iso": "us-iso-east-1", | ||
| "aws-iso-b": "us-isob-east-1", | ||
| "aws-iso-e": "eu-isoe-west-1", | ||
| } | ||
|
|
||
| func SelectTerraformSetup(config *SetupConfig) terraform.SetupFn { // nolint:gocyclo | ||
| credsCache := NewAWSCredentialsProviderCache(WithCacheLogger(config.Logger)) | ||
| return func(ctx context.Context, c client.Client, mg resource.Managed) (terraform.Setup, error) { | ||
|
|
@@ -93,7 +104,13 @@ func SelectTerraformSetup(config *SetupConfig) terraform.SetupFn { // nolint:goc | |
| } | ||
| ps.ClientMetadata = map[string]string{ | ||
| keyAccountID: credCache.accountID, | ||
| keyPartition: "aws", | ||
| } | ||
|
|
||
| if pc.Spec.Endpoint != nil && pc.Spec.Endpoint.PartitionID != nil { | ||
| ps.ClientMetadata[keyPartition] = *pc.Spec.Endpoint.PartitionID | ||
| } | ||
|
Comment on lines
+110
to
+112
There was a problem hiding this comment. nit:
So, we could set up a map from regions identifiers to parition identifiers using the // map from a region to its partition
regionMap := make(map[string]string)
for _, p := range endpoints.DefaultResolver().(endpoints.EnumPartitions).Partitions() {
for id, _ := range p.Regions() {
// not necessary, as a sanity check in case we've misunderstood something
if pn := regionMap[id]; pn != "" {
panic(errors.Errorf("region %q is already in partition %q", id, pn))
}
regionMap[id] = p.ID()
}
}Then if the resource has Because In case new regions are added before we bump the We can always defer to a default parition, i.e., Not mandatory but I believe it will yield a better UX while the implementation should be robust. Also if at some point, we need to register a new region with the region map but cannot update the There was a problem hiding this comment. this was also in my mind. I was a bit hesitant to source partitions from
Searched for an equivalent in probably for short term this won't be an issue, but I thought that we can implement something for now to enable the feature, and iterate for cheaper maintenance code. |
||
|
|
||
| // several external name configs depend on the setup.Configuration for templating region | ||
| ps.Configuration = map[string]any{ | ||
| keyRegion: awsCfg.Region, | ||
|
|
@@ -127,11 +144,22 @@ func getAWSConfigWithDefaultRegion(ctx context.Context, c client.Client, obj run | |
| return nil, err | ||
| } | ||
| if cfg.Region == "" && obj.GetObjectKind().GroupVersionKind().Group == "iam.aws.upbound.io" { | ||
| cfg.Region = getIAMRegion(pc) | ||
| } | ||
| return cfg, nil | ||
| } | ||
|
|
||
| func getIAMRegion(pc *v1beta1.ProviderConfig) string { | ||
| defaultRegion := "us-east-1" | ||
| if pc == nil || pc.Spec.Endpoint == nil || pc.Spec.Endpoint.PartitionID == nil { | ||
| return defaultRegion | ||
|
There was a problem hiding this comment. Please see the comment regarding the There was a problem hiding this comment. @ulucinar while I understand the comment about
IIUC, unlike other resources, we anyways need some "partition-like" input at the Alternative can be introducing an optional "partition" input for IAM MR specs, but I don't think this has a big benefit. Because the associated credentials of the provider config are "partition-bound" as described here, i.e. they will be only valid for a single particular partition. The credentials & ProviderConfig actually "dictates" the partition already. Unlike handling of regions, where it is possible to use the same provider config and switch regions at MR, this does not make sense for partitions. There was a problem hiding this comment. What I don't like about this implementation is that we do not inject the Looks like to me it would have been better to have |
||
| } | ||
| if region, ok := iamRegions[*pc.Spec.Endpoint.PartitionID]; ok { | ||
| return region | ||
| } | ||
| return defaultRegion | ||
| } | ||
|
|
||
| type metaOnlyPrimary struct { | ||
| meta any | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the most risky changes in this PR are the conversions of the hardcoded ARNs in the external names to the calls to
fullArnTemplate. But the conversion is rather straightforward and we can trigger uptest on the resources with external-name implementation changes, where applicable.Not probably for this PR, but we could also have a unit test that asserts the expected external-name syntax for the configured resources by invoking the implemented external-name configuration (and probably other aspects of the configuration). cc. @turkenf