search

cloudposse / terraform-aws-eks-node-group

Terraform module to provision a fully managed AWS EKS Node Group
https://cloudposse.com/accelerate
Apache License 2.0
91 stars 128 forks source link

feat: Implement optional additional principals #51

Closed trallnag closed 3 years ago

trallnag commented 3 years ago

what

This PR adds a variable that allows users to declare additional principals that should be included in the assume role policy.

why

Where I work the managed AWS accounts enforce that every role should be able be assumed by a "watchdog" role. If a role does not contain this trusted relationship, it will be added automatically and lead to Terraform drift.

todo

If this PR is in principle acceptable I would require tips / help:

trallnag commented 3 years ago 
==> Executing Valid Owner Checker (74.490722ms)
    [err] line 7: HTTP error occurred while calling GitHub: GET https://api.github.com/repos/cloudposse/terraform-aws-eks-node-group/teams?per_page=100: 404 Not Found []

I assume that I don't have any influence on this error

Nuru commented 3 years ago 
==> Executing Valid Owner Checker (74.490722ms)
    [err] line 7: HTTP error occurred while calling GitHub: GET https://api.github.com/repos/cloudposse/terraform-aws-eks-node-group/teams?per_page=100: 404 Not Found []

I assume that I don't have any influence on this error

@trallnag Thank you for your PR. Please rebase this PR on the current master branch. That should fix this error, and also provide automatic code formatting and documentation generation when you make commits. Mainly, documentation for variables derives from their Terraform declaration, particularly the description so please give the description field extra attention.

We are still considering some subtle points about your PR, such as the variable name, but should have an answer for you soon. Are there any other Cloud Posse Terraform modules where you think you will want to make the same change?

Nuru commented 3 years ago

@aknysh @maximmi Is there a way we can add automated testing for this feature? Have some similar CP bot user role we could add and try to assume the role?

trallnag commented 3 years ago

Hey @Nuru,

Are there any other Cloud Posse Terraform modules where you think you will want to make the same change?

For me personally the other terraform-aws-eks-* Cloudposse modules would be interesting as well. I'm postponing opening PRs / issues there for now to not have duplicated discussions

Nuru commented 3 years ago

Hey @Nuru,

Are there any other Cloud Posse Terraform modules where you think you will want to make the same change?

For me personally the other terraform-aws-eks-* Cloudposse modules would be interesting as well. I'm postponing opening PRs / issues there for now to not have duplicated discussions

@trallnag Please expand on the list you would like to see changed. Like you, we want to have this discussion just once (here) and solve it for all the modules you are going to want to update in the foreseeable future. It will help us to see all the use cases to ensure that we come up with a robust solution that can be consistently applied across all the modules.

trallnag commented 3 years ago

@Nuru, here is the list:

But these are modules I would be using / I already use

mergify[bot] commented 3 years ago

This pull request is now in conflict. Could you fix it @trallnag? 🙏