Merge pull request #2047 from smarterclayton/make_request_testable

Make client.Request/RESTClient more testable and fakeable

cmd/integration/integration.go

@@ -120,7 +120,7 @@ func startComponents(manifestURL string) (apiServerURL string) {
 	}
 
 	cl := client.NewOrDie(&client.Config{Host: apiServer.URL, Version: testapi.Version()})
-	cl.PollPeriod = time.Second * 1
+	cl.PollPeriod = time.Millisecond * 100
 	cl.Sync = true
 
 	helper, err := master.NewEtcdHelper(etcdClient, "")
@@ -312,7 +312,6 @@ func runAtomicPutTest(c *client.Client) {
 				err := c.Get().
 					Path("services").
 					Path(svc.Name).
-					PollPeriod(100 * time.Millisecond).
 					Do().
 					Into(&tmpSvc)
 				if err != nil {

pkg/api/types.go

@@ -603,6 +603,7 @@ type Status struct {
 type StatusDetails struct {
 	// The ID attribute of the resource associated with the status StatusReason
 	// (when there is a single ID which can be described).
+	// TODO: replace with Name with v1beta3
 	ID string `json:"id,omitempty" yaml:"id,omitempty"`
 	// The kind attribute of the resource associated with the status StatusReason.
 	// On some operations may differ from the requested resource Kind.

pkg/client/flags_test.go

@@ -0,0 +1,76 @@
+/*
+Copyright 2014 Google Inc. All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package client
+
+import (
+	"testing"
+
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
+)
+
+type fakeFlagSet struct {
+	t   *testing.T
+	set util.StringSet
+}
+
+func (f *fakeFlagSet) StringVar(p *string, name, value, usage string) {
+	if p == nil {
+		f.t.Errorf("unexpected nil pointer")
+	}
+	if usage == "" {
+		f.t.Errorf("unexpected empty usage")
+	}
+	f.set.Insert(name)
+}
+
+func (f *fakeFlagSet) BoolVar(p *bool, name string, value bool, usage string) {
+	if p == nil {
+		f.t.Errorf("unexpected nil pointer")
+	}
+	if usage == "" {
+		f.t.Errorf("unexpected empty usage")
+	}
+	f.set.Insert(name)
+}
+
+func (f *fakeFlagSet) UintVar(p *uint, name string, value uint, usage string) {
+	if p == nil {
+		f.t.Errorf("unexpected nil pointer")
+	}
+	if usage == "" {
+		f.t.Errorf("unexpected empty usage")
+	}
+	f.set.Insert(name)
+}
+
+func TestBindClientConfigFlags(t *testing.T) {
+	flags := &fakeFlagSet{t, util.StringSet{}}
+	config := &Config{}
+	BindClientConfigFlags(flags, config)
+	if len(flags.set) != 6 {
+		t.Errorf("unexpected flag set: %#v", flags)
+	}
+}
+
+func TestBindKubeletClientConfigFlags(t *testing.T) {
+	flags := &fakeFlagSet{t, util.StringSet{}}
+	config := &KubeletConfig{}
+	BindKubeletClientConfigFlags(flags, config)
+	if len(flags.set) != 5 {
+		t.Errorf("unexpected flag set: %#v", flags)
+	}
+}

pkg/client/request.go

@@ -28,32 +28,65 @@ import (
 	"time"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/watch"
 	watchjson "github.com/GoogleCloudPlatform/kubernetes/pkg/watch/json"
-	"github.com/golang/glog"
 )
 
 // specialParams lists parameters that are handled specially and which users of Request
 // are therefore not allowed to set manually.
 var specialParams = util.NewStringSet("sync", "timeout")
 
+// PollFunc is called when a server operation returns 202 accepted. The name of the
+// operation is extracted from the response and passed to this function. Return a
+// request to retrieve the result of the operation, or false for the second argument
+// if polling should end.
+type PollFunc func(name string) (*Request, bool)
+
+// HTTPClient is an interface for testing a request object.
+type HTTPClient interface {
+	Do(req *http.Request) (*http.Response, error)
+}
+
 // Request allows for building up a request to a server in a chained fashion.
 // Any errors are stored until the end of your call, so you only have to
 // check once.
 type Request struct {
-	c          *RESTClient
-	err        error
-	verb       string
-	path       string
-	body       io.Reader
-	params     map[string]string
-	selector   labels.Selector
-	timeout    time.Duration
-	sync       bool
-	pollPeriod time.Duration
+	// required
+	client  HTTPClient
+	verb    string
+	baseURL *url.URL
+	codec   runtime.Codec
+
+	// optional, will be invoked if the server returns a 202 to decide
+	// whether to poll.
+	poller PollFunc
+
+	// accessible via method setters
+	path     string
+	params   map[string]string
+	selector labels.Selector
+	sync     bool
+	timeout  time.Duration
+
+	// output
+	err  error
+	body io.Reader
+}
+
+// NewRequest creates a new request with the core attributes.
+func NewRequest(client HTTPClient, verb string, baseURL *url.URL, codec runtime.Codec) *Request {
+	return &Request{
+		client:  client,
+		verb:    verb,
+		baseURL: baseURL,
+		codec:   codec,
+
+		path: baseURL.Path,
+	}
 }
 
 // Path appends an item to the request path. You must call Path at least once.
@@ -76,6 +109,9 @@ func (r *Request) Sync(sync bool) *Request {
 
 // Namespace applies the namespace scope to a request
 func (r *Request) Namespace(namespace string) *Request {
+	if r.err != nil {
+		return r
+	}
 	if len(namespace) > 0 {
 		return r.setParam("namespace", namespace)
 	}
@@ -135,6 +171,9 @@ func (r *Request) setParam(paramName, value string) *Request {
 		r.err = fmt.Errorf("must set %v through the corresponding function, not directly.", paramName)
 		return r
 	}
+	if r.params == nil {
+		r.params = make(map[string]string)
+	}
 	r.params[paramName] = value
 	return r
 }
@@ -172,7 +211,7 @@ func (r *Request) Body(obj interface{}) *Request {
 	case io.Reader:
 		r.body = t
 	case runtime.Object:
-		data, err := r.c.Codec.Encode(t)
+		data, err := r.codec.Encode(t)
 		if err != nil {
 			r.err = err
 			return r
@@ -184,21 +223,24 @@ func (r *Request) Body(obj interface{}) *Request {
 	return r
 }
 
-// PollPeriod sets the poll period.
-// If the server sends back a "working" status message, then repeatedly poll the server
-// to see if the operation has completed yet, waiting 'd' between each poll.
-// If you want to handle the "working" status yourself (it'll be delivered as StatusErr),
-// set d to 0 to turn off this behavior.
-func (r *Request) PollPeriod(d time.Duration) *Request {
+// NoPoll indicates a server "working" response should be returned as an error
+func (r *Request) NoPoll() *Request {
+	return r.Poller(nil)
+}
+
+// Poller indicates this request should use the specified poll function to determine whether
+// a server "working" response should be retried. The poller is responsible for waiting or
+// outputting messages to the client.
+func (r *Request) Poller(poller PollFunc) *Request {
 	if r.err != nil {
 		return r
 	}
-	r.pollPeriod = d
+	r.poller = poller
 	return r
 }
 
 func (r *Request) finalURL() string {
-	finalURL := *r.c.baseURL
+	finalURL := *r.baseURL
 	finalURL.Path = r.path
 	query := url.Values{}
 	for key, value := range r.params {
@@ -227,18 +269,18 @@ func (r *Request) Watch() (watch.Interface, error) {
 	if err != nil {
 		return nil, err
 	}
-	client := r.c.Client
+	client := r.client
 	if client == nil {
 		client = http.DefaultClient
 	}
-	response, err := client.Do(req)
+	resp, err := client.Do(req)
 	if err != nil {
 		return nil, err
 	}
-	if response.StatusCode != http.StatusOK {
-		return nil, fmt.Errorf("Got status: %v", response.StatusCode)
+	if resp.StatusCode != http.StatusOK {
+		return nil, fmt.Errorf("Got status: %v", resp.StatusCode)
 	}
-	return watch.NewStreamWatcher(watchjson.NewDecoder(response.Body, r.c.Codec)), nil
+	return watch.NewStreamWatcher(watchjson.NewDecoder(resp.Body, r.codec)), nil
 }
 
 // Stream formats and executes the request, and offers streaming of the response.
@@ -251,51 +293,106 @@ func (r *Request) Stream() (io.ReadCloser, error) {
 	if err != nil {
 		return nil, err
 	}
-	client := r.c.Client
+	client := r.client
 	if client == nil {
 		client = http.DefaultClient
 	}
-	response, err := client.Do(req)
+	resp, err := client.Do(req)
 	if err != nil {
 		return nil, err
 	}
-	return response.Body, nil
+	return resp.Body, nil
 }
 
-// Do formats and executes the request. Returns the API object received, or an error.
+// Do formats and executes the request. Returns a Result object for easy response
+// processing. Handles polling the server in the event a continuation was sent.
 func (r *Request) Do() Result {
+	client := r.client
+	if client == nil {
+		client = http.DefaultClient
+	}
+
 	for {
 		if r.err != nil {
 			return Result{err: r.err}
 		}
+
 		req, err := http.NewRequest(r.verb, r.finalURL(), r.body)
 		if err != nil {
 			return Result{err: err}
 		}
-		respBody, created, err := r.c.doRequest(req)
+
+		resp, err := client.Do(req)
 		if err != nil {
-			if s, ok := err.(APIStatus); ok {
-				status := s.Status()
-				if status.Status == api.StatusWorking && r.pollPeriod != 0 {
-					if status.Details != nil {
-						id := status.Details.ID
-						if len(id) > 0 {
-							glog.Infof("Waiting for completion of /operations/%s", id)
-							time.Sleep(r.pollPeriod)
-							// Make a poll request
-							pollOp := r.c.PollFor(id).PollPeriod(r.pollPeriod)
-							// Could also say "return r.Do()" but this way doesn't grow the callstack.
-							r = pollOp
-							continue
-						}
-					}
-				}
-			}
+			return Result{err: err}
 		}
-		return Result{respBody, created, err, r.c.Codec}
+
+		respBody, created, err := r.transformResponse(resp, req)
+		if poll, ok := r.shouldPoll(err); ok {
+			r = poll
+			continue
+		}
+
+		return Result{respBody, created, err, r.codec}
 	}
 }
 
+// shouldPoll checks the server error for an incomplete operation
+// and if found returns a request that would check the response.
+// If no polling is necessary or possible, it will return false.
+func (r *Request) shouldPoll(err error) (*Request, bool) {
+	if err == nil || r.poller == nil {
+		return nil, false
+	}
+	apistatus, ok := err.(APIStatus)
+	if !ok {
+		return nil, false
+	}
+	status := apistatus.Status()
+	if status.Status != api.StatusWorking {
+		return nil, false
+	}
+	if status.Details == nil || len(status.Details.ID) == 0 {
+		return nil, false
+	}
+	return r.poller(status.Details.ID)
+}
+
+// transformResponse converts an API response into a structured API object.
+func (r *Request) transformResponse(resp *http.Response, req *http.Request) ([]byte, bool, error) {
+	defer resp.Body.Close()
+
+	body, err := ioutil.ReadAll(resp.Body)
+	if err != nil {
+		return nil, false, err
+	}
+
+	// Did the server give us a status response?
+	isStatusResponse := false
+	var status api.Status
+	if err := r.codec.DecodeInto(body, &status); err == nil && status.Status != "" {
+		isStatusResponse = true
+	}
+
+	switch {
+	case resp.StatusCode < http.StatusOK || resp.StatusCode > http.StatusPartialContent:
+		if !isStatusResponse {
+			return nil, false, fmt.Errorf("request [%#v] failed (%d) %s: %s", req, resp.StatusCode, resp.Status, string(body))
+		}
+		return nil, false, errors.FromObject(&status)
+	}
+
+	// If the server gave us a status back, look at what it was.
+	if isStatusResponse && status.Status != api.StatusSuccess {
+		// "Working" requests need to be handled specially.
+		// "Failed" requests are clearly just an error and it makes sense to return them as such.
+		return nil, false, errors.FromObject(&status)
+	}
+
+	created := resp.StatusCode == http.StatusCreated
+	return body, created, err
+}
+
 // Result contains the result of calling Request.Do().
 type Result struct {
 	body    []byte

pkg/client/request_test.go

@@ -19,9 +19,12 @@ package client
 import (
 	"bytes"
 	"encoding/base64"
+	"errors"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
+	"os"
 	"reflect"
 	"strings"
 	"testing"
@@ -29,6 +32,7 @@ import (
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/testapi"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta1"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta2"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
@@ -38,6 +42,238 @@ import (
 	watchjson "github.com/GoogleCloudPlatform/kubernetes/pkg/watch/json"
 )
 
+func skipPolling(name string) (*Request, bool) {
+	return nil, false
+}
+
+func TestRequestWithErrorWontChange(t *testing.T) {
+	original := Request{err: errors.New("test")}
+	r := original
+	changed := r.Param("foo", "bar").
+		SelectorParam("labels", labels.Set{"a": "b"}.AsSelector()).
+		UintParam("uint", 1).
+		AbsPath("/abs").
+		Path("test").
+		ParseSelectorParam("foo", "a=b").
+		Namespace("new").
+		NoPoll().
+		Body("foo").
+		Poller(skipPolling).
+		Timeout(time.Millisecond).
+		Sync(true)
+	if changed != &r {
+		t.Errorf("returned request should point to the same object")
+	}
+	if !reflect.DeepEqual(&original, changed) {
+		t.Errorf("expected %#v, got %#v", &original, changed)
+	}
+}
+
+func TestRequestParseSelectorParam(t *testing.T) {
+	r := (&Request{}).ParseSelectorParam("foo", "a")
+	if r.err == nil || r.params != nil {
+		t.Errorf("should have set err and left params nil: %#v", r)
+	}
+}
+
+func TestRequestParam(t *testing.T) {
+	r := (&Request{}).Param("foo", "a")
+	if !reflect.DeepEqual(map[string]string{"foo": "a"}, r.params) {
+		t.Errorf("should have set a param: %#v", r)
+	}
+}
+
+type NotAnAPIObject struct{}
+
+func (NotAnAPIObject) IsAnAPIObject() {}
+
+func TestRequestBody(t *testing.T) {
+	// test unknown type
+	r := (&Request{}).Body([]string{"test"})
+	if r.err == nil || r.body != nil {
+		t.Errorf("should have set err and left body nil: %#v", r)
+	}
+
+	// test error set when failing to read file
+	f, err := ioutil.TempFile("", "test")
+	if err != nil {
+		t.Fatalf("unable to create temp file")
+	}
+	os.Remove(f.Name())
+	r = (&Request{}).Body(f.Name())
+	if r.err == nil || r.body != nil {
+		t.Errorf("should have set err and left body nil: %#v", r)
+	}
+
+	// test unencodable api object
+	r = (&Request{codec: latest.Codec}).Body(&NotAnAPIObject{})
+	if r.err == nil || r.body != nil {
+		t.Errorf("should have set err and left body nil: %#v", r)
+	}
+}
+
+func TestResultIntoWithErrReturnsErr(t *testing.T) {
+	res := Result{err: errors.New("test")}
+	if err := res.Into(&api.Pod{}); err != res.err {
+		t.Errorf("should have returned exact error from result")
+	}
+}
+
+func TestTransformResponse(t *testing.T) {
+	invalid := []byte("aaaaa")
+	uri, _ := url.Parse("http://localhost")
+	testCases := []struct {
+		Response *http.Response
+		Data     []byte
+		Created  bool
+		Error    bool
+	}{
+		{Response: &http.Response{StatusCode: 200}, Data: []byte{}},
+		{Response: &http.Response{StatusCode: 201}, Data: []byte{}, Created: true},
+		{Response: &http.Response{StatusCode: 199}, Error: true},
+		{Response: &http.Response{StatusCode: 500}, Error: true},
+		{Response: &http.Response{StatusCode: 200, Body: ioutil.NopCloser(bytes.NewReader(invalid))}, Data: invalid},
+		{Response: &http.Response{StatusCode: 200, Body: ioutil.NopCloser(bytes.NewReader(invalid))}, Data: invalid},
+	}
+	for i, test := range testCases {
+		r := NewRequest(nil, "", uri, testapi.Codec())
+		if test.Response.Body == nil {
+			test.Response.Body = ioutil.NopCloser(bytes.NewReader([]byte{}))
+		}
+		response, created, err := r.transformResponse(test.Response, &http.Request{})
+		hasErr := err != nil
+		if hasErr != test.Error {
+			t.Errorf("%d: unexpected error: %f %v", i, test.Error, err)
+		}
+		if !(test.Data == nil && response == nil) && !reflect.DeepEqual(test.Data, response) {
+			t.Errorf("%d: unexpected response: %#v %#v", i, test.Data, response)
+		}
+		if test.Created != created {
+			t.Errorf("%d: expected created %f, got %f", i, test.Created, created)
+		}
+	}
+}
+
+type clientFunc func(req *http.Request) (*http.Response, error)
+
+func (f clientFunc) Do(req *http.Request) (*http.Response, error) {
+	return f(req)
+}
+
+func TestRequestWatch(t *testing.T) {
+	testCases := []struct {
+		Request *Request
+		Err     bool
+	}{
+		{
+			Request: &Request{err: errors.New("bail")},
+			Err:     true,
+		},
+		{
+			Request: &Request{baseURL: &url.URL{}, path: "%"},
+			Err:     true,
+		},
+		{
+			Request: &Request{
+				client: clientFunc(func(req *http.Request) (*http.Response, error) {
+					return nil, errors.New("err")
+				}),
+				baseURL: &url.URL{},
+			},
+			Err: true,
+		},
+		{
+			Request: &Request{
+				client: clientFunc(func(req *http.Request) (*http.Response, error) {
+					return &http.Response{StatusCode: http.StatusForbidden}, nil
+				}),
+				baseURL: &url.URL{},
+			},
+			Err: true,
+		},
+	}
+	for i, testCase := range testCases {
+		watch, err := testCase.Request.Watch()
+		hasErr := err != nil
+		if hasErr != testCase.Err {
+			t.Errorf("%d: expected %f, got %f: %v", i, testCase.Err, hasErr, err)
+		}
+		if hasErr && watch != nil {
+			t.Errorf("%d: watch should be nil when error is returned", i)
+		}
+	}
+}
+
+func TestRequestStream(t *testing.T) {
+	testCases := []struct {
+		Request *Request
+		Err     bool
+	}{
+		{
+			Request: &Request{err: errors.New("bail")},
+			Err:     true,
+		},
+		{
+			Request: &Request{baseURL: &url.URL{}, path: "%"},
+			Err:     true,
+		},
+		{
+			Request: &Request{
+				client: clientFunc(func(req *http.Request) (*http.Response, error) {
+					return nil, errors.New("err")
+				}),
+				baseURL: &url.URL{},
+			},
+			Err: true,
+		},
+	}
+	for i, testCase := range testCases {
+		body, err := testCase.Request.Stream()
+		hasErr := err != nil
+		if hasErr != testCase.Err {
+			t.Errorf("%d: expected %f, got %f: %v", i, testCase.Err, hasErr, err)
+		}
+		if hasErr && body != nil {
+			t.Errorf("%d: body should be nil when error is returned", i)
+		}
+	}
+}
+
+func TestRequestDo(t *testing.T) {
+	testCases := []struct {
+		Request *Request
+		Err     bool
+	}{
+		{
+			Request: &Request{err: errors.New("bail")},
+			Err:     true,
+		},
+		{
+			Request: &Request{baseURL: &url.URL{}, path: "%"},
+			Err:     true,
+		},
+		{
+			Request: &Request{
+				client: clientFunc(func(req *http.Request) (*http.Response, error) {
+					return nil, errors.New("err")
+				}),
+				baseURL: &url.URL{},
+			},
+			Err: true,
+		},
+	}
+	for i, testCase := range testCases {
+		body, err := testCase.Request.Do().Raw()
+		hasErr := err != nil
+		if hasErr != testCase.Err {
+			t.Errorf("%d: expected %f, got %f: %v", i, testCase.Err, hasErr, err)
+		}
+		if hasErr && body != nil {
+			t.Errorf("%d: body should be nil when error is returned", i)
+		}
+	}
+}
+
 func TestDoRequestNewWay(t *testing.T) {
 	reqBody := "request body"
 	expectedObj := &api.Service{Port: 12345}
@@ -48,6 +284,7 @@ func TestDoRequestNewWay(t *testing.T) {
 		T:            t,
 	}
 	testServer := httptest.NewServer(&fakeHandler)
+	defer testServer.Close()
 	c := NewOrDie(&Config{Host: testServer.URL, Version: "v1beta2", Username: "user", Password: "pass"})
 	obj, err := c.Verb("POST").
 		Path("foo/bar").
@@ -351,15 +588,18 @@ func TestBody(t *testing.T) {
 	}
 }
 
-func TestSetPollPeriod(t *testing.T) {
+func TestSetPoller(t *testing.T) {
 	c := NewOrDie(&Config{})
 	r := c.Get()
-	if r.pollPeriod == 0 {
+	if c.PollPeriod == 0 {
 		t.Errorf("polling should be on by default")
 	}
-	r.PollPeriod(time.Hour)
-	if r.pollPeriod != time.Hour {
-		t.Errorf("'PollPeriod' doesn't work")
+	if r.poller == nil {
+		t.Errorf("polling should be on by default")
+	}
+	r.NoPoll()
+	if r.poller != nil {
+		t.Errorf("'NoPoll' doesn't work")
 	}
 }
 
@@ -374,6 +614,16 @@ func TestPolling(t *testing.T) {
 
 	callNumber := 0
 	testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if callNumber == 0 {
+			if r.URL.Path != "/api/v1beta1/" {
+				t.Fatalf("unexpected request URL path %s", r.URL.Path)
+			}
+		} else {
+			if r.URL.Path != "/api/v1beta1/operations/1234" {
+				t.Fatalf("unexpected request URL path %s", r.URL.Path)
+			}
+		}
+		t.Logf("About to write %d", callNumber)
 		data, err := v1beta1.Codec.Encode(objects[callNumber])
 		if err != nil {
 			t.Errorf("Unexpected encode error")
@@ -383,11 +633,11 @@ func TestPolling(t *testing.T) {
 	}))
 
 	c := NewOrDie(&Config{Host: testServer.URL, Version: "v1beta1", Username: "user", Password: "pass"})
-
+	c.PollPeriod = 1 * time.Millisecond
 	trials := []func(){
 		func() {
 			// Check that we do indeed poll when asked to.
-			obj, err := c.Get().PollPeriod(5 * time.Millisecond).Do().Get()
+			obj, err := c.Get().Do().Get()
 			if err != nil {
 				t.Errorf("Unexpected error: %v %#v", err, err)
 				return
@@ -402,7 +652,7 @@ func TestPolling(t *testing.T) {
 		},
 		func() {
 			// Check that we don't poll when asked not to.
-			obj, err := c.Get().PollPeriod(0).Do().Get()
+			obj, err := c.Get().NoPoll().Do().Get()
 			if err == nil {
 				t.Errorf("Unexpected non error: %v", obj)
 				return

pkg/client/restclient.go

@@ -17,16 +17,13 @@ limitations under the License.
 package client
 
 import (
-	"fmt"
-	"io/ioutil"
-	"net/http"
 	"net/url"
 	"strings"
 	"time"
 
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
+
+	"github.com/golang/glog"
 )
 
 // RESTClient imposes common Kubernetes API conventions on a set of resource paths.
@@ -45,7 +42,11 @@ type RESTClient struct {
 
 	// Set specific behavior of the client.  If not set http.DefaultClient will be
 	// used.
-	Client *http.Client
+	Client HTTPClient
+
+	// Set the poll behavior of this client. If not set the DefaultPoll method will
+	// be called.
+	Poller PollFunc
 
 	Sync       bool
 	PollPeriod time.Duration
@@ -68,56 +69,13 @@ func NewRESTClient(baseURL *url.URL, c runtime.Codec) *RESTClient {
 		Codec:   c,
 
 		// Make asynchronous requests by default
-		// TODO: flip me to the default
 		Sync: false,
+
 		// Poll frequently when asynchronous requests are provided
 		PollPeriod: time.Second * 2,
 	}
 }
 
-// doRequest executes a request against a server
-func (c *RESTClient) doRequest(request *http.Request) ([]byte, bool, error) {
-	client := c.Client
-	if client == nil {
-		client = http.DefaultClient
-	}
-
-	response, err := client.Do(request)
-	if err != nil {
-		return nil, false, err
-	}
-	defer response.Body.Close()
-	body, err := ioutil.ReadAll(response.Body)
-	if err != nil {
-		return body, false, err
-	}
-
-	// Did the server give us a status response?
-	isStatusResponse := false
-	var status api.Status
-	if err := c.Codec.DecodeInto(body, &status); err == nil && status.Status != "" {
-		isStatusResponse = true
-	}
-
-	switch {
-	case response.StatusCode < http.StatusOK || response.StatusCode > http.StatusPartialContent:
-		if !isStatusResponse {
-			return nil, false, fmt.Errorf("request [%#v] failed (%d) %s: %s", request, response.StatusCode, response.Status, string(body))
-		}
-		return nil, false, errors.FromObject(&status)
-	}
-
-	// If the server gave us a status back, look at what it was.
-	if isStatusResponse && status.Status != api.StatusSuccess {
-		// "Working" requests need to be handled specially.
-		// "Failed" requests are clearly just an error and it makes sense to return them as such.
-		return nil, false, errors.FromObject(&status)
-	}
-
-	created := response.StatusCode == http.StatusCreated
-	return body, created, err
-}
-
 // Verb begins a request with a verb (GET, POST, PUT, DELETE).
 //
 // Example usage of RESTClient's request building interface:
@@ -136,15 +94,11 @@ func (c *RESTClient) Verb(verb string) *Request {
 	// if c.Client != nil {
 	// 	timeout = c.Client.Timeout
 	// }
-	return &Request{
-		verb:       verb,
-		c:          c,
-		path:       c.baseURL.Path,
-		sync:       c.Sync,
-		timeout:    c.Timeout,
-		params:     map[string]string{},
-		pollPeriod: c.PollPeriod,
+	poller := c.Poller
+	if poller == nil {
+		poller = c.DefaultPoll
 	}
+	return NewRequest(c.Client, verb, c.baseURL, c.Codec).Poller(poller).Sync(c.Sync).Timeout(c.Timeout)
 }
 
 // Post begins a POST request. Short for c.Verb("POST").
@@ -168,6 +122,16 @@ func (c *RESTClient) Delete() *Request {
 }
 
 // PollFor makes a request to do a single poll of the completion of the given operation.
-func (c *RESTClient) PollFor(operationID string) *Request {
-	return c.Get().Path("operations").Path(operationID).Sync(false).PollPeriod(0)
+func (c *RESTClient) Operation(name string) *Request {
+	return c.Get().Path("operations").Path(name).Sync(false).NoPoll()
+}
+
+func (c *RESTClient) DefaultPoll(name string) (*Request, bool) {
+	if c.PollPeriod == 0 {
+		return nil, false
+	}
+	glog.Infof("Waiting for completion of operation %s", name)
+	time.Sleep(c.PollPeriod)
+	// Make a poll request
+	return c.Operation(name).Poller(c.DefaultPoll), true
 }

pkg/client/restclient_test.go

@@ -19,7 +19,6 @@ package client
 import (
 	"net/http"
 	"net/http/httptest"
-	"net/url"
 	"reflect"
 	"testing"
 
@@ -100,36 +99,6 @@ func TestValidatesHostParameter(t *testing.T) {
 	}
 }
 
-func TestDoRequest(t *testing.T) {
-	invalid := "aaaaa"
-	uri, _ := url.Parse("http://localhost")
-	testClients := []testClient{
-		{Request: testRequest{Method: "GET", Path: "good"}, Response: Response{StatusCode: 200}},
-		{Request: testRequest{Method: "GET", Path: "good"}, Response: Response{StatusCode: 201}, Created: true},
-		{Request: testRequest{Method: "GET", Path: "bad%ZZ"}, 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}},
-		{Request: testRequest{Method: "GET", Path: "failread"}, Response: Response{StatusCode: 200, RawBody: &invalid}},
-		{Client: &Client{&RESTClient{baseURL: uri, Codec: testapi.Codec()}}, Request: testRequest{Method: "GET", Path: "nocertificate"}, Error: true},
-	}
-	for _, c := range testClients {
-		client := c.Setup()
-		prefix := *client.baseURL
-		prefix.Path += c.Request.Path
-		request := &http.Request{
-			Method: c.Request.Method,
-			Header: make(http.Header),
-			URL:    &prefix,
-		}
-		response, created, err := client.doRequest(request)
-		if c.Created != created {
-			t.Errorf("expected created %f, got %f", c.Created, created)
-		}
-		//t.Logf("dorequest: %#v\n%#v\n%v", request.URL, response, err)
-		c.ValidateRaw(t, response, err)
-	}
-}
-
 func TestDoRequestBearer(t *testing.T) {
 	status := &api.Status{Status: api.StatusWorking}
 	expectedBody, _ := latest.Codec.Encode(status)
@@ -139,12 +108,15 @@ func TestDoRequestBearer(t *testing.T) {
 		T:            t,
 	}
 	testServer := httptest.NewServer(&fakeHandler)
-	request, _ := http.NewRequest("GET", testServer.URL+"/foo/bar", nil)
+	request, _ := http.NewRequest("GET", testServer.URL, nil)
 	c, err := RESTClientFor(&Config{Host: testServer.URL, BearerToken: "test"})
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-	c.doRequest(request)
+	err = c.Get().Do().Error()
+	if err == nil {
+		t.Fatalf("unexpected non-error: %v", err)
+	}
 	if fakeHandler.RequestReceived.Header.Get("Authorization") != "Bearer test" {
 		t.Errorf("Request is missing authorization header: %#v", *request)
 	}
@@ -159,18 +131,17 @@ func TestDoRequestAccepted(t *testing.T) {
 		T:            t,
 	}
 	testServer := httptest.NewServer(&fakeHandler)
-	request, _ := http.NewRequest("GET", testServer.URL+"/foo/bar", nil)
-	c, err := RESTClientFor(&Config{Host: testServer.URL, Username: "test"})
+	c, err := RESTClientFor(&Config{Host: testServer.URL, Username: "test", Version: testapi.Version()})
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-	body, _, err := c.doRequest(request)
-	if fakeHandler.RequestReceived.Header["Authorization"] == nil {
-		t.Errorf("Request is missing authorization header: %#v", *request)
-	}
+	body, err := c.Get().Path("test").Do().Raw()
 	if err == nil {
 		t.Fatalf("Unexpected non-error")
 	}
+	if fakeHandler.RequestReceived.Header["Authorization"] == nil {
+		t.Errorf("Request is missing authorization header: %#v", fakeHandler.RequestReceived)
+	}
 	se, ok := err.(APIStatus)
 	if !ok {
 		t.Fatalf("Unexpected kind of error: %#v", err)
@@ -179,9 +150,9 @@ func TestDoRequestAccepted(t *testing.T) {
 		t.Errorf("Unexpected status: %#v %#v", se.Status(), status)
 	}
 	if body != nil {
-		t.Errorf("Expected nil body, but saw: '%s'", body)
+		t.Errorf("Expected nil body, but saw: '%s'", string(body))
 	}
-	fakeHandler.ValidateRequest(t, "/foo/bar", "GET", nil)
+	fakeHandler.ValidateRequest(t, "/"+testapi.Version()+"/test", "GET", nil)
 }
 
 func TestDoRequestAcceptedSuccess(t *testing.T) {
@@ -193,17 +164,16 @@ func TestDoRequestAcceptedSuccess(t *testing.T) {
 		T:            t,
 	}
 	testServer := httptest.NewServer(&fakeHandler)
-	request, _ := http.NewRequest("GET", testServer.URL+"/foo/bar", nil)
-	c, err := RESTClientFor(&Config{Host: testServer.URL, Username: "user", Password: "pass"})
+	c, err := RESTClientFor(&Config{Host: testServer.URL, Username: "user", Password: "pass", Version: testapi.Version()})
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-	body, _, err := c.doRequest(request)
-	if fakeHandler.RequestReceived.Header["Authorization"] == nil {
-		t.Errorf("Request is missing authorization header: %#v", *request)
-	}
+	body, err := c.Get().Path("test").Do().Raw()
 	if err != nil {
-		t.Errorf("Unexpected error %#v", err)
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if fakeHandler.RequestReceived.Header["Authorization"] == nil {
+		t.Errorf("Request is missing authorization header: %#v", fakeHandler.RequestReceived)
 	}
 	statusOut, err := latest.Codec.Decode(body)
 	if err != nil {
@@ -212,7 +182,7 @@ func TestDoRequestAcceptedSuccess(t *testing.T) {
 	if !reflect.DeepEqual(status, statusOut) {
 		t.Errorf("Unexpected mis-match. Expected %#v.  Saw %#v", status, statusOut)
 	}
-	fakeHandler.ValidateRequest(t, "/foo/bar", "GET", nil)
+	fakeHandler.ValidateRequest(t, "/"+testapi.Version()+"/test", "GET", nil)
 }
 
 func TestDoRequestFailed(t *testing.T) {
@@ -224,12 +194,11 @@ func TestDoRequestFailed(t *testing.T) {
 		T:            t,
 	}
 	testServer := httptest.NewServer(&fakeHandler)
-	request, _ := http.NewRequest("GET", testServer.URL+"/foo/bar", nil)
 	c, err := RESTClientFor(&Config{Host: testServer.URL})
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-	body, _, err := c.doRequest(request)
+	body, err := c.Get().Do().Raw()
 	if err == nil || body != nil {
 		t.Errorf("unexpected non-error: %#v", body)
 	}
@@ -252,12 +221,12 @@ func TestDoRequestCreated(t *testing.T) {
 		T:            t,
 	}
 	testServer := httptest.NewServer(&fakeHandler)
-	request, _ := http.NewRequest("GET", testServer.URL+"/foo/bar", nil)
-	c, err := RESTClientFor(&Config{Host: testServer.URL, Username: "user", Password: "pass"})
+	c, err := RESTClientFor(&Config{Host: testServer.URL, Username: "user", Password: "pass", Version: testapi.Version()})
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-	body, created, err := c.doRequest(request)
+	created := false
+	body, err := c.Get().Path("test").Do().WasCreated(&created).Raw()
 	if err != nil {
 		t.Errorf("Unexpected error %#v", err)
 	}
@@ -271,5 +240,12 @@ func TestDoRequestCreated(t *testing.T) {
 	if !reflect.DeepEqual(status, statusOut) {
 		t.Errorf("Unexpected mis-match. Expected %#v.  Saw %#v", status, statusOut)
 	}
-	fakeHandler.ValidateRequest(t, "/foo/bar", "GET", nil)
+	fakeHandler.ValidateRequest(t, "/"+testapi.Version()+"/test", "GET", nil)
+}
+
+func TestDefaultPoll(t *testing.T) {
+	c := &RESTClient{PollPeriod: 0}
+	if req, ok := c.DefaultPoll("test"); req != nil || ok {
+		t.Errorf("expected nil request and not poll")
+	}
 }