kubeadm: warn but do not error out on missing CA keys on CP join #94988

neolit123 · 2020-09-23T00:31:21Z

What this PR does / why we need it:

- Modify validateCACertAndKey() to print warnings for missing keys
instead of erroring out.
- Update unit tests.

This allows doing a CP node join in a case where the user has:
- copied shared certificates to the new CP node, but not copied
ca.key files, treating the cluster CAs as external
- signed other required certificates in advance

The kubeconfig phase of "kubeadm init" detects external CA mode
and skips the generation of kubeconfig files. The kubeconfig
handling during control-plane join executes
CreateJoinControlPlaneKubeConfigFiles() which requires the presence
of ca.key when preparing the spec of a kubeconfig file and prevents
usage of external CA mode.

Modify CreateJoinControlPlaneKubeConfigFiles() to skip generating
the kubeconfig files if external CA mode is detected.

Which issue(s) this PR fixes:

xref kubernetes/kubeadm#2296

Special notes for your reviewer:
NONE

Does this PR introduce a user-facing change?:

kubeadm: warn but do not error out on missing "ca.key" files for root CA, front-proxy CA and etcd CA, during "kubeadm join --control-plane" if the user has provided all certificates, keys and kubeconfig files which require signing with the given CA keys.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

neolit123 · 2020-09-23T00:31:37Z

/kind bug
/priority important-soon

k8s-ci-robot · 2020-09-23T00:32:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/kubeadm/OWNERS~~ [neolit123]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

neolit123 · 2020-09-23T20:20:41Z

this is ready for a review.
the summary is that CP join is currently broken for external CA mode (no ca.key). we should backport the change to the entire skew. i was able to verify this using a kinder e2e test for external CA that i will send a PR for soon.

/assign @fabriziopandini

neolit123 · 2020-09-23T20:38:04Z

/retest

neolit123 · 2020-09-23T20:38:33Z

/hold

fabriziopandini · 2020-09-24T09:59:01Z

cmd/kubeadm/app/phases/certs/certs.go

 func SharedCertificateExists(cfg *kubeadmapi.ClusterConfiguration) (bool, error) {

-	if err := validateCACertAndKey(certKeyLocation{cfg.CertificatesDir, kubeadmconstants.CACertAndKeyBaseName, "", "CA"}); err != nil {
+	if err := validateCACertAndKeyWarn(certKeyLocation{cfg.CertificatesDir, kubeadmconstants.CACertAndKeyBaseName, "", "CA"}); err != nil {


I'm pretty sure we should we apply the same change to FrontProxyCA
Instead, if I remember well, etcd does not support an external EtcdCA mode...

i guess we should have warnings instead of errors for the front-proxy's CA key and etcd's CA key too.
if the CA is external, users may choose to generate everything externally and place the above CA certificates only, without the keys. unlike the root singing CA key, these keys are not needed during cluster runtime.

@fabriziopandini i have update the PR.
instead of adding a new function validateCACertAndKey is re-purposed to throw a warning.

- Modify validateCACertAndKey() to print warnings for missing keys instead of erroring out. - Update unit tests. This allows doing a CP node join in a case where the user has: - copied shared certificates to the new CP node, but not copied ca.key files, treating the cluster CAs as external - signed other required certificates in advance

The kubeconfig phase of "kubeadm init" detects external CA mode and skips the generation of kubeconfig files. The kubeconfig handling during control-plane join executes CreateJoinControlPlaneKubeConfigFiles() which requires the presence of ca.key when preparing the spec of a kubeconfig file and prevents usage of external CA mode. Modify CreateJoinControlPlaneKubeConfigFiles() to skip generating the kubeconfig files if external CA mode is detected.

fabriziopandini · 2020-09-25T08:56:13Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go

+	var externaCA bool
+	caKeyPath := filepath.Join(cfg.CertificatesDir, kubeadmconstants.CAKeyName)
+	if _, err := os.Stat(caKeyPath); os.IsNotExist(err) {
+		externaCA = true
+	}


Should we use certs.UsingExternalCA for determining if we are in external CA mode?

i think UsingExternalCA and UsingExternalFrontProxyCA should only check existence of .key files.

.key files missing would indicate to kubeadm about the general assumption of "external CA", but the validation of certificate signatures should be a separate step based on the .crt.

fabriziopandini · 2020-09-28T12:16:06Z

/lgtm

I think that in the future we should make the external CA mode something explicit, e.g. with a flag in the config, instead of guessing user intentions from the file existing on disk

neolit123 · 2020-09-28T12:30:28Z

/hold cancel

this must be backported to the support skew. will send the cherry picks later today.

…988-origin-release-1.17 Automated cherry pick of #94988: kubeadm: warn but do not error out on missing CA keys on

…988-origin-release-1.18 Automated cherry pick of #94988: kubeadm: warn but do not error out on missing CA keys on

…988-origin-release-1.19 Automated cherry pick of #94988: kubeadm: warn but do not error out on missing CA keys on

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 23, 2020

k8s-ci-robot requested review from fabriziopandini and yastij September 23, 2020 00:32

neolit123 mentioned this pull request Sep 23, 2020

CA.Key required for joining the another node to cluster with External CA and Kubeadm kubernetes/kubeadm#2296

Closed

k8s-ci-robot assigned fabriziopandini Sep 23, 2020

neolit123 changed the title ~~WIP: kubeadm: warn but do not error out on missing ca.key on CP join~~ kubeadm: warn but do not error out on missing ca.key on CP join Sep 23, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 23, 2020

neolit123 force-pushed the 1.20-tollerate-missing-ca-key-on-join branch from f32fccb to 2a86041 Compare September 23, 2020 20:35

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 23, 2020

neolit123 mentioned this pull request Sep 24, 2020

kinder: add workflows for testing external CA mode kubernetes/kubeadm#2298

Merged

fabriziopandini reviewed Sep 24, 2020

View reviewed changes

neolit123 added 2 commits September 24, 2020 18:11

neolit123 force-pushed the 1.20-tollerate-missing-ca-key-on-join branch from 2a86041 to 7c783fa Compare September 24, 2020 15:12

neolit123 changed the title ~~kubeadm: warn but do not error out on missing ca.key on CP join~~ kubeadm: warn but do not error out on missing CA keys on CP join Sep 24, 2020

fabriziopandini reviewed Sep 25, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 28, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2020

neolit123 mentioned this pull request Sep 28, 2020

sig-cluster-lifecycle: add test jobs for external CA mode kubernetes/test-infra#19380

Merged

k8s-ci-robot merged commit db9f1e9 into kubernetes:master Sep 28, 2020

k8s-ci-robot added this to the v1.20 milestone Sep 28, 2020

k8s-ci-robot added a commit that referenced this pull request Sep 28, 2020

Merge pull request #95118 from neolit123/automated-cherry-pick-of-#94…

222082f

…988-origin-release-1.17 Automated cherry pick of #94988: kubeadm: warn but do not error out on missing CA keys on

k8s-ci-robot added a commit that referenced this pull request Sep 28, 2020

Merge pull request #95119 from neolit123/automated-cherry-pick-of-#94…

32fd318

…988-origin-release-1.18 Automated cherry pick of #94988: kubeadm: warn but do not error out on missing CA keys on

k8s-ci-robot added a commit that referenced this pull request Sep 28, 2020

Merge pull request #95120 from neolit123/automated-cherry-pick-of-#94…

e97a0c2

…988-origin-release-1.19 Automated cherry pick of #94988: kubeadm: warn but do not error out on missing CA keys on

github-actions bot mentioned this pull request Oct 6, 2020

Week Ending October 4, 2020 dev-obs/actus#238

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: warn but do not error out on missing CA keys on CP join #94988

kubeadm: warn but do not error out on missing CA keys on CP join #94988

neolit123 commented Sep 23, 2020 •

edited

neolit123 commented Sep 23, 2020

k8s-ci-robot commented Sep 23, 2020

neolit123 commented Sep 23, 2020 •

edited

neolit123 commented Sep 23, 2020

neolit123 commented Sep 23, 2020

fabriziopandini Sep 24, 2020

neolit123 Sep 24, 2020 •

edited

neolit123 Sep 24, 2020

fabriziopandini Sep 25, 2020

neolit123 Sep 26, 2020 •

edited

fabriziopandini commented Sep 28, 2020

neolit123 commented Sep 28, 2020

kubeadm: warn but do not error out on missing CA keys on CP join #94988

kubeadm: warn but do not error out on missing CA keys on CP join #94988

Conversation

neolit123 commented Sep 23, 2020 • edited

neolit123 commented Sep 23, 2020

k8s-ci-robot commented Sep 23, 2020

neolit123 commented Sep 23, 2020 • edited

neolit123 commented Sep 23, 2020

neolit123 commented Sep 23, 2020

fabriziopandini Sep 24, 2020

Choose a reason for hiding this comment

neolit123 Sep 24, 2020 • edited

Choose a reason for hiding this comment

neolit123 Sep 24, 2020

Choose a reason for hiding this comment

fabriziopandini Sep 25, 2020

Choose a reason for hiding this comment

neolit123 Sep 26, 2020 • edited

Choose a reason for hiding this comment

fabriziopandini commented Sep 28, 2020

neolit123 commented Sep 28, 2020

neolit123 commented Sep 23, 2020 •

edited

neolit123 commented Sep 23, 2020 •

edited

neolit123 Sep 24, 2020 •

edited

neolit123 Sep 26, 2020 •

edited