Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

fix: prefer core resources when enabling a type #1294

Merged

Conversation

makkes
Copy link
Contributor

@makkes makkes commented Oct 9, 2020

What this PR does / why we need it:
When a resource type such as pods is services is passed to
kubefedctl enable and there's a match for this name in the core
group as well as another group, there was no way to tell kubefedctl
that the core resource should be enabled.

Now, whenever an API resource from the core group matches the
desired type to be enabled, this resource type is preferred.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1078

Special notes for your reviewer:
Here's the steps to test this:

  • kind create cluster
  • kubectl apply --filename https://github.com/knative/serving/releases/download/v0.18.0/serving-crds.yaml
  • make kubefedctl
  • ./bin/kubefedctl enable services --kubefed-namespace="default" -o yaml

Before this change:

$ ./bin/kubefedctl enable services --kubefed-namespace="default" -o yaml
F1012 22:22:47.217201   66506 enable.go:111] Error: Multiple resources are matched by "services": services, services.serving.knative.dev. A group-qualified plural name must be provided.

With this change:

$ ./bin/kubefedctl enable services --kubefed-namespace="default" -o yaml
---
apiVersion: core.kubefed.io/v1beta1
kind: FederatedTypeConfig
[...]

When a resource type such as `pods` is `services` is passed to
`kubefedctl enable` and there's a match for this name in the `core`
group as well as another group, there was no way to tell `kubefedctl`
that the `core` resource should be enabled.

Now, whenever an API resource from the `core` group matches the
desired type to be enabled, this resource type is preferred.
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 9, 2020
@makkes
Copy link
Contributor Author

makkes commented Oct 9, 2020

/assign @jimmidyson

@irfanurrehman
Copy link
Contributor

@makkes thanks for doing this.
The implicitly selection of core when no group is specified and the core group is in the matches, seems fine to me.
However, it should be documented simply because its implicit. I think a good place to put it would be here in someway referring to details mentioned here.
Can you please do that.

@jimmidyson
Copy link
Contributor

jimmidyson commented Oct 13, 2020

I wonder if we should do the same as kubectl does and follow API group/version priority when short names (or unqualified as in this case) are used?

@makkes
Copy link
Contributor Author

makkes commented Oct 13, 2020

@jimmidyson do you have a hint where I would find out how kubectl does it?

@jimmidyson
Copy link
Contributor

@makkes Check out k8s.io/client-go/discovery package, which allows you to list server resources.

@makkes
Copy link
Contributor Author

makkes commented Oct 13, 2020

I wonder if we should do the same as kubectl does and follow API group/version priority when short names (or unqualified as in this case) are used?

We're already using ServerPreferredResources() to fetch resources to consider so already following kubectl's approach with regards to order. The only difference being that we error out when multiple resources match. In the service of keeping backwards-incompatible changes to a minimum, I would prefer to leave this PR as it is and follow-up with a PR that changes semantics to align with kubectl's approach more.

@jimmidyson wdyt?

@jimmidyson
Copy link
Contributor

This PR effectively puts core v1 API as higher priority than anything else, which is reasonable and fixes the reported bug, so I'm happy to approve this PR as-is and follow up with a PR that aligns with kubectl semantics. I agree with @irfanurrehman that this should be documented and then we can get this merged.

I think we can clean up the code quite a bit by using discovery client smarter, something like:

restMapper := restmapper.NewDeferredDiscoveryRESTMapper(discoveryClient)
shortcutRestMapper := restmapper.NewShortcutExpander(restMapper, discoveryClient)

gvks, err := shortcutRestMapper.KindsFor(schema.GroupVersionResource{Group: optionalGroup, Version: optionalVersion, Resource: resource})

@makkes
Copy link
Contributor Author

makkes commented Oct 13, 2020

I agree and will add documentation soon!

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 14, 2020
@makkes
Copy link
Contributor Author

makkes commented Oct 14, 2020

@irfanurrehman @jimmidyson I added a section to the user guide.

@jimmidyson
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jimmidyson, makkes

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 14, 2020
@irfanurrehman
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2020
@k8s-ci-robot k8s-ci-robot merged commit 2c002f9 into kubernetes-retired:master Oct 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kubedctl enable services has conflict with knative services
4 participants