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

Add group alias names to API resources to allow discovery #43338

Merged
merged 3 commits into from
Jun 6, 2017
Merged

Add group alias names to API resources to allow discovery #43338

merged 3 commits into from
Jun 6, 2017

Conversation

fabianofranz
Copy link
Contributor

@fabianofranz fabianofranz commented Mar 18, 2017 •
edited
Loading

What this PR does / why we need it:
Adds GroupNames []string to API resources, which represents the list of group aliases that every resource belongs to.

Partially fixes #41353

This moves the logic of "all" (which currently translates to "pods,replicationcontrollers,services,...") to the server-side. Will allow clients like kubectl to discover group aliases instead of having it hardcoded and the API server to better handle consistency across multiple clients, version skew, etc; and will make "all" un-special and allow other groups to be created.

As a follow-up we'll patch kubectl to make groups aliases discoverable and the hardcoded list a fallback while we still have to support it.

Related to #42595 (comment).

Release note:

Adds the `Categories []string` field to API resources, which represents the list of group aliases (e.g. "all") that every resource belongs to.

@kubernetes/sig-api-machinery-misc @deads2k @bgrant0607

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 18, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 18, 2017
@liggitt
Copy link
Member

liggitt commented Mar 18, 2017

I like the concept, but "group" is a really overloaded term. Is there something else we can call this?

@deads2k
Copy link
Contributor

deads2k commented Mar 20, 2017

I like the concept, but "group" is a really overloaded term. Is there something else we can call this?

What about categories?

@fabianofranz
Copy link
Contributor Author

I like the concept, but "group" is a really overloaded term. Is there something else we can call this?

What about categories?

Aliases, categories, alias groups, alias classes, in aliases, part of?

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 20, 2017
@fabianofranz
Copy link
Contributor Author

@k8s-bot node e2e test this
@k8s-bot unit test this

@lavalamp lavalamp assigned lavalamp and unassigned dims Mar 20, 2017
lavalamp
lavalamp reviewed Mar 20, 2017
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go Outdated
@@ -693,6 +693,8 @@ type APIResource struct {
Verbs Verbs `json:"verbs" protobuf:"bytes,4,opt,name=verbs"`
// shortNames is a list of suggested short names of the resource.
ShortNames []string `json:"shortNames,omitempty" protobuf:"bytes,5,rep,name=shortNames"`
// groupNames is a list of group aliases this resource belongs to (e.g. 'all')
GroupNames []string `json:"groupNames,omitempty" protobuf:"bytes,6,rep,name=groupNames"`
Copy link
Member

Choose a reason for hiding this comment

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

AliasMembership? Aliases? AliasInclusion? AliasesAssociated?

Definitely this can't be called anything with the word "group", too confusing with api groups.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think its quite an alias. Alias implies its a one to one match. This is more like a category where the category explodes into multiple resources.

Copy link
Member

Choose a reason for hiding this comment

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

Other thought: What if we extend ShortName to just include such things?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about AssociatedCategories, InCategories, LinkedCategories?

Copy link
Contributor

Choose a reason for hiding this comment

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

Other thought: What if we extend ShortName to just include such things?

I don't think that you want to accidentally end up with a category name since that will do weird things in the CLI. all/some-name is unlikely to play nice in kubectl.

That does raise some interesting name-squatting concerns. Perhaps a CLI user should be forced to opt-in to particular categories in some way?

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if a shortname and resource name collide today?

Shortnames are resolved ahead of resource names. I found it while looking at writing a new kind of RESTMapper.

Copy link
Member

@liggitt liggitt Mar 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

hmm, that seems backwards. can we reverse that before we ship a collision?

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 seems backwards

Looks like the order should be resource names > shortnames > categories.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, that seems backwards. can we reverse that before we ship a collision?

As I said, "while looking at writing a new kind of RESTMapper". Fix for 1.7, but given layering probably not for 1.6.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR updated to call it Categories.

@fabianofranz
Copy link
Contributor Author

@k8s-bot unit test this

@fabianofranz fabianofranz force-pushed the group_aliases_in_api branch 3 times, most recently from 88da41c to 0dc2082 Compare March 27, 2017 17:48
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 27, 2017
@fabianofranz fabianofranz force-pushed the group_aliases_in_api branch 2 times, most recently from c8367bc to f9a4a5c Compare March 27, 2017 19:10
@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 27, 2017
@fabianofranz
Copy link
Contributor Author

@k8s-bot unit test this

@fabianofranz fabianofranz force-pushed the group_aliases_in_api branch 2 times, most recently from 4039048 to 2132656 Compare March 28, 2017 04:04
deads2k
deads2k reviewed Mar 28, 2017
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go
@@ -697,6 +697,8 @@ type APIResource struct {
Verbs Verbs `json:"verbs" protobuf:"bytes,4,opt,name=verbs"`
// shortNames is a list of suggested short names of the resource.
ShortNames []string `json:"shortNames,omitempty" protobuf:"bytes,5,rep,name=shortNames"`
// categories is a list of the grouped resources this resource belongs to (e.g. 'all')
Categories []string `json:"categories,omitempty" protobuf:"bytes,7,rep,name=categories"`
Copy link
Contributor

Choose a reason for hiding this comment

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

lgtm

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 2, 2017
@fabianofranz fabianofranz added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2017
@fabianofranz
Copy link
Contributor Author

@k8s-bot pull-kubernetes-federation-e2e-gce test this
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 3, 2017
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 4, 2017
@fabianofranz fabianofranz added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 4, 2017
@fabianofranz
Copy link
Contributor Author

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@fabianofranz
Copy link
Contributor Author

@k8s-bot pull-kubernetes-unit test this

@deads2k
Copy link
Contributor

deads2k commented Jun 5, 2017

/retest

@deads2k
Copy link
Contributor

deads2k commented Jun 5, 2017

hadn't seen this before. From an integration tests failed to listen on 127.0.0.1:43079: listen tcp 127.0.0.1:43079: bind: address already in use https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/43338/pull-kubernetes-unit/34892/

/retest

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2017
@fabianofranz fabianofranz added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2017
@fabianofranz
Copy link
Contributor Author

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@fabianofranz
Copy link
Contributor Author

@k8s-bot pull-kubernetes-e2e-kops-aws test this

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jun 5, 2017 •
edited
Loading

@fabianofranz: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-federation-e2e-gce 9cd7f871d7afbd4590f124ed718398aa3ef562b5 link @k8s-bot pull-kubernetes-federation-e2e-gce test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@fabianofranz
Copy link
Contributor Author

@k8s-bot pull-kubernetes-e2e-kops-aws test this

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 5, 2017
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 5, 2017
@fabianofranz fabianofranz added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 46967, 46992, 43338, 46717, 46672)

@k8s-github-robot k8s-github-robot merged commit 6b50a5c into kubernetes:master Jun 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

@liggitt liggitt liggitt left review comments

@lavalamp lavalamp lavalamp left review comments

Assignees

@lavalamp lavalamp

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api Indicates an issue on api area. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.

Use of hardcoded "UserResources" breaks kubectl get all if server does not have at least one of the resources
9 participants
@fabianofranz @k8s-reviewable @liggitt @deads2k @smarterclayton @k8s-github-robot @k8s-ci-robot @lavalamp @dims