select an RBAC version for kubefed it knows how to speak #50537

liggitt · 2017-08-11T23:34:41Z

kubefed tries to speak whatever version of RBAC the server has, regardless of whether it knows about that version or not. the version discovery it does has to select a version both it and the server speak.

related to #50534

fixes kubefed's ability to create RBAC roles in version-skewed clusters

nikhiljindal · 2017-08-11T23:53:01Z

Thanks! To clarify the clientset that kubefed is using needs to know about these versions (for ex: be able to convert internal Role struct to v1.Role struct). kubefed does not have any version specific logic, right?

nikhiljindal · 2017-08-11T23:57:39Z

federation/pkg/kubefed/util/util.go

@@ -285,27 +288,47 @@ func getRBACVersion(discoveryclient discovery.CachedDiscoveryInterface) (*schema
 		return nil, fmt.Errorf("Couldn't get clientset to create RBAC roles in the host cluster: %v", err)
 	}

+	// These are the RBAC versions we can speak
+	knownVersions := map[schema.GroupVersion]bool{


This is a generic problem we need to solve for all API resources that kubefed creates.
Filed #50540

do you do discovery the same way for other resources, and work with internal versions and expect to be able to convert to the server's preferred version?

I don't think we try to discover group versions for other resources. However, we do work with internal versions the same way, but that doesn't qualify to this list I guess.

So at other places, we assume that the server supports a specific version (like extensions/v1beta1.Deployments?). Why not do the same here?

Whenever we will add a new version, we will likely miss adding it here anyways.

Probably because kubefed tried to support clusters with only alpha or only beta RBAC. I'm not opposed to fixing the version to v1beta1 for now, and moving to v1 later when v1beta1 is deprecated, though I'd keep this PR and the associated pick small, then change approaches going forward in 1.8

Doing it as a follow up sgtm

nikhiljindal · 2017-08-11T23:58:02Z

/lgtm
/approve

k8s-github-robot · 2017-08-11T23:58:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, nikhiljindal

Associated issue: 50534

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

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

~~federation/OWNERS~~ [nikhiljindal]

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

liggitt · 2017-08-11T23:59:44Z

To clarify the clientset that kubefed is using needs to know about these versions (for ex: be able to convert internal Role struct to v1.Role struct). kubefed does not have any version specific logic, right?

Exactly, it's all about the conversion. Your choices are either to work with external versions (like kubectl does when it just passes through a manifest given to it with kubectl create -f ...) and require the server to speak that version, or to work with internal versions and select an external version the server knows about that you know how to convert to.

k8s-github-robot · 2017-08-12T01:27:05Z

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

k8s-ci-robot · 2017-08-12T01:47:27Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-unit	`7f1a617`	link	`/test pull-kubernetes-unit`

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.

liggitt · 2017-08-12T02:22:03Z

Unrelated unit test failure. /retest

k8s-github-robot · 2017-08-12T02:43:54Z

Automatic merge from submit-queue (batch tested with PRs 50537, 49699, 50160, 49025, 50205)

…7-upstream-release-1.7 Automatic merge from submit-queue Automated cherry pick of #50537 fixes #50534 Cherry pick of #50537 on release-1.7. #50537: select an RBAC version for kubefed it knows how to speak ```release-note fixes kubefed's ability to create RBAC roles in version-skewed clusters ```

k8s-cherrypick-bot · 2017-08-12T08:05:41Z

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 11, 2017

k8s-github-robot assigned shashidharatd and nikhiljindal Aug 11, 2017

k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 11, 2017

liggitt mentioned this pull request Aug 11, 2017

Automated cherry pick of #50537 #50538

Merged

k8s-github-robot added the release-note-label-needed label Aug 11, 2017

liggitt added the cherrypick-candidate label Aug 11, 2017

liggitt added this to the v1.7 milestone Aug 11, 2017

liggitt added kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. queue/fix labels Aug 11, 2017

liggitt assigned madhusudancs and unassigned shashidharatd Aug 11, 2017

select an RBAC version for kubefed it knows how to speak

7f1a617

liggitt force-pushed the kubefed-rbac branch from 7a0fae6 to 7f1a617 Compare August 11, 2017 23:48

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 11, 2017

nikhiljindal mentioned this pull request Aug 11, 2017

kubefed should use an API version that both it and server uses #50540

Closed

nikhiljindal reviewed Aug 11, 2017

View reviewed changes

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

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Aug 11, 2017

liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note-label-needed labels Aug 12, 2017

k8s-github-robot merged commit c207dd5 into kubernetes:master Aug 12, 2017

wojtek-t added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Aug 12, 2017

k8s-cherrypick-bot removed the cherrypick-candidate label Aug 12, 2017

liggitt deleted the kubefed-rbac branch August 18, 2017 04:04

irfanurrehman mentioned this pull request Oct 30, 2017

kubefed should use an API version that both it and server uses kubernetes-retired/federation#27

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

select an RBAC version for kubefed it knows how to speak #50537

select an RBAC version for kubefed it knows how to speak #50537

liggitt commented Aug 11, 2017 •

edited

Loading

nikhiljindal commented Aug 11, 2017

nikhiljindal Aug 11, 2017

liggitt Aug 11, 2017 •

edited

Loading

madhusudancs Aug 12, 2017

nikhiljindal Aug 12, 2017

liggitt Aug 12, 2017

nikhiljindal Aug 12, 2017

nikhiljindal commented Aug 11, 2017 •

edited

Loading

k8s-github-robot commented Aug 11, 2017

liggitt commented Aug 11, 2017 •

edited

Loading

k8s-github-robot commented Aug 12, 2017

k8s-ci-robot commented Aug 12, 2017 •

edited

Loading

liggitt commented Aug 12, 2017

k8s-github-robot commented Aug 12, 2017

k8s-cherrypick-bot commented Aug 12, 2017

select an RBAC version for kubefed it knows how to speak #50537

select an RBAC version for kubefed it knows how to speak #50537

Conversation

liggitt commented Aug 11, 2017 • edited Loading

nikhiljindal commented Aug 11, 2017

nikhiljindal Aug 11, 2017

Choose a reason for hiding this comment

liggitt Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

madhusudancs Aug 12, 2017

Choose a reason for hiding this comment

nikhiljindal Aug 12, 2017

Choose a reason for hiding this comment

liggitt Aug 12, 2017

Choose a reason for hiding this comment

nikhiljindal Aug 12, 2017

Choose a reason for hiding this comment

nikhiljindal commented Aug 11, 2017 • edited Loading

k8s-github-robot commented Aug 11, 2017

liggitt commented Aug 11, 2017 • edited Loading

k8s-github-robot commented Aug 12, 2017

k8s-ci-robot commented Aug 12, 2017 • edited Loading

liggitt commented Aug 12, 2017

k8s-github-robot commented Aug 12, 2017

k8s-cherrypick-bot commented Aug 12, 2017

liggitt commented Aug 11, 2017 •

edited

Loading

liggitt Aug 11, 2017 •

edited

Loading

nikhiljindal commented Aug 11, 2017 •

edited

Loading

liggitt commented Aug 11, 2017 •

edited

Loading

k8s-ci-robot commented Aug 12, 2017 •

edited

Loading