Merge pull request #48991 from smarterclayton/cleanup_restclient

Automatic merge from submit-queue

Remove old, core/v1 specific constructs from RESTClient

Now that metav1 is abstracted from the APIs, RESTClient should also be agnostic to the core API.

* Remove `LabelSelectorParam` and `FieldSelectorParam` - use `VersionedParams` with `ListOptions`
* Remove `UintParam`
* Remove all legacy field selector logic from `VersionedParams` - ParameterCodec now handles that
* Remove special parameters (like `timeout`) which is no longer set by most clients

pkg/kubectl/resource/helper.go

@@ -58,6 +58,7 @@ func (m *Helper) Get(namespace, name string, export bool) (runtime.Object, error
 		Resource(m.Resource).
 		Name(name)
 	if export {
+		// TODO: I should be part of GetOptions
 		req.Param("export", strconv.FormatBool(export))
 	}
 	return req.Do().Get()
@@ -68,8 +69,11 @@ func (m *Helper) List(namespace, apiVersion string, selector labels.Selector, ex
 	req := m.RESTClient.Get().
 		NamespaceIfScoped(namespace, m.NamespaceScoped).
 		Resource(m.Resource).
-		LabelsSelectorParam(selector)
+		VersionedParams(&metav1.ListOptions{
+			LabelSelector: selector.String(),
+		}, metav1.ParameterCodec)
 	if export {
+		// TODO: I should be part of ListOptions
 		req.Param("export", strconv.FormatBool(export))
 	}
 	return req.Do().Get()
@@ -79,9 +83,11 @@ func (m *Helper) Watch(namespace, resourceVersion, apiVersion string, labelSelec
 	return m.RESTClient.Get().
 		NamespaceIfScoped(namespace, m.NamespaceScoped).
 		Resource(m.Resource).
-		Param("resourceVersion", resourceVersion).
+		VersionedParams(&metav1.ListOptions{
-		Param("watch", "true").
+			ResourceVersion: resourceVersion,
-		LabelsSelectorParam(labelSelector).
+			Watch:           true,
+			LabelSelector:   labelSelector.String(),
+		}, metav1.ParameterCodec).
 		Watch()
 }
 
@@ -89,9 +95,11 @@ func (m *Helper) WatchSingle(namespace, name, resourceVersion string) (watch.Int
 	return m.RESTClient.Get().
 		NamespaceIfScoped(namespace, m.NamespaceScoped).
 		Resource(m.Resource).
-		Param("resourceVersion", resourceVersion).
+		VersionedParams(&metav1.ListOptions{
-		Param("watch", "true").
+			ResourceVersion: resourceVersion,
-		FieldsSelectorParam(fields.OneTermEqualSelector("metadata.name", name)).
+			Watch:           true,
+			FieldSelector:   fields.OneTermEqualSelector("metadata.name", name).String(),
+		}, metav1.ParameterCodec).
 		Watch()
 }
 

staging/src/k8s.io/client-go/rest/BUILD

@@ -28,7 +28,6 @@ go_test(
         "//vendor/k8s.io/apimachinery/pkg/api/equality:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
-        "//vendor/k8s.io/apimachinery/pkg/labels:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/serializer:go_default_library",
@@ -65,8 +64,6 @@ go_library(
         "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
-        "//vendor/k8s.io/apimachinery/pkg/fields:go_default_library",
-        "//vendor/k8s.io/apimachinery/pkg/labels:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/serializer/streaming:go_default_library",

staging/src/k8s.io/client-go/rest/request.go

@@ -33,16 +33,12 @@ import (
 	"time"
 
 	"github.com/golang/glog"
-	"k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/fields"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/runtime/serializer/streaming"
 	"k8s.io/apimachinery/pkg/util/net"
-	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/watch"
 	restclientwatch "k8s.io/client-go/rest/watch"
 	"k8s.io/client-go/tools/metrics"
@@ -50,10 +46,6 @@ import (
 )
 
 var (
-	// specialParams lists parameters that are handled specially and which users of Request
-	// are therefore not allowed to set manually.
-	specialParams = sets.NewString("timeout")
-
 	// longThrottleLatency defines threshold for logging requests. All requests being
 	// throttle for more than longThrottleLatency will be logged.
 	longThrottleLatency = 50 * time.Millisecond
@@ -269,7 +261,7 @@ func (r *Request) AbsPath(segments ...string) *Request {
 }
 
 // RequestURI overwrites existing path and parameters with the value of the provided server relative
-// URI. Some parameters (those in specialParameters) cannot be overwritten.
+// URI.
 func (r *Request) RequestURI(uri string) *Request {
 	if r.err != nil {
 		return r
@@ -291,143 +283,6 @@ func (r *Request) RequestURI(uri string) *Request {
 	return r
 }
 
-const (
-	// A constant that clients can use to refer in a field selector to the object name field.
-	// Will be automatically emitted as the correct name for the API version.
-	nodeUnschedulable = "spec.unschedulable"
-	objectNameField   = "metadata.name"
-	podHost           = "spec.nodeName"
-	podStatus         = "status.phase"
-	secretType        = "type"
-
-	eventReason                  = "reason"
-	eventSource                  = "source"
-	eventType                    = "type"
-	eventInvolvedKind            = "involvedObject.kind"
-	eventInvolvedNamespace       = "involvedObject.namespace"
-	eventInvolvedName            = "involvedObject.name"
-	eventInvolvedUID             = "involvedObject.uid"
-	eventInvolvedAPIVersion      = "involvedObject.apiVersion"
-	eventInvolvedResourceVersion = "involvedObject.resourceVersion"
-	eventInvolvedFieldPath       = "involvedObject.fieldPath"
-)
-
-type clientFieldNameToAPIVersionFieldName map[string]string
-
-func (c clientFieldNameToAPIVersionFieldName) filterField(field, value string) (newField, newValue string, err error) {
-	newFieldName, ok := c[field]
-	if !ok {
-		return "", "", fmt.Errorf("%v - %v - no field mapping defined", field, value)
-	}
-	return newFieldName, value, nil
-}
-
-type resourceTypeToFieldMapping map[string]clientFieldNameToAPIVersionFieldName
-
-func (r resourceTypeToFieldMapping) filterField(resourceType, field, value string) (newField, newValue string, err error) {
-	fMapping, ok := r[resourceType]
-	if !ok {
-		return "", "", fmt.Errorf("%v - %v - %v - no field mapping defined", resourceType, field, value)
-	}
-	return fMapping.filterField(field, value)
-}
-
-type versionToResourceToFieldMapping map[schema.GroupVersion]resourceTypeToFieldMapping
-
-// filterField transforms the given field/value selector for the given groupVersion and resource
-func (v versionToResourceToFieldMapping) filterField(groupVersion *schema.GroupVersion, resourceType, field, value string) (newField, newValue string, err error) {
-	rMapping, ok := v[*groupVersion]
-	if !ok {
-		// no groupVersion overrides registered, default to identity mapping
-		return field, value, nil
-	}
-	newField, newValue, err = rMapping.filterField(resourceType, field, value)
-	if err != nil {
-		// no groupVersionResource overrides registered, default to identity mapping
-		return field, value, nil
-	}
-	return newField, newValue, nil
-}
-
-var fieldMappings = versionToResourceToFieldMapping{
-	v1.SchemeGroupVersion: resourceTypeToFieldMapping{
-		"nodes": clientFieldNameToAPIVersionFieldName{
-			objectNameField:   objectNameField,
-			nodeUnschedulable: nodeUnschedulable,
-		},
-		"pods": clientFieldNameToAPIVersionFieldName{
-			objectNameField: objectNameField,
-			podHost:         podHost,
-			podStatus:       podStatus,
-		},
-		"secrets": clientFieldNameToAPIVersionFieldName{
-			secretType: secretType,
-		},
-		"serviceAccounts": clientFieldNameToAPIVersionFieldName{
-			objectNameField: objectNameField,
-		},
-		"endpoints": clientFieldNameToAPIVersionFieldName{
-			objectNameField: objectNameField,
-		},
-		"events": clientFieldNameToAPIVersionFieldName{
-			objectNameField:              objectNameField,
-			eventReason:                  eventReason,
-			eventSource:                  eventSource,
-			eventType:                    eventType,
-			eventInvolvedKind:            eventInvolvedKind,
-			eventInvolvedNamespace:       eventInvolvedNamespace,
-			eventInvolvedName:            eventInvolvedName,
-			eventInvolvedUID:             eventInvolvedUID,
-			eventInvolvedAPIVersion:      eventInvolvedAPIVersion,
-			eventInvolvedResourceVersion: eventInvolvedResourceVersion,
-			eventInvolvedFieldPath:       eventInvolvedFieldPath,
-		},
-	},
-}
-
-// FieldsSelectorParam adds the given selector as a query parameter with the name paramName.
-func (r *Request) FieldsSelectorParam(s fields.Selector) *Request {
-	if r.err != nil {
-		return r
-	}
-	if s == nil {
-		return r
-	}
-	if s.Empty() {
-		return r
-	}
-	s2, err := s.Transform(func(field, value string) (newField, newValue string, err error) {
-		return fieldMappings.filterField(r.content.GroupVersion, r.resource, field, value)
-	})
-	if err != nil {
-		r.err = err
-		return r
-	}
-	return r.setParam(metav1.FieldSelectorQueryParam(r.content.GroupVersion.String()), s2.String())
-}
-
-// LabelsSelectorParam adds the given selector as a query parameter
-func (r *Request) LabelsSelectorParam(s labels.Selector) *Request {
-	if r.err != nil {
-		return r
-	}
-	if s == nil {
-		return r
-	}
-	if s.Empty() {
-		return r
-	}
-	return r.setParam(metav1.LabelSelectorQueryParam(r.content.GroupVersion.String()), s.String())
-}
-
-// UintParam creates a query parameter with the given value.
-func (r *Request) UintParam(paramName string, u uint64) *Request {
-	if r.err != nil {
-		return r
-	}
-	return r.setParam(paramName, strconv.FormatUint(u, 10))
-}
-
 // Param creates a query parameter with the given string value.
 func (r *Request) Param(paramName, s string) *Request {
 	if r.err != nil {
@@ -439,6 +294,8 @@ func (r *Request) Param(paramName, s string) *Request {
 // VersionedParams will take the provided object, serialize it to a map[string][]string using the
 // implicit RESTClient API version and the default parameter codec, and then add those as parameters
 // to the request. Use this to provide versioned query parameters from client libraries.
+// VersionedParams will not write query parameters that have omitempty set and are empty. If a
+// parameter has already been set it is appended to (Params and VersionedParams are additive).
 func (r *Request) VersionedParams(obj runtime.Object, codec runtime.ParameterCodec) *Request {
 	if r.err != nil {
 		return r
@@ -449,52 +306,15 @@ func (r *Request) VersionedParams(obj runtime.Object, codec runtime.ParameterCod
 		return r
 	}
 	for k, v := range params {
-		for _, value := range v {
+		if r.params == nil {
-			// TODO: Move it to setParam method, once we get rid of
+			r.params = make(url.Values)
-			// FieldSelectorParam & LabelSelectorParam methods.
-			if k == metav1.LabelSelectorQueryParam(r.content.GroupVersion.String()) && value == "" {
-				// Don't set an empty selector for backward compatibility.
-				// Since there is no way to get the difference between empty
-				// and unspecified string, we don't set it to avoid having
-				// labelSelector= param in every request.
-				continue
-			}
-			if k == metav1.FieldSelectorQueryParam(r.content.GroupVersion.String()) {
-				if len(value) == 0 {
-					// Don't set an empty selector for backward compatibility.
-					// Since there is no way to get the difference between empty
-					// and unspecified string, we don't set it to avoid having
-					// fieldSelector= param in every request.
-					continue
-				}
-				// TODO: Filtering should be handled somewhere else.
-				selector, err := fields.ParseSelector(value)
-				if err != nil {
-					r.err = fmt.Errorf("unparsable field selector: %v", err)
-					return r
-				}
-				filteredSelector, err := selector.Transform(
-					func(field, value string) (newField, newValue string, err error) {
-						return fieldMappings.filterField(r.content.GroupVersion, r.resource, field, value)
-					})
-				if err != nil {
-					r.err = fmt.Errorf("untransformable field selector: %v", err)
-					return r
-				}
-				value = filteredSelector.String()
-			}
-
-			r.setParam(k, value)
 		}
+		r.params[k] = append(r.params[k], v...)
 	}
 	return r
 }
 
 func (r *Request) setParam(paramName, value string) *Request {
-	if specialParams.Has(paramName) {
-		r.err = fmt.Errorf("must set %v through the corresponding function, not directly.", paramName)
-		return r
-	}
 	if r.params == nil {
 		r.params = make(url.Values)
 	}

staging/src/k8s.io/client-go/rest/request_test.go

@@ -38,7 +38,6 @@ import (
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/runtime/serializer"
@@ -100,8 +99,6 @@ func TestRequestWithErrorWontChange(t *testing.T) {
 	}
 	r := original
 	changed := r.Param("foo", "bar").
-		LabelsSelectorParam(labels.Set{"a": "b"}.AsSelector()).
-		UintParam("uint", 1).
 		AbsPath("/abs").
 		Prefix("test").
 		Suffix("testing").
@@ -257,7 +254,7 @@ func TestRequestVersionedParamsFromListOptions(t *testing.T) {
 		"resourceVersion": []string{"1", "2"},
 		"timeoutSeconds":  []string{"10"},
 	}) {
-		t.Errorf("should have set a param: %#v", r)
+		t.Errorf("should have set a param: %#v %v", r.params, r.err)
 	}
 }
 
@@ -1276,7 +1273,6 @@ func TestDoRequestNewWayReader(t *testing.T) {
 		Resource("bar").
 		Name("baz").
 		Prefix("foo").
-		LabelsSelectorParam(labels.Set{"name": "foo"}.AsSelector()).
 		Timeout(time.Second).
 		Body(bytes.NewBuffer(reqBodyExpected)).
 		Do().Get()
@@ -1291,7 +1287,7 @@ func TestDoRequestNewWayReader(t *testing.T) {
 	}
 	tmpStr := string(reqBodyExpected)
 	requestURL := defaultResourcePathWithPrefix("foo", "bar", "", "baz")
-	requestURL += "?" + metav1.LabelSelectorQueryParam(v1.SchemeGroupVersion.String()) + "=name%3Dfoo&timeout=1s"
+	requestURL += "?timeout=1s"
 	fakeHandler.ValidateRequest(t, requestURL, "POST", &tmpStr)
 }
 
@@ -1316,7 +1312,6 @@ func TestDoRequestNewWayObj(t *testing.T) {
 		Suffix("baz").
 		Name("bar").
 		Resource("foo").
-		LabelsSelectorParam(labels.Set{"name": "foo"}.AsSelector()).
 		Timeout(time.Second).
 		Body(reqObj).
 		Do().Get()
@@ -1331,7 +1326,7 @@ func TestDoRequestNewWayObj(t *testing.T) {
 	}
 	tmpStr := string(reqBodyExpected)
 	requestURL := defaultResourcePathWithPrefix("", "foo", "", "bar/baz")
-	requestURL += "?" + metav1.LabelSelectorQueryParam(v1.SchemeGroupVersion.String()) + "=name%3Dfoo&timeout=1s"
+	requestURL += "?timeout=1s"
 	fakeHandler.ValidateRequest(t, requestURL, "POST", &tmpStr)
 }
 
@@ -1489,33 +1484,14 @@ func TestAbsPath(t *testing.T) {
 	}
 }
 
-func TestUintParam(t *testing.T) {
-	table := []struct {
-		name      string
-		testVal   uint64
-		expectStr string
-	}{
-		{"foo", 31415, "http://localhost?foo=31415"},
-		{"bar", 42, "http://localhost?bar=42"},
-		{"baz", 0, "http://localhost?baz=0"},
-	}
-
-	for _, item := range table {
-		u, _ := url.Parse("http://localhost")
-		r := NewRequest(nil, "GET", u, "", ContentConfig{GroupVersion: &schema.GroupVersion{Group: "test"}}, Serializers{}, nil, nil).AbsPath("").UintParam(item.name, item.testVal)
-		if e, a := item.expectStr, r.URL().String(); e != a {
-			t.Errorf("expected %v, got %v", e, a)
-		}
-	}
-}
-
 func TestUnacceptableParamNames(t *testing.T) {
 	table := []struct {
 		name          string
 		testVal       string
 		expectSuccess bool
 	}{
-		{"timeout", "42", false},
+		// timeout is no longer "protected"
+		{"timeout", "42", true},
 	}
 
 	for _, item := range table {

staging/src/k8s.io/client-go/tools/cache/listwatch.go

@@ -58,21 +58,21 @@ type Getter interface {
 // NewListWatchFromClient creates a new ListWatch from the specified client, resource, namespace and field selector.
 func NewListWatchFromClient(c Getter, resource string, namespace string, fieldSelector fields.Selector) *ListWatch {
 	listFunc := func(options metav1.ListOptions) (runtime.Object, error) {
+		options.FieldSelector = fieldSelector.String()
 		return c.Get().
 			Namespace(namespace).
 			Resource(resource).
 			VersionedParams(&options, metav1.ParameterCodec).
-			FieldsSelectorParam(fieldSelector).
 			Do().
 			Get()
 	}
 	watchFunc := func(options metav1.ListOptions) (watch.Interface, error) {
 		options.Watch = true
+		options.FieldSelector = fieldSelector.String()
 		return c.Get().
 			Namespace(namespace).
 			Resource(resource).
 			VersionedParams(&options, metav1.ParameterCodec).
-			FieldsSelectorParam(fieldSelector).
 			Watch()
 	}
 	return &ListWatch{ListFunc: listFunc, WatchFunc: watchFunc}

staging/src/k8s.io/metrics/pkg/client/custom_metrics/BUILD

@@ -16,6 +16,7 @@ go_library(
     tags = ["automanaged"],
     deps = [
         "//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/labels:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/serializer:go_default_library",

staging/src/k8s.io/metrics/pkg/client/custom_metrics/client.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 
 	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	serializer "k8s.io/apimachinery/pkg/runtime/serializer"
@@ -180,7 +181,9 @@ func (m *rootScopedMetrics) GetForObjects(groupKind schema.GroupKind, selector l
 		Resource(resourceName).
 		Name(v1alpha1.AllObjects).
 		SubResource(metricName).
-		LabelsSelectorParam(selector).
+		VersionedParams(&metav1.ListOptions{
+			LabelSelector: selector.String(),
+		}, metav1.ParameterCodec).
 		Do().
 		Into(res)
 
@@ -234,7 +237,9 @@ func (m *namespacedMetrics) GetForObjects(groupKind schema.GroupKind, selector l
 		Namespace(m.namespace).
 		Name(v1alpha1.AllObjects).
 		SubResource(metricName).
-		LabelsSelectorParam(selector).
+		VersionedParams(&metav1.ListOptions{
+			LabelSelector: selector.String(),
+		}, metav1.ParameterCodec).
 		Do().
 		Into(res)
 

test/integration/client/client_test.go

@@ -523,9 +523,11 @@ func TestSingleWatch(t *testing.T) {
 	w, err := client.Core().RESTClient().Get().
 		Namespace(ns.Name).
 		Resource("events").
-		Param("resourceVersion", rv1).
+		VersionedParams(&metav1.ListOptions{
-		Param("watch", "true").
+			ResourceVersion: rv1,
-		FieldsSelectorParam(fields.OneTermEqualSelector("metadata.name", "event-9")).
+			Watch:           true,
+			FieldSelector:   fields.OneTermEqualSelector("metadata.name", "event-9").String(),
+		}, metav1.ParameterCodec).
 		Watch()
 
 	if err != nil {