fix: Put KubeletExtraArgs in double quotes for Windows

[yveslaroche]

PR o'clock

Description

Put KubeletExtraArgs in double quotes for Windows, it is already double quoted in the Bash script. This allows for variable substitution within the string e.g.

kubelet_extra_args = "node.kubernetes.io/lifecycle=$(Invoke-WebRequest -Uri http://169.254.169.254/latest/meta-data/instance-life-cycle | Select-Object -ExpandProperty Content)"

Checklist

• CI tests are passing
• README.md has been updated after any changes to variables and outputs. See https://github.com/terraform-aws-modules/terraform-aws-eks/#doc-generation

[bogdan-alov]

LGTM

[barryib]

Is this different from #1046 ? Because the double quote in linux introduce a bug for multiple taints support. See FAQ update for variable substitution in linux. Does it work for windows ?

See also #1004

[barryib]

/ping @yveslaroche and @bogdanalov-sw

[yveslaroche]

I have successfully tested applying multiple labels and taints for both Linux and Windows nodes using double quotes in the bootstrap scripts with the examples below.

# Linux
kubelet_extra_args = "--node-labels=node.kubernetes.io/role=worker,node.kubernetes.io/lifecycle=$(curl -s http://169.254.169.254/latest/meta-data/instance-life-cycle) --register-with-taints=somekey=somevalue:NoSchedule,anotherkey=anothervalue:NoSchedule"

# Windows
kubelet_extra_args = "--node-labels=node.kubernetes.io/role=windows,node.kubernetes.io/lifecycle=$(Invoke-WebRequest -Uri http://169.254.169.254/latest/meta-data/instance-life-cycle | Select-Object -ExpandProperty Content) --register-with-taints=os=windows:NoSchedule,anotherkey=anothervalue:NoSchedule"

In addition, I tested the taints applied in #1004. In this case the node won't register, but if I change the name of one of the keys to something else, it works. Hence, I believe the problem is applying two taints with the same key and effect, but different values.

[barryib]

By reading this awslabs/amazon-eks-ami#179 it sounds like we should use '' (single quotes) to surround kubelet_extra_args and "" (double quotes) to surround arguments.

Something like: '--register-with-taints="os=windows:NoSchedule,anotherkey=anothervalue:NoSchedule"'

I haven’t test it yet, but I'm going to dig a bit.

When we use single quotes for KubeletExtraAgrs, in linux, you should escape variable:

$ echo 'my name is ${USER}'
my name is ${USER}

$ echo 'my name is '${USER}''
my name is foo

In addition, I tested the taints applied in #1004. In this case the node won't register, but if I change the name of one of the keys to something else, it works. Hence, I believe the problem is applying two taints with the same key and effect, but different values.

I think you can normally provide multiple taints with the same key (by example key1=value1:NoSchedule and key1=value1:NoExecute) https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/

[yveslaroche]

@barryib

I think you can normally provide multiple taints with the same key (by example key1=value1:NoSchedule and key1=value1:NoExecute)
Yes, but that's a different effect that's set which is allowed. Setting the same key and effect, but with a different value multiple times is not allowed. If you apply it manually you'd get:

error: node ip-xx-xx-xx-xx.us-west-2.compute.internal already has env taint(s) with same effect(s)

[barryib]

@barryib

I think you can normally provide multiple taints with the same key (by example key1=value1:NoSchedule and key1=value1:NoExecute)
Yes, but that's a different effect that's set which is allowed. Setting the same key and effect, but with a different value multiple times is not allowed. If you apply it manually you'd get:

error: node ip-xx-xx-xx-xx.us-west-2.compute.internal already has env taint(s) with same effect(s)

Sorry for the typo. I was thinking about key1=value1:NoSchedule and key1=value2:NoSchedule. I'll make some tests also.

[barryib]

@yveslaroche just made some tests and I can confirm your point.

[stale]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
To track this PR (even if closed), please open a corresponding issue if one does not already exist.

[stale]

This PR has been automatically closed because it has not had recent activity since being marked as stale.
Please reopen when work resumes.

[sebas-w]

Are we able to reopen this? What additional items were needed to get this merged? @barryib

[barryib]

I don't use windows at all, so the point is how this different from #1082 (comment) ? There is already a PR to put back single quote for linux workers #1046.

I need people to test this.

[stale]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
To track this PR (even if closed), please open a corresponding issue if one does not already exist.

[antonbabenko]

Thanks @yveslaroche !

v17.8.0 has been just released.

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.