Merge pull request #16327 from deads2k/api-07-fieldselector

Automatic merge from submit-queue (batch tested with PRs 16384, 16327, 16199, 16286, 16378)

fix remaining field selectors 

Really hoping that #16305 works out.  This updates all the rest for that pattern and allows us to remove a ton of boilerplate while maintaining decent unit test coverage.  Still need a "you forgot to add a test" test, but that was already missing.

hack/import-restrictions.json

@@ -173,8 +173,7 @@
     ],
     "allowedImportPackages": [
       "vendor/k8s.io/kubernetes/pkg/api",
-      "vendor/k8s.io/kubernetes/pkg/api/v1",
-      "vendor/k8s.io/kubernetes/pkg/registry/core/namespace"
+      "vendor/k8s.io/kubernetes/pkg/api/v1"
     ]
   },
 

pkg/api/apihelpers/apitesting/fields.go

@@ -8,21 +8,6 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
-func TestFieldLabelConversions(t *testing.T, scheme *runtime.Scheme, version, kind string, expectedLabels map[string]string, customLabels ...string) {
-	for label := range expectedLabels {
-		_, _, err := scheme.ConvertFieldLabel(version, kind, label, "")
-		if err != nil {
-			t.Errorf("No conversion registered for %s for %s %s", label, version, kind)
-		}
-	}
-	for _, label := range customLabels {
-		_, _, err := scheme.ConvertFieldLabel(version, kind, label, "")
-		if err != nil {
-			t.Errorf("No conversion registered for %s for %s %s", label, version, kind)
-		}
-	}
-}
-
 // FieldKeyCheck gathers information to check if the field key conversions are working correctly.  It takes many parameters
 // in an attempt to reflect reality
 type FieldKeyCheck struct {
@@ -46,6 +31,10 @@ func (f FieldKeyCheck) Check(t *testing.T) {
 			t.Errorf("illegal field conversion %q for %v", externalFieldKey, f.Kind)
 			continue
 		}
+		// we get this by default
+		if internalFieldKey == "metadata.name" {
+			continue
+		}
 
 		fieldSet := fields.Set{}
 		if err := f.FieldKeyEvaluatorFn(internalObj, fieldSet); err != nil {

pkg/api/apihelpers/fields.go

@@ -1,8 +1,6 @@
 package apihelpers
 
 import (
-	"fmt"
-
 	"k8s.io/apimachinery/pkg/runtime"
 )
 
@@ -16,19 +14,3 @@ func LegacyMetaV1FieldSelectorConversionWithName(label, value string) (string, s
 		return runtime.DefaultMetaV1FieldSelectorConversion(label, value)
 	}
 }
-
-// GetFieldLabelConversionFunc returns a field label conversion func, which does the following:
-// * returns overrideLabels[label], value, nil if the specified label exists in the overrideLabels map
-// * returns label, value, nil if the specified label exists as a key in the supportedLabels map (values in this map are unused, it is intended to be a prototypical label/value map)
-// * otherwise, returns an error
-func GetFieldLabelConversionFunc(supportedLabels map[string]string, overrideLabels map[string]string) func(label, value string) (string, string, error) {
-	return func(label, value string) (string, string, error) {
-		if label, overridden := overrideLabels[label]; overridden {
-			return label, value, nil
-		}
-		if _, supported := supportedLabels[label]; supported {
-			return label, value, nil
-		}
-		return "", "", fmt.Errorf("field label not supported: %s", label)
-	}
-}

pkg/authorization/apis/authorization/fields.go

@@ -1,13 +1,17 @@
 package authorization
 
-import "k8s.io/apimachinery/pkg/fields"
+import (
+	"fmt"
 
-// PolicyBindingToSelectableFields returns a label set that represents the object
-// changes to the returned keys require registering conversions for existing versions using Scheme.AddFieldLabelConversionFunc
-func PolicyBindingToSelectableFields(policyBinding *PolicyBinding) fields.Set {
-	return fields.Set{
-		"metadata.name":       policyBinding.Name,
-		"metadata.namespace":  policyBinding.Namespace,
-		"policyRef.namespace": policyBinding.PolicyRef.Namespace,
+	"k8s.io/apimachinery/pkg/fields"
+	runtime "k8s.io/apimachinery/pkg/runtime"
+)
+
+func PolicyBindingFieldSelector(obj runtime.Object, fieldSet fields.Set) error {
+	policyBinding, ok := obj.(*PolicyBinding)
+	if !ok {
+		return fmt.Errorf("%T not a PolicyBinding", obj)
 	}
+	fieldSet["policyRef.namespace"] = policyBinding.PolicyRef.Namespace
+	return nil
 }

pkg/authorization/apis/authorization/v1/conversion.go

@@ -394,15 +394,32 @@ func addConversionFuncs(scheme *runtime.Scheme) error {
 		return err
 	}
 
-	if err := scheme.AddFieldLabelConversionFunc("v1", "PolicyBinding",
-		apihelpers.GetFieldLabelConversionFunc(newer.PolicyBindingToSelectableFields(&newer.PolicyBinding{}), nil),
-	); err != nil {
+	return nil
+}
+
+func addLegacyFieldSelectorKeyConversions(scheme *runtime.Scheme) error {
+	if err := scheme.AddFieldLabelConversionFunc(LegacySchemeGroupVersion.String(), "PolicyBinding", legacyPolicyBindingFieldSelectorKeyConversionFunc); err != nil {
 		return err
 	}
+	return nil
+}
 
+func addFieldSelectorKeyConversions(scheme *runtime.Scheme) error {
 	return nil
 }
 
+// because field selectors can vary in support by version they are exposed under, we have one function for each
+// groupVersion we're registering for
+
+func legacyPolicyBindingFieldSelectorKeyConversionFunc(label, value string) (internalLabel, internalValue string, err error) {
+	switch label {
+	case "policyRef.namespace":
+		return label, value, nil
+	default:
+		return runtime.DefaultMetaV1FieldSelectorConversion(label, value)
+	}
+}
+
 var _ runtime.NestedObjectDecoder = &PolicyRule{}
 var _ runtime.NestedObjectEncoder = &PolicyRule{}
 

pkg/authorization/apis/authorization/v1/conversion_test.go

@@ -10,12 +10,11 @@ import (
 )
 
 func TestFieldSelectorConversions(t *testing.T) {
-	converter := runtime.NewScheme()
-	LegacySchemeBuilder.AddToScheme(converter)
-
-	apitesting.TestFieldLabelConversions(t, converter, "v1", "PolicyBinding",
-		// Ensure all currently returned labels are supported
-		authorizationapi.PolicyBindingToSelectableFields(&authorizationapi.PolicyBinding{}),
-	)
-
+	apitesting.FieldKeyCheck{
+		SchemeBuilder: []func(*runtime.Scheme) error{LegacySchemeBuilder.AddToScheme, authorizationapi.LegacySchemeBuilder.AddToScheme},
+		Kind:          LegacySchemeGroupVersion.WithKind("PolicyBinding"),
+		// Ensure previously supported labels have conversions. DO NOT REMOVE THINGS FROM THIS LIST
+		AllowedExternalFieldKeys: []string{"policyRef.namespace"},
+		FieldKeyEvaluatorFn:      authorizationapi.PolicyBindingFieldSelector,
+	}.Check(t)
 }

pkg/authorization/apis/authorization/v1/register.go

@@ -15,10 +15,10 @@ var (
 	SchemeGroupVersion       = schema.GroupVersion{Group: GroupName, Version: "v1"}
 	LegacySchemeGroupVersion = schema.GroupVersion{Group: LegacyGroupName, Version: "v1"}
 
-	LegacySchemeBuilder    = runtime.NewSchemeBuilder(addLegacyKnownTypes, addConversionFuncs, RegisterDefaults)
+	LegacySchemeBuilder    = runtime.NewSchemeBuilder(addLegacyKnownTypes, addConversionFuncs, addLegacyFieldSelectorKeyConversions, RegisterDefaults)
 	AddToSchemeInCoreGroup = LegacySchemeBuilder.AddToScheme
 
-	SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes, addConversionFuncs, RegisterDefaults)
+	SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes, addConversionFuncs, addFieldSelectorKeyConversions, RegisterDefaults)
 	AddToScheme   = SchemeBuilder.AddToScheme
 )
 

pkg/build/apis/build/fields.go

@@ -1,14 +1,19 @@
 package build
 
-import "k8s.io/apimachinery/pkg/fields"
-
-// BuildToSelectableFields returns a label set that represents the object
-// changes to the returned keys require registering conversions for existing versions using Scheme.AddFieldLabelConversionFunc
-func BuildToSelectableFields(build *Build) fields.Set {
-	return fields.Set{
-		"metadata.name":      build.Name,
-		"metadata.namespace": build.Namespace,
-		"status":             string(build.Status.Phase),
-		"podName":            GetBuildPodName(build),
+import (
+	"fmt"
+
+	"k8s.io/apimachinery/pkg/fields"
+	runtime "k8s.io/apimachinery/pkg/runtime"
+)
+
+func BuildFieldSelector(obj runtime.Object, fieldSet fields.Set) error {
+	build, ok := obj.(*Build)
+	if !ok {
+		return fmt.Errorf("%T not a Build", obj)
 	}
+	fieldSet["status"] = string(build.Status.Phase)
+	fieldSet["podName"] = GetBuildPodName(build)
+
+	return nil
 }

pkg/build/apis/build/v1/conversion.go

@@ -174,24 +174,45 @@ func addConversionFuncs(scheme *runtime.Scheme) error {
 		return err
 	}
 
-	if err := scheme.AddFieldLabelConversionFunc(SchemeGroupVersion.String(), "Build",
-		apihelpers.GetFieldLabelConversionFunc(newer.BuildToSelectableFields(&newer.Build{}), nil),
-	); err != nil {
-		return err
-	}
-
 	return nil
 }
 
-func addLegacyFieldLabelConversions(scheme *runtime.Scheme) error {
-	if err := scheme.AddFieldLabelConversionFunc("v1", "Build",
-		apihelpers.GetFieldLabelConversionFunc(newer.BuildToSelectableFields(&newer.Build{}), map[string]string{"name": "metadata.name"}),
-	); err != nil {
+func addLegacyFieldSelectorKeyConversions(scheme *runtime.Scheme) error {
+	if err := scheme.AddFieldLabelConversionFunc(LegacySchemeGroupVersion.String(), "Build", legacyBuildFieldSelectorKeyConversionFunc); err != nil {
 		return err
 	}
+	if err := scheme.AddFieldLabelConversionFunc(LegacySchemeGroupVersion.String(), "BuildConfig", apihelpers.LegacyMetaV1FieldSelectorConversionWithName); err != nil {
+		return err
+	}
+	return nil
+}
 
-	if err := scheme.AddFieldLabelConversionFunc("v1", "BuildConfig", apihelpers.LegacyMetaV1FieldSelectorConversionWithName); err != nil {
+func addFieldSelectorKeyConversions(scheme *runtime.Scheme) error {
+	if err := scheme.AddFieldLabelConversionFunc(SchemeGroupVersion.String(), "Build", buildFieldSelectorKeyConversionFunc); err != nil {
 		return err
 	}
 	return nil
 }
+
+// because field selectors can vary in support by version they are exposed under, we have one function for each
+// groupVersion we're registering for
+
+func legacyBuildFieldSelectorKeyConversionFunc(label, value string) (internalLabel, internalValue string, err error) {
+	switch label {
+	case "status",
+		"podName":
+		return label, value, nil
+	default:
+		return apihelpers.LegacyMetaV1FieldSelectorConversionWithName(label, value)
+	}
+}
+
+func buildFieldSelectorKeyConversionFunc(label, value string) (internalLabel, internalValue string, err error) {
+	switch label {
+	case "status",
+		"podName":
+		return label, value, nil
+	default:
+		return runtime.DefaultMetaV1FieldSelectorConversion(label, value)
+	}
+}

pkg/build/apis/build/v1/conversion_test.go

@@ -16,15 +16,29 @@ import (
 var Convert = knewer.Scheme.Convert
 
 func TestFieldSelectorConversions(t *testing.T) {
-	converter := runtime.NewScheme()
-	LegacySchemeBuilder.AddToScheme(converter)
+	apitesting.FieldKeyCheck{
+		SchemeBuilder: []func(*runtime.Scheme) error{LegacySchemeBuilder.AddToScheme, newer.LegacySchemeBuilder.AddToScheme},
+		Kind:          LegacySchemeGroupVersion.WithKind("Build"),
+		// Ensure previously supported labels have conversions. DO NOT REMOVE THINGS FROM THIS LIST
+		AllowedExternalFieldKeys: []string{"name", "status", "podName"},
+		FieldKeyEvaluatorFn:      newer.BuildFieldSelector,
+	}.Check(t)
+
+	apitesting.FieldKeyCheck{
+		SchemeBuilder: []func(*runtime.Scheme) error{LegacySchemeBuilder.AddToScheme, newer.LegacySchemeBuilder.AddToScheme},
+		Kind:          LegacySchemeGroupVersion.WithKind("BuildConfig"),
+		// Ensure previously supported labels have conversions. DO NOT REMOVE THINGS FROM THIS LIST
+		AllowedExternalFieldKeys: []string{"name"},
+	}.Check(t)
 
-	apitesting.TestFieldLabelConversions(t, converter, "v1", "Build",
-		// Ensure all currently returned labels are supported
-		newer.BuildToSelectableFields(&newer.Build{}),
+	apitesting.FieldKeyCheck{
+		SchemeBuilder: []func(*runtime.Scheme) error{SchemeBuilder.AddToScheme, newer.SchemeBuilder.AddToScheme},
+		Kind:          SchemeGroupVersion.WithKind("Build"),
 		// Ensure previously supported labels have conversions. DO NOT REMOVE THINGS FROM THIS LIST
-		"name", "status", "podName",
-	)
+		AllowedExternalFieldKeys: []string{"status", "podName"},
+		FieldKeyEvaluatorFn:      newer.BuildFieldSelector,
+	}.Check(t)
+
 }
 
 func TestBinaryBuildRequestOptions(t *testing.T) {

pkg/build/apis/build/v1/register.go

@@ -13,10 +13,10 @@ var (
 	SchemeGroupVersion       = schema.GroupVersion{Group: GroupName, Version: "v1"}
 	LegacySchemeGroupVersion = schema.GroupVersion{Group: LegacyGroupName, Version: "v1"}
 
-	LegacySchemeBuilder    = runtime.NewSchemeBuilder(addLegacyKnownTypes, addConversionFuncs, RegisterDefaults, addLegacyFieldLabelConversions)
+	LegacySchemeBuilder    = runtime.NewSchemeBuilder(addLegacyKnownTypes, addConversionFuncs, addLegacyFieldSelectorKeyConversions, RegisterDefaults)
 	AddToSchemeInCoreGroup = LegacySchemeBuilder.AddToScheme
 
-	SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes, addConversionFuncs, RegisterDefaults)
+	SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes, addConversionFuncs, addFieldSelectorKeyConversions, RegisterDefaults)
 	AddToScheme   = SchemeBuilder.AddToScheme
 )
 

pkg/build/registry/build/etcd/etcd.go

@@ -6,6 +6,7 @@ import (
 	"k8s.io/apiserver/pkg/registry/generic"
 	"k8s.io/apiserver/pkg/registry/generic/registry"
 	"k8s.io/apiserver/pkg/registry/rest"
+	"k8s.io/apiserver/pkg/storage"
 	kapi "k8s.io/kubernetes/pkg/api"
 
 	buildapi "github.com/openshift/origin/pkg/build/apis/build"
@@ -25,15 +26,17 @@ func NewREST(optsGetter restoptions.Getter) (*REST, *DetailsREST, error) {
 		Copier:                   kapi.Scheme,
 		NewFunc:                  func() runtime.Object { return &buildapi.Build{} },
 		NewListFunc:              func() runtime.Object { return &buildapi.BuildList{} },
-		PredicateFunc:            build.Matcher,
 		DefaultQualifiedResource: buildapi.Resource("builds"),
 
 		CreateStrategy: build.Strategy,
 		UpdateStrategy: build.Strategy,
 		DeleteStrategy: build.Strategy,
 	}
 
-	options := &generic.StoreOptions{RESTOptions: optsGetter, AttrFunc: build.GetAttrs}
+	options := &generic.StoreOptions{
+		RESTOptions: optsGetter,
+		AttrFunc:    storage.AttrFunc(storage.DefaultNamespaceScopedAttr).WithFieldMutation(buildapi.BuildFieldSelector),
+	}
 	if err := store.CompleteWithOptions(options); err != nil {
 		return nil, nil, err
 	}

pkg/build/registry/build/strategy.go

@@ -1,16 +1,12 @@
 package build
 
 import (
-	"fmt"
 	"reflect"
 
 	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/util/validation/field"
 	apirequest "k8s.io/apiserver/pkg/endpoints/request"
-	kstorage "k8s.io/apiserver/pkg/storage"
 	"k8s.io/apiserver/pkg/storage/names"
 	kapi "k8s.io/kubernetes/pkg/api"
 
@@ -82,24 +78,6 @@ func (strategy) CheckGracefulDelete(obj runtime.Object, options *metav1.DeleteOp
 	return false
 }
 
-// GetAttrs returns labels and fields of a given object for filtering purposes
-func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, bool, error) {
-	build, ok := obj.(*buildapi.Build)
-	if !ok {
-		return nil, nil, false, fmt.Errorf("not a Build")
-	}
-	return labels.Set(build.ObjectMeta.Labels), buildapi.BuildToSelectableFields(build), build.Initializers != nil, nil
-}
-
-// Matcher returns a generic matcher for a given label and field selector.
-func Matcher(label labels.Selector, field fields.Selector) kstorage.SelectionPredicate {
-	return kstorage.SelectionPredicate{
-		Label:    label,
-		Field:    field,
-		GetAttrs: GetAttrs,
-	}
-}
-
 type detailsStrategy struct {
 	strategy
 }