search

crossplane-contrib / provider-upjet-aws

AWS Provider for Crossplane.
https://marketplace.upbound.io/providers/upbound/provider-family-aws/
Apache License 2.0
148 stars 125 forks source link

[revive] Use the configured AWS partition when constructing ARNs #1554

Open erhancagirici opened 3 weeks ago

erhancagirici commented 3 weeks ago

Description of your changes

Revives the the original PR from @mbbush #1223

I have:

How has this code been tested

mbbush commented 3 weeks ago

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 commented 3 weeks ago

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

erhancagirici commented 3 weeks ago

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

erhancagirici commented 3 weeks ago

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

erhancagirici commented 3 weeks ago

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

erhancagirici commented 3 weeks ago

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

ulucinar commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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