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] 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{} {