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 <dave.chen@arm.com>

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"
 
@@ -375,7 +376,7 @@ func TestSharedCertificateExists(t *testing.T) {
 	var tests = []struct {
 		name           string
 		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)
 			}