diff options
author | blanet <moweng.xx@alibaba-inc.com> | 2022-08-08 07:05:14 +0300 |
---|---|---|
committer | blanet <moweng.xx@alibaba-inc.com> | 2022-09-05 05:46:46 +0300 |
commit | b619bfb798e6ad9924f82dbe337a1ac6133a1fd6 (patch) | |
tree | 72bb7f7e5034f0127488b5f5ff7df01c176ebbc8 | |
parent | fc3329a6bf32398958d20f3815d1f67fe70dc154 (diff) |
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
-rw-r--r-- | internal/metadata/featureflag/featureflag.go | 23 | ||||
-rw-r--r-- | internal/metadata/featureflag/featureflag_test.go | 68 |
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) |