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

feat: Implement optional additional principals #51

Closed
wants to merge 5 commits into from
Closed

feat: Implement optional additional principals #51

wants to merge 5 commits into from

Conversation

trallnag
Copy link

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:

  • Is the naming of the variable okay?
  • How to update docs? Manually?
  • Should I revert the terraform fmt changes?

@trallnag trallnag requested review from a team as code owners January 11, 2021 17:12
@trallnag trallnag requested review from adamcrews and SweetOps and removed request for a team January 11, 2021 17:12
@trallnag
Copy link
Author

==> 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

Tim Schwenke added 2 commits January 11, 2021 20:33
Signed-off-by: Tim Schwenke <tim.and.trallnag+code@gmail.com>
There seems to be a bug regarding using named iterators. So I replaced it with
the default named variable. See here:

<hashicorp/terraform#22340>

Signed-off-by: Tim Schwenke <tim.and.trallnag+code@gmail.com>
@Nuru
Copy link
Sponsor Contributor

Nuru commented Jan 19, 2021

==> 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
Copy link
Sponsor Contributor

Nuru commented Jan 19, 2021

@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
Copy link
Author

trallnag commented Jan 19, 2021

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
Copy link
Sponsor Contributor

Nuru commented Jan 19, 2021

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.

Tim Schwenke and others added 2 commits January 23, 2021 16:24
@trallnag trallnag requested a review from a team as a code owner January 23, 2021 15:33
@trallnag
Copy link
Author

@mergify
Copy link

mergify bot commented May 13, 2021

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

@Nuru Nuru mentioned this pull request Aug 29, 2021
@Nuru Nuru closed this in #84 Aug 30, 2021
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.

None yet

3 participants