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.

Sign up for GitHub

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: Add security groups and policies to worker launcher template #933

Merged
merged 8 commits into from
Jul 12, 2020
Merged

feat: Add security groups and policies to worker launcher template #933

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a1f945f
feat: Add aws_security_group_rule.workers_egress_internet to output v…
mvaal Jun 26, 2020
73927c3
Changed name from cluster_workers_egress_internet to workers_egress_i…
mvaal Jun 26, 2020
c9e7189
Changed name from cluster_workers_egress_internet to workers_egress_i…
mvaal Jun 26, 2020
6695d24
Changed aws_autoscaling_group.workers to depend on SGs and policies
mvaal Jun 26, 2020
4992038
Add cluster SG rules to cluster dependencies
mvaal Jun 26, 2020
a84f4eb
Moved worker dependencies into aws_launch_configuration.workers
mvaal Jun 26, 2020
37ddfef
Duplicated worker dependencies into workers_launch_template aws_launc…
mvaal Jun 29, 2020
ff3dffb
Merge branch 'master' into master
mvaal Jul 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cluster.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ resource "aws_eks_cluster" "this" {
}

depends_on = [
aws_security_group_rule.cluster_egress_internet,
aws_security_group_rule.cluster_https_worker_ingress,
aws_iam_role_policy_attachment.cluster_AmazonEKSClusterPolicy,
aws_iam_role_policy_attachment.cluster_AmazonEKSServicePolicy,
aws_cloudwatch_log_group.this
Expand Down
22 changes: 19 additions & 3 deletions workers.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,22 @@ resource "aws_launch_configuration" "workers" {
lifecycle {
create_before_destroy = true
}

# Prevent premature access of security group roles and policies by pods that
# require permissions on create/destroy that depend on workers.
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
]
Comment on lines +262 to +274
Copy link
Contributor

Choose a reason for hiding this comment

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

Please duplicate this on the aws_launch_template in workers_launch_template.tf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
  ]
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know your thoughts on this when you have time, thanks!

}

resource "random_pet" "workers" {
Expand All @@ -271,14 +287,14 @@ resource "random_pet" "workers" {

resource "aws_security_group" "workers" {
count = var.worker_create_security_group && var.create_eks ? 1 : 0
name_prefix = aws_eks_cluster.this[0].name
name_prefix = var.cluster_name
description = "Security group for all nodes in the cluster."
vpc_id = var.vpc_id
tags = merge(
var.tags,
{
"Name" = "${aws_eks_cluster.this[0].name}-eks_worker_sg"
"kubernetes.io/cluster/${aws_eks_cluster.this[0].name}" = "owned"
"Name" = "${var.cluster_name}-eks_worker_sg"
"kubernetes.io/cluster/${var.cluster_name}" = "owned"
},
)
}
Expand Down