Skip to content

Commit

Permalink
Merge pull request #16305 from deads2k/route-02-field-selector
Browse files Browse the repository at this point in the history 
Automatic merge from submit-queue (batch tested with PRs 16089, 16305, 16219, 15934, 16366)

fix the route field selectors

This makes the field selectors for routes work correctly and makes a slightly different pattern which works with the upstream defaulting and fieldkey methods so that we won't slip on object meta updates in the future.  It also uses the actual scheme registration to determine if the field key conversion methods work.
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot authored Sep 16, 2017
2 parents dc9c4c3 + ba743f0 commit ca68fa4
Show file tree
Hide file tree
Showing 11 changed files with 181 additions and 103 deletions.
51 changes: 21 additions & 30 deletions hack/import-restrictions.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,15 @@
],
"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",
"vendor/k8s.io/kubernetes/pkg/api",
"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",
Expand All @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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"
]
Expand All @@ -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": []
},

{
Expand All @@ -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"
]
},
Expand Down Expand Up @@ -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"
]
},

Expand All @@ -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"
]
},

Expand All @@ -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": [
Expand Down
51 changes: 51 additions & 0 deletions pkg/api/apihelpers/apitesting/fields.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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
23 changes: 14 additions & 9 deletions pkg/route/apis/route/fields.go
View file
Original file line number Diff line number Diff line change
@@ -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
}
44 changes: 37 additions & 7 deletions pkg/route/apis/route/v1/conversion.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
22 changes: 14 additions & 8 deletions pkg/route/apis/route/v1/conversion_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Loading

0 comments on commit ca68fa4

Please sign in to comment.