capa fix hardcoded role arn for aws iam authenticator

[AmitSahastra]

State

READY

What is the purpose of the pull request

ARN hardcoded with ":aws:" partition do not work with "us-gov" cloud.

Implementation

Issue: For few roles/policies arn partition is hardcoded to "aws". While working with "aws-us-gov" cloud partition this does not work and cluster creation failed.

Fix: Check for aws cloud "partition" or "region" to derive correct arn and use it.

/kind bug

Special notes for your reviewer:

Checklist:

• squashed commits
• includes documentation
• adds unit tests
• adds or updates e2e tests

[k8s-ci-robot]

Hi @AmitSahastra. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Skarlso]

/ok-to-test

[AmitSahastra]

/retest

[Skarlso]

/lgtm

@sadysnaat Are you happy with this too? :)

[Skarlso]

Generally, it's okay; I have one question. Most changes restrict the default partition and only allow the government partition to work.

Is that the intention?

[Skarlso]

Ops sorry I probably fumbled one of the suggestions.

[AmitSahastra]

Generally, it's okay; I have one question. Most changes restrict the default partition and only allow the government partition to work.

Is that the intention?

The intention was to make sure the provider works for the specific partition.

Due to the hardcoded "aws" partition, there was an issue with resolving regions, ami, and images that are different for different partitions, making sure we look dynamically for region/partition so it works for both.

Avoided going for pain partition here so as to not expose resources meant for specific partition to be exposed to others.

[sadysnaat]

Instead of determining this partition all the time using if else we should do it one time and store the result

and call functions with partition

[AmitSahastra]

Currently

/lgtm
@sadysnaat Are you happy with this too? :)

I think we need to refactor the duplicate code and avoid deducing partition using if else all the time

we should do this once and store in service.

@sadysnaat AND @Skarlso Pls take look at the latest code, have made code changes to have util func to get a portion from the region, do we want to update service/crd to store partition ??

[Skarlso]

@sadysnaat @AmitSahastra So sorry, but on second thought, I don't think we need to store this info for three reasons:

• It's derived automatically, anyways
• We didn't store it in the first place
• If we DO store it, that will generate a diff of templates in tools such as terraform or other diff tracking appliances between the local template and the applied one since it changes after it's applied.

That last was the best counterargument, as it can confuse users.

I suggest reverting the spec part. The rest is fine to merge. WDYT?

[sadysnaat]

Hi @Skarlso

my points

It's derived automatically, anyways

yes but if we store if we need not to derive it again and again like we do for

cluster-api-provider-aws/api/v1beta1/network_types.go

Line 149 in f63dcb9

type NetworkSpec struct {

where we backfill networking info

We didn't store it in the first place

that's because this flow was broken and didn't need derivation, for older versions capa would fail to provision bastion host

If we DO store it, that will generate a diff of templates in tools such as terraform or other diff tracking appliances between the local template and the applied one since it changes after it's applied.

I think we do it all the time one mentioned in networkspec other place is https://github.com/kubernetes-sigs/cluster-api/blob/0587101abbc0d558af4f2715b0be536b825742f6/api/v1beta1/cluster_types.go#L53
where endpoint is filled in after cluster provisioning has begun.

I am not sure how tools handle this but if you have reference to any repo which gets affected would like to see how clusterapi.cluster.endpoint changes are handled there

[Skarlso]

Right. We do it all the time but it's not necessarily good. 😁

Okay then. Fine with me.

[AmitSahastra]

Right. We do it all the time but it's not necessarily good. 😁

Okay then. Fine with me.

@Skarlso @sadysnaat can you provide final review on this.
any action item pending from my side ?

[Skarlso]

Okay, if we are doing this, than it's incorrect that v1beta1 has changes in it and the conversion is incorrect too. I'll try to show you later how it can be done.

[Skarlso]

/test ?

[k8s-ci-robot]

@Skarlso: The following commands are available to trigger required jobs:

• /test pull-cluster-api-provider-aws-build
• /test pull-cluster-api-provider-aws-test
• /test pull-cluster-api-provider-aws-verify

The following commands are available to trigger optional jobs:

• /test pull-cluster-api-provider-aws-apidiff-main
• /test pull-cluster-api-provider-aws-e2e
• /test pull-cluster-api-provider-aws-e2e-blocking
• /test pull-cluster-api-provider-aws-e2e-clusterclass
• /test pull-cluster-api-provider-aws-e2e-conformance
• /test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts
• /test pull-cluster-api-provider-aws-e2e-eks
• /test pull-cluster-api-provider-aws-e2e-eks-gc
• /test pull-cluster-api-provider-aws-e2e-eks-testing

Use /test all to run the following jobs that were automatically triggered:

• pull-cluster-api-provider-aws-apidiff-main
• pull-cluster-api-provider-aws-build
• pull-cluster-api-provider-aws-test
• pull-cluster-api-provider-aws-verify

In response to this:

/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Skarlso]

/test pull-cluster-api-provider-aws-e2e-eks
/lgtm

[Skarlso]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Skarlso

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Skarlso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Skarlso]

It looks like this needs to be reverted, since this has been merged the periodic e2e unmanaged S3 thing fails.

[Skarlso]

@AmitSahastra Could you open this again please in a separate PR so we can run other tests on it?

We need to find out what went wrong. :)

[AmitSahastra]

@AmitSahastra Could you open this again please in a separate PR so we can run other tests on it?

We need to find out what went wrong. :)

Saw this comment today. have opened new PR #4010.
Pls take a look.

[Skarlso]

Thanks!