[revive] Use the configured AWS partition when constructing ARNs

[erhancagirici]

Description of your changes

Revives the the original PR from @mbbush #1223

• rebased onto latest main.
• added aws-iso-e partition

I have:

• Read and followed Crossplane's contribution process.
• Run make reviewable to ensure this PR is ready for review.
• Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

[mbbush]

LGTM. Thanks for picking this up!

[mbbush]

There may be additional resource kinds that need partition-awareness, that I didn't include in my original PR, and I haven't looked at any test results yet, but I trust the upbound folks to handle that

[ulucinar]

Hi @erhancagirici,
Left some comments to provide a better UX while keeping the implementation more robust and easy to maintain, and some other nits.

[ulucinar]

nit:
Looks like we have a robust & low maintenance cost way of mapping a region to a partition:

• Here, it's stated that every region is in exactly one partition.
• aws-sdk-go maintains the set of available regions for each partition.

So, we could set up a map from regions identifiers to parition identifiers using the github.com/aws/aws-sdk-go/aws/endpoints as follows:

	// 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 spec.forProvider.region specified, we can deduce the partition to be used from the regionMap. Let's initialize the map only once in init.

Because spec.endpoint.partitionId is already part of our ProviderConfig API, let's also keep it so that if a user wants to override the partition we compute, we still honor it (so we had better lookup the partition first and override it if ProviderConfig specifies one).

In case new regions are added before we bump the aws-go-sdk version (which depends on upstream Terraform), ProviderConfig overrides should be a workaround (so if the map does not contain an entry for the specified region, let's not immediately error but instead, check the ProviderConfig's spec.endpoint.partitionId override.

We can always defer to a default parition, i.e., aws if no partition resolution mechanism yields one.

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 aws-go-sdk to a version that already contains the region, we can extend the map with a manually added key until the SDK update.

[erhancagirici]

this was also in my mind. I was a bit hesitant to source partitions from github.com/aws/aws-sdk-go as it has a deprecation notice here, stating

Per that announcement, as of 7/31/2024, the SDK has entered maintenance mode. Going forward, we will limit releases to address critical bug fixes and security issues only. The SDK will not receive API updates for new or existing services, or be updated to support new regions.
Maintenance mode will last for 1 year. After 7/31/2025, the SDK will enter end-of-support, and no updates at all will be made. We recommend that you migrate to AWS SDK for Go v2. For additional details as well as information on how to migrate, please refer to the linked announcement.

Searched for an equivalent in aws-sdk-go-v2 but relevant parts were moved to internal packages, (if I did not miss anything) I could not find anything exported for this.

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.

[ulucinar]

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:

map[aws:us-east-1 aws-cn:cn-north-1 aws-iso:us-iso-east-1 aws-iso-b:us-isob-east-1 aws-iso-e:eu-isoe-west-1 aws-us-gov:us-gov-west-1]

Please note the difference for the partition aws-cn, don't know if this really matters.

Please also see the discussion below regarding partition lookups using the region as a key, which are also applicable here.

Also please note that we have spec.endpoint.signingRegion in the ProviderConfig API, which we should factor into this implementation. But I suggest we do it with a follow-up PR.

[ulucinar]

Please see the comment regarding the iamRegions map above. We should not be using us-east-1 for the partitions other than aws and this implementation leaves the burden of configuring the right partition for an IAM resource to the user by specifying a proper ProviderConfig. While we should allow for overrides via the ProviderConfig (as it's already part of the ProviderConfig API, and for cases where the SDK has not updated its known partitions), we can provide a better UX with a robust & low maintenance cost implementation that relies on the aws-go-sdk.

[erhancagirici]

@ulucinar while I understand the comment about iamRegions map, I don't think it is directly relevant for the particular case of IAM resources. Note that this code part is only called for IAM MRs.

this implementation leaves the burden of configuring the right partition for an IAM resource to the user by specifying a proper ProviderConfig

IIUC, unlike other resources, we anyways need some "partition-like" input at the ProviderConfig level as IAM resources are "regionless", so there is nothing else from which we can "infer" the partition, by only looking at the MR. Configuring a partition is actually a requirement rather than a config burden.

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.

[ulucinar]

What I don't like about this implementation is that we do not inject the region parameter for IAM resources and we rely on spec.endpoint.partitionID to choose the correct region. For resources with an explicit region parameter, we can in fact infer the correct region and the signing region from that parameter. The current implementation provides a more uniform UX in this regard. You always need to specify the required region parameter (for non-IAM resources) and if the specified region is not in the aws partition, then specify the correct partition in spec.endpoint.partitionID, which can be improved. But then we have slightly different semantics for IAM resources.

Looks like to me it would have been better to have region as spec.region in the ProviderConfig API.

[ulucinar]

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

[ulucinar]

Hmm, it's not straightforward to port this code to aws-sdk-go-v2, as the endpoints package has been removed...

[erhancagirici]

/test-examples="examples/ec2/v1beta1/vpc.yaml"

[erhancagirici]

/test-examples="examples/ec2/v1beta1/vpc.yaml"

[erhancagirici]

/test-examples="examples/sns/v1beta1/topic.yaml"

[erhancagirici]

/test-examples="examples/iam/v1beta1/policy.yaml"

[ulucinar]

Looks like the source of truth for the region -> partition mapping can be this file:
https://github.com/aws/aws-sdk-go-v2/blob/061540b5a73ead172928c9c5aebc48c011bf4ee1/codegen/smithy-aws-go-codegen/src/main/resources/software/amazon/smithy/aws/go/codegen/endpoints.json

One option is to start with the endpoints package (which also provides static region/partition information, i.e., the region information is code generated from the model file) and later we can have a simple conversion in this repo from the model file referenced.

I'm okay with the approach here. We can follow up with an implementation using endpoints package and/or consuming the endpoints model file with follow up PRs. The behavior will be backwards compatible, with this PR, setting the partition ID for non-aws partitions will be mandatory and later we can lift this constraint with a backwards compatible improvement which will also be more robust and with less maintenance burden for us (relying on the upstream endpoints model file). In other words, it's okay to accumulate some technical depth here if we are short on time.

[ulucinar]

Btw, I saw there's also an ec2.DescribeRegions operation. I gave it a try with the AllRegions option but it does not know about other partitions.

[ulucinar]

As a pointer to the codegen approach, here's a reference:

> curl -sL "https://raw.githubusercontent.com/aws/aws-sdk-go-v2/061540b5a73ead172928c9c5aebc48c011bf4ee1/codegen/smithy-aws-go-codegen/src/main/resources/software/amazon/smithy/aws/go/codegen/endpoints.json" | jq '.partitions[].partition'

"aws"
"aws-cn"
"aws-us-gov"
"aws-iso"
"aws-iso-b"
"aws-iso-e"
"aws-iso-f"

[erhancagirici]

Looks like the source of truth for the region -> partition mapping can be this file: https://github.com/aws/aws-sdk-go-v2/blob/061540b5a73ead172928c9c5aebc48c011bf4ee1/codegen/smithy-aws-go-codegen/src/main/resources/software/amazon/smithy/aws/go/codegen/endpoints.json

Yes, discovered terraform's aws-sdk-base utilizes some code generation based on that JSON.
generator
template
generated

aws-sdk-go-base also exposes various helper funcs such as PartitionForRegion https://github.com/hashicorp/aws-sdk-go-base/blob/82665eea5027433455fc960dac2a7e1140e212b4/endpoints/partition.go#L64.

we can follow some generation approach too or directly consume aws-sdk-go-base