From d4e9e90d867bd79614157eb3f1176e3410d042ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20H=C3=B6rl?= Date: Mon, 11 Dec 2017 17:18:25 +0000 Subject: [PATCH 1/4] Move etcd path logic from constructor to Start() This means that if you want to customize the path to your etcd, instead of doing `etcd.Path = "/my/path"` you should do: ``` etcd.PathFinder = func(_ string) string { return "/my/path" } ``` The advantage of this is that we move logic out of the constructor, so we need less crazy dependancy injection logic in our tests, and we get closer to being able to use the 0-value Etcd struct. --- pkg/framework/test/bin_path_finder.go | 2 + pkg/framework/test/etcd.go | 27 ++--- pkg/framework/test/etcd_constructor_test.go | 28 ------ pkg/framework/test/etcd_test.go | 10 +- .../test/testfakes/fake_bin_path_finder.go | 98 +++++++++++++++++++ 5 files changed, 115 insertions(+), 50 deletions(-) delete mode 100644 pkg/framework/test/etcd_constructor_test.go create mode 100644 pkg/framework/test/testfakes/fake_bin_path_finder.go diff --git a/pkg/framework/test/bin_path_finder.go b/pkg/framework/test/bin_path_finder.go index f6ad7965b..53a55d37e 100644 --- a/pkg/framework/test/bin_path_finder.go +++ b/pkg/framework/test/bin_path_finder.go @@ -22,6 +22,8 @@ func init() { // a resolvable path to the binary in question type BinPathFinder func(symbolicName string) (binPath string) +//go:generate counterfeiter . BinPathFinder + // DefaultBinPathFinder is an implementation of BinPathFinder which checks the an environment // variable, derived from the symbolic name, and falls back to a default assets location when // this variable is not set diff --git a/pkg/framework/test/etcd.go b/pkg/framework/test/etcd.go index 505d8bb5c..7efec1354 100644 --- a/pkg/framework/test/etcd.go +++ b/pkg/framework/test/etcd.go @@ -15,7 +15,7 @@ import ( // Etcd knows how to run an etcd server. Set it up with the path to a precompiled binary. type Etcd struct { - Path string + PathFinder BinPathFinder ProcessStarter simpleSessionStarter DataDirManager dataDirManager Config *EtcdConfig @@ -43,8 +43,6 @@ type SimpleSession interface { type simpleSessionStarter func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error) -var etcdBinPathFinder = DefaultBinPathFinder - // NewEtcd returns a Etcd process configured with sane defaults func NewEtcd() (*Etcd, error) { starter := func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error) { @@ -57,29 +55,12 @@ func NewEtcd() (*Etcd, error) { } return &Etcd{ - Path: etcdBinPathFinder("etcd"), ProcessStarter: starter, DataDirManager: NewTempDirManager(), Config: config, }, nil } -// NewEtcdWithBinaryAndConfig returns a Etcd process, using the handed in binary and config. -func NewEtcdWithBinaryAndConfig(pathToEtcd string, config *EtcdConfig) *Etcd { - starter := func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error) { - return gexec.Start(command, out, err) - } - - etcd := &Etcd{ - Path: pathToEtcd, - ProcessStarter: starter, - DataDirManager: NewTempDirManager(), - Config: config, - } - - return etcd -} - // URL returns the URL Etcd is listening on. Clients can use this to connect to Etcd. func (e *Etcd) URL() string { return e.Config.ClientURL @@ -87,6 +68,10 @@ func (e *Etcd) URL() string { // Start starts the etcd, waits for it to come up, and returns an error, if occoured. func (e *Etcd) Start() error { + if e.PathFinder == nil { + e.PathFinder = DefaultBinPathFinder + } + if err := e.Config.Validate(); err != nil { return err } @@ -120,7 +105,7 @@ func (e *Etcd) Start() error { "serving insecure client requests on %s", clientURL.Host)) timedOut := time.After(20 * time.Second) - command := exec.Command(e.Path, args...) + command := exec.Command(e.PathFinder("etcd"), args...) e.session, err = e.ProcessStarter(command, e.stdOut, e.stdErr) if err != nil { return err diff --git a/pkg/framework/test/etcd_constructor_test.go b/pkg/framework/test/etcd_constructor_test.go deleted file mode 100644 index 38f4d9bb9..000000000 --- a/pkg/framework/test/etcd_constructor_test.go +++ /dev/null @@ -1,28 +0,0 @@ -package test - -import ( - . "github.com/onsi/ginkgo" - . "github.com/onsi/gomega" -) - -var _ = Describe("Etcd", func() { - Context("when constructed with the zero-config constructor", func() { - var ( - previousBinPathFinder BinPathFinder - ) - BeforeEach(func() { - previousBinPathFinder = etcdBinPathFinder - etcdBinPathFinder = func(name string) (binPath string) { - return "/the/path/to/etcd" - } - }) - AfterEach(func() { - etcdBinPathFinder = previousBinPathFinder - }) - It("gets a sensible path", func() { - etcd, err := NewEtcd() - Expect(err).NotTo(HaveOccurred()) - Expect(etcd.Path).To(Equal("/the/path/to/etcd")) - }) - }) -}) diff --git a/pkg/framework/test/etcd_test.go b/pkg/framework/test/etcd_test.go index c9cd78ff5..16f169952 100644 --- a/pkg/framework/test/etcd_test.go +++ b/pkg/framework/test/etcd_test.go @@ -17,6 +17,7 @@ var _ = Describe("Etcd", func() { var ( fakeSession *testfakes.FakeSimpleSession fakeDataDirManager *testfakes.FakeDataDirManager + fakePathFinder *testfakes.FakeBinPathFinder etcd *Etcd etcdConfig *EtcdConfig ) @@ -24,6 +25,7 @@ var _ = Describe("Etcd", func() { BeforeEach(func() { fakeSession = &testfakes.FakeSimpleSession{} fakeDataDirManager = &testfakes.FakeDataDirManager{} + fakePathFinder = &testfakes.FakeBinPathFinder{} etcdConfig = &EtcdConfig{ ClientURL: "http://this.is.etcd.listening.for.clients:1234", @@ -31,7 +33,7 @@ var _ = Describe("Etcd", func() { } etcd = &Etcd{ - Path: "", + PathFinder: fakePathFinder.Spy, DataDirManager: fakeDataDirManager, Config: etcdConfig, } @@ -49,8 +51,10 @@ var _ = Describe("Etcd", func() { fakeSession.ExitCodeReturnsOnCall(0, -1) fakeSession.ExitCodeReturnsOnCall(1, 143) + fakePathFinder.ReturnsOnCall(0, "/path/to/some/etcd") etcd.ProcessStarter = func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error) { + Expect(command.Path).To(Equal("/path/to/some/etcd")) fmt.Fprint(err, "serving insecure client requests on this.is.etcd.listening.for.clients:1234") return fakeSession, nil } @@ -59,6 +63,10 @@ var _ = Describe("Etcd", func() { err := etcd.Start() Expect(err).NotTo(HaveOccurred()) + By("...in turn calling the PathFinder") + Expect(fakePathFinder.CallCount()).To(Equal(1)) + Expect(fakePathFinder.ArgsForCall(0)).To(Equal("etcd")) + Eventually(etcd).Should(gbytes.Say("Everything is dandy")) Expect(fakeSession.ExitCodeCallCount()).To(Equal(0)) Expect(etcd).NotTo(gexec.Exit()) diff --git a/pkg/framework/test/testfakes/fake_bin_path_finder.go b/pkg/framework/test/testfakes/fake_bin_path_finder.go new file mode 100644 index 000000000..4503dcb91 --- /dev/null +++ b/pkg/framework/test/testfakes/fake_bin_path_finder.go @@ -0,0 +1,98 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package testfakes + +import ( + "sync" + + "k8s.io/kubectl/pkg/framework/test" +) + +type FakeBinPathFinder struct { + Stub func(symbolicName string) (binPath string) + mutex sync.RWMutex + argsForCall []struct { + symbolicName string + } + returns struct { + result1 string + } + returnsOnCall map[int]struct { + result1 string + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeBinPathFinder) Spy(symbolicName string) (binPath string) { + fake.mutex.Lock() + ret, specificReturn := fake.returnsOnCall[len(fake.argsForCall)] + fake.argsForCall = append(fake.argsForCall, struct { + symbolicName string + }{symbolicName}) + fake.recordInvocation("BinPathFinder", []interface{}{symbolicName}) + fake.mutex.Unlock() + if fake.Stub != nil { + return fake.Stub(symbolicName) + } + if specificReturn { + return ret.result1 + } + return fake.returns.result1 +} + +func (fake *FakeBinPathFinder) CallCount() int { + fake.mutex.RLock() + defer fake.mutex.RUnlock() + return len(fake.argsForCall) +} + +func (fake *FakeBinPathFinder) ArgsForCall(i int) string { + fake.mutex.RLock() + defer fake.mutex.RUnlock() + return fake.argsForCall[i].symbolicName +} + +func (fake *FakeBinPathFinder) Returns(result1 string) { + fake.Stub = nil + fake.returns = struct { + result1 string + }{result1} +} + +func (fake *FakeBinPathFinder) ReturnsOnCall(i int, result1 string) { + fake.Stub = nil + if fake.returnsOnCall == nil { + fake.returnsOnCall = make(map[int]struct { + result1 string + }) + } + fake.returnsOnCall[i] = struct { + result1 string + }{result1} +} + +func (fake *FakeBinPathFinder) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.mutex.RLock() + defer fake.mutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeBinPathFinder) 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) +} + +var _ test.BinPathFinder = new(FakeBinPathFinder).Spy From d62ff0422837e85b2991881f29895122d9fac71b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20H=C3=B6rl?= Date: Tue, 12 Dec 2017 11:11:02 +0000 Subject: [PATCH 2/4] Move APIServer path logic into Start() Same as in previous commit for Etcd ( d4e9e90d867bd79614157eb3f1176e3410d042ba ) --- pkg/framework/test/apiserver.go | 11 +++++------ pkg/framework/test/apiserver_constructor_test.go | 13 ------------- pkg/framework/test/apiserver_test.go | 11 ++++++++++- 3 files changed, 15 insertions(+), 20 deletions(-) diff --git a/pkg/framework/test/apiserver.go b/pkg/framework/test/apiserver.go index bc486e1f1..93ff0f2cc 100644 --- a/pkg/framework/test/apiserver.go +++ b/pkg/framework/test/apiserver.go @@ -14,8 +14,7 @@ import ( // APIServer knows how to run a kubernetes apiserver. Set it up with the path to a precompiled binary. type APIServer struct { - // The path to the apiserver binary - Path string + PathFinder BinPathFinder ProcessStarter simpleSessionStarter CertDirManager certDirManager Config *APIServerConfig @@ -32,8 +31,6 @@ type certDirManager interface { //go:generate counterfeiter . certDirManager -var apiServerBinPathFinder = DefaultBinPathFinder - // NewAPIServer creates a new APIServer Fixture Process func NewAPIServer(config *APIServerConfig) (*APIServer, error) { starter := func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error) { @@ -46,7 +43,6 @@ func NewAPIServer(config *APIServerConfig) (*APIServer, error) { } return &APIServer{ - Path: apiServerBinPathFinder("kube-apiserver"), Config: config, ProcessStarter: starter, CertDirManager: NewTempDirManager(), @@ -61,6 +57,9 @@ func (s *APIServer) URL() string { // Start starts the apiserver, waits for it to come up, and returns an error, if occoured. func (s *APIServer) Start() error { + if s.PathFinder == nil { + s.PathFinder = DefaultBinPathFinder + } if err := s.Config.Validate(); err != nil { return err } @@ -100,7 +99,7 @@ func (s *APIServer) Start() error { detectedStart := s.stdErr.Detect(fmt.Sprintf("Serving insecurely on %s", clientURL.Host)) timedOut := time.After(20 * time.Second) - command := exec.Command(s.Path, args...) + command := exec.Command(s.PathFinder("kube-apiserver"), args...) s.session, err = s.ProcessStarter(command, s.stdOut, s.stdErr) if err != nil { return err diff --git a/pkg/framework/test/apiserver_constructor_test.go b/pkg/framework/test/apiserver_constructor_test.go index 4b73cac8f..a502ffb24 100644 --- a/pkg/framework/test/apiserver_constructor_test.go +++ b/pkg/framework/test/apiserver_constructor_test.go @@ -6,22 +6,10 @@ import ( ) var _ = Describe("NewAPIServer", func() { - var oldAPIServerBinPathFinder BinPathFinder - BeforeEach(func() { - oldAPIServerBinPathFinder = apiServerBinPathFinder - }) - AfterEach(func() { - apiServerBinPathFinder = oldAPIServerBinPathFinder - }) - It("can construct a properly configured APIServer", func() { config := &APIServerConfig{ APIServerURL: "some APIServer URL", } - apiServerBinPathFinder = func(name string) string { - Expect(name).To(Equal("kube-apiserver")) - return "some api server path" - } apiServer, err := NewAPIServer(config) @@ -29,7 +17,6 @@ var _ = Describe("NewAPIServer", func() { Expect(apiServer).NotTo(BeNil()) Expect(apiServer.ProcessStarter).NotTo(BeNil()) Expect(apiServer.CertDirManager).NotTo(BeNil()) - Expect(apiServer.Path).To(Equal("some api server path")) Expect(apiServer.Etcd).NotTo(BeNil()) Expect(apiServer.Config).To(Equal(config)) }) diff --git a/pkg/framework/test/apiserver_test.go b/pkg/framework/test/apiserver_test.go index 569e87309..53cfc0078 100644 --- a/pkg/framework/test/apiserver_test.go +++ b/pkg/framework/test/apiserver_test.go @@ -22,18 +22,20 @@ var _ = Describe("Apiserver", func() { apiServer *APIServer apiServerConfig *APIServerConfig fakeEtcdProcess *testfakes.FakeFixtureProcess + fakePathFinder *testfakes.FakeBinPathFinder ) BeforeEach(func() { fakeSession = &testfakes.FakeSimpleSession{} fakeCertDirManager = &testfakes.FakeCertDirManager{} fakeEtcdProcess = &testfakes.FakeFixtureProcess{} + fakePathFinder = &testfakes.FakeBinPathFinder{} apiServerConfig = &APIServerConfig{ APIServerURL: "http://this.is.the.API.server:8080", } apiServer = &APIServer{ - Path: "", + PathFinder: fakePathFinder.Spy, CertDirManager: fakeCertDirManager, Config: apiServerConfig, Etcd: fakeEtcdProcess, @@ -53,7 +55,10 @@ var _ = Describe("Apiserver", func() { fakeSession.ExitCodeReturnsOnCall(0, -1) fakeSession.ExitCodeReturnsOnCall(1, 143) + fakePathFinder.Returns("/some/path/to/apiserver") + apiServer.ProcessStarter = func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error) { + Expect(command.Path).To(Equal("/some/path/to/apiserver")) fmt.Fprint(err, "Serving insecurely on this.is.the.API.server:8080") return fakeSession, nil } @@ -66,6 +71,10 @@ var _ = Describe("Apiserver", func() { Expect(fakeEtcdProcess.StartCallCount()).To(Equal(1), "the Etcd process should be started exactly once") + By("...in turn calls the PathFinder") + Expect(fakePathFinder.CallCount()).To(Equal(1)) + Expect(fakePathFinder.ArgsForCall(0)).To(Equal("kube-apiserver")) + Eventually(apiServer).Should(gbytes.Say("Everything is fine")) Expect(fakeSession.ExitCodeCallCount()).To(Equal(0)) Expect(apiServer).NotTo(gexec.Exit()) From 657963b319f6cce845ea666db080e869f41259fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20H=C3=B6rl?= Date: Tue, 12 Dec 2017 15:46:53 +0000 Subject: [PATCH 3/4] Make APIServer manage its own port allocations This means we will no longer need to pass a free port into the APIServer constructor. --- pkg/framework/test/apiserver.go | 26 ++- pkg/framework/test/apiserver_config.go | 2 +- pkg/framework/test/apiserver_config_test.go | 6 - pkg/framework/test/apiserver_test.go | 201 ++++++++++------ pkg/framework/test/port_finder.go | 59 +++++ pkg/framework/test/port_finder_test.go | 80 +++++++ .../test/testfakes/fake_address_manager.go | 220 ++++++++++++++++++ 7 files changed, 501 insertions(+), 93 deletions(-) create mode 100644 pkg/framework/test/testfakes/fake_address_manager.go diff --git a/pkg/framework/test/apiserver.go b/pkg/framework/test/apiserver.go index 93ff0f2cc..0b0077798 100644 --- a/pkg/framework/test/apiserver.go +++ b/pkg/framework/test/apiserver.go @@ -3,7 +3,6 @@ package test import ( "fmt" "io" - "net/url" "os/exec" "time" @@ -14,6 +13,7 @@ import ( // APIServer knows how to run a kubernetes apiserver. Set it up with the path to a precompiled binary. type APIServer struct { + AddressManager AddressManager PathFinder BinPathFinder ProcessStarter simpleSessionStarter CertDirManager certDirManager @@ -52,7 +52,10 @@ func NewAPIServer(config *APIServerConfig) (*APIServer, error) { // URL returns the URL APIServer is listening on. Clients can use this to connect to APIServer. func (s *APIServer) URL() string { - return s.Config.APIServerURL + // TODO handle errors + port, _ := s.AddressManager.Port() + host, _ := s.AddressManager.Host() + return fmt.Sprintf("http://%s:%d", host, port) } // Start starts the apiserver, waits for it to come up, and returns an error, if occoured. @@ -60,28 +63,31 @@ func (s *APIServer) Start() error { if s.PathFinder == nil { s.PathFinder = DefaultBinPathFinder } + if s.AddressManager == nil { + s.AddressManager = &DefaultAddressManager{} + } if err := s.Config.Validate(); err != nil { return err } - err := s.Etcd.Start() + port, addr, err := s.AddressManager.Initialize("localhost") if err != nil { return err } - s.stdOut = gbytes.NewBuffer() - s.stdErr = gbytes.NewBuffer() - certDir, err := s.CertDirManager.Create() if err != nil { return err } - clientURL, err := url.Parse(s.Config.APIServerURL) + err = s.Etcd.Start() if err != nil { return err } + s.stdOut = gbytes.NewBuffer() + s.stdErr = gbytes.NewBuffer() + args := []string{ "--authorization-mode=Node,RBAC", "--runtime-config=admissionregistration.k8s.io/v1alpha1", @@ -92,11 +98,11 @@ func (s *APIServer) Start() error { "--storage-backend=etcd3", fmt.Sprintf("--etcd-servers=%s", s.Etcd.URL()), fmt.Sprintf("--cert-dir=%s", certDir), - fmt.Sprintf("--insecure-port=%s", clientURL.Port()), - fmt.Sprintf("--insecure-bind-address=%s", clientURL.Hostname()), + fmt.Sprintf("--insecure-port=%d", port), + fmt.Sprintf("--insecure-bind-address=%s", addr), } - detectedStart := s.stdErr.Detect(fmt.Sprintf("Serving insecurely on %s", clientURL.Host)) + detectedStart := s.stdErr.Detect(fmt.Sprintf("Serving insecurely on %s:%d", addr, port)) timedOut := time.After(20 * time.Second) command := exec.Command(s.PathFinder("kube-apiserver"), args...) diff --git a/pkg/framework/test/apiserver_config.go b/pkg/framework/test/apiserver_config.go index 19ae5e694..7d9fbd742 100644 --- a/pkg/framework/test/apiserver_config.go +++ b/pkg/framework/test/apiserver_config.go @@ -4,7 +4,7 @@ import "github.com/asaskevich/govalidator" // APIServerConfig is a struct holding data to configure the API Server process type APIServerConfig struct { - APIServerURL string `valid:"required,url"` + APIServerURL string `valid:"url"` } // Validate checks that the config contains only valid URLs diff --git a/pkg/framework/test/apiserver_config_test.go b/pkg/framework/test/apiserver_config_test.go index ca4a16708..d32865025 100644 --- a/pkg/framework/test/apiserver_config_test.go +++ b/pkg/framework/test/apiserver_config_test.go @@ -16,12 +16,6 @@ var _ = Describe("APIServerConfig", func() { Expect(err).NotTo(HaveOccurred()) }) - It("errors on empty config", func() { - conf := &APIServerConfig{} - err := conf.Validate() - Expect(err).To(MatchError(ContainSubstring("APIServerURL: non zero value required"))) - }) - It("errors on malformed URLs", func() { conf := &APIServerConfig{ APIServerURL: "something not URLish", diff --git a/pkg/framework/test/apiserver_test.go b/pkg/framework/test/apiserver_test.go index 53cfc0078..a889fb4a6 100644 --- a/pkg/framework/test/apiserver_test.go +++ b/pkg/framework/test/apiserver_test.go @@ -23,6 +23,7 @@ var _ = Describe("Apiserver", func() { apiServerConfig *APIServerConfig fakeEtcdProcess *testfakes.FakeFixtureProcess fakePathFinder *testfakes.FakeBinPathFinder + fakeAddressManager *testfakes.FakeAddressManager ) BeforeEach(func() { @@ -30,11 +31,13 @@ var _ = Describe("Apiserver", func() { fakeCertDirManager = &testfakes.FakeCertDirManager{} fakeEtcdProcess = &testfakes.FakeFixtureProcess{} fakePathFinder = &testfakes.FakeBinPathFinder{} + fakeAddressManager = &testfakes.FakeAddressManager{} apiServerConfig = &APIServerConfig{ - APIServerURL: "http://this.is.the.API.server:8080", + APIServerURL: "", } apiServer = &APIServer{ + AddressManager: fakeAddressManager, PathFinder: fakePathFinder.Spy, CertDirManager: fakeCertDirManager, Config: apiServerConfig, @@ -42,104 +45,150 @@ var _ = Describe("Apiserver", func() { } }) - It("can be queried for the URL it listens on", func() { - Expect(apiServer.URL()).To(Equal("http://this.is.the.API.server:8080")) - }) + Describe("starting and stopping the server", func() { + Context("when given a path to a binary that runs for a long time", func() { + It("can start and stop that binary", func() { + sessionBuffer := gbytes.NewBuffer() + fmt.Fprint(sessionBuffer, "Everything is fine") + fakeSession.BufferReturns(sessionBuffer) - Context("when given a path to a binary that runs for a long time", func() { - It("can start and stop that binary", func() { - sessionBuffer := gbytes.NewBuffer() - fmt.Fprint(sessionBuffer, "Everything is fine") - fakeSession.BufferReturns(sessionBuffer) + fakeSession.ExitCodeReturnsOnCall(0, -1) + fakeSession.ExitCodeReturnsOnCall(1, 143) - fakeSession.ExitCodeReturnsOnCall(0, -1) - fakeSession.ExitCodeReturnsOnCall(1, 143) + fakePathFinder.Returns("/some/path/to/apiserver") + fakeAddressManager.InitializeReturns(1234, "this.is.the.API.server", nil) - fakePathFinder.Returns("/some/path/to/apiserver") + apiServer.ProcessStarter = func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error) { + Expect(command.Args).To(ContainElement("--insecure-port=1234")) + Expect(command.Args).To(ContainElement("--insecure-bind-address=this.is.the.API.server")) + Expect(command.Path).To(Equal("/some/path/to/apiserver")) + fmt.Fprint(err, "Serving insecurely on this.is.the.API.server:1234") + return fakeSession, nil + } - apiServer.ProcessStarter = func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error) { - Expect(command.Path).To(Equal("/some/path/to/apiserver")) - fmt.Fprint(err, "Serving insecurely on this.is.the.API.server:8080") - return fakeSession, nil - } + By("Starting the API Server") + err := apiServer.Start() + Expect(err).NotTo(HaveOccurred()) - By("Starting the API Server") - err := apiServer.Start() - Expect(err).NotTo(HaveOccurred()) + By("...in turn starting Etcd") + Expect(fakeEtcdProcess.StartCallCount()).To(Equal(1), + "the Etcd process should be started exactly once") - By("starting Etcd") - Expect(fakeEtcdProcess.StartCallCount()).To(Equal(1), - "the Etcd process should be started exactly once") + By("...in turn calling the PathFinder") + Expect(fakePathFinder.CallCount()).To(Equal(1)) + Expect(fakePathFinder.ArgsForCall(0)).To(Equal("kube-apiserver")) - By("...in turn calls the PathFinder") - Expect(fakePathFinder.CallCount()).To(Equal(1)) - Expect(fakePathFinder.ArgsForCall(0)).To(Equal("kube-apiserver")) + By("...in turn calling the PortFinder") + Expect(fakeAddressManager.InitializeCallCount()).To(Equal(1)) + Expect(fakeAddressManager.InitializeArgsForCall(0)).To(Equal("localhost")) - Eventually(apiServer).Should(gbytes.Say("Everything is fine")) - Expect(fakeSession.ExitCodeCallCount()).To(Equal(0)) - Expect(apiServer).NotTo(gexec.Exit()) - Expect(fakeSession.ExitCodeCallCount()).To(Equal(1)) - Expect(fakeCertDirManager.CreateCallCount()).To(Equal(1)) + Eventually(apiServer).Should(gbytes.Say("Everything is fine")) + 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") - apiServer.Stop() + By("Stopping the API Server") + apiServer.Stop() - Expect(fakeEtcdProcess.StopCallCount()).To(Equal(1)) - Expect(apiServer).To(gexec.Exit(143)) - Expect(fakeSession.TerminateCallCount()).To(Equal(1)) - Expect(fakeSession.WaitCallCount()).To(Equal(1)) - Expect(fakeSession.ExitCodeCallCount()).To(Equal(2)) - Expect(fakeCertDirManager.DestroyCallCount()).To(Equal(1)) + Expect(fakeEtcdProcess.StopCallCount()).To(Equal(1)) + Expect(apiServer).To(gexec.Exit(143)) + Expect(fakeSession.TerminateCallCount()).To(Equal(1)) + Expect(fakeSession.WaitCallCount()).To(Equal(1)) + Expect(fakeSession.ExitCodeCallCount()).To(Equal(2)) + Expect(fakeCertDirManager.DestroyCallCount()).To(Equal(1)) + }) }) - }) - Context("when starting etcd fails", func() { - It("propagates the error, and does not start the process", func() { - fakeEtcdProcess.StartReturnsOnCall(0, fmt.Errorf("starting etcd failed")) - 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 starting etcd fails") - return nil, nil - } + Context("when starting etcd fails", func() { + It("propagates the error, and does not start the process", func() { + fakeEtcdProcess.StartReturnsOnCall(0, fmt.Errorf("starting etcd failed")) + 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 starting etcd fails") + return nil, nil + } - err := apiServer.Start() - Expect(err).To(MatchError(ContainSubstring("starting etcd failed"))) + err := apiServer.Start() + Expect(err).To(MatchError(ContainSubstring("starting etcd failed"))) + }) }) - }) - Context("when the certificate directory cannot be created", func() { - It("propagates the error, and does not start the process", func() { - fakeCertDirManager.CreateReturnsOnCall(0, "", fmt.Errorf("Error on cert directory creation.")) + 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 - } + 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."))) + err := apiServer.Start() + Expect(err).To(MatchError(ContainSubstring("Error on cert directory creation."))) + Expect(fakeEtcdProcess.StartCallCount()).To(Equal(0)) + }) }) - }) - Context("when the starter returns an error", func() { - It("propagates the error", func() { - apiServer.ProcessStarter = func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error) { - return nil, fmt.Errorf("Some error in the apiserver starter.") - } + 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")) - err := apiServer.Start() - Expect(err).To(MatchError(ContainSubstring("Some error in the apiserver starter."))) + 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 getting a free port fails") + return nil, nil + } + + Expect(apiServer.Start()).To(MatchError(ContainSubstring("some error finding a free port"))) + Expect(fakeEtcdProcess.StartCallCount()).To(Equal(0)) + }) }) + + Context("when the starter returns an error", func() { + It("propagates the error", func() { + apiServer.ProcessStarter = func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error) { + return nil, fmt.Errorf("Some error in the apiserver starter.") + } + + err := apiServer.Start() + Expect(err).To(MatchError(ContainSubstring("Some error in the apiserver starter."))) + }) + }) + + Context("when we try to stop a server that hasn't been started", func() { + It("is a noop and does not call exit on the session", func() { + apiServer.ProcessStarter = func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error) { + return fakeSession, nil + } + apiServer.Stop() + Expect(fakeSession.ExitCodeCallCount()).To(Equal(0)) + }) + }) + }) - Context("when we try to stop a server that hasn't been started", func() { - It("is a noop and does not call exit on the session", func() { - apiServer.ProcessStarter = func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error) { - return fakeSession, nil - } - apiServer.Stop() - Expect(fakeSession.ExitCodeCallCount()).To(Equal(0)) + Describe("querying the server for its URL", func() { + It("can be queried for the URL it listens on", func() { + fakeAddressManager.HostReturns("the.host.for.api.server", nil) + fakeAddressManager.PortReturns(5678, nil) + Expect(apiServer.URL()).To(Equal("http://the.host.for.api.server:5678")) + }) + + Context("when we query for the URL before starting the server", func() { + + Context("and so the addressmanager fails to give us a port", func() { + It("propagates the failure", func() { + //TODO + }) + }) + + Context("and so the addressmanager fails to give us a host", func() { + It("propagates the failure", func() { + //TODO + }) + }) + }) }) }) diff --git a/pkg/framework/test/port_finder.go b/pkg/framework/test/port_finder.go index 233ae00b7..f4fd09654 100644 --- a/pkg/framework/test/port_finder.go +++ b/pkg/framework/test/port_finder.go @@ -5,11 +5,70 @@ import ( "net" ) +// AddressManager knows how to generate and remember a single address on some +// local interface for a service to listen on. +type AddressManager interface { + Initialize(host string) (port int, resolvedAddress string, err error) + Host() (string, error) + Port() (int, error) +} + +//go:generate counterfeiter . AddressManager + +// DefaultAddressManager implements an AddressManager. It allocates a new address +// (interface & port) a process can bind and keeps track of that. +type DefaultAddressManager struct { + port int + host string +} + +// Initialize returns a address a process can listen on. Given a hostname it returns an address, +// a tuple consisting of a free port and the hostname resolved to its IP. +func (d *DefaultAddressManager) Initialize(host string) (port int, resolvedHost string, err error) { + if d.port != 0 { + return 0, "", fmt.Errorf("this DefaultAddressManager is already initialized") + } + addr, err := net.ResolveTCPAddr("tcp", fmt.Sprintf("%s:0", host)) + if err != nil { + return + } + l, err := net.ListenTCP("tcp", addr) + if err != nil { + return + } + d.port = l.Addr().(*net.TCPAddr).Port + defer func() { + err = l.Close() + }() + d.host = addr.IP.String() + return d.port, d.host, nil +} + +// Port returns the port that this DefaultAddressManager is managing. Port returns an +// error if this DefaultAddressManager has not yet been initialized. +func (d *DefaultAddressManager) Port() (int, error) { + if d.port == 0 { + return 0, fmt.Errorf("this DefaultAdressManager has is not initialized yet") + } + return d.port, nil +} + +// Host returns the host that this DefaultAddressManager is managing. Host returns an +// error if this DefaultAddressManager has not yet been initialized. +func (d *DefaultAddressManager) Host() (string, error) { + if d.host == "" { + return "", fmt.Errorf("this DefaultAdressManager has is not initialized yet") + } + return d.host, nil +} + // PortFinder is the signature of a function returning a free port and a resolved // address to listen/bind on, and an erro in case there were some problems finding // a free pair of address & port type PortFinder func(host string) (port int, resolvedAddress string, err error) +//go:generate counterfeiter . PortFinder + // DefaultPortFinder is the default implementation of PortFinder: It resolves that // hostname or IP handed in and asks the kernel for a random port. To make that a bit // safer, it also tries to bind to that port to make sure it is not in use. If all goes diff --git a/pkg/framework/test/port_finder_test.go b/pkg/framework/test/port_finder_test.go index faae4208d..f8b98e2ac 100644 --- a/pkg/framework/test/port_finder_test.go +++ b/pkg/framework/test/port_finder_test.go @@ -3,10 +3,90 @@ package test_test import ( . "k8s.io/kubectl/pkg/framework/test" + "fmt" + "net" + . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) +var _ = Describe("DefaultAddressManager", func() { + var defaultAddressManager *DefaultAddressManager + BeforeEach(func() { + defaultAddressManager = &DefaultAddressManager{} + }) + + Describe("Initialize", func() { + It("returns a free port and an address to bind to", func() { + port, host, err := defaultAddressManager.Initialize("localhost") + + Expect(err).NotTo(HaveOccurred()) + Expect(host).To(Equal("127.0.0.1")) + Expect(port).NotTo(Equal(0)) + + addr, err := net.ResolveTCPAddr("tcp", fmt.Sprintf("%s:%d", host, port)) + Expect(err).NotTo(HaveOccurred()) + l, err := net.ListenTCP("tcp", addr) + defer func() { + Expect(l.Close()).To(Succeed()) + }() + Expect(err).NotTo(HaveOccurred()) + }) + + Context("when given an invalid hostname", func() { + It("propagates the error", func() { + _, _, err := defaultAddressManager.Initialize("this is not a hostname") + + Expect(err).To(MatchError(ContainSubstring("no such host"))) + }) + }) + + Context("when given a hostname that we don't have permission to listen on", func() { + It("propagates the error", func() { + _, _, err := defaultAddressManager.Initialize("example.com") + + Expect(err).To(MatchError(ContainSubstring("bind: can't assign requested address"))) + }) + }) + + Context("initialized multiple times", func() { + It("fails", func() { + _, _, err := defaultAddressManager.Initialize("localhost") + Expect(err).NotTo(HaveOccurred()) + _, _, err = defaultAddressManager.Initialize("localhost") + Expect(err).To(MatchError(ContainSubstring("already initialized"))) + }) + }) + }) + Describe("Port", func() { + It("returns an error if Initialize has not been called yet", func() { + _, err := defaultAddressManager.Port() + Expect(err).To(MatchError(ContainSubstring("not initialized yet"))) + }) + It("returns the same port as previously allocated by Initialize", func() { + expectedPort, _, err := defaultAddressManager.Initialize("localhost") + Expect(err).NotTo(HaveOccurred()) + actualPort, err := defaultAddressManager.Port() + Expect(err).NotTo(HaveOccurred()) + Expect(actualPort).To(Equal(expectedPort)) + }) + }) + Describe("Host", func() { + It("returns an error if Initialize has not been called yet", func() { + _, err := defaultAddressManager.Host() + Expect(err).To(MatchError(ContainSubstring("not initialized yet"))) + }) + It("returns the same port as previously allocated by Initialize", func() { + _, expectedHost, err := defaultAddressManager.Initialize("localhost") + Expect(err).NotTo(HaveOccurred()) + actualHost, err := defaultAddressManager.Host() + Expect(err).NotTo(HaveOccurred()) + Expect(actualHost).To(Equal(expectedHost)) + }) + + }) +}) + var _ = Describe("DefaultPortFinder", func() { It("returns a free port and an address to bind to", func() { port, addr, err := DefaultPortFinder("127.0.0.1") diff --git a/pkg/framework/test/testfakes/fake_address_manager.go b/pkg/framework/test/testfakes/fake_address_manager.go new file mode 100644 index 000000000..7c24dfb1a --- /dev/null +++ b/pkg/framework/test/testfakes/fake_address_manager.go @@ -0,0 +1,220 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package testfakes + +import ( + "sync" + + "k8s.io/kubectl/pkg/framework/test" +) + +type FakeAddressManager struct { + InitializeStub func(host string) (port int, resolvedAddress string, err error) + initializeMutex sync.RWMutex + initializeArgsForCall []struct { + host string + } + initializeReturns struct { + result1 int + result2 string + result3 error + } + initializeReturnsOnCall map[int]struct { + result1 int + result2 string + result3 error + } + HostStub func() (string, error) + hostMutex sync.RWMutex + hostArgsForCall []struct{} + hostReturns struct { + result1 string + result2 error + } + hostReturnsOnCall map[int]struct { + result1 string + result2 error + } + PortStub func() (int, error) + portMutex sync.RWMutex + portArgsForCall []struct{} + portReturns struct { + result1 int + result2 error + } + portReturnsOnCall map[int]struct { + result1 int + result2 error + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeAddressManager) Initialize(host string) (port int, resolvedAddress string, err error) { + fake.initializeMutex.Lock() + ret, specificReturn := fake.initializeReturnsOnCall[len(fake.initializeArgsForCall)] + fake.initializeArgsForCall = append(fake.initializeArgsForCall, struct { + host string + }{host}) + fake.recordInvocation("Initialize", []interface{}{host}) + fake.initializeMutex.Unlock() + if fake.InitializeStub != nil { + return fake.InitializeStub(host) + } + if specificReturn { + return ret.result1, ret.result2, ret.result3 + } + return fake.initializeReturns.result1, fake.initializeReturns.result2, fake.initializeReturns.result3 +} + +func (fake *FakeAddressManager) InitializeCallCount() int { + fake.initializeMutex.RLock() + defer fake.initializeMutex.RUnlock() + return len(fake.initializeArgsForCall) +} + +func (fake *FakeAddressManager) InitializeArgsForCall(i int) string { + fake.initializeMutex.RLock() + defer fake.initializeMutex.RUnlock() + return fake.initializeArgsForCall[i].host +} + +func (fake *FakeAddressManager) InitializeReturns(result1 int, result2 string, result3 error) { + fake.InitializeStub = nil + fake.initializeReturns = struct { + result1 int + result2 string + result3 error + }{result1, result2, result3} +} + +func (fake *FakeAddressManager) InitializeReturnsOnCall(i int, result1 int, result2 string, result3 error) { + fake.InitializeStub = nil + if fake.initializeReturnsOnCall == nil { + fake.initializeReturnsOnCall = make(map[int]struct { + result1 int + result2 string + result3 error + }) + } + fake.initializeReturnsOnCall[i] = struct { + result1 int + result2 string + result3 error + }{result1, result2, result3} +} + +func (fake *FakeAddressManager) Host() (string, error) { + fake.hostMutex.Lock() + ret, specificReturn := fake.hostReturnsOnCall[len(fake.hostArgsForCall)] + fake.hostArgsForCall = append(fake.hostArgsForCall, struct{}{}) + fake.recordInvocation("Host", []interface{}{}) + fake.hostMutex.Unlock() + if fake.HostStub != nil { + return fake.HostStub() + } + if specificReturn { + return ret.result1, ret.result2 + } + return fake.hostReturns.result1, fake.hostReturns.result2 +} + +func (fake *FakeAddressManager) HostCallCount() int { + fake.hostMutex.RLock() + defer fake.hostMutex.RUnlock() + return len(fake.hostArgsForCall) +} + +func (fake *FakeAddressManager) HostReturns(result1 string, result2 error) { + fake.HostStub = nil + fake.hostReturns = struct { + result1 string + result2 error + }{result1, result2} +} + +func (fake *FakeAddressManager) HostReturnsOnCall(i int, result1 string, result2 error) { + fake.HostStub = nil + if fake.hostReturnsOnCall == nil { + fake.hostReturnsOnCall = make(map[int]struct { + result1 string + result2 error + }) + } + fake.hostReturnsOnCall[i] = struct { + result1 string + result2 error + }{result1, result2} +} + +func (fake *FakeAddressManager) Port() (int, error) { + fake.portMutex.Lock() + ret, specificReturn := fake.portReturnsOnCall[len(fake.portArgsForCall)] + fake.portArgsForCall = append(fake.portArgsForCall, struct{}{}) + fake.recordInvocation("Port", []interface{}{}) + fake.portMutex.Unlock() + if fake.PortStub != nil { + return fake.PortStub() + } + if specificReturn { + return ret.result1, ret.result2 + } + return fake.portReturns.result1, fake.portReturns.result2 +} + +func (fake *FakeAddressManager) PortCallCount() int { + fake.portMutex.RLock() + defer fake.portMutex.RUnlock() + return len(fake.portArgsForCall) +} + +func (fake *FakeAddressManager) PortReturns(result1 int, result2 error) { + fake.PortStub = nil + fake.portReturns = struct { + result1 int + result2 error + }{result1, result2} +} + +func (fake *FakeAddressManager) PortReturnsOnCall(i int, result1 int, result2 error) { + fake.PortStub = nil + if fake.portReturnsOnCall == nil { + fake.portReturnsOnCall = make(map[int]struct { + result1 int + result2 error + }) + } + fake.portReturnsOnCall[i] = struct { + result1 int + result2 error + }{result1, result2} +} + +func (fake *FakeAddressManager) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.initializeMutex.RLock() + defer fake.initializeMutex.RUnlock() + fake.hostMutex.RLock() + defer fake.hostMutex.RUnlock() + fake.portMutex.RLock() + defer fake.portMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeAddressManager) 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) +} + +var _ test.AddressManager = new(FakeAddressManager) From ffa8ee46e59575e3e9e812ee92835ba65edccfcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20H=C3=B6rl?= Date: Tue, 12 Dec 2017 16:39:28 +0000 Subject: [PATCH 4/4] Give APIServer constructor sane defaults The APIServer constructor previously required careful configuration. Now it takes no arguments, and gives you an APIServer that you can `.Start()`. If you want to configure it, you still can. For example, you can set the environment variable `TEST_ASSET_KUBE_APISERVER` to the path to your apiserver binary, or you can override the PathFinder in go code: ``` myAPIServer := test.NewAPIServer() myAPIServer.PathFinder = func(_ string) string { return "/path/to/my/apiserver/binary" } ``` Previously the responsibility of choosing a port that the APIServer could listen on was left to the caller. Now APIServer delegates that responsibility to an AddressManager. By default you get a random unused port on localhost. If you want to customize that behaviour, you can overwrite the AddressManager: ``` myAPIServer := test.NewAPIServer() myAPIServer.AddressManager = myAddressManager ``` If this is a common request, then in future we might provide some common custom AddressManagers. --- pkg/framework/test/apiserver.go | 50 +++++++++++-------- pkg/framework/test/apiserver_config.go | 14 ------ pkg/framework/test/apiserver_config_test.go | 26 ---------- .../test/apiserver_constructor_test.go | 23 --------- pkg/framework/test/apiserver_test.go | 39 ++++++++++++--- .../democli/integration/integration_test.go | 3 +- pkg/framework/test/etcd.go | 4 +- pkg/framework/test/etcd_test.go | 4 +- pkg/framework/test/fixtures.go | 47 ++--------------- pkg/framework/test/fixtures_test.go | 5 +- .../test/integration/integration_test.go | 8 ++- .../test/testfakes/fake_fixture_process.go | 21 +++++--- 12 files changed, 94 insertions(+), 150 deletions(-) delete mode 100644 pkg/framework/test/apiserver_config.go delete mode 100644 pkg/framework/test/apiserver_config_test.go delete mode 100644 pkg/framework/test/apiserver_constructor_test.go diff --git a/pkg/framework/test/apiserver.go b/pkg/framework/test/apiserver.go index 0b0077798..0ac192c16 100644 --- a/pkg/framework/test/apiserver.go +++ b/pkg/framework/test/apiserver.go @@ -17,7 +17,6 @@ type APIServer struct { PathFinder BinPathFinder ProcessStarter simpleSessionStarter CertDirManager certDirManager - Config *APIServerConfig Etcd FixtureProcess session SimpleSession stdOut *gbytes.Buffer @@ -32,7 +31,7 @@ type certDirManager interface { //go:generate counterfeiter . certDirManager // NewAPIServer creates a new APIServer Fixture Process -func NewAPIServer(config *APIServerConfig) (*APIServer, error) { +func NewAPIServer() (*APIServer, error) { starter := func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error) { return gexec.Start(command, out, err) } @@ -43,7 +42,6 @@ func NewAPIServer(config *APIServerConfig) (*APIServer, error) { } return &APIServer{ - Config: config, ProcessStarter: starter, CertDirManager: NewTempDirManager(), Etcd: etcd, @@ -51,24 +49,21 @@ func NewAPIServer(config *APIServerConfig) (*APIServer, error) { } // URL returns the URL APIServer is listening on. Clients can use this to connect to APIServer. -func (s *APIServer) URL() string { - // TODO handle errors - port, _ := s.AddressManager.Port() - host, _ := s.AddressManager.Host() - return fmt.Sprintf("http://%s:%d", host, port) +func (s *APIServer) URL() (string, error) { + port, err := s.AddressManager.Port() + if err != nil { + return "", err + } + host, err := s.AddressManager.Host() + if err != nil { + return "", err + } + return fmt.Sprintf("http://%s:%d", host, port), nil } // Start starts the apiserver, waits for it to come up, and returns an error, if occoured. func (s *APIServer) Start() error { - if s.PathFinder == nil { - s.PathFinder = DefaultBinPathFinder - } - if s.AddressManager == nil { - s.AddressManager = &DefaultAddressManager{} - } - if err := s.Config.Validate(); err != nil { - return err - } + s.ensureInitialized() port, addr, err := s.AddressManager.Initialize("localhost") if err != nil { @@ -85,8 +80,11 @@ func (s *APIServer) Start() error { return err } - s.stdOut = gbytes.NewBuffer() - s.stdErr = gbytes.NewBuffer() + etcdURLString, err := s.Etcd.URL() + if err != nil { + s.Etcd.Stop() + return err + } args := []string{ "--authorization-mode=Node,RBAC", @@ -96,7 +94,7 @@ func (s *APIServer) Start() error { "--admission-control-config-file=", "--bind-address=0.0.0.0", "--storage-backend=etcd3", - fmt.Sprintf("--etcd-servers=%s", s.Etcd.URL()), + fmt.Sprintf("--etcd-servers=%s", etcdURLString), fmt.Sprintf("--cert-dir=%s", certDir), fmt.Sprintf("--insecure-port=%d", port), fmt.Sprintf("--insecure-bind-address=%s", addr), @@ -119,6 +117,18 @@ func (s *APIServer) Start() error { } } +func (s *APIServer) ensureInitialized() { + if s.PathFinder == nil { + s.PathFinder = DefaultBinPathFinder + } + if s.AddressManager == nil { + s.AddressManager = &DefaultAddressManager{} + } + + s.stdOut = gbytes.NewBuffer() + s.stdErr = gbytes.NewBuffer() +} + // Stop stops this process gracefully, waits for its termination, and cleans up the cert directory. func (s *APIServer) Stop() { if s.session != nil { diff --git a/pkg/framework/test/apiserver_config.go b/pkg/framework/test/apiserver_config.go deleted file mode 100644 index 7d9fbd742..000000000 --- a/pkg/framework/test/apiserver_config.go +++ /dev/null @@ -1,14 +0,0 @@ -package test - -import "github.com/asaskevich/govalidator" - -// APIServerConfig is a struct holding data to configure the API Server process -type APIServerConfig struct { - APIServerURL string `valid:"url"` -} - -// Validate checks that the config contains only valid URLs -func (c *APIServerConfig) Validate() error { - _, err := govalidator.ValidateStruct(c) - return err -} diff --git a/pkg/framework/test/apiserver_config_test.go b/pkg/framework/test/apiserver_config_test.go deleted file mode 100644 index d32865025..000000000 --- a/pkg/framework/test/apiserver_config_test.go +++ /dev/null @@ -1,26 +0,0 @@ -package test_test - -import ( - . "k8s.io/kubectl/pkg/framework/test" - - . "github.com/onsi/ginkgo" - . "github.com/onsi/gomega" -) - -var _ = Describe("APIServerConfig", func() { - It("does not error on valid config", func() { - conf := &APIServerConfig{ - APIServerURL: "http://this.is.some.url:1234", - } - err := conf.Validate() - Expect(err).NotTo(HaveOccurred()) - }) - - It("errors on malformed URLs", func() { - conf := &APIServerConfig{ - APIServerURL: "something not URLish", - } - err := conf.Validate() - Expect(err).To(MatchError(ContainSubstring("APIServerURL: something not URLish does not validate as url"))) - }) -}) diff --git a/pkg/framework/test/apiserver_constructor_test.go b/pkg/framework/test/apiserver_constructor_test.go deleted file mode 100644 index a502ffb24..000000000 --- a/pkg/framework/test/apiserver_constructor_test.go +++ /dev/null @@ -1,23 +0,0 @@ -package test - -import ( - . "github.com/onsi/ginkgo" - . "github.com/onsi/gomega" -) - -var _ = Describe("NewAPIServer", func() { - It("can construct a properly configured APIServer", func() { - config := &APIServerConfig{ - APIServerURL: "some APIServer URL", - } - - apiServer, err := NewAPIServer(config) - - Expect(err).NotTo(HaveOccurred()) - Expect(apiServer).NotTo(BeNil()) - Expect(apiServer.ProcessStarter).NotTo(BeNil()) - Expect(apiServer.CertDirManager).NotTo(BeNil()) - Expect(apiServer.Etcd).NotTo(BeNil()) - Expect(apiServer.Config).To(Equal(config)) - }) -}) diff --git a/pkg/framework/test/apiserver_test.go b/pkg/framework/test/apiserver_test.go index a889fb4a6..7f9bd813e 100644 --- a/pkg/framework/test/apiserver_test.go +++ b/pkg/framework/test/apiserver_test.go @@ -20,7 +20,6 @@ var _ = Describe("Apiserver", func() { fakeSession *testfakes.FakeSimpleSession fakeCertDirManager *testfakes.FakeCertDirManager apiServer *APIServer - apiServerConfig *APIServerConfig fakeEtcdProcess *testfakes.FakeFixtureProcess fakePathFinder *testfakes.FakeBinPathFinder fakeAddressManager *testfakes.FakeAddressManager @@ -33,14 +32,10 @@ var _ = Describe("Apiserver", func() { fakePathFinder = &testfakes.FakeBinPathFinder{} fakeAddressManager = &testfakes.FakeAddressManager{} - apiServerConfig = &APIServerConfig{ - APIServerURL: "", - } apiServer = &APIServer{ AddressManager: fakeAddressManager, PathFinder: fakePathFinder.Spy, CertDirManager: fakeCertDirManager, - Config: apiServerConfig, Etcd: fakeEtcdProcess, } }) @@ -48,6 +43,7 @@ var _ = Describe("Apiserver", func() { Describe("starting and stopping the server", func() { Context("when given a path to a binary that runs for a long time", func() { It("can start and stop that binary", func() { + //TODO Break this long test up sessionBuffer := gbytes.NewBuffer() fmt.Fprint(sessionBuffer, "Everything is fine") fakeSession.BufferReturns(sessionBuffer) @@ -57,10 +53,12 @@ var _ = Describe("Apiserver", func() { fakePathFinder.Returns("/some/path/to/apiserver") fakeAddressManager.InitializeReturns(1234, "this.is.the.API.server", nil) + fakeEtcdProcess.URLReturns("the etcd url", nil) apiServer.ProcessStarter = func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error) { 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.Path).To(Equal("/some/path/to/apiserver")) fmt.Fprint(err, "Serving insecurely on this.is.the.API.server:1234") return fakeSession, nil @@ -82,6 +80,9 @@ var _ = Describe("Apiserver", func() { Expect(fakeAddressManager.InitializeCallCount()).To(Equal(1)) Expect(fakeAddressManager.InitializeArgsForCall(0)).To(Equal("localhost")) + By("...getting the URL of Etcd") + Expect(fakeEtcdProcess.URLCallCount()).To(Equal(1)) + Eventually(apiServer).Should(gbytes.Say("Everything is fine")) Expect(fakeSession.ExitCodeCallCount()).To(Equal(0)) Expect(apiServer).NotTo(gexec.Exit()) @@ -114,6 +115,22 @@ var _ = Describe("Apiserver", func() { }) }) + Context("when getting the URL of Etcd fails", func() { + It("propagates the error, stop Etcd and keep APIServer down", func() { + fakeEtcdProcess.URLReturns("", fmt.Errorf("no etcd url")) + + 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 getting etcd's URL fails") + return nil, nil + } + + err := apiServer.Start() + Expect(err).To(MatchError(ContainSubstring("no etcd url"))) + Expect(fakeEtcdProcess.StopCallCount()).To(Equal(1)) + }) + }) + 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.")) @@ -172,20 +189,26 @@ var _ = Describe("Apiserver", func() { It("can be queried for the URL it listens on", func() { fakeAddressManager.HostReturns("the.host.for.api.server", nil) fakeAddressManager.PortReturns(5678, nil) - Expect(apiServer.URL()).To(Equal("http://the.host.for.api.server:5678")) + apiServerURL, err := apiServer.URL() + Expect(err).NotTo(HaveOccurred()) + Expect(apiServerURL).To(Equal("http://the.host.for.api.server:5678")) }) Context("when we query for the URL before starting the server", func() { Context("and so the addressmanager fails to give us a port", func() { It("propagates the failure", func() { - //TODO + fakeAddressManager.PortReturns(0, fmt.Errorf("boom")) + _, err := apiServer.URL() + Expect(err).To(MatchError(ContainSubstring("boom"))) }) }) Context("and so the addressmanager fails to give us a host", func() { It("propagates the failure", func() { - //TODO + fakeAddressManager.HostReturns("", fmt.Errorf("biff!")) + _, err := apiServer.URL() + Expect(err).To(MatchError(ContainSubstring("biff!"))) }) }) diff --git a/pkg/framework/test/democli/integration/integration_test.go b/pkg/framework/test/democli/integration/integration_test.go index bd7a7713d..5be382406 100644 --- a/pkg/framework/test/democli/integration/integration_test.go +++ b/pkg/framework/test/democli/integration/integration_test.go @@ -21,7 +21,8 @@ var _ = Describe("DemoCLI Integration", func() { }) It("can get a list of pods", func() { - apiURL := fixtures.APIServerURL() + apiURL, err := fixtures.APIServerURL() + Expect(err).NotTo(HaveOccurred()) command := exec.Command(pathToDemoCommand, "listPods", "--api-url", apiURL) session, err := gexec.Start(command, GinkgoWriter, GinkgoWriter) diff --git a/pkg/framework/test/etcd.go b/pkg/framework/test/etcd.go index 7efec1354..aeb76cf62 100644 --- a/pkg/framework/test/etcd.go +++ b/pkg/framework/test/etcd.go @@ -62,8 +62,8 @@ func NewEtcd() (*Etcd, error) { } // URL returns the URL Etcd is listening on. Clients can use this to connect to Etcd. -func (e *Etcd) URL() string { - return e.Config.ClientURL +func (e *Etcd) URL() (string, error) { + return e.Config.ClientURL, nil } // Start starts the etcd, waits for it to come up, and returns an error, if occoured. diff --git a/pkg/framework/test/etcd_test.go b/pkg/framework/test/etcd_test.go index 16f169952..d0e16c1c8 100644 --- a/pkg/framework/test/etcd_test.go +++ b/pkg/framework/test/etcd_test.go @@ -40,7 +40,9 @@ var _ = Describe("Etcd", func() { }) It("can be queried for the port it listens on", func() { - Expect(etcd.URL()).To(Equal("http://this.is.etcd.listening.for.clients:1234")) + etcdURL, err := etcd.URL() + Expect(err).NotTo(HaveOccurred()) + Expect(etcdURL).To(Equal("http://this.is.etcd.listening.for.clients:1234")) }) Context("when given a path to a binary that runs for a long time", func() { diff --git a/pkg/framework/test/fixtures.go b/pkg/framework/test/fixtures.go index ec92c2378..b93ccd820 100644 --- a/pkg/framework/test/fixtures.go +++ b/pkg/framework/test/fixtures.go @@ -1,10 +1,5 @@ package test -import ( - "fmt" - "net" -) - // Fixtures is a struct that knows how to start all your test fixtures. // // Right now, that means Etcd and your APIServer. This is likely to increase in future. @@ -17,23 +12,16 @@ type Fixtures struct { // and other internals. type FixtureProcess interface { Start() error + //TODO Stop should return an error Stop() - URL() string + URL() (string, error) } //go:generate counterfeiter . FixtureProcess // NewFixtures will give you a Fixtures struct that's properly wired together. func NewFixtures() (*Fixtures, error) { - apiServerConfig := &APIServerConfig{} - - if url, urlErr := getHTTPListenURL(); urlErr == nil { - apiServerConfig.APIServerURL = url - } else { - return nil, urlErr - } - - apiServer, err := NewAPIServer(apiServerConfig) + apiServer, err := NewAPIServer() if err != nil { return nil, err } @@ -75,33 +63,6 @@ func (f *Fixtures) Stop() error { } // APIServerURL returns the URL to the APIServer. Clients can use this URL to connect to the APIServer. -func (f *Fixtures) APIServerURL() string { +func (f *Fixtures) APIServerURL() (string, error) { return f.APIServer.URL() } - -func getHTTPListenURL() (url string, err error) { - host := "127.0.0.1" - port, err := getFreePort(host) - if err != nil { - return "", err - } - return fmt.Sprintf("http://%s:%d", host, port), nil -} - -func getFreePort(host string) (port int, err error) { - var addr *net.TCPAddr - addr, err = net.ResolveTCPAddr("tcp", fmt.Sprintf("%s:0", host)) - if err != nil { - return - } - var l *net.TCPListener - l, err = net.ListenTCP("tcp", addr) - if err != nil { - return - } - defer func() { - err = l.Close() - }() - - return l.Addr().(*net.TCPAddr).Port, nil -} diff --git a/pkg/framework/test/fixtures_test.go b/pkg/framework/test/fixtures_test.go index 05e561c23..4df52276c 100644 --- a/pkg/framework/test/fixtures_test.go +++ b/pkg/framework/test/fixtures_test.go @@ -51,9 +51,10 @@ var _ = Describe("Fixtures", func() { }) It("can be queried for the APIServer URL", func() { - fakeAPIServerProcess.URLReturns("some url to the apiserver") + fakeAPIServerProcess.URLReturns("some url to the apiserver", nil) - url := fixtures.APIServerURL() + url, err := fixtures.APIServerURL() + Expect(err).NotTo(HaveOccurred()) Expect(url).To(Equal("some url to the apiserver")) }) diff --git a/pkg/framework/test/integration/integration_test.go b/pkg/framework/test/integration/integration_test.go index fb64ba21a..d9e340132 100644 --- a/pkg/framework/test/integration/integration_test.go +++ b/pkg/framework/test/integration/integration_test.go @@ -25,9 +25,13 @@ var _ = Describe("The Testing Framework", func() { Expect(err).NotTo(HaveOccurred(), "Expected fixtures to start successfully") var apiServerURL, etcdClientURL *url.URL - etcdClientURL, err = url.Parse(fixtures.APIServer.(*test.APIServer).Etcd.URL()) + etcdUrlString, err := fixtures.APIServer.(*test.APIServer).Etcd.URL() Expect(err).NotTo(HaveOccurred()) - apiServerURL, err = url.Parse(fixtures.APIServerURL()) + etcdClientURL, err = url.Parse(etcdUrlString) + Expect(err).NotTo(HaveOccurred()) + urlString, err := fixtures.APIServerURL() + Expect(err).NotTo(HaveOccurred()) + apiServerURL, err = url.Parse(urlString) Expect(err).NotTo(HaveOccurred()) isEtcdListeningForClients := isSomethingListeningOnPort(etcdClientURL.Host) diff --git a/pkg/framework/test/testfakes/fake_fixture_process.go b/pkg/framework/test/testfakes/fake_fixture_process.go index 7a7ef58e1..eca820dc1 100644 --- a/pkg/framework/test/testfakes/fake_fixture_process.go +++ b/pkg/framework/test/testfakes/fake_fixture_process.go @@ -20,14 +20,16 @@ type FakeFixtureProcess struct { StopStub func() stopMutex sync.RWMutex stopArgsForCall []struct{} - URLStub func() string + URLStub func() (string, error) uRLMutex sync.RWMutex uRLArgsForCall []struct{} uRLReturns struct { result1 string + result2 error } uRLReturnsOnCall map[int]struct { result1 string + result2 error } invocations map[string][][]interface{} invocationsMutex sync.RWMutex @@ -89,7 +91,7 @@ func (fake *FakeFixtureProcess) StopCallCount() int { return len(fake.stopArgsForCall) } -func (fake *FakeFixtureProcess) URL() string { +func (fake *FakeFixtureProcess) URL() (string, error) { fake.uRLMutex.Lock() ret, specificReturn := fake.uRLReturnsOnCall[len(fake.uRLArgsForCall)] fake.uRLArgsForCall = append(fake.uRLArgsForCall, struct{}{}) @@ -99,9 +101,9 @@ func (fake *FakeFixtureProcess) URL() string { return fake.URLStub() } if specificReturn { - return ret.result1 + return ret.result1, ret.result2 } - return fake.uRLReturns.result1 + return fake.uRLReturns.result1, fake.uRLReturns.result2 } func (fake *FakeFixtureProcess) URLCallCount() int { @@ -110,23 +112,26 @@ func (fake *FakeFixtureProcess) URLCallCount() int { return len(fake.uRLArgsForCall) } -func (fake *FakeFixtureProcess) URLReturns(result1 string) { +func (fake *FakeFixtureProcess) URLReturns(result1 string, result2 error) { fake.URLStub = nil fake.uRLReturns = struct { result1 string - }{result1} + result2 error + }{result1, result2} } -func (fake *FakeFixtureProcess) URLReturnsOnCall(i int, result1 string) { +func (fake *FakeFixtureProcess) URLReturnsOnCall(i int, result1 string, result2 error) { fake.URLStub = nil if fake.uRLReturnsOnCall == nil { fake.uRLReturnsOnCall = make(map[int]struct { result1 string + result2 error }) } fake.uRLReturnsOnCall[i] = struct { result1 string - }{result1} + result2 error + }{result1, result2} } func (fake *FakeFixtureProcess) Invocations() map[string][][]interface{} {