Skip to content

Commit ab9f41f

Browse files
openshift-merge-robotopenshift-merge-robot
authored
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.
2 parents 6edb23d + 82c48ea commit ab9f41f

File tree

40 files changed

+488
-397
lines changed

40 files changed

+488
-397
lines changed

‎hack/import-restrictions.json

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -173,8 +173,7 @@
173173
],
174174
"allowedImportPackages": [
175175
"vendor/k8s.io/kubernetes/pkg/api",
176-
"vendor/k8s.io/kubernetes/pkg/api/v1",
177-
"vendor/k8s.io/kubernetes/pkg/registry/core/namespace"
176+
"vendor/k8s.io/kubernetes/pkg/api/v1"
178177
]
179178
},
180179

‎pkg/api/apihelpers/apitesting/fields.go

Lines changed: 4 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -8,21 +8,6 @@ import (
88
"k8s.io/apimachinery/pkg/runtime/schema"
99
)
1010

11-
func TestFieldLabelConversions(t *testing.T, scheme *runtime.Scheme, version, kind string, expectedLabels map[string]string, customLabels ...string) {
12-
for label := range expectedLabels {
13-
_, _, err := scheme.ConvertFieldLabel(version, kind, label, "")
14-
if err != nil {
15-
t.Errorf("No conversion registered for %s for %s %s", label, version, kind)
16-
}
17-
}
18-
for _, label := range customLabels {
19-
_, _, err := scheme.ConvertFieldLabel(version, kind, label, "")
20-
if err != nil {
21-
t.Errorf("No conversion registered for %s for %s %s", label, version, kind)
22-
}
23-
}
24-
}
25-
2611
// FieldKeyCheck gathers information to check if the field key conversions are working correctly. It takes many parameters
2712
// in an attempt to reflect reality
2813
type FieldKeyCheck struct {
@@ -46,6 +31,10 @@ func (f FieldKeyCheck) Check(t *testing.T) {
4631
t.Errorf("illegal field conversion %q for %v", externalFieldKey, f.Kind)
4732
continue
4833
}
34+
// we get this by default
35+
if internalFieldKey == "metadata.name" {
36+
continue
37+
}
4938

5039
fieldSet := fields.Set{}
5140
if err := f.FieldKeyEvaluatorFn(internalObj, fieldSet); err != nil {

‎pkg/api/apihelpers/fields.go

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
package apihelpers
22

33
import (
4-
"fmt"
5-
64
"k8s.io/apimachinery/pkg/runtime"
75
)
86

@@ -16,19 +14,3 @@ func LegacyMetaV1FieldSelectorConversionWithName(label, value string) (string, s
1614
return runtime.DefaultMetaV1FieldSelectorConversion(label, value)
1715
}
1816
}
19-
20-
// GetFieldLabelConversionFunc returns a field label conversion func, which does the following:
21-
// * returns overrideLabels[label], value, nil if the specified label exists in the overrideLabels map
22-
// * 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)
23-
// * otherwise, returns an error
24-
func GetFieldLabelConversionFunc(supportedLabels map[string]string, overrideLabels map[string]string) func(label, value string) (string, string, error) {
25-
return func(label, value string) (string, string, error) {
26-
if label, overridden := overrideLabels[label]; overridden {
27-
return label, value, nil
28-
}
29-
if _, supported := supportedLabels[label]; supported {
30-
return label, value, nil
31-
}
32-
return "", "", fmt.Errorf("field label not supported: %s", label)
33-
}
34-
}

‎pkg/authorization/apis/authorization/fields.go

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,17 @@
11
package authorization
22

3-
import "k8s.io/apimachinery/pkg/fields"
3+
import (
4+
"fmt"
45

5-
// PolicyBindingToSelectableFields returns a label set that represents the object
6-
// changes to the returned keys require registering conversions for existing versions using Scheme.AddFieldLabelConversionFunc
7-
func PolicyBindingToSelectableFields(policyBinding *PolicyBinding) fields.Set {
8-
return fields.Set{
9-
"metadata.name": policyBinding.Name,
10-
"metadata.namespace": policyBinding.Namespace,
11-
"policyRef.namespace": policyBinding.PolicyRef.Namespace,
6+
"k8s.io/apimachinery/pkg/fields"
7+
runtime "k8s.io/apimachinery/pkg/runtime"
8+
)
9+
10+
func PolicyBindingFieldSelector(obj runtime.Object, fieldSet fields.Set) error {
11+
policyBinding, ok := obj.(*PolicyBinding)
12+
if !ok {
13+
return fmt.Errorf("%T not a PolicyBinding", obj)
1214
}
15+
fieldSet["policyRef.namespace"] = policyBinding.PolicyRef.Namespace
16+
return nil
1317
}

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

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -394,15 +394,32 @@ func addConversionFuncs(scheme *runtime.Scheme) error {
394394
return err
395395
}
396396

397-
if err := scheme.AddFieldLabelConversionFunc("v1", "PolicyBinding",
398-
apihelpers.GetFieldLabelConversionFunc(newer.PolicyBindingToSelectableFields(&newer.PolicyBinding{}), nil),
399-
); err != nil {
397+
return nil
398+
}
399+
400+
func addLegacyFieldSelectorKeyConversions(scheme *runtime.Scheme) error {
401+
if err := scheme.AddFieldLabelConversionFunc(LegacySchemeGroupVersion.String(), "PolicyBinding", legacyPolicyBindingFieldSelectorKeyConversionFunc); err != nil {
400402
return err
401403
}
404+
return nil
405+
}
402406

407+
func addFieldSelectorKeyConversions(scheme *runtime.Scheme) error {
403408
return nil
404409
}
405410

411+
// because field selectors can vary in support by version they are exposed under, we have one function for each
412+
// groupVersion we're registering for
413+
414+
func legacyPolicyBindingFieldSelectorKeyConversionFunc(label, value string) (internalLabel, internalValue string, err error) {
415+
switch label {
416+
case "policyRef.namespace":
417+
return label, value, nil
418+
default:
419+
return runtime.DefaultMetaV1FieldSelectorConversion(label, value)
420+
}
421+
}
422+
406423
var _ runtime.NestedObjectDecoder = &PolicyRule{}
407424
var _ runtime.NestedObjectEncoder = &PolicyRule{}
408425

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

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,11 @@ import (
1010
)
1111

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

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,10 @@ var (
1515
SchemeGroupVersion = schema.GroupVersion{Group: GroupName, Version: "v1"}
1616
LegacySchemeGroupVersion = schema.GroupVersion{Group: LegacyGroupName, Version: "v1"}
1717

18-
LegacySchemeBuilder = runtime.NewSchemeBuilder(addLegacyKnownTypes, addConversionFuncs, RegisterDefaults)
18+
LegacySchemeBuilder = runtime.NewSchemeBuilder(addLegacyKnownTypes, addConversionFuncs, addLegacyFieldSelectorKeyConversions, RegisterDefaults)
1919
AddToSchemeInCoreGroup = LegacySchemeBuilder.AddToScheme
2020

21-
SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes, addConversionFuncs, RegisterDefaults)
21+
SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes, addConversionFuncs, addFieldSelectorKeyConversions, RegisterDefaults)
2222
AddToScheme = SchemeBuilder.AddToScheme
2323
)
2424

‎pkg/build/apis/build/fields.go

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,19 @@
11
package build
22

3-
import "k8s.io/apimachinery/pkg/fields"
4-
5-
// BuildToSelectableFields returns a label set that represents the object
6-
// changes to the returned keys require registering conversions for existing versions using Scheme.AddFieldLabelConversionFunc
7-
func BuildToSelectableFields(build *Build) fields.Set {
8-
return fields.Set{
9-
"metadata.name": build.Name,
10-
"metadata.namespace": build.Namespace,
11-
"status": string(build.Status.Phase),
12-
"podName": GetBuildPodName(build),
3+
import (
4+
"fmt"
5+
6+
"k8s.io/apimachinery/pkg/fields"
7+
runtime "k8s.io/apimachinery/pkg/runtime"
8+
)
9+
10+
func BuildFieldSelector(obj runtime.Object, fieldSet fields.Set) error {
11+
build, ok := obj.(*Build)
12+
if !ok {
13+
return fmt.Errorf("%T not a Build", obj)
1314
}
15+
fieldSet["status"] = string(build.Status.Phase)
16+
fieldSet["podName"] = GetBuildPodName(build)
17+
18+
return nil
1419
}

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

Lines changed: 32 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -174,24 +174,45 @@ func addConversionFuncs(scheme *runtime.Scheme) error {
174174
return err
175175
}
176176

177-
if err := scheme.AddFieldLabelConversionFunc(SchemeGroupVersion.String(), "Build",
178-
apihelpers.GetFieldLabelConversionFunc(newer.BuildToSelectableFields(&newer.Build{}), nil),
179-
); err != nil {
180-
return err
181-
}
182-
183177
return nil
184178
}
185179

186-
func addLegacyFieldLabelConversions(scheme *runtime.Scheme) error {
187-
if err := scheme.AddFieldLabelConversionFunc("v1", "Build",
188-
apihelpers.GetFieldLabelConversionFunc(newer.BuildToSelectableFields(&newer.Build{}), map[string]string{"name": "metadata.name"}),
189-
); err != nil {
180+
func addLegacyFieldSelectorKeyConversions(scheme *runtime.Scheme) error {
181+
if err := scheme.AddFieldLabelConversionFunc(LegacySchemeGroupVersion.String(), "Build", legacyBuildFieldSelectorKeyConversionFunc); err != nil {
190182
return err
191183
}
184+
if err := scheme.AddFieldLabelConversionFunc(LegacySchemeGroupVersion.String(), "BuildConfig", apihelpers.LegacyMetaV1FieldSelectorConversionWithName); err != nil {
185+
return err
186+
}
187+
return nil
188+
}
192189

193-
if err := scheme.AddFieldLabelConversionFunc("v1", "BuildConfig", apihelpers.LegacyMetaV1FieldSelectorConversionWithName); err != nil {
190+
func addFieldSelectorKeyConversions(scheme *runtime.Scheme) error {
191+
if err := scheme.AddFieldLabelConversionFunc(SchemeGroupVersion.String(), "Build", buildFieldSelectorKeyConversionFunc); err != nil {
194192
return err
195193
}
196194
return nil
197195
}
196+
197+
// because field selectors can vary in support by version they are exposed under, we have one function for each
198+
// groupVersion we're registering for
199+
200+
func legacyBuildFieldSelectorKeyConversionFunc(label, value string) (internalLabel, internalValue string, err error) {
201+
switch label {
202+
case "status",
203+
"podName":
204+
return label, value, nil
205+
default:
206+
return apihelpers.LegacyMetaV1FieldSelectorConversionWithName(label, value)
207+
}
208+
}
209+
210+
func buildFieldSelectorKeyConversionFunc(label, value string) (internalLabel, internalValue string, err error) {
211+
switch label {
212+
case "status",
213+
"podName":
214+
return label, value, nil
215+
default:
216+
return runtime.DefaultMetaV1FieldSelectorConversion(label, value)
217+
}
218+
}

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

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,29 @@ import (
1616
var Convert = knewer.Scheme.Convert
1717

1818
func TestFieldSelectorConversions(t *testing.T) {
19-
converter := runtime.NewScheme()
20-
LegacySchemeBuilder.AddToScheme(converter)
19+
apitesting.FieldKeyCheck{
20+
SchemeBuilder: []func(*runtime.Scheme) error{LegacySchemeBuilder.AddToScheme, newer.LegacySchemeBuilder.AddToScheme},
21+
Kind: LegacySchemeGroupVersion.WithKind("Build"),
22+
// Ensure previously supported labels have conversions. DO NOT REMOVE THINGS FROM THIS LIST
23+
AllowedExternalFieldKeys: []string{"name", "status", "podName"},
24+
FieldKeyEvaluatorFn: newer.BuildFieldSelector,
25+
}.Check(t)
26+
27+
apitesting.FieldKeyCheck{
28+
SchemeBuilder: []func(*runtime.Scheme) error{LegacySchemeBuilder.AddToScheme, newer.LegacySchemeBuilder.AddToScheme},
29+
Kind: LegacySchemeGroupVersion.WithKind("BuildConfig"),
30+
// Ensure previously supported labels have conversions. DO NOT REMOVE THINGS FROM THIS LIST
31+
AllowedExternalFieldKeys: []string{"name"},
32+
}.Check(t)
2133

22-
apitesting.TestFieldLabelConversions(t, converter, "v1", "Build",
23-
// Ensure all currently returned labels are supported
24-
newer.BuildToSelectableFields(&newer.Build{}),
34+
apitesting.FieldKeyCheck{
35+
SchemeBuilder: []func(*runtime.Scheme) error{SchemeBuilder.AddToScheme, newer.SchemeBuilder.AddToScheme},
36+
Kind: SchemeGroupVersion.WithKind("Build"),
2537
// Ensure previously supported labels have conversions. DO NOT REMOVE THINGS FROM THIS LIST
26-
"name", "status", "podName",
27-
)
38+
AllowedExternalFieldKeys: []string{"status", "podName"},
39+
FieldKeyEvaluatorFn: newer.BuildFieldSelector,
40+
}.Check(t)
41+
2842
}
2943

3044
func TestBinaryBuildRequestOptions(t *testing.T) {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,10 @@ var (
1313
SchemeGroupVersion = schema.GroupVersion{Group: GroupName, Version: "v1"}
1414
LegacySchemeGroupVersion = schema.GroupVersion{Group: LegacyGroupName, Version: "v1"}
1515

16-
LegacySchemeBuilder = runtime.NewSchemeBuilder(addLegacyKnownTypes, addConversionFuncs, RegisterDefaults, addLegacyFieldLabelConversions)
16+
LegacySchemeBuilder = runtime.NewSchemeBuilder(addLegacyKnownTypes, addConversionFuncs, addLegacyFieldSelectorKeyConversions, RegisterDefaults)
1717
AddToSchemeInCoreGroup = LegacySchemeBuilder.AddToScheme
1818

19-
SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes, addConversionFuncs, RegisterDefaults)
19+
SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes, addConversionFuncs, addFieldSelectorKeyConversions, RegisterDefaults)
2020
AddToScheme = SchemeBuilder.AddToScheme
2121
)
2222

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"k8s.io/apiserver/pkg/registry/generic"
77
"k8s.io/apiserver/pkg/registry/generic/registry"
88
"k8s.io/apiserver/pkg/registry/rest"
9+
"k8s.io/apiserver/pkg/storage"
910
kapi "k8s.io/kubernetes/pkg/api"
1011

1112
buildapi "github.com/openshift/origin/pkg/build/apis/build"
@@ -25,15 +26,17 @@ func NewREST(optsGetter restoptions.Getter) (*REST, *DetailsREST, error) {
2526
Copier: kapi.Scheme,
2627
NewFunc: func() runtime.Object { return &buildapi.Build{} },
2728
NewListFunc: func() runtime.Object { return &buildapi.BuildList{} },
28-
PredicateFunc: build.Matcher,
2929
DefaultQualifiedResource: buildapi.Resource("builds"),
3030

3131
CreateStrategy: build.Strategy,
3232
UpdateStrategy: build.Strategy,
3333
DeleteStrategy: build.Strategy,
3434
}
3535

36-
options := &generic.StoreOptions{RESTOptions: optsGetter, AttrFunc: build.GetAttrs}
36+
options := &generic.StoreOptions{
37+
RESTOptions: optsGetter,
38+
AttrFunc: storage.AttrFunc(storage.DefaultNamespaceScopedAttr).WithFieldMutation(buildapi.BuildFieldSelector),
39+
}
3740
if err := store.CompleteWithOptions(options); err != nil {
3841
return nil, nil, err
3942
}

‎pkg/build/registry/build/strategy.go

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,12 @@
11
package build
22

33
import (
4-
"fmt"
54
"reflect"
65

76
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
8-
"k8s.io/apimachinery/pkg/fields"
9-
"k8s.io/apimachinery/pkg/labels"
107
"k8s.io/apimachinery/pkg/runtime"
118
"k8s.io/apimachinery/pkg/util/validation/field"
129
apirequest "k8s.io/apiserver/pkg/endpoints/request"
13-
kstorage "k8s.io/apiserver/pkg/storage"
1410
"k8s.io/apiserver/pkg/storage/names"
1511
kapi "k8s.io/kubernetes/pkg/api"
1612

@@ -82,24 +78,6 @@ func (strategy) CheckGracefulDelete(obj runtime.Object, options *metav1.DeleteOp
8278
return false
8379
}
8480

85-
// GetAttrs returns labels and fields of a given object for filtering purposes
86-
func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, bool, error) {
87-
build, ok := obj.(*buildapi.Build)
88-
if !ok {
89-
return nil, nil, false, fmt.Errorf("not a Build")
90-
}
91-
return labels.Set(build.ObjectMeta.Labels), buildapi.BuildToSelectableFields(build), build.Initializers != nil, nil
92-
}
93-
94-
// Matcher returns a generic matcher for a given label and field selector.
95-
func Matcher(label labels.Selector, field fields.Selector) kstorage.SelectionPredicate {
96-
return kstorage.SelectionPredicate{
97-
Label: label,
98-
Field: field,
99-
GetAttrs: GetAttrs,
100-
}
101-
}
102-
10381
type detailsStrategy struct {
10482
strategy
10583
}

0 commit comments

Comments
 (0)