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

Correct APIGroup for RoleBindingBuilder Subjects #53239

Merged

Conversation

enj
Copy link
Member

@enj enj commented Sep 29, 2017

This change corrects RoleBindingBuilder to use the RBAC API group with users and groups as subjects (service accounts use the empty string since they are in the legacy core group). This is based on the defaulting in pkg/apis/rbac/v1/defaults.go#SetDefaults_Subject. This is required because the bootstrap RBAC data is built with these helpers and does not go through defaulting, whereas the data retrieved from the server has already gone through defaulting. This can lead to the reconciliation code incorrectly adding duplicate subjects because it believes that they are missing (since the API groups do not match).

Signed-off-by: Monis Khan [email protected]

Fixes an issue with RBAC reconciliation that could cause duplicated subjects in some bootstrapped rolebindings on each restart of the API server.

/assign @liggitt
/sig auth

Fixes #53296
Fixes openshift/origin/issues/16611

@k8s-ci-robot
Copy link
Contributor

@enj: GitHub didn't allow me to assign the following users: liggit.

Note that only kubernetes members can be assigned.

In response to this:

This change corrects RoleBindingBuilder to use the RBAC API group with users and groups as subjects (service accounts use the empty string since they are in the legacy core group). This is based on the defaulting in pkg/apis/rbac/v1/defaults.go#SetDefaults_Subject. This is required because the bootstrap RBAC data is built with these helpers and does not go through defaulting, whereas the data retrieved from the server has already gone through defaulting. This can lead to the reconciliation code incorrectly adding duplicate subjects because it believes that they are missing (since the API groups do not match).

Signed-off-by: Monis Khan [email protected]

/assign @LiGgit
/sig auth

Fixes openshift/origin/issues/16611

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.

@k8s-ci-robot k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 29, 2017
@k8s-github-robot k8s-github-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Sep 29, 2017
@enj
Copy link
Member Author

enj commented Sep 29, 2017

/assign @liggitt
/unassign @thockin

@k8s-ci-robot k8s-ci-robot assigned liggitt and unassigned thockin Sep 29, 2017
@k8s-github-robot k8s-github-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 29, 2017
@liggitt
Copy link
Member

liggitt commented Sep 29, 2017

needs a test to make sure we don't drift in the future:

package rbac_test

import (
        reflect "reflect"
        "testing"

        runtime "k8s.io/apimachinery/pkg/runtime"
        "k8s.io/apimachinery/pkg/util/diff"
        "k8s.io/kubernetes/pkg/api"
        "k8s.io/kubernetes/pkg/apis/rbac"
        _ "k8s.io/kubernetes/pkg/apis/rbac/install"
        "k8s.io/kubernetes/pkg/apis/rbac/v1"
)

func TestBindingRoundTrip(t *testing.T) {

        rb := rbac.NewRoleBinding("role", "ns").Groups("g").SAs("ns", "sa").Users("u").BindingOrDie()
        crb := rbac.NewClusterBinding("role").Groups("g").SAs("ns", "sa").Users("u").BindingOrDie()

        role := &rbac.Role{
                Rules: []rbac.PolicyRule{
                        rbac.NewRule("verb").Groups("").Resources("foo").RuleOrDie(),
                        rbac.NewRule("verb").URLs("/foo").RuleOrDie(),
                },
        }

        for _, internalObj := range []runtime.Object{&rb, &crb, role} {
                v1Obj, err := api.Scheme.ConvertToVersion(internalObj, v1.SchemeGroupVersion)
                if err != nil {
                        t.Errorf("err on %T: %v", internalObj, err)
                        continue
                }
                api.Scheme.Default(v1Obj)
                roundTrippedObj, err := api.Scheme.ConvertToVersion(v1Obj, rbac.SchemeGroupVersion)
                if err != nil {
                        t.Errorf("err on %T: %v", internalObj, err)
                        continue
                }
                if !reflect.DeepEqual(internalObj, roundTrippedObj) {
                        t.Errorf("err on %T: got difference:\n%s", internalObj, diff.ObjectDiff(internalObj, roundTrippedObj))
                        continue
                }
        }
}


@liggitt
Copy link
Member

liggitt commented Sep 29, 2017

lgtm otherwise. we'll want to pick to 1.8 and 1.7

@liggitt
Copy link
Member

liggitt commented Sep 29, 2017

needs a kube issue and release note

@enj enj force-pushed the enj/i/role_binding_api_group/16611 branch from e4dd418 to 256d258 Compare September 29, 2017 16:50
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 29, 2017
@enj enj force-pushed the enj/i/role_binding_api_group/16611 branch from 256d258 to 04cc6f4 Compare September 29, 2017 16:57
@ericchiang
Copy link
Contributor

Need to update the linting file.

W0929 17:25:21.876] Some packages in hack/.golint_failures are passing golint. Please remove them.
W0929 17:25:21.877] 
W0929 17:25:21.877]   pkg/apis/rbac

This change corrects RoleBindingBuilder to use the RBAC API group
with users and groups as subjects (service accounts use the empty
string since they are in the legacy core group).  This is based on
the defaulting in pkg/apis/rbac/v1/defaults.go#SetDefaults_Subject.
This is required because the bootstrap RBAC data is built with these
helpers and does not go through defaulting, whereas the data
retrieved from the server has already gone through defaulting.  This
can lead to the reconciliation code incorrectly adding duplicate
subjects because it believes that they are missing (since the API
groups do not match).

Signed-off-by: Monis Khan <[email protected]>
@enj enj force-pushed the enj/i/role_binding_api_group/16611 branch from 04cc6f4 to 5eb5b3e Compare September 30, 2017 16:16
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 30, 2017
@enj
Copy link
Member Author

enj commented Sep 30, 2017

@ericchiang @liggitt comments addressed.

@liggitt
Copy link
Member

liggitt commented Sep 30, 2017

/lgtm
Open cherry picks against 1.8 and 1.7, please

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

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: enj, liggitt
We suggest the following additional approver: lavalamp

Assign the PR to them by writing /assign @lavalamp in a comment when ready.

Associated issue: 53296

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

@liggitt liggitt added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed non-release-blocker labels Sep 30, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 51034, 53239). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 72d9774 into kubernetes:master Sep 30, 2017
k8s-github-robot pushed a commit that referenced this pull request Oct 2, 2017
…stream-release-1.7

Automatic merge from submit-queue.

Automated cherry pick of #53239

Cherry pick of #53239 on release-1.7.

#53239: Correct APIGroup for RoleBindingBuilder Subjects
k8s-github-robot pushed a commit that referenced this pull request Oct 4, 2017
…stream-release-1.8

Automatic merge from submit-queue.

Automated cherry pick of #53239

Cherry pick of #53239 on release-1.8.

#53239: Correct APIGroup for RoleBindingBuilder Subjects
@k8s-cherrypick-bot
Copy link

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

k8s-github-robot pushed a commit that referenced this pull request Oct 4, 2017
…-upstream-release-1.7

Automatic merge from submit-queue.

Automated cherry pick of #46542

Cherry pick of #46542  on release-1.7.

#53239: Ignore pods for quota marked for deletion whose node is unreachable
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. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants