Adding JSON configuration options for overwrites to bootstrap script

[Skarlso]

Closes https://github.com/awslabs/amazon-eks-ami/issues/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.

[Skarlso]

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]

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]

I suggested it, because:

1. 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"}'.
2. 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

3. 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 👎 .
4. 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]

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]

@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]

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]

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]

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]

@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]

@stevehipwell as written in https://github.com/weaveworks/eksctl/issues/4459#issuecomment-1055690876 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]

@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]

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]

@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]

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]

@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]

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

[stevehipwell]

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

CC @bwagner5

[Skarlso]

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

[bwagner5]

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]

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

[stevehipwell]

@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]

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

[cartermckinnon]

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]

@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]

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