From a6d7920f445a36c58c8566541d58c660dcd83cb9 Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Tue, 16 Apr 2019 17:33:24 +0300 Subject: [PATCH] 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. --- cmd/kubeadm/app/phases/certs/BUILD | 1 + cmd/kubeadm/app/phases/certs/certs.go | 69 +++++---------- cmd/kubeadm/app/phases/certs/certs_test.go | 99 +++++++++------------- 3 files changed, 60 insertions(+), 109 deletions(-) diff --git a/cmd/kubeadm/app/phases/certs/BUILD b/cmd/kubeadm/app/phases/certs/BUILD index 0d3333e08b..b9c566ec88 100644 --- a/cmd/kubeadm/app/phases/certs/BUILD +++ b/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", ], diff --git a/cmd/kubeadm/app/phases/certs/certs.go b/cmd/kubeadm/app/phases/certs/certs.go index e03c719ae4..8d6dfa0a0b 100644 --- a/cmd/kubeadm/app/phases/certs/certs.go +++ b/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. diff --git a/cmd/kubeadm/app/phases/certs/certs_test.go b/cmd/kubeadm/app/phases/certs/certs_test.go index 858cfab6ca..191f7aa950 100644 --- a/cmd/kubeadm/app/phases/certs/certs_test.go +++ b/cmd/kubeadm/app/phases/certs/certs_test.go @@ -301,84 +301,65 @@ func TestWriteCSRFilesIfNotExist(t *testing.T) { } -func TestWriteKeyFilesIfNotExist(t *testing.T) { +func TestCreateServiceAccountKeyAndPublicKeyFiles(t *testing.T) { + setupKey, err := keyutil.MakeEllipticPrivateKeyPEM() + if err != nil { + t.Fatalf("Can't setup test: %v", err) + } - setupKey, _ := NewServiceAccountSigningKey() - key, _ := NewServiceAccountSigningKey() - - 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) + 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 !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) - } - - 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") + } + }) } }