Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix the route field selectors #16305

Merged
merged 1 commit into from
Sep 16, 2017
Merged

fix the route field selectors #16305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
51 changes: 21 additions & 30 deletions hack/import-restrictions.json
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

return scheme.AddField...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return scheme.AddField...

Are you sure? This stanza is copy/pasteable as more Kinds are added.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That still can be, and in most cases is the last line in addConversionFuncs. Above you'll keep adding type-specific conversion funcs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That still can be, and in most cases is the last line in addConversionFuncs. Above you'll keep adding type-specific conversion funcs.

Eh, it really can't be. Conversions aren't tied to a particular external schema version, but field conversions are. That's why there are two different methods.

return err
}
return nil
}

func addFieldConversionFuncs(scheme *runtime.Scheme) error {
if err := scheme.AddFieldLabelConversionFunc(SchemeGroupVersion.String(), "Route", routeFieldSelectorConversionFunc); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the other pr adding this to the rest, this struck me. Shouldn't that be return apihelpers.LegacyMetaV1FieldSelectorConversionWithName(label, value) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the other pr adding this to the rest, this struck me. Shouldn't that be return apihelpers.LegacyMetaV1FieldSelectorConversionWithName(label, value) ?

Interestingly enough, no. The "name" -> "metadata.name" mapping never existed for routes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

}
}

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
Open in desktop
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