handling of extraArgs which are allowed multiple times

[ghouscht]

The current implementation of extraArgs in the ControlPlaneComponent is backed by a map[string]string type and this leads to some problems we're currently facing. For example the api-server allows some arguments multiple times (--service-account-key-file and --tls-sni-cert-key). We can't handle this with the extraArgs type as it is a map which of course does not allow the same key twice and thus overrides previous declarations.

Example:

---
apiVersion: kubeadm.k8s.io/v1beta1
kind: ClusterConfiguration
kubernetesVersion: 1.14.2
apiServer:
  extraArgs:
    "tls-sni-cert-key": "/etc/kubernetes/pki/apiserver.crt,/etc/kubernetes/pki/apiserver.key"
    "tls-sni-cert-key": "/etc/kubernetes/pki/mycert.crt,/etc/kubernetes/pki/mykey.key" # overrides the previous declaration

(This is probably also a problem for other control plane components but I haven't checked that.)

Our current approach to solve this, is to "patch" the api-server manifest after the initial setup with kubeadm is complete (ansible playbook). This works quite well as a workaround, but I think kubeadm should allow us to solve this without the need to touch the generated kube-apiserver manifest.

Originally posted by @ghouscht in #1439 (comment)

---

EDIT by neolit123

• implemented in this PR for v1beta4
  kubeadm add support for structured ExtraArgs kubernetes#119156

• optimize argument.go logic
  kubeadm: Optimize the logic to override the arguments kubernetes#121020

• TODO
  the same problem happens for CLI flags:
  kubeadm init phase for apiserver does not accept multi valued flag in extra-args #1413
  see the discussion there.

[zbindenren]

How about the following structure:

---
apiVersion: kubeadm.k8s.io/v1beta1
kind: ClusterConfiguration
kubernetesVersion: 1.14.2
apiServer:
  extraArgs:
    - name: "tls-sni-cert-key"
      value: "/etc/kubernetes/pki/apiserver.crt,/etc/kubernetes/pki/apiserver.key"
    - name: "tls-sni-cert-key"
      value: "/etc/kubernetes/pki/mycert.crt,/etc/kubernetes/pki/mykey.key"

[creation-shin-chan]

@zbindenren
I have the same problem wit "--service-account-key-file". Your suggestion didn't work. kubeadm just gives me unmarshal error.

[neolit123]

for now you'd have to patch your manifest files manually after they are created.
(i.e. after kubeadm finishes running).

there is a WIP alpha feature for 1.16 that will allow you to customize static pods using patches:
#1379

but i'm keeping this issue open in case we figure a good way to allow multiple keys with the same name.

[zbindenren]

@creation-shin-chan that does not actually work. This was just a proposition how it could be implemented.

[creation-shin-chan]

@zbindenren
I create kubeadm-config.yaml as you suggested. However, kubeadm upgrade --config=kubeadm-config.yaml give me unsharl error.

In kube-apiserver.yaml manifest (in /etc/kubernetes/manifests), I added these lines.

    - --service-account-key-file=/etc/kubernetes/pki/pub.pem
    - --service-account-signing-key-file=/etc/kubernetes/pki/priv.key

Therefore, I need --service-account-key-file=/etc/kubernetes/pki/pub.pem and --service-account-key-file=/etc/kubernetes/pki/sa.pub in kube-apiserver manifest.

So I tried adding service-account-key-file: /etc/kubernetes/pki/pub.pem to kubeadm-config.yaml and run kubeadm init or upgrade, --service-account-key-file=/etc/kubernetes/pki/sa.pub disappears and kube-controller-manager doesn't start up with unauthorized error.

At the moment, I need to add the public keys into a single file or manually add --service-account-key-file=/etc/kubernetes/pki/pub.pem parameter.
However, every time I run kubeadm upgrade to upgrade kubernetes version, the parameter manually added disappears so I need to add it again manually.

[neolit123]

However, every time I run kubeadm upgrade to upgrade kubernetes version, the parameter manually added disappears so I need to add it again manually.

yes, upgrade will overwrite the existing manifests on disk. that is because it uses the CluserConfiguration in the kubeadm-config ConfigMap as the source of truth and the ClusterConfiguration does not support multiple keys with the same name in a map.

please note that using kubeadm upgrade --config=kubeadm-config.yaml to reconfigure the cluster is not recommended it really performs more than updating your manifests.

ticket for cluster reconf #970
but reconf will not work unless we support the multiple keys.

as mentioned kustomize patches will allow you to patch your changes and preserve them during init and uprade:
#1379 (WIP)
but if you already started the cluster, then you need to design your patches for upgrade.

[neolit123]

merging with #1413
that issue outlines the CLI flag problem and this one outlines the kubeadm config problem.

[hickeng]

@neolit123 I'm seeing the same issue with kustomize dropping all but the last value when run against a kubeadm.yaml that has two similarly named extraArgs present (tls-sni-cert-key in this specific case).

I'm not clear how patching afterwards using kustomize is expected to improve this.
Please note this is using kustomize binary and NOT the kustomize integrated into kubeadm.

[fabriziopandini]

@hickeng
The ExtraArgs field in the kubeadm config file is a map[string]string, so it does not support duplicated key no matter if you pass them manually or if you try to add them with kustomize.
Instead, If I got it right, what @neolit123 is suggesting is to use kustomize to patch the generated API server Pod manifest changing the args list directly

[dnmgns]

👀

[neolit123]

marking as api-change, because of the map[string]string binding in the API.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[neolit123]

/remove-lifecycle stale

[prashantpsr24]

Facing the same issue with kubeadm

[neolit123]

for now you have to patch your YAML manually or it should be possible with the --exterimental-kustomize= feature too, currently.

[xlgao-zju]

@neolit123 any plan to change the ExtraArgs to other form? Like

type arg struct {
  key    string
  value string
}

instead of map[string]string

[neolit123]

we haven't made this fix because it also has to change in the latest API v1beta3.
but a multi-map of sorts would work too:
https://stackoverflow.com/a/19519386

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[neolit123]

/remove-lifecycle stale

[neolit123]

/remove-lifecycle stale

[challa-pradyumna]

if not for this API version it can be done for the next one, but it might take a while before v-next. EDIT: the API change itself is not that complicated, but kubeadm just makes to many assumptions about this being a map[string]string in a lot of places...

name/value as in podspec and an API change makes the most sense; similar was proposed above.

alternatively this could also be a []map[string]string, this has a very close syntax for yaml users (who are a majority):

extraArgs:
  - foo1: bar1
  - foo2: bar2

but not so similar for JSON users.

the same feels a bit hacky and requires validation to limit to one max key per map[string]string, so that there are no:

extraArgs:
  - foo1: bar1
    foo2: bar2
  - foo3: bar3

How about

extraArgs:
- foo1:
  - bar1
- foo2:
  - bar2
  - bar3

[chendave]

Just note we are planning to release v1beta4 in 1.29 release, this is potentially a candidate to fix in the 1.29. Now that we will release a new API, we can just update the extraArgs from

ExtraArgs map[string]string

to something like

ExtraArgs []Args {
    Name string
    Value string
}

and update all the impacted feature and code path, e.g. kubeadm config migrate.

[chendave]

/help

for anyone who want to take this, pls assign this to yourself.

[k8s-ci-robot]

@chendave:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

for anyone who want to take this, pls assign this to yourself.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ruquanzhao]

/assign

[neolit123]

@ruquanzhao
i've assigned myself. reminder that this is also a related problem (i.e. duplicate CLI "extra-args" flags)

the same problem happens for CLI flags:
#1413
see the discussion there.