From 02e02170af2b718cee3914e51900ec6c53feec78 Mon Sep 17 00:00:00 2001 From: David Eads Date: Fri, 8 Sep 2017 07:58:42 -0400 Subject: [PATCH] update route field selectors --- hack/import-restrictions.json | 51 +++++++++------------- pkg/api/apihelpers/apitesting/fields.go | 51 ++++++++++++++++++++++ pkg/route/apis/route/fields.go | 23 ++++++---- pkg/route/apis/route/v1/conversion.go | 44 ++++++++++++++++--- pkg/route/apis/route/v1/conversion_test.go | 22 ++++++---- pkg/route/apis/route/v1/defaults_test.go | 43 ++++++++++-------- pkg/route/apis/route/v1/register.go | 4 +- pkg/route/registry/route/etcd/etcd.go | 7 ++- pkg/route/registry/route/strategy.go | 21 --------- 9 files changed, 168 insertions(+), 98 deletions(-) diff --git a/hack/import-restrictions.json b/hack/import-restrictions.json index 8c81a24c3cab..c94580d218c5 100644 --- a/hack/import-restrictions.json +++ b/hack/import-restrictions.json @@ -60,7 +60,8 @@ ], "allowedImportPackageRoots": [ "vendor/k8s.io/apimachinery", - "vendor/github.com/gogo/protobuf" + "vendor/github.com/gogo/protobuf", + "github.com/openshift/origin/pkg/api/apihelpers" ], "allowedImportPackages": [ "vendor/github.com/google/gofuzz", @@ -68,8 +69,6 @@ "vendor/k8s.io/kubernetes/pkg/api/v1", "vendor/k8s.io/kubernetes/pkg/api/validation", "vendor/k8s.io/kubernetes/pkg/apis/rbac", - "github.com/openshift/origin/pkg/api/apihelpers/apitesting", - "github.com/openshift/origin/pkg/api/apihelpers", "github.com/openshift/origin/pkg/api/install", "vendor/k8s.io/kubernetes/pkg/api/helper", "github.com/openshift/origin/pkg/user/apis/user/validation", @@ -86,13 +85,12 @@ ], "allowedImportPackageRoots": [ "vendor/k8s.io/apimachinery", - "vendor/github.com/gogo/protobuf" + "vendor/github.com/gogo/protobuf", + "github.com/openshift/origin/pkg/api/apihelpers" ], "allowedImportPackages": [ "vendor/k8s.io/kubernetes/pkg/api", "vendor/k8s.io/kubernetes/pkg/api/v1", - "github.com/openshift/origin/pkg/api/apihelpers/apitesting", - "github.com/openshift/origin/pkg/api/apihelpers", "github.com/openshift/origin/pkg/api/install", "github.com/openshift/origin/pkg/util/namer", "github.com/openshift/origin/pkg/build/util", @@ -131,7 +129,8 @@ ], "allowedImportPackageRoots": [ "vendor/k8s.io/apimachinery", - "vendor/github.com/gogo/protobuf" + "vendor/github.com/gogo/protobuf", + "github.com/openshift/origin/pkg/api/apihelpers" ], "allowedImportPackages": [ "vendor/k8s.io/kubernetes/pkg/api", @@ -144,8 +143,6 @@ "vendor/github.com/docker/distribution/manifest/schema1", "vendor/github.com/docker/distribution/manifest/schema2", "vendor/github.com/blang/semver", - "github.com/openshift/origin/pkg/api/apihelpers", - "github.com/openshift/origin/pkg/api/apihelpers/apitesting", "github.com/openshift/origin/pkg/image/apis/image/install", "github.com/openshift/origin/pkg/image/reference" ] @@ -158,12 +155,10 @@ ], "allowedImportPackageRoots": [ "vendor/k8s.io/apimachinery", - "vendor/github.com/gogo/protobuf" - ], - "allowedImportPackages": [ - "github.com/openshift/origin/pkg/api/apihelpers/apitesting", + "vendor/github.com/gogo/protobuf", "github.com/openshift/origin/pkg/api/apihelpers" - ] + ], + "allowedImportPackages": [] }, { @@ -173,13 +168,12 @@ ], "allowedImportPackageRoots": [ "vendor/k8s.io/apimachinery", - "vendor/github.com/gogo/protobuf" + "vendor/github.com/gogo/protobuf", + "github.com/openshift/origin/pkg/api/apihelpers" ], "allowedImportPackages": [ "vendor/k8s.io/kubernetes/pkg/api", "vendor/k8s.io/kubernetes/pkg/api/v1", - "github.com/openshift/origin/pkg/api/apihelpers/apitesting", - "github.com/openshift/origin/pkg/api/apihelpers", "vendor/k8s.io/kubernetes/pkg/registry/core/namespace" ] }, @@ -208,14 +202,12 @@ ], "allowedImportPackageRoots": [ "vendor/k8s.io/apimachinery", - "vendor/github.com/gogo/protobuf" + "vendor/github.com/gogo/protobuf", + "github.com/openshift/origin/pkg/api/apihelpers" ], "allowedImportPackages": [ "vendor/k8s.io/kubernetes/pkg/api", - "vendor/k8s.io/kubernetes/pkg/api/v1", - "github.com/openshift/origin/pkg/api/apihelpers", - "github.com/openshift/origin/pkg/api/apihelpers/apitesting", - "github.com/openshift/origin/pkg/api/install" + "vendor/k8s.io/kubernetes/pkg/api/v1" ] }, @@ -238,12 +230,12 @@ ], "allowedImportPackageRoots": [ "vendor/k8s.io/apimachinery", - "vendor/github.com/gogo/protobuf" + "vendor/github.com/gogo/protobuf", + "github.com/openshift/origin/pkg/api/apihelpers" ], "allowedImportPackages": [ "vendor/k8s.io/kubernetes/pkg/api", - "vendor/k8s.io/kubernetes/pkg/api/v1", - "github.com/openshift/origin/pkg/api/apihelpers" + "vendor/k8s.io/kubernetes/pkg/api/v1" ] }, @@ -254,19 +246,18 @@ ], "allowedImportPackageRoots": [ "vendor/k8s.io/apimachinery", - "vendor/github.com/gogo/protobuf" + "vendor/github.com/gogo/protobuf", + "github.com/openshift/origin/pkg/api/apihelpers" ], "allowedImportPackages": [ "vendor/k8s.io/kubernetes/pkg/api", - "vendor/k8s.io/kubernetes/pkg/api/v1", - "github.com/openshift/origin/pkg/api/apihelpers/apitesting", - "github.com/openshift/origin/pkg/api/apihelpers" + "vendor/k8s.io/kubernetes/pkg/api/v1" ] }, { - "checkedPackages": [ + "checkedPackageRoots": [ "github.com/openshift/origin/pkg/api/apihelpers" ], "allowedImportPackageRoots": [ diff --git a/pkg/api/apihelpers/apitesting/fields.go b/pkg/api/apihelpers/apitesting/fields.go index 4a4b9763a433..e19f756f423f 100644 --- a/pkg/api/apihelpers/apitesting/fields.go +++ b/pkg/api/apihelpers/apitesting/fields.go @@ -3,7 +3,9 @@ package apitesting import ( "testing" + "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" ) func TestFieldLabelConversions(t *testing.T, scheme *runtime.Scheme, version, kind string, expectedLabels map[string]string, customLabels ...string) { @@ -20,3 +22,52 @@ func TestFieldLabelConversions(t *testing.T, scheme *runtime.Scheme, version, ki } } } + +// 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 { + SchemeBuilder runtime.SchemeBuilder + Kind schema.GroupVersionKind + AllowedExternalFieldKeys []string + FieldKeyEvaluatorFn FieldKeyEvaluator +} + +func (f FieldKeyCheck) Check(t *testing.T) { + scheme := runtime.NewScheme() + f.SchemeBuilder.AddToScheme(scheme) + internalObj, err := scheme.New(f.Kind.GroupKind().WithVersion(runtime.APIVersionInternal)) + if err != nil { + t.Errorf("unable to new up %v", f.Kind) + } + + for _, externalFieldKey := range f.AllowedExternalFieldKeys { + internalFieldKey, _, err := scheme.ConvertFieldLabel(f.Kind.GroupVersion().String(), f.Kind.Kind, externalFieldKey, "") + if err != nil { + t.Errorf("illegal field conversion %q for %v", externalFieldKey, f.Kind) + continue + } + + fieldSet := fields.Set{} + if err := f.FieldKeyEvaluatorFn(internalObj, fieldSet); err != nil { + t.Errorf("unable to valuate field keys for %v: %v", f.Kind, err) + continue + } + + found := false + for actualInternalFieldKey := range fieldSet { + if internalFieldKey == actualInternalFieldKey { + found = true + break + } + } + if !found { + t.Errorf("%q converted to %q which has no internal field key match for %v", externalFieldKey, internalFieldKey, f.Kind) + continue + } + + } + +} + +// FieldKeyEvaluator overlaps with the storage mutation func. We use this to confirm that the non-meta fields are actually being handled +type FieldKeyEvaluator func(obj runtime.Object, fieldSet fields.Set) error diff --git a/pkg/route/apis/route/fields.go b/pkg/route/apis/route/fields.go index dcd5c21813b8..f6bf3e4ce4a0 100644 --- a/pkg/route/apis/route/fields.go +++ b/pkg/route/apis/route/fields.go @@ -1,14 +1,19 @@ package route -import "k8s.io/apimachinery/pkg/fields" +import ( + "fmt" -// RouteToSelectableFields returns a label set that represents the object -func RouteToSelectableFields(route *Route) fields.Set { - return fields.Set{ - "metadata.name": route.Name, - "metadata.namespace": route.Namespace, - "spec.path": route.Spec.Path, - "spec.host": route.Spec.Host, - "spec.to.name": route.Spec.To.Name, + "k8s.io/apimachinery/pkg/fields" + runtime "k8s.io/apimachinery/pkg/runtime" +) + +func RouteFieldSelector(obj runtime.Object, fieldSet fields.Set) error { + route, ok := obj.(*Route) + if !ok { + return fmt.Errorf("%T not a Route", obj) } + fieldSet["spec.path"] = route.Spec.Path + fieldSet["spec.host"] = route.Spec.Host + fieldSet["spec.to.name"] = route.Spec.To.Name + return nil } diff --git a/pkg/route/apis/route/v1/conversion.go b/pkg/route/apis/route/v1/conversion.go index 92abe47f2d9f..7fabaabab440 100644 --- a/pkg/route/apis/route/v1/conversion.go +++ b/pkg/route/apis/route/v1/conversion.go @@ -2,13 +2,43 @@ package v1 import ( "k8s.io/apimachinery/pkg/runtime" - - "github.com/openshift/origin/pkg/api/apihelpers" - routeapi "github.com/openshift/origin/pkg/route/apis/route" ) -func addConversionFuncs(scheme *runtime.Scheme) error { - return scheme.AddFieldLabelConversionFunc("v1", "Route", - apihelpers.GetFieldLabelConversionFunc(routeapi.RouteToSelectableFields(&routeapi.Route{}), nil), - ) +func addLegacyFieldConversionFuncs(scheme *runtime.Scheme) error { + if err := scheme.AddFieldLabelConversionFunc(LegacySchemeGroupVersion.String(), "Route", legacyRouteFieldSelectorConversionFunc); err != nil { + return err + } + return nil +} + +func addFieldConversionFuncs(scheme *runtime.Scheme) error { + if err := scheme.AddFieldLabelConversionFunc(SchemeGroupVersion.String(), "Route", routeFieldSelectorConversionFunc); 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 legacyRouteFieldSelectorConversionFunc(label, value string) (internalLabel, internalValue string, err error) { + switch label { + case "spec.path", + "spec.host", + "spec.to.name": + return label, value, nil + default: + return runtime.DefaultMetaV1FieldSelectorConversion(label, value) + } +} + +func routeFieldSelectorConversionFunc(label, value string) (internalLabel, internalValue string, err error) { + switch label { + case "spec.path", + "spec.host", + "spec.to.name": + return label, value, nil + default: + return runtime.DefaultMetaV1FieldSelectorConversion(label, value) + } } diff --git a/pkg/route/apis/route/v1/conversion_test.go b/pkg/route/apis/route/v1/conversion_test.go index 5e25aba01091..ff04e688714a 100644 --- a/pkg/route/apis/route/v1/conversion_test.go +++ b/pkg/route/apis/route/v1/conversion_test.go @@ -6,19 +6,25 @@ import ( "k8s.io/apimachinery/pkg/runtime" "github.com/openshift/origin/pkg/api/apihelpers/apitesting" - routeapi "github.com/openshift/origin/pkg/route/apis/route" + "github.com/openshift/origin/pkg/route/apis/route" ) func TestFieldSelectorConversions(t *testing.T) { - converter := runtime.NewScheme() - LegacySchemeBuilder.AddToScheme(converter) + apitesting.FieldKeyCheck{ + SchemeBuilder: []func(*runtime.Scheme) error{LegacySchemeBuilder.AddToScheme, route.LegacySchemeBuilder.AddToScheme}, + Kind: LegacySchemeGroupVersion.WithKind("Route"), + // Ensure previously supported labels have conversions. DO NOT REMOVE THINGS FROM THIS LIST + AllowedExternalFieldKeys: []string{"spec.host", "spec.path", "spec.to.name"}, + FieldKeyEvaluatorFn: route.RouteFieldSelector, + }.Check(t) - apitesting.TestFieldLabelConversions(t, converter, "v1", "Route", - // Ensure all currently returned labels are supported - routeapi.RouteToSelectableFields(&routeapi.Route{}), + apitesting.FieldKeyCheck{ + SchemeBuilder: []func(*runtime.Scheme) error{SchemeBuilder.AddToScheme, route.SchemeBuilder.AddToScheme}, + Kind: SchemeGroupVersion.WithKind("Route"), // Ensure previously supported labels have conversions. DO NOT REMOVE THINGS FROM THIS LIST - "spec.host", "spec.path", "spec.to.name", - ) + AllowedExternalFieldKeys: []string{"spec.host", "spec.path", "spec.to.name"}, + FieldKeyEvaluatorFn: route.RouteFieldSelector, + }.Check(t) } func TestSupportingCamelConstants(t *testing.T) { diff --git a/pkg/route/apis/route/v1/defaults_test.go b/pkg/route/apis/route/v1/defaults_test.go index 37020697d7ca..161701b96a55 100644 --- a/pkg/route/apis/route/v1/defaults_test.go +++ b/pkg/route/apis/route/v1/defaults_test.go @@ -1,63 +1,68 @@ -package v1_test +package v1 import ( "reflect" "testing" "k8s.io/apimachinery/pkg/runtime" - kapi "k8s.io/kubernetes/pkg/api" + "k8s.io/apimachinery/pkg/runtime/serializer" - routeapiv1 "github.com/openshift/origin/pkg/route/apis/route/v1" - - // install all APIs - _ "github.com/openshift/origin/pkg/api/install" + "github.com/openshift/origin/pkg/route/apis/route" ) +var scheme = runtime.NewScheme() +var codecs = serializer.NewCodecFactory(scheme) + +func init() { + SchemeBuilder.AddToScheme(scheme) + route.SchemeBuilder.AddToScheme(scheme) +} + func TestDefaults(t *testing.T) { - obj := &routeapiv1.Route{ - Spec: routeapiv1.RouteSpec{ - To: routeapiv1.RouteTargetReference{Name: "other"}, - TLS: &routeapiv1.TLSConfig{}, + obj := &Route{ + Spec: RouteSpec{ + To: RouteTargetReference{Name: "other"}, + TLS: &TLSConfig{}, }, - Status: routeapiv1.RouteStatus{ - Ingress: []routeapiv1.RouteIngress{{}}, + Status: RouteStatus{ + Ingress: []RouteIngress{{}}, }, } obj2 := roundTrip(t, obj) - out, ok := obj2.(*routeapiv1.Route) + out, ok := obj2.(*Route) if !ok { t.Errorf("Unexpected object: %v", obj2) t.FailNow() } - if out.Spec.TLS.Termination != routeapiv1.TLSTerminationEdge { + if out.Spec.TLS.Termination != TLSTerminationEdge { t.Errorf("did not default termination: %#v", out) } - if out.Spec.WildcardPolicy != routeapiv1.WildcardPolicyNone { + if out.Spec.WildcardPolicy != WildcardPolicyNone { t.Errorf("did not default wildcard policy: %#v", out) } if out.Spec.To.Kind != "Service" { t.Errorf("did not default object reference kind: %#v", out) } - if out.Status.Ingress[0].WildcardPolicy != routeapiv1.WildcardPolicyNone { + if out.Status.Ingress[0].WildcardPolicy != WildcardPolicyNone { t.Errorf("did not default status ingress wildcard policy: %#v", out) } } func roundTrip(t *testing.T, obj runtime.Object) runtime.Object { - data, err := runtime.Encode(kapi.Codecs.LegacyCodec(routeapiv1.SchemeGroupVersion), obj) + data, err := runtime.Encode(codecs.LegacyCodec(SchemeGroupVersion), obj) if err != nil { t.Errorf("%v\n %#v", err, obj) return nil } - obj2, err := runtime.Decode(kapi.Codecs.UniversalDecoder(), data) + obj2, err := runtime.Decode(codecs.UniversalDecoder(), data) if err != nil { t.Errorf("%v\nData: %s\nSource: %#v", err, string(data), obj) return nil } obj3 := reflect.New(reflect.TypeOf(obj).Elem()).Interface().(runtime.Object) - err = kapi.Scheme.Convert(obj2, obj3, nil) + err = scheme.Convert(obj2, obj3, nil) if err != nil { t.Errorf("%v\nSource: %#v", err, obj2) return nil diff --git a/pkg/route/apis/route/v1/register.go b/pkg/route/apis/route/v1/register.go index 51aca2e481bc..27830aac65d7 100644 --- a/pkg/route/apis/route/v1/register.go +++ b/pkg/route/apis/route/v1/register.go @@ -16,10 +16,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, addLegacyFieldConversionFuncs, RegisterDefaults) AddToSchemeInCoreGroup = LegacySchemeBuilder.AddToScheme - SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes, addConversionFuncs, RegisterDefaults) + SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes, addFieldConversionFuncs, RegisterDefaults) AddToScheme = SchemeBuilder.AddToScheme ) diff --git a/pkg/route/registry/route/etcd/etcd.go b/pkg/route/registry/route/etcd/etcd.go index 7044db95b078..0770977a04c8 100644 --- a/pkg/route/registry/route/etcd/etcd.go +++ b/pkg/route/registry/route/etcd/etcd.go @@ -8,6 +8,7 @@ import ( "k8s.io/apiserver/pkg/registry/generic/registry" "k8s.io/apiserver/pkg/registry/rest" kapirest "k8s.io/apiserver/pkg/registry/rest" + "k8s.io/apiserver/pkg/storage" kapi "k8s.io/kubernetes/pkg/api" "github.com/openshift/origin/pkg/route" @@ -30,7 +31,6 @@ func NewREST(optsGetter restoptions.Getter, allocator route.RouteAllocator, sarC Copier: kapi.Scheme, NewFunc: func() runtime.Object { return &routeapi.Route{} }, NewListFunc: func() runtime.Object { return &routeapi.RouteList{} }, - PredicateFunc: routeregistry.Matcher, DefaultQualifiedResource: routeapi.Resource("routes"), CreateStrategy: strategy, @@ -38,7 +38,10 @@ func NewREST(optsGetter restoptions.Getter, allocator route.RouteAllocator, sarC DeleteStrategy: strategy, } - options := &generic.StoreOptions{RESTOptions: optsGetter, AttrFunc: routeregistry.GetAttrs} + options := &generic.StoreOptions{ + RESTOptions: optsGetter, + AttrFunc: storage.AttrFunc(storage.DefaultNamespaceScopedAttr).WithFieldMutation(routeapi.RouteFieldSelector), + } if err := store.CompleteWithOptions(options); err != nil { return nil, nil, err } diff --git a/pkg/route/registry/route/strategy.go b/pkg/route/registry/route/strategy.go index 89cfb8af571d..3b613a7b6237 100644 --- a/pkg/route/registry/route/strategy.go +++ b/pkg/route/registry/route/strategy.go @@ -3,12 +3,9 @@ package route import ( "fmt" - "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" - "k8s.io/apiserver/pkg/storage" "k8s.io/apiserver/pkg/storage/names" kapi "k8s.io/kubernetes/pkg/api" kvalidation "k8s.io/kubernetes/pkg/api/validation" @@ -298,21 +295,3 @@ func DecorateLegacyRouteWithEmptyDestinationCACertificates(obj runtime.Object) e return fmt.Errorf("unknown type passed to %T", obj) } } - -// GetAttrs returns labels and fields of a given object for filtering purposes -func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, bool, error) { - route, ok := obj.(*routeapi.Route) - if !ok { - return nil, nil, false, fmt.Errorf("not a route") - } - return labels.Set(route.Labels), routeapi.RouteToSelectableFields(route), route.Initializers != nil, nil -} - -// Matcher returns a matcher for a route -func Matcher(label labels.Selector, field fields.Selector) storage.SelectionPredicate { - return storage.SelectionPredicate{ - Label: label, - Field: field, - GetAttrs: GetAttrs, - } -}