From 9d0ec2cd7107b62f3dc263887b40fe0b7c44d813 Mon Sep 17 00:00:00 2001 From: Sean Sullivan Date: Mon, 17 Aug 2020 17:17:03 -0700 Subject: [PATCH] Allow colon in the name of RBAC resources --- pkg/inventory/inventory_test.go | 10 -- pkg/object/objmetadata.go | 94 +++++++++++++---- pkg/object/objmetadata_test.go | 181 ++++++++++++++++++++++---------- 3 files changed, 197 insertions(+), 88 deletions(-) diff --git a/pkg/inventory/inventory_test.go b/pkg/inventory/inventory_test.go index 2a1c4561..233f54c7 100644 --- a/pkg/inventory/inventory_test.go +++ b/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, diff --git a/pkg/object/objmetadata.go b/pkg/object/objmetadata.go index 8be511e1..225ffbaa 100644 --- a/pkg/object/objmetadata.go +++ b/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 -// ConfigMap key, but it is not allowed as a character in -// resource name. -const fieldSeparator = "_" +const ( + // Separates inventory fields. This string is allowable as a + // ConfigMap key, but it is not allowed as a character in + // 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 -// any invalid characters; true otherwise. The allowed characters for -// a Kubernetes resource name are: +// validateNameChars returns false if the passed name is not a valid +// resource name; true otherwise. For almost all resources, the following +// 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, -// and returns a pointer to an ObjMetadata struct storing the -// five fields. Example inventory string: +// ParseObjMetadata takes a string, splits it into its four fields, +// and returns an ObjMetadata struct storing the four fields. +// 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) - if len(parts) == 4 { - gk := schema.GroupKind{ - Group: strings.TrimSpace(parts[2]), - Kind: strings.TrimSpace(parts[3]), - } - return CreateObjMetadata(parts[0], parts[1], gk) +// +// NOTE: name field can contain double underscore (__), which represents +// a colon. RBAC resources can have this additional character (:) in their name. +func ParseObjMetadata(s string) (ObjMetadata, error) { + // Parse first field namespace + index := strings.Index(s, fieldSeparator) + if index == -1 { + return ObjMetadata{}, fmt.Errorf("unable to parse stored object metadata: %s", s) + } + 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) } - return ObjMetadata{}, fmt.Errorf("unable to decode inventory: %s", inv) + 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) } diff --git a/pkg/object/objmetadata_test.go b/pkg/object/objmetadata_test.go index f4a0dded..cf6c056b 100644 --- a/pkg/object/objmetadata_test.go +++ b/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 { - inv, err := CreateObjMetadata(test.namespace, test.name, test.gk) - if !test.isError { - if err != nil { - t.Errorf("Error creating ObjMetadata when it should have worked.") - } else if test.expected != inv.String() { - t.Errorf("Expected inventory\n(%s) != created inventory\n(%s)\n", test.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(test.namespace), - Name: strings.TrimSpace(test.name), - GroupKind: test.gk, + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + inv, err := CreateObjMetadata(tc.namespace, tc.name, tc.gk) + if !tc.isError { + if err != nil { + t.Errorf("Error creating ObjMetadata when it should have worked.") + } 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) + } } - 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 tc.isError && err == nil { + t.Errorf("Should have returned an error in CreateObjMetadata()") } - } - 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 { - actual, err := ParseObjMetadata(test.invStr) - if !test.isError { - if err != nil { - t.Errorf("Error parsing inventory when it should have worked.") - } else if !test.inventory.Equals(&actual) { - t.Errorf("Expected inventory (%s) != parsed inventory (%s)\n", test.inventory, actual) + for tn, tc := range tests { + t.Run(tn, func(t *testing.T) { + actual, err := ParseObjMetadata(tc.invStr) + if !tc.isError { + if err != nil { + t.Errorf("Error parsing inventory when it should have worked: %s", err) + } else if !tc.inventory.Equals(&actual) { + t.Errorf("Expected inventory (%s) != parsed inventory (%s)\n", tc.inventory, actual) + } + } + if tc.isError && err == nil { + t.Errorf("Should have returned an error in ParseObjMetadata()") } - } - if test.isError && err == nil { - t.Errorf("Should have returned an error in ParseObjMetadata()") - } + }) } }