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.

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

Support field selectors for CRDs #53345

Merged

Conversation

ncdc
Copy link
Member

@ncdc ncdc commented Oct 2, 2017

Signed-off-by: Andy Goldstein [email protected]

What this PR does / why we need it: allow field selectors to be used with custom resources

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #51046, fixes #49424

Special notes for your reviewer:

Release note:

Custom resources served through CustomResourceDefinition now support field selectors for `metadata.name` and `metadata.namespace`.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 2, 2017
@tamalsaha
Copy link
Member

Thanks @ncdc !

@sttts, can this be picked for client-go 5.0.0 release?

@ncdc
Copy link
Member Author

ncdc commented Oct 2, 2017

@deads2k where in the apiextensions-apiserver integration tests would you recommend testing this? In testSimpleCRUD?

@0xmichalis
Copy link
Contributor

Can you add a test in hack/make-rules/test-cmd-util.sh?

@ncdc
Copy link
Member Author

ncdc commented Oct 2, 2017

@Kargakis I'd rather put it in the integration tests

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 3, 2017
@ncdc
Copy link
Member Author

ncdc commented Oct 3, 2017

I just pushed an integration test in a separate commit. It's largely a copy of testSimpleCRUD but it instead creates 2 resources and tries to ensure that it only gets data for the object requested in the fieldSelector. I also verified that without the first commit in this PR, the new tests fail.

@nikhita
Copy link
Member

nikhita commented Oct 3, 2017

/retest

t.Errorf("expected %v, got %v", e, a)
}

listWithItem, err := noxuResourceClient.List(metav1.ListOptions{FieldSelector: "metadata.name=foo"})
Copy link
Member

Choose a reason for hiding this comment

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

Could you arrange for the objects to differ in a custom spec field, and select for that here? It would be good to test that it works for fields outside metadata.

Copy link
Member Author

Choose a reason for hiding this comment

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

That doesn't work. I believe it's because of

