From b6d9815b95940002545b4aa7affb892d94637a49 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Sun, 16 Jul 2017 00:22:11 -0400 Subject: [PATCH 1/3] Remove use of (Label|Field)SelectorParam --- pkg/kubectl/resource/helper.go | 22 +++++++++++++------ .../k8s.io/client-go/tools/cache/listwatch.go | 4 ++-- .../metrics/pkg/client/custom_metrics/BUILD | 1 + .../pkg/client/custom_metrics/client.go | 9 ++++++-- test/integration/client/client_test.go | 8 ++++--- 5 files changed, 30 insertions(+), 14 deletions(-) diff --git a/pkg/kubectl/resource/helper.go b/pkg/kubectl/resource/helper.go index 2cbdd847471..e6452244b67 100644 --- a/pkg/kubectl/resource/helper.go +++ b/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). - Param("watch", "true"). - LabelsSelectorParam(labelSelector). + VersionedParams(&metav1.ListOptions{ + ResourceVersion: resourceVersion, + 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). - Param("watch", "true"). - FieldsSelectorParam(fields.OneTermEqualSelector("metadata.name", name)). + VersionedParams(&metav1.ListOptions{ + ResourceVersion: resourceVersion, + Watch: true, + FieldSelector: fields.OneTermEqualSelector("metadata.name", name).String(), + }, metav1.ParameterCodec). Watch() } diff --git a/staging/src/k8s.io/client-go/tools/cache/listwatch.go b/staging/src/k8s.io/client-go/tools/cache/listwatch.go index af01d474579..4c976533b01 100644 --- a/staging/src/k8s.io/client-go/tools/cache/listwatch.go +++ b/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} diff --git a/staging/src/k8s.io/metrics/pkg/client/custom_metrics/BUILD b/staging/src/k8s.io/metrics/pkg/client/custom_metrics/BUILD index 0ce38e03934..1e1c1ebe852 100644 --- a/staging/src/k8s.io/metrics/pkg/client/custom_metrics/BUILD +++ b/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", diff --git a/staging/src/k8s.io/metrics/pkg/client/custom_metrics/client.go b/staging/src/k8s.io/metrics/pkg/client/custom_metrics/client.go index 12e280c3cea..807b1f81c65 100644 --- a/staging/src/k8s.io/metrics/pkg/client/custom_metrics/client.go +++ b/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) diff --git a/test/integration/client/client_test.go b/test/integration/client/client_test.go index 5769ab32fcc..8d498e166b0 100644 --- a/test/integration/client/client_test.go +++ b/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). - Param("watch", "true"). - FieldsSelectorParam(fields.OneTermEqualSelector("metadata.name", "event-9")). + VersionedParams(&metav1.ListOptions{ + ResourceVersion: rv1, + Watch: true, + FieldSelector: fields.OneTermEqualSelector("metadata.name", "event-9").String(), + }, metav1.ParameterCodec). Watch() if err != nil { From f0e11c5b09abe46dd67a4f5e6f8bce9696888cee Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Sun, 16 Jul 2017 00:24:11 -0400 Subject: [PATCH 2/3] Remove "special" restclient parameters --- staging/src/k8s.io/client-go/rest/request.go | 10 +--------- staging/src/k8s.io/client-go/rest/request_test.go | 3 ++- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/staging/src/k8s.io/client-go/rest/request.go b/staging/src/k8s.io/client-go/rest/request.go index 13899ab2333..d8e5dcc607d 100644 --- a/staging/src/k8s.io/client-go/rest/request.go +++ b/staging/src/k8s.io/client-go/rest/request.go @@ -50,10 +50,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 +265,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 @@ -491,10 +487,6 @@ func (r *Request) VersionedParams(obj runtime.Object, codec runtime.ParameterCod } 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) } diff --git a/staging/src/k8s.io/client-go/rest/request_test.go b/staging/src/k8s.io/client-go/rest/request_test.go index 6242afb3f5b..5863a970069 100755 --- a/staging/src/k8s.io/client-go/rest/request_test.go +++ b/staging/src/k8s.io/client-go/rest/request_test.go @@ -1515,7 +1515,8 @@ func TestUnacceptableParamNames(t *testing.T) { testVal string expectSuccess bool }{ - {"timeout", "42", false}, + // timeout is no longer "protected" + {"timeout", "42", true}, } for _, item := range table { From 112e0fa9da069d147fffe5bd0638ebce4a9bba42 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Sun, 16 Jul 2017 00:40:24 -0400 Subject: [PATCH 3/3] Remove Kube specific api constructs from restclient All callers must use VersionedParameters, which no longer has special behavior for Kube resources. --- staging/src/k8s.io/client-go/rest/BUILD | 3 - staging/src/k8s.io/client-go/rest/request.go | 182 +----------------- .../src/k8s.io/client-go/rest/request_test.go | 31 +-- 3 files changed, 8 insertions(+), 208 deletions(-) diff --git a/staging/src/k8s.io/client-go/rest/BUILD b/staging/src/k8s.io/client-go/rest/BUILD index ac39ce797ea..1e761402bb4 100644 --- a/staging/src/k8s.io/client-go/rest/BUILD +++ b/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", diff --git a/staging/src/k8s.io/client-go/rest/request.go b/staging/src/k8s.io/client-go/rest/request.go index d8e5dcc607d..2d676cec06f 100644 --- a/staging/src/k8s.io/client-go/rest/request.go +++ b/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" @@ -287,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 { @@ -435,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 @@ -445,43 +306,10 @@ func (r *Request) VersionedParams(obj runtime.Object, codec runtime.ParameterCod return r } for k, v := range params { - for _, value := range v { - // TODO: Move it to setParam method, once we get rid of - // 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) + if r.params == nil { + r.params = make(url.Values) } + r.params[k] = append(r.params[k], v...) } return r } diff --git a/staging/src/k8s.io/client-go/rest/request_test.go b/staging/src/k8s.io/client-go/rest/request_test.go index 5863a970069..fa016dfe1ce 100755 --- a/staging/src/k8s.io/client-go/rest/request_test.go +++ b/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,26 +1484,6 @@ 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