feat: Add security groups and policies to worker launcher template #933
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 tasks
mvaal
changed the title
Modified aws_security_group.workers to not depend on aws_eks_cluster.this as it would cause circular dependency. This should prevent SGs from being destroyed until after cluster is destroyed.
aws_autoscaling_groups.workers already depends on aws_launch_configuration.workers and aws_launch_configuration.workers is where the security group dependencies are set, makes the most sense here instead of at the ASG level.
dpiddockcmp
suggested changes
Jun 28, 2020
Comment on lines
+262
to
+274
| depends_on = [ | ||
| aws_security_group_rule.workers_egress_internet, | ||
| aws_security_group_rule.workers_ingress_self, | ||
| aws_security_group_rule.workers_ingress_cluster, | ||
| aws_security_group_rule.workers_ingress_cluster_kubelet, | ||
| aws_security_group_rule.workers_ingress_cluster_https, | ||
| aws_security_group_rule.workers_ingress_cluster_primary, | ||
| aws_security_group_rule.cluster_primary_ingress_workers, | ||
| aws_iam_role_policy_attachment.workers_AmazonEKSWorkerNodePolicy, | ||
| aws_iam_role_policy_attachment.workers_AmazonEKS_CNI_Policy, | ||
| aws_iam_role_policy_attachment.workers_AmazonEC2ContainerRegistryReadOnly, | ||
| aws_iam_role_policy_attachment.workers_additional_policies | ||
| ] |
There was a problem hiding this comment.
Please duplicate this on the aws_launch_template in workers_launch_template.tf
There was a problem hiding this comment.
I've made the change, however, now I am thinking it might be a better approach to do this rather than copy/pasting the code. This should also help cover future scenarios.
worker_security_group_id = var.worker_create_security_group ? null_resource.worker_security_group_setup.worker_security_group_id : var.worker_security_group_id
resource "null_resource" "worker_security_group_setup" {
worker_security_group_id = join("", aws_security_group.workers.*.id)
depends_on = [
aws_security_group_rule.workers_egress_internet,
aws_security_group_rule.workers_ingress_self,
aws_security_group_rule.workers_ingress_cluster,
aws_security_group_rule.workers_ingress_cluster_kubelet,
aws_security_group_rule.workers_ingress_cluster_https,
aws_security_group_rule.workers_ingress_cluster_primary,
aws_security_group_rule.cluster_primary_ingress_workers,
aws_iam_role_policy_attachment.workers_AmazonEKSWorkerNodePolicy,
aws_iam_role_policy_attachment.workers_AmazonEKS_CNI_Policy,
aws_iam_role_policy_attachment.workers_AmazonEC2ContainerRegistryReadOnly,
aws_iam_role_policy_attachment.workers_additional_policies
]
}
There was a problem hiding this comment.
Let me know your thoughts on this when you have time, thanks!
…h_template aws_launch_template also depends on worker_security_group_id
dpiddockcmp
approved these changes
Jul 12, 2020
1 task
barryib
pushed a commit
to Polyconseil/terraform-aws-eks
that referenced
this pull request
Oct 25, 2020
…roup rules and IAM policies (terraform-aws-modules#933) In order to ensure proper ordering when running terraform destroy. This will block Terraform from removing up security group rules before the cluster has finished its clean up chores.
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR o'clock
References #934
Add
workers_egress_internetsecurity group (and all other security groups and policies) as dependencies to the worker launch config.Description
In order to ensure proper ordering when running terraform destroy, I need to be able to establish an explicit dependency on the exposed resource because that resource allows pods to communicate with the control plane. Without being able to declare the dependency, terraform will likely destroy the rule early, which will prevent other resources from proper destruction. This makes the workers depend on the SGs and policies without exposing the internals.
Checklist