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

Ability to add additional security groups to a node group #66

Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@

# Cloud Posse must review any changes to standard context definition,
# but some changes can be rubber-stamped.
**/*.tf @cloudposse/engineering @cloudposse/approvers
README.yaml @cloudposse/engineering @cloudposse/approvers
**/*.tf @cloudposse/engineering @cloudposse/contributors @cloudposse/approvers
README.yaml @cloudposse/engineering @cloudposse/contributors @cloudposse/approvers
README.md @cloudposse/engineering @cloudposse/contributors @cloudposse/approvers
docs/*.md @cloudposse/engineering @cloudposse/contributors @cloudposse/approvers

Expand Down
2 changes: 1 addition & 1 deletion .github/auto-release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ template: |

replacers:
# Remove irrelevant information from Renovate bot
- search: '/---\s+^#.*Renovate configuration(?:.|\n)*?This PR has been generated .*/gm'
- search: '/(?<=---\s+)+^#.*(Renovate configuration|Configuration)(?:.|\n)*?This PR has been generated .*/gm'
replace: ''
# Remove Renovate bot banner image
- search: '/\[!\[[^\]]*Renovate\][^\]]*\](\([^)]*\))?\s*\n+/gm'
Expand Down
4 changes: 3 additions & 1 deletion .github/workflows/auto-release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ name: auto-release
on:
push:
branches:
- main
- master
- production

jobs:
publish:
Expand All @@ -14,7 +16,7 @@ jobs:
id: get-merged-pull-request
with:
github_token: ${{ secrets.PUBLIC_REPO_ACCESS_TOKEN }}
# Drafts your next Release notes as Pull Requests are merged into "master"
# Drafts your next Release notes as Pull Requests are merged into "main"
- uses: release-drafter/release-drafter@v5
if: "!contains(steps.get-merged-pull-request.outputs.labels, 'no-release')"
with:
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/validate-codeowners.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
name: Validate Codeowners
on:
workflow_dispatch:

pull_request:

jobs:
Expand Down
183 changes: 95 additions & 88 deletions README.md

Large diffs are not rendered by default.

176 changes: 92 additions & 84 deletions docs/terraform.md

Large diffs are not rendered by default.

7 changes: 5 additions & 2 deletions launch-template.tf
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,11 @@ locals {
launch_template_ami = length(local.configured_ami_image_id) == 0 ? (local.features_require_ami ? data.aws_ami.selected[0].image_id : "") : local.configured_ami_image_id

launch_template_vpc_security_group_ids = (
local.need_remote_access_sg ?
concat(data.aws_eks_cluster.this[0].vpc_config[*].cluster_security_group_id, aws_security_group.remote_access.*.id) : []
concat(
local.ng.additional_security_group_ids,
local.get_cluster_data ? data.aws_eks_cluster.this[0].vpc_config[*].cluster_security_group_id : [],
local.need_remote_access_sg ? aws_security_group.remote_access.*.id : []
)
)

# launch_template_key = join(":", coalescelist(local.launch_template_vpc_security_group_ids, ["closed"]))
Expand Down
8 changes: 5 additions & 3 deletions main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@ locals {
configured_ami_image_id = var.ami_image_id == null ? "" : var.ami_image_id
need_ami_id = local.enabled ? local.features_require_ami && length(local.configured_ami_image_id) == 0 : false
need_imds_settings = var.metadata_http_endpoint != "enabled" || var.metadata_http_put_response_hop_limit != 1 || var.metadata_http_tokens != "optional"
features_require_launch_template = local.enabled ? length(var.resources_to_tag) > 0 || local.need_userdata || local.features_require_ami || local.need_imds_settings : false
features_require_launch_template = local.enabled ? length(var.resources_to_tag) > 0 || local.need_userdata || local.features_require_ami || local.need_imds_settings || length(var.additional_security_group_ids) : false
nustiueudinastea marked this conversation as resolved.
Show resolved Hide resolved

have_ssh_key = var.ec2_ssh_key != null && var.ec2_ssh_key != ""

need_remote_access_sg = local.enabled && local.have_ssh_key && local.generate_launch_template

get_cluster_data = local.enabled ? (local.need_cluster_kubernetes_version || local.need_bootstrap || local.need_remote_access_sg) : false
get_cluster_data = local.enabled ? (local.need_cluster_kubernetes_version || local.need_bootstrap || local.need_remote_access_sg || length(var.additional_security_group_ids) > 0) : false
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Not needed. get_cluster_date is true only if we need to invoke a data provider to get more information about the cluster.

Suggested change
get_cluster_data = local.enabled ? (local.need_cluster_kubernetes_version || local.need_bootstrap || local.need_remote_access_sg || length(var.additional_security_group_ids) > 0) : false
get_cluster_data = local.enabled ? (local.need_cluster_kubernetes_version || local.need_bootstrap || local.need_remote_access_sg) : false


autoscaler_enabled = var.enable_cluster_autoscaler != null ? var.enable_cluster_autoscaler : var.cluster_autoscaler_enabled == true
#
Expand Down Expand Up @@ -86,6 +86,7 @@ locals {
ec2_ssh_key = local.have_ssh_key ? var.ec2_ssh_key : "none"
# Keep sorted so that change in order does not trigger replacement via random_pet
source_security_group_ids = local.ng_needs_remote_access ? sort(var.source_security_group_ids) : []
additional_security_group_ids = sort(var.additional_security_group_ids)
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

The ng object is just for data that is needed for the aws_eks_node_group resource. Since additional_security_group_ids only affects the launch template, this should not be here.

}
}

Expand Down Expand Up @@ -114,7 +115,8 @@ resource "random_pet" "cbd" {
# actually track security groups by using
# source_security_group_ids = join(",", local.ng.source_security_group_ids, aws_security_group.remote_access.*.id)
#
source_security_group_ids = local.need_remote_access_sg ? "generated for launch template" : join(",", local.ng.source_security_group_ids)
source_security_group_ids = local.need_remote_access_sg ? "generated for launch template" : join(",", local.ng.source_security_group_ids)
additional_security_group_ids = join(",", local.ng.additional_security_group_ids)

launch_template_id = local.use_launch_template ? local.launch_template_id : "none"
}
Expand Down
6 changes: 6 additions & 0 deletions variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ variable "source_security_group_ids" {
description = "Set of EC2 Security Group IDs to allow SSH access (port 22) to the worker nodes. If you specify `ec2_ssh_key`, but do not specify this configuration when you create an EKS Node Group, port 22 on the worker nodes is opened to the Internet (0.0.0.0/0)"
}

variable "additional_security_group_ids" {
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

We have a new naming convention. Please rename this associated_security_group_ids

Suggested change
variable "additional_security_group_ids" {
variable "associated_security_group_ids" {

type = list(string)
default = []
description = "Set of additional EC2 Security Group IDs that will be associated with the EKS Node Group"
}

variable "desired_size" {
type = number
description = "Initial desired number of worker nodes (external changes ignored)"
Expand Down