From b51e09d5fe4372b2f0d399ff286afa9583fae623 Mon Sep 17 00:00:00 2001
From: Donny Xia <donnyxia@google.com>
Date: Wed, 23 Sep 2020 13:03:12 -0700
Subject: [PATCH] fix multiple match issue in rolebinding

---
 api/filters/nameref/nameref.go                |  16 ++-
 api/krusty/rolebindingacrossnamespace_test.go | 129 ++++++++++++++++++
 2 files changed, 141 insertions(+), 4 deletions(-)

diff --git a/api/filters/nameref/nameref.go b/api/filters/nameref/nameref.go
index dff9fe03a..f2f53b9d6 100644
--- a/api/filters/nameref/nameref.go
+++ b/api/filters/nameref/nameref.go
@@ -88,12 +88,20 @@ func (f Filter) setScalar(node *yaml.RNode) error {
 
 func filterReferralCandidates(
 	referrer *resource.Resource,
-	matches []*resource.Resource) []*resource.Resource {
+	matches []*resource.Resource,
+	target resid.Gvk,
+) []*resource.Resource {
 	var ret []*resource.Resource
 	for _, m := range matches {
-		if referrer.PrefixesSuffixesEquals(m) {
-			ret = append(ret, m)
+		// If target kind is not ServiceAccount, we shouldn't consider condidates which
+		// doesn't have same namespace.
+		if target.Kind != "ServiceAccount" && m.GetNamespace() != referrer.GetNamespace() {
+			continue
 		}
+		if !referrer.PrefixesSuffixesEquals(m) {
+			continue
+		}
+		ret = append(ret, m)
 	}
 	return ret
 }
@@ -118,7 +126,7 @@ func selectReferral(
 			// If there's more than one match,
 			// filter the matches by prefix and suffix
 			if len(matches) > 1 {
-				filteredMatches := filterReferralCandidates(referrer, matches)
+				filteredMatches := filterReferralCandidates(referrer, matches, target)
 				if len(filteredMatches) > 1 {
 					return "", "", fmt.Errorf(
 						"multiple matches for %s:\n  %v",
diff --git a/api/krusty/rolebindingacrossnamespace_test.go b/api/krusty/rolebindingacrossnamespace_test.go
index 08d1e6204..a8825c0f8 100644
--- a/api/krusty/rolebindingacrossnamespace_test.go
+++ b/api/krusty/rolebindingacrossnamespace_test.go
@@ -213,3 +213,132 @@ roleRef:
   name: my-role-ns2
 `)
 }
+
+// The ServiceAccount in subjects in role binding can be across namespace
+// but the roleRef is not. This test is used to cover such case.
+func TestRoleBindingWhenSubjectsAcrossNamespace(t *testing.T) {
+	th := kusttest_test.MakeEnhancedHarness(t)
+	defer th.Reset()
+	th.WriteK("/app", `
+resources:
+- ./ns1
+- ./ns2
+`)
+	th.WriteK("/app/ns1", `
+namespace: namespace-1
+resources:
+- role-ns1.yaml
+- rolebinding-ns1.yaml
+`)
+	th.WriteF("/app/ns1/role-ns1.yaml", `
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+  name: testRole
+rules:
+  - apiGroups: [""]
+    resources: ["pods"]
+    verbs: ["get"]
+`)
+	th.WriteF("/app/ns1/rolebinding-ns1.yaml", `
+apiVersion: rbac.authorization.k8s.io/v1
+kind: RoleBinding
+metadata:
+  name: testRoleBinding
+roleRef:
+  apiGroup: rbac.authorization.k8s.io
+  kind: Role
+  name: testRole
+subjects:
+  - kind: ServiceAccount
+    name: testAccount
+    namespace: namespace-2
+`)
+	th.WriteK("/app/ns2", `
+namespace: namespace-2
+resources:
+- role-ns2.yaml
+- rolebinding-ns2.yaml
+`)
+	th.WriteF("/app/ns2/role-ns2.yaml", `
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+  name: testRole
+rules:
+  - apiGroups: [""]
+    resources: ["pods"]
+    verbs: ["get"]
+`)
+	th.WriteF("/app/ns2/rolebinding-ns2.yaml", `
+apiVersion: rbac.authorization.k8s.io/v1
+kind: RoleBinding
+metadata:
+  name: testRoleBinding
+roleRef:
+  apiGroup: rbac.authorization.k8s.io
+  kind: Role
+  name: testRole
+subjects:
+  - kind: ServiceAccount
+    name: testAccount
+    namespace: namespace-1
+`)
+
+	m := th.Run("/app", th.MakeDefaultOptions())
+	th.AssertActualEqualsExpected(m, `
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+  name: testRole
+  namespace: namespace-1
+rules:
+- apiGroups:
+  - ""
+  resources:
+  - pods
+  verbs:
+  - get
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: RoleBinding
+metadata:
+  name: testRoleBinding
+  namespace: namespace-1
+roleRef:
+  apiGroup: rbac.authorization.k8s.io
+  kind: Role
+  name: testRole
+subjects:
+- kind: ServiceAccount
+  name: testAccount
+  namespace: namespace-2
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+  name: testRole
+  namespace: namespace-2
+rules:
+- apiGroups:
+  - ""
+  resources:
+  - pods
+  verbs:
+  - get
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: RoleBinding
+metadata:
+  name: testRoleBinding
+  namespace: namespace-2
+roleRef:
+  apiGroup: rbac.authorization.k8s.io
+  kind: Role
+  name: testRole
+subjects:
+- kind: ServiceAccount
+  name: testAccount
+  namespace: namespace-1
+`)
+}