Merge branch 'xx/ensure-featureflag-always-in-lowercase' into 'master'

featureflag: Ensure to only contain valid characters

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4793

Merged-by: John Cai <jcai@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Co-authored-by: blanet <moweng.xx@alibaba-inc.com>

2 files changed, 72 insertions, 26 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 f34abfabf..b2776d94a 100644
--- a/internal/metadata/featureflag/featureflag_test.go
+++ b/internal/metadata/featureflag/featureflag_test.go
@@ -76,20 +76,63 @@ func TestFeatureFlag_enabled(t *testing.T) {
 		desc        string
 		flag        string
 		headers     map[string]string
+		shouldPanic bool
 		enabled     bool
 		onByDefault bool
 	}{
 		{
-			desc:        "empty name and no headers",
+			desc:        "empty name",
 			flag:        "",
-			headers:     nil,
-			enabled:     false,
+			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:     true,
 			onByDefault: false,
 		},
 		{
 			desc:        "no headers",
 			flag:        "flag",
 			headers:     nil,
+			shouldPanic: false,
 			enabled:     false,
 			onByDefault: false,
 		},
@@ -97,6 +140,7 @@ func TestFeatureFlag_enabled(t *testing.T) {
 			desc:        "no 'gitaly-feature' prefix in flag name",
 			flag:        "flag",
 			headers:     map[string]string{"flag": "true"},
+			shouldPanic: false,
 			enabled:     false,
 			onByDefault: false,
 		},
@@ -104,27 +148,15 @@ func TestFeatureFlag_enabled(t *testing.T) {
 			desc:        "not valid header value",
 			flag:        "flag",
 			headers:     map[string]string{"gitaly-feature-flag": "TRUE"},
+			shouldPanic: false,
 			enabled:     false,
 			onByDefault: false,
 		},
 		{
-			desc:        "flag name with underscores",
-			flag:        "flag_under_score",
-			headers:     map[string]string{"gitaly-feature-flag-under-score": "true"},
-			enabled:     true,
-			onByDefault: false,
-		},
-		{
-			desc:        "flag name with dashes",
-			flag:        "flag-dash-ok",
-			headers:     map[string]string{"gitaly-feature-flag-dash-ok": "true"},
-			enabled:     true,
-			onByDefault: false,
-		},
-		{
 			desc:        "flag explicitly disabled",
 			flag:        "flag",
 			headers:     map[string]string{"gitaly-feature-flag": "false"},
+			shouldPanic: false,
 			enabled:     false,
 			onByDefault: true,
 		},
@@ -132,6 +164,7 @@ func TestFeatureFlag_enabled(t *testing.T) {
 			desc:        "flag enabled by default but missing",
 			flag:        "flag",
 			headers:     map[string]string{},
+			shouldPanic: false,
 			enabled:     true,
 			onByDefault: true,
 		},
@@ -139,7 +172,13 @@ func TestFeatureFlag_enabled(t *testing.T) {
 		t.Run(tc.desc, func(t *testing.T) {
 			ctx := metadata.NewIncomingContext(createContext(), metadata.New(tc.headers))
 
-			ff := FeatureFlag{tc.flag, tc.onByDefault}
+			var ff FeatureFlag
+			ffInitFunc := func() { ff = NewFeatureFlag(tc.flag, "", "", tc.onByDefault) }
+			if tc.shouldPanic {
+				require.PanicsWithValue(t, "invalid feature flag name.", ffInitFunc)
+				return
+			}
+			require.NotPanics(t, ffInitFunc)
 			require.Equal(t, tc.enabled, ff.IsEnabled(ctx))
 			require.Equal(t, tc.enabled, !ff.IsDisabled(ctx))
 		})