Merge pull request #106042 from chendave/aggregate

kubeadm: aggregate all the errors when the shared certs are validated

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
 		}

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
+// 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 + etcd/ca.key, etcd/ca.crt if local/stacked etcd
+// 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 keys are non-fatal and produce warnings.
+// 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
 }
 

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
+		name           string
-		files         certstestutil.PKIFiles
+		files          certstestutil.PKIFiles
-		expectedError bool
+		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:
+			case err != nil:
-				t.Errorf("error SharedCertificateExists failed when not expected to fail: %v", err)
+				if agg, ok := err.(utilerrors.Aggregate); ok && len(agg.Errors()) != test.expectedErrors {
-			case test.expectedError && err == nil:
+					t.Errorf("SharedCertificateExists didn't fail with the expected number of errors, expected: %v, got: %v", test.expectedErrors, len(agg.Errors()))
-				t.Errorf("error SharedCertificateExists didn't failed when expected")
+				}
+			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)
 			}