Simplify gvk, speed up cluster-scoped checks.

kyaml/resid/gvk.go

@@ -16,13 +16,26 @@ type Gvk struct {
 	Group   string `json:"group,omitempty" yaml:"group,omitempty"`
 	Version string `json:"version,omitempty" yaml:"version,omitempty"`
 	Kind    string `json:"kind,omitempty" yaml:"kind,omitempty"`
+	// isClusterScoped is true if the object is known, per the openapi
+	// data in use, to be cluster scoped, and false otherwise.
+	isClusterScoped bool
+}
+
+func NewGvk(g, v, k string) Gvk {
+	result := Gvk{Group: g, Version: v, Kind: k}
+	result.isClusterScoped =
+		openapi.IsCertainlyClusterScoped(result.AsTypeMeta())
+	return result
+}
+
+func GvkFromNode(r *yaml.RNode) Gvk {
+	g, v := ParseGroupVersion(r.GetApiVersion())
+	return NewGvk(g, v, r.GetKind())
 }
 
 // FromKind makes a Gvk with only the kind specified.
 func FromKind(k string) Gvk {
-	return Gvk{
-		Kind: k,
-	}
+	return NewGvk("", "", k)
 }
 
 // ParseGroupVersion parses a KRM metadata apiVersion field.
@@ -56,11 +69,7 @@ func GvkFromString(s string) Gvk {
 	if k == noKind {
 		k = ""
 	}
-	return Gvk{
-		Group:   g,
-		Version: v,
-		Kind:    k,
-	}
+	return NewGvk(g, v, k)
 }
 
 // Values that are brief but meaningful in logs.
