Set namespace rolebinding behavior (#680)

* feat: Update RoleBinding subject namespaces

This commit adds support to update the namespace for
ServiceAccount resources which are referenced in the subjects
field of RoleBinding and ClusterRoleBinding resources.

Given a ServiceAccount with name sa-name is included in the
ResourceList, any RoleBinding or ClusterRoleBinding subject with
kind ServiceAccount and name sa-name will also have its namespace
set.

For the time being this functionality is being implemented in kpt
rather than the upstream kustomize library. If this behavior is
well received, it may be a future consideration to push this
functionality into kustomize.

* test: Add e2e test for set-namespace RoleBindings

This test covers the behavior for how set-namespace interacts
with the RoleBinding and ClusterRoleBinding resource types.

* test: e2e test set-namespace/ensure-name-substring

This test case covers the interaction between the
ensure-name-substring and set-namespace functions, particularly
when interacting with RoleBinding/ClusterRoleBinding resource
types.

Author: sdowell

functions/go/set-namespace/README.md

@@ -37,19 +37,35 @@ target the namespace. There are a few cases that worth pointing out:
 
 - If there is a `Namespace` resource, its `metadata.name` field will be updated.
 - If there's a `RoleBinding` or `ClusterRoleBinding` resource, the function will
-  update the namespace in the `ServiceAccount` if and only if the subject
-  element `name` is `default`. In the following example, the `set-namespace`
-  function will update `subjects.namespace` since the
-  corresponding `subjects.name` is `default`.
+  update the namespace in the `ServiceAccount` if one of the following are true:
+  1) the subject element `name` is `default`.
+  2) the subject element `name` matches the name of a `ServiceAccount` resource declared in the package.
+  
+In the following example, the `set-namespace` function will update:
+- `subjects[0].namespace` since `subjects[0].name` is `default`.
+- `subjects[1].namespace` since `subjects[1].name` matches a `ServiceAccount`
+  name declared in the package.
 
 ```yaml
+apiVersion: v1
+kind: ServiceAccount
+metadata:
+  name: service-account
+  namespace: original-namespace
+---
 kind: RoleBinding
 apiVersion: rbac.authorization.k8s.io/v1
 metadata:
   ...
 subjects:
   - kind: ServiceAccount
-    name: default # <======== using name default here
+    name: default # <================== name default is used
+    namespace: original-namespace # <== this will be updated
+  - kind: ServiceAccount
+    name: service-account # <========== name matches above ServiceAccount
+    namespace: original-namespace # <== this will be updated
+  - kind: ServiceAccount
+    name: other-service-account # <==== this will NOT be updated
     namespace: original-namespace
 roleRef:
   kind: Role

functions/go/set-namespace/namespace_transformer.go

@@ -9,11 +9,20 @@ import (
 
 	"sigs.k8s.io/kustomize/api/filters/namespace"
 	"sigs.k8s.io/kustomize/api/resmap"
+	"sigs.k8s.io/kustomize/api/resource"
 	"sigs.k8s.io/kustomize/api/types"
 	"sigs.k8s.io/kustomize/kyaml/filtersutil"
+	kyaml "sigs.k8s.io/kustomize/kyaml/yaml"
 	"sigs.k8s.io/yaml"
 )
 
+const (
+	subjectsField          = "subjects"
+	serviceAccountKind     = "ServiceAccount"
+	roleBindingKind        = "RoleBinding"
+	clusterRoleBindingKind = "ClusterRoleBinding"
+)
+
 // Change or set the namespace of non-cluster level resources.
 type plugin struct {
 	// Desired namespace.
@@ -48,6 +57,8 @@ func (p *plugin) Transform(m resmap.ResMap) error {
 	if len(p.Namespace) == 0 {
 		return nil
 	}
+	sal := serviceAccountLookup{}
+	sal.FromResMap(m)
 	for _, r := range m.Resources() {
 		if r.IsNilOrEmpty() {
 			// Don't mutate empty objects?
@@ -60,6 +71,67 @@ func (p *plugin) Transform(m resmap.ResMap) error {
 		if err != nil {
 			return err
 		}
+		if err = p.roleBindingHack(r, sal); err != nil {
+			return err
+		}
 	}
 	return nil
 }
+
+// roleBindingHack extends the behavior of the upstream kustomize filter
+// This extension adds the following behavior:
+// Given a ServiceAccount is present in the ResourceList, additionally set the
+// namespace for that ServiceAccount where it is referenced in the "subjects"
+// field of a RoleBinding or ClusterRoleBinding
+func (p *plugin) roleBindingHack(r *resource.Resource, sal serviceAccountLookup) error {
+	kind := r.GetKind()
+	if kind != roleBindingKind && kind != clusterRoleBindingKind {
+		return nil
+	}
+	subjects, err := r.Pipe(kyaml.Lookup(subjectsField))
+	if err != nil || kyaml.IsMissingOrNull(subjects) {
+		return err
+	}
+
+	err = subjects.VisitElements(func(o *kyaml.RNode) error {
+		subjectKind := o.GetKind()
+		if subjectKind != serviceAccountKind {
+			return nil
+		}
+		var nameRN *kyaml.RNode
+		nameRN, err = o.Pipe(kyaml.Lookup("name"))
+		if err != nil || kyaml.IsMissingOrNull(nameRN) {
+			return err
+		}
+		nameStr := kyaml.GetValue(nameRN)
+		if !sal.HasServiceAccount(nameStr) {
+			return nil
+		}
+		v := kyaml.NewScalarRNode(p.Namespace)
+		return o.PipeE(
+			kyaml.LookupCreate(kyaml.ScalarNode, "namespace"),
+			kyaml.FieldSetter{Value: v},
+		)
+	})
+	return err
+}
+
+// ServiceAccountLookup provides an API for tracking ServiceAccount resources
+type serviceAccountLookup struct {
+	serviceAccountMap map[string]bool
+}
+
+// FromResMap reads through a ResMap to populate ServiceAccountLookup
+func (sal *serviceAccountLookup) FromResMap(m resmap.ResMap) {
+	sal.serviceAccountMap = make(map[string]bool)
+	for _, r := range m.Resources() {
+		if r.GetKind() == serviceAccountKind {
+			sal.serviceAccountMap[r.GetName()] = true
+		}
+	}
+}
+
+// HasServiceAccount returns whether a ServiceAccount with the provided name is present
+func (sal *serviceAccountLookup) HasServiceAccount(name string) bool {
+	return sal.serviceAccountMap[name]
+}

functions/go/set-namespace/namespace_transformer_test.go

@@ -162,7 +162,7 @@ subjects:
   namespace: test
 - kind: ServiceAccount
   name: service-account
-  namespace: system
+  namespace: test
 - kind: ServiceAccount
   name: another
   namespace: random

tests/set-namespace/ensure-name-substring/.expected/diff.patch

@@ -0,0 +1,52 @@
+diff --git a/resources.yaml b/resources.yaml
+index 0315d44..3a16469 100644
+--- a/resources.yaml
++++ b/resources.yaml
+@@ -1,21 +1,21 @@
+ apiVersion: v1
+ kind: ServiceAccount
+ metadata:
+-  name: service-account
+-  namespace: system
++  name: dev-service-account
++  namespace: example-ns
+ ---
+ apiVersion: rbac.authorization.k8s.io/v1
+ kind: RoleBinding
+ metadata:
+-  name: manager-rolebinding
+-  namespace: system
++  name: dev-manager-rolebinding
++  namespace: example-ns
+ subjects:
+ - kind: ServiceAccount
+   name: default
+-  namespace: system
++  namespace: example-ns
+ - kind: ServiceAccount
+-  name: service-account
+-  namespace: system
++  name: dev-service-account
++  namespace: example-ns
+ - kind: ServiceAccount
+   name: another
+   namespace: random
+@@ -23,14 +23,14 @@ subjects:
+ apiVersion: rbac.authorization.k8s.io/v1
+ kind: ClusterRoleBinding
+ metadata:
+-  name: manager-cluster-rolebinding
++  name: dev-manager-cluster-rolebinding
+ subjects:
+ - kind: ServiceAccount
+   name: default
+-  namespace: system
++  namespace: example-ns
+ - kind: ServiceAccount
+-  name: service-account
+-  namespace: system
++  name: dev-service-account
++  namespace: example-ns
+ - kind: ServiceAccount
+   name: another
+   namespace: random

tests/set-namespace/ensure-name-substring/.krmignore

@@ -0,0 +1 @@
+.expected

tests/set-namespace/ensure-name-substring/Kptfile

@@ -0,0 +1,12 @@
+apiVersion: kpt.dev/v1
+kind: Kptfile
+metadata:
+  name: example
+pipeline:
+  mutators:
+    - image: gcr.io/kpt-fn/ensure-name-substring:unstable
+      configMap:
+        prepend: dev-
+    - image: gcr.io/kpt-fn/set-namespace:unstable
+      configMap:
+        namespace: example-ns

tests/set-namespace/ensure-name-substring/resources.yaml

@@ -0,0 +1,36 @@
+apiVersion: v1
+kind: ServiceAccount
+metadata:
+  name: service-account
+  namespace: system
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: RoleBinding
+metadata:
+  name: manager-rolebinding
+  namespace: system
+subjects:
+- kind: ServiceAccount
+  name: default
+  namespace: system
+- kind: ServiceAccount
+  name: service-account
+  namespace: system
+- kind: ServiceAccount
+  name: another
+  namespace: random
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRoleBinding
+metadata:
+  name: manager-cluster-rolebinding
+subjects:
+- kind: ServiceAccount
+  name: default
+  namespace: system
+- kind: ServiceAccount
+  name: service-account
+  namespace: system
+- kind: ServiceAccount
+  name: another
+  namespace: random

tests/set-namespace/rolebindings/.expected/diff.patch

@@ -0,0 +1,42 @@
+diff --git a/resources.yaml b/resources.yaml
+index 0315d44..78167d3 100644
+--- a/resources.yaml
++++ b/resources.yaml
+@@ -2,20 +2,20 @@ apiVersion: v1
+ kind: ServiceAccount
+ metadata:
+   name: service-account
+-  namespace: system
++  namespace: example-ns
+ ---
+ apiVersion: rbac.authorization.k8s.io/v1
+ kind: RoleBinding
+ metadata:
+   name: manager-rolebinding
+-  namespace: system
++  namespace: example-ns
+ subjects:
+ - kind: ServiceAccount
+   name: default
+-  namespace: system
++  namespace: example-ns
+ - kind: ServiceAccount
+   name: service-account
+-  namespace: system
++  namespace: example-ns
+ - kind: ServiceAccount
+   name: another
+   namespace: random
+@@ -27,10 +27,10 @@ metadata:
+ subjects:
+ - kind: ServiceAccount
+   name: default
+-  namespace: system
++  namespace: example-ns
+ - kind: ServiceAccount
+   name: service-account
+-  namespace: system
++  namespace: example-ns
+ - kind: ServiceAccount
+   name: another
+   namespace: random

tests/set-namespace/rolebindings/.krmignore

@@ -0,0 +1 @@
+.expected

tests/set-namespace/rolebindings/Kptfile

@@ -0,0 +1,9 @@
+apiVersion: kpt.dev/v1
+kind: Kptfile
+metadata:
+  name: example
+pipeline:
+  mutators:
+    - image: gcr.io/kpt-fn/set-namespace:unstable
+      configMap:
+        namespace: example-ns