backport of commit f2e26c3 (#17870)

Co-authored-by: Daniel Upton <daniel@floppy.co>

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

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

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) {

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)
 

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) {

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) {

internal/controller/api_test.go

@@ -190,7 +190,7 @@ func TestController_String(t *testing.T) {
 		WithPlacement(controller.PlacementEachServer)
 
 	require.Equal(t,
-		`<Controller managed_type="demo.v2.artist", watched_types=["demo.v2.album"], backoff=<base="5s", max="1h0m0s">, placement="each-server">`,
+		`<Controller managed_type="demo.v2.Artist", watched_types=["demo.v2.Album"], backoff=<base="5s", max="1h0m0s">, 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 <Controller managed_type="demo.v2.artist", watched_types=[], backoff=<base="5ms", max="16m40s">, placement="singleton">`,
+		`cannot register controller without a reconciler <Controller managed_type="demo.v2.Artist", watched_types=[], backoff=<base="5ms", max="16m40s">, placement="singleton">`,
 		func() { mgr.Register(ctrl) })
 }
 

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" }`
 )
 

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))

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)
+			}
+		})
+	}
+}

internal/resource/tombstone.go

@@ -6,6 +6,6 @@ var (
 	TypeV1Tombstone = &pbresource.Type{
 		Group:        "internal",
 		GroupVersion: "v1",
-		Kind:         "tombstone",
+		Kind:         "Tombstone",
 	}
 )