using double quotes instead of single quotes for k3s_exec args

[schlichtanders]

This enables the use of shell features inside the k3s_exec_server_args and k3s_exec_agent_args.

One example usecase is the automatic tainting of a node if it cannot connect to the standard image registries.

Previous discussions:

• Automatically apply taint during node self-registering if IP is blacklisted #839
• [Feature] Add the ability to set custom k3s agent args #862

Discussion

Advantage:

• double quotes are more powerful than single quotes
• no need to escape single quotes (Terraform strings use double quotes, so that inside these the usage of single quotes is much easier than double quotes, which need to be quoted)

Downside:

• double quotes now need to be quoted, which should be something like \\\" in terraform or

<<-EOF
\"whatever should be double quoted\"
EOF

(a bit complex, but the EOF wayaround seems okay enough)

• in this sense the change is breaking, however probably only a tiny fraction is really using inner quotes, if at all

Alternatives considered

• I tried other ways of filling variables with content without using quotes at all, but the edgecase where too numerous and hard to follow. I think it is best to decide for double quotes.

• One could also make the quotechar configurable. I tried this, but the resulting terraform code looks hard to understand and only a tiny few will ever use this. Hence I argue for changing to double quotes straight away.

[schlichtanders]

@aleksasiriski @mysticaltech could you review this tiny change?

[mysticaltech]

Hey @schlichtanders, thanks for this, will do ASAP tonight.

What do you think about upgrade scenarios?

Currently, we have a pending feature request to make changing kube.tf values update the k3s config.yaml. Currently this is not done, but we do something similar for k3s_registries.yaml so it's doable in a robust way. It may need to be achieved before we merge this, as to allow for upgradability without having to edit the config.yaml manually in each node.

[schlichtanders]

Thank you for the comment. That is actually super valuable.

I think having a user configurable way to change the k3s config.yaml flexibly is better than this pullrequest.
The reason I would prefer changing config.yaml is because of this k3s documentation:

It is also possible to use both a configuration file and CLI arguments. In these situations, values will be loaded from both sources, but CLI arguments will take precedence. For repeatable arguments such as --node-label, the CLI arguments will overwrite all values in the list.
https://docs.k3s.io/installation/configuration#configuration-file

So apparently, to change a kubelet or label or taint with args is super complicated and error prone because people would need to copy those from the config.yaml anyway.

Hence given this new state, I am actually rather for closing this one and go for the configurability of the k3s config.yaml instead. Which other pullrequest is it?

[schlichtanders]

Just found that k3s actually supports multiple config files which can overwrite each other.
May be useful for the other approach https://docs.k3s.io/installation/configuration

[mysticaltech]

@schlichtanders What I was reffering to was this issue #850, but yes, it's super interesting that multiple config.yaml files are supported. However, I think this PR is good, but I just need to implement the config.yaml update feature first to make upgrades to the new version possible without requiring manual edits of the file (that will get done automatically, so basically no need to play with many files). To see how that will work, you can take a look at the k3s_registries.yaml logic, it involves some bash scripting to make sure the logic is robust.

[mysticaltech]

@schlichtanders Just for clarity, will try to merge this on the week-end after I have shipped the pre-required feature as described above.

[schlichtanders]

I haven't needed this for the restoration.

Still I want to check in, because it would be great if this could either be closed with a reason, or merged.

[mysticaltech]

@schlichtanders Most definitely we will merge it soon. I'm just waiting to have some hours to focus on this, as it's pending a feature request as explained above. This is coming, I hope this week if I can manage.

[mysticaltech]

@schlichtanders Is this no longer needed, the k3s dynamic update after first launch was just merged. But I see 0 change here. Please add back a new PR if needed.