kubeadm: Make it possible to configure volume mounts via the config file #49840

andrewrynhard · 2017-07-29T21:01:37Z

What this PR does / why we need it:
Kubeadm mounts host CA certs into api server and controller manager. It uses /etc/pki and does not allow for the path to be configurable. This PR adds a default to /etc/pki but also allows a user to configure the path in the config file. In the case of using Container Linux, the CAs are located at /usr/share/ca-certificates, so without this PR the hardcoded /etc/pki path is used and will break, for example, the --cloud-provider flag because of missing CAs.

Fixes kubernetes/kubeadm#484
Fixes kubernetes/kubeadm#476
Fixes kubernetes/kubeadm#441

/cc @luxas

k8s-ci-robot · 2017-07-29T21:01:41Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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. I understand the commands that are listed here.

k8s-ci-robot · 2017-07-29T21:01:44Z

Hi @andrewrynhard. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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. I understand the commands that are listed here.

andrewrynhard · 2017-07-29T23:40:48Z

Manifests are generated correctly

core # cat /etc/kubernetes/manifests/kube-apiserver.yaml | grep /usr/share/ca-certificates
    - mountPath: /usr/share/ca-certificates
      path: /usr/share/ca-certificates
core # cat /etc/kubernetes/manifests/kube-controller-manager.yaml | grep /usr/share/ca-certificates
    - mountPath: /usr/share/ca-certificates
      path: /usr/share/ca-certificates

luxas

I talked to @andrewrynhard about this on Slack and we decided that a generic variant of this is much better. Something like this:

type HostPathMount struct {
	Name string
	HostPath string
	MountPath string
}

type MasterConfiguration struct {
        ...
	APIServerExtraVolumes []HostPathMount
	ControllerManagerExtraVolumes []HostPathMount
	SchedulerExtraVolumes []HostPathMount
}

luxas · 2017-07-31T15:59:09Z

/release-note
/ok-to-test

timothysc · 2017-08-01T21:23:09Z

@mattmoyer

mattmoyer

I had a couple of style questions. Other than picking the right name and fixing some tests, I don't see a reason not to make this configurable.

Distros are pretty inconsistent about where they keep "system" trust stores (/etc/pki/ vs. /usr/share/ca-certificates vs. /etc/ssl/).

mattmoyer · 2017-08-04T14:58:51Z

cmd/kubeadm/app/apis/kubeadm/types.go

@@ -51,6 +51,8 @@ type MasterConfiguration struct {
 	APIServerCertSANs []string
 	// CertificatesDir specifies where to store or look for all required certificates
 	CertificatesDir string
+	// CACertificatesPkiDir specifies where to mount host CA certficates
+	CACertificatesPkiDir string


Just a style nit but this should be CACertificatesPKIDir (per this guide).

I think the name is a little confusing too, since this doesn't have to do with the kubeadm-generated cluster CA like CertificatesDir does. Maybe SystemCACertificatesDir, HostCACertificatesDir, or ExternalCACertificatesDir?

mattmoyer · 2017-08-04T14:59:19Z

cmd/kubeadm/app/apis/kubeadm/v1alpha1/types.go

@@ -51,6 +51,8 @@ type MasterConfiguration struct {
 	APIServerCertSANs []string `json:"apiServerCertSANs"`
 	// CertificatesDir specifies where to store or look for all required certificates
 	CertificatesDir string `json:"certificatesDir"`
+	// CACertificatesPkiDir specifies where to mount host CA certficates
+	CACertificatesPkiDir string `json:"caCertificatesPkiDir"`


Same style nit here (s/Pki/PKI/).

mattmoyer · 2017-08-04T14:59:35Z

cmd/kubeadm/app/phases/controlplane/volumes.go

@@ -135,7 +130,7 @@ func newVolumeMount(name, path string, readOnly bool) v1.VolumeMount {
 }

 // getEtcdCertVolumes returns the volumes/volumemounts needed for talking to an external etcd cluster
-func getEtcdCertVolumes(etcdCfg kubeadmapi.Etcd) ([]v1.Volume, []v1.VolumeMount) {
+func getEtcdCertVolumes(etcdCfg kubeadmapi.Etcd, caCertsPkiVolumePath string) ([]v1.Volume, []v1.VolumeMount) {


Same style nit here also (s/Pki/PKI/).

mattmoyer · 2017-08-04T14:59:56Z

cmd/kubeadm/app/phases/controlplane/volumes.go

@@ -175,7 +170,7 @@ func getEtcdCertVolumes(etcdCfg kubeadmapi.Etcd) ([]v1.Volume, []v1.VolumeMount)
 // isPkiVolumeMountNeeded specifies whether /etc/pki should be host-mounted into the containers
 // On some systems were we host-mount /etc/ssl/certs, it is also required to mount /etc/pki. This is needed
 // due to symlinks pointing from files in /etc/ssl/certs into /etc/pki/
-func isPkiVolumeMountNeeded() bool {
+func isPkiVolumeMountNeeded(caCertsPkiVolumePath string) bool {


Same style nit here also (s/Pki/PKI/).

mattmoyer · 2017-08-04T15:02:51Z

cmd/kubeadm/app/phases/controlplane/volumes.go

-// caCertsPkiVolumePath specifies the path that can be conditionally mounted into the apiserver and controller-manager containers
-// as /etc/ssl/certs might be a symlink to it. It's a variable since it may be changed in unit testing. This var MUST NOT be changed
-// in normal codepaths during runtime.
-var caCertsPkiVolumePath = "/etc/pki"


It looks like the unit tests that used this variable need a fix (test log):

[...] W0804 00:30:36.473] # k8s.io/kubernetes/cmd/kubeadm/app/phases/controlplane W0804 00:30:36.474] cmd/kubeadm/app/phases/controlplane/volumes_test.go:276: not enough arguments in call to getEtcdCertVolumes W0804 00:30:36.474] have (kubeadm.Etcd) W0804 00:30:36.475] want (kubeadm.Etcd, string) W0804 00:30:36.475] cmd/kubeadm/app/phases/controlplane/volumes_test.go:514: undefined: caCertsPkiVolumePath W0804 00:30:36.475] cmd/kubeadm/app/phases/controlplane/volumes_test.go:515: undefined: caCertsPkiVolumePath [...]

mattmoyer · 2017-08-04T15:14:27Z

I'm also +1 on the idea of a generic HostPathMount configuration.

luxas · 2017-10-20T11:03:13Z

ping @andrewrynhard You're gonna take a look at this soon, right?

andrewrynhard · 2017-10-31T16:29:26Z

/retest

andrewrynhard · 2017-11-01T05:32:27Z

/test pull-kubernetes-unit

luxas

/lgtm

k8s-github-robot · 2017-11-01T06:29:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewrynhard, luxas

Associated issue: 484

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubeadm/OWNERS~~ [luxas]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

luxas · 2017-11-01T06:29:35Z

/release-note-none

fejta-bot · 2017-11-01T09:04:59Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-11-01T11:56:03Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-11-01T14:27:17Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-ci-robot · 2017-11-01T15:00:53Z

@andrewrynhard: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-bazel	`6450d92`	link	`/test pull-kubernetes-bazel`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

andrewrynhard · 2017-11-01T15:20:04Z

/test pull-kubernetes-unit

k8s-github-robot · 2017-11-01T15:20:21Z

Automatic merge from submit-queue (batch tested with PRs 49840, 54937, 54543). If you want to cherry-pick this change to another branch, please follow the instructions here.

ArchiFleKs · 2018-01-03T15:32:54Z

Hi, does this PR allows to use volume type ? I'm trying with kubeadm config file to mount the cloud_config file for an OpenStack provider but all the extra volumes are of type DirectoryOrCreate

luxas · 2018-01-03T15:49:38Z

@ArchiFleKs Not right now, please file an issue with more details if that's something you'd like to see

usernkey · 2018-05-10T07:39:26Z

Is the feature of volume mounts via the config file been released on the current kubeadm?

I need to mount the cloud_config to add the AWS support :) using this approach

apiServerExtraArgs:
cloud-provider: "openstack"
cloud-config: "/etc/kubernetes/cloud.conf"
controllerManagerExtraArgs:
cloud-provider: "openstack"
cloud-config: "/etc/kubernetes/cloud.conf"

root@ip-10-1-18-136:~# kubeadm version
kubeadm version: &version.Info{Major:"1", Minor:"10", GitVersion:"v1.10.2", GitCommit:"81753b10df112992bf51bbc2c2f85208aad78335", GitTreeState:"clean", BuildDate:"2018-04-27T09:10:24Z", GoVersion:"go1.9.3", Compiler:"gc", Platform:"linux/amd64"}

andrewrynhard · 2018-05-10T18:14:35Z

@usernkey it is, see https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/apis/kubeadm/v1alpha1/types.go#L93-L99. Has been available since 1.8 I believe.

usernkey · 2018-05-11T07:30:51Z

cool thanks I'll try it

k8s-github-robot assigned krousey and timothysc Jul 29, 2017

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 29, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jul 29, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 29, 2017

andrewrynhard force-pushed the variable_certs_dir branch 2 times, most recently from a3fc2c0 to ea1097b Compare July 29, 2017 21:09

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 29, 2017

andrewrynhard force-pushed the variable_certs_dir branch from ea1097b to f81d7bf Compare July 29, 2017 21:53

andrewrynhard changed the title ~~Make the CA PKI path configurable~~ kubeadm: make the CA PKI path configurable Jul 30, 2017

luxas suggested changes Jul 31, 2017

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. release-note-label-needed labels Jul 31, 2017

timothysc assigned luxas and unassigned timothysc Aug 1, 2017

andrewrynhard force-pushed the variable_certs_dir branch from f81d7bf to 6450d92 Compare August 4, 2017 00:19

mattmoyer suggested changes Aug 4, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2017

andrewrynhard force-pushed the variable_certs_dir branch from 6450d92 to 3cc384b Compare October 25, 2017 18:26

k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 25, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2017

Allow extra volumes to be defined

5a64c04

andrewrynhard force-pushed the variable_certs_dir branch from 667d81a to 5a64c04 Compare November 1, 2017 04:44

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2017

luxas approved these changes Nov 1, 2017

View reviewed changes

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 1, 2017

k8s-github-robot merged commit c119a60 into kubernetes:master Nov 1, 2017

andrewrynhard deleted the variable_certs_dir branch November 1, 2017 15:21

arthur0 mentioned this pull request Nov 7, 2017

Tracking Issue: Concise outline for using cloud providers kubernetes/kubeadm#526

Closed

andrewrynhard mentioned this pull request Nov 17, 2017

Automated cherrypick of #55320 and #49840 for release-1.8 #55955

Closed

tamalsaha mentioned this pull request Jan 4, 2018

Kubeadm does not mount files/directories used in apiServerExtraArgs kubernetes/kubeadm#640

Closed

mattymo mentioned this pull request Mar 20, 2018

Keep kubeadm config in sync with kubelet deployment kubernetes-sigs/kubespray#2505

Closed

oz123 mentioned this pull request Nov 9, 2018

Support extra volumes for Kubeadm kubernetes/minikube#2767

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: Make it possible to configure volume mounts via the config file #49840

kubeadm: Make it possible to configure volume mounts via the config file #49840

andrewrynhard commented Jul 29, 2017 •

edited

k8s-ci-robot commented Jul 29, 2017

k8s-ci-robot commented Jul 29, 2017

andrewrynhard commented Jul 29, 2017

luxas left a comment

luxas commented Jul 31, 2017

timothysc commented Aug 1, 2017

mattmoyer left a comment

mattmoyer Aug 4, 2017

mattmoyer Aug 4, 2017

mattmoyer Aug 4, 2017

mattmoyer Aug 4, 2017

mattmoyer Aug 4, 2017

mattmoyer commented Aug 4, 2017

luxas commented Oct 20, 2017

andrewrynhard commented Oct 31, 2017

andrewrynhard commented Nov 1, 2017

luxas left a comment

k8s-github-robot commented Nov 1, 2017

luxas commented Nov 1, 2017

fejta-bot commented Nov 1, 2017

fejta-bot commented Nov 1, 2017

k8s-github-robot commented Nov 1, 2017

k8s-ci-robot commented Nov 1, 2017 •

edited

andrewrynhard commented Nov 1, 2017

k8s-github-robot commented Nov 1, 2017

ArchiFleKs commented Jan 3, 2018

luxas commented Jan 3, 2018

usernkey commented May 10, 2018 •

edited

andrewrynhard commented May 10, 2018 •

edited

usernkey commented May 11, 2018 •

edited

kubeadm: Make it possible to configure volume mounts via the config file #49840

kubeadm: Make it possible to configure volume mounts via the config file #49840

Conversation

andrewrynhard commented Jul 29, 2017 • edited

k8s-ci-robot commented Jul 29, 2017

k8s-ci-robot commented Jul 29, 2017

andrewrynhard commented Jul 29, 2017

luxas left a comment

Choose a reason for hiding this comment

luxas commented Jul 31, 2017

timothysc commented Aug 1, 2017

mattmoyer left a comment

Choose a reason for hiding this comment

mattmoyer Aug 4, 2017

Choose a reason for hiding this comment

mattmoyer Aug 4, 2017

Choose a reason for hiding this comment

mattmoyer Aug 4, 2017

Choose a reason for hiding this comment

mattmoyer Aug 4, 2017

Choose a reason for hiding this comment

mattmoyer Aug 4, 2017

Choose a reason for hiding this comment

mattmoyer commented Aug 4, 2017

luxas commented Oct 20, 2017

andrewrynhard commented Oct 31, 2017

andrewrynhard commented Nov 1, 2017

luxas left a comment

Choose a reason for hiding this comment

k8s-github-robot commented Nov 1, 2017

luxas commented Nov 1, 2017

fejta-bot commented Nov 1, 2017

fejta-bot commented Nov 1, 2017

k8s-github-robot commented Nov 1, 2017

k8s-ci-robot commented Nov 1, 2017 • edited

andrewrynhard commented Nov 1, 2017

k8s-github-robot commented Nov 1, 2017

ArchiFleKs commented Jan 3, 2018

luxas commented Jan 3, 2018

usernkey commented May 10, 2018 • edited

andrewrynhard commented May 10, 2018 • edited

usernkey commented May 11, 2018 • edited

andrewrynhard commented Jul 29, 2017 •

edited

k8s-ci-robot commented Nov 1, 2017 •

edited

usernkey commented May 10, 2018 •

edited

andrewrynhard commented May 10, 2018 •

edited

usernkey commented May 11, 2018 •

edited