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

Remove of autoscaling IAM policy related stuff #716

Merged
merged 10 commits into from
Feb 4, 2020
Merged

Remove of autoscaling IAM policy related stuff #716

merged 10 commits into from
Feb 4, 2020

Conversation

max-rocket-internet
Copy link
Contributor

@max-rocket-internet max-rocket-internet commented Jan 28, 2020

PR o'clock

Description

I think we want to remove the cluster-autoscaler policy from the module. One reason is to reduce complexity but also now that IRSA is GA for EKS, that makes a much more elegant solution.

Checklist

@max-rocket-internet
Copy link
Contributor Author

@barryib or we just remove the whole lot now? Otherwise it's 2 breaking changes, one setting defaults to false and then another removing the vars and policy?

@barryib
Copy link
Member

barryib commented Jan 28, 2020

Can you please update your branch and upgrade to terraform-docs 0.8.1 ?

@barryib
Copy link
Member

barryib commented Jan 28, 2020

I think we can remove it and hold the merge for #710

@max-rocket-internet max-rocket-internet changed the title Disable management of autoscaling IAM policy by default Remove of autoscaling IAM policy related stuff Jan 29, 2020
@max-rocket-internet
Copy link
Contributor Author

OK I think we should just remove it completely in one PR to avoid 2 breaking changes. I've written notes on the release with instructions.

examples/irsa/main.tf Outdated Show resolved Hide resolved
workers.tf Show resolved Hide resolved
Copy link
Member

@barryib barryib left a comment

Choose a reason for hiding this comment

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

Thanks @max-rocket-internet. Awesome as usual.

@barryib barryib merged commit 626a393 into terraform-aws-modules:master Feb 4, 2020
@max-rocket-internet max-rocket-internet deleted the no_ca_policy branch February 5, 2020 10:52
@sc250024
Copy link
Contributor

sc250024 commented Mar 6, 2020

@max-rocket-internet Not to bring up an already merged PR, but why were the ASG tags removed? From your description:

One reason is to reduce complexity but also now that IRSA is GA for EKS, that makes a much more elegant solution.

I understand removing the IAM stuff for complexity, but why does IRSA justify removing the tags from the ASGs? AFAIK, they're still needed for Cluster Autoscaler, no matter what mechanism the pods are using to authenticate with AWS. Did something change in that regard? How would a person keep them if they upgrade to v9.0.0 ?

@max-rocket-internet
Copy link
Contributor Author

Hey @sc250024

why does IRSA justify removing the tags from the ASGs

The cluster-autoscaler needs some IAM policy and some tags on the ASGs.

Because we stopped managing the IAM policy (because IRSA is better for this) then I think it makes sense to move the cluster-autoscaler tags to the normal method that we already have for tagging ASGs: using worker_groups[]. tags e.g. here

Does that make sense?

@max-rocket-internet
Copy link
Contributor Author

How would a person keep them if they upgrade to v9.0.0 ?

Just add this to your worker group: https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/examples/irsa/main.tf#L73-L83

@sc250024
Copy link
Contributor

sc250024 commented Mar 6, 2020

@max-rocket-internet Got it. I was already using IRSA for Cluster Autoscaler before v9.0.0, so I was already creating the IAM policy myself in another way. However, I was using the ASG tags.

Anyway, what you mentioned makes sense now. Thank you!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants