allow using assumeRoleChain with IAM resources by ctkeyser · Pull Request #451 · crossplane-contrib/provider-upjet-aws

ctkeyser · 2023-01-17T16:33:55Z

Description of your changes

Fixes #215

currently assumeRoleChain doesn't work for IAM resources because IAM resources don't require a region. the error is

cannot get terraform setup: failed to retrieve aws credentials from aws config: failed to refresh cached credentials, operation error STS: AssumeRole, failed to resolve service endpoint, an AWS region is required, but was not found

this change adds a fallback to "aws-global" region for STS requests in cases where no region is set in the managed resource

I have:

Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

I built the provider w/ the fix, installed it in my environment, and confirmed I no longer get the 'region is required' error

Upbound-CLA · 2023-01-17T16:33:59Z

All committers have signed the CLA.

ulucinar · 2023-01-24T11:48:37Z


+	// AssumeRoleSTSRegion defines the region used for STS requests. If not defined,
+	// the managed resource region will be used if available
+	AssumeRoleSTSRegion *string `json:"assumeRoleSTSRegion,omitempty"`


An alternative to consider would be to add the the STS region option to the v1beta1.AssumeRoleOptions type. Then, the ProviderConfig would look like:

... spec: assumeRoleChain: - stsRegion: us-west-1 roleARN: <role ARN 1> - stsRegion: us-west-1 roleARN: <role ARN 2> credentials: secretRef: key: creds name: aws-creds namespace: upbound-system source: Secret

However, you'd need to configure an STS region for each role in the chain if you are provisioning a resource without a region (e.g., IAM role) and not sure if there's utility in configuring different STS regions for different roles in the chain. Of course, if needed, we could have both levels of API exposed, e.g., a spec.assumeRoleSTSRegion and spec.assumeRoleChain[].stsRegion (which we could add later if needed). So, I think the proposed API is fine. What do you think?

my first idea was also to put it in v1beta1.AssumeRoleOptions, but then I moved it because I didn't think there would be a need to chain assume role requests across different regions. I definitely would have rather put it inside an assumeRole config, but the current structure doesn't really support that. so I think this is probably the best option

As a side note for documentation purposes, we had also previously discussed defaulting to a region specified in the ProviderConfig if a managed resource does not specify an optional spec.forProvider.region. The discussion's context was not this PR's target issue (but rather the UX around provider-aws MR APIs). But looks like it's also related to the current discussion.

ulucinar · 2023-01-24T11:50:40Z


+	// AssumeRoleSTSRegion defines the region used for STS requests. If not defined,
+	// the managed resource region will be used if available
+	AssumeRoleSTSRegion *string `json:"assumeRoleSTSRegion,omitempty"`


nit: We may give an example of the new API under examples/providerconfig/, e.g., in examples/providerconfig/user-creds-with-assume-role.yaml or in a new example file.

yeah definitely

ulucinar · 2023-01-24T11:52:08Z

Hi @ctkeyser,
Thank you for the proposal. I think it looks fine but left some review comments so that we discuss some alternatives. Also left a comment here regarding the underlying issue.

ulucinar · 2023-01-24T11:52:34Z

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

ctkeyser · 2023-01-24T14:47:04Z

thank you for the review @ulucinar. let me know if you have more thoughts on this

ulucinar · 2023-01-25T06:47:13Z

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

ulucinar

Thank you @ctkeyser for fixing the issue with provisioning IAM resources when using STS role chaining, lgtm.

hasheddan · 2023-01-30T15:42:58Z

+	regionOpt := func(o *sts.Options) {
+		if pcs.AssumeRoleSTSRegion != nil {
+			o.Region = *pcs.AssumeRoleSTSRegion
+		}
+	}


@ctkeyser @ulucinar I wonder if it would make sense to fall back to the AWS global partition here if an STS region is not defined? The AWS SDK itself will do so if it does not recognize the region that is passed, but will error if no region is passed. Thoughts?

Hi @hasheddan,
Makes sense to me.

that sounds good to me, but I'm not sure what that change looks like. I tried changing region to global and aws-global and I ended up getting errors saying dns doesn't resolve for sts.aws-global.amazonaws.com or sts.global.amazonaws.com. dns does resolve for sts.us-west-1.amazonaws.com though

I also don't see STS in this list of global endpoints https://docs.aws.amazon.com/general/latest/gr/rande.html#global-endpoints

@ctkeyser I believe it may just be sts.amazonaws.com https://docs.aws.amazon.com/IAM/latest/UserGuide/id_credentials_temp_enable-regions.html

ok I tested just falling back to aws-global if no region set, and that does work in my environment:

--- a/internal/clients/provider_config.go +++ b/internal/clients/provider_config.go @@ -265,8 +265,8 @@ func UseProviderSecret(ctx context.Context, data []byte, profile, region string) func GetRoleChainConfig(ctx context.Context, pcs *v1beta1.ProviderConfigSpec, cfg *aws.Config) (*aws.Config, error) { pCfg := cfg regionOpt := func(o *sts.Options) { - if pcs.AssumeRoleSTSRegion != nil { - o.Region = *pcs.AssumeRoleSTSRegion + if cfg.Region == "" { + o.Region = GlobalRegion } } for _, aro := range pcs.AssumeRoleChain {

I was able to assume a role and create an IAM resource with that
I also tried changing getRegion to return GlobalRegion if no region was found in the managed resource, but that didn't work. got the error:

managed/iam.aws.upbound.io/v1beta1, kind=role cannot run refresh: refresh failed: Invalid AWS Region: aws-global

do you guys think this is good enough and we don't need the providerconfig option? so basically any region-less managed resource will use aws-global for STS. that seems simple enough to me

This seems reasonable to me -- we can always expand the API of ProviderConfig if needed. So in summary:

If an assume role chain is being used and the MR specifies a region, we will use that region endpoint for the STS operations.

If chain is being used and the MR does not specify a region, we use the global STS endpoint.

I think potentially using a separate region's STS endpoint could be desired by a user, but let's wait until it is requested since it involves API expansion. Are you aligned @ulucinar?

I don't know much about the use cases here. But one hypothetical scenario I may imagine is what happens if networking access to the host sts.amazonaws.com is blocked (due to law, temporary issue, etc.). But my understanding is that it (role chaining) was not working up to now for region-less resources and if we make a backward-compatible change (i.e., only revert back to sts.amazonaws.com for role chaining if the MR does not specify a region), we should not be breaking any existing (already working) scenarios and would be solving the problem for at least a subset of the users (of provider-aws).

So, I think we are good to go. But to be on the safe side, even if, currently, we will not allow configuration of the STS region for region-less resources, let's scope the change only for region-less resources (so that the change is backward-compatible as discussed above).

ok I pushed the change. it only sets the region to aws-global for STS requests when no region is set, so it should only occur in situations where it currently doesn't work at all

ulucinar · 2023-02-01T14:24:27Z

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

hasheddan

One comment but non-blocking -- thanks @ctkeyser! Might also be worth squashing commit history here 👍🏻

hasheddan · 2023-02-01T14:25:54Z

+	regionOpt := func(o *sts.Options) {
+		if cfg.Region == "" {
+			o.Region = GlobalRegion
+		}
+	}


might be nice to pull this out into something like:

NewRegionFallbackOpt(region string) func(*sts.Options) { if region != "" { return func(*sts.Options) {} } return func(o *sts.Options) { o.Region = GlobalRegion } }

But I am fine with deferring doing so for now.

ctkeyser · 2023-02-01T15:11:12Z

@hasheddan should I squash and force-push or is squash+merge preferred?

hasheddan · 2023-02-01T15:12:28Z

@ctkeyser squash and force push would be great if you don't mind!

hasheddan · 2023-02-01T16:12:21Z

I'll go ahead and merge here since @ulucinar previously approved and indicated alignment on the updated direction 👍🏻

ctkeyser force-pushed the assumeroleregion branch from 435addc to ffc0bac Compare January 17, 2023 16:55

ulucinar self-requested a review January 23, 2023 21:57

ulucinar reviewed Jan 24, 2023

View reviewed changes

ulucinar approved these changes Jan 26, 2023

View reviewed changes

haarchri mentioned this pull request Jan 28, 2023

Not able to create IAM Role using assumeRole in providerConfig crossplane/crossplane#3675

Closed

hasheddan reviewed Jan 30, 2023

View reviewed changes

hasheddan approved these changes Feb 1, 2023

View reviewed changes

use aws-global region for STS if no region set

fa0eafb

ctkeyser force-pushed the assumeroleregion branch from ddfcdf1 to fa0eafb Compare February 1, 2023 15:14

hasheddan merged commit 4d3ead6 into crossplane-contrib:main Feb 1, 2023

hasheddan mentioned this pull request Mar 7, 2023

Default STS region to global for WebIdentity #593

Merged

1 task

Uh oh!

Conversation

ctkeyser commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of your changes

How has this code been tested

Uh oh!

Upbound-CLA commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ulucinar Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ulucinar commented Jan 24, 2023

Uh oh!

ulucinar commented Jan 24, 2023

Uh oh!

ctkeyser commented Jan 24, 2023

Uh oh!

ulucinar commented Jan 25, 2023

Uh oh!

ulucinar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ulucinar Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ulucinar commented Feb 1, 2023

Uh oh!

hasheddan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctkeyser commented Feb 1, 2023

Uh oh!

hasheddan commented Feb 1, 2023

Uh oh!

hasheddan commented Feb 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ctkeyser commented Jan 17, 2023 •

edited

Loading

Upbound-CLA commented Jan 17, 2023 •

edited

Loading

ulucinar Jan 24, 2023 •

edited

Loading

ulucinar Feb 1, 2023 •

edited

Loading