Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't add trailing slash to provided --base-role-arn #195

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Bobonium
Copy link

The current implementation always adds a trailing slash to the baseRoleArn if the last character ist not already a slash. This behavior breaks my use-case.

Here's an example of my use case:
full arn:

  • A) arn:aws:iam::1234567890:role/helloworld-foo
  • B) arn:aws:iam::1234567890:role/helloworld-bar

base-role-arn: arn:aws:iam::1234567890:role/helloworld-

pod annotation for iam A: iam.amazonaws.com/role: foo
pod annotation for iam B: iam.amazonaws.com/role: foo

Everything else in the repository would already allow my use-case to work, but the fact that the slash is currently always enforced breaks it completely.

As a sidenote the current regex for validating is ^arn:(\w|-)*:iam::\d+:role\/?(\w+|-|\/|\.)*$
But according to the AWS IAM reference slashes are not allowed as a part of the role-name

Names of users, groups, roles, policies, instance profiles, and server certificates must be alphanumeric, including the following common characters: plus (+), equal (=), comma (,), period (.), at (@), underscore (_), and hyphen (-).

Therefore I think the correct regex should also be: ^arn:(\w|-)*:iam::\d+:role\/?(\w+|\+|@|-|\.|\,|\=|\_)*$
Although I did not touch that as I have not verified if the AWS documentation is correct in that regard.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 19.786% when pulling 5ac8264 on Bobonium:allow_base_role_arn_prefix into 80efbb1 on jtblin:master.

@gramidt
Copy link

gramidt commented Feb 20, 2019

I was actually planning to make this change too! Glad you have made it!

My use case is similar as well.

Use case:

base-role-arn: arn:aws:iam::1234567890:role/acme-prod-us-east-2-k8s-

pod annotation for some role: iam.amazonaws.com/role: user-reader

This is required so that our configs do not need to know the full prefix (acme-prod-us-east-2-k8s-) which is a combination of namespace, environment, and region. Currently I have some magic taking care of this, but would love to remove that and simplify our configs for our multi region deployments.

We may however want to make the this configurable, so we don't break existing usage for everyone that may have forgot to put a trailing slash on the arn.

@Bobonium
Copy link
Author

This project is still in it's 0.X.X so there should be no need to enforce backwards compatibility. Especially since the adding of the slash is so far in no way implied based on the documentation. Additionally even the regex is built in a way that it will fail the initial check without it. Therefore I would not add another Parameter, that no one really needs because that would just add up in the future.

@gramidt
Copy link

gramidt commented Mar 9, 2019

Makes complete sense, @Bobonium!

LGTM!

@patrickjahns
Copy link

@mwhittington21 @jtblin

Is it possible to integrate the change upstream? Any further requirements/tasks from your side in order to include it in one of the next releases?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants