From c85fb0e6acf9b69d55a7e6166cb2fb8dd75383c8 Mon Sep 17 00:00:00 2001 From: Dave Chen Date: Mon, 1 Nov 2021 17:25:43 +0800 Subject: [PATCH] Aggregate all the errors when the shared certs are validated Instead of the individual error and return, it's better to aggregate all the errors so that we can fix them all at once. Take the chance to fix some comments, since kubeadm are not checking that the certs are equal across controlplane. Signed-off-by: Dave Chen --- cmd/kubeadm/app/cmd/phases/join/preflight.go | 2 +- cmd/kubeadm/app/phases/certs/certs.go | 21 +++++----- cmd/kubeadm/app/phases/certs/certs_test.go | 41 +++++++++++++------- 3 files changed, 40 insertions(+), 24 deletions(-) diff --git a/cmd/kubeadm/app/cmd/phases/join/preflight.go b/cmd/kubeadm/app/cmd/phases/join/preflight.go index e810cfe822f..6cd8a452819 100644 --- a/cmd/kubeadm/app/cmd/phases/join/preflight.go +++ b/cmd/kubeadm/app/cmd/phases/join/preflight.go @@ -147,7 +147,7 @@ func checkIfReadyForAdditionalControlPlane(initConfiguration *kubeadmapi.Cluster } if !hasCertificateKey { - // checks if the certificates that must be equal across controlplane instances are provided + // checks if the certificates are provided and are still valid, not expired yet. if ret, err := certs.SharedCertificateExists(initConfiguration); !ret { return err } diff --git a/cmd/kubeadm/app/phases/certs/certs.go b/cmd/kubeadm/app/phases/certs/certs.go index 2ebd58b836e..7e8a60bce91 100644 --- a/cmd/kubeadm/app/phases/certs/certs.go +++ b/cmd/kubeadm/app/phases/certs/certs.go @@ -26,6 +26,7 @@ import ( "github.com/pkg/errors" + utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/util/keyutil" "k8s.io/klog/v2" @@ -304,30 +305,32 @@ type certKeyLocation struct { uxName string } -// SharedCertificateExists verifies if the shared certificates - the certificates that must be -// equal across control-plane nodes: ca.key, ca.crt, sa.key, sa.pub + etcd/ca.key, etcd/ca.crt if local/stacked etcd -// Missing keys are non-fatal and produce warnings. +// SharedCertificateExists verifies if the shared certificates exist and are still valid - the certificates must be +// equal across control-plane nodes: ca.key, ca.crt, sa.key, sa.pub, front-proxy-ca.key, front-proxy-ca.crt and etcd/ca.key, etcd/ca.crt if local/stacked etcd +// Missing private keys of CA are non-fatal and produce warnings. func SharedCertificateExists(cfg *kubeadmapi.ClusterConfiguration) (bool, error) { - + var errs []error if err := validateCACertAndKey(certKeyLocation{cfg.CertificatesDir, kubeadmconstants.CACertAndKeyBaseName, "", "CA"}); err != nil { - return false, err + errs = append(errs, err) } if err := validatePrivatePublicKey(certKeyLocation{cfg.CertificatesDir, "", kubeadmconstants.ServiceAccountKeyBaseName, "service account"}); err != nil { - return false, err + errs = append(errs, err) } if err := validateCACertAndKey(certKeyLocation{cfg.CertificatesDir, kubeadmconstants.FrontProxyCACertAndKeyBaseName, "", "front-proxy CA"}); err != nil { - return false, err + errs = append(errs, err) } // in case of local/stacked etcd if cfg.Etcd.External == nil { if err := validateCACertAndKey(certKeyLocation{cfg.CertificatesDir, kubeadmconstants.EtcdCACertAndKeyBaseName, "", "etcd CA"}); err != nil { - return false, err + errs = append(errs, err) } } - + if len(errs) != 0 { + return false, utilerrors.NewAggregate(errs) + } return true, nil } diff --git a/cmd/kubeadm/app/phases/certs/certs_test.go b/cmd/kubeadm/app/phases/certs/certs_test.go index 51e8afa5ce5..c00269fed19 100644 --- a/cmd/kubeadm/app/phases/certs/certs_test.go +++ b/cmd/kubeadm/app/phases/certs/certs_test.go @@ -31,6 +31,7 @@ import ( "github.com/pkg/errors" "github.com/stretchr/testify/assert" + utilerrors "k8s.io/apimachinery/pkg/util/errors" certutil "k8s.io/client-go/util/cert" "k8s.io/client-go/util/keyutil" @@ -373,9 +374,9 @@ func TestSharedCertificateExists(t *testing.T) { publicKey := key.Public() var tests = []struct { - name string - files certstestutil.PKIFiles - expectedError bool + name string + files certstestutil.PKIFiles + expectedErrors int }{ { name: "success", @@ -401,7 +402,7 @@ func TestSharedCertificateExists(t *testing.T) { "etcd/ca.crt": caCert, "etcd/ca.key": caKey, }, - expectedError: true, + expectedErrors: 1, }, { name: "missing ca.key", @@ -414,7 +415,6 @@ func TestSharedCertificateExists(t *testing.T) { "etcd/ca.crt": caCert, "etcd/ca.key": caKey, }, - expectedError: false, }, { name: "missing sa.key", @@ -427,7 +427,7 @@ func TestSharedCertificateExists(t *testing.T) { "etcd/ca.crt": caCert, "etcd/ca.key": caKey, }, - expectedError: true, + expectedErrors: 1, }, { name: "missing front-proxy.crt", @@ -440,20 +440,32 @@ func TestSharedCertificateExists(t *testing.T) { "etcd/ca.crt": caCert, "etcd/ca.key": caKey, }, - expectedError: true, + expectedErrors: 1, }, { name: "missing etcd/ca.crt", files: certstestutil.PKIFiles{ "ca.crt": caCert, "ca.key": caKey, + "front-proxy-ca.crt": caCert, "front-proxy-ca.key": caKey, "sa.pub": publicKey, "sa.key": key, - "etcd/ca.crt": caCert, "etcd/ca.key": caKey, }, - expectedError: true, + expectedErrors: 1, + }, + { + name: "missing multiple certs (ca.crt and etcd/ca.crt)", + files: certstestutil.PKIFiles{ + "ca.key": caKey, + "front-proxy-ca.crt": caCert, + "front-proxy-ca.key": caKey, + "sa.pub": publicKey, + "sa.key": key, + "etcd/ca.key": caKey, + }, + expectedErrors: 2, }, } @@ -472,12 +484,13 @@ func TestSharedCertificateExists(t *testing.T) { // executes create func ret, err := SharedCertificateExists(cfg) - switch { - case !test.expectedError && err != nil: - t.Errorf("error SharedCertificateExists failed when not expected to fail: %v", err) - case test.expectedError && err == nil: - t.Errorf("error SharedCertificateExists didn't failed when expected") + case err != nil: + if agg, ok := err.(utilerrors.Aggregate); ok && len(agg.Errors()) != test.expectedErrors { + t.Errorf("SharedCertificateExists didn't fail with the expected number of errors, expected: %v, got: %v", test.expectedErrors, len(agg.Errors())) + } + case err == nil && test.expectedErrors != 0: + t.Errorf("error SharedCertificateExists didn't fail when expected") case ret != (err == nil): t.Errorf("error SharedCertificateExists returned %v when expected to return %v", ret, err == nil) }