[revive] Use the configured AWS partition when constructing ARNs #1554
Conversation
erhancagirici
changed the title
erhancagirici
changed the title
|
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 |
There was a problem hiding this comment.
Hi @erhancagirici,
Left some comments to provide a better UX while keeping the implementation more robust and easy to maintain, and some other nits.
internal/clients/aws.go
Outdated
| if pc.Spec.Endpoint != nil && pc.Spec.Endpoint.PartitionID != nil { | ||
| ps.ClientMetadata[keyPartition] = *pc.Spec.Endpoint.PartitionID | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What Erhan described is pretty much exactly my original thinking when I first wrote this. It can be done with the go sdk v1, but not v2, and given the pending deprecation I didn't want to add a new dependency on v1.
There was a problem hiding this comment.
as suggested, now we try to determine the partition from the supplied region from MR.
- If we have an explicit
spec.endpoint.partitionIdspecified in theProviderConfig, we fall back to that. - if the MR specified region and
spec.endpoint.partitionIdconflicts, we error out. - regionless MRs (iam group) relies on
partitionIdfrom theProviderConfigregardless of the situation, as there is no other way to infer their intended partition.
internal/clients/aws.go
Outdated
| @@ -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:
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.
There was a problem hiding this comment.
If I recall correctly I originally got it from the source code of the AWS Java SDK, but I could be misremembering or remembering something else.
I don't recall noticing the existence of spec.endpoint.signingRegion when I worked on this long ago.
There was a problem hiding this comment.
As suggested, default IAM region map is now being generated from the source of truth in aws-sdk-go-v2 here
| 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 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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Looks like to me it would have been better to have region as spec.region in the ProviderConfig API
I also think that having somespec.defaultRegionin theProviderConfigis the neater solution. Though for now, we keep the API backward compatible behavior-wise and avoid big refactors.
There was a problem hiding this comment.
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
|
Hmm, it's not straightforward to port this code to |
|
/test-examples="examples/ec2/v1beta1/vpc.yaml" |
1 similar comment
|
/test-examples="examples/ec2/v1beta1/vpc.yaml" |
|
/test-examples="examples/sns/v1beta1/topic.yaml" |
|
/test-examples="examples/iam/v1beta1/policy.yaml" |
|
Looks like the source of truth for the region -> partition mapping can be this file: One option is to start with the I'm okay with the approach here. We can follow up with an implementation using |
|
Btw, I saw there's also an ec2.DescribeRegions operation. I gave it a try with the |
|
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" |
Yes, discovered terraform's
we can follow some generation approach too or directly consume |
|
Curious if this pull request still has hope. We currently can't get the provider to work in our non-standard region due to this issue. Is there an issue with the proposed approach? Or is it just a lack of time and contributors? If it's the latter, I may be able to invest some time. Thanks! |
|
@zonybob its mostly around validation. We don't have access to these non-standard regions which makes validation difficult. |
Yes, I'd be willing to help you validate! Depending on which region I can access to test, it could take me a few days to get to. |
|
Following two issues seem to me that they would be fixed by this PR, though I haven't examined them in detail: |
|
I can test the current pull request on GovCloud. What's the most advisable way to get a built image? |
@zonybob, please see: crossplane/crossplane#3817 (comment) |
erhancagirici
force-pushed
the
partition
branch
from
9561358 to
fe08da0
Compare
@zonybob if you want to build your own images be sure to generate the family provider itself along with needed service providers. I have pre-built images out of this PR, for Your provider packages should look something similar to this apiVersion: pkg.crossplane.io/v1
kind: Provider
metadata:
name: provider-family-aws
spec:
package: index.docker.io/erhancag/provider-family-aws:v1.20.0-dev.fe08da0ac-partitions
skipDependencyResolution: trueIAM provider apiVersion: pkg.crossplane.io/v1
kind: Provider
metadata:
name: provider-aws-iam
spec:
package: index.docker.io/erhancag/provider-aws-iam:v1.20.0-dev.fe08da0ac-partitions
skipDependencyResolution: trueyour provider config should look sth like: apiVersion: aws.upbound.io/v1beta1
kind: ProviderConfig
metadata:
name: default
spec:
credentials:
...YOUR CREDENTIAL CONFIGURATION...
endpoint:
partitionId: aws-us-gov
url:
type: Auto |
|
/test-examples="examples/ec2/v1beta1/vpc.yaml" |
|
/test-examples="examples/ec2/v1beta2/route.yaml" |
|
/test-examples="examples/ec2/v1beta1/internetgateway.yaml" |
|
/test-examples="examples/batch/v1beta1/schedulingpolicy.yaml" https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/13012178929 |
|
/test-examples="examples/cloudformation/v1beta1/stack.yaml" https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/13012210165 |
|
/test-examples="examples/elasticsearch/v1beta1/domain.yaml" https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/13017760965 |
|
/test-examples="examples/networkmanager/v1beta1/customergatewayassociation.yaml" https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/13012306301 |
|
/test-examples="examples/ecs/v1beta1/cluster.yaml" https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/13013003698 |
|
I have run a couple of examples in aws-us-gov, and I can see them all become Ready/Synced=True. They also have proper ARNs. Is there a way you'd like me to help report test results? |
Hi @zonybob, |
Signed-off-by: Matt Bush <[email protected]> # Conflicts: # internal/clients/aws.go
…tition Signed-off-by: Matt Bush <[email protected]> # Conflicts: # config/externalname.go
Signed-off-by: Matt Bush <[email protected]>
Signed-off-by: Matt Bush <[email protected]>
Signed-off-by: Matt Bush <[email protected]>
Signed-off-by: Erhan Cagirici <[email protected]>
Signed-off-by: Erhan Cagirici <[email protected]>
Signed-off-by: Erhan Cagirici <[email protected]>
Signed-off-by: Erhan Cagirici <[email protected]>
Signed-off-by: Erhan Cagirici <[email protected]>
Signed-off-by: Erhan Cagirici <[email protected]>
Signed-off-by: Erhan Cagirici <[email protected]>
Signed-off-by: Erhan Cagirici <[email protected]>
Signed-off-by: Erhan Cagirici <[email protected]>
- OpenIDConnectProvider.iam - InternetGateway.ec2 - Subnet.ec2 Signed-off-by: Fatih Türken <[email protected]>
Signed-off-by: Erhan Cagirici <[email protected]>
Signed-off-by: Fatih Türken <[email protected]>
Signed-off-by: Fatih Türken <[email protected]>
|
/test-examples="examples/elasticsearch/v1beta1/domain.yaml" https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/13017760965 |
There was a problem hiding this comment.
@erhancagirici @turkenf @mbbush thank you all very much for the great effort here, it is a very impressive development for the provider-upjet-aws. LGTM!
Description of your changes
Revives the the original PR from @mbbush #1223
rebased onto latest main
added aws-iso-e partition
default regions for IAM MRs are now generated from AWS service definitions, instead of being hardcoded.
make generatewill be handling the generation.Introduced a new accepted value
Autoforspec.endpoint.url.typeinProviderConfig. This allows specifying a partition inspec.endpoint.partitionIdwithout having to specify a full endpoint configuration. Relevant AWS endpoints will be automatically resolved.MRs with
regionparameter will use theregionto infer thepartitionwithout extra config in theProviderConfig.ProviderConfigand theregionprovided by the resource conflicts, this conflict will intentionally cause an error during reconcile.For Region-less MRs (like MRs in
iam.aws.upbound.io) to work with non-default partitions, referencedProviderConfigmust specify thepartitionId, an example foraws-cnpartition is:This
ProviderConfigwith explicit partition config is the recommended way to work with non-default AWS partitions.I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
aws-cnandaws-us-govaccess. Special thanks to them!