From 5eb5b3e40231223738adfd22918a72a516df98b9 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Thu, 28 Sep 2017 21:49:52 -0400 Subject: [PATCH] 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 --- hack/.golint_failures | 1 - pkg/apis/rbac/BUILD | 14 +++++++ pkg/apis/rbac/helpers.go | 4 +- pkg/apis/rbac/helpers_test.go | 72 +++++++++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+), 3 deletions(-) create mode 100644 pkg/apis/rbac/helpers_test.go diff --git a/hack/.golint_failures b/hack/.golint_failures index 92a67928375..4b70f200a79 100644 --- a/hack/.golint_failures +++ b/hack/.golint_failures @@ -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 diff --git a/pkg/apis/rbac/BUILD b/pkg/apis/rbac/BUILD index ce8db5f6b5f..f875a668704 100644 --- a/pkg/apis/rbac/BUILD +++ b/pkg/apis/rbac/BUILD @@ -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", + ], +) diff --git a/pkg/apis/rbac/helpers.go b/pkg/apis/rbac/helpers.go index 549aedc98fc..efc4c61c569 100644 --- a/pkg/apis/rbac/helpers.go +++ b/pkg/apis/rbac/helpers.go @@ -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 } diff --git a/pkg/apis/rbac/helpers_test.go b/pkg/apis/rbac/helpers_test.go new file mode 100644 index 00000000000..de9f24a090a --- /dev/null +++ b/pkg/apis/rbac/helpers_test.go @@ -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 + } + } +}