Added support for custom AMIs and node taints and other kubelet args #29
Conversation
xario
requested review from
jhosteny and
Gowiem
and removed request for
a team
| 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.
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 | ||
| ) |
| 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.
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.
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.
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.
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.
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.
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_ARGSin 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.shto 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.conffile.Here is an outline of what is happening the hook script:
30-kubelet-extra-args.confKUBELET_EXTRA_ARGSinto an associative arrayKUBELET_NEW_ARGSinto the arrayNEW_TAINTSto the entry--register-with-taintsin the arrayNEW_LABELSto the entry--node-labelsin the array30-kubelet-extra-args.confThis preserve the arguments that the
bootstrap.shadds 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_clusterin 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_keyby creating clusters using Terraform in AWS.