Merge pull request #43436 from xilabao/add-set-rolebinding-command

Automatic merge from submit-queue

add set rolebinding/clusterrolebinding command

add command to set user/group/serviceaccount in rolebinding/clusterrolebinding /cc @liggitt @deads2k

docs/.generated_docs

@@ -89,6 +89,7 @@ docs/man/man1/kubectl-scale.1
 docs/man/man1/kubectl-set-image.1
 docs/man/man1/kubectl-set-resources.1
 docs/man/man1/kubectl-set-selector.1
+docs/man/man1/kubectl-set-subject.1
 docs/man/man1/kubectl-set.1
 docs/man/man1/kubectl-stop.1
 docs/man/man1/kubectl-taint.1
@@ -180,6 +181,7 @@ docs/user-guide/kubectl/kubectl_set.md
 docs/user-guide/kubectl/kubectl_set_image.md
 docs/user-guide/kubectl/kubectl_set_resources.md
 docs/user-guide/kubectl/kubectl_set_selector.md
+docs/user-guide/kubectl/kubectl_set_subject.md
 docs/user-guide/kubectl/kubectl_taint.md
 docs/user-guide/kubectl/kubectl_top.md
 docs/user-guide/kubectl/kubectl_top_node.md

docs/man/man1/kubectl-set-subject.1

@@ -0,0 +1,3 @@
+This file is autogenerated, but we've stopped checking such files into the
+repository to reduce the need for rebases. Please run hack/generate-docs.sh to
+populate this file.

docs/user-guide/kubectl/kubectl_set_subject.md

@@ -0,0 +1,3 @@
+This file is autogenerated, but we've stopped checking such files into the
+repository to reduce the need for rebases. Please run hack/generate-docs.sh to
+populate this file.

hack/make-rules/test-cmd-util.sh

