Allow colon in the name of RBAC resources

pkg/inventory/inventory_test.go

@@ -437,16 +437,6 @@ func TestLegacyInventoryName(t *testing.T) {
 			isModified: false,
 			isError:    true,
 		},
-		"Info name differs from object name should return error": {
-			info: &resource.Info{
-				Namespace: testNamespace,
-				Name:      inventoryObjName,
-				Object:    &legacyInvObj,
-			},
-			invName:    inventoryObjName,
-			isModified: false,
-			isError:    true,
-		},
 		"Legacy inventory name gets random suffix": {
 			info: &resource.Info{
 				Namespace: testNamespace,

pkg/object/objmetadata.go

@@ -22,15 +22,30 @@ import (
 	"strconv"
 	"strings"
 
+	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/cli-runtime/pkg/resource"
 )
 
-// Separates inventory fields. This string is allowable as a
+const (
-// ConfigMap key, but it is not allowed as a character in
+	// Separates inventory fields. This string is allowable as a
-// resource name.
+	// ConfigMap key, but it is not allowed as a character in
-const fieldSeparator = "_"
+	// resource name.
+	fieldSeparator = "_"
+	// Transform colons in the RBAC resource names to double
+	// underscore.
+	colonTranscoded = "__"
+)
+
+// RBACGroupKind is a map of the RBAC resources. Needed since name validation
+// is different than other k8s resources.
+var RBACGroupKind = map[schema.GroupKind]bool{
+	{Group: rbacv1.GroupName, Kind: "Role"}:               true,
+	{Group: rbacv1.GroupName, Kind: "ClusterRole"}:        true,
+	{Group: rbacv1.GroupName, Kind: "RoleBinding"}:        true,
+	{Group: rbacv1.GroupName, Kind: "ClusterRoleBinding"}: true,
+}
 
 // ObjMetadata organizes and stores the indentifying information
 // for an object. This struct (as a string) is stored in a
@@ -52,7 +67,7 @@ func CreateObjMetadata(namespace string, name string, gk schema.GroupKind) (ObjM
 	}
 	// Manually validate name, since by the time k8s reports the error
 	// the invalid name has already been encoded into the inventory object.
-	if !validateNameChars(name) {
+	if !validateNameChars(name, gk) {
 		return ObjMetadata{}, fmt.Errorf("invalid characters in object name: %s", name)
 	}
 	if gk.Empty() {
@@ -65,9 +80,9 @@ func CreateObjMetadata(namespace string, name string, gk schema.GroupKind) (ObjM
 	}, nil
 }
 
-// validateNameChars returns false if the passed name string contains
+// validateNameChars returns false if the passed name is not a valid
-// any invalid characters; true otherwise. The allowed characters for
+// resource name; true otherwise. For almost all resources, the following
-// a Kubernetes resource name are:
+// characters are allowed:
 //
 //   Most resource types require a name that can be used as a DNS label name
 //   as defined in RFC 1123. This means the name must:
@@ -77,29 +92,56 @@ func CreateObjMetadata(namespace string, name string, gk schema.GroupKind) (ObjM
 //   * start with an alphanumeric character
 //   * end with an alphanumeric character
 //
-func validateNameChars(name string) bool {
+// For RBAC resources we also allow the colon character.
+func validateNameChars(name string, gk schema.GroupKind) bool {
+	if _, exists := RBACGroupKind[gk]; exists {
+		name = strings.ReplaceAll(name, ":", "")
+	}
 	errs := validation.IsDNS1123Subdomain(name)
 	return len(errs) == 0
 }
 
-// ParseObjMetadata takes a string, splits it into its five fields,
+// ParseObjMetadata takes a string, splits it into its four fields,
-// and returns a pointer to an ObjMetadata struct storing the
+// and returns an ObjMetadata struct storing the four fields.
-// five fields. Example inventory string:
+// Example inventory string:
 //
 //   test-namespace_test-name_apps_ReplicaSet
 //
 // Returns an error if unable to parse and create the ObjMetadata
 // struct.
-func ParseObjMetadata(inv string) (ObjMetadata, error) {
+//
-	parts := strings.Split(inv, fieldSeparator)
+// NOTE: name field can contain double underscore (__), which represents
-	if len(parts) == 4 {
+// a colon. RBAC resources can have this additional character (:) in their name.
-		gk := schema.GroupKind{
+func ParseObjMetadata(s string) (ObjMetadata, error) {
-			Group: strings.TrimSpace(parts[2]),
+	// Parse first field namespace
-			Kind:  strings.TrimSpace(parts[3]),
+	index := strings.Index(s, fieldSeparator)
-		}
+	if index == -1 {
-		return CreateObjMetadata(parts[0], parts[1], gk)
+		return ObjMetadata{}, fmt.Errorf("unable to parse stored object metadata: %s", s)
 	}
-	return ObjMetadata{}, fmt.Errorf("unable to decode inventory: %s", inv)
+	namespace := s[:index]
+	s = s[index+1:]
+	// Next, parse last field kind
+	index = strings.LastIndex(s, fieldSeparator)
+	if index == -1 {
+		return ObjMetadata{}, fmt.Errorf("unable to parse stored object metadata: %s", s)
+	}
+	kind := s[index+1:]
+	s = s[:index]
+	// Next, parse next to last field group
+	index = strings.LastIndex(s, fieldSeparator)
+	if index == -1 {
+		return ObjMetadata{}, fmt.Errorf("unable to parse stored object metadata: %s", s)
+	}
+	group := s[index+1:]
+	// Finally, second field name. Name may contain colon transcoded as double underscore.
+	name := s[:index]
+	name = strings.ReplaceAll(name, colonTranscoded, ":")
+	// Create the ObjMetadata object from the four parsed fields.
+	gk := schema.GroupKind{
+		Group: strings.TrimSpace(group),
+		Kind:  strings.TrimSpace(kind),
+	}
+	return CreateObjMetadata(namespace, name, gk)
 }
 
 // Equals compares two ObjMetadata and returns true if they are equal. This does
@@ -111,11 +153,17 @@ func (o *ObjMetadata) Equals(other *ObjMetadata) bool {
 	return *o == *other
 }
 
-// String create a string version of the ObjMetadata struct.
+// String create a string version of the ObjMetadata struct. For RBAC resources,
+// the "name" field transcodes ":" into double underscore for valid storing
+// as the label of a ConfigMap.
 func (o *ObjMetadata) String() string {
+	name := o.Name
+	if _, exists := RBACGroupKind[o.GroupKind]; exists {
+		name = strings.ReplaceAll(name, ":", colonTranscoded)
+	}
 	return fmt.Sprintf("%s%s%s%s%s%s%s",
 		o.Namespace, fieldSeparator,
-		o.Name, fieldSeparator,
+		name, fieldSeparator,
 		o.GroupKind.Group, fieldSeparator,
 		o.GroupKind.Kind)
 }

pkg/object/objmetadata_test.go

@@ -7,18 +7,19 @@ import (
 	"strings"
 	"testing"
 
+	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
 func TestCreateObjMetadata(t *testing.T) {
-	tests := []struct {
+	tests := map[string]struct {
 		namespace string
 		name      string
 		gk        schema.GroupKind
 		expected  string
 		isError   bool
 	}{
-		{
+		"Namespace with only whitespace": {
 			namespace: "  \n",
 			name:      " test-name\t",
 			gk: schema.GroupKind{
@@ -28,7 +29,7 @@ func TestCreateObjMetadata(t *testing.T) {
 			expected: "_test-name_apps_ReplicaSet",
 			isError:  false,
 		},
-		{
+		"Name with leading/trailing whitespace": {
 			namespace: "test-namespace ",
 			name:      " test-name\t",
 			gk: schema.GroupKind{
@@ -38,8 +39,7 @@ func TestCreateObjMetadata(t *testing.T) {
 			expected: "test-namespace_test-name_apps_ReplicaSet",
 			isError:  false,
 		},
-		// Error with empty name.
+		"Empty name is an error": {
-		{
 			namespace: "test-namespace ",
 			name:      " \t",
 			gk: schema.GroupKind{
@@ -49,16 +49,14 @@ func TestCreateObjMetadata(t *testing.T) {
 			expected: "",
 			isError:  true,
 		},
-		// Error with empty GroupKind.
+		"Empty GroupKind is an error": {
-		{
 			namespace: "test-namespace",
 			name:      "test-name",
 			gk:        schema.GroupKind{},
 			expected:  "",
 			isError:   true,
 		},
-		// Error with invalid name characters "_".
+		"Underscore is invalid name character": {
-		{
 			namespace: "test-namespace",
 			name:      "test_name", // Invalid "_" character
 			gk: schema.GroupKind{
@@ -68,8 +66,7 @@ func TestCreateObjMetadata(t *testing.T) {
 			expected: "",
 			isError:  true,
 		},
-		// Error name not starting with alphanumeric char
+		"Name not starting with alphanumeric character is error": {
-		{
 			namespace: "test-namespace",
 			name:      "-test",
 			gk: schema.GroupKind{
@@ -79,8 +76,7 @@ func TestCreateObjMetadata(t *testing.T) {
 			expected: "",
 			isError:  true,
 		},
-		// Error name not ending with alphanumeric char
+		"Name not ending with alphanumeric character is error": {
-		{
 			namespace: "test-namespace",
 			name:      "test-",
 			gk: schema.GroupKind{
@@ -90,34 +86,56 @@ func TestCreateObjMetadata(t *testing.T) {
 			expected: "",
 			isError:  true,
 		},
+		"Colon is allowed in the name for RBAC resources": {
+			namespace: "test-namespace",
+			name:      "system::kube-scheduler",
+			gk: schema.GroupKind{
+				Group: rbacv1.GroupName,
+				Kind:  "Role",
+			},
+			expected: "test-namespace_system____kube-scheduler_rbac.authorization.k8s.io_Role",
+			isError:  false,
+		},
+		"Colon is not allowed in the name for non-RBAC resources": {
+			namespace: "test-namespace",
+			name:      "system::kube-scheduler",
+			gk: schema.GroupKind{
+				Group: "",
+				Kind:  "Pod",
+			},
+			expected: "",
+			isError:  true,
+		},
 	}
 
-	for _, test := range tests {
+	for name, tc := range tests {
-		inv, err := CreateObjMetadata(test.namespace, test.name, test.gk)
+		t.Run(name, func(t *testing.T) {
-		if !test.isError {
+			inv, err := CreateObjMetadata(tc.namespace, tc.name, tc.gk)
-			if err != nil {
+			if !tc.isError {
-				t.Errorf("Error creating ObjMetadata when it should have worked.")
+				if err != nil {
-			} else if test.expected != inv.String() {
+					t.Errorf("Error creating ObjMetadata when it should have worked.")
-				t.Errorf("Expected inventory\n(%s) != created inventory\n(%s)\n", test.expected, inv.String())
+				} else if tc.expected != inv.String() {
+					t.Errorf("Expected inventory\n(%s) != created inventory\n(%s)\n", tc.expected, inv.String())
+				}
+				// Parsing back the just created inventory string to ObjMetadata,
+				// so that tests will catch any change to CreateObjMetadata that
+				// would break ParseObjMetadata.
+				expectedObjMetadata := &ObjMetadata{
+					Namespace: strings.TrimSpace(tc.namespace),
+					Name:      strings.TrimSpace(tc.name),
+					GroupKind: tc.gk,
+				}
+				actual, err := ParseObjMetadata(inv.String())
+				if err != nil {
+					t.Errorf("Error parsing back ObjMetadata, when it should have worked.")
+				} else if !expectedObjMetadata.Equals(&actual) {
+					t.Errorf("Expected inventory (%s) != parsed inventory (%s)\n", expectedObjMetadata, actual)
+				}
 			}
-			// Parsing back the just created inventory string to ObjMetadata,
+			if tc.isError && err == nil {
-			// so that tests will catch any change to CreateObjMetadata that
+				t.Errorf("Should have returned an error in CreateObjMetadata()")
-			// would break ParseObjMetadata.
-			expectedObjMetadata := &ObjMetadata{
-				Namespace: strings.TrimSpace(test.namespace),
-				Name:      strings.TrimSpace(test.name),
-				GroupKind: test.gk,
 			}
-			actual, err := ParseObjMetadata(inv.String())
+		})
-			if err != nil {
-				t.Errorf("Error parsing back ObjMetadata, when it should have worked.")
-			} else if !expectedObjMetadata.Equals(&actual) {
-				t.Errorf("Expected inventory (%s) != parsed inventory (%s)\n", expectedObjMetadata, actual)
-			}
-		}
-		if test.isError && err == nil {
-			t.Errorf("Should have returned an error in CreateObjMetadata()")
-		}
 	}
 }
 
@@ -206,12 +224,12 @@ func TestObjMetadataEquals(t *testing.T) {
 }
 
 func TestParseObjMetadata(t *testing.T) {
-	tests := []struct {
+	tests := map[string]struct {
 		invStr    string
 		inventory *ObjMetadata
 		isError   bool
 	}{
-		{
+		"Simple inventory string parse with empty namespace and whitespace": {
 			invStr: "_test-name_apps_ReplicaSet\t",
 			inventory: &ObjMetadata{
 				Name: "test-name",
@@ -222,7 +240,7 @@ func TestParseObjMetadata(t *testing.T) {
 			},
 			isError: false,
 		},
-		{
+		"Basic inventory string parse": {
 			invStr: "test-namespace_test-name_apps_Deployment",
 			inventory: &ObjMetadata{
 				Namespace: "test-namespace",
@@ -234,32 +252,85 @@ func TestParseObjMetadata(t *testing.T) {
 			},
 			isError: false,
 		},
-		// Not enough fields -- error
+		"RBAC resources can have colon (double underscore) in their name": {
-		{
+			invStr: "test-namespace_kubeadm__nodes-kubeadm-config_rbac.authorization.k8s.io_Role",
+			inventory: &ObjMetadata{
+				Namespace: "test-namespace",
+				Name:      "kubeadm:nodes-kubeadm-config",
+				GroupKind: schema.GroupKind{
+					Group: rbacv1.GroupName,
+					Kind:  "Role",
+				},
+			},
+			isError: false,
+		},
+		"RBAC resources can have double colon (double underscore) in their name": {
+			invStr: "test-namespace_system____leader-locking-kube-scheduler_rbac.authorization.k8s.io_Role",
+			inventory: &ObjMetadata{
+				Namespace: "test-namespace",
+				Name:      "system::leader-locking-kube-scheduler",
+				GroupKind: schema.GroupKind{
+					Group: rbacv1.GroupName,
+					Kind:  "Role",
+				},
+			},
+			isError: false,
+		},
+		"Test double underscore (colon) at beginning of name": {
+			invStr: "test-namespace___leader-locking-kube-scheduler_rbac.authorization.k8s.io_ClusterRole",
+			inventory: &ObjMetadata{
+				Namespace: "test-namespace",
+				Name:      ":leader-locking-kube-scheduler",
+				GroupKind: schema.GroupKind{
+					Group: rbacv1.GroupName,
+					Kind:  "ClusterRole",
+				},
+			},
+			isError: false,
+		},
+		"Test double underscore (colon) at end of name": {
+			invStr: "test-namespace_leader-locking-kube-scheduler___rbac.authorization.k8s.io_RoleBinding",
+			inventory: &ObjMetadata{
+				Namespace: "test-namespace",
+				Name:      "leader-locking-kube-scheduler:",
+				GroupKind: schema.GroupKind{
+					Group: rbacv1.GroupName,
+					Kind:  "RoleBinding",
+				},
+			},
+			isError: false,
+		},
+		"Not enough fields -- error": {
 			invStr:    "_test-name_apps",
 			inventory: &ObjMetadata{},
 			isError:   true,
 		},
-		// Too many fields
+		"Only one field (no separators) -- error": {
-		{
+			invStr:    "test-namespacetest-nametest-grouptest-kind",
+			inventory: &ObjMetadata{},
+			isError:   true,
+		},
+		"Too many fields": {
 			invStr:    "test-namespace_test-name_apps_foo_Deployment",
 			inventory: &ObjMetadata{},
 			isError:   true,
 		},
 	}
 
-	for _, test := range tests {
+	for tn, tc := range tests {
-		actual, err := ParseObjMetadata(test.invStr)
+		t.Run(tn, func(t *testing.T) {
-		if !test.isError {
+			actual, err := ParseObjMetadata(tc.invStr)
-			if err != nil {
+			if !tc.isError {
-				t.Errorf("Error parsing inventory when it should have worked.")
+				if err != nil {
-			} else if !test.inventory.Equals(&actual) {
+					t.Errorf("Error parsing inventory when it should have worked: %s", err)
-				t.Errorf("Expected inventory (%s) != parsed inventory (%s)\n", test.inventory, actual)
+				} else if !tc.inventory.Equals(&actual) {
+					t.Errorf("Expected inventory (%s) != parsed inventory (%s)\n", tc.inventory, actual)
+				}
 			}
-		}
+			if tc.isError && err == nil {
-		if test.isError && err == nil {
+				t.Errorf("Should have returned an error in ParseObjMetadata()")
-			t.Errorf("Should have returned an error in ParseObjMetadata()")
+			}
-		}
+		})
 	}
 }