From b28f1e55b7010c1e8754f48a43e85b03afd34059 Mon Sep 17 00:00:00 2001 From: m-Bilal Date: Sat, 27 Nov 2021 19:53:49 +0530 Subject: [PATCH 1/3] fixes 4240; added null check on namespace when resource is a RoleBinding --- api/resmap/reswrangler.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/api/resmap/reswrangler.go b/api/resmap/reswrangler.go index 31bfe1fee..5a462a6db 100644 --- a/api/resmap/reswrangler.go +++ b/api/resmap/reswrangler.go @@ -436,7 +436,9 @@ func getNamespacesForRoleBinding(r *resource.Resource) map[string]bool { if ns, ok1 := subject["namespace"]; ok1 { if kind, ok2 := subject["kind"]; ok2 { if kind.(string) == "ServiceAccount" { - result[ns.(string)] = true + if n, ok3 := ns.(string); ok3 { + result[n] = true + } } } } From ff7b2f20d5be3839319f3d6e23503ffabc2b968e Mon Sep 17 00:00:00 2001 From: m-Bilal Date: Sat, 1 Jan 2022 21:52:37 +0530 Subject: [PATCH 2/3] Throwing error instead of silently ignoring invalid input --- .../accumulator/namereferencetransformer.go | 5 ++++- api/resmap/resmap.go | 2 +- api/resmap/reswrangler.go | 21 ++++++++++++------- api/resmap/reswrangler_test.go | 5 ++++- 4 files changed, 22 insertions(+), 11 deletions(-) diff --git a/api/internal/accumulator/namereferencetransformer.go b/api/internal/accumulator/namereferencetransformer.go index 1ed75e9df..e039db4b1 100644 --- a/api/internal/accumulator/namereferencetransformer.go +++ b/api/internal/accumulator/namereferencetransformer.go @@ -52,7 +52,10 @@ func (t *nameReferenceTransformer) Transform(m resmap.ResMap) error { fMap := t.determineFilters(m.Resources()) debug(fMap) for r, fList := range fMap { - c := m.SubsetThatCouldBeReferencedByResource(r) + c, err := m.SubsetThatCouldBeReferencedByResource(r) + if err != nil { + return err + } for _, f := range fList { f.Referrer = r f.ReferralCandidates = c diff --git a/api/resmap/resmap.go b/api/resmap/resmap.go index 960441b79..a033223b1 100644 --- a/api/resmap/resmap.go +++ b/api/resmap/resmap.go @@ -212,7 +212,7 @@ type ResMap interface { // This is a filter; it excludes things that cannot be // referenced by the resource, e.g. objects in other // namespaces. Cluster wide objects are never excluded. - SubsetThatCouldBeReferencedByResource(*resource.Resource) ResMap + SubsetThatCouldBeReferencedByResource(*resource.Resource) (ResMap, error) // DeAnchor replaces YAML aliases with structured data copied from anchors. // This cannot be undone; if desired, call DeepCopy first. diff --git a/api/resmap/reswrangler.go b/api/resmap/reswrangler.go index 5a462a6db..93d82e64d 100644 --- a/api/resmap/reswrangler.go +++ b/api/resmap/reswrangler.go @@ -389,14 +389,17 @@ func (m *resWrangler) makeCopy(copier resCopier) ResMap { // SubsetThatCouldBeReferencedByResource implements ResMap. func (m *resWrangler) SubsetThatCouldBeReferencedByResource( - referrer *resource.Resource) ResMap { + referrer *resource.Resource) (ResMap, error) { referrerId := referrer.CurId() if referrerId.IsClusterScoped() { // A cluster scoped resource can refer to anything. - return m + return m, nil } result := newOne() - roleBindingNamespaces := getNamespacesForRoleBinding(referrer) + roleBindingNamespaces, err := getNamespacesForRoleBinding(referrer) + if err != nil { + return nil, err + } for _, possibleTarget := range m.rList { id := possibleTarget.CurId() if id.IsClusterScoped() { @@ -416,20 +419,20 @@ func (m *resWrangler) SubsetThatCouldBeReferencedByResource( result.append(possibleTarget) } } - return result + return result, nil } // getNamespacesForRoleBinding returns referenced ServiceAccount namespaces // if the resource is a RoleBinding -func getNamespacesForRoleBinding(r *resource.Resource) map[string]bool { +func getNamespacesForRoleBinding(r *resource.Resource) (map[string]bool, error) { result := make(map[string]bool) if r.GetKind() != "RoleBinding" { - return result + return result, nil } //nolint staticcheck subjects, err := r.GetSlice("subjects") if err != nil || subjects == nil { - return result + return result, nil } for _, s := range subjects { subject := s.(map[string]interface{}) @@ -438,12 +441,14 @@ func getNamespacesForRoleBinding(r *resource.Resource) map[string]bool { if kind.(string) == "ServiceAccount" { if n, ok3 := ns.(string); ok3 { result[n] = true + } else { + return nil, errors.New("Invalid Input: namespace is blank") } } } } } - return result + return result, nil } // AppendAll implements ResMap. diff --git a/api/resmap/reswrangler_test.go b/api/resmap/reswrangler_test.go index a47dddab7..bb3a0fbca 100644 --- a/api/resmap/reswrangler_test.go +++ b/api/resmap/reswrangler_test.go @@ -566,7 +566,10 @@ func TestSubsetThatCouldBeReferencedByResource(t *testing.T) { for name, test := range tests { test := test t.Run(name, func(t *testing.T) { - got := m.SubsetThatCouldBeReferencedByResource(test.filter) + got, err1 := m.SubsetThatCouldBeReferencedByResource(test.filter) + if err1 != nil { + t.Fatalf("Expected error %v: ", err1) + } err := test.expected.ErrorIfNotEqualLists(got) if err != nil { test.expected.Debug("expected") From 7674c220b1f8de920b0b2db6c156d1d31039d8d0 Mon Sep 17 00:00:00 2001 From: m-Bilal Date: Sun, 9 Jan 2022 19:05:49 +0530 Subject: [PATCH 3/3] Improved error message and test cases for 4240 --- api/krusty/blankvalues_test.go | 49 ++++++++++++++++++++++++++++++++++ api/resmap/reswrangler.go | 2 +- 2 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 api/krusty/blankvalues_test.go diff --git a/api/krusty/blankvalues_test.go b/api/krusty/blankvalues_test.go new file mode 100644 index 000000000..dfba4f365 --- /dev/null +++ b/api/krusty/blankvalues_test.go @@ -0,0 +1,49 @@ +package krusty_test + +import ( + "strings" + "testing" + + kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest" +) + +// test for https://github.com/kubernetes-sigs/kustomize/issues/4240 +func TestBlankNamespace4240(t *testing.T) { + th := kusttest_test.MakeHarness(t) + th.WriteK(".", ` +resources: +- resource.yaml +`) + + th.WriteF("resource.yaml", ` +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: test +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: test +rules: [] +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: test +subjects: +- kind: ServiceAccount + name: test + namespace: +roleRef: + kind: Role + name: test + apiGroup: rbac.authorization.k8s.io +`) + + err := th.RunWithErr(".", th.MakeDefaultOptions()) + if !strings.Contains(err.Error(), "Invalid Input: namespace is blank for resource") { + t.Fatalf("unexpected error: %q", err) + } +} diff --git a/api/resmap/reswrangler.go b/api/resmap/reswrangler.go index 93d82e64d..8b8d7834b 100644 --- a/api/resmap/reswrangler.go +++ b/api/resmap/reswrangler.go @@ -442,7 +442,7 @@ func getNamespacesForRoleBinding(r *resource.Resource) (map[string]bool, error) if n, ok3 := ns.(string); ok3 { result[n] = true } else { - return nil, errors.New("Invalid Input: namespace is blank") + return nil, errors.New(fmt.Sprintf("Invalid Input: namespace is blank for resource %q\n", r.CurId())) } } }