Merge pull request #53239 from enj/enj/i/role_binding_api_group/16611

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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Correct APIGroup for RoleBindingBuilder Subjects

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>

```release-note
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
This commit is contained in:
Kubernetes Submit Queue 2017-09-30 12:14:15 -07:00 committed by GitHub
commit 72d97746df
4 changed files with 88 additions and 3 deletions

View File

@ -114,7 +114,6 @@ pkg/apis/policy
pkg/apis/policy/v1alpha1
pkg/apis/policy/v1beta1
pkg/apis/policy/validation
pkg/apis/rbac
pkg/apis/rbac/v1
pkg/apis/rbac/v1beta1
pkg/apis/rbac/validation

View File

@ -3,6 +3,7 @@ package(default_visibility = ["//visibility:public"])
load(
"@io_bazel_rules_go//go:def.bzl",
"go_library",
"go_test",
)
go_library(
@ -43,3 +44,16 @@ filegroup(
],
tags = ["automanaged"],
)
go_test(
name = "go_default_xtest",
srcs = ["helpers_test.go"],
deps = [
":go_default_library",
"//pkg/api:go_default_library",
"//pkg/apis/rbac/install:go_default_library",
"//pkg/apis/rbac/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/diff:go_default_library",
],
)

View File

@ -348,7 +348,7 @@ func NewRoleBindingForClusterRole(roleName, namespace string) *RoleBindingBuilde
// Groups adds the specified groups as the subjects of the RoleBinding.
func (r *RoleBindingBuilder) Groups(groups ...string) *RoleBindingBuilder {
for _, group := range groups {
r.RoleBinding.Subjects = append(r.RoleBinding.Subjects, Subject{Kind: GroupKind, Name: group})
r.RoleBinding.Subjects = append(r.RoleBinding.Subjects, Subject{Kind: GroupKind, APIGroup: GroupName, Name: group})
}
return r
}
@ -356,7 +356,7 @@ func (r *RoleBindingBuilder) Groups(groups ...string) *RoleBindingBuilder {
// Users adds the specified users as the subjects of the RoleBinding.
func (r *RoleBindingBuilder) Users(users ...string) *RoleBindingBuilder {
for _, user := range users {
r.RoleBinding.Subjects = append(r.RoleBinding.Subjects, Subject{Kind: UserKind, Name: user})
r.RoleBinding.Subjects = append(r.RoleBinding.Subjects, Subject{Kind: UserKind, APIGroup: GroupName, Name: user})
}
return r
}

View File

@ -0,0 +1,72 @@
/*
Copyright 2017 The Kubernetes Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
package rbac_test
import (
"reflect"
"testing"
"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/v1"
// install RBAC types
_ "k8s.io/kubernetes/pkg/apis/rbac/install"
)
// TestHelpersRoundTrip confirms that the rbac.New* helper functions produce RBAC objects that match objects
// that have gone through conversion and defaulting. This is required because these helper functions are
// used to create the bootstrap RBAC policy which is used during reconciliation. If they produced objects
// that did not match, reconciliation would incorrectly add duplicate data to the cluster's RBAC policy.
func TestHelpersRoundTrip(t *testing.T) {
rb := rbac.NewRoleBinding("role", "ns").Groups("g").SAs("ns", "sa").Users("u").BindingOrDie()
rbcr := rbac.NewRoleBindingForClusterRole("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("g").Resources("foo").RuleOrDie(),
rbac.NewRule("verb").URLs("/foo").RuleOrDie(),
},
}
clusterRole := &rbac.ClusterRole{
Rules: []rbac.PolicyRule{
rbac.NewRule("verb").Groups("g").Resources("foo").RuleOrDie(),
rbac.NewRule("verb").URLs("/foo").RuleOrDie(),
},
}
for _, internalObj := range []runtime.Object{&rb, &rbcr, &crb, role, clusterRole} {
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
}
}
}