Correct APIGroup for RoleBindingBuilder Subjects

[enj]

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 mkhan@redhat.com

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]

@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 mkhan@redhat.com

/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.

[enj]

/assign @liggitt
/unassign @thockin

[liggitt]

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]

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

[liggitt]

needs a kube issue and release note

[ericchiang]

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

[enj]

@ericchiang @liggitt comments addressed.

[liggitt]

/lgtm
Open cherry picks against 1.8 and 1.7, please

[k8s-github-robot]

[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:

• hack/OWNERS
• pkg/apis/OWNERS [liggitt]

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

[k8s-github-robot]

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-cherrypick-bot]

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.