featureflag: Add more constraint to flag name

Name of a feature flag must contain at least 2 characters. Can only
contain lowercase letters, digits, and '_'. Must start with a letter,
and cannot end with '_'.

Feature flag name would be used to construct the corresponding metadata
key, so:

- Only characters allowed by grpc metadata keys can be used and
  uppercase letters would be normalized to lower, see
  https://pkg.go.dev/google.golang.org/grpc/metadata#New
- It is critical that feature flags don't contain a dash, because the
  client converts dashes to underscores when converting a feature flag's
  name to the metadata key, and vice versa. The name wouldn't round-trip
  in case it had underscores and must thus use dashes instead.

Signed-off-by: blanet <moweng.xx@alibaba-inc.com>
Helped-by: John Cai <jcai@gitlab.com>
Changelog: fixed

2 files changed, 59 insertions, 32 deletions

diff --git a/internal/metadata/featureflag/featureflag.go b/internal/metadata/featureflag/featureflag.go
index e2c91568f..24481d108 100644
--- a/internal/metadata/featureflag/featureflag.go
+++ b/internal/metadata/featureflag/featureflag.go
@@ -3,6 +3,7 @@ package featureflag
 import (
 	"context"
 	"fmt"
+	"regexp"
 	"strconv"
 	"strings"
 
@@ -36,6 +37,18 @@ var (
 	flagsByName = map[string]FeatureFlag{}
 )
 
+// Feature flags must contain at least 2 characters. Can only contain lowercase letters,
+// digits, and '_'. They must start with a letter, and cannot end with '_'.
+// Feature flag name would be used to construct the corresponding metadata key, so:
+//   - Only characters allowed by grpc metadata keys can be used and uppercase letters
+//     would be normalized to lowercase, see
+//     https://pkg.go.dev/google.golang.org/grpc/metadata#New
+//   - It is critical that feature flags don't contain a dash, because the client converts
+//     dashes to underscores when converting a feature flag's name to the metadata key,
+//     and vice versa. The name wouldn't round-trip in case it had underscores and must
+//     thus use dashes instead.
+var ffNameRegexp = regexp.MustCompile(`^[a-z][a-z0-9_]*[a-z0-9]$`)
+
 const (
 	// ffPrefix is the prefix used for Gitaly-scoped feature flags.
 	ffPrefix = "gitaly-feature-"
@@ -64,14 +77,8 @@ type FeatureFlag struct {
 // input that are not used for anything but only for the sake of linking to the feature flag rollout
 // issue in the Gitaly project.
 func NewFeatureFlag(name, version, rolloutIssueURL string, onByDefault bool) FeatureFlag {
-	if strings.ContainsAny(name, "-:") {
-		// It is critical that feature flags don't contain either a dash nor a colon:
-		//
-		// - We convert dashes to underscores when converting a feature flag's name to the
-		//   metadata key, and vice versa. The name wouldn't round-trip in case it had
-		//   underscores and must thus use dashes instead.
-		// - We use colons to separate feature flags and their values.
-		panic("Feature flags must not contain dashes or colons.")
+	if !ffNameRegexp.MatchString(name) {
+		panic("invalid feature flag name.")
 	}
 
 	featureFlag := FeatureFlag{
diff --git a/internal/metadata/featureflag/featureflag_test.go b/internal/metadata/featureflag/featureflag_test.go
index a31205858..b2776d94a 100644
--- a/internal/metadata/featureflag/featureflag_test.go
+++ b/internal/metadata/featureflag/featureflag_test.go
@@ -81,11 +81,51 @@ func TestFeatureFlag_enabled(t *testing.T) {
 		onByDefault bool
 	}{
 		{
-			desc:        "empty name and no headers",
+			desc:        "empty name",
 			flag:        "",
-			headers:     nil,
+			shouldPanic: true,
+		},
+		{
+			desc:        "flag name too short",
+			flag:        "a",
+			shouldPanic: true,
+		},
+		{
+			desc:        "flag name start with number",
+			flag:        "0_flag",
+			shouldPanic: true,
+		},
+		{
+			desc:        "flag name start with underscore",
+			flag:        "_flag",
+			shouldPanic: true,
+		},
+		{
+			desc:        "flag name end with underscore",
+			flag:        "flag_",
+			shouldPanic: true,
+		},
+		{
+			desc:        "flag name with uppercase",
+			flag:        "Flag",
+			shouldPanic: true,
+		},
+		{
+			desc:        "flag name with dashes",
+			flag:        "flag-with-dash",
+			shouldPanic: true,
+		},
+		{
+			desc:        "flag name with characters disallowed by grpc metadata key",
+			flag:        "flag_with_colon:_and_slash/",
+			shouldPanic: true,
+		},
+		{
+			desc:        "flag name with underscores",
+			flag:        "flag_under_score",
+			headers:     map[string]string{"gitaly-feature-flag-under-score": "true"},
 			shouldPanic: false,
-			enabled:     false,
+			enabled:     true,
 			onByDefault: false,
 		},
 		{
@@ -113,26 +153,6 @@ func TestFeatureFlag_enabled(t *testing.T) {
 			onByDefault: false,
 		},
 		{
-			desc:        "flag name with underscores",
-			flag:        "flag_under_score",
-			headers:     map[string]string{"gitaly-feature-flag-under-score": "true"},
-			shouldPanic: false,
-			enabled:     true,
-			onByDefault: false,
-		},
-		{
-			desc:        "flag name with dashes",
-			flag:        "flag-with-dash",
-			headers:     map[string]string{"gitaly-feature-flag-with-dash": "true"},
-			shouldPanic: true,
-		},
-		{
-			desc:        "flag name with colons",
-			flag:        "flag_with_colon:",
-			headers:     map[string]string{"gitaly-feature-flag-with-colon:": "true"},
-			shouldPanic: true,
-		},
-		{
 			desc:        "flag explicitly disabled",
 			flag:        "flag",
 			headers:     map[string]string{"gitaly-feature-flag": "false"},
@@ -155,7 +175,7 @@ func TestFeatureFlag_enabled(t *testing.T) {
 			var ff FeatureFlag
 			ffInitFunc := func() { ff = NewFeatureFlag(tc.flag, "", "", tc.onByDefault) }
 			if tc.shouldPanic {
-				require.PanicsWithValue(t, "Feature flags must not contain dashes or colons.", ffInitFunc)
+				require.PanicsWithValue(t, "invalid feature flag name.", ffInitFunc)
 				return
 			}
 			require.NotPanics(t, ffInitFunc)