diff --git a/agent/grpc-external/services/resource/list_by_owner_test.go b/agent/grpc-external/services/resource/list_by_owner_test.go index 19fe799ca..218971a05 100644 --- a/agent/grpc-external/services/resource/list_by_owner_test.go +++ b/agent/grpc-external/services/resource/list_by_owner_test.go @@ -74,7 +74,7 @@ func TestListByOwner_TypeNotRegistered(t *testing.T) { }) require.Error(t, err) require.Equal(t, codes.InvalidArgument.String(), status.Code(err).String()) - require.Contains(t, err.Error(), "resource type demo.v2.artist not registered") + require.Contains(t, err.Error(), "resource type demo.v2.Artist not registered") } func TestListByOwner_Empty(t *testing.T) { @@ -126,7 +126,7 @@ func TestListByOwner_Many(t *testing.T) { } func TestListByOwner_ACL_PerTypeDenied(t *testing.T) { - authz := AuthorizerFrom(t, `key_prefix "resource/demo.v2.album/" { policy = "deny" }`) + authz := AuthorizerFrom(t, `key_prefix "resource/demo.v2.Album/" { policy = "deny" }`) _, rsp, err := roundTripListByOwner(t, authz) // verify resource filtered out, hence no results @@ -135,7 +135,7 @@ func TestListByOwner_ACL_PerTypeDenied(t *testing.T) { } func TestListByOwner_ACL_PerTypeAllowed(t *testing.T) { - authz := AuthorizerFrom(t, `key_prefix "resource/demo.v2.album/" { policy = "read" }`) + authz := AuthorizerFrom(t, `key_prefix "resource/demo.v2.Album/" { policy = "read" }`) album, rsp, err := roundTripListByOwner(t, authz) // verify resource not filtered out diff --git a/agent/grpc-external/services/resource/list_test.go b/agent/grpc-external/services/resource/list_test.go index 7d102b090..4d6b50951 100644 --- a/agent/grpc-external/services/resource/list_test.go +++ b/agent/grpc-external/services/resource/list_test.go @@ -58,7 +58,7 @@ func TestList_TypeNotFound(t *testing.T) { }) require.Error(t, err) require.Equal(t, codes.InvalidArgument.String(), status.Code(err).String()) - require.Contains(t, err.Error(), "resource type demo.v2.artist not registered") + require.Contains(t, err.Error(), "resource type demo.v2.Artist not registered") } func TestList_Empty(t *testing.T) { @@ -178,7 +178,7 @@ func TestList_ACL_ListAllowed_ReadDenied(t *testing.T) { // allow list, deny read authz := AuthorizerFrom(t, demo.ArtistV2ListPolicy, - `key_prefix "resource/demo.v2.artist/" { policy = "deny" }`) + `key_prefix "resource/demo.v2.Artist/" { policy = "deny" }`) _, rsp, err := roundTripList(t, authz) // verify resource filtered out by key:read denied hence no results diff --git a/agent/grpc-external/services/resource/read_test.go b/agent/grpc-external/services/resource/read_test.go index 237895eac..cca911ec1 100644 --- a/agent/grpc-external/services/resource/read_test.go +++ b/agent/grpc-external/services/resource/read_test.go @@ -71,7 +71,7 @@ func TestRead_TypeNotFound(t *testing.T) { _, err = client.Read(context.Background(), &pbresource.ReadRequest{Id: artist.Id}) require.Error(t, err) require.Equal(t, codes.InvalidArgument.String(), status.Code(err).String()) - require.Contains(t, err.Error(), "resource type demo.v2.artist not registered") + require.Contains(t, err.Error(), "resource type demo.v2.Artist not registered") } func TestRead_ResourceNotFound(t *testing.T) { diff --git a/agent/grpc-external/services/resource/watch_test.go b/agent/grpc-external/services/resource/watch_test.go index 687fe0d06..95695f295 100644 --- a/agent/grpc-external/services/resource/watch_test.go +++ b/agent/grpc-external/services/resource/watch_test.go @@ -66,7 +66,7 @@ func TestWatchList_TypeNotFound(t *testing.T) { err = mustGetError(t, rspCh) require.Equal(t, codes.InvalidArgument.String(), status.Code(err).String()) - require.Contains(t, err.Error(), "resource type demo.v2.artist not registered") + require.Contains(t, err.Error(), "resource type demo.v2.Artist not registered") } func TestWatchList_GroupVersionMatches(t *testing.T) { @@ -172,7 +172,7 @@ func TestWatchList_ACL_ListAllowed_ReadDenied(t *testing.T) { // allow list, deny read authz := AuthorizerFrom(t, ` key_prefix "resource/" { policy = "list" } - key_prefix "resource/demo.v2.artist/" { policy = "deny" } + key_prefix "resource/demo.v2.Artist/" { policy = "deny" } `) rspCh, _ := roundTripACL(t, authz) @@ -187,7 +187,7 @@ func TestWatchList_ACL_ListAllowed_ReadAllowed(t *testing.T) { // allow list, allow read authz := AuthorizerFrom(t, ` key_prefix "resource/" { policy = "list" } - key_prefix "resource/demo.v2.artist/" { policy = "read" } + key_prefix "resource/demo.v2.Artist/" { policy = "read" } `) rspCh, artist := roundTripACL(t, authz) diff --git a/agent/grpc-external/services/resource/write_status_test.go b/agent/grpc-external/services/resource/write_status_test.go index f65c7918f..aa2633017 100644 --- a/agent/grpc-external/services/resource/write_status_test.go +++ b/agent/grpc-external/services/resource/write_status_test.go @@ -180,7 +180,7 @@ func TestWriteStatus_TypeNotFound(t *testing.T) { _, err = client.WriteStatus(testContext(t), validWriteStatusRequest(t, res)) require.Error(t, err) require.Equal(t, codes.InvalidArgument.String(), status.Code(err).String()) - require.Contains(t, err.Error(), "resource type demo.v2.artist not registered") + require.Contains(t, err.Error(), "resource type demo.v2.Artist not registered") } func TestWriteStatus_ResourceNotFound(t *testing.T) { diff --git a/agent/grpc-external/services/resource/write_test.go b/agent/grpc-external/services/resource/write_test.go index 3da4ec478..4ec25ee26 100644 --- a/agent/grpc-external/services/resource/write_test.go +++ b/agent/grpc-external/services/resource/write_test.go @@ -151,7 +151,7 @@ func TestWrite_TypeNotFound(t *testing.T) { _, err = client.Write(testContext(t), &pbresource.WriteRequest{Resource: res}) require.Error(t, err) require.Equal(t, codes.InvalidArgument.String(), status.Code(err).String()) - require.Contains(t, err.Error(), "resource type demo.v2.artist not registered") + require.Contains(t, err.Error(), "resource type demo.v2.Artist not registered") } func TestWrite_ACLs(t *testing.T) { diff --git a/internal/controller/api_test.go b/internal/controller/api_test.go index 2006664b2..e80a2d7d7 100644 --- a/internal/controller/api_test.go +++ b/internal/controller/api_test.go @@ -190,7 +190,7 @@ func TestController_String(t *testing.T) { WithPlacement(controller.PlacementEachServer) require.Equal(t, - `, placement="each-server">`, + `, placement="each-server">`, ctrl.String(), ) } @@ -201,7 +201,7 @@ func TestController_NoReconciler(t *testing.T) { ctrl := controller.ForType(demo.TypeV2Artist) require.PanicsWithValue(t, - `cannot register controller without a reconciler , placement="singleton">`, + `cannot register controller without a reconciler , placement="singleton">`, func() { mgr.Register(ctrl) }) } diff --git a/internal/resource/demo/demo.go b/internal/resource/demo/demo.go index 842b75739..20ad89c96 100644 --- a/internal/resource/demo/demo.go +++ b/internal/resource/demo/demo.go @@ -33,36 +33,36 @@ var ( TypeV1Artist = &pbresource.Type{ Group: "demo", GroupVersion: "v1", - Kind: "artist", + Kind: "Artist", } // TypeV1Album represents a collection of an artist's songs. TypeV1Album = &pbresource.Type{ Group: "demo", GroupVersion: "v1", - Kind: "album", + Kind: "Album", } // TypeV2Artist represents a musician or group of musicians. TypeV2Artist = &pbresource.Type{ Group: "demo", GroupVersion: "v2", - Kind: "artist", + Kind: "Artist", } // TypeV2Album represents a collection of an artist's songs. TypeV2Album = &pbresource.Type{ Group: "demo", GroupVersion: "v2", - Kind: "album", + Kind: "Album", } ) const ( - ArtistV1ReadPolicy = `key_prefix "resource/demo.v1.artist/" { policy = "read" }` - ArtistV1WritePolicy = `key_prefix "resource/demo.v1.artist/" { policy = "write" }` - ArtistV2ReadPolicy = `key_prefix "resource/demo.v2.artist/" { policy = "read" }` - ArtistV2WritePolicy = `key_prefix "resource/demo.v2.artist/" { policy = "write" }` + ArtistV1ReadPolicy = `key_prefix "resource/demo.v1.Artist/" { policy = "read" }` + ArtistV1WritePolicy = `key_prefix "resource/demo.v1.Artist/" { policy = "write" }` + ArtistV2ReadPolicy = `key_prefix "resource/demo.v2.Artist/" { policy = "read" }` + ArtistV2WritePolicy = `key_prefix "resource/demo.v2.Artist/" { policy = "write" }` ArtistV2ListPolicy = `key_prefix "resource/" { policy = "list" }` ) diff --git a/internal/resource/registry.go b/internal/resource/registry.go index 232e3998d..0004acfff 100644 --- a/internal/resource/registry.go +++ b/internal/resource/registry.go @@ -5,6 +5,7 @@ package resource import ( "fmt" + "regexp" "sync" "google.golang.org/protobuf/proto" @@ -13,6 +14,12 @@ import ( "github.com/hashicorp/consul/proto-public/pbresource" ) +var ( + groupRegexp = regexp.MustCompile(`^[a-z][a-z\d_]+$`) + groupVersionRegexp = regexp.MustCompile(`^v([a-z\d]+)?\d$`) + kindRegexp = regexp.MustCompile(`^[A-Z][A-Za-z\d]+$`) +) + type Registry interface { // Register the given resource type and its hooks. Register(reg Registration) @@ -82,14 +89,23 @@ func NewRegistry() Registry { } func (r *TypeRegistry) Register(registration Registration) { - r.lock.Lock() - defer r.lock.Unlock() - typ := registration.Type if typ.Group == "" || typ.GroupVersion == "" || typ.Kind == "" { panic("type field(s) cannot be empty") } + switch { + case !groupRegexp.MatchString(typ.Group): + panic(fmt.Sprintf("Type.Group must be in snake_case. Got: %q", typ.Group)) + case !groupVersionRegexp.MatchString(typ.GroupVersion): + panic(fmt.Sprintf("Type.GroupVersion must be lowercase, start with `v`, and end with a number (e.g. `v2` or `v1alpha1`). Got: %q", typ.Group)) + case !kindRegexp.MatchString(typ.Kind): + panic(fmt.Sprintf("Type.Kind must be in PascalCase. Got: %q", typ.Kind)) + } + + r.lock.Lock() + defer r.lock.Unlock() + key := ToGVK(registration.Type) if _, ok := r.registrations[key]; ok { panic(fmt.Sprintf("resource type %s already registered", key)) diff --git a/internal/resource/registry_test.go b/internal/resource/registry_test.go index 7979d618c..c9d177715 100644 --- a/internal/resource/registry_test.go +++ b/internal/resource/registry_test.go @@ -28,36 +28,9 @@ func TestRegister(t *testing.T) { require.True(t, proto.Equal(demo.TypeV2Artist, actual.Type)) // register existing should panic - require.PanicsWithValue(t, "resource type demo.v2.artist already registered", func() { + require.PanicsWithValue(t, "resource type demo.v2.Artist already registered", func() { r.Register(reg) }) - - // type missing required fields should panic - testcases := map[string]*pbresource.Type{ - "empty group": { - Group: "", - GroupVersion: "v2", - Kind: "artist", - }, - "empty group version": { - Group: "", - GroupVersion: "v2", - Kind: "artist", - }, - "empty kind": { - Group: "demo", - GroupVersion: "v2", - Kind: "", - }, - } - - for desc, typ := range testcases { - t.Run(desc, func(t *testing.T) { - require.PanicsWithValue(t, "type field(s) cannot be empty", func() { - r.Register(resource.Registration{Type: typ}) - }) - }) - } } func TestRegister_Defaults(t *testing.T) { @@ -102,7 +75,7 @@ func TestResolve(t *testing.T) { serviceType := &pbresource.Type{ Group: "mesh", GroupVersion: "v1", - Kind: "service", + Kind: "Service", } // not found @@ -115,3 +88,89 @@ func TestResolve(t *testing.T) { assert.True(t, ok) assert.Equal(t, registration.Type, serviceType) } + +func TestRegister_TypeValidation(t *testing.T) { + registry := resource.NewRegistry() + + testCases := map[string]struct { + fn func(*pbresource.Type) + valid bool + }{ + "Valid": {valid: true}, + "Group empty": { + fn: func(t *pbresource.Type) { t.Group = "" }, + valid: false, + }, + "Group PascalCase": { + fn: func(t *pbresource.Type) { t.Group = "Foo" }, + valid: false, + }, + "Group kebab-case": { + fn: func(t *pbresource.Type) { t.Group = "foo-bar" }, + valid: false, + }, + "Group snake_case": { + fn: func(t *pbresource.Type) { t.Group = "foo_bar" }, + valid: true, + }, + "GroupVersion empty": { + fn: func(t *pbresource.Type) { t.GroupVersion = "" }, + valid: false, + }, + "GroupVersion snake_case": { + fn: func(t *pbresource.Type) { t.GroupVersion = "v_1" }, + valid: false, + }, + "GroupVersion kebab-case": { + fn: func(t *pbresource.Type) { t.GroupVersion = "v-1" }, + valid: false, + }, + "GroupVersion no leading v": { + fn: func(t *pbresource.Type) { t.GroupVersion = "1" }, + valid: false, + }, + "GroupVersion no trailing number": { + fn: func(t *pbresource.Type) { t.GroupVersion = "OnePointOh" }, + valid: false, + }, + "Kind PascalCase with numbers": { + fn: func(t *pbresource.Type) { t.Kind = "Number1" }, + valid: true, + }, + "Kind camelCase": { + fn: func(t *pbresource.Type) { t.Kind = "barBaz" }, + valid: false, + }, + "Kind snake_case": { + fn: func(t *pbresource.Type) { t.Kind = "bar_baz" }, + valid: false, + }, + "Kind empty": { + fn: func(t *pbresource.Type) { t.Kind = "" }, + valid: false, + }, + } + for desc, tc := range testCases { + t.Run(desc, func(t *testing.T) { + reg := func() { + typ := &pbresource.Type{ + Group: "foo", + GroupVersion: "v1", + Kind: "Bar", + } + if tc.fn != nil { + tc.fn(typ) + } + registry.Register(resource.Registration{ + Type: typ, + }) + } + + if tc.valid { + require.NotPanics(t, reg) + } else { + require.Panics(t, reg) + } + }) + } +} diff --git a/internal/resource/tombstone.go b/internal/resource/tombstone.go index 289aec2d5..6d0285c60 100644 --- a/internal/resource/tombstone.go +++ b/internal/resource/tombstone.go @@ -6,6 +6,6 @@ var ( TypeV1Tombstone = &pbresource.Type{ Group: "internal", GroupVersion: "v1", - Kind: "tombstone", + Kind: "Tombstone", } )