Merge pull request #3863 from monopole/simplifyGvk

Simplify gvk, speed up cluster-scoped checks.

api/filters/fieldspec/fieldspec.go

@@ -11,6 +11,7 @@ import (
 	"sigs.k8s.io/kustomize/api/internal/utils"
 	"sigs.k8s.io/kustomize/api/types"
 	"sigs.k8s.io/kustomize/kyaml/errors"
+	"sigs.k8s.io/kustomize/kyaml/resid"
 	"sigs.k8s.io/kustomize/kyaml/yaml"
 )
 
@@ -45,8 +46,8 @@ type Filter struct {
 
 func (fltr Filter) Filter(obj *yaml.RNode) (*yaml.RNode, error) {
 	// check if the FieldSpec applies to the object
-	if match, err := isMatchGVK(fltr.FieldSpec, obj); !match || err != nil {
-		return obj, errors.Wrap(err)
+	if match := isMatchGVK(fltr.FieldSpec, obj); !match {
+		return obj, nil
 	}
 	fltr.path = utils.PathSplitter(fltr.FieldSpec.Path)
 	err := fltr.filter(obj)
@@ -158,28 +159,24 @@ func isSequenceField(name string) (string, bool) {
 }
 
 // isMatchGVK returns true if the fs.GVK matches the obj GVK.
-func isMatchGVK(fs types.FieldSpec, obj *yaml.RNode) (bool, error) {
-	meta, err := obj.GetMeta()
-	if err != nil {
-		return false, err
-	}
-	if fs.Kind != "" && fs.Kind != meta.Kind {
+func isMatchGVK(fs types.FieldSpec, obj *yaml.RNode) bool {
+	if kind := obj.GetKind(); fs.Kind != "" && fs.Kind != kind {
 		// kind doesn't match
-		return false, err
+		return false
 	}
 
 	// parse the group and version from the apiVersion field
-	group, version := parseGV(meta.APIVersion)
+	group, version := resid.ParseGroupVersion(obj.GetApiVersion())
 
 	if fs.Group != "" && fs.Group != group {
 		// group doesn't match
-		return false, nil
+		return false
 	}
 
 	if fs.Version != "" && fs.Version != version {
 		// version doesn't match
-		return false, nil
+		return false
 	}
 
-	return true, nil
+	return true
 }

api/filters/fieldspec/fieldspec_test.go

@@ -46,10 +46,11 @@ xxx:
 		"empty path": {
 			fieldSpec: `
 group: foo
+version: v1
 kind: Bar
 `,
 			input: `
-apiVersion: foo
+apiVersion: foo/v1
 kind: Bar
 xxx:
 `,
@@ -59,7 +60,7 @@ kind: Bar
 xxx:
 `,
 			error: `considering field '' of object
-apiVersion: foo
+apiVersion: foo/v1
 kind: Bar
 xxx:
 : cannot set or create an empty field name`,
@@ -195,11 +196,14 @@ kind: Bar
 			input: `
 a:
   b: c
+`,
+			expected: `
+a:
+  b: c
 `,
 			filter: fieldspec.Filter{
 				SetValue: filtersutil.SetScalar("e"),
 			},
-			error: "missing Resource metadata",
 		},
 
 		"miss-match-type": {

api/filters/fieldspec/gvk.go

@@ -1,49 +0,0 @@
-// Copyright 2019 The Kubernetes Authors.
-// SPDX-License-Identifier: Apache-2.0
-package fieldspec
-
-import (
-	"strings"
-
-	"sigs.k8s.io/kustomize/kyaml/resid"
-	"sigs.k8s.io/kustomize/kyaml/yaml"
-)
-
-// Return true for 'v' followed by a 1 or 2, and don't look at rest.
-// I.e. 'v1', 'v1beta1', 'v2', would return true.
-func looksLikeACoreApiVersion(s string) bool {
-	if len(s) < 2 {
-		return false
-	}
-	if s[0:1] != "v" {
-		return false
-	}
-	return s[1:2] == "1" || s[1:2] == "2"
-}
-
-// parseGV parses apiVersion field into group and version.
-func parseGV(apiVersion string) (group, version string) {
-	// parse the group and version from the apiVersion field
-	parts := strings.SplitN(apiVersion, "/", 2)
-	group = parts[0]
-	if len(parts) > 1 {
-		version = parts[1]
-	}
-	// Special case the original "apiVersion" of what
-	// we now call the "core" (empty) group.
-	if version == "" && looksLikeACoreApiVersion(group) {
-		version = group
-		group = ""
-	}
-	return
-}
-
-// GetGVK parses the metadata into a GVK
-func GetGVK(meta yaml.ResourceMeta) resid.Gvk {
-	group, version := parseGV(meta.APIVersion)
-	return resid.Gvk{
-		Group:   group,
-		Version: version,
-		Kind:    meta.Kind,
-	}
-}

api/filters/fieldspec/gvk_test.go

@@ -1,156 +0,0 @@
-// Copyright 2019 The Kubernetes Authors.
-// SPDX-License-Identifier: Apache-2.0
-package fieldspec
-
-import (
-	"strings"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"sigs.k8s.io/kustomize/kyaml/resid"
-	"sigs.k8s.io/kustomize/kyaml/yaml"
-)
-
-func TestParseGV(t *testing.T) {
-	testCases := map[string]struct {
-		input           string
-		expectedGroup   string
-		expectedVersion string
-	}{
-		"empty": {
-			input:           "",
-			expectedGroup:   "",
-			expectedVersion: "",
-		},
-		"certSigning": {
-			input:           "certificates.k8s.io/v1beta1",
-			expectedGroup:   "certificates.k8s.io",
-			expectedVersion: "v1beta1",
-		},
-		"extensions": {
-			input:           "extensions/v1beta1",
-			expectedGroup:   "extensions",
-			expectedVersion: "v1beta1",
-		},
-		"normal": {
-			input:           "apps/v1",
-			expectedGroup:   "apps",
-			expectedVersion: "v1",
-		},
-		"justApps": {
-			input:           "apps",
-			expectedGroup:   "apps",
-			expectedVersion: "",
-		},
-		"coreV1": {
-			input:           "v1",
-			expectedGroup:   "",
-			expectedVersion: "v1",
-		},
-		"coreV2": {
-			input:           "v2",
-			expectedGroup:   "",
-			expectedVersion: "v2",
-		},
-		"coreV2Beta1": {
-			input:           "v2beta1",
-			expectedGroup:   "",
-			expectedVersion: "v2beta1",
-		},
-	}
-
-	for tn, tc := range testCases {
-		t.Run(tn, func(t *testing.T) {
-			group, version := parseGV(tc.input)
-			if !assert.Equal(t, tc.expectedGroup, group) {
-				t.FailNow()
-			}
-			if !assert.Equal(t, tc.expectedVersion, version) {
-				t.FailNow()
-			}
-		})
-	}
-}
-
-func TestGetGVK(t *testing.T) {
-	testCases := map[string]struct {
-		input      string
-		expected   resid.Gvk
-		parseError string
-		metaError  string
-	}{
-		"empty": {
-			input: `
-`,
-			parseError: "EOF",
-		},
-		"junk": {
-			input: `
-congress: effective
-`,
-			metaError: "missing Resource metadata",
-		},
-		"normal": {
-			input: `
-apiVersion: apps/v1
-kind: Deployment
-`,
-			expected: resid.Gvk{Group: "apps", Version: "v1", Kind: "Deployment"},
-		},
-		"apiVersionOnlyWithSlash": {
-			input: `
-apiVersion: apps/v1
-`,
-			expected: resid.Gvk{Group: "apps", Version: "v1", Kind: ""},
-		},
-		"apiVersionOnlyNoSlash1": {
-			input: `
-apiVersion: apps
-`,
-			expected: resid.Gvk{Group: "apps", Version: "", Kind: ""},
-		},
-		"apiVersionOnlyNoSlash2": {
-			input: `
-apiVersion: v1
-`,
-			expected: resid.Gvk{Group: "", Version: "v1", Kind: ""},
-		},
-	}
-
-	for tn, tc := range testCases {
-		t.Run(tn, func(t *testing.T) {
-			obj, err := yaml.Parse(tc.input)
-			if len(tc.parseError) != 0 {
-				if err == nil {
-					t.Error("expected parse error")
-					return
-				}
-				if !strings.Contains(err.Error(), tc.parseError) {
-					t.Errorf("expected parse err '%s', got '%v'", tc.parseError, err)
-				}
-				return
-			}
-			if !assert.NoError(t, err) {
-				t.FailNow()
-			}
-			meta, err := obj.GetMeta()
-			if len(tc.metaError) != 0 {
-				if err == nil {
-					t.Error("expected meta error")
-					return
-				}
-				if !strings.Contains(err.Error(), tc.metaError) {
-					t.Errorf("expected meta err '%s', got '%v'", tc.metaError, err)
-				}
-				return
-			}
-			if !assert.NoError(t, err) {
-				t.FailNow()
-			}
-			gvk := GetGVK(meta)
-			if !assert.Equal(t, tc.expected, gvk) {
-				t.FailNow()
-			}
-		})
-	}
-}

api/filters/nameref/nameref.go

@@ -257,8 +257,7 @@ func previousIdSelectedByGvk(gvk *resid.Gvk) sieveFunc {
 
 // If the we are updating a 'roleRef/name' field, the 'apiGroup' and 'kind'
 // fields in the same 'roleRef' map must be considered.
-// If either object is cluster-scoped (!IsNamespaceableKind), there
-// can be a referral.
+// If either object is cluster-scoped, there can be a referral.
 // E.g. a RoleBinding (which exists in a namespace) can refer
 // to a ClusterRole (cluster-scoped) object.
 // https://kubernetes.io/docs/reference/access-authn-authz/rbac/#role-and-clusterrole
@@ -285,12 +284,12 @@ func prefixSuffixEquals(other resource.ResCtx) sieveFunc {
 
 func (f Filter) sameCurrentNamespaceAsReferrer() sieveFunc {
 	referrerCurId := f.Referrer.CurId()
-	if !referrerCurId.IsNamespaceableKind() {
+	if referrerCurId.IsClusterScoped() {
 		// If the referrer is cluster-scoped, let anything through.
 		return acceptAll
 	}
 	return func(r *resource.Resource) bool {
-		if !r.CurId().IsNamespaceableKind() {
+		if r.CurId().IsClusterScoped() {
 			// Allow cluster-scoped through.
 			return true
 		}

api/filters/namespace/namespace.go

@@ -4,11 +4,11 @@
 package namespace
 
 import (
-	"sigs.k8s.io/kustomize/api/filters/fieldspec"
 	"sigs.k8s.io/kustomize/api/filters/filtersutil"
 	"sigs.k8s.io/kustomize/api/filters/fsslice"
 	"sigs.k8s.io/kustomize/api/types"
 	"sigs.k8s.io/kustomize/kyaml/kio"
+	"sigs.k8s.io/kustomize/kyaml/resid"
 	"sigs.k8s.io/kustomize/kyaml/yaml"
 )
 
@@ -54,16 +54,11 @@ func (ns Filter) run(node *yaml.RNode) (*yaml.RNode, error) {
 // hacks applies the namespace transforms that are hardcoded rather
 // than specified through FieldSpecs.
 func (ns Filter) hacks(obj *yaml.RNode) error {
-	meta, err := obj.GetMeta()
-	if err != nil {
+	gvk := resid.GvkFromNode(obj)
+	if err := ns.metaNamespaceHack(obj, gvk); err != nil {
 		return err
 	}
-
-	if err := ns.metaNamespaceHack(obj, meta); err != nil {
-		return err
-	}
-
-	return ns.roleBindingHack(obj, meta)
+	return ns.roleBindingHack(obj, gvk)
 }
 
 // metaNamespaceHack is a hack for implementing the namespace transform
@@ -74,9 +69,8 @@ func (ns Filter) hacks(obj *yaml.RNode) error {
 // This hack should be updated to allow individual resources to specify
 // if they are cluster scoped through either an annotation on the resources,
 // or through inlined OpenAPI on the resource as a YAML comment.
-func (ns Filter) metaNamespaceHack(obj *yaml.RNode, meta yaml.ResourceMeta) error {
-	gvk := fieldspec.GetGVK(meta)
-	if !gvk.IsNamespaceableKind() {
+func (ns Filter) metaNamespaceHack(obj *yaml.RNode, gvk resid.Gvk) error {
+	if gvk.IsClusterScoped() {
 		return nil
 	}
 	f := fsslice.Filter{
@@ -104,8 +98,8 @@ func (ns Filter) metaNamespaceHack(obj *yaml.RNode, meta yaml.ResourceMeta) erro
 //   ...
 // - name: "something-else" # this will not have the namespace set
 //   ...
-func (ns Filter) roleBindingHack(obj *yaml.RNode, meta yaml.ResourceMeta) error {
-	if meta.Kind != roleBindingKind && meta.Kind != clusterRoleBindingKind {
+func (ns Filter) roleBindingHack(obj *yaml.RNode, gvk resid.Gvk) error {
+	if gvk.Kind != roleBindingKind && gvk.Kind != clusterRoleBindingKind {
 		return nil
 	}
 

api/internal/accumulator/namereferencetransformer_test.go

@@ -885,9 +885,9 @@ func TestNameReferenceClusterWide(t *testing.T) {
 			}).ResMap()
 
 	clusterRoleId := resid.NewResId(
-		resid.Gvk{Group: "rbac.authorization.k8s.io", Version: "v1", Kind: "ClusterRole"}, modifiedname)
+		resid.NewGvk("rbac.authorization.k8s.io", "v1", "ClusterRole"), modifiedname)
 	clusterRoleBindingId := resid.NewResId(
-		resid.Gvk{Group: "rbac.authorization.k8s.io", Version: "v1", Kind: "ClusterRoleBinding"}, modifiedname)
+		resid.NewGvk("rbac.authorization.k8s.io", "v1", "ClusterRoleBinding"), modifiedname)
 	clusterRole, _ := expected.GetByCurrentId(clusterRoleId)
 	clusterRole.AppendRefBy(clusterRoleBindingId)
 
@@ -1012,9 +1012,11 @@ func TestNameReferenceNamespaceTransformation(t *testing.T) {
 			}).ResMap()
 
 	clusterRoleId := resid.NewResId(
-		resid.Gvk{Group: "rbac.authorization.k8s.io", Version: "v1", Kind: "ClusterRole"}, modifiedname)
+		resid.NewGvk("rbac.authorization.k8s.io", "v1", "ClusterRole"),
+		modifiedname)
 	clusterRoleBindingId := resid.NewResId(
-		resid.Gvk{Group: "rbac.authorization.k8s.io", Version: "v1", Kind: "ClusterRoleBinding"}, modifiedname)
+		resid.NewGvk("rbac.authorization.k8s.io", "v1", "ClusterRoleBinding"),
+		modifiedname)
 	clusterRole, _ := expected.GetByCurrentId(clusterRoleId)
 	clusterRole.AppendRefBy(clusterRoleBindingId)
 

api/internal/accumulator/resaccumulator.go

@@ -72,7 +72,7 @@ func (ra *ResAccumulator) MergeVars(incoming []types.Var) error {
 	for _, v := range incoming {
 		targetId := resid.NewResIdWithNamespace(v.ObjRef.GVK(), v.ObjRef.Name, v.ObjRef.Namespace)
 		idMatcher := targetId.GvknEquals
-		if targetId.Namespace != "" || !targetId.IsNamespaceableKind() {
+		if targetId.Namespace != "" || targetId.IsClusterScoped() {
 			// Preserve backward compatibility. An empty namespace means
 			// wildcard search on the namespace hence we still use GvknEquals
 			idMatcher = targetId.Equals

api/resmap/resmap.go

@@ -168,8 +168,7 @@ type ResMap interface {
 
 	// GroupedByCurrentNamespace returns a map of namespace
 	// to a slice of *Resource in that namespace.
-	// Resources for whom IsNamespaceableKind is false are
-	// are not included at all (see NonNamespaceable).
+	// Cluster-scoped Resources are not included (see ClusterScoped).
 	// Resources with an empty namespace are placed
 	// in the resid.DefaultNamespace entry.
 	GroupedByCurrentNamespace() map[string][]*resource.Resource
@@ -179,10 +178,10 @@ type ResMap interface {
 	// one to perform the grouping.
 	GroupedByOriginalNamespace() map[string][]*resource.Resource
 
-	// NonNamespaceable returns a slice of resources that
+	// ClusterScoped returns a slice of resources that
 	// cannot be placed in a namespace, e.g.
 	// Node, ClusterRole, Namespace itself, etc.
-	NonNamespaceable() []*resource.Resource
+	ClusterScoped() []*resource.Resource
 
 	// AllIds returns all CurrentIds.
 	AllIds() []resid.ResId

api/resmap/reswrangler.go

@@ -238,8 +238,8 @@ func (m *resWrangler) GroupedByCurrentNamespace() map[string][]*resource.Resourc
 	return items
 }
 
-// NonNamespaceable implements ResMap.NonNamespaceable
-func (m *resWrangler) NonNamespaceable() []*resource.Resource {
+// ClusterScoped implements ResMap.ClusterScoped
+func (m *resWrangler) ClusterScoped() []*resource.Resource {
 	return m.groupedByCurrentNamespace()[resid.TotallyNotANamespace]
 }
 
@@ -388,7 +388,7 @@ func (m *resWrangler) makeCopy(copier resCopier) ResMap {
 func (m *resWrangler) SubsetThatCouldBeReferencedByResource(
 	referrer *resource.Resource) ResMap {
 	referrerId := referrer.CurId()
-	if !referrerId.IsNamespaceableKind() {
+	if referrerId.IsClusterScoped() {
 		// A cluster scoped resource can refer to anything.
 		return m
 	}
@@ -396,7 +396,7 @@ func (m *resWrangler) SubsetThatCouldBeReferencedByResource(
 	roleBindingNamespaces := getNamespacesForRoleBinding(referrer)
 	for _, possibleTarget := range m.rList {
 		id := possibleTarget.CurId()
-		if !id.IsNamespaceableKind() {
+		if id.IsClusterScoped() {
 			// A cluster-scoped resource can be referred to by anything.
 			result.append(possibleTarget)
 			continue

api/resmap/reswrangler_test.go

@@ -192,7 +192,7 @@ metadata:
 }
 
 func TestGetMatchingResourcesByCurrentId(t *testing.T) {
-	cmap := resid.Gvk{Version: "v1", Kind: "ConfigMap"}
+	cmap := resid.NewGvk("", "v1", "ConfigMap")
 
 	r1 := rf.FromMap(
 		map[string]interface{}{

api/resource/resource.go

@@ -87,12 +87,7 @@ func (r *Resource) GetBinaryDataMap() map[string]string {
 }
 
 func (r *Resource) GetGvk() resid.Gvk {
-	meta, err := r.node.GetMeta()
-	if err != nil {
-		return resid.GvkFromString("")
-	}
-	g, v := resid.ParseGroupVersion(meta.APIVersion)
-	return resid.Gvk{Group: g, Version: v, Kind: meta.Kind}
+	return resid.GvkFromNode(r.node)
 }
 
 func (r *Resource) Hash(h ifc.KustHasher) (string, error) {

api/resource/resource_test.go

@@ -87,11 +87,13 @@ func TestResourceId(t *testing.T) {
 		{
 			in: testConfigMap,
 			id: resid.NewResIdWithNamespace(
-				resid.Gvk{Version: "v1", Kind: "ConfigMap"}, "winnie", "hundred-acre-wood"),
+				resid.NewGvk("", "v1", "ConfigMap"),
+				"winnie", "hundred-acre-wood"),
 		},
 		{
 			in: testDeployment,
-			id: resid.NewResId(resid.Gvk{Group: "apps", Version: "v1", Kind: "Deployment"}, "pooh"),
+			id: resid.NewResId(
+				resid.NewGvk("apps", "v1", "Deployment"), "pooh"),
 		},
 	}
 	for _, test := range tests {