From 818f357128cd43da04120f7907c38f42d3107578 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 28 Aug 2014 09:56:38 -0400 Subject: [PATCH] Client should validate the incoming host value Convert host:port and URLs passed to client.New() into the proper values, and return an error if the value is invalid. Change CLI to return an error if -master is invalid. Remove Client.rawRequest which was not in use, and fix the involved tests. Add NewOrDie Preserves the behavior of the client to not auth when a non-https URL is passed (although in the future this should be corrected). --- cmd/apiserver/apiserver.go | 5 +- cmd/controller-manager/controller-manager.go | 7 +- cmd/integration/integration.go | 4 +- cmd/kubecfg/kubecfg.go | 40 +++--- cmd/proxy/proxy.go | 5 +- pkg/client/client.go | 127 +++++++++--------- pkg/client/client_test.go | 77 +++++++---- pkg/client/request.go | 2 +- pkg/client/request_test.go | 45 ++++--- pkg/controller/replication_controller_test.go | 10 +- pkg/kubecfg/proxy_server.go | 8 +- pkg/service/endpoints_controller_test.go | 21 ++- plugin/cmd/scheduler/scheduler.go | 6 +- plugin/pkg/scheduler/factory/factory_test.go | 16 ++- test/integration/client_test.go | 2 +- 15 files changed, 203 insertions(+), 172 deletions(-) diff --git a/cmd/apiserver/apiserver.go b/cmd/apiserver/apiserver.go index 5fb6792624..7a564b219a 100644 --- a/cmd/apiserver/apiserver.go +++ b/cmd/apiserver/apiserver.go @@ -92,7 +92,10 @@ func main() { Port: *minionPort, } - client := client.New("http://"+net.JoinHostPort(*address, strconv.Itoa(int(*port))), nil) + client, err := client.New(net.JoinHostPort(*address, strconv.Itoa(int(*port))), nil) + if err != nil { + glog.Fatalf("Invalid server address: %v", err) + } m := master.New(&master.Config{ Client: client, diff --git a/cmd/controller-manager/controller-manager.go b/cmd/controller-manager/controller-manager.go index d32992b120..091e7bddeb 100644 --- a/cmd/controller-manager/controller-manager.go +++ b/cmd/controller-manager/controller-manager.go @@ -46,9 +46,12 @@ func main() { glog.Fatal("usage: controller-manager -master ") } - controllerManager := controller.NewReplicationManager( - client.New("http://"+*master, nil)) + kubeClient, err := client.New(*master, nil) + if err != nil { + glog.Fatalf("Invalid -master: %v", err) + } + controllerManager := controller.NewReplicationManager(kubeClient) controllerManager.Run(10 * time.Second) select {} } diff --git a/cmd/integration/integration.go b/cmd/integration/integration.go index fdb07b5d0e..36f1d004b9 100644 --- a/cmd/integration/integration.go +++ b/cmd/integration/integration.go @@ -104,7 +104,7 @@ func startComponents(manifestURL string) (apiServerURL string) { } } - cl := client.New(apiServer.URL, nil) + cl := client.NewOrDie(apiServer.URL, nil) cl.PollPeriod = time.Second * 1 cl.Sync = true @@ -301,7 +301,7 @@ func main() { // Wait for the synchronization threads to come up. time.Sleep(time.Second * 10) - kubeClient := client.New(apiServerURL, nil) + kubeClient := client.NewOrDie(apiServerURL, nil) // Run tests in parallel testFuncs := []testFunc{ diff --git a/cmd/kubecfg/kubecfg.go b/cmd/kubecfg/kubecfg.go index cd9fe3e11e..ae03a1a264 100644 --- a/cmd/kubecfg/kubecfg.go +++ b/cmd/kubecfg/kubecfg.go @@ -20,7 +20,6 @@ import ( "flag" "fmt" "io/ioutil" - "net/url" "os" "reflect" "sort" @@ -30,7 +29,7 @@ import ( "time" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" - kube_client "github.com/GoogleCloudPlatform/kubernetes/pkg/client" + "github.com/GoogleCloudPlatform/kubernetes/pkg/client" "github.com/GoogleCloudPlatform/kubernetes/pkg/kubecfg" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" "github.com/GoogleCloudPlatform/kubernetes/pkg/version" @@ -118,7 +117,6 @@ func main() { verflag.PrintAndExitIfRequested() - secure := true var masterServer string if len(*httpServer) > 0 { masterServer = *httpServer @@ -127,26 +125,26 @@ func main() { } else { masterServer = "http://localhost:8080" } - parsedURL, err := url.Parse(masterServer) + kubeClient, err := client.New(masterServer, nil) if err != nil { - glog.Fatalf("Unable to parse %v as a URL\n", err) - } - if parsedURL.Scheme != "" && parsedURL.Scheme != "https" { - secure = false + glog.Fatalf("Unable to parse %s as a URL: %v", masterServer, err) } - var auth *kube_client.AuthInfo - if secure { - auth, err = kubecfg.LoadAuthInfo(*authConfig, os.Stdin) + // TODO: this won't work if TLS is enabled with client cert auth, but no + // passwords are required. Refactor when we address client auth abstraction. + if kubeClient.Secure() { + auth, err := kubecfg.LoadAuthInfo(*authConfig, os.Stdin) if err != nil { glog.Fatalf("Error loading auth: %v", err) } + kubeClient, err = client.New(masterServer, auth) + if err != nil { + glog.Fatalf("Unable to parse %s as a URL: %v", masterServer, err) + } } - client := kube_client.New(masterServer, auth) - if *serverVersion { - got, err := client.ServerVersion() + got, err := kubeClient.ServerVersion() if err != nil { fmt.Printf("Couldn't read version from server: %v\n", err) os.Exit(1) @@ -156,7 +154,7 @@ func main() { } if *preventSkew { - got, err := client.ServerVersion() + got, err := kubeClient.ServerVersion() if err != nil { fmt.Printf("Couldn't read version from server: %v\n", err) os.Exit(1) @@ -169,7 +167,7 @@ func main() { if *proxy { glog.Info("Starting to serve on localhost:8001") - server := kubecfg.NewProxyServer(*www, masterServer, auth) + server := kubecfg.NewProxyServer(*www, kubeClient) glog.Fatal(server.Serve()) } @@ -179,7 +177,7 @@ func main() { } method := flag.Arg(0) - matchFound := executeAPIRequest(method, client) || executeControllerRequest(method, client) + matchFound := executeAPIRequest(method, kubeClient) || executeControllerRequest(method, kubeClient) if matchFound == false { glog.Fatalf("Unknown command %s", method) } @@ -206,7 +204,7 @@ func checkStorage(storage string) bool { return false } -func executeAPIRequest(method string, s *kube_client.Client) bool { +func executeAPIRequest(method string, c *client.Client) bool { storage, path, hasSuffix := storagePathFromArg(flag.Arg(1)) validStorage := checkStorage(storage) verb := "" @@ -235,7 +233,7 @@ func executeAPIRequest(method string, s *kube_client.Client) bool { glog.Fatalf("usage: kubecfg [OPTIONS] %s <%s>", method, prettyWireStorage()) } case "update": - obj, err := s.Verb("GET").Path(path).Do().Get() + obj, err := c.Verb("GET").Path(path).Do().Get() if err != nil { glog.Fatalf("error obtaining resource version for update: %v", err) } @@ -253,7 +251,7 @@ func executeAPIRequest(method string, s *kube_client.Client) bool { return false } - r := s.Verb(verb). + r := c.Verb(verb). Path(path). ParseSelectorParam("labels", *selector) if setBody { @@ -323,7 +321,7 @@ func executeAPIRequest(method string, s *kube_client.Client) bool { return true } -func executeControllerRequest(method string, c *kube_client.Client) bool { +func executeControllerRequest(method string, c *client.Client) bool { parseController := func() string { if len(flag.Args()) != 2 { glog.Fatal("usage: kubecfg [OPTIONS] stop|rm|rollingupdate ") diff --git a/cmd/proxy/proxy.go b/cmd/proxy/proxy.go index af16a8c17b..982aa3f879 100644 --- a/cmd/proxy/proxy.go +++ b/cmd/proxy/proxy.go @@ -53,7 +53,10 @@ func main() { if *master != "" { glog.Infof("Using api calls to get config %v", *master) //TODO: add auth info - client := client.New(*master, nil) + client, err := client.New(*master, nil) + if err != nil { + glog.Fatalf("Invalid -master: %v", err) + } config.NewSourceAPI( client, 30*time.Second, diff --git a/pkg/client/client.go b/pkg/client/client.go index 6412457d2b..04e2529c13 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -20,7 +20,6 @@ import ( "crypto/tls" "encoding/json" "fmt" - "io" "io/ioutil" "net/http" "net/url" @@ -30,7 +29,6 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/version" "github.com/GoogleCloudPlatform/kubernetes/pkg/watch" - "github.com/golang/glog" ) // Interface holds the methods for clients of Kubernetes, @@ -88,6 +86,28 @@ type Client struct { *RESTClient } +// New creates a Kubernetes client. This client works with pods, replication controllers +// and services. It allows operations such as list, get, update and delete on these objects. +// host must be a host string, a host:port combo, or an http or https URL. Passing a prefix +// to a URL will prepend the server path. Returns an error if host cannot be converted to a +// valid URL. +func New(host string, auth *AuthInfo) (*Client, error) { + restClient, err := NewRESTClient(host, auth, "/api/v1beta1/") + if err != nil { + return nil, err + } + return &Client{restClient}, nil +} + +// NewOrDie creates a Kubernetes client and panics if the provided host is invalid. +func NewOrDie(host string, auth *AuthInfo) *Client { + client, err := New(host, auth) + if err != nil { + panic(err) + } + return client +} + // StatusErr might get returned from an api call if your request is still being processed // and hence the expected return data is not available yet. type StatusErr struct { @@ -109,20 +129,31 @@ type AuthInfo struct { // Host is the http://... base for the URL type RESTClient struct { host string + prefix string + secure bool auth *AuthInfo httpClient *http.Client Sync bool PollPeriod time.Duration Timeout time.Duration - Prefix string } // NewRESTClient creates a new RESTClient. This client performs generic REST functions // such as Get, Put, Post, and Delete on specified paths. -func NewRESTClient(host string, auth *AuthInfo, prefix string) *RESTClient { +func NewRESTClient(host string, auth *AuthInfo, path string) (*RESTClient, error) { + prefix, err := normalizePrefix(host, path) + if err != nil { + return nil, err + } + base := *prefix + base.Path = "" + base.RawQuery = "" + base.Fragment = "" return &RESTClient{ - auth: auth, - host: host, + host: base.String(), + prefix: prefix.Path, + secure: prefix.Scheme == "https", + auth: auth, httpClient: &http.Client{ Transport: &http.Transport{ TLSClientConfig: &tls.Config{ @@ -133,15 +164,36 @@ func NewRESTClient(host string, auth *AuthInfo, prefix string) *RESTClient { Sync: false, PollPeriod: time.Second * 2, Timeout: time.Second * 20, - Prefix: prefix, - } - + }, nil } -// New creates a Kubernetes client. This client works with pods, replication controllers -// and services. It allows operations such as list, get, update and delete on these objects. -func New(host string, auth *AuthInfo) *Client { - return &Client{NewRESTClient(host, auth, "/api/v1beta1/")} +// normalizePrefix ensures the passed initial value is valid +func normalizePrefix(host, prefix string) (*url.URL, error) { + if host == "" { + return nil, fmt.Errorf("host must be a URL or a host:port pair") + } + base := host + hostURL, err := url.Parse(base) + if err != nil { + return nil, err + } + if hostURL.Scheme == "" { + hostURL, err = url.Parse("http://" + base) + if err != nil { + return nil, err + } + if hostURL.Path != "" && hostURL.Path != "/" { + return nil, fmt.Errorf("host must be a URL or a host:port pair: %s", base) + } + } + hostURL.Path += prefix + + return hostURL, nil +} + +// Secure returns true if the client is configured for secure connections. +func (c *RESTClient) Secure() bool { + return c.secure } // Execute a request, adds authentication (if auth != nil), and HTTPS cert ignoring. @@ -186,55 +238,6 @@ func (c *RESTClient) doRequest(request *http.Request) ([]byte, error) { return body, err } -// Underlying base implementation of performing a request. -// method is the HTTP method (e.g. "GET") -// path is the path on the host to hit -// requestBody is the body of the request. Can be nil. -// target the interface to marshal the JSON response into. Can be nil. -func (c *RESTClient) rawRequest(method, path string, requestBody io.Reader, target interface{}) ([]byte, error) { - reqUrl, err := c.makeURL(path) - if err != nil { - return nil, err - } - - request, err := http.NewRequest(method, reqUrl, requestBody) - if err != nil { - return nil, err - } - body, err := c.doRequest(request) - if err != nil { - return body, err - } - if target != nil { - err = api.DecodeInto(body, target) - } - if err != nil { - glog.Infof("Failed to parse: %s\n", string(body)) - // FIXME: no need to return err here? - } - return body, err -} - -func (c *RESTClient) makeURL(path string) (string, error) { - base := c.host - hostURL, err := url.Parse(base) - if err != nil { - return "", err - } - if hostURL.Scheme == "" { - hostURL, err = url.Parse("http://" + base) - if err != nil { - return "", err - } - if hostURL.Path != "" && hostURL.Path != "/" { - return "", fmt.Errorf("host must be a URL or a host:port pair: %s", base) - } - } - hostURL.Path += c.Prefix + path - - return hostURL.String(), nil -} - // ListPods takes a selector, and returns the list of pods that match that selector func (c *Client) ListPods(selector labels.Selector) (result api.PodList, err error) { err = c.Get().Path("pods").SelectorParam("labels", selector).Do().Into(&result) diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index b00391dae7..0b9f92c765 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -21,6 +21,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "path" "reflect" "testing" @@ -31,24 +32,22 @@ import ( ) // TODO: Move this to a common place, it's needed in multiple tests. -var apiPath = "/api/v1beta1" - -func makeURL(suffix string) string { - return apiPath + suffix -} +const apiPath = "/api/v1beta1" func TestValidatesHostParameter(t *testing.T) { testCases := map[string]struct { - Value string - Err bool + Host string + Prefix string + Err bool }{ - "foo.bar.com": {"http://foo.bar.com/api/v1beta1/", false}, - "http://host/server": {"http://host/server/api/v1beta1/", false}, - "host/server": {"", true}, + "127.0.0.1": {"http://127.0.0.1", "/api/v1beta1/", false}, + "127.0.0.1:8080": {"http://127.0.0.1:8080", "/api/v1beta1/", false}, + "foo.bar.com": {"http://foo.bar.com", "/api/v1beta1/", false}, + "http://host/server": {"http://host", "/server/api/v1beta1/", false}, + "host/server": {"", "", true}, } for k, expected := range testCases { - c := RESTClient{host: k, Prefix: "/api/v1beta1/"} - actual, err := c.makeURL("") + c, err := NewRESTClient(k, nil, "/api/v1beta1/") switch { case err == nil && expected.Err: t.Errorf("expected error but was nil") @@ -56,9 +55,15 @@ func TestValidatesHostParameter(t *testing.T) { case err != nil && !expected.Err: t.Errorf("unexpected error %v", err) continue + case err != nil: + continue } - if expected.Value != actual { - t.Errorf("%s: expected %s, got %s", k, expected.Value, actual) + if e, a := expected.Host, c.host; e != a { + t.Errorf("%s: expected host %s, got %s", k, e, a) + continue + } + if e, a := expected.Prefix, c.prefix; e != a { + t.Errorf("%s: expected prefix %s, got %s", k, e, a) continue } } @@ -349,9 +354,10 @@ func (c *testClient) Setup() *testClient { } c.server = httptest.NewServer(c.handler) if c.Client == nil { - c.Client = New("", nil) + c.Client = NewOrDie("localhost", nil) } c.Client.host = c.server.URL + c.Client.prefix = "/api/v1beta1/" c.QueryValidator = map[string]func(string, string) bool{} return c } @@ -374,7 +380,7 @@ func (c *testClient) Validate(t *testing.T, received interface{}, err error) { // We check the query manually, so blank it out so that FakeHandler.ValidateRequest // won't check it. c.handler.RequestReceived.URL.RawQuery = "" - c.handler.ValidateRequest(t, makeURL(c.Request.Path), c.Request.Method, requestBody) + c.handler.ValidateRequest(t, path.Join(apiPath, c.Request.Path), c.Request.Method, requestBody) for key, values := range c.Request.Query { validator, ok := c.QueryValidator[key] if !ok { @@ -437,18 +443,27 @@ func TestDeleteService(t *testing.T) { c.Validate(t, nil, err) } -func TestMakeRequest(t *testing.T) { +func TestDoRequest(t *testing.T) { + invalid := "aaaaa" testClients := []testClient{ - {Request: testRequest{Method: "GET", Path: "/good"}, Response: Response{StatusCode: 200}}, - {Request: testRequest{Method: "GET", Path: "/bad%ZZ"}, Error: true}, - {Client: New("", &AuthInfo{"foo", "bar"}), Request: testRequest{Method: "GET", Path: "/auth", Header: "Authorization"}, Response: Response{StatusCode: 200}}, - {Client: &Client{&RESTClient{httpClient: http.DefaultClient}}, Request: testRequest{Method: "GET", Path: "/nocertificate"}, Error: true}, - {Request: testRequest{Method: "GET", Path: "/error"}, Response: Response{StatusCode: 500}, Error: true}, - {Request: testRequest{Method: "POST", Path: "/faildecode"}, Response: Response{StatusCode: 200, Body: "aaaaa"}, Target: &struct{}{}, Error: true}, - {Request: testRequest{Method: "GET", Path: "/failread"}, Response: Response{StatusCode: 200, Body: "aaaaa"}, Target: &struct{}{}, Error: true}, + {Request: testRequest{Method: "GET", Path: "good"}, Response: Response{StatusCode: 200}}, + {Request: testRequest{Method: "GET", Path: "bad%ZZ"}, Error: true}, + {Client: NewOrDie("localhost", &AuthInfo{"foo", "bar"}), Request: testRequest{Method: "GET", Path: "auth", Header: "Authorization"}, Response: Response{StatusCode: 200}}, + {Client: &Client{&RESTClient{httpClient: http.DefaultClient}}, Request: testRequest{Method: "GET", Path: "nocertificate"}, Error: true}, + {Request: testRequest{Method: "GET", Path: "error"}, Response: Response{StatusCode: 500}, Error: true}, + {Request: testRequest{Method: "POST", Path: "faildecode"}, Response: Response{StatusCode: 200, RawBody: &invalid}, Target: &struct{}{}}, + {Request: testRequest{Method: "GET", Path: "failread"}, Response: Response{StatusCode: 200, RawBody: &invalid}, Target: &struct{}{}}, } for _, c := range testClients { - response, err := c.Setup().rawRequest(c.Request.Method, c.Request.Path[1:], nil, c.Target) + client := c.Setup() + prefix, _ := url.Parse(client.host) + prefix.Path = client.prefix + c.Request.Path + request := &http.Request{ + Method: c.Request.Method, + Header: make(http.Header), + URL: prefix, + } + response, err := client.doRequest(request) c.Validate(t, response, err) } } @@ -464,7 +479,10 @@ func TestDoRequestAccepted(t *testing.T) { testServer := httptest.NewServer(&fakeHandler) request, _ := http.NewRequest("GET", testServer.URL+"/foo/bar", nil) auth := AuthInfo{User: "user", Password: "pass"} - c := New(testServer.URL, &auth) + c, err := New(testServer.URL, &auth) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } body, err := c.doRequest(request) if request.Header["Authorization"] == nil { t.Errorf("Request is missing authorization header: %#v", *request) @@ -498,7 +516,10 @@ func TestDoRequestAcceptedSuccess(t *testing.T) { testServer := httptest.NewServer(&fakeHandler) request, _ := http.NewRequest("GET", testServer.URL+"/foo/bar", nil) auth := AuthInfo{User: "user", Password: "pass"} - c := New(testServer.URL, &auth) + c, err := New(testServer.URL, &auth) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } body, err := c.doRequest(request) if request.Header["Authorization"] == nil { t.Errorf("Request is missing authorization header: %#v", *request) @@ -532,7 +553,7 @@ func TestGetServerVersion(t *testing.T) { w.WriteHeader(http.StatusOK) w.Write(output) })) - client := New(server.URL, nil) + client := NewOrDie(server.URL, nil) got, err := client.ServerVersion() if err != nil { diff --git a/pkg/client/request.go b/pkg/client/request.go index 3cc9b12461..8eb538b509 100644 --- a/pkg/client/request.go +++ b/pkg/client/request.go @@ -56,7 +56,7 @@ func (c *RESTClient) Verb(verb string) *Request { return &Request{ verb: verb, c: c, - path: c.Prefix, + path: c.prefix, sync: c.Sync, timeout: c.Timeout, params: map[string]string{}, diff --git a/pkg/client/request_test.go b/pkg/client/request_test.go index d27cde5354..360e3962ed 100644 --- a/pkg/client/request_test.go +++ b/pkg/client/request_test.go @@ -45,8 +45,8 @@ func TestDoRequestNewWay(t *testing.T) { } testServer := httptest.NewServer(&fakeHandler) auth := AuthInfo{User: "user", Password: "pass"} - s := New(testServer.URL, &auth) - obj, err := s.Verb("POST"). + c := NewOrDie(testServer.URL, &auth) + obj, err := c.Verb("POST"). Path("foo/bar"). Path("baz"). ParseSelectorParam("labels", "name=foo"). @@ -80,8 +80,8 @@ func TestDoRequestNewWayReader(t *testing.T) { } testServer := httptest.NewServer(&fakeHandler) auth := AuthInfo{User: "user", Password: "pass"} - s := New(testServer.URL, &auth) - obj, err := s.Verb("POST"). + c := NewOrDie(testServer.URL, &auth) + obj, err := c.Verb("POST"). Path("foo/bar"). Path("baz"). SelectorParam("labels", labels.Set{"name": "foo"}.AsSelector()). @@ -117,8 +117,8 @@ func TestDoRequestNewWayObj(t *testing.T) { } testServer := httptest.NewServer(&fakeHandler) auth := AuthInfo{User: "user", Password: "pass"} - s := New(testServer.URL, &auth) - obj, err := s.Verb("POST"). + c := NewOrDie(testServer.URL, &auth) + obj, err := c.Verb("POST"). Path("foo/bar"). Path("baz"). SelectorParam("labels", labels.Set{"name": "foo"}.AsSelector()). @@ -167,8 +167,8 @@ func TestDoRequestNewWayFile(t *testing.T) { } testServer := httptest.NewServer(&fakeHandler) auth := AuthInfo{User: "user", Password: "pass"} - s := New(testServer.URL, &auth) - obj, err := s.Verb("POST"). + c := NewOrDie(testServer.URL, &auth) + obj, err := c.Verb("POST"). Path("foo/bar"). Path("baz"). ParseSelectorParam("labels", "name=foo"). @@ -192,7 +192,7 @@ func TestDoRequestNewWayFile(t *testing.T) { } func TestVerbs(t *testing.T) { - c := New("", nil) + c := NewOrDie("localhost", nil) if r := c.Post(); r.verb != "POST" { t.Errorf("Post verb is wrong") } @@ -209,7 +209,7 @@ func TestVerbs(t *testing.T) { func TestAbsPath(t *testing.T) { expectedPath := "/bar/foo" - c := New("", nil) + c := NewOrDie("localhost", nil) r := c.Post().Path("/foo").AbsPath(expectedPath) if r.path != expectedPath { t.Errorf("unexpected path: %s, expected %s", r.path, expectedPath) @@ -217,7 +217,7 @@ func TestAbsPath(t *testing.T) { } func TestSync(t *testing.T) { - c := New("", nil) + c := NewOrDie("localhost", nil) r := c.Get() if r.sync { t.Errorf("sync has wrong default") @@ -238,13 +238,13 @@ func TestUintParam(t *testing.T) { testVal uint64 expectStr string }{ - {"foo", 31415, "?foo=31415"}, - {"bar", 42, "?bar=42"}, - {"baz", 0, "?baz=0"}, + {"foo", 31415, "http://localhost?foo=31415"}, + {"bar", 42, "http://localhost?bar=42"}, + {"baz", 0, "http://localhost?baz=0"}, } for _, item := range table { - c := New("", nil) + c := NewOrDie("localhost", nil) r := c.Get().AbsPath("").UintParam(item.name, item.testVal) if e, a := item.expectStr, r.finalURL(); e != a { t.Errorf("expected %v, got %v", e, a) @@ -263,7 +263,7 @@ func TestUnacceptableParamNames(t *testing.T) { } for _, item := range table { - c := New("", nil) + c := NewOrDie("localhost", nil) r := c.Get().setParam(item.name, item.testVal) if e, a := item.expectSuccess, r.err == nil; e != a { t.Errorf("expected %v, got %v (%v)", e, a, r.err) @@ -272,7 +272,7 @@ func TestUnacceptableParamNames(t *testing.T) { } func TestSetPollPeriod(t *testing.T) { - c := New("", nil) + c := NewOrDie("localhost", nil) r := c.Get() if r.pollPeriod == 0 { t.Errorf("polling should be on by default") @@ -303,12 +303,12 @@ func TestPolling(t *testing.T) { })) auth := AuthInfo{User: "user", Password: "pass"} - s := New(testServer.URL, &auth) + c := NewOrDie(testServer.URL, &auth) trials := []func(){ func() { // Check that we do indeed poll when asked to. - obj, err := s.Get().PollPeriod(5 * time.Millisecond).Do().Get() + obj, err := c.Get().PollPeriod(5 * time.Millisecond).Do().Get() if err != nil { t.Errorf("Unexpected error: %v %#v", err, err) return @@ -323,7 +323,7 @@ func TestPolling(t *testing.T) { }, func() { // Check that we don't poll when asked not to. - obj, err := s.Get().PollPeriod(0).Do().Get() + obj, err := c.Get().PollPeriod(0).Do().Get() if err == nil { t.Errorf("Unexpected non error: %v", obj) return @@ -405,7 +405,10 @@ func TestWatch(t *testing.T) { } })) - s := New(testServer.URL, &auth) + s, err := New(testServer.URL, &auth) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } watching, err := s.Get().Path("path/to/watch/thing").Watch() if err != nil { diff --git a/pkg/controller/replication_controller_test.go b/pkg/controller/replication_controller_test.go index 659613f88a..c5e3a8480e 100644 --- a/pkg/controller/replication_controller_test.go +++ b/pkg/controller/replication_controller_test.go @@ -114,7 +114,7 @@ func TestSyncReplicationControllerDoesNothing(t *testing.T) { ResponseBody: string(body), } testServer := httptest.NewTLSServer(&fakeHandler) - client := client.New(testServer.URL, nil) + client := client.NewOrDie(testServer.URL, nil) fakePodControl := FakePodControl{} @@ -134,7 +134,7 @@ func TestSyncReplicationControllerDeletes(t *testing.T) { ResponseBody: string(body), } testServer := httptest.NewTLSServer(&fakeHandler) - client := client.New(testServer.URL, nil) + client := client.NewOrDie(testServer.URL, nil) fakePodControl := FakePodControl{} @@ -154,7 +154,7 @@ func TestSyncReplicationControllerCreates(t *testing.T) { ResponseBody: string(body), } testServer := httptest.NewTLSServer(&fakeHandler) - client := client.New(testServer.URL, nil) + client := client.NewOrDie(testServer.URL, nil) fakePodControl := FakePodControl{} @@ -174,7 +174,7 @@ func TestCreateReplica(t *testing.T) { ResponseBody: string(body), } testServer := httptest.NewTLSServer(&fakeHandler) - client := client.New(testServer.URL, nil) + client := client.NewOrDie(testServer.URL, nil) podControl := RealPodControl{ kubeClient: client, @@ -307,7 +307,7 @@ func TestSyncronize(t *testing.T) { t.Errorf("Unexpected request for %v", req.RequestURI) }) testServer := httptest.NewServer(mux) - client := client.New(testServer.URL, nil) + client := client.NewOrDie(testServer.URL, nil) manager := NewReplicationManager(client) fakePodControl := FakePodControl{} manager.podControl = &fakePodControl diff --git a/pkg/kubecfg/proxy_server.go b/pkg/kubecfg/proxy_server.go index 8d9415c582..799f5f36f1 100644 --- a/pkg/kubecfg/proxy_server.go +++ b/pkg/kubecfg/proxy_server.go @@ -26,8 +26,6 @@ import ( // ProxyServer is a http.Handler which proxies Kubernetes APIs to remote API server. type ProxyServer struct { - Host string - Auth *client.AuthInfo Client *client.Client } @@ -37,11 +35,9 @@ func newFileHandler(prefix, base string) http.Handler { // NewProxyServer creates and installs a new ProxyServer. // It automatically registers the created ProxyServer to http.DefaultServeMux. -func NewProxyServer(filebase, host string, auth *client.AuthInfo) *ProxyServer { +func NewProxyServer(filebase string, kubeClient *client.Client) *ProxyServer { server := &ProxyServer{ - Host: host, - Auth: auth, - Client: client.New(host, auth), + Client: kubeClient, } http.Handle("/api/", server) http.Handle("/static/", newFileHandler("/static/", filebase)) diff --git a/pkg/service/endpoints_controller_test.go b/pkg/service/endpoints_controller_test.go index 06f46b7057..aaa899147a 100644 --- a/pkg/service/endpoints_controller_test.go +++ b/pkg/service/endpoints_controller_test.go @@ -128,14 +128,12 @@ func TestSyncEndpointsEmpty(t *testing.T) { ResponseBody: string(body), } testServer := httptest.NewTLSServer(&fakeHandler) - client := client.New(testServer.URL, nil) + client := client.NewOrDie(testServer.URL, nil) serviceRegistry := registrytest.ServiceRegistry{} endpoints := NewEndpointController(&serviceRegistry, client) - err := endpoints.SyncServiceEndpoints() - if err != nil { + if err := endpoints.SyncServiceEndpoints(); err != nil { t.Errorf("unexpected error: %v", err) } - } func TestSyncEndpointsError(t *testing.T) { @@ -145,13 +143,12 @@ func TestSyncEndpointsError(t *testing.T) { ResponseBody: string(body), } testServer := httptest.NewTLSServer(&fakeHandler) - client := client.New(testServer.URL, nil) + client := client.NewOrDie(testServer.URL, nil) serviceRegistry := registrytest.ServiceRegistry{ Err: fmt.Errorf("test error"), } endpoints := NewEndpointController(&serviceRegistry, client) - err := endpoints.SyncServiceEndpoints() - if err != serviceRegistry.Err { + if err := endpoints.SyncServiceEndpoints(); err != serviceRegistry.Err { t.Errorf("Errors don't match: %#v %#v", err, serviceRegistry.Err) } } @@ -163,7 +160,7 @@ func TestSyncEndpointsItems(t *testing.T) { ResponseBody: string(body), } testServer := httptest.NewTLSServer(&fakeHandler) - client := client.New(testServer.URL, nil) + client := client.NewOrDie(testServer.URL, nil) serviceRegistry := registrytest.ServiceRegistry{ List: api.ServiceList{ Items: []api.Service{ @@ -176,8 +173,7 @@ func TestSyncEndpointsItems(t *testing.T) { }, } endpoints := NewEndpointController(&serviceRegistry, client) - err := endpoints.SyncServiceEndpoints() - if err != nil { + if err := endpoints.SyncServiceEndpoints(); err != nil { t.Errorf("unexpected error: %v", err) } if len(serviceRegistry.Endpoints.Endpoints) != 1 { @@ -190,7 +186,7 @@ func TestSyncEndpointsPodError(t *testing.T) { StatusCode: 500, } testServer := httptest.NewTLSServer(&fakeHandler) - client := client.New(testServer.URL, nil) + client := client.NewOrDie(testServer.URL, nil) serviceRegistry := registrytest.ServiceRegistry{ List: api.ServiceList{ Items: []api.Service{ @@ -203,8 +199,7 @@ func TestSyncEndpointsPodError(t *testing.T) { }, } endpoints := NewEndpointController(&serviceRegistry, client) - err := endpoints.SyncServiceEndpoints() - if err == nil { + if err := endpoints.SyncServiceEndpoints(); err == nil { t.Error("Unexpected non-error") } } diff --git a/plugin/cmd/scheduler/scheduler.go b/plugin/cmd/scheduler/scheduler.go index 05242cc8dd..6a58f2420e 100644 --- a/plugin/cmd/scheduler/scheduler.go +++ b/plugin/cmd/scheduler/scheduler.go @@ -24,6 +24,7 @@ import ( verflag "github.com/GoogleCloudPlatform/kubernetes/pkg/version/flag" "github.com/GoogleCloudPlatform/kubernetes/plugin/pkg/scheduler" "github.com/GoogleCloudPlatform/kubernetes/plugin/pkg/scheduler/factory" + "github.com/golang/glog" ) var ( @@ -38,7 +39,10 @@ func main() { verflag.PrintAndExitIfRequested() // TODO: security story for plugins! - kubeClient := client.New("http://"+*master, nil) + kubeClient, err := client.New(*master, nil) + if err != nil { + glog.Fatalf("Invalid -master: %v", err) + } configFactory := &factory.ConfigFactory{Client: kubeClient} config := configFactory.Create() diff --git a/plugin/pkg/scheduler/factory/factory_test.go b/plugin/pkg/scheduler/factory/factory_test.go index fb268d7bef..68e1b117ee 100644 --- a/plugin/pkg/scheduler/factory/factory_test.go +++ b/plugin/pkg/scheduler/factory/factory_test.go @@ -37,7 +37,8 @@ func TestCreate(t *testing.T) { T: t, } server := httptest.NewServer(&handler) - factory := ConfigFactory{client.New(server.URL, nil)} + client := client.NewOrDie(server.URL, nil) + factory := ConfigFactory{client} factory.Create() } @@ -87,7 +88,7 @@ func TestCreateWatches(t *testing.T) { T: t, } server := httptest.NewServer(&handler) - factory.Client = client.New(server.URL, nil) + factory.Client = client.NewOrDie(server.URL, nil) // This test merely tests that the correct request is made. item.watchFactory(item.rv) handler.ValidateRequest(t, item.location, "GET", nil) @@ -114,7 +115,8 @@ func TestPollMinions(t *testing.T) { T: t, } server := httptest.NewServer(&handler) - cf := ConfigFactory{client.New(server.URL, nil)} + client := client.NewOrDie(server.URL, nil) + cf := ConfigFactory{client} ce, err := cf.pollMinions() if err != nil { @@ -137,7 +139,7 @@ func TestDefaultErrorFunc(t *testing.T) { T: t, } server := httptest.NewServer(&handler) - factory := ConfigFactory{client.New(server.URL, nil)} + factory := ConfigFactory{client.NewOrDie(server.URL, nil)} queue := cache.NewFIFO() errFunc := factory.makeDefaultErrorFunc(queue) @@ -242,10 +244,10 @@ func TestBind(t *testing.T) { T: t, } server := httptest.NewServer(&handler) - b := binder{client.New(server.URL, nil)} + client := client.NewOrDie(server.URL, nil) + b := binder{client} - err := b.Bind(item.binding) - if err != nil { + if err := b.Bind(item.binding); err != nil { t.Errorf("Unexpected error: %v", err) continue } diff --git a/test/integration/client_test.go b/test/integration/client_test.go index 69cf292f55..b5ccdc526f 100644 --- a/test/integration/client_test.go +++ b/test/integration/client_test.go @@ -43,7 +43,7 @@ func TestClient(t *testing.T) { storage, codec := m.API_v1beta1() s := httptest.NewServer(apiserver.Handle(storage, codec, "/api/v1beta1/")) - client := client.New(s.URL, nil) + client := client.NewOrDie(s.URL, nil) info, err := client.ServerVersion() if err != nil {