@@ -90,10 +99,13 @@ func (x Gvk) String() string {
 
 // ApiVersion returns the combination of Group and Version
 func (x Gvk) ApiVersion() string {
-	if x.Group == "" {
-		return x.Version
+	var sb strings.Builder
+	if x.Group != "" {
+		sb.WriteString(x.Group)
+		sb.WriteString("/")
 	}
-	return x.Group + "/" + x.Version
+	sb.WriteString(x.Version)
+	return sb.String()
 }
 
 // StringWoEmptyField returns a string representation of the GVK. Non-exist
@@ -207,26 +219,16 @@ func (x Gvk) IsSelected(selector *Gvk) bool {
 	return true
 }
 
-// toKyamlTypeMeta returns a yaml.TypeMeta from x's information.
-func (x Gvk) toKyamlTypeMeta() yaml.TypeMeta {
-	var apiVersion strings.Builder
-	if x.Group != "" {
-		apiVersion.WriteString(x.Group)
-		apiVersion.WriteString("/")
-	}
-	apiVersion.WriteString(x.Version)
+// AsTypeMeta returns a yaml.TypeMeta from x's information.
+func (x Gvk) AsTypeMeta() yaml.TypeMeta {
 	return yaml.TypeMeta{
-		APIVersion: apiVersion.String(),
+		APIVersion: x.ApiVersion(),
 		Kind:       x.Kind,
 	}
 }
 
-// IsNamespaceableKind returns true if x is a namespaceable Gvk,
-// e.g. instances of Pod and Deployment are namespaceable,
-// but instances of Node and Namespace are not namespaceable.
-// Alternative name for this method: IsNotClusterScoped.
-// Implements https://kubernetes.io/docs/concepts/overview/working-with-objects/namespaces/#not-all-objects-are-in-a-namespace
-func (x Gvk) IsNamespaceableKind() bool {
-	isNamespaceScoped, found := openapi.IsNamespaceScoped(x.toKyamlTypeMeta())
-	return !found || isNamespaceScoped
+// IsClusterScoped returns true if the Gvk is certainly cluster scoped
+// with respect to the available openapi data.
+func (x Gvk) IsClusterScoped() bool {
+	return x.isClusterScoped
 }

kyaml/resid/gvk_test.go

@@ -259,30 +259,45 @@ func TestSelectByGVK(t *testing.T) {
 	}
 }
 
-func TestIsNamespaceableKind(t *testing.T) {
+func TestIsClusterScoped(t *testing.T) {
 	testCases := []struct {
-		name     string
-		gvk      Gvk
-		expected bool
+		name            string
+		gvk             Gvk
+		isClusterScoped bool
 	}{
 		{
-			"namespaceable resource",
-			Gvk{Group: "apps", Version: "v1", Kind: "Deployment"},
-			true,
-		},
-		{
-			"clusterscoped resource",
-			Gvk{Group: "", Version: "v1", Kind: "Namespace"},
+			"deployment is namespaceable",
+			NewGvk("apps", "v1", "Deployment"),
 			false,
 		},
 		{
-			"unknown resource (should default to namespaceable)",
-			Gvk{Group: "example1.com", Version: "v1", Kind: "Bar"},
+			"clusterscoped resource",
+			NewGvk("", "v1", "Namespace"),
 			true,
 		},
 		{
 			"unknown resource (should default to namespaceable)",
-			Gvk{Group: "apps", Version: "v1", Kind: "ClusterRoleBinding"},
+			NewGvk("example1.com", "v1", "BoatyMcBoatface"),
+			false,
+		},
+		{
+			"node is cluster scoped",
+			NewGvk("", "v1", "Node"),
+			true,
+		},
+		{
+			"Role is namespace scoped",
+			NewGvk("rbac.authorization.k8s.io", "v1", "Role"),
+			false,
+		},
+		{
+			"ClusterRole is cluster scoped",
+			NewGvk("rbac.authorization.k8s.io", "v1", "ClusterRole"),
+			true,
+		},
+		{
+			"ClusterRoleBinding is cluster scoped",
+			NewGvk("rbac.authorization.k8s.io", "v1", "ClusterRoleBinding"),
 			true,
 		},
 	}
@@ -290,8 +305,7 @@ func TestIsNamespaceableKind(t *testing.T) {
 	for i := range testCases {
 		test := testCases[i]
 		t.Run(test.name, func(t *testing.T) {
-			isNamespaceable := test.gvk.IsNamespaceableKind()
-			assert.Equal(t, test.expected, isNamespaceable)
+			assert.Equal(t, test.isClusterScoped, test.gvk.IsClusterScoped())
 		})
 	}
 }

kyaml/resid/resid.go

@@ -12,13 +12,10 @@ type ResId struct {
 	// Gvk of the resource.
 	Gvk `json:",inline,omitempty" yaml:",inline,omitempty"`
 
-	// Name of the resource before transformation.
+	// Name of the resource.
 	Name string `json:"name,omitempty" yaml:"name,omitempty"`
 
-	// Namespace the resource belongs to.
-	// An untransformed resource has no namespace.
-	// A fully transformed resource has the namespace
-	// from the top most overlay.
+	// Namespace the resource belongs to, if it can belong to a namespace.
 	Namespace string `json:"namespace,omitempty" yaml:"namespace,omitempty"`
 }
 
@@ -30,12 +27,12 @@ func NewResIdWithNamespace(k Gvk, n, ns string) ResId {
 
 // NewResId creates new ResId.
 func NewResId(k Gvk, n string) ResId {
-	return ResId{Gvk: k, Name: n}
+	return NewResIdWithNamespace(k, n, "")
 }
 
 // NewResIdKindOnly creates a new ResId.
 func NewResIdKindOnly(k string, n string) ResId {
-	return ResId{Gvk: FromKind(k), Name: n}
+	return NewResId(FromKind(k), n)
 }
 
 const (
@@ -113,7 +110,7 @@ func (id ResId) IsNsEquals(o ResId) bool {
 // ResId and the Namespace is either not set or set
 // to DefaultNamespace.
 func (id ResId) IsInDefaultNs() bool {
-	return id.IsNamespaceableKind() && id.isPutativelyDefaultNs()
+	return !id.IsClusterScoped() && id.isPutativelyDefaultNs()
 }
 
 func (id ResId) isPutativelyDefaultNs() bool {
@@ -124,7 +121,7 @@ func (id ResId) isPutativelyDefaultNs() bool {
 // namespace for use in reporting and equality tests.
 func (id ResId) EffectiveNamespace() string {
 	// The order of these checks matters.
-	if !id.IsNamespaceableKind() {
+	if id.IsClusterScoped() {
 		return TotallyNotANamespace
 	}
 	if id.isPutativelyDefaultNs() {

kyaml/resid/resid_test.go

@@ -464,42 +464,42 @@ func TestFromString(t *testing.T) {
 }
 
 func TestEffectiveNamespace(t *testing.T) {
-	var test = []struct {
+	var testCases = map[string]struct {
 		id       ResId
 		expected string
 	}{
-		{
+		"tst1": {
 			id: ResId{
-				Gvk:  Gvk{Group: "", Version: "v1", Kind: "Node"},
+				Gvk:  NewGvk("", "v1", "Node"),
 				Name: "nm",
 			},
 			expected: TotallyNotANamespace,
 		},
-		{
+		"tst2": {
 			id: ResId{
 				Namespace: "foo",
-				Gvk:       Gvk{Group: "", Version: "v1", Kind: "Node"},
+				Gvk:       NewGvk("", "v1", "Node"),
 				Name:      "nm",
 			},
 			expected: TotallyNotANamespace,
 		},
-		{
+		"tst3": {
 			id: ResId{
 				Namespace: "foo",
-				Gvk:       Gvk{Group: "g", Version: "v", Kind: "k"},
+				Gvk:       NewGvk("g", "v", "k"),
 				Name:      "nm",
 			},
 			expected: "foo",
 		},
-		{
+		"tst4": {
 			id: ResId{
 				Namespace: "",
-				Gvk:       Gvk{Group: "g", Version: "v", Kind: "k"},
+				Gvk:       NewGvk("g", "v", "k"),
 				Name:      "nm",
 			},
 			expected: DefaultNamespace,
 		},
-		{
+		"tst5": {
 			id: ResId{
 				Gvk:  Gvk{Group: "g", Version: "v", Kind: "k"},
 				Name: "nm",
@@ -508,10 +508,12 @@ func TestEffectiveNamespace(t *testing.T) {
 		},
 	}
 
-	for _, tst := range test {
-		if actual := tst.id.EffectiveNamespace(); actual != tst.expected {
-			t.Fatalf("EffectiveNamespace was %s, expected %s",
-				actual, tst.expected)
-		}
+	for n, tst := range testCases {
+		t.Run(n, func(t *testing.T) {
+			if actual := tst.id.EffectiveNamespace(); actual != tst.expected {
+				t.Fatalf("EffectiveNamespace was %s, expected %s",
+					actual, tst.expected)
+			}
+		})
 	}
 }