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

Added support for custom AMIs and node taints and other kubelet args #29

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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 examples/complete/fixtures.us-east-2.tfvars
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,5 @@ disk_size = 20
kubernetes_labels = {}

before_cluster_joining_userdata = "echo foo"

node_taints = ["dedicated=special:NoSchedule"]
1 change: 1 addition & 0 deletions examples/complete/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ module "eks_node_group" {
kubernetes_version = var.kubernetes_version
kubernetes_labels = var.kubernetes_labels
disk_size = var.disk_size
node_taints = var.node_taints

before_cluster_joining_userdata = var.before_cluster_joining_userdata

Expand Down
18 changes: 18 additions & 0 deletions examples/complete/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -116,3 +116,21 @@ variable "before_cluster_joining_userdata" {
default = ""
description = "Additional commands to execute on each worker node before joining the EKS cluster (before executing the `bootstrap.sh` script). For more info, see https://kubedex.com/90-days-of-aws-eks-in-production"
}

variable "ami_id" {
type = string
default = null
description = "Can be used to specify a custom Amazon Machine Image (AMI) id. If not specified the default AMI for the kubernetes version will be used"
}

variable "kubelet_extra_args" {
type = list(string)
default = []
description = "Can be used to specify additional kubelet arguments, other than taint and labels which are provided though other variables"
}

variable "node_taints" {
type = list(string)
default = []
description = "Can be used to specify node taints in the following format: key=value:effect, e.g: dedicated=special:NoSchedule"
}
63 changes: 51 additions & 12 deletions main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,22 @@ locals {
)
aws_policy_prefix = format("arn:%s:iam::aws:policy", join("", data.aws_partition.current.*.partition))

node_labels = [
for item in keys(var.kubernetes_labels):
join("=", [item, lookup(var.kubernetes_labels, item)])
]

userdata_vars = {
before_cluster_joining_userdata = var.before_cluster_joining_userdata
before_cluster_joining_userdata = var.before_cluster_joining_userdata,
kubelet_extra_args = replace(join(" ", var.kubelet_extra_args), "'", ""),
node_taints = replace(join(",", var.node_taints), "'", ""),
node_labels = replace(join(",", local.node_labels),"'", "")
Comment on lines +25 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not like the idea of blindly stripping quotes out of values. What if they have backslash escapes, for example?

}

userdata_ami_vars = {
cluster_name = var.cluster_name,
node_group_name = module.label.id,
ami_id = var.ami_id
}

# Use a custom launch_template if one was passed as an input
Expand Down Expand Up @@ -133,6 +147,8 @@ resource "aws_launch_template" "default" {
}
}

image_id = var.ami_id

instance_type = var.instance_types[0]

dynamic "tag_specifications" {
Expand All @@ -143,7 +159,38 @@ resource "aws_launch_template" "default" {
}
}

user_data = base64encode(templatefile("${path.module}/userdata.tpl", local.userdata_vars))
vpc_security_group_ids = concat(
[data.aws_eks_cluster.eks_cluster.vpc_config[0].cluster_security_group_id],
var.source_security_group_ids,
aws_security_group.remote_access_security_group.*.id
)
Comment on lines +162 to +166
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?


key_name = var.ec2_ssh_key

user_data = base64encode(
format("%s%s",
templatefile("${path.module}/userdata.tpl", local.userdata_vars),
(var.ami_id != null ? templatefile("${path.module}/userdata_ami.tpl", local.userdata_ami_vars) : "")
Comment on lines +171 to +173
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this 2 files concatenated rather than 1 file?

)
)
}

data "aws_eks_cluster" "eks_cluster" {
name = var.cluster_name
}

resource "aws_security_group" "remote_access_security_group" {
count = var.ec2_ssh_key != null && length(var.source_security_group_ids) == 0 ? 1 : 0
name = "eks-${var.cluster_name}-${module.label.id}-remote-access"
description = "Security group for all nodes in the nodeGroup to allow SSH access"
vpc_id = data.aws_eks_cluster.eks_cluster.vpc_config[0].vpc_id

ingress {
from_port = 22
to_port = 22
protocol = "tcp"
cidr_blocks = ["0.0.0.0/0"]
}
}
Comment on lines +182 to 194
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the advantage of this over the previous dynamic remote access block in aws_eks_node_group?


