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

@@ -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 {

pkg/framework/test/apiserver_config.go

@@ -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
-}

pkg/framework/test/apiserver_config_test.go

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

pkg/framework/test/apiserver_constructor_test.go

@@ -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))
-	})
-})

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

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)

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.

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() {

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

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

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)

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