Use `CertDir` instead of `CertDirManager` in APIServer

To make our framework easier to use, we now use a `CertDir` struct
instead of the `CertDirManager` to create and destroy (potentially
temporary) directories.

This `CertDir` holds either one of or both a path to a dir (as string)
and a function which can clean up the directory. In the default case a
temporary directory is created and cleaned up. For any other use case
users can just specify the path to an existing directory or override the
cleanup function.

pkg/framework/test/apiserver.go

@@ -34,13 +34,8 @@ type APIServer struct {
 	// See the `SpecialPathFinder` example.
 	ProcessStarter SimpleSessionStarter
 
-	// CertDirManager is responsible to provide a directory where the APIServer can find and store certain
+	// CertDir is a struct holding a path to a certificate directory and a function to cleanup that directory.
-	// certificates and keys, and to clean up after the APIServer was shut down
+	CertDir *CertDir
-	// If not specified, a empty temporary directory is created and deleted.
-	//
-	// You can customise this if, e.g. you wish to pre-populate the directory with certs & keys from your central
-	// secrets store.  See the `CredHubCertDirManager` example.
-	CertDirManager CertDirManager
 
 	// Etcd is an implementation of a ControlPlaneProcess and is responsible to run Etcd and provide its coordinates.
 	// If not specified, a brand new instance of Etcd is brought up.
@@ -85,14 +80,12 @@ func (s *APIServer) URL() (string, error) {
 
 // Start starts the apiserver, waits for it to come up, and returns an error, if occoured.
 func (s *APIServer) Start() error {
-	s.ensureInitialized()
+	err := s.ensureInitialized()
-
-	port, addr, err := s.AddressManager.Initialize()
 	if err != nil {
 		return err
 	}
 
-	certDir, err := s.CertDirManager.Create()
+	port, addr, err := s.AddressManager.Initialize()
 	if err != nil {
 		return err
 	}
@@ -119,7 +112,7 @@ func (s *APIServer) Start() error {
 		"--bind-address=0.0.0.0",
 		"--storage-backend=etcd3",
 		fmt.Sprintf("--etcd-servers=%s", etcdURLString),
-		fmt.Sprintf("--cert-dir=%s", certDir),
+		fmt.Sprintf("--cert-dir=%s", s.CertDir.Path),
 		fmt.Sprintf("--insecure-port=%d", port),
 		fmt.Sprintf("--insecure-bind-address=%s", addr),
 	}
@@ -141,7 +134,7 @@ func (s *APIServer) Start() error {
 	}
 }
 
-func (s *APIServer) ensureInitialized() {
+func (s *APIServer) ensureInitialized() error {
 	if s.Path == "" {
 		s.Path = DefaultBinPathFinder("kube-apiserver")
 	}
@@ -153,8 +146,12 @@ func (s *APIServer) ensureInitialized() {
 			return gexec.Start(command, out, err)
 		}
 	}
-	if s.CertDirManager == nil {
+	if s.CertDir == nil {
-		s.CertDirManager = NewTempDirManager()
+		certDir, err := newCertDir()
+		if err != nil {
+			return err
+		}
+		s.CertDir = certDir
 	}
 	if s.Etcd == nil {
 		s.Etcd = &Etcd{}
@@ -168,6 +165,8 @@ func (s *APIServer) ensureInitialized() {
 
 	s.stdOut = gbytes.NewBuffer()
 	s.stdErr = gbytes.NewBuffer()
+
+	return nil
 }
 
 // Stop stops this process gracefully, waits for its termination, and cleans up the cert directory.
@@ -191,7 +190,10 @@ func (s *APIServer) Stop() error {
 		return err
 	}
 
-	return s.CertDirManager.Destroy()
+	if s.CertDir.Cleanup == nil {
+		return nil
+	}
+	return s.CertDir.Cleanup()
 }
 
 // ExitCode returns the exit code of the process, if it has exited. If it hasn't exited yet, ExitCode returns -1.

pkg/framework/test/apiserver_test.go

@@ -20,16 +20,15 @@ import (
 var _ = Describe("Apiserver", func() {
 	var (
 		fakeSession        *testfakes.FakeSimpleSession
-		fakeCertDirManager *testfakes.FakeCertDirManager
 		apiServer          *APIServer
 		fakeEtcdProcess    *testfakes.FakeControlPlaneProcess
 		fakeAddressManager *testfakes.FakeAddressManager
 		apiServerStopper   chan struct{}
+		cleanupCallCount   int
 	)
 
 	BeforeEach(func() {
 		fakeSession = &testfakes.FakeSimpleSession{}
-		fakeCertDirManager = &testfakes.FakeCertDirManager{}
 		fakeEtcdProcess = &testfakes.FakeControlPlaneProcess{}
 		fakeAddressManager = &testfakes.FakeAddressManager{}
 
@@ -42,9 +41,15 @@ var _ = Describe("Apiserver", func() {
 		apiServer = &APIServer{
 			AddressManager: fakeAddressManager,
 			Path:           "/some/path/to/apiserver",
-			CertDirManager: fakeCertDirManager,
+			CertDir: &CertDir{
-			Etcd:           fakeEtcdProcess,
+				Path: "/some/path/to/certdir",
-			StopTimeout:    500 * time.Millisecond,
+				Cleanup: func() error {
+					cleanupCallCount += 1
+					return nil
+				},
+			},
+			Etcd:        fakeEtcdProcess,
+			StopTimeout: 500 * time.Millisecond,
 		}
 	})
 
@@ -66,6 +71,7 @@ var _ = Describe("Apiserver", func() {
 					Expect(command.Args).To(ContainElement("--insecure-port=1234"))
 					Expect(command.Args).To(ContainElement("--insecure-bind-address=this.is.the.API.server"))
 					Expect(command.Args).To(ContainElement("--etcd-servers=the etcd url"))
+					Expect(command.Args).To(ContainElement("--cert-dir=/some/path/to/certdir"))
 					Expect(command.Path).To(Equal("/some/path/to/apiserver"))
 					fmt.Fprint(err, "Serving insecurely on this.is.the.API.server:1234")
 					return fakeSession, nil
@@ -82,9 +88,6 @@ var _ = Describe("Apiserver", func() {
 				By("...in turn calling the AddressManager")
 				Expect(fakeAddressManager.InitializeCallCount()).To(Equal(1))
 
-				By("...in turn calling the CertDirManager")
-				Expect(fakeCertDirManager.CreateCallCount()).To(Equal(1))
-
 				By("...getting the URL of Etcd")
 				Expect(fakeEtcdProcess.URLCallCount()).To(Equal(1))
 
@@ -92,23 +95,21 @@ var _ = Describe("Apiserver", func() {
 				Expect(fakeSession.ExitCodeCallCount()).To(Equal(0))
 				Expect(apiServer).NotTo(gexec.Exit())
 				Expect(fakeSession.ExitCodeCallCount()).To(Equal(1))
-				Expect(fakeCertDirManager.CreateCallCount()).To(Equal(1))
 
 				By("Stopping the API Server")
 				Expect(apiServer.Stop()).To(Succeed())
 
-				Expect(fakeCertDirManager.DestroyCallCount()).To(Equal(1))
+				Expect(cleanupCallCount).To(Equal(1))
 				Expect(fakeEtcdProcess.StopCallCount()).To(Equal(1))
 				Expect(apiServer).To(gexec.Exit(143))
 				Expect(fakeSession.TerminateCallCount()).To(Equal(1))
 				Expect(fakeSession.ExitCodeCallCount()).To(Equal(2))
-				Expect(fakeCertDirManager.DestroyCallCount()).To(Equal(1))
 			})
 		})
 
 		Context("when the certificate directory cannot be destroyed", func() {
 			It("propagates the error", func() {
-				fakeCertDirManager.DestroyReturns(fmt.Errorf("destroy failed"))
+				apiServer.CertDir.Cleanup = func() error { return fmt.Errorf("destroy failed") }
 				fakeAddressManager.InitializeReturns(1234, "this.is.apiserver", nil)
 				apiServer.ProcessStarter = func(Command *exec.Cmd, out, err io.Writer) (SimpleSession, error) {
 					fmt.Fprint(err, "Serving insecurely on this.is.apiserver:1234")
@@ -121,6 +122,25 @@ var _ = Describe("Apiserver", func() {
 			})
 		})
 
+		Context("when there is on function to cleanup the certificate directory", func() {
+			It("does not panic", func() {
+				apiServer.CertDir.Cleanup = nil
+				fakeAddressManager.InitializeReturns(1234, "this.is.apiserver", nil)
+				apiServer.ProcessStarter = func(Command *exec.Cmd, out, err io.Writer) (SimpleSession, error) {
+					fmt.Fprint(err, "Serving insecurely on this.is.apiserver:1234")
+					return fakeSession, nil
+				}
+
+				Expect(apiServer.Start()).To(Succeed())
+
+				var err error
+				Expect(func() {
+					err = apiServer.Stop()
+				}).NotTo(Panic())
+				Expect(err).NotTo(HaveOccurred())
+			})
+		})
+
 		Context("when etcd cannot be stopped", func() {
 			It("propagates the error", func() {
 				fakeEtcdProcess.StopReturns(fmt.Errorf("stopping etcd failed"))
@@ -184,22 +204,6 @@ var _ = Describe("Apiserver", func() {
 			})
 		})
 
-		Context("when the certificate directory cannot be created", func() {
-			It("propagates the error, and does not start any process", func() {
-				fakeCertDirManager.CreateReturnsOnCall(0, "", fmt.Errorf("Error on cert directory creation."))
-
-				apiServer.ProcessStarter = func(Command *exec.Cmd, out, err io.Writer) (SimpleSession, error) {
-					Expect(true).To(BeFalse(),
-						"the api server process starter shouldn't be called if creating the cert dir fails")
-					return nil, nil
-				}
-
-				err := apiServer.Start()
-				Expect(err).To(MatchError(ContainSubstring("Error on cert directory creation.")))
-				Expect(fakeEtcdProcess.StartCallCount()).To(Equal(0))
-			})
-		})
-
 		Context("when the address manager fails to get a new address", func() {
 			It("propagates the error and does not start any process", func() {
 				fakeAddressManager.InitializeReturns(0, "", fmt.Errorf("some error finding a free port"))

pkg/framework/test/cert_dir.go

@@ -0,0 +1,26 @@
+package test
+
+import (
+	"io/ioutil"
+	"os"
+)
+
+// CertDir holds a path to a directory and knows how to tear down / cleanup that directory
+type CertDir struct {
+	Path    string
+	Cleanup func() error
+}
+
+func newCertDir() (*CertDir, error) {
+	path, err := ioutil.TempDir("", "cert_dir-")
+	if err != nil {
+		return nil, err
+	}
+
+	return &CertDir{
+		Path: path,
+		Cleanup: func() error {
+			return os.RemoveAll(path)
+		},
+	}, nil
+}

pkg/framework/test/cert_dir_test.go

@@ -0,0 +1,16 @@
+package test
+
+import (
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("NewCertDir", func() {
+	It("returns a valid CertDir struct", func() {
+		certDir, err := newCertDir()
+		Expect(err).NotTo(HaveOccurred())
+		Expect(certDir.Path).To(BeADirectory())
+		Expect(certDir.Cleanup()).To(Succeed())
+		Expect(certDir.Path).NotTo(BeAnExistingFile())
+	})
+})

pkg/framework/test/examples_api_server_special_cert_dir_manager_test.go

@@ -1,38 +0,0 @@
-package test_test
-
-import (
-	"fmt"
-
-	"io/ioutil"
-	"os"
-
-	. "k8s.io/kubectl/pkg/framework/test"
-)
-
-func ExampleAPIServer_credHubCertDirManager() {
-	apiServer := &APIServer{
-		CertDirManager: NewCredHubCertDirManager(),
-	}
-	fmt.Println(apiServer)
-}
-
-func credHubLoader(dir string, prefix string) (string, error) {
-	tempDir, _ := ioutil.TempDir("/var/cache/kube/cert", prefix+"cred-hub-")
-	loadCertsFromCredHub(tempDir)
-	return tempDir, nil
-}
-
-func credHubSaver(tempDir string) error {
-	saveCertsToCredHub(tempDir)
-	return os.RemoveAll(tempDir)
-}
-
-func NewCredHubCertDirManager() *TempDirManager {
-	return &TempDirManager{
-		Maker:   credHubLoader,
-		Remover: credHubSaver,
-	}
-}
-
-func loadCertsFromCredHub(dir string) { /* to de implemented */ }
-func saveCertsToCredHub(dir string)   { /* to be implemented */ }

pkg/framework/test/testfakes/fake_cert_dir_manager.go

@@ -1,140 +0,0 @@
-// Code generated by counterfeiter. DO NOT EDIT.
-package testfakes
-
-import (
-	"sync"
-)
-
-type FakeCertDirManager struct {
-	CreateStub        func() (string, error)
-	createMutex       sync.RWMutex
-	createArgsForCall []struct{}
-	createReturns     struct {
-		result1 string
-		result2 error
-	}
-	createReturnsOnCall map[int]struct {
-		result1 string
-		result2 error
-	}
-	DestroyStub        func() error
-	destroyMutex       sync.RWMutex
-	destroyArgsForCall []struct{}
-	destroyReturns     struct {
-		result1 error
-	}
-	destroyReturnsOnCall map[int]struct {
-		result1 error
-	}
-	invocations      map[string][][]interface{}
-	invocationsMutex sync.RWMutex
-}
-
-func (fake *FakeCertDirManager) Create() (string, error) {
-	fake.createMutex.Lock()
-	ret, specificReturn := fake.createReturnsOnCall[len(fake.createArgsForCall)]
-	fake.createArgsForCall = append(fake.createArgsForCall, struct{}{})
-	fake.recordInvocation("Create", []interface{}{})
-	fake.createMutex.Unlock()
-	if fake.CreateStub != nil {
-		return fake.CreateStub()
-	}
-	if specificReturn {
-		return ret.result1, ret.result2
-	}
-	return fake.createReturns.result1, fake.createReturns.result2
-}
-
-func (fake *FakeCertDirManager) CreateCallCount() int {
-	fake.createMutex.RLock()
-	defer fake.createMutex.RUnlock()
-	return len(fake.createArgsForCall)
-}
-
-func (fake *FakeCertDirManager) CreateReturns(result1 string, result2 error) {
-	fake.CreateStub = nil
-	fake.createReturns = struct {
-		result1 string
-		result2 error
-	}{result1, result2}
-}
-
-func (fake *FakeCertDirManager) CreateReturnsOnCall(i int, result1 string, result2 error) {
-	fake.CreateStub = nil
-	if fake.createReturnsOnCall == nil {
-		fake.createReturnsOnCall = make(map[int]struct {
-			result1 string
-			result2 error
-		})
-	}
-	fake.createReturnsOnCall[i] = struct {
-		result1 string
-		result2 error
-	}{result1, result2}
-}
-
-func (fake *FakeCertDirManager) Destroy() error {
-	fake.destroyMutex.Lock()
-	ret, specificReturn := fake.destroyReturnsOnCall[len(fake.destroyArgsForCall)]
-	fake.destroyArgsForCall = append(fake.destroyArgsForCall, struct{}{})
-	fake.recordInvocation("Destroy", []interface{}{})
-	fake.destroyMutex.Unlock()
-	if fake.DestroyStub != nil {
-		return fake.DestroyStub()
-	}
-	if specificReturn {
-		return ret.result1
-	}
-	return fake.destroyReturns.result1
-}
-
-func (fake *FakeCertDirManager) DestroyCallCount() int {
-	fake.destroyMutex.RLock()
-	defer fake.destroyMutex.RUnlock()
-	return len(fake.destroyArgsForCall)
-}
-
-func (fake *FakeCertDirManager) DestroyReturns(result1 error) {
-	fake.DestroyStub = nil
-	fake.destroyReturns = struct {
-		result1 error
-	}{result1}
-}
-
-func (fake *FakeCertDirManager) DestroyReturnsOnCall(i int, result1 error) {
-	fake.DestroyStub = nil
-	if fake.destroyReturnsOnCall == nil {
-		fake.destroyReturnsOnCall = make(map[int]struct {
-			result1 error
-		})
-	}
-	fake.destroyReturnsOnCall[i] = struct {
-		result1 error
-	}{result1}
-}
-
-func (fake *FakeCertDirManager) Invocations() map[string][][]interface{} {
-	fake.invocationsMutex.RLock()
-	defer fake.invocationsMutex.RUnlock()
-	fake.createMutex.RLock()
-	defer fake.createMutex.RUnlock()
-	fake.destroyMutex.RLock()
-	defer fake.destroyMutex.RUnlock()
-	copiedInvocations := map[string][][]interface{}{}
-	for key, value := range fake.invocations {
-		copiedInvocations[key] = value
-	}
-	return copiedInvocations
-}
-
-func (fake *FakeCertDirManager) recordInvocation(key string, args []interface{}) {
-	fake.invocationsMutex.Lock()
-	defer fake.invocationsMutex.Unlock()
-	if fake.invocations == nil {
-		fake.invocations = map[string][][]interface{}{}
-	}
-	if fake.invocations[key] == nil {
-		fake.invocations[key] = [][]interface{}{}
-	}
-	fake.invocations[key] = append(fake.invocations[key], args)
-}