bootstrap-provider: allow the content of a file to come from a configmap or secret

[richardcase]

User Story

As an operator I would like to be able to be able to get the contents of a file from a configmap or secret so that i don't have to embed the contents directly within the KubeadmConfig. This is especially useful for custom configuration using PreKubeadmCommands and PostKubeadmCommands.

Detailed Description

I would like to see Contents changed with the File struct so that you can specify the contents in 3 ways:

• Explicit value (how you currently do it)
• Reference a config map and get the contents from there when creating the bootstrap data
• Reference a secret and get the contents from the secret when creating the bootstrap data

This is the current situation:

apiVersion: bootstrap.cluster.x-k8s.io/v1alpha2
kind: KubeadmConfig
metadata:
  name: capi-quickstart-controlplane-0
spec:
  clusterConfiguration:
    apiServer:
      extraArgs:
        cloud-config: /etc/kubernetes/azure.json
        cloud-provider: azure
      extraVolumes:
      - hostPath: /etc/kubernetes/azure.json
        mountPath: /etc/kubernetes/azure.json
        name: cloud-config
        readOnly: true
      timeoutForControlPlane: 20m
    controllerManager:
      extraArgs:
        allocate-node-cidrs: "false"
        cloud-config: /etc/kubernetes/azure.json
        cloud-provider: azure
      extraVolumes:
      - hostPath: /etc/kubernetes/azure.json
        mountPath: /etc/kubernetes/azure.json
        name: cloud-config
        readOnly: true
  files:
  - content: |
      {
        "cloud": "AzurePublicCloud",
        "tenantId": "${AZURE_TENANT_ID}",
        "subscriptionId": "${AZURE_SUBSCRIPTION_ID}",
        "aadClientId": "${AZURE_CLIENT_ID}",
        "aadClientSecret": "${AZURE_CLIENT_SECRET}",
        ....
      }
    owner: root:root
    path: /etc/kubernetes/azure.json
    permissions: "0644"

And it could be changed to something like this:

  files:
  - content
        valueFrom:
            secretKeyRef:
                   name: azurecreds
                   key: azure.json
    owner: root:root
    path: /etc/kubernetes/azure.json
    permissions: "0644"

Or using an ObjectReference

Anything else you would like to add:

/kind feature

[ncdc]

I like this idea. To make API conversion easier, we should probably keep content as-is, and consider adding a contentFrom field as well:

type File struct {
  // ...

  // Content is the actual content of the file. Mutually exclusive with ContentFrom.
  Content string `json:"content"`

  // ContentFrom indicates the content for the file should be retrieved from a referenced resource. Mutually exclusive with Content.
  ContentFrom FileContentSource `json:"contentFrom"`
}

type FileContentSource struct {
  ConfigMapKeyRef *corev1.ConfigMapKeySelector `json:"configMapKeyRef"`

  SecretKeyRef *corev1.SecretKeySelector `json:"secretKeyRef"`
}

[ncdc]

/help

[detiber]

@ncdc are you thinking also that the use of ContentFrom would prevent the ability to convert to an earlier version?

[nstogner]

I would love to take a stab at this one.

[ncdc]

@detiber I think it would have to, given that we don't have a Client currently available in our conversion code (although we could create one).

I also think there are differently levels of compatibility, maybe. For a change like this, it really should only affect the bootstrap controller, and if you've updated to v1a3 for the CRDs, you presumably are updating the controller at the same time. Are there use cases for external tooling/consumers of the KubeadmConfig where they are reading the resource and doing something with it?

[ncdc]

/assign @nstogner

[ncdc]

Please comment /lifecycle active when you start working on this - thanks.

[detiber]

I also think there are differently levels of compatibility, maybe. For a change like this, it really should only affect the bootstrap controller, and if you've updated to v1a3 for the CRDs, you presumably are updating the controller at the same time. Are there use cases for external tooling/consumers of the KubeadmConfig where they are reading the resource and doing something with it?

I'm not entirely sure that we can say that something isn't interacting with these resources. Longer term, I think we'd want to avoid upgrades that require stopping all tooling (cluster-api owned or not).

In the v1alpha2 space, the upgrade tool interacts with the bootstrap resources when trying to create a new control plane instance, for example.

[ncdc]

If our APIs were beta or v1, I think this would be of much more serious concern. But we're alpha, and I view being able to upgrade to newer versions significantly more important than supporting older clients against newer versions. I am happy to proceed with this change, and we can have strongly worded guidance that there are new features in the newer alpha version that may break things if you continue to try to use tools compiled against v1alpha2.

[nstogner]

/lifecycle active

[nstogner]

Initial Thoughts

Avoid pushing a k8s client down into cloudinit by defining a new cloudinit.File type which only contains data for files, no references to k8s objects. Resolve content refs in ./bootstrap/kubeadm/controllers and translate api.File types into cloudinit.File types before calling funcs like cloudinit.NewInitControlPlane(...).

bootstrap/kubeadm/cloudinit/cloudinit.go:

type BaseUserData struct {
	...
-	AdditionalFiles     []bootstrapv1.File
+	AdditionalFiles     []File
	...
}

+type File struct {
+	Path string
+	Owner string
+	Permissions string
+	Encoding bootstrapv1.Encoding // Use api type here?
+	Content string
+}

For reference, kubelet appears to do a similar translation of API type into a simple key-value pair type in kubelet_pods.go:

func (kl *Kubelet) makeEnvironmentVariables(pod *v1.Pod, container *v1.Container, podIP string) ([]kubecontainer.EnvVar, error) {

[nstogner]

Alternatively to creating a new cloudinit.File type, we could resolve api.File.Content from api.File.ContentFrom before passing api.File into cloudinit, however this may muddy the exposed cloudinit input params with non-used variables.

[ncdc]

Having an internal cloudinit.File seems ok at a quick glance. Let's wait until @chuckha is back next week so he can comment as well.

[nstogner]

I got excited and went ahead and took a quick pass at it. Love to hear feedback.

[fabriziopandini]

Please, while designing the API around this feature please consider also the need of support "Pivot/Move" of those resources.
The part that most worries me is to understand if more that one kubeadm config can reference to the same configmap/secret

[vincepri]

/area clusterctl

[ncdc]

Given that we're still debating the API, I'm going to remove this from the milestone for the time being. We can always bring it back if we get to a resolution and the timing works out.

/milestone Next
/remove-priority important-soon
/priority important-longterm

[fabriziopandini]

/remove-area clusterctl
clusterctl now supports moving shared objects, so the design of this API should not be a problem anymore if the OwnerRef chain is properly set (see https://master.cluster-api.sigs.k8s.io/clusterctl/provider-contract.html#ownerreferences-chain)

[vincepri]

Should we move this discussion to a more concrete proposal in a google doc or PR?

[vincepri]

/priority backlog
/area ux

[alexeldeib]

/assign
/remove-help

[CecileRobertMichon]

@alexeldeib can this issue be closed?

[alexeldeib]

yep 👍

/close

[k8s-ci-robot]

@alexeldeib: Closing this issue.

In response to this:

yep 👍

/close

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.