featureflag: Detect use of feature flags which are not set up by tests

When introducing feature flags, then all tests which hit that feature
flag should ideally test the code with both the feature flag enabled and
the feature flag disabled. In the past, there's been many cases though
where this didn't work out: feature flag tests were forgotten, removed
by accident, contexts were overwritten unexpectedly, and other problems
which are easy to make as a developer. The end result is less test
coverage and thus the possibility for breakage.

We can up our game quite easily though: almost all of our tests use
`testhelper.Context()` to construct contexts, which we also use to
automatically inject enabled feature flags. We can piggy-back on this
infrastructure to inject a sanity check into the context, too: if a
certain key is set in the context, then we can make all feature flag
checks fail in case the checked feature flag has not been explicitly
injected into the context.

Implement this check in our feature flags code. `testhelper.Context()`
now starts to inject this sentinel value to always enable strict feature
flag checking for our tests, where our feature sets code knows to mark
injected feature flags as injected. Like this, all tests which hit a
specific feature flag which wasn't injected via feature sets will start
to panic and thus fail the test.

Note that this requires some test changes in the smarthttp package for
tests which depend on the state of all feature flags because Praefect
will set all feature flags which are unset. As a result, tests would
behave differently depending on whether we run them with Praefect or
without them if feature flags weren't all injected explicitly.

1 files changed, 26 insertions, 0 deletions

diff --git a/internal/metadata/featureflag/featureflag.go b/internal/metadata/featureflag/featureflag.go
index ee54ca734..de95e870b 100644
--- a/internal/metadata/featureflag/featureflag.go
+++ b/internal/metadata/featureflag/featureflag.go
@@ -2,6 +2,7 @@ package featureflag
 
 import (
 	"context"
+	"fmt"
 	"strconv"
 	"strings"
 
@@ -31,6 +32,25 @@ var (
 	All = []FeatureFlag{}
 )
 
+const explicitFeatureFlagKey = "require_explicit_feature_flag_checks"
+
+// ContextWithExplicitFeatureFlags marks the context such that all feature flags which are checked
+// must have been explicitly set in that context. If a feature flag wasn't set to an explicit value,
+// then checking this feature flag will panic. This is not for use in production systems, but is
+// intended for tests to verify that we test each feature flag properly.
+func ContextWithExplicitFeatureFlags(ctx context.Context) context.Context {
+	incomingMD, ok := metadata.FromIncomingContext(ctx)
+	if !ok {
+		incomingMD = metadata.New(map[string]string{})
+	}
+	incomingMD.Set(explicitFeatureFlagKey, "true")
+
+	ctx = metadata.NewIncomingContext(ctx, incomingMD)
+	ctx = metadata.AppendToOutgoingContext(ctx, explicitFeatureFlagKey, "true")
+
+	return ctx
+}
+
 // FeatureFlag gates the implementation of new or changed functionality.
 type FeatureFlag struct {
 	// Name is the name of the feature flag.
@@ -59,6 +79,12 @@ func (ff FeatureFlag) IsEnabled(ctx context.Context) bool {
 
 	val, ok := ff.valueFromContext(ctx)
 	if !ok {
+		if md, ok := metadata.FromIncomingContext(ctx); ok {
+			if _, ok := md[explicitFeatureFlagKey]; ok {
+				panic(fmt.Sprintf("checking for feature %q without use of feature sets", ff.Name))
+			}
+		}
+
 		return ff.OnByDefault
 	}