Merge pull request #53298 from enj/automated-cherry-pick-of-#53239-upstream-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

Author: Kubernetes Submit Queue

hack/.golint_failures

@@ -125,7 +125,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

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",
+    ],
+)

pkg/apis/rbac/helpers.go

@@ -348,15 +348,15 @@ 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
 }
 
 // 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
 }

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
+		}
+	}
+}