coordinator: Fix feature flags being set twice

In order to guarantee a persistent view of enabled and disabled feature
flags the coordinator makes sure to inject all feature flags that aren't
explicitly set in the gRPC context with their default flags as seen by
Praefect. This fixes a race where the default of any feature flag may
change during an upgrade that can lead to the flag being default-enabled
on some Gitaly nodes, while it's default-disabled on others.

This mechanism has a bug though: we first convert the incoming context
to an outgoing one and thus retain all metadata. We then compute the
full set of feature flags we wish to inject, including already-set ones.
And then finally amend this metadata to the outgoing context. The end
result is that any feature flag that has been explicitly set in the
incoming context will be set twice in the outgoing context.

Fortunately, this bug is benign because we always use the same value in
both cases, and our feature flagging code only ever checks the first
value. So this bug doesn't cause any adverse effects.

Fix it regardless by only setting feature flags in the outgoing context
that haven't yet been set before.

Changelog: fixed

2 files changed, 4 insertions, 13 deletions

diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go
index 82223bfb7..9fbb8171c 100644
--- a/internal/praefect/coordinator.go
+++ b/internal/praefect/coordinator.go
@@ -589,8 +589,6 @@ func (c *Coordinator) maintenanceStreamParameters(ctx context.Context, call grpc
 // streamParametersContexts converts the contexts with incoming metadata into a context that is
 // usable by peer Gitaly nodes.
 func streamParametersContext(ctx context.Context) context.Context {
-	outgoingCtx := metadata.IncomingToOutgoing(ctx)
-
 	// When upgrading Gitaly nodes where the upgrade contains feature flag default changes, then
 	// there will be a window where a subset of Gitaly nodes has a different understanding of
 	// the current default value. If the feature flag wasn't set explicitly on upgrade by
@@ -619,12 +617,14 @@ func streamParametersContext(ctx context.Context) context.Context {
 		rawFeatureFlags = map[string]string{}
 	}
 
+	outgoingCtx := metadata.IncomingToOutgoing(ctx)
 	for _, ff := range featureflag.DefinedFlags() {
 		if _, ok := rawFeatureFlags[ff.MetadataKey()]; !ok {
-			rawFeatureFlags[ff.MetadataKey()] = strconv.FormatBool(ff.OnByDefault)
+			outgoingCtx = grpc_metadata.AppendToOutgoingContext(
+				outgoingCtx, ff.MetadataKey(), strconv.FormatBool(ff.OnByDefault),
+			)
 		}
 	}
-	outgoingCtx = featureflag.OutgoingWithRaw(outgoingCtx, rawFeatureFlags)
 
 	return outgoingCtx
 }
diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go
index e898ba895..38e26a9f0 100644
--- a/internal/praefect/coordinator_test.go
+++ b/internal/praefect/coordinator_test.go
@@ -2803,10 +2803,6 @@ func TestStreamParametersContext(t *testing.T) {
 			),
 			expectedOutgoingMD: metadata.Join(
 				metadataForFlags(expectedFlags()),
-				metadata.Pairs(
-					enabledFF.MetadataKey(), "true",
-					disabledFF.MetadataKey(), "false",
-				),
 			),
 			expectedFlags: expectedFlags(),
 		},
@@ -2827,10 +2823,6 @@ func TestStreamParametersContext(t *testing.T) {
 					expectedFlag{flag: enabledFF, enabled: false},
 					expectedFlag{flag: disabledFF, enabled: true},
 				)),
-				metadata.Pairs(
-					enabledFF.MetadataKey(), "false",
-					disabledFF.MetadataKey(), "true",
-				),
 			),
 			expectedFlags: expectedFlags(
 				expectedFlag{flag: enabledFF, enabled: false},
@@ -2856,7 +2848,6 @@ func TestStreamParametersContext(t *testing.T) {
 					expectedFlag{flag: disabledFF, enabled: true},
 				)),
 				metadata.Pairs(
-					disabledFF.MetadataKey(), "true",
 					"incoming", "value",
 				),
 			),