Adding JSON configuration options for overwrites to bootstrap script #875

Skarlso · 2022-03-02T08:00:51Z

Closes #873

Description of changes:

Changes bootstrap.sh to accept a JSON kubelet config which merges it with the existing config after all changes have been applied by the bootstrap script.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

suket22

@cartermckinnon anything you want to add to this?

suket22 · 2022-05-31T23:16:58Z

files/bootstrap.sh

@@ -55,6 +56,11 @@ while [[ $# -gt 0 ]]; do
            shift
            shift
            ;;
+        --kubelet-extra-json-config)


You've defined the arg as --kubelet-extra-json-args in the helper on #L23. Can we instead have this as --kubelet-extra-json-config-file and take a pointer to a file instead of the file contents?

You probably also want to default KUBELET_EXTRA_JSON_CONFIG like on #L126, otherwise it might fail with this variable not being bound in the if -n check later on.

Hm, I'm not sure we can. I remember something about this, but we pass it in as definitive data. I don't think we can make it be on the instance when this is being executed.

Yep. This is incoming data. We can't make it point to a file which we can't make to exist.

Do you mean incoming as in supplied by the eksctl user? Also, why can't we make the file exist? You're calling the bootstrap script yourself in UserData so why not create the file and seed the contents in there?

My only nit is I'd call this --kubelet-extra-config/KUBELET_EXTRA_CONFIG. We don't need to mention the format of the config file here, and this should have the same semantics as --kubelet-extra-args.

Create the file with what content? :) To be more precise, yes this is data coming from an outside user. For example eksctl. They don't have access to the node or aren't running a custom AMI so all the entry they have is user data. eksctl is no longer constructing userdata. I know we could add things as a file, but for that to happen, we would have to again, mess with user data. This is just plain incoming Json extension of the kube config.

I guess I can overload this setting to also accept a file and then we assume the file exists somehow ( either by putting it there by hand in a custom AMI or making the UserData create the file ) and load the contents. It just feels like overloading a single flag to have multiple purpose is a bad idea. :D But that's just my IMHO. :)

eksctl is no longer constructing userdata

Maybe I'm misunderstanding, but isn't eksctl creating userData with /etc/eks/bootstrap.sh $args? I don't think I quite understand why eksctl cannot take in the json input for kubeletExtraConfig, put it in a file and then provide it to the bootstrap script.

If instead, it an eksctl customer that's fully authoring their UserData (including their bootstrap script), then I'd expect them to do -

cat <<EOF > /etc/eks/kubelet-custom-config.json { "kubeReserved": 240m } EOF /etc/eks/bootstrap.sh --kubelet-extra-config /etc/eks/kubelet-custom-config.json $otherArgs

I understand that the eksctl API accepts additional kubelet config as JSON / YAML input if I'm reading this right, but I don't think we need to force the bootstrap script's interface to also match that semantic.

It doesn't work, because anything we do just gets overwritten ultimately in this script. Whatever we do outside, the value just gets clobbered over with some defaults.

The reason we don't do the first part is that using kubelet-extra-config is cumbersome and overly complex for simple values and passing in some json data. :)

suket22 · 2022-05-31T23:26:11Z

files/bootstrap.sh

+if [[ -n "${KUBELET_EXTRA_JSON_CONFIG}" ]]; then
+    TMP_KUBE_CONF='/tmp/kubelet-conf-temp.json'
+    KUBELET_CONFIG_EXTRA='/tmp/kubelet-conf-extra.json'
+    echo '%s' > ${KUBELET_CONFIG_EXTRA}


If we take in a pointer to a file, could we change this entire section to something simpler like -

if [[ -n "${KUBELET_EXTRA_JSON_CONFIG}" ]]; then echo "bootstrap.sh: merging user json options into kubelet-config.json" jq -s '.[0] + .[1]' "${KUBELET_CONFIG}" "${KUBELET_EXTRA_JSON_CONFIG}" > $KUBELET_CONFIG fi

jq doesn't produce a partial result, so we shouldn't need to redirect to a temp file and then copy it over unless I'm missing something else :)

Yeah if I remember correctly, this indirection was added because of a different thing. But it was a couple months ago now, so I'll try to remember why.

This is incoming data that gets merged with the existing data.

suket22 · 2022-05-31T23:26:16Z

files/bootstrap.sh

@@ -20,6 +20,7 @@ function print_help {
    echo "--b64-cluster-ca The base64 encoded cluster CA content. Only valid when used with --apiserver-endpoint. Bypasses calling \"aws eks describe-cluster\""
    echo "--apiserver-endpoint The EKS cluster API Server endpoint. Only valid when used with --b64-cluster-ca. Bypasses calling \"aws eks describe-cluster\""
    echo "--kubelet-extra-args Extra arguments to add to the kubelet. Useful for adding labels or taints."
+    echo "--kubelet-extra-json-args Extra json configuration for kubelet. Userful for adding all kinds of extra configuration such as evictionHard or kubeReserved settings in JSON format."


Can we also add Will override any conflicting setting that the script defaults to.

cartermckinnon

I'd like to see this support extra config in a file, as @suket22 brought up. I'm not opposed to supporting JSON inline in addition to this.

We could call the option --kubelet-extra-config/KUBELET_EXTRA_CONFIG, and if the value isn't a valid file, treat it as inline JSON.

Skarlso · 2022-06-02T04:42:13Z

But that's a separate issue. This issue deals with the fact when there is extra input from the user to a node on which there are no extra files. Only incoming data that we would like to merge with kube config. I can add an extra flag that deals with that. I think making this value to be both needlessly complicates things including the parsing and detecting if it's a file or not. Not to mention overloading the naming and the purpose of a single flag. SRP and all that?

suket22 · 2022-06-02T15:51:28Z

Overloading a single arg to either be a file or actual content seems odd to me, and don't think we should go down the route.

cartermckinnon · 2022-06-02T17:33:47Z

I suggested it, because:

When you only want to set a parameter or two, like in @suket22 's example, it feels a little silly to throw that in a file instead of just doing --kubelet-extra-config '{"kubeReserved": "240m"}'.
The implementation is pretty simple:

if [[ -n "${KUBELET_EXTRA_CONFIG}" ]]; then
    echo "bootstrap.sh: merging user json options into kubelet-config.json"
    if [ ! -f "${KUBELET_EXTRA_CONFIG" ]
    then
        # value isn't a file, so treat it as JSON
        TEMP_FILE=$(mktemp)
        echo "$KUBELET_EXTRA_CONFIG" > $TEMP_FILE
        KUBELET_EXTRA_CONFIG=$TEMP_FILE
    fi
    jq -s '.[0] + .[1]' "${KUBELET_CONFIG}" "${KUBELET_EXTRA_CONFIG}" > $KUBELET_CONFIG
fi

We should only have one flag related to extra kubelet JSON config, i.e. we can't have one for JSON inline and one for a JSON file. We'd need to do a 3-way merge 👎 .
Accepting either content inline or content from a file isn't really novel. One example of this is curl's --data/-d flag. Another is the AWS CLI, which accepts a file for any parameter value (commonly used for flags like --filters; which also usually accept two different content formats inline -- JSON and some other "shorthand"). More info here

It's not a hill I need to die on, but it sounds like inline JSON was @Skarlso 's original goal here.

cartermckinnon · 2022-06-02T17:38:14Z

I'd like to get some more context on how eksctl intends to leverage this change. This PR is essentially moving this block into our bootstrap script. There's logic to create a JSON file containing a user's input here, though I'm not following how/when that's used.

Skarlso · 2022-06-02T18:04:42Z

@cartermckinnon Ah I might have misunderstood what you want. You would like to change an existing flag to either accept a file or a set of JSON data? That wouldn't be a problem actually.

How we would leverage it actually is by just providing a few things without them getting overwritten with a nice value which doesn't require us to pass in a convoluted thing where we have to pre-create files and edit everything.

We have existing code which translates and creates data based on custom YAML data which is then just directly forwarded to the bootstrap script.

apiVersion: eksctl.io/v1alpha5
kind: ClusterConfig

metadata:
  name: dev-cluster-1
  region: eu-north-1

nodeGroups:
  - name: ng-1
    instanceType: m5a.xlarge
    desiredCapacity: 1
    kubeletExtraConfig:
        kubeReserved:
            cpu: "300m"
            memory: "300Mi"
            ephemeral-storage: "1Gi"
        kubeReservedCgroup: "/kube-reserved"
        systemReserved:
            cpu: "300m"
            memory: "300Mi"
            ephemeral-storage: "1Gi"
        evictionHard:
            memory.available:  "200Mi"
            nodefs.available: "10%"
        featureGates:
            RotateKubeletServerCertificate: true # has to be enabled, otherwise it will be disabled

This is super easy and convenient for the users. We don't write this out into a file and try to merge them together with other data. We pass in our thing, and would pass this in which than uses the bash magic to create the extra kube config that then can be used to set things up. It's really slick and nice user flow IMHO.

Now, if I could pass this very same thing into kube-extra-config I'm okay with that too as long as everything gets applied properly. :)

Oh and it needs to happen after bootstrap is finished so things will be applied correctly.

Skarlso · 2022-06-02T18:15:50Z

I know how to implement it, that wasn't my problem with it. :D

if [[ -n "${KUBELET_EXTRA_CONFIG}" ]]; then

So you're thinking about a scenario where there is a custom AMI and the file has been already backed into it, I guess? ( Or UserData defined more than one file ). I can see that being a thing, however, I just would like to point out that just because others do something it doesn't make it a good thing. :D

But I can get behind that I guess.

suket22 · 2022-06-02T19:13:29Z

So you're thinking about a scenario where there is a custom AMI and the file has been already backed into it, I guess?

I was actually thinking of using this in the context of MIME multi-parts, where the first part (maybe user managed) populates the file and the second part uses this file. This could help folks override kubelet settings on Managed Nodes and Karpenter, without going the full custom AMI route.

We'd establish a runtime contract stating "always put your kubeletConfig in this path /etc/eks/config in your UserData section". This way we don't need to change the EKS API shape to include kubeletParameters, but rather just rely on folks using UserData in their launch templates.

Skarlso · 2022-06-02T19:38:42Z

I was actually thinking of using this in the context of MIME multi-parts, where the first part (maybe user managed) populates the file and the second part uses this file.

If the data can be passed in, sure. I don't see the point though since you can merge the data immediately. But maybe I'm missing something there. :)

The main point is, that incoming data trying to do anything will be overwritten with data defaulted in the bootstrap script. That's what we are trying to avoid / fix here.

stevehipwell · 2022-06-03T06:25:43Z

@suket22 doesn't #855 cover most of the use case here (I know it's not perfect)? Basically additional kubelet args can be set but you can't override values explicitly set in bootstrap.sh.

I'd think that the desired logic, however it is implemented, should be that values which bootstrap.sh sets explicitly need inputs to override if it's valid to do so and anything else should able to be appended. In addition to work with MNGs there needs to be a way to persist the user input after it's passed in as multi part data.

Skarlso · 2022-06-03T06:39:13Z

@stevehipwell as written in eksctl-io/eksctl#4459 (comment) we would like to avoid setting a gazillion specific arguments that are a maintenance nightmare and can change over time. Where as, passing in a stream of JSON data can be arbitrary in any regard and up to the user to define entirely.

stevehipwell · 2022-06-03T06:55:49Z

@Skarlso the only specific arguments or env values are to override bootstrap.sh explicitly set kubelet args, which need to integrate with the bootstrap logic. In your desired design the full context of the bootstrapping is lost from your modifications, so there will be more settings off limits especially for MNGs.

Skarlso · 2022-06-03T06:58:53Z

Sorry, is this an incomplete sentence? :D

the only specific arguments or env values are to override bootstrap.sh explicitly set kubelet args, which need to integrate with the bootstrap logic.

I can't really parse what you mean here.

In your desired design the full context of the bootstrapping is lost from your modifications, so there will be more settings off limits especially for MNGs.

What? :D Which context are you referring to?

stevehipwell · 2022-06-03T08:51:38Z

@Skarlso I don't think it's incomplete; if you're using bootstrap.sh you will need to provide variables to customise it's behaviour around the configuration it explicitly sets. For everything else you should be able to pass through your configuration, but you do need to know what can't be passed through. The context comes from the fact that kubelet args may, and often do, have corresponding changes required and from the fact that in the context of a MNG there are more constraints.

If done right bootstrapping should follow an open/closed design; you provide constrained input variables to configure the closed functionality and then extend on top of the result within the constraints allowed, which can be dynamically set based on the bootstrap inputs. E.g. disable default kube reserved values being calculated with a bootstrap variable and pass kube reserved values through (only allowed because of the bootstrap input).

Skarlso · 2022-06-03T09:04:14Z

ah, thanks for the longer explanation, that makes more sense now. Yes, the values are dependent, sure. And some things need to be calculated based on those values. But where is that requirement coming from? What I mean is, can it be left to the user saying, listen, of you set X you need to be sure you set Y properly for things to work otherwise, they might break and we don't take responsibility on that. If you know what I'm saying?

Otherwise, I agree that we should let things lying around with which the user can shoot themselves in the foot I guess. In which case, the argument based approach would make more sense because it gives more control on other calculations?

stevehipwell · 2022-06-03T09:15:33Z

