-
-
Notifications
You must be signed in to change notification settings - Fork 128
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this review requests changes, I am not actually requesting that you make these changes. I am just posting this to explain why we are not going to accept this PR. We are already working on supporting these features and will have an update soon.
kubelet_extra_args = replace(join(" ", var.kubelet_extra_args), "'", ""), | ||
node_taints = replace(join(",", var.node_taints), "'", ""), | ||
node_labels = replace(join(",", local.node_labels),"'", "") |
There was a problem hiding this comment.
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?
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 | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
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) : "") |
There was a problem hiding this comment.
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?
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"] | ||
} | ||
} |
There was a problem hiding this comment.
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
?
# 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) |
There was a problem hiding this comment.
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.
|
||
# Execute the hook script before starting the services | ||
sed -i "/^systemctl daemon-reload\$/i . /etc/eks/extra_args.sh" /etc/eks/bootstrap.sh | ||
--// |
There was a problem hiding this comment.
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.
Content-Type: text/x-shellscript; charset="us-ascii" | ||
#!/bin/bash | ||
Content-Type: text/x-shellscript | ||
Content-Type: charset="us-ascii" |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
@xario Thank you for your contribution. We already have in the works something similar, but more along the lines of our other modules. I think your version is more complex and sophisticated than we need or want to support long term, so I am going to close this PR. Expect support for labels and taints in less than a week from now. |
What
These changes are in relation to the PR about launch configurations (#27). This PR is circumventing the problem with not being able to specify the
KUBELET_EXTRA_ARGS
in user_data when not specifyingami_id
, as noted in the PR which rolled back some of the changes (#28).As AWS appends a call to the
/etc/eks/bootstrap.sh
to the user_data, it is necessary to modify the bootstrap file to include the changes to the config. This is done by adding a hook right before the services are started, which allows for updates to the/etc/systemd/system/kubelet.service.d/30-kubelet-extra-args.conf
file.Here is an outline of what is happening the hook script:
30-kubelet-extra-args.conf
KUBELET_EXTRA_ARGS
into an associative arrayKUBELET_NEW_ARGS
into the arrayNEW_TAINTS
to the entry--register-with-taints
in the arrayNEW_LABELS
to the entry--node-labels
in the array30-kubelet-extra-args.conf
This preserve the arguments that the
bootstrap.sh
adds toKUBELET_EXTRA_ARGS
, while still being able to add user specified arguments.A minor change is that I had to make to the way the security groups are added to a node group, as I got an error
Remote access configuration cannot be specified with a launch template
. They are now added to the launch configuration by usingvpc_security_group_ids
. By specifying this, it overwrites the security group AWS adds normally, that allows for communication with the control plane, which is why I had to add a data source for theaws_eks_cluster
in order to retrieve the security group id. This change can be omitted in this PR and added as a separate PR if you like.Why
ami_id
.References
PS: I have not updated tests and documentation yet, as I wanted to get your reaction to the changes before continuing.
The changes have been tested with and without specifying an
ami_id
, and with and without specifying anec2_ssh_key
by creating clusters using Terraform in AWS.