From 274bd08afc4a037b36d5701a2c65a86e180ec183 Mon Sep 17 00:00:00 2001 From: Todd Treece <360020+toddtreece@users.noreply.github.com> Date: Wed, 3 Jul 2024 18:11:45 -0400 Subject: [PATCH] K8s: Improve key generation and parsing (#90014) --- pkg/apiserver/go.mod | 6 +- pkg/apiserver/registry/generic/key.go | 92 ++++++++------ pkg/apiserver/registry/generic/key_test.go | 120 ++++++++++++++++++ .../apiserver/storage/entity/utils_test.go | 4 +- .../sqlstash/testdata/grpc-req-create.json | 2 +- .../sqlstash/testdata/grpc-req-delete.json | 2 +- .../sqlstash/testdata/grpc-req-update.json | 2 +- .../sqlstash/testdata/grpc-res-entity.json | 2 +- 8 files changed, 181 insertions(+), 49 deletions(-) create mode 100644 pkg/apiserver/registry/generic/key_test.go diff --git a/pkg/apiserver/go.mod b/pkg/apiserver/go.mod index 828015a3f06..74055d945c6 100644 --- a/pkg/apiserver/go.mod +++ b/pkg/apiserver/go.mod @@ -4,6 +4,7 @@ go 1.21.10 require ( github.com/bwmarrin/snowflake v0.3.0 + github.com/google/go-cmp v0.6.0 github.com/grafana/grafana/pkg/apimachinery v0.0.0-20240701135906-559738ce6ae1 github.com/prometheus/client_golang v1.19.0 github.com/stretchr/testify v1.9.0 @@ -11,7 +12,9 @@ require ( k8s.io/apimachinery v0.29.3 k8s.io/apiserver v0.29.2 k8s.io/client-go v0.29.3 + k8s.io/component-base v0.29.2 k8s.io/klog/v2 v2.120.1 + k8s.io/utils v0.0.0-20230726121419-3b25d923346b ) require ( @@ -34,7 +37,6 @@ require ( github.com/golang/protobuf v1.5.4 // indirect github.com/google/btree v1.1.2 // indirect github.com/google/gnostic-models v0.6.8 // indirect - github.com/google/go-cmp v0.6.0 // indirect github.com/google/gofuzz v1.2.0 // indirect github.com/google/pprof v0.0.0-20240416155748-26353dc0451f // indirect github.com/google/uuid v1.6.0 // indirect @@ -84,9 +86,7 @@ require ( gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect k8s.io/api v0.29.3 // indirect - k8s.io/component-base v0.29.2 // indirect k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 // indirect - k8s.io/utils v0.0.0-20230726121419-3b25d923346b // indirect sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.28.0 // indirect sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect diff --git a/pkg/apiserver/registry/generic/key.go b/pkg/apiserver/registry/generic/key.go index 35ce83a6b35..f257cc03894 100644 --- a/pkg/apiserver/registry/generic/key.go +++ b/pkg/apiserver/registry/generic/key.go @@ -12,63 +12,75 @@ import ( ) type Key struct { - Group string - Resource string - Namespace string - Name string + Group string `json:"group,omitempty"` + Resource string `json:"resource"` + Namespace string `json:"namespace,omitempty"` + Name string `json:"name,omitempty"` } -func ParseKey(key string) (*Key, error) { - // //[/namespaces/][/] - parts := strings.Split(key, "/") - if len(parts) < 3 { - return nil, fmt.Errorf("invalid key (expecting at least 2 parts): %s", key) +// ParseKey parses a key string into a Key. +// Format: [/group/]/resource/[/namespace/][/name/] +func ParseKey(raw string) (*Key, error) { + parts := strings.Split(raw, "/") + key := &Key{} + + // Skip the first empty string + if parts[0] == "" { + parts = parts[1:] } - if parts[0] != "" { - return nil, fmt.Errorf("invalid key (expecting leading slash): %s", key) + for i := 0; i < len(parts); i += 2 { + k := parts[i] + if i+1 >= len(parts) { + return nil, fmt.Errorf("invalid key: %s", raw) + } + v := parts[i+1] + switch k { + case "group": + key.Group = v + case "resource": + key.Resource = v + case "namespace": + key.Namespace = v + case "name": + key.Name = v + default: + return nil, fmt.Errorf("invalid key name: %s", key) + } } - k := &Key{ - Group: parts[1], - Resource: parts[2], + if len(key.Resource) == 0 { + return nil, fmt.Errorf("missing resource: %s", raw) } - if len(parts) == 3 { - return k, nil - } - - if parts[3] != "namespaces" { - k.Name = parts[3] - return k, nil - } - - if len(parts) < 5 { - return nil, fmt.Errorf("invalid key (expecting namespace after 'namespaces'): %s", key) - } - - k.Namespace = parts[4] - - if len(parts) == 5 { - return k, nil - } - - k.Name = parts[5] - - return k, nil + return key, nil } +// String returns the string representation of the Key. func (k *Key) String() string { - s := "/" + k.Group + "/" + k.Resource + var builder strings.Builder + + if len(k.Group) > 0 { + builder.WriteString("/group/") + builder.WriteString(k.Group) + } + if len(k.Resource) > 0 { + builder.WriteString("/resource/") + builder.WriteString(k.Resource) + } if len(k.Namespace) > 0 { - s += "/namespaces/" + k.Namespace + builder.WriteString("/namespace/") + builder.WriteString(k.Namespace) } if len(k.Name) > 0 { - s += "/" + k.Name + builder.WriteString("/name/") + builder.WriteString(k.Name) } - return s + + return builder.String() } +// IsEqual returns true if the keys are equal. func (k *Key) IsEqual(other *Key) bool { return k.Group == other.Group && k.Resource == other.Resource && diff --git a/pkg/apiserver/registry/generic/key_test.go b/pkg/apiserver/registry/generic/key_test.go new file mode 100644 index 00000000000..c000bd58784 --- /dev/null +++ b/pkg/apiserver/registry/generic/key_test.go @@ -0,0 +1,120 @@ +package generic + +import ( + "reflect" + "testing" +) + +func TestParseKey(t *testing.T) { + tests := []struct { + name string + raw string + expected *Key + wantErr bool + }{ + { + name: "All keys", + raw: "/group/test-group/resource/test-resource/namespace/test-namespace/name/test-name", + expected: &Key{Group: "test-group", Resource: "test-resource", Namespace: "test-namespace", Name: "test-name"}, + wantErr: false, + }, + { + name: "Missing group", + raw: "/resource/test-resource/namespace/test-namespace/name/test-name", + expected: &Key{Group: "", Resource: "test-resource", Namespace: "test-namespace", Name: "test-name"}, + wantErr: false, + }, + { + name: "Missing namespace", + raw: "/group/test-group/resource/test-resource/name/test-name", + expected: &Key{Group: "test-group", Resource: "test-resource", Namespace: "", Name: "test-name"}, + wantErr: false, + }, + { + name: "Missing name", + raw: "/group/test-group/resource/test-resource/namespace/test-namespace", + expected: &Key{Group: "test-group", Resource: "test-resource", Namespace: "test-namespace", Name: ""}, + wantErr: false, + }, + { + name: "Missing resource", + raw: "/group/test-group/namespace/test-namespace/name/test-name", + expected: nil, + wantErr: true, + }, + { + name: "Empty string", + raw: "", + expected: nil, + wantErr: true, + }, + { + name: "Invalid key", + raw: "/", + expected: nil, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ParseKey(tt.raw) + if (err != nil) != tt.wantErr { + t.Errorf("ParseKey() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.expected) { + t.Errorf("ParseKey() = %v, expected %v", got, tt.expected) + } + }) + } +} + +func BenchmarkKey_String(b *testing.B) { + key := &Key{Group: "test-group", Resource: "test-resource", Namespace: "test-namespace", Name: "test-name"} + for i := 0; i < b.N; i++ { + _ = key.String() + } +} +func TestKey_String(t *testing.T) { + tests := []struct { + name string + key *Key + expected string + }{ + { + name: "All fields", + key: &Key{Group: "test-group", Resource: "test-resource", Namespace: "test-namespace", Name: "test-name"}, + expected: "/group/test-group/resource/test-resource/namespace/test-namespace/name/test-name", + }, + { + name: "Missing group", + key: &Key{Resource: "test-resource", Namespace: "test-namespace", Name: "test-name"}, + expected: "/resource/test-resource/namespace/test-namespace/name/test-name", + }, + { + name: "Missing namespace", + key: &Key{Group: "test-group", Resource: "test-resource", Name: "test-name"}, + expected: "/group/test-group/resource/test-resource/name/test-name", + }, + { + name: "Missing name", + key: &Key{Group: "test-group", Resource: "test-resource", Namespace: "test-namespace"}, + expected: "/group/test-group/resource/test-resource/namespace/test-namespace", + }, + { + name: "Missing resource", + key: &Key{Group: "test-group", Namespace: "test-namespace", Name: "test-name"}, + expected: "/group/test-group/namespace/test-namespace/name/test-name", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.key.String() + if got != tt.expected { + t.Errorf("Key.String() = %s, expected %s", got, tt.expected) + } + }) + } +} diff --git a/pkg/services/apiserver/storage/entity/utils_test.go b/pkg/services/apiserver/storage/entity/utils_test.go index 950beec6012..5136b6a44d6 100644 --- a/pkg/services/apiserver/storage/entity/utils_test.go +++ b/pkg/services/apiserver/storage/entity/utils_test.go @@ -86,7 +86,7 @@ func TestResourceToEntity(t *testing.T) { }, }, }, - expectedKey: "/playlist.grafana.app/playlists/namespaces/default/test-name", + expectedKey: "/group/playlist.grafana.app/resource/playlists/namespace/default/name/test-name", expectedGroupVersion: "v0alpha1", expectedName: "test-name", expectedNamespace: "default", @@ -157,7 +157,7 @@ func TestEntityToResource(t *testing.T) { }{ { entity: &entityStore.Entity{ - Key: "/playlist.grafana.app/playlists/namespaces/default/test-uid", + Key: "/group/playlist.grafana.app/resource/playlists/namespaces/default/name/test-uid", GroupVersion: "v0alpha1", Name: "test-uid", Title: "A playlist", diff --git a/pkg/services/store/entity/sqlstash/testdata/grpc-req-create.json b/pkg/services/store/entity/sqlstash/testdata/grpc-req-create.json index 27a8fd2ca07..ae7c2a0125b 100644 --- a/pkg/services/store/entity/sqlstash/testdata/grpc-req-create.json +++ b/pkg/services/store/entity/sqlstash/testdata/grpc-req-create.json @@ -6,7 +6,7 @@ "namespace": "default", "name": "adnj1llchbbi8a", "group_version": "v0alpha1", - "key": "/playlist.grafana.app/playlists/namespaces/default/adnj1llchbbi8a", + "key": "/group/playlist.grafana.app/resource/playlists/namespaces/default/name/adnj1llchbbi8a", "meta": "eyJtZXRhZGF0YSI6eyJuYW1lIjoiYWRuajFsbGNoYmJpOGEiLCJuYW1lc3BhY2UiOiJkZWZhdWx0IiwidWlkIjoiYjAxOTljNjAtNWYzYS00MWJlLTliYTYtN2E1MmYxZGU4M2ZmIiwiY3JlYXRpb25UaW1lc3RhbXAiOiIyMDI0LTA2LTAyVDAzOjI4OjE3WiIsImFubm90YXRpb25zIjp7ImdyYWZhbmEuYXBwL29yaWdpbktleSI6IjIiLCJncmFmYW5hLmFwcC9vcmlnaW5OYW1lIjoiU1FMIiwiZ3JhZmFuYS5hcHAvb3JpZ2luVGltZXN0YW1wIjoiMjAyNC0wNi0wMlQwMzoyODoxN1oiLCJncmFmYW5hLmFwcC91cGRhdGVkVGltZXN0YW1wIjoiMjAyNC0wNi0wMlQwMzoyODoxN1oifX19", "body": "eyJraW5kIjoiUGxheWxpc3QiLCJhcGlWZXJzaW9uIjoicGxheWxpc3QuZ3JhZmFuYS5hcHAvdjBhbHBoYTEiLCJtZXRhZGF0YSI6eyJuYW1lIjoiYWRuajFsbGNoYmJpOGEiLCJuYW1lc3BhY2UiOiJkZWZhdWx0IiwidWlkIjoiYjAxOTljNjAtNWYzYS00MWJlLTliYTYtN2E1MmYxZGU4M2ZmIiwiY3JlYXRpb25UaW1lc3RhbXAiOiIyMDI0LTA2LTAyVDAzOjI4OjE3WiIsImFubm90YXRpb25zIjp7ImdyYWZhbmEuYXBwL29yaWdpbktleSI6IjIiLCJncmFmYW5hLmFwcC9vcmlnaW5OYW1lIjoiU1FMIiwiZ3JhZmFuYS5hcHAvb3JpZ2luVGltZXN0YW1wIjoiMjAyNC0wNi0wMlQwMzoyODoxN1oiLCJncmFmYW5hLmFwcC91cGRhdGVkVGltZXN0YW1wIjoiMjAyNC0wNi0wMlQwMzoyODoxN1oifX0sInNwZWMiOnsidGl0bGUiOiJ0ZXN0IHBsYXlsaXN0IiwiaW50ZXJ2YWwiOiI1bSIsIml0ZW1zIjpbeyJ0eXBlIjoiZGFzaGJvYXJkX2J5X3VpZCIsInZhbHVlIjoiY2RuaXY1M2dtZDR3MGUifV19fQo=", "title": "test playlist", diff --git a/pkg/services/store/entity/sqlstash/testdata/grpc-req-delete.json b/pkg/services/store/entity/sqlstash/testdata/grpc-req-delete.json index 4802693b534..a3a593b17d3 100644 --- a/pkg/services/store/entity/sqlstash/testdata/grpc-req-delete.json +++ b/pkg/services/store/entity/sqlstash/testdata/grpc-req-delete.json @@ -1,3 +1,3 @@ { - "key": "/playlist.grafana.app/playlists/namespaces/default/sdfsdfsdf" + "key": "/group/playlist.grafana.app/resource/playlists/namespaces/default/name/sdfsdfsdf" } diff --git a/pkg/services/store/entity/sqlstash/testdata/grpc-req-update.json b/pkg/services/store/entity/sqlstash/testdata/grpc-req-update.json index a94dabcb233..5b93ee07546 100644 --- a/pkg/services/store/entity/sqlstash/testdata/grpc-req-update.json +++ b/pkg/services/store/entity/sqlstash/testdata/grpc-req-update.json @@ -7,7 +7,7 @@ "namespace": "default", "name": "sdfsdfsdf", "group_version": "v0alpha1", - "key": "/playlist.grafana.app/playlists/namespaces/default/sdfsdfsdf", + "key": "/group/playlist.grafana.app/resource/playlists/namespaces/default/name/sdfsdfsdf", "meta": "eyJtZXRhZGF0YSI6eyJuYW1lIjoic2Rmc2Rmc2RmIiwibmFtZXNwYWNlIjoiZGVmYXVsdCIsInVpZCI6IjNjNzY5YjJlLWFhYTctNDZmNi1hYjgzLWUwMzgwNTBhNmE3NSIsInJlc291cmNlVmVyc2lvbiI6IjEiLCJjcmVhdGlvblRpbWVzdGFtcCI6IjIwMjQtMDYtMDJUMDM6NDk6MjlaIiwibWFuYWdlZEZpZWxkcyI6W3sibWFuYWdlciI6Ik1vemlsbGEiLCJvcGVyYXRpb24iOiJVcGRhdGUiLCJhcGlWZXJzaW9uIjoicGxheWxpc3QuZ3JhZmFuYS5hcHAvdjBhbHBoYTEiLCJ0aW1lIjoiMjAyNC0wNi0wMlQwMzo1Mzo1NVoiLCJmaWVsZHNUeXBlIjoiRmllbGRzVjEiLCJmaWVsZHNWMSI6eyJmOnNwZWMiOnsiZjppbnRlcnZhbCI6e30sImY6aXRlbXMiOnt9LCJmOnRpdGxlIjp7fX19fV19fQ==", "body": "eyJraW5kIjoiUGxheWxpc3QiLCJhcGlWZXJzaW9uIjoicGxheWxpc3QuZ3JhZmFuYS5hcHAvdjBhbHBoYTEiLCJtZXRhZGF0YSI6eyJuYW1lIjoic2Rmc2Rmc2RmIiwibmFtZXNwYWNlIjoiZGVmYXVsdCIsInVpZCI6IjNjNzY5YjJlLWFhYTctNDZmNi1hYjgzLWUwMzgwNTBhNmE3NSIsInJlc291cmNlVmVyc2lvbiI6IjEiLCJjcmVhdGlvblRpbWVzdGFtcCI6IjIwMjQtMDYtMDJUMDM6NDk6MjlaIiwibWFuYWdlZEZpZWxkcyI6W3sibWFuYWdlciI6Ik1vemlsbGEiLCJvcGVyYXRpb24iOiJVcGRhdGUiLCJhcGlWZXJzaW9uIjoicGxheWxpc3QuZ3JhZmFuYS5hcHAvdjBhbHBoYTEiLCJ0aW1lIjoiMjAyNC0wNi0wMlQwMzo1Mzo1NVoiLCJmaWVsZHNUeXBlIjoiRmllbGRzVjEiLCJmaWVsZHNWMSI6eyJmOnNwZWMiOnsiZjppbnRlcnZhbCI6e30sImY6aXRlbXMiOnt9LCJmOnRpdGxlIjp7fX19fV19LCJzcGVjIjp7InRpdGxlIjoieHpjdnp4Y3Zxd2Vxd2UiLCJpbnRlcnZhbCI6IjVtIiwiaXRlbXMiOlt7InR5cGUiOiJkYXNoYm9hcmRfYnlfdWlkIiwidmFsdWUiOiJjZG5pdjUzZ21kNHcwZSJ9XX19Cg==", "title": "xzcvzxcvqweqwe", diff --git a/pkg/services/store/entity/sqlstash/testdata/grpc-res-entity.json b/pkg/services/store/entity/sqlstash/testdata/grpc-res-entity.json index b0bf49ededf..438e1292259 100644 --- a/pkg/services/store/entity/sqlstash/testdata/grpc-res-entity.json +++ b/pkg/services/store/entity/sqlstash/testdata/grpc-res-entity.json @@ -6,7 +6,7 @@ "namespace": "default", "name": "sdfsdfsdf", "group_version": "v0alpha1", - "key": "/playlist.grafana.app/playlists/namespaces/default/sdfsdfsdf", + "key": "/group/playlist.grafana.app/resource/playlists/namespace/default/name/sdfsdfsdf", "meta": "eyJtZXRhZGF0YSI6eyJuYW1lIjoic2Rmc2Rmc2RmIiwibmFtZXNwYWNlIjoiZGVmYXVsdCIsInVpZCI6IjAyZmVhOGVlLTk2ZDYtNGIzMy04ZGI5LTU5MmI0NzU4NTM4NSIsImNyZWF0aW9uVGltZXN0YW1wIjoiMjAyNC0wNi0wNFQxNToxODozNFoiLCJtYW5hZ2VkRmllbGRzIjpbeyJtYW5hZ2VyIjoiTW96aWxsYSIsIm9wZXJhdGlvbiI6IlVwZGF0ZSIsImFwaVZlcnNpb24iOiJwbGF5bGlzdC5ncmFmYW5hLmFwcC92MGFscGhhMSIsInRpbWUiOiIyMDI0LTA2LTA0VDE1OjE4OjM0WiIsImZpZWxkc1R5cGUiOiJGaWVsZHNWMSIsImZpZWxkc1YxIjp7ImY6c3BlYyI6eyJmOmludGVydmFsIjp7fSwiZjppdGVtcyI6e30sImY6dGl0bGUiOnt9fX19XX19", "body": "eyJraW5kIjoiUGxheWxpc3QiLCJhcGlWZXJzaW9uIjoicGxheWxpc3QuZ3JhZmFuYS5hcHAvdjBhbHBoYTEiLCJtZXRhZGF0YSI6eyJuYW1lIjoic2Rmc2Rmc2RmIiwibmFtZXNwYWNlIjoiZGVmYXVsdCIsInVpZCI6IjAyZmVhOGVlLTk2ZDYtNGIzMy04ZGI5LTU5MmI0NzU4NTM4NSIsImNyZWF0aW9uVGltZXN0YW1wIjoiMjAyNC0wNi0wNFQxNToxODozNFoiLCJtYW5hZ2VkRmllbGRzIjpbeyJtYW5hZ2VyIjoiTW96aWxsYSIsIm9wZXJhdGlvbiI6IlVwZGF0ZSIsImFwaVZlcnNpb24iOiJwbGF5bGlzdC5ncmFmYW5hLmFwcC92MGFscGhhMSIsInRpbWUiOiIyMDI0LTA2LTA0VDE1OjE4OjM0WiIsImZpZWxkc1R5cGUiOiJGaWVsZHNWMSIsImZpZWxkc1YxIjp7ImY6c3BlYyI6eyJmOmludGVydmFsIjp7fSwiZjppdGVtcyI6e30sImY6dGl0bGUiOnt9fX19XX0sInNwZWMiOnsidGl0bGUiOiJ4emN2enhjdiIsImludGVydmFsIjoiNW0iLCJpdGVtcyI6W3sidHlwZSI6ImRhc2hib2FyZF9ieV91aWQiLCJ2YWx1ZSI6ImNkbml2NTNnbWQ0dzBlIn1dfX0K", "title": "xzcvzxcv",