support KubeletConfiguration and flag passthrough in NodeConfig #1593
Conversation
|
Chatted offline, I think we have to find a more generic way to accept kubelet config customizations. Clients like Karpenter want to pass a defined subset of the options, but clients like We shouldn't need to make a code change here for every kubelet config option that every user wants to modify, it's toil for the maintainers and an artificial limitation for users. I don't think we can do any meaningful validation of these options; when |
I'm curious to interrogate this assumption. I think that there's value in knowing how customers are using your API, and intentionally constraining things. I think we can pretty trivially fine examples of kubeletconfiguration that would never be used. |
I think this data is valuable, but I think asking folks to cut an issue/PR here, get it merged, and wait for a release is a lot of friction just to permit them to set a field in the kubelet config file. If it were me, I'd just hack around it myself.
If the knob exists, someone wants (or perhaps less often, needs) to turn it. 😉 |
ndbaker1
force-pushed
the
kubeletconfig-ext
branch
4 times, most recently
from
532b88c
to
8cdc11e
Compare
ndbaker1
changed the title
ndbaker1
force-pushed
the
kubeletconfig-ext
branch
from
8cdc11e
to
74cbc67
Compare
ndbaker1
force-pushed
the
kubeletconfig-ext
branch
from
74cbc67
to
e284eab
Compare
For the Karpenter use case, I think this makes sense because there's some inherent expectations that you can make. However, in the case of these AMIs as a whole, the use cases are pretty vast. I don't think that we can realistically decide what parameters are and are not useful to customers without just allowing a majority of them to be set regardless, and this code will be significantly less maintainable because any new (or unsupported) config that needs to be added will require a PR. Given, may be a minor PR, but will still need to be cut and then released. Some people will just hope to hack their user data with I like the idea of making it easy for customers to configure the fields they want and easily adopt new config options. |
| assert::file-contains /etc/eks/kubelet/environment "--register-with-taints=foo=:NoSchedule,foo2=baz:NoSchedule" | ||
|
|
||
| mock::kubelet 1.28.0 | ||
| nodeadm init --skip run --config-source file://config.yaml |
There was a problem hiding this comment.
Would be sweet if we could do
cat <<EOF | nodeadm init -f -
... inline file
EOF
There was a problem hiding this comment.
most users wont have a need to actually call nodeadm based on how its configured to boot in the ami, but im a fan of the style
ndbaker1
force-pushed
the
kubeletconfig-ext
branch
from
702b52d
to
48c4bb8
Compare
| name: my-cluster | ||
| apiServerEndpoint: https://example.com | ||
| certificateAuthority: Y2VydGlmaWNhdGVBdXRob3JpdHk= | ||
| cidr: 10.100.0.0/16 |
There was a problem hiding this comment.
Does this cause NodeConfig to inject kubelet flags? What if they conflict with user defined flags?
There was a problem hiding this comment.
carter just answered this one while we determined the input style; we will generate flags in nodeadm as a string and then user-provided flags can override any of these
ndbaker1
force-pushed
the
kubeletconfig-ext
branch
from
dc8e722
to
35a1321
Compare
| // InlineConfig is a raw document of a kubelet config that can be provided | ||
| // by the user to override default generated configurations | ||
| // https://kubernetes.io/docs/reference/config-api/kubelet-config.v1/ | ||
| InlineConfig string `json:"inline,omitempty"` |
There was a problem hiding this comment.
JSON key should match the struct field name here, and I think we should use extraConfig or just config
There was a problem hiding this comment.
im going with config for the brevity 😬
| @@ -130,17 +127,6 @@ func enrichConfig(log *zap.Logger, cfg *api.NodeConfig) error { | |||
| } | |||
| cfg.Status.Instance = *instanceDetails | |||
| log.Info("Instance details populated", zap.Reflect("details", instanceDetails)) | |||
|
|
|||
| if featuregates.DefaultFalse(featuregates.DescribeClusterDetails, cfg.Spec.FeatureGates) { | |||
There was a problem hiding this comment.
Try to isolate changes like this in their own PR going forward 😉
Issue #, if available:
Description of changes:
supporting KubeletConfiguration parameters through the
NodeConfigas pass through as well as the kubelet cli flags.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Testing Done
added e2e tests to cover the additional configuration surface area
See this guide for recommended testing for PRs. Some tests may not apply. Completing tests and providing additional validation steps are not required, but it is recommended and may reduce review time and time to merge.