Merge pull request kubernetes#77252 from rojkov/rewrite-CreateServiceAccountKeyAndPublicKeyFiles

kubeadm: do unit testing of actual public function

Author: k8s-ci-robot

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")
+			}
+		})
 	}
 }