@Skarlso longer term I think a binary to process a well defined configuration is the way forward (as mentioned in #855). My personal vote goes for the Bottlerocket configuration (or a subset) being processed by a Go binary specific to AL2 (potentially AL2022 too).

Skarlso · 2022-06-03T09:58:37Z

@stevehipwell For sure, but that's a massive step up from this small little option to define custom Kube config, right?

stevehipwell · 2022-06-03T12:10:27Z

@Skarlso it probably depends on the scope of the configuration supported, but it'd solve a number of hard problems.

CC @bwagner5

Skarlso · 2022-06-03T12:29:57Z

Hmmm. I'm not completely against doing it, if it would be beneficial :)

bwagner5 · 2022-06-03T14:24:37Z

I'm convinced a new bootstrap binary is needed and probably isn't too hard to implement. We need better input options into a bootstrap script and we need better testing. The bash script has gotten us pretty far, but a bootstrap script that uses the same or similar format to Bottlerocket would be optimal IMO. The configuration scope in a new bootstrap binary could probably be scoped down quite a bit at first. We could start by supporting mostly what the current bootstrap script supports but fix the mechanisms for passing input and the order of overrides / merges. We could also get rid of some older support that bootstrap script has, like the dockerd support.

There are other advantages to a new bootstrap script, for example, unifying the AL2, eventual AL2022, and Windows bootstrap processes into one project.

Skarlso · 2022-06-03T14:26:38Z

Heck, let's do it. I'm game. Do you have a ticket? Should I close this? What do you prefer?

stevehipwell · 2022-06-04T17:22:50Z

@bwagner5 with a limited initial config scope and init container support all use cases would be catered for. As more of the Bottlerocket config is supported the fewer use cases would need init containers.

I'd suggest that this binary is created in a new repo and that a future aim is to make the logic available as a package to be imported where required (e.g. Karpenter to replicate reserved logic).

I'd be keen to contribute to this effort.

Skarlso · 2022-06-07T10:12:00Z

So..... what happens to this now? :D

cartermckinnon · 2023-07-07T17:46:41Z

I'm going to close this, because the consensus is that a re-write of the bootstrap is the right time/way to make this change. My hope is that can land in/around AL2023 support.

bennesp · 2023-09-21T12:06:21Z

@stevehipwell @cartermckinnon do you know if something has being started in this context ? I would like to help but I don't know if something has changed in the last months

bryantbiggs · 2023-09-21T12:42:24Z

for kubelet specifically, I would recommend waiting for 1.28 and utilizing the new drop-in configuration directory https://kubernetes.io/docs/tasks/administer-cluster/kubelet-config-file/#kubelet-conf-d

This would allow the EKS AMI to set the required values and allow users to augment by writing the values they wish to set into a config file in that drop in directory. This should also play quite nicely with managed nodegroups where the EKS service is actually calling the bootstrap command which makes it difficult for users to pass additional flags

The one change we might need to make here is to add support for the --config-dir flag to make this process easier with managed nodegroups

Adding JSON configuration options for overwrites to bootstrap script

2d133c6

Skarlso mentioned this pull request Mar 2, 2022

Support kubeletExtraConfig in managed node groups eksctl-io/eksctl#4459

Closed

suket22 reviewed May 31, 2022

View reviewed changes

Skarlso added 2 commits June 1, 2022 13:41

Merge branch 'master' into allow-extra-kubelet-json-config

97f51d3

Adjusted naming and the description

cf75dc2

cartermckinnon requested changes Jun 1, 2022

View reviewed changes

suket22 mentioned this pull request Jul 5, 2022

[Bug] maxPodsPerNode does't work with eks 1.22 eksctl-io/eksctl#5134

Closed

cartermckinnon closed this Jul 7, 2023

cartermckinnon mentioned this pull request Jul 7, 2023

Support bootstrap env from file #855

Closed

bennesp mentioned this pull request Sep 21, 2023

How to add kubelet arguments or configuration to AL2 node group terraform-aws-modules/terraform-aws-eks#2709

Closed

Adding JSON configuration options for overwrites to bootstrap script #875

Adding JSON configuration options for overwrites to bootstrap script #875

Conversation

Skarlso commented Mar 2, 2022

suket22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skarlso Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cartermckinnon left a comment • edited Loading

Choose a reason for hiding this comment

Skarlso commented Jun 2, 2022

suket22 commented Jun 2, 2022

cartermckinnon commented Jun 2, 2022 • edited Loading

cartermckinnon commented Jun 2, 2022

Skarlso commented Jun 2, 2022 • edited Loading

Skarlso commented Jun 2, 2022 • edited Loading

suket22 commented Jun 2, 2022

Skarlso commented Jun 2, 2022

stevehipwell commented Jun 3, 2022

Skarlso commented Jun 3, 2022

stevehipwell commented Jun 3, 2022

Skarlso commented Jun 3, 2022

stevehipwell commented Jun 3, 2022

Skarlso commented Jun 3, 2022

stevehipwell commented Jun 3, 2022

Skarlso commented Jun 3, 2022

stevehipwell commented Jun 3, 2022

Skarlso commented Jun 3, 2022

bwagner5 commented Jun 3, 2022

Skarlso commented Jun 3, 2022

stevehipwell commented Jun 4, 2022

Skarlso commented Jun 7, 2022

cartermckinnon commented Jul 7, 2023

bennesp commented Sep 21, 2023

bryantbiggs commented Sep 21, 2023 • edited Loading

Skarlso Jun 2, 2022 •

edited

Loading

cartermckinnon left a comment •

edited

Loading

cartermckinnon commented Jun 2, 2022 •

edited

Loading

Skarlso commented Jun 2, 2022 •

edited

Loading

Skarlso commented Jun 2, 2022 •

edited

Loading

bryantbiggs commented Sep 21, 2023 •

edited

Loading