// objectMetaFieldsSet returns a fields that represent the ObjectMeta.
func objectMetaFieldsSet(objectMeta metav1.Object, namespaceScoped bool) fields.Set {
if namespaceScoped {
return fields.Set{
"metadata.name": objectMeta.GetName(),
"metadata.namespace": objectMeta.GetNamespace(),
}
}
return fields.Set{
"metadata.name": objectMeta.GetName(),
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I could write some code that creates a fields.Set of every possible field leaf (excluding slices); e.g., if we have this:

a: b
c: 1
d:
  - a
  - b
e:
  f: g
  h:
    i: j

the field set would be

a: b
c: 1
e.f: g
e.h.i: j

Copy link
Member

Choose a reason for hiding this comment

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

I think you're right. I must have misremembered that it worked for other fields in my earlier experiment. Should we specify in the release note that CRD field selectors only support name and namespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@Kargakis I don't have any concerns with the UX of allowing arbitrary fields; that was my plan when I thought all we needed to do was stop blocking it. However, it turns out we also have to recursively flatten the whole custom object into a map, for every object returned by a list, in order to make filtering work for arbitrary fields.

That seems extreme given how conservative other objects are with their supported field selectors. For example, Pod only picks out 3 fields from spec that you're allowed to use in fieldSelector.

If you have a use case for filtering on arbitrary fields, would you be ok with having to specify a whitelist in the CRD spec, so we don't have to export every field regardless of whether anyone wants it?

We could discuss that on a separate feature request issue. Either way I think we should go ahead and merge this fix for metadata.name, since that's important for watching single objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreeing to go ahead with this PR before discussing abritrary keys in a follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

@enisoc yeah, a whitelist would be great to have. Please cc me one the new issue when one is made.

Copy link
Member

Choose a reason for hiding this comment

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

@Kargakis I think it's better if you file the issue. I don't actually have any use case in mind for needing arbitrary field selectors, so I can't explain why the feature is being requested.

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened #53459

}

func (crdObjectConverter) ConvertFieldLabel(version, kind, label, value string) (string, string, error) {
// Just return the passed-in label and value, as CRDs only support a single version currently.
Copy link
Member

Choose a reason for hiding this comment

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

For other types, we return an error at this point if the field label isn't in a whitelist:

return "", "", fmt.Errorf("field label not supported: %s", label)

If we don't error out, it just comes back as an empty list, which is misleading.

Whitelisting metadata.name and metadata.namespace would also mean we can remove the caveat about this only working because CRD currently supports only one version -- at least until ObjectMeta v2.

Copy link
Member Author

Choose a reason for hiding this comment

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

@enisoc are you saying this should just return the result of k8s.io/apimachinery/pkg/runtime/conversion.go#DefaultMetaV1FieldSelectorConversion()?

Copy link
Member

Choose a reason for hiding this comment

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

Hm the behavior there seems reasonable, although it's slightly different than what other resources do. Other cluster-scoped resources seem to error out on metadata.namespace:

err = scheme.AddFieldLabelConversionFunc("v1", "Node",
func(label, value string) (string, string, error) {
switch label {
case "metadata.name":
return label, value, nil
case "spec.unschedulable":
return label, value, nil
default:
return "", "", fmt.Errorf("field label not supported: %s", label)
}
},
)

Copy link
Member Author

Choose a reason for hiding this comment

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

We know if it's cluster-scoped or not, so I'll just write a custom function

Copy link
Member Author

Choose a reason for hiding this comment

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

@enisoc just pushed an update - ptal!

@enisoc
Copy link
Member

enisoc commented Oct 3, 2017

/lgtm

BTW I updated the release note to specifically call out the fields that are supported.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 3, 2017
@ncdc
Copy link
Member Author

ncdc commented Oct 3, 2017

Ok thanks. I need to squash, so it'll need 1 more round of lgtm. Will ping in a few.

Signed-off-by: Andy Goldstein <[email protected]>
@ncdc ncdc force-pushed the crd-add-fieldSelector-support branch from cd3119c to 2ff8730 Compare October 3, 2017 19:22
@ncdc
Copy link
Member Author

ncdc commented Oct 3, 2017

Squashed

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 3, 2017
@enisoc
Copy link
Member

enisoc commented Oct 3, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 3, 2017
}

case <-time.After(5 * time.Second):
t.Errorf("missing watch event")
Copy link
Contributor

Choose a reason for hiding this comment

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

is 5 seconds enough to avoid flakes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps not, but this is not net new (I copied an existing test)

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2017
@ncdc
Copy link
Member Author

ncdc commented Oct 4, 2017

Re-applying lgtm b/c the only new commit was to update bazel.

@ncdc ncdc added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2017
@deads2k
Copy link
Contributor

deads2k commented Oct 4, 2017

/approve

@ncdc ncdc removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2017
@ncdc
Copy link
Member Author

ncdc commented Oct 4, 2017

Gah I had an extra .go file in the repo root that caused bazel to do stuff it shouldn't have. Fixing.

Signed-off-by: Andy Goldstein <[email protected]>
@ncdc ncdc force-pushed the crd-add-fieldSelector-support branch from 283ad90 to 74b4db2 Compare October 4, 2017 13:32
@ncdc
Copy link
Member Author

ncdc commented Oct 4, 2017

Ok, that's better. @deads2k mind giving it a quick glance again and tagging?

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2017
@deads2k
Copy link
Contributor

deads2k commented Oct 4, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, enisoc, ncdc

Associated issue: 51046

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@sttts
Copy link
Contributor

sttts commented Oct 4, 2017

/retest

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit e9a0b15 into kubernetes:master Oct 4, 2017
@tamalsaha
Copy link
Member

In case anyone is listening, this used to work with TPRs and was broken when we moved to CRDs.

@0xmichalis
Copy link
Contributor

index

@dims
Copy link
Member

dims commented Oct 5, 2017

@Kargakis : it should be RED!

@0xmichalis
Copy link
Contributor

In all seriousness, @tamalsaha if there is a backport that needs to be done or what you mention is still an issue, consider opening a separate issue.

@tamalsaha
Copy link
Member

tamalsaha commented Oct 5, 2017

@Kargakis , as a workaround, we now watch everything in a namespace and skip with an if . That works for now.

@resouer
Copy link
Contributor

resouer commented Jan 20, 2018

@ncdc Great work! This is included in 1.9 release right?

@nikhita
Copy link
Member

nikhita commented Jan 21, 2018

This is included in 1.9 release right?

Yes. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to use a fieldSelector with custom resources CRD and TPR doesn't support watching one single instance