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

Rework nodegroup IAM fields for v1alpha4 #468

Merged
merged 3 commits into from
Jan 29, 2019
Merged

Rework nodegroup IAM fields for v1alpha4 #468

merged 3 commits into from
Jan 29, 2019

Conversation

errordeveloper
Copy link
Contributor

@errordeveloper errordeveloper commented Jan 25, 2019

Description

Fixes #464.

Checklist

  • Code compiles correctly (i.e make build)
  • Added tests that cover your change (if possible)
  • All tests passing (i.e. make test)

@errordeveloper
Copy link
Contributor Author

This is mostly ready but I need to make sure we can get instance role ARN back once we are given the profile ARN, or we might just require both to be set. The role ARN is required for nodegroup auth config map (unless there is a way to pass profile ARN of instead role ARN - should check docs).

@errordeveloper
Copy link
Contributor Author

@christopherhein do you happen to know if authenticator requires strictly instance role ARNs, or it could work with instance profile ARNs?

@christopherhein
Copy link
Contributor

@christopherhein do you happen to know if authenticator requires strictly instance role ARNs, or it could work with instance profile ARNs?

IIRC it will require the role ARN cause Instance Profile ARNs don't have support for assume role or for the get-caller-identity request, what resolves on the node when the authenticator is used is an assumed role for the associated role.

Could use this request to get the proper role - https://docs.aws.amazon.com/cli/latest/reference/iam/get-instance-profile.html

@errordeveloper
Copy link
Contributor Author

Thanks, Chris! Sounds like we can make that call, but if user sets profile and role ARNs, they can save themselves from that call if they absolutely must :)

@errordeveloper errordeveloper force-pushed the iam-fields branch 2 times, most recently from 00a2576 to 5f49a01 Compare January 28, 2019 13:38
@errordeveloper errordeveloper changed the title WIP: Rework nodegroup IAM fields for v1alpha4 Rework nodegroup IAM fields for v1alpha4 Jan 28, 2019
@errordeveloper
Copy link
Contributor Author

@knorby if you'd like to take a look, this is functionally completion, just needs some tests; I'm gonna add the docs in a separate PR, as there are quite a few API changes coming with v1alpha4.

- add instance profile ARN field
- use profile and role ARNs when both are set, or fetch instance role
  based on profile, otherwise create profile or create both
Copy link
Contributor

@christopherhein christopherhein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @errordeveloper 👍

I can't seem to see why that test is failing…

@knorby
Copy link
Contributor

knorby commented Jan 28, 2019

@errordeveloper Thanks for getting this out so quick!

So, I ran a couple tests, and I was able to get working results when I set both the role and instance profile ARN (I haven't tested with just the profile, which looks like would have its own distinct behavior). With the role only, I get this error:

[✖]  AWS::EC2::SecurityGroup/SG: CREATE_FAILED – "Resource creation cancelled"
[✖]  AWS::IAM::InstanceProfile/NodeInstanceProfile: CREATE_FAILED – "The specified value for roleName is invalid. It must contain only alphanumeric characters and/or the following: +=,.@_- (Service: AmazonIdentityManagement; Status Code: 400; Error Code: ValidationError; Request ID: [...])"

I assume the bad characters are coming from the full ARN string. I ended up just using the autogenerated Instance Profile, which is what I assume a lot of people would do. Speaking personally, I don't really know all the nuances of roles over profiles and I can't imagine doing anything else, so I'd vote for just pulling the profile from the role.

@errordeveloper
Copy link
Contributor Author

@knorby thanks for testing and sharing your thoughts!

So, I ran a couple tests, and I was able to get working results when I set both the role and instance profile ARN (I haven't tested with just the profile, which looks like would have its own distinct behavior).

Yes, if you set just the profile, you can save yourself from taking care of an additional identifier. But some user might want to set both as they don't have access to IAM APIs and are given identifier of IAM resources that are managed by others in their org.

I assume the bad characters are coming from the full ARN string.

Seems like. We don't validate ARNs just yet, as you get an error from CloudFormation fairly quickly. Also, we do need to make parameter validation more high-level than it currently is.

I ended up just using the autogenerated Instance Profile, which is what I assume a lot of people would do.

Yes, that's really the safest choice, and it means you don't have to take worry about what the role is, and you can enable autoscaler, ECR access and Route53 very easily.

May I ask what was the motivation to set custom role in the first place?

Speaking personally, I don't really know all the nuances of roles over profiles and I can't imagine doing anything else, so I'd vote for just pulling the profile from the role.

As I mentioned above, some users cannot make any IAM calls, so we need to provide a way for them to pass both ARNs...

@errordeveloper errordeveloper merged commit bdd08b3 into master Jan 29, 2019
@errordeveloper
Copy link
Contributor Author

I can't seem to see why that test is failing…

@christopherhein it was a flaky one; I opened #481.

@errordeveloper errordeveloper deleted the iam-fields branch January 29, 2019 13:26
@knorby
Copy link
Contributor

knorby commented Jan 30, 2019

@errordeveloper

Yes, that's really the safest choice, and it means you don't have to take worry about what the role is, and you can enable autoscaler, ECR access and Route53 very easily.

May I ask what was the motivation to set custom role in the first place?

Sorry, I may not have been clear. I meant the instance profile associated with the existing role, whatever AWS just gives you. I'm using kube2iam with purpose-specific roles for each of the policies you mentioned, for a variety of reasons. Trust relationships were the snag I was hitting.

torredil pushed a commit to torredil/eksctl that referenced this pull request May 20, 2022
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.

3 participants