feat(eks): IAM roles for service accounts #6062
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
@eladb question regarding CDK as related to this PR. Following to the EKS Documentation I'm trying create an IAM role with an assume policy which contains a condition. This is something that can easily be done with CDK. However the condition's key needs to be a calculated value of the custom resource, which is a token in CDK. CloudFormation doesn't support this. Is there are way to resolve this before passing it through? const condition: { [id: string]: any; } = {};
condition[this.openIDConnectSubject!] = `system:serviceaccount:${namespace}:${name}`; |
| * | ||
| * @example https://5E1D0CEXAMPLEA591B746AFC5AB30262.yl4.us-west-2.eks.amazonaws.com | ||
| */ | ||
| public readonly openIDConnectIssuerUrl: string; |
There was a problem hiding this comment.
Since this is an attribute it should have a prefix “cluster”. Rename to: “clusterOpenIdConnectIssueUrl”
Yes, indeed, CloudFormation doesn't support using intrinsic functions as map keys... Can you provide a bit more context into your code? I am not sure I fully see where you are using this. |
|
I've pushed the code how it should work as you can see in the |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
@eladb the initial implementation is working. Eventually I've solved the problem if IAM condition keys with a CustomerResource. It's not the most elegant solution, but I think the only one available at the moment. I'm currently focusing on the tests. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Initial round of comments.
Would be nice to validate this logic against https://github.com/smaato/cfn-resource-provider.
I also feel like perhaps we should actually move these two new resources to the IAM module instead of the EKS module... This might be a bit of a hassle given the IAM module can't use the custom resource provider framework at the moment (i.e. it needs to use "raw" custom resources).
| throw new Error( | ||
| `Cannot delete a profile without a physical id` | ||
| ); |
There was a problem hiding this comment.
| throw new Error( | |
| `Cannot delete a profile without a physical id` | |
| ); | |
| throw new Error(`Cannot delete an OIDC provider without a physical id`); |
| const deleteOpenIDConnectProviderResponse = await this.eks.deleteOpenIDConnectProvider( | ||
| deleteOpenIDConnectProvider | ||
| ); |
There was a problem hiding this comment.
style nit:
| const deleteOpenIDConnectProviderResponse = await this.eks.deleteOpenIDConnectProvider( | |
| deleteOpenIDConnectProvider | |
| ); | |
| const deleteOpenIDConnectProviderResponse = await this.eks.deleteOpenIDConnectProvider(deleteOpenIDConnectProvider); |
| } catch (getOpenIDConnectProviderError) { | ||
| if (getOpenIDConnectProviderError.code === "NoSuchEntity") { | ||
| this.log( | ||
| "received NoSuchEntityFoundException, this means the profile has been deleted (or never existed)" |
There was a problem hiding this comment.
| "received NoSuchEntityFoundException, this means the profile has been deleted (or never existed)" | |
| "received NoSuchEntityFoundException, this means the provider has been deleted (or never existed)" |
| if (!statement.Condition) { | ||
| statement.Condition = {}; | ||
| } | ||
| statement.Condition.StringEquals = condition; |
There was a problem hiding this comment.
what happens if the policy already has a statement.Condition.StringEquals?
|
|
||
| const role = await this.eks.getRole({ RoleName: this.physicalResourceId}); | ||
| if (!role) { | ||
| throw new Error(`no role found with name ${this.physicalResourceId}`); |
There was a problem hiding this comment.
Should we just consider this a success? (idempotency)
| const deleteOpenIDConnectProvider: aws.IAM.DeleteOpenIDConnectProviderRequest = { | ||
| OpenIDConnectProviderArn: this.physicalResourceId | ||
| }; | ||
|
|
There was a problem hiding this comment.
Maybe call "checkResource" first and return success if it's not found (idempotency)
|
Do you need any help with this PR? Happy to contribute some code in order to push this forward. |
|
Thanks @RafalWilinski your help would be appreciated. The coming week I might have some more time to finish this. The code is working but needs a bit of refactoring and the addition of tests. @eladb I'm wondering if the code needs to move the to |
Yes, I think it needs to move to IAM, which makes this dramatically more complex. Let me pick this task up and then you'll be able to leverage this work here. OK? |
|
@eladb sure, that will be great. Last night I was struggling a bit how to put this in |
I have used the same logic as what they regarding creating the provider. However their update logic is more sophisticated which I can change in my implementation to be more resilient to to changes from EKS. (Client ID or Certificate changes) |
packages/@aws-cdk/aws-iam/README.md
Outdated
| }); | ||
| ``` | ||
|
|
||
| The `OpenIdConnectPrincipal` class can be used as a principal used with a `OpenIdConnectProvider`, for example: |
There was a problem hiding this comment.
Can you move this to the open is connect section please?
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
You'll need to update the EKS integration tests by running |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Got it 👍 Took some time to get it run correctly on one of my AWS accounts. |
eladb
changed the title
|
@vlesierse updated PR title & message |
|
Amazing! Thank you @vlesierse and @eladb for all the great work. |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
## Commit Message feat(eks): IAM roles for service accounts (aws#6062) Adds support for IAM roles for service account which allows pods the assume IAM roles. NOTE: currently there are no condition set on the IAM Role which results that there are no restrictions on other pods to assume the role. This will be fixed in a subsequent PR. See README for details. Fixes aws#5388 Fixes aws#3949 ## End of Commit Message - [x] Enable OpenID Connect Provider - [x] Service Account construct - [ ] Role constraints - [x] Add `cluster.addServiceAccount` convenience method - [x] Integration Tests - [x] Unit Tests - [x] Update README.md ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* <!-- Please read the contribution guidelines and follow the pull-request checklist: https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md -->
Commit Message
feat(eks): IAM roles for service accounts (#6062)
Adds support for IAM roles for service account which allows pods the assume IAM roles.
NOTE: currently there are no condition set on the IAM Role which results that there are no restrictions on other pods to assume the role. This will be fixed in a subsequent PR.
See README for details.
Fixes #5388
Fixes #3949
End of Commit Message
cluster.addServiceAccountconvenience methodBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license