resource "aws_eks_node_group" "default" {
Expand All @@ -152,9 +199,9 @@ resource "aws_eks_node_group" "default" {
node_group_name = module.label.id
node_role_arn = join("", aws_iam_role.default.*.arn)
subnet_ids = var.subnet_ids
ami_type = var.ami_type
ami_type = var.ami_id == null ? var.ami_type : null
labels = var.kubernetes_labels
release_version = var.ami_release_version
release_version = var.ami_id == null ? var.kubernetes_version : null
version = var.kubernetes_version

tags = local.node_group_tags
Expand All @@ -170,14 +217,6 @@ resource "aws_eks_node_group" "default" {
version = local.launch_template.latest_version
}

dynamic "remote_access" {
for_each = var.ec2_ssh_key != null && var.ec2_ssh_key != "" ? ["true"] : []
content {
ec2_ssh_key = var.ec2_ssh_key
source_security_group_ids = var.source_security_group_ids
}
}

# Ensure that IAM Role permissions are created before and deleted after EKS Node Group handling.
# Otherwise, EKS will not be able to properly delete EC2 Instances and Elastic Network Interfaces.
depends_on = [
Expand Down
131 changes: 128 additions & 3 deletions userdata.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,134 @@ MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="//"

--//
Content-Type: text/x-shellscript; charset="us-ascii"
#!/bin/bash
Content-Type: text/x-shellscript
Content-Type: charset="us-ascii"
Comment on lines -5 to +6
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK you are not allowed to have multiple Content-Type headers. For sure I see no advantage to it.


${before_cluster_joining_userdata}

--//--
cat << 'EOF' > /etc/eks/extra_args.sh
#!/usr/bin/env bash
Copy link
Contributor

Choose a reason for hiding this comment

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

While arguably #!/usr/bin/env bash is more portable than #!/bin/bash it is less secure and if the AMI you are using does not have bash installed as /bin/bash then what are the chances that is it also using /etc/systemd/system/kubelet.service.d/30-kubelet-extra-args.conf? Not great. So I would leave this as #!/bin/bash

set -ex +u

function prepare_lines() {
local KUBELET_EXTRA_ARGS="$1"

# Break each argument into its own line, and replace the first '=' with '%' for easier processing later
# Example output:
# --register-with-taints%dedicated=test:NoSchedule
# --node-labels%eks.amazonaws.com/nodegroup=test-mng,other.label=other
#
local EXTRA_ARGS_LINES=$(echo "$KUBELET_EXTRA_ARGS" | xargs -d' ' -I{} bash -c 'echo "{}" | sed "0,/=/s//%/"')

# Remove blank lines
# Example output:
# --register-with-taints%dedicated=test:NoSchedule
# --node-labels%eks.amazonaws.com/nodegroup=test-mng,other.label=other
local EXTRA_ARGS_LINES=$(echo "$EXTRA_ARGS_LINES" | sed '/^$/d')

echo "$EXTRA_ARGS_LINES"
}

# 30-kubelet-extra-args.conf example:
# [Service]
# Environment='KUBELET_EXTRA_ARGS=--register-with-taints="dedicated=test:NoSchedule" --node-labels=eks.amazonaws.com/nodegroup=test-mng,other.label=other'

# Get the last line and cut out the part between the single quotes
# Example output:
# KUBELET_EXTRA_ARGS=--register-with-taints="dedicated=test:NoSchedule" --node-labels=eks.amazonaws.com/nodegroup=test-mng,other.label=other
ENVIRONMENT=$(tail -n1 /etc/systemd/system/kubelet.service.d/30-kubelet-extra-args.conf | cut -d\' -f2)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is brittle, in that it requires that the last line of the file be desired line and will break if/when the default changes.


# Get the value of KUBELET_EXTRA_ARGS
# Example output:
# --register-with-taints="dedicated=test:NoSchedule" --node-labels=eks.amazonaws.com/nodegroup=test-mng,other.label=other
KUBELET_EXTRA_ARGS=$(echo "$ENVIRONMENT" | cut -d= -f 2-)

# Declare an associate array for later use
declare -A EXTRA_ARGS=()

# Loop over each of the lines and add each key value pair to the associate array
# Example output:
# [--node-labels]="eks.amazonaws.com/nodegroup=test-mng,other.label=other"
# [--register-with-taints]="dedicated=test:NoSchedule"
while IFS=% read -r key value; do
EXTRA_ARGS[$key]="$value"
done < <(prepare_lines "$KUBELET_EXTRA_ARGS")

# Additional args from Terraform, separated by spaces
# Example:
# --log-file=/tmp/kubelet.log
KUBELET_NEW_ARGS='${kubelet_extra_args}'

if [[ -n "$KUBELET_NEW_ARGS" ]]; then
# Declare an associate array for later use
declare -A NEW_EXTRA_ARGS=()

# Loop over each of the lines and add each key value pair to the associate array
# Example output:
# [--log-file]="/tmp/kubelet.log"
# [--hostname-override]="dedicated-node-az1"
while IFS=% read -r key value; do
NEW_EXTRA_ARGS[$key]="$value"
done < <(prepare_lines "$KUBELET_NEW_ARGS")

for KEY in "$${!NEW_EXTRA_ARGS[@]}"; do
# If there is existing values, add a comma before adding the new values
test -n "$${EXTRA_ARGS[$KEY]}" && EXTRA_ARGS[$KEY]+=","
EXTRA_ARGS[$KEY]+="$${NEW_EXTRA_ARGS[$KEY]}"
done
fi

# Additional taints from Terraform, separated by commas
# Example:
# dedicated=new-test:NoSchedule
NEW_TAINTS='${node_taints}'

# Additional labels from Terraform, separated by commas
# Example:
# new.label=new
NEW_LABELS='${node_labels}'

# Add the new taints to the associate array, if defined
# Example output:
# [--node-labels]="eks.amazonaws.com/nodegroup=test-mng,other.label=other"
# [--register-with-taints]="dedicated=test:NoSchedule,dedicated=new-test:NoSchedule"
if [[ -n "$NEW_TAINTS" ]]; then
# If there is existing taints, add a comma before adding the new taints
test -n "$${EXTRA_ARGS['--register-with-taints']}" && EXTRA_ARGS['--register-with-taints']+=","
EXTRA_ARGS['--register-with-taints']+="$NEW_TAINTS"
fi

# Add the new labels to the associate array, if defined
# Example output:
# [--node-labels]="eks.amazonaws.com/nodegroup=test-mng,other.label=other,new.label=new"
# [--register-with-taints]="dedicated=test:NoSchedule,dedicated=new-test:NoSchedule"
if [[ -n "$NEW_LABELS" ]]; then
# If there is existing labels, add a comma before adding the new labels
test -n "$${EXTRA_ARGS['--node-labels']}" && EXTRA_ARGS['--node-labels']+=","
EXTRA_ARGS['--node-labels']+="$NEW_LABELS"
fi

# Build string from the joined arguments to add to the output file
# Example:
# --log-file=/tmp/kubelet.log --hostname-override=dedicated-node-az1 --node-labels=eks.amazonaws.com/nodegroup=test-mng,other.label=other,new.label=new --register-with-taints=dedicated=test:NoSchedule,dedicated=new-test:NoSchedule
NEW_KUBELET_EXTRA_ARGS=""
for i in "$${!EXTRA_ARGS[@]}";do
NEW_KUBELET_EXTRA_ARGS+=$(printf "%s=%s " "$i" "$${EXTRA_ARGS[$i]}")
done

# Remove the trailing space
NEW_KUBELET_EXTRA_ARGS=$(echo "$NEW_KUBELET_EXTRA_ARGS" | xargs)

# Write the final output file
# Example:
# [Service]
# Environment='KUBELET_EXTRA_ARGS=--log-file=/tmp/kubelet.log --hostname-override=dedicated-node-az1 --node-labels=eks.amazonaws.com/nodegroup=test-mng,other.label=other,new.label=new --register-with-taints=dedicated=test:NoSchedule,dedicated=new-test:NoSchedule'
cat <<CONF > /etc/systemd/system/kubelet.service.d/30-kubelet-extra-args.conf
[Service]
Environment='KUBELET_EXTRA_ARGS=$${NEW_KUBELET_EXTRA_ARGS}'
CONF
EOF

# Execute the hook script before starting the services
sed -i "/^systemctl daemon-reload\$/i . /etc/eks/extra_args.sh" /etc/eks/bootstrap.sh
--//
Copy link
Contributor

Choose a reason for hiding this comment

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

While I applaud the effort to be comprehensive, this still seems to me a lot more complicated and brittle than it needs to be. I do not want to be supporting this a year from now.

5 changes: 5 additions & 0 deletions userdata_ami.tpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Content-Type: text/x-shellscript
Content-Type: charset="us-ascii"

/etc/eks/bootstrap.sh ${cluster_name} --kubelet-extra-args "--node-labels=eks.amazonaws.com/nodegroup=${node_group_name},eks.amazonaws.com/nodegroup-image=${ami_id}"
--//
18 changes: 18 additions & 0 deletions variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -119,3 +119,21 @@ variable "before_cluster_joining_userdata" {
default = ""
description = "Additional commands to execute on each worker node before joining the EKS cluster (before executing the `bootstrap.sh` script). For more info, see https://kubedex.com/90-days-of-aws-eks-in-production"
}

variable "ami_id" {
type = string
default = null
description = "Can be used to specify a custom Amazon Machine Image (AMI) id. If not specified the default AMI for the kubernetes version will be used"
}

variable "kubelet_extra_args" {
type = list(string)
default = []
description = "Can be used to specify additional kubelet arguments, other than taint and labels which are provided though other variables"
}

variable "node_taints" {
type = list(string)
default = []
description = "Can be used to specify node taints in the following format: key=value:effect, e.g: dedicated=special:NoSchedule"
}