kubeadm: do unit testing of actual public function

Even though CreateServiceAccountKeyAndPublicKeyFiles() function is
an interface function it's not unittested. Instead it wraps a couple
of internal functions which are used only inside CreateServiceAccountKeyAndPublicKeyFiles()
and those internal functions are tested.

Rewrite the function to do only what it's intended to do and add unit
tests for it.

Author: Dmitry Rozhkov

cmd/kubeadm/app/phases/certs/BUILD

@@ -39,6 +39,7 @@ go_library(
         "//cmd/kubeadm/app/constants:go_default_library",
         "//cmd/kubeadm/app/util/pkiutil:go_default_library",
         "//staging/src/k8s.io/client-go/util/cert:go_default_library",
+        "//staging/src/k8s.io/client-go/util/keyutil:go_default_library",
         "//vendor/github.com/pkg/errors:go_default_library",
         "//vendor/k8s.io/klog:go_default_library",
     ],

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

@@ -25,6 +25,7 @@ import (
 
 	"github.com/pkg/errors"
 	certutil "k8s.io/client-go/util/cert"
+	"k8s.io/client-go/util/keyutil"
 	"k8s.io/klog"
 	kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
 	pkiutil "k8s.io/kubernetes/cmd/kubeadm/app/util/pkiutil"
@@ -67,27 +68,31 @@ func CreatePKIAssets(cfg *kubeadmapi.InitConfiguration) error {
 // If the sa public/private key files already exists in the target folder, they are used only if evaluated equals; otherwise an error is returned.
 func CreateServiceAccountKeyAndPublicKeyFiles(certsDir string) error {
 	klog.V(1).Infoln("creating a new public/private key files for signing service account users")
-	saSigningKey, err := NewServiceAccountSigningKey()
+	_, err := keyutil.PrivateKeyFromFile(filepath.Join(certsDir, kubeadmconstants.ServiceAccountPrivateKeyName))
+	if err == nil {
+		// kubeadm doesn't validate the existing certificate key more than this;
+		// Basically, if we find a key file with the same path kubeadm thinks those files
+		// are equal and doesn't bother writing a new file
+		fmt.Printf("[certs] Using the existing %q key\n", kubeadmconstants.ServiceAccountKeyBaseName)
+		return nil
+	} else if !os.IsNotExist(err) {
+		return errors.Wrapf(err, "file %s existed but it could not be loaded properly", kubeadmconstants.ServiceAccountPrivateKeyName)
+	}
+
+	// The key does NOT exist, let's generate it now
+	key, err := pkiutil.NewPrivateKey()
 	if err != nil {
 		return err
 	}
 
-	return writeKeyFilesIfNotExist(
-		certsDir,
-		kubeadmconstants.ServiceAccountKeyBaseName,
-		saSigningKey,
-	)
-}
+	// Write .key and .pub files to disk
+	fmt.Printf("[certs] Generating %q key and public key\n", kubeadmconstants.ServiceAccountKeyBaseName)
 
-// NewServiceAccountSigningKey generate public/private key pairs for signing service account tokens.
-func NewServiceAccountSigningKey() (crypto.Signer, error) {
-	// The key does NOT exist, let's generate it now
-	saSigningKey, err := pkiutil.NewPrivateKey()
-	if err != nil {
-		return nil, errors.Wrap(err, "failure while creating service account token signing key")
+	if err := pkiutil.WriteKey(certsDir, kubeadmconstants.ServiceAccountKeyBaseName, key); err != nil {
+		return err
 	}
 
-	return saSigningKey, nil
+	return pkiutil.WritePublicKey(certsDir, kubeadmconstants.ServiceAccountKeyBaseName, key.Public())
 }
 
 // CreateCACertAndKeyFiles generates and writes out a given certificate authority.
@@ -246,42 +251,6 @@ func writeCertificateFilesIfNotExist(pkiDir string, baseName string, signingCert
 	return nil
 }
 
-// writeKeyFilesIfNotExist write a new key to the given path.
-// If there already is a key file at the given path; kubeadm tries to load it and check if the values in the
-// existing and the expected key equals. If they do; kubeadm will just skip writing the file as it's up-to-date,
-// otherwise this function returns an error.
-func writeKeyFilesIfNotExist(pkiDir string, baseName string, key crypto.Signer) error {
-
-	// Checks if the key exists in the PKI directory
-	if pkiutil.CertOrKeyExist(pkiDir, baseName) {
-
-		// Try to load .key from the PKI directory
-		_, err := pkiutil.TryLoadKeyFromDisk(pkiDir, baseName)
-		if err != nil {
-			return errors.Wrapf(err, "%s key existed but it could not be loaded properly", baseName)
-		}
-
-		// kubeadm doesn't validate the existing certificate key more than this;
-		// Basically, if we find a key file with the same path kubeadm thinks those files
-		// are equal and doesn't bother writing a new file
-		fmt.Printf("[certs] Using the existing %q key\n", baseName)
-	} else {
-
-		// Write .key and .pub files to disk
-		fmt.Printf("[certs] Generating %q key and public key\n", baseName)
-
-		if err := pkiutil.WriteKey(pkiDir, baseName, key); err != nil {
-			return errors.Wrapf(err, "failure while saving %s key", baseName)
-		}
-
-		if err := pkiutil.WritePublicKey(pkiDir, baseName, key.Public()); err != nil {
-			return errors.Wrapf(err, "failure while saving %s public key", baseName)
-		}
-	}
-
-	return nil
-}
-
 // writeCSRFilesIfNotExist writes a new CSR to the given path.
 // If there already is a CSR file at the given path; kubeadm tries to load it and check if it's a valid certificate.
 // otherwise this function returns an error.

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

@@ -301,84 +301,65 @@ func TestWriteCSRFilesIfNotExist(t *testing.T) {
 
 }
 
-func TestWriteKeyFilesIfNotExist(t *testing.T) {
-
-	setupKey, _ := NewServiceAccountSigningKey()
-	key, _ := NewServiceAccountSigningKey()
+func TestCreateServiceAccountKeyAndPublicKeyFiles(t *testing.T) {
+	setupKey, err := keyutil.MakeEllipticPrivateKeyPEM()
+	if err != nil {
+		t.Fatalf("Can't setup test: %v", err)
+	}
 
-	var tests = []struct {
-		setupFunc     func(pkiDir string) error
-		expectedError bool
-		expectedKey   crypto.Signer
+	tcases := []struct {
+		name        string
+		setupFunc   func(pkiDir string) error
+		expectedErr bool
+		expectedKey []byte
 	}{
 		{ // key does not exists > key written
-			expectedKey: key,
+			name: "generate successfully",
 		},
 		{ // key exists > existing key used
+			name: "use existing key",
 			setupFunc: func(pkiDir string) error {
-				return writeKeyFilesIfNotExist(pkiDir, "dummy", setupKey)
+				err := keyutil.WriteKey(filepath.Join(pkiDir, kubeadmconstants.ServiceAccountPrivateKeyName), setupKey)
+				return err
 			},
 			expectedKey: setupKey,
 		},
 		{ // some file exists, but it is not a valid key > err
+			name: "empty key",
 			setupFunc: func(pkiDir string) error {
-				testutil.SetupEmptyFiles(t, pkiDir, "dummy.key")
+				testutil.SetupEmptyFiles(t, pkiDir, kubeadmconstants.ServiceAccountPrivateKeyName)
 				return nil
 			},
-			expectedError: true,
+			expectedErr: true,
 		},
 	}
+	for _, tt := range tcases {
+		t.Run(tt.name, func(t *testing.T) {
+			dir := testutil.SetupTempDir(t)
+			defer os.RemoveAll(dir)
 
-	for _, test := range tests {
-		// Create temp folder for the test case
-		tmpdir := testutil.SetupTempDir(t)
-		defer os.RemoveAll(tmpdir)
-
-		// executes setup func (if necessary)
-		if test.setupFunc != nil {
-			if err := test.setupFunc(tmpdir); err != nil {
-				t.Errorf("error executing setupFunc: %v", err)
-				continue
+			if tt.setupFunc != nil {
+				if err := tt.setupFunc(dir); err != nil {
+					t.Fatalf("error executing setupFunc: %v", err)
+				}
 			}
-		}
 
-		// executes create func
-		err := writeKeyFilesIfNotExist(tmpdir, "dummy", key)
-
-		if !test.expectedError && err != nil {
-			t.Errorf("error writeKeyFilesIfNotExist failed when not expected to fail: %v", err)
-			continue
-		} else if test.expectedError && err == nil {
-			t.Error("error writeKeyFilesIfNotExist didn't failed when expected")
-			continue
-		} else if test.expectedError || test.expectedKey == nil {
-			continue
-		}
-
-		// asserts expected files are there
-		testutil.AssertFileExists(t, tmpdir, "dummy.key", "dummy.pub")
-
-		// check created key
-		resultingKey, err := pkiutil.TryLoadKeyFromDisk(tmpdir, "dummy")
-		if err != nil {
-			t.Errorf("failure reading created key: %v", err)
-			continue
-		}
-
-		resultingKeyPEM, err := keyutil.MarshalPrivateKeyToPEM(resultingKey)
-		if err != nil {
-			t.Errorf("failure marshaling created key: %v", err)
-			continue
-		}
-
-		expectedKeyPEM, err := keyutil.MarshalPrivateKeyToPEM(test.expectedKey)
-		if err != nil {
-			t.Fatalf("Failed to marshal expected private key: %v", err)
-		}
+			err := CreateServiceAccountKeyAndPublicKeyFiles(dir)
+			if (err != nil) != tt.expectedErr {
+				t.Fatalf("expected error: %v, got: %v, error: %v", tt.expectedErr, err != nil, err)
+			} else if tt.expectedErr {
+				return
+			}
 
-		if !bytes.Equal(resultingKeyPEM, expectedKeyPEM) {
-			t.Error("created key does not match expected key")
-		}
+			resultingKeyPEM, wasGenerated, err := keyutil.LoadOrGenerateKeyFile(filepath.Join(dir, kubeadmconstants.ServiceAccountPrivateKeyName))
+			if err != nil {
+				t.Errorf("Can't load created key: %v", err)
+			} else if wasGenerated {
+				t.Error("The key was not created")
+			} else if tt.expectedKey != nil && !bytes.Equal(resultingKeyPEM, tt.expectedKey) {
+				t.Error("Non-existing key is used")
+			}
+		})
 	}
 }