@@ -3012,21 +3012,41 @@ runTests() {
     kube::test::get_object_assert clusterrole/resourcename-reader "{{range.rules}}{{range.apiGroups}}{{.}}:{{end}}{{end}}" ':'
     kube::test::get_object_assert clusterrole/resourcename-reader "{{range.rules}}{{range.resourceNames}}{{.}}:{{end}}{{end}}" 'foo:'
 
-    # test `kubectl create clusterrolebinding`
+    # test `kubectl create rolebinding/clusterrolebinding`
+    # test `kubectl set subject rolebinding/clusterrolebinding`
     kubectl create "${kube_flags[@]}" clusterrolebinding super-admin --clusterrole=admin --user=super-admin
     kube::test::get_object_assert clusterrolebinding/super-admin "{{range.subjects}}{{.name}}:{{end}}" 'super-admin:'
+    kubectl set subject "${kube_flags[@]}" clusterrolebinding super-admin --user=foo
+    kube::test::get_object_assert clusterrolebinding/super-admin "{{range.subjects}}{{.name}}:{{end}}" 'super-admin:foo:'
+
     kubectl create "${kube_flags[@]}" clusterrolebinding super-group --clusterrole=admin --group=the-group
     kube::test::get_object_assert clusterrolebinding/super-group "{{range.subjects}}{{.name}}:{{end}}" 'the-group:'
+    kubectl set subject "${kube_flags[@]}" clusterrolebinding super-group --group=foo
+    kube::test::get_object_assert clusterrolebinding/super-group "{{range.subjects}}{{.name}}:{{end}}" 'the-group:foo:'
+
     kubectl create "${kube_flags[@]}" clusterrolebinding super-sa --clusterrole=admin --serviceaccount=otherns:sa-name
     kube::test::get_object_assert clusterrolebinding/super-sa "{{range.subjects}}{{.namespace}}:{{end}}" 'otherns:'
     kube::test::get_object_assert clusterrolebinding/super-sa "{{range.subjects}}{{.name}}:{{end}}" 'sa-name:'
+    kubectl set subject "${kube_flags[@]}" clusterrolebinding super-sa --serviceaccount=otherfoo:foo
+    kube::test::get_object_assert clusterrolebinding/super-sa "{{range.subjects}}{{.namespace}}:{{end}}" 'otherns:otherfoo:'
+    kube::test::get_object_assert clusterrolebinding/super-sa "{{range.subjects}}{{.name}}:{{end}}" 'sa-name:foo:'
+
     kubectl create "${kube_flags[@]}" rolebinding admin --clusterrole=admin --user=default-admin -n default
     kube::test::get_object_assert rolebinding/admin "{{range.subjects}}{{.name}}:{{end}}" 'default-admin:'
+    kubectl set subject "${kube_flags[@]}" rolebinding admin --user=foo -n default
+    kube::test::get_object_assert rolebinding/admin "{{range.subjects}}{{.name}}:{{end}}" 'default-admin:foo:'
+
     kubectl create "${kube_flags[@]}" rolebinding localrole --role=localrole --group=the-group -n default
     kube::test::get_object_assert rolebinding/localrole "{{range.subjects}}{{.name}}:{{end}}" 'the-group:'
+    kubectl set subject "${kube_flags[@]}" rolebinding localrole --group=foo -n default
+    kube::test::get_object_assert rolebinding/localrole "{{range.subjects}}{{.name}}:{{end}}" 'the-group:foo:'
+
     kubectl create "${kube_flags[@]}" rolebinding sarole --role=localrole --serviceaccount=otherns:sa-name -n default
     kube::test::get_object_assert rolebinding/sarole "{{range.subjects}}{{.namespace}}:{{end}}" 'otherns:'
     kube::test::get_object_assert rolebinding/sarole "{{range.subjects}}{{.name}}:{{end}}" 'sa-name:'
+    kubectl set subject "${kube_flags[@]}" rolebinding sarole --serviceaccount=otherfoo:foo -n default
+    kube::test::get_object_assert rolebinding/sarole "{{range.subjects}}{{.namespace}}:{{end}}" 'otherns:otherfoo:'
+    kube::test::get_object_assert rolebinding/sarole "{{range.subjects}}{{.name}}:{{end}}" 'sa-name:foo:'
   fi
 
   ########

pkg/kubectl/cmd/set/BUILD

@@ -16,10 +16,12 @@ go_library(
         "set_image.go",
         "set_resources.go",
         "set_selector.go",
+        "set_subject.go",
     ],
     tags = ["automanaged"],
     deps = [
         "//pkg/api:go_default_library",
+        "//pkg/apis/rbac:go_default_library",
         "//pkg/kubectl:go_default_library",
         "//pkg/kubectl/cmd/templates:go_default_library",
         "//pkg/kubectl/cmd/util:go_default_library",
@@ -32,6 +34,7 @@ go_library(
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/errors:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/strategicpatch:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/validation:go_default_library",
     ],
@@ -42,6 +45,7 @@ go_test(
     srcs = [
         "set_image_test.go",
         "set_selector_test.go",
+        "set_subject_test.go",
         "set_test.go",
     ],
     data = [
@@ -53,6 +57,7 @@ go_test(
         "//pkg/api:go_default_library",
         "//pkg/apis/batch:go_default_library",
         "//pkg/apis/extensions:go_default_library",
+        "//pkg/apis/rbac:go_default_library",
         "//pkg/kubectl/cmd/testing:go_default_library",
         "//pkg/kubectl/cmd/util:go_default_library",
         "//pkg/kubectl/resource:go_default_library",

pkg/kubectl/cmd/set/set.go

@@ -44,6 +44,7 @@ func NewCmdSet(f cmdutil.Factory, out, err io.Writer) *cobra.Command {
 	cmd.AddCommand(NewCmdImage(f, out, err))
 	cmd.AddCommand(NewCmdResources(f, out, err))
 	cmd.AddCommand(NewCmdSelector(f, out))
+	cmd.AddCommand(NewCmdSubject(f, out, err))
 
 	return cmd
 }

pkg/kubectl/cmd/set/set_subject.go

@@ -0,0 +1,278 @@
+/*
+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 set
+
+import (
+	"fmt"
+	"io"
+	"strings"
+
+	"github.com/spf13/cobra"
+
+	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/kubernetes/pkg/apis/rbac"
+	"k8s.io/kubernetes/pkg/kubectl/cmd/templates"
+	cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
+	"k8s.io/kubernetes/pkg/kubectl/resource"
+	"k8s.io/kubernetes/pkg/util/i18n"
+)
+
+var (
+	subject_long = templates.LongDesc(`
+	Update User, Group or ServiceAccount in a RoleBinding/ClusterRoleBinding.`)
+
+	subject_example = templates.Examples(`
+	# Update a ClusterRoleBinding for serviceaccount1
+	kubectl set subject clusterrolebinding admin --serviceaccount=namespace:serviceaccount1
+
+	# Update a RoleBinding for user1, user2, and group1
+	kubectl set subject rolebinding admin --user=user1 --user=user2 --group=group1
+
+	# Print the result (in yaml format) of updating rolebinding subjects from a local, without hitting the server
+	kubectl create rolebinding admin --role=admin --user=admin -o yaml --dry-run | kubectl set subject --local -f - --user=foo -o yaml`)
+)
+
+type updateSubjects func(existings []rbac.Subject, targets []rbac.Subject) (bool, []rbac.Subject)
+
+// SubjectOptions is the start of the data required to perform the operation. As new fields are added, add them here instead of
+// referencing the cmd.Flags
+type SubjectOptions struct {
+	resource.FilenameOptions
+
+	Mapper            meta.RESTMapper
+	Typer             runtime.ObjectTyper
+	Infos             []*resource.Info
+	Encoder           runtime.Encoder
+	Out               io.Writer
+	Err               io.Writer
+	Selector          string
+	ContainerSelector string
+	ShortOutput       bool
+	All               bool
+	DryRun            bool
+	Local             bool
+
+	Users           []string
+	Groups          []string
+	ServiceAccounts []string
+
+	PrintObject func(mapper meta.RESTMapper, obj runtime.Object, out io.Writer) error
+}
+
+func NewCmdSubject(f cmdutil.Factory, out io.Writer, errOut io.Writer) *cobra.Command {
+	options := &SubjectOptions{
+		Out: out,
+		Err: errOut,
+	}
+
+	cmd := &cobra.Command{
+		Use:     "subject (-f FILENAME | TYPE NAME) [--user=username] [--group=groupname] [--serviceaccount=namespace:serviceaccountname] [--dry-run]",
+		Short:   i18n.T("Update User, Group or ServiceAccount in a RoleBinding/ClusterRoleBinding"),
+		Long:    subject_long,
+		Example: subject_example,
+		Run: func(cmd *cobra.Command, args []string) {
+			cmdutil.CheckErr(options.Complete(f, cmd, args))
+			cmdutil.CheckErr(options.Validate())
+			cmdutil.CheckErr(options.Run(f, addSubjects))
+		},
+	}
+
+	cmdutil.AddPrinterFlags(cmd)
+	usage := "the resource to update the subjects"
+	cmdutil.AddFilenameOptionFlags(cmd, &options.FilenameOptions, usage)
+	cmd.Flags().BoolVar(&options.All, "all", false, "select all resources in the namespace of the specified resource types")
+	cmd.Flags().StringVarP(&options.Selector, "selector", "l", "", "Selector (label query) to filter on, supports '=', '==', and '!='.")
+	cmd.Flags().BoolVar(&options.Local, "local", false, "If true, set resources will NOT contact api-server but run locally.")
+	cmdutil.AddDryRunFlag(cmd)
+	cmd.Flags().StringArrayVar(&options.Users, "user", []string{}, "usernames to bind to the role")
+	cmd.Flags().StringArrayVar(&options.Groups, "group", []string{}, "groups to bind to the role")
+	cmd.Flags().StringArrayVar(&options.ServiceAccounts, "serviceaccount", []string{}, "service accounts to bind to the role")
+	return cmd
+}
+
+func (o *SubjectOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []string) error {
+	o.Local = cmdutil.GetFlagBool(cmd, "local")
+	o.Mapper, o.Typer = f.Object()
+	o.Encoder = f.JSONEncoder()
+	o.ShortOutput = cmdutil.GetFlagString(cmd, "output") == "name"
+	o.DryRun = cmdutil.GetDryRunFlag(cmd)
+	o.PrintObject = func(mapper meta.RESTMapper, obj runtime.Object, out io.Writer) error {
+		return f.PrintObject(cmd, mapper, obj, out)
+	}
+
+	cmdNamespace, enforceNamespace, err := f.DefaultNamespace()
+	if err != nil {
+		return err
+	}
+
+	builder := resource.NewBuilder(o.Mapper, f.CategoryExpander(), o.Typer, resource.ClientMapperFunc(f.ClientForMapping), f.Decoder(true)).
+		ContinueOnError().
+		NamespaceParam(cmdNamespace).DefaultNamespace().
+		FilenameParam(enforceNamespace, &o.FilenameOptions).
+		Flatten()
+	if !o.Local {
+		builder = builder.
+			SelectorParam(o.Selector).
+			ResourceTypeOrNameArgs(o.All, args...).
+			Latest()
+	}
+	o.Infos, err = builder.Do().Infos()
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func (o *SubjectOptions) Validate() error {
+	if len(o.Users) == 0 && len(o.Groups) == 0 && len(o.ServiceAccounts) == 0 {
+		return fmt.Errorf("you must specify at least one value of user, group or serviceaccount")
+	}
+
+	for _, sa := range o.ServiceAccounts {
+		tokens := strings.Split(sa, ":")
+		if len(tokens) != 2 || tokens[1] == "" {
+			return fmt.Errorf("serviceaccount must be <namespace>:<name>")
+		}
+
+		for _, info := range o.Infos {
+			_, ok := info.Object.(*rbac.ClusterRoleBinding)
+			if ok && tokens[0] == "" {
+				return fmt.Errorf("serviceaccount must be <namespace>:<name>, namespace must be specified")
+			}
+		}
+	}
+
+	return nil
+}
+
+func (o *SubjectOptions) Run(f cmdutil.Factory, fn updateSubjects) error {
+	var err error
+	patches := CalculatePatches(o.Infos, o.Encoder, func(info *resource.Info) ([]byte, error) {
+		subjects := []rbac.Subject{}
+		for _, user := range sets.NewString(o.Users...).List() {
+			subject := rbac.Subject{
+				Kind:     rbac.UserKind,
+				APIGroup: rbac.GroupName,
+				Name:     user,
+			}
+			subjects = append(subjects, subject)
+		}
+		for _, group := range sets.NewString(o.Groups...).List() {
+			subject := rbac.Subject{
+				Kind:     rbac.GroupKind,
+				APIGroup: rbac.GroupName,
+				Name:     group,
+			}
+			subjects = append(subjects, subject)
+		}
+		for _, sa := range sets.NewString(o.ServiceAccounts...).List() {
+			tokens := strings.Split(sa, ":")
+			namespace := tokens[0]
+			name := tokens[1]
+			if len(namespace) == 0 {
+				namespace, _, err = f.DefaultNamespace()
+				if err != nil {
+					return nil, err
+				}
+			}
+			subject := rbac.Subject{
+				Kind:      rbac.ServiceAccountKind,
+				Namespace: namespace,
+				Name:      name,
+			}
+			subjects = append(subjects, subject)
+		}
+
+		transformed, err := updateSubjectForObject(info.Object, subjects, fn)
+		if transformed && err == nil {
+			return runtime.Encode(o.Encoder, info.Object)
+		}
+		return nil, err
+	})
+
+	allErrs := []error{}
+	for _, patch := range patches {
+		info := patch.Info
+		if patch.Err != nil {
+			allErrs = append(allErrs, fmt.Errorf("error: %s/%s %v\n", info.Mapping.Resource, info.Name, patch.Err))
+			continue
+		}
+
+		//no changes
+		if string(patch.Patch) == "{}" || len(patch.Patch) == 0 {
+			allErrs = append(allErrs, fmt.Errorf("info: %s %q was not changed\n", info.Mapping.Resource, info.Name))
+			continue
+		}
+
+		if o.Local || o.DryRun {
+			fmt.Fprintln(o.Out, "running in local/dry-run mode...")
+			return o.PrintObject(o.Mapper, info.Object, o.Out)
+		}
+
+		obj, err := resource.NewHelper(info.Client, info.Mapping).Patch(info.Namespace, info.Name, types.StrategicMergePatchType, patch.Patch)
+		if err != nil {
+			allErrs = append(allErrs, fmt.Errorf("failed to patch subjects to rolebinding: %v\n", err))
+			continue
+		}
+		info.Refresh(obj, true)
+
+		cmdutil.PrintSuccess(o.Mapper, o.ShortOutput, o.Out, info.Mapping.Resource, info.Name, false, "subjects updated")
+	}
+	return utilerrors.NewAggregate(allErrs)
+}
+
+//Note: the obj mutates in the function
+func updateSubjectForObject(obj runtime.Object, subjects []rbac.Subject, fn updateSubjects) (bool, error) {
+	switch t := obj.(type) {
+	case *rbac.RoleBinding:
+		transformed, result := fn(t.Subjects, subjects)
+		t.Subjects = result
+		return transformed, nil
+	case *rbac.ClusterRoleBinding:
+		transformed, result := fn(t.Subjects, subjects)
+		t.Subjects = result
+		return transformed, nil
+	default:
+		return false, fmt.Errorf("setting subjects is only supported for RoleBinding/ClusterRoleBinding")
+	}
+}
+
+func addSubjects(existings []rbac.Subject, targets []rbac.Subject) (bool, []rbac.Subject) {
+	transformed := false
+	updated := existings
+	for _, item := range targets {
+		if !contain(existings, item) {
+			updated = append(updated, item)
+			transformed = true
+		}
+	}
+	return transformed, updated
+}
+
+func contain(slice []rbac.Subject, item rbac.Subject) bool {
+	for _, v := range slice {
+		if v == item {
+			return true
+		}
+	}
+	return false
+}

pkg/kubectl/cmd/set/set_subject_test.go

@@ -0,0 +1,427 @@
+/*
+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 set
+
+import (
+	"reflect"
+	"testing"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/kubernetes/pkg/apis/rbac"
+	cmdtesting "k8s.io/kubernetes/pkg/kubectl/cmd/testing"
+	"k8s.io/kubernetes/pkg/kubectl/resource"
+)
+
+func TestValidate(t *testing.T) {
+	f, tf, _, _ := cmdtesting.NewAPIFactory()
+	tf.Namespace = "test"
+
+	tests := map[string]struct {
+		options   *SubjectOptions
+		expectErr bool
+	}{
+		"test-missing-subjects": {
+			options: &SubjectOptions{
+				Users:           []string{},
+				Groups:          []string{},
+				ServiceAccounts: []string{},
+			},
+			expectErr: true,
+		},
+		"test-invalid-serviceaccounts": {
+			options: &SubjectOptions{
+				Users:           []string{},
+				Groups:          []string{},
+				ServiceAccounts: []string{"foo"},
+			},
+			expectErr: true,
+		},
+		"test-missing-serviceaccounts-name": {
+			options: &SubjectOptions{
+				Users:           []string{},
+				Groups:          []string{},
+				ServiceAccounts: []string{"foo:"},
+			},
+			expectErr: true,
+		},
+		"test-missing-serviceaccounts-namespace": {
+			options: &SubjectOptions{
+				Infos: []*resource.Info{
+					{
+						Object: &rbac.ClusterRoleBinding{
+							ObjectMeta: metav1.ObjectMeta{
+								Name: "clusterrolebinding",
+							},
+							RoleRef: rbac.RoleRef{
+								APIGroup: "rbac.authorization.k8s.io",
+								Kind:     "ClusterRole",
+								Name:     "role",
+							},
+						},
+					},
+				},
+				Users:           []string{},
+				Groups:          []string{},
+				ServiceAccounts: []string{":foo"},
+			},
+			expectErr: true,
+		},
+		"test-valid-case": {
+			options: &SubjectOptions{
+				Infos: []*resource.Info{
+					{
+						Object: &rbac.RoleBinding{
+							ObjectMeta: metav1.ObjectMeta{
+								Name:      "rolebinding",
+								Namespace: "one",
+							},
+							RoleRef: rbac.RoleRef{
+								APIGroup: "rbac.authorization.k8s.io",
+								Kind:     "ClusterRole",
+								Name:     "role",
+							},
+						},
+					},
+				},
+				Users:           []string{"foo"},
+				Groups:          []string{"foo"},
+				ServiceAccounts: []string{"ns:foo"},
+			},
+			expectErr: false,
+		},
+	}
+
+	for name, test := range tests {
+		test.options.Mapper, _ = f.Object()
+		err := test.options.Validate()
+		if test.expectErr && err != nil {
+			continue
+		}
+		if !test.expectErr && err != nil {
+			t.Errorf("%s: unexpected error: %v", name, err)
+		}
+	}
+}
+
+func TestUpdateSubjectForObject(t *testing.T) {
+	tests := []struct {
+		Name     string
+		obj      runtime.Object
+		subjects []rbac.Subject
+		expected []rbac.Subject
+		wantErr  bool
+	}{
+		{
+			Name: "invalid object type",
+			obj: &rbac.Role{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "role",
+					Namespace: "one",
+				},
+			},
+			wantErr: true,
+		},
+		{
+			Name: "add resource with users in rolebinding",
+			obj: &rbac.RoleBinding{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "rolebinding",
+					Namespace: "one",
+				},
+				Subjects: []rbac.Subject{
+					{
+						APIGroup: "rbac.authorization.k8s.io",
+						Kind:     "User",
+						Name:     "a",
+					},
+				},
+			},
+			subjects: []rbac.Subject{
+				{
+					APIGroup: "rbac.authorization.k8s.io",
+					Kind:     "User",
+					Name:     "a",
+				},
+				{
+					APIGroup: "rbac.authorization.k8s.io",
+					Kind:     "User",
+					Name:     "b",
+				},
+			},
+			expected: []rbac.Subject{
+				{
+					APIGroup: "rbac.authorization.k8s.io",
+					Kind:     "User",
+					Name:     "a",
+				},
+				{
+					APIGroup: "rbac.authorization.k8s.io",
+					Kind:     "User",
+					Name:     "b",
+				},
+			},
+			wantErr: false,
+		},
+		{
+			Name: "add resource with groups in rolebinding",
+			obj: &rbac.RoleBinding{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "rolebinding",
+					Namespace: "one",
+				},
+				Subjects: []rbac.Subject{
+					{
+						APIGroup: "rbac.authorization.k8s.io",
+						Kind:     "Group",
+						Name:     "a",
+					},
+				},
+			},
+			subjects: []rbac.Subject{
+				{
+					APIGroup: "rbac.authorization.k8s.io",
+					Kind:     "Group",
+					Name:     "a",
+				},
+				{
+					APIGroup: "rbac.authorization.k8s.io",
+					Kind:     "Group",
+					Name:     "b",
+				},
+			},
+			expected: []rbac.Subject{
+				{
+					APIGroup: "rbac.authorization.k8s.io",
+					Kind:     "Group",
+					Name:     "a",
+				},
+				{
+					APIGroup: "rbac.authorization.k8s.io",
+					Kind:     "Group",
+					Name:     "b",
+				},
+			},
+			wantErr: false,
+		},
+		{
+			Name: "add resource with serviceaccounts in rolebinding",
+			obj: &rbac.RoleBinding{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "rolebinding",
+					Namespace: "one",
+				},
+				Subjects: []rbac.Subject{
+					{
+						Kind:      "ServiceAccount",
+						Namespace: "one",
+						Name:      "a",
+					},
+				},
+			},
+			subjects: []rbac.Subject{
+				{
+					Kind:      "ServiceAccount",
+					Namespace: "one",
+					Name:      "a",
+				},
+				{
+					Kind:      "ServiceAccount",
+					Namespace: "one",
+					Name:      "b",
+				},
+			},
+			expected: []rbac.Subject{
+				{
+					Kind:      "ServiceAccount",
+					Namespace: "one",
+					Name:      "a",
+				},
+				{
+					Kind:      "ServiceAccount",
+					Namespace: "one",
+					Name:      "b",
+				},
+			},
+			wantErr: false,
+		},
+		{
+			Name: "add resource with serviceaccounts in clusterrolebinding",
+			obj: &rbac.ClusterRoleBinding{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "clusterrolebinding",
+				},
+				Subjects: []rbac.Subject{
+					{
+						APIGroup: "rbac.authorization.k8s.io",
+						Kind:     "User",
+						Name:     "a",
+					},
+					{
+						APIGroup: "rbac.authorization.k8s.io",
+						Kind:     "Group",
+						Name:     "a",
+					},
+				},
+			},
+			subjects: []rbac.Subject{
+				{
+					Kind:      "ServiceAccount",
+					Namespace: "one",
+					Name:      "a",
+				},
+			},
+			expected: []rbac.Subject{
+				{
+					APIGroup: "rbac.authorization.k8s.io",
+					Kind:     "User",
+					Name:     "a",
+				},
+				{
+					APIGroup: "rbac.authorization.k8s.io",
+					Kind:     "Group",
+					Name:     "a",
+				},
+				{
+					Kind:      "ServiceAccount",
+					Namespace: "one",
+					Name:      "a",
+				},
+			},
+			wantErr: false,
+		},
+	}
+	for _, tt := range tests {
+		if _, err := updateSubjectForObject(tt.obj, tt.subjects, addSubjects); (err != nil) != tt.wantErr {
+			t.Errorf("%q. updateSubjectForObject() error = %v, wantErr %v", tt.Name, err, tt.wantErr)
+		}
+
+		want := tt.expected
+		var got []rbac.Subject
+		switch t := tt.obj.(type) {
+		case *rbac.RoleBinding:
+			got = t.Subjects
+		case *rbac.ClusterRoleBinding:
+			got = t.Subjects
+		}
+		if !reflect.DeepEqual(got, want) {
+			t.Errorf("%q. updateSubjectForObject() failed", tt.Name)
+			t.Errorf("Got: %v", got)
+			t.Errorf("Want: %v", want)
+		}
+	}
+}
+
+func TestAddSubject(t *testing.T) {
+	tests := []struct {
+		Name       string
+		existing   []rbac.Subject
+		subjects   []rbac.Subject
+		expected   []rbac.Subject
+		wantChange bool
+	}{
+		{
+			Name: "add resource with users",
+			existing: []rbac.Subject{
+				{
+					APIGroup: "rbac.authorization.k8s.io",
+					Kind:     "User",
+					Name:     "a",
+				},
+				{
+					APIGroup: "rbac.authorization.k8s.io",
+					Kind:     "User",
+					Name:     "b",
+				},
+			},
+			subjects: []rbac.Subject{
+				{
+					APIGroup: "rbac.authorization.k8s.io",
+					Kind:     "User",
+					Name:     "a",
+				},
+			},
+			expected: []rbac.Subject{
+				{
+					APIGroup: "rbac.authorization.k8s.io",
+					Kind:     "User",
+					Name:     "a",
+				},
+				{
+					APIGroup: "rbac.authorization.k8s.io",
+					Kind:     "User",
+					Name:     "b",
+				},
+			},
+			wantChange: false,
+		},
+		{
+			Name: "add resource with serviceaccounts",
+			existing: []rbac.Subject{
+				{
+					Kind:      "ServiceAccount",
+					Namespace: "one",
+					Name:      "a",
+				},
+				{
+					Kind:      "ServiceAccount",
+					Namespace: "one",
+					Name:      "b",
+				},
+			},
+			subjects: []rbac.Subject{
+				{
+					Kind:      "ServiceAccount",
+					Namespace: "two",
+					Name:      "a",
+				},
+			},
+			expected: []rbac.Subject{
+				{
+					Kind:      "ServiceAccount",
+					Namespace: "one",
+					Name:      "a",
+				},
+				{
+					Kind:      "ServiceAccount",
+					Namespace: "one",
+					Name:      "b",
+				},
+				{
+					Kind:      "ServiceAccount",
+					Namespace: "two",
+					Name:      "a",
+				},
+			},
+			wantChange: true,
+		},
+	}
+	for _, tt := range tests {
+		changed := false
+		got := []rbac.Subject{}
+		if changed, got = addSubjects(tt.existing, tt.subjects); (changed != false) != tt.wantChange {
+			t.Errorf("%q. addSubjects() changed = %v, wantChange = %v", tt.Name, changed, tt.wantChange)
+		}
+
+		want := tt.expected
+		if !reflect.DeepEqual(got, want) {
+			t.Errorf("%q. addSubjects() failed", tt.Name)
+			t.Errorf("Got: %v", got)
+			t.Errorf("Want: %v", want)
+		}
+	}
+}