Skip to content
This repository was archived by the owner on Mar 5, 2024. It is now read-only.

Adding ARN prefix for assumeRoleArn automatically #459

Merged
merged 1 commit into from
Jul 7, 2021

Conversation

njuettner
Copy link
Contributor

We ran into some issues when using --role-base-arn-autodetect and not providing full ARN via --assume-role-arn in kiam v4.0 .

As you can see in the server logs, the ARN is autocompleted for the requested role by the pod (z2h2q-Route53Manager-Role) but it's not autocompleted for the role that the server has to assume (z2h2q-IAMManager-Role).

{"level":"error","msg":"error requesting credentials: AccessDenied: User: arn:aws:sts::999999999999:assumed-role/gs-cluster-z2h2q-role-tccpn/i-038e8e95d771c82ad is not authorized to perform: sts:AssumeRole on resource: z2h2q-IAMManager-Role\n\tstatus code: 403, request id: f5e0a631-c0cb-4a9c-beaa-580a24ef96ac","pod.iam.role":{"Name":"z2h2q-Route53Manager-Role","ARN":"arn:aws:iam::999999999999:role/z2h2q-Route53Manager-Role"},"pod.iam.roleArn":"arn:aws:iam::999999999999:role/z2h2q-Route53Manager-Role","time":"2021-01-12T16:40:19Z"}
{"generation.metadata":0,"level":"error","msg":"error warming credentials: AccessDenied: User: arn:aws:sts::999999999999:assumed-role/gs-cluster-z2h2q-role-tccpn/i-038e8e95d771c82ad is not authorized to perform: sts:AssumeRole on resource: z2h2q-IAMManager-Role\n\tstatus code: 403, request id: f5e0a631-c0cb-4a9c-beaa-580a24ef96ac","pod.iam.role":"z2h2q-Route53Manager-Role","pod.name":"external-dns-67ddc97ccd-drg9f","pod.namespace":"kube-system","pod.status.ip":"10.2.41.94","pod.status.phase":"Running","resource.version":"10056","time":"2021-01-12T16:40:19Z"}

This is our server parameters that we use for release 3.X and if we put the full ARN it works perfectly:

 containers:
     - args:
       - --json-log
       - --level=info
       - --bind=0.0.0.0:6443
       - --cert=/etc/kiam/tls/tls.crt
       - --key=/etc/kiam/tls/tls.key
       - --ca=/etc/kiam/tls/ca.crt
       - --role-base-arn-autodetect
       - --assume-role-arn=z2h2q-IAMManager-Role
       - --session-duration=15m
       - --sync=1m
       - --prometheus-listen-addr=0.0.0.0:9620
       - --prometheus-sync-interval=5s
       - --region=eu-west-1

The PR tries to fix adding the ARN prefix when not giving the full string.

Additionally it removes two fields from credentialsCache which are not used anymore and a small error when logging an error.

@pingles
Copy link
Contributor

pingles commented Jan 14, 2021

Thanks for highlighting this!

Could we add some tests to the ServerBuilder or elsewhere to prove and document the newly corrected behaviour please?

@njuettner
Copy link
Contributor Author

njuettner commented Jan 15, 2021

@pingles sure, in order to do this I would need to modify the code. I would need to mock the ec2 metadata service in https://github.com/uswitch/kiam/blob/master/pkg/aws/sts/resolver_detect_arn.go#L42. Because the prefix is taken from the instance IAM info.

Do you have a good idea how to mock ec2metadata? Do you already have something in place?

@cyrus-mc
Copy link

cyrus-mc commented Jul 6, 2021

This should really be called out in the upgrade notes for v4.0. For version 3.6 my setup was as follows:

  • set --role-base-arn-autodetect
  • set assume-role-arn to just the IAMRole name

This worked. Immediately after upgrading to v4.0 we experienced the following errors:

{"level":"error","msg":"error requesting credentials: InvalidParameter: 1 validation error(s) found.\n- minimum field size of 20, AssumeRoleInput.RoleArn.\n","pod.iam.role":{"Name":"EKS_flux.test","ARN":"arn:aws:iam::xxxx:role/EKS_flux.test"},"pod.iam.roleArn":"arn:aws:iam::xxxx:role/EKS_flux.test","time":"2021-07-02T20:24:22Z"}

Took a lot of digging to figure out but the only combination of configuration that seems to work for 4.0 is

  • set --role-base-arn
  • set assume-role-arn to the full ARN

Specifying this in the upgrade notes as a breaking change (until this PR is merged) would have saved me a fair amount of time.

@pingles pingles merged commit 8103483 into uswitch:master Jul 7, 2021
@pingles
Copy link
Contributor

pingles commented Jul 7, 2021

Sorry @cyrus-mc; wasn't intentional to leave out breaking changes in the notes. I've merged this change, I'll try and tag for a point release in a bit.

@njuettner njuettner deleted the fix-assume-role branch July 8, 2021 11:50
jontg added a commit to airkit/kiam-fork that referenced this pull request May 18, 2022
GenPage added a commit to plangrid/kiam that referenced this pull request Jun 12, 2023
GenPage added a commit to plangrid/kiam that referenced this pull request Jun 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants