Make RESTClient more generic to API version, simplify version handling

pkg/client/client.go

@@ -44,7 +44,7 @@ func (c *Client) ReplicationControllers(namespace string) ReplicationControllerI
 }
 
 func (c *Client) Nodes() NodeInterface {
-	return newNodes(c, c.preV1Beta3)
+	return newNodes(c)
 }
 
 func (c *Client) Events(namespace string) EventInterface {
@@ -78,9 +78,6 @@ type APIStatus interface {
 // Client is the implementation of a Kubernetes client.
 type Client struct {
 	*RESTClient
-
-	// preV1Beta3 is true for v1beta1 and v1beta2
-	preV1Beta3 bool
 }
 
 // ServerVersion retrieves and parses the server's version.
@@ -132,3 +129,8 @@ func IsTimeout(err error) bool {
 	}
 	return false
 }
+
+// preV1Beta3 returns true if the provided API version is an API introduced before v1beta3.
+func preV1Beta3(version string) bool {
+	return version == "v1beta1" || version == "v1beta2"
+}

pkg/client/client_test.go

@@ -165,7 +165,7 @@ func (c *testClient) ValidateCommon(t *testing.T, err error) {
 // buildResourcePath is a convenience function for knowing if a namespace should be in a path param or not
 func buildResourcePath(namespace, resource string) string {
 	if len(namespace) > 0 {
-		if NamespaceInPathFor(testapi.Version()) {
+		if !(testapi.Version() == "v1beta1" || testapi.Version() == "v1beta2") {
 			return path.Join("ns", namespace, resource)
 		}
 	}
@@ -183,7 +183,7 @@ func buildQueryValues(namespace string, query url.Values) url.Values {
 		}
 	}
 	if len(namespace) > 0 {
-		if !NamespaceInPathFor(testapi.Version()) {
+		if testapi.Version() == "v1beta1" || testapi.Version() == "v1beta2" {
 			v.Set("namespace", namespace)
 		}
 	}
@@ -765,7 +765,7 @@ func TestNewMinionPath(t *testing.T) {
 		Response: Response{StatusCode: 200},
 	}
 	cl := c.Setup()
-	cl.preV1Beta3 = false
+	cl.apiVersion = "v1beta3"
 	err := cl.Nodes().Delete("foo")
 	c.Validate(t, nil, err)
 }

pkg/client/fake.go

@@ -90,23 +90,28 @@ func (f HTTPClientFunc) Do(req *http.Request) (*http.Response, error) {
 type FakeRESTClient struct {
 	Client HTTPClient
 	Codec  runtime.Codec
+	Legacy bool
 	Req    *http.Request
 	Resp   *http.Response
 	Err    error
 }
 
 func (c *FakeRESTClient) Get() *Request {
-	return NewRequest(c, "GET", &url.URL{Host: "localhost"}, c.Codec, true)
+	return NewRequest(c, "GET", &url.URL{Host: "localhost"}, c.Codec, c.Legacy, c.Legacy)
 }
+
 func (c *FakeRESTClient) Put() *Request {
-	return NewRequest(c, "PUT", &url.URL{Host: "localhost"}, c.Codec, true)
+	return NewRequest(c, "PUT", &url.URL{Host: "localhost"}, c.Codec, c.Legacy, c.Legacy)
 }
+
 func (c *FakeRESTClient) Post() *Request {
-	return NewRequest(c, "POST", &url.URL{Host: "localhost"}, c.Codec, true)
+	return NewRequest(c, "POST", &url.URL{Host: "localhost"}, c.Codec, c.Legacy, c.Legacy)
 }
+
 func (c *FakeRESTClient) Delete() *Request {
-	return NewRequest(c, "DELETE", &url.URL{Host: "localhost"}, c.Codec, true)
+	return NewRequest(c, "DELETE", &url.URL{Host: "localhost"}, c.Codec, c.Legacy, c.Legacy)
 }
+
 func (c *FakeRESTClient) Do(req *http.Request) (*http.Response, error) {
 	c.Req = req
 	if c.Client != HTTPClient(nil) {

pkg/client/helper.go

@@ -24,6 +24,7 @@ import (
 	"strings"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
 )
 
 // Config holds the common attributes that can be passed to a Kubernetes client on
@@ -34,9 +35,19 @@ type Config struct {
 	// Prefix is the sub path of the server. If not specified, the client will set
 	// a default value.  Use "/" to indicate the server root should be used
 	Prefix string
-	// Version is the API version to talk to. If not specified, the client will use
-	// the preferred version.
+	// Version is the API version to talk to. Must be provided when initializing
+	// a RESTClient directly. When initializing a Client, will be set with the default
+	// code version.
 	Version string
+	// LegacyBehavior defines whether the RESTClient should follow conventions that
+	// existed prior to v1beta3 in Kubernetes - namely, namespace (if specified)
+	// not being part of the path, and resource names allowing mixed case. Set to
+	// true when using Kubernetes v1beta1 or v1beta2.
+	LegacyBehavior bool
+	// Codec specifies the encoding and decoding behavior for runtime.Objects passed
+	// to a RESTClient or Client. Required when initializing a RESTClient, optional
+	// when initializing a Client.
+	Codec runtime.Codec
 
 	// Server requires Basic authentication
 	Username string
@@ -80,16 +91,14 @@ type KubeletConfig struct {
 // is not valid.
 func New(c *Config) (*Client, error) {
 	config := *c
-	if config.Prefix == "" {
-		config.Prefix = "/api"
+	if err := SetKubernetesDefaults(&config); err != nil {
+		return nil, err
 	}
 	client, err := RESTClientFor(&config)
 	if err != nil {
 		return nil, err
 	}
-	version := defaultVersionFor(&config)
-	isPreV1Beta3 := (version == "v1beta1" || version == "v1beta2")
-	return &Client{client, isPreV1Beta3}, nil
+	return &Client{client}, nil
 }
 
 // NewOrDie creates a Kubernetes client and panics if the provided API version is not recognized.
@@ -101,23 +110,45 @@ func NewOrDie(c *Config) *Client {
 	return client
 }
 
-// RESTClientFor returns a RESTClient that satisfies the requested attributes on a client Config
-// object.
-func RESTClientFor(config *Config) (*RESTClient, error) {
-	version := defaultVersionFor(config)
-
-	// Set version
+// SetKubernetesDefaults sets default values on the provided client config for accessing the
+// Kubernetes API or returns an error if any of the defaults are impossible or invalid.
+func SetKubernetesDefaults(config *Config) error {
+	if config.Prefix == "" {
+		config.Prefix = "/api"
+	}
+	if len(config.Version) == 0 {
+		config.Version = defaultVersionFor(config)
+	}
+	version := config.Version
 	versionInterfaces, err := latest.InterfacesFor(version)
 	if err != nil {
-		return nil, fmt.Errorf("API version '%s' is not recognized (valid values: %s)", version, strings.Join(latest.Versions, ", "))
+		return fmt.Errorf("API version '%s' is not recognized (valid values: %s)", version, strings.Join(latest.Versions, ", "))
+	}
+	if config.Codec == nil {
+		config.Codec = versionInterfaces.Codec
+	}
+	config.LegacyBehavior = (version == "v1beta1" || version == "v1beta2")
+	return nil
+}
+
+// RESTClientFor returns a RESTClient that satisfies the requested attributes on a client Config
+// object. Note that a RESTClient may require fields that are optional when initializing a Client.
+// A RESTClient created by this method is generic - it expects to operate on an API that follows
+// the Kubernetes conventions, but may not be the Kubernetes API.
+func RESTClientFor(config *Config) (*RESTClient, error) {
+	if len(config.Version) == 0 {
+		return nil, fmt.Errorf("version is required when initializing a RESTClient")
+	}
+	if config.Codec == nil {
+		return nil, fmt.Errorf("Codec is required when initializing a RESTClient")
 	}
 
 	baseURL, err := defaultServerUrlFor(config)
 	if err != nil {
 		return nil, err
 	}
 
-	client := NewRESTClient(baseURL, versionInterfaces.Codec, NamespaceInPathFor(version))
+	client := NewRESTClient(baseURL, config.Version, config.Codec, config.LegacyBehavior)
 
 	transport, err := TransportFor(config)
 	if err != nil {
@@ -229,17 +260,17 @@ func IsConfigTransportTLS(config *Config) bool {
 	return baseURL.Scheme == "https"
 }
 
-// defaultServerUrlFor is shared between IsConfigTransportTLS and RESTClientFor
+// defaultServerUrlFor is shared between IsConfigTransportTLS and RESTClientFor. It
+// requires Host and Version to be set prior to being called.
 func defaultServerUrlFor(config *Config) (*url.URL, error) {
-	version := defaultVersionFor(config)
 	// TODO: move the default to secure when the apiserver supports TLS by default
 	// config.Insecure is taken to mean "I want HTTPS but don't bother checking the certs against a CA."
 	defaultTLS := config.CertFile != "" || config.Insecure
 	host := config.Host
 	if host == "" {
 		host = "localhost"
 	}
-	return DefaultServerURL(host, config.Prefix, version, defaultTLS)
+	return DefaultServerURL(host, config.Prefix, config.Version, defaultTLS)
 }
 
 // defaultVersionFor is shared between defaultServerUrlFor and RESTClientFor
@@ -252,9 +283,3 @@ func defaultVersionFor(config *Config) string {
 	}
 	return version
 }
-
-// namespaceInPathFor is used to control what api version should use namespace in url paths
-func NamespaceInPathFor(version string) bool {
-	// we use query param for v1beta1/v1beta2, v1beta3+ will use path param
-	return (version != "v1beta1" && version != "v1beta2")
-}

pkg/client/helper_test.go

@@ -85,6 +85,10 @@ func TestIsConfigTransportTLS(t *testing.T) {
 		},
 	}
 	for _, testCase := range testCases {
+		if err := SetKubernetesDefaults(testCase.Config); err != nil {
+			t.Errorf("setting defaults failed for %#v: %v", testCase.Config, err)
+			continue
+		}
 		useTLS := IsConfigTransportTLS(testCase.Config)
 		if testCase.TransportTLS != useTLS {
 			t.Errorf("expected %v for %#v", testCase.TransportTLS, testCase.Config)

pkg/client/minions.go

@@ -35,18 +35,17 @@ type NodeInterface interface {
 
 // nodes implements NodesInterface
 type nodes struct {
-	r          *Client
-	preV1Beta3 bool
+	r *Client
 }
 
 // newNodes returns a nodes object. Uses "minions" as the
 // URL resource name for v1beta1 and v1beta2.
-func newNodes(c *Client, isPreV1Beta3 bool) *nodes {
-	return &nodes{c, isPreV1Beta3}
+func newNodes(c *Client) *nodes {
+	return &nodes{c}
 }
 
 func (c *nodes) resourceName() string {
-	if c.preV1Beta3 {
+	if preV1Beta3(c.r.APIVersion()) {
 		return "minions"
 	}
 	return "nodes"

pkg/client/request.go

@@ -89,8 +89,12 @@ type Request struct {
 	// whether to poll.
 	poller PollFunc
 
-	// If true, put ns/<namespace> in path; if false, add "?namespace=<namespace>" as a query parameter
-	namespaceInPath bool
+	// If true, add "?namespace=<namespace>" as a query parameter, if false put ns/<namespace> in path
+	// Query parameter is considered legacy behavior
+	namespaceInQuery bool
+	// If true, lowercase resource prior to inserting into a path, if false, leave it as is. Preserving
+	// case is considered legacy behavior.
+	preserveResourceCase bool
 
 	// generic components accessible via method setters
 	path    string
@@ -110,15 +114,18 @@ type Request struct {
 	body io.Reader
 }
 
-// NewRequest creates a new request with the core attributes.
-func NewRequest(client HTTPClient, verb string, baseURL *url.URL, codec runtime.Codec, namespaceInPath bool) *Request {
+// NewRequest creates a new request helper object for accessing runtime.Objects on a server.
+func NewRequest(client HTTPClient, verb string, baseURL *url.URL,
+	codec runtime.Codec, namespaceInQuery bool, preserveResourceCase bool) *Request {
 	return &Request{
-		client:          client,
-		verb:            verb,
-		baseURL:         baseURL,
-		codec:           codec,
-		namespaceInPath: namespaceInPath,
-		path:            baseURL.Path,
+		client:  client,
+		verb:    verb,
+		baseURL: baseURL,
+		path:    baseURL.Path,
+
+		codec:                codec,
+		namespaceInQuery:     namespaceInQuery,
+		preserveResourceCase: preserveResourceCase,
 	}
 }
 
@@ -323,11 +330,15 @@ func (r *Request) Poller(poller PollFunc) *Request {
 
 func (r *Request) finalURL() string {
 	p := r.path
-	if r.namespaceInPath {
+	if !r.namespaceInQuery {
 		p = path.Join(p, "ns", r.namespace)
 	}
 	if len(r.resource) != 0 {
-		p = path.Join(p, r.resource)
+		resource := r.resource
+		if !r.preserveResourceCase {
+			resource = strings.ToLower(resource)
+		}
+		p = path.Join(p, resource)
 	}
 	// Join trims trailing slashes, so preserve r.path's trailing slash for backwards compat if nothing was changed
 	if len(r.resourceName) != 0 || len(r.subpath) != 0 {
@@ -342,7 +353,7 @@ func (r *Request) finalURL() string {
 		query.Add(key, value)
 	}
 
-	if !r.namespaceInPath && len(r.namespace) > 0 {
+	if r.namespaceInQuery && len(r.namespace) > 0 {
 		query.Add("namespace", r.namespace)
 	}
 

pkg/client/request_test.go

@@ -75,21 +75,21 @@ func TestRequestWithErrorWontChange(t *testing.T) {
 }
 
 func TestRequestPreservesBaseTrailingSlash(t *testing.T) {
-	r := &Request{baseURL: &url.URL{}, path: "/path/"}
+	r := &Request{baseURL: &url.URL{}, path: "/path/", namespaceInQuery: true}
 	if s := r.finalURL(); s != "/path/" {
 		t.Errorf("trailing slash should be preserved: %s", s)
 	}
 }
 
 func TestRequestAbsPathPreservesTrailingSlash(t *testing.T) {
-	r := (&Request{baseURL: &url.URL{}}).AbsPath("/foo/")
+	r := (&Request{baseURL: &url.URL{}, namespaceInQuery: true}).AbsPath("/foo/")
 	if s := r.finalURL(); s != "/foo/" {
 		t.Errorf("trailing slash should be preserved: %s", s)
 	}
 }
 
 func TestRequestAbsPathJoins(t *testing.T) {
-	r := (&Request{baseURL: &url.URL{}}).AbsPath("foo/bar", "baz")
+	r := (&Request{baseURL: &url.URL{}, namespaceInQuery: true}).AbsPath("foo/bar", "baz")
 	if s := r.finalURL(); s != "foo/bar/baz" {
 		t.Errorf("trailing slash should be preserved: %s", s)
 	}
@@ -100,6 +100,7 @@ func TestRequestSetsNamespace(t *testing.T) {
 		baseURL: &url.URL{
 			Path: "/",
 		},
+		namespaceInQuery: true,
 	}).Namespace("foo")
 	if r.namespace == "" {
 		t.Errorf("namespace should be set: %#v", r)
@@ -112,7 +113,6 @@ func TestRequestSetsNamespace(t *testing.T) {
 		baseURL: &url.URL{
 			Path: "/",
 		},
-		namespaceInPath: true,
 	}).Namespace("foo")
 	if s := r.finalURL(); s != "ns/foo" {
 		t.Errorf("namespace should be in path: %s", s)
@@ -121,9 +121,8 @@ func TestRequestSetsNamespace(t *testing.T) {
 
 func TestRequestOrdersNamespaceInPath(t *testing.T) {
 	r := (&Request{
-		baseURL:         &url.URL{},
-		path:            "/test/",
-		namespaceInPath: true,
+		baseURL: &url.URL{},
+		path:    "/test/",
 	}).Name("bar").Resource("baz").Namespace("foo")
 	if s := r.finalURL(); s != "/test/ns/foo/baz/bar" {
 		t.Errorf("namespace should be in order in path: %s", s)
@@ -212,7 +211,7 @@ func TestTransformResponse(t *testing.T) {
 		{Response: &http.Response{StatusCode: 200, Body: ioutil.NopCloser(bytes.NewReader(invalid))}, Data: invalid},
 	}
 	for i, test := range testCases {
-		r := NewRequest(nil, "", uri, testapi.Codec(), true)
+		r := NewRequest(nil, "", uri, testapi.Codec(), true, true)
 		if test.Response.Body == nil {
 			test.Response.Body = ioutil.NopCloser(bytes.NewReader([]byte{}))
 		}