Skip to content

Commit 98f9340

Browse files
committed
xds: shadow the entire non-enforced CSRF remainder
Envoy draws filter_enabled and shadow_enabled independently, so a partial shadow_enabled compounded with filter_enabled instead of partitioning with it: a request selected by neither was left unenforced and unevaluated, never reaching the csrf stats. Enforcement was skewed upwards too, since the filter re-randomizes both predicates at each of its two call sites -- shadowFraction 90 rejected ~17% of invalid requests rather than 10%. Pin shadow_enabled to 100% and let filter_enabled alone carry the rollout. featureEnabled() short-circuits at 100% without touching the PRNG, so the filter's first gate becomes unconditional -- every mutating request reaches the stats block -- and the reject decision reduces to a single filter_enabled draw. shadowFraction 40 now yields exactly 60% enforced and 40% shadowed, with nothing unevaluated. Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
1 parent 20e5a78 commit 98f9340

3 files changed

Lines changed: 16 additions & 13 deletions

File tree

internal/xds/translator/csrf.go

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -144,16 +144,22 @@ func (*csrf) patchResources(*types.ResourceVersionTable, []*ir.HTTPRoute) error
144144

145145
// buildXdsCSRFPolicy builds the full Envoy CSRF policy from the IR.
146146
func buildXdsCSRFPolicy(csrf *ir.CSRF) *csrfv3.CsrfPolicy {
147-
// Envoy only lets an invalid request through in shadow mode if that request wasn't
148-
// also selected by FilterEnabled, so enforcement is set to the complement of the
149-
// shadow fraction: every request is either enforced or observed, never neither.
147+
// Envoy rolls the dice separately for FilterEnabled and ShadowEnabled on every request.
148+
// FilterEnabled decides whether the request is enforced. ShadowEnabled only applies to
149+
// requests that FilterEnabled did not pick, because Envoy checks it as "shadow && !enabled".
150+
//
151+
// So FilterEnabled alone carries the fraction, set to the complement of the shadow
152+
// fraction, while ShadowEnabled stays at 100% to pick up everything FilterEnabled left.
153+
// A shadow fraction of 40% gives FilterEnabled 60%: 60% of requests are enforced and the
154+
// other 40% are checked and recorded in the csrf stats but allowed through, so every
155+
// request lands in one group or the other.
150156
// With no shadow fraction, all requests are enforced.
151157
enforcedFraction := fractionalpercent.FromIn32(100)
152158
var shadowEnabled *corev3.RuntimeFractionalPercent
153159
if csrf.ShadowFraction != nil {
154160
enforcedFraction = fractionalpercent.FromFraction(complementFraction(csrf.ShadowFraction))
155161
shadowEnabled = &corev3.RuntimeFractionalPercent{
156-
DefaultValue: fractionalpercent.FromFraction(csrf.ShadowFraction),
162+
DefaultValue: fractionalpercent.FromIn32(100),
157163
}
158164
}
159165

@@ -164,9 +170,6 @@ func buildXdsCSRFPolicy(csrf *ir.CSRF) *csrfv3.CsrfPolicy {
164170
ShadowEnabled: shadowEnabled,
165171
}
166172

167-
// Values are passed directly to Envoy without transformation. Users must provide
168-
// host:port values (not full URLs) because Envoy's CSRF filter strips the scheme
169-
// from the Origin header before matching.
170173
for _, origin := range csrf.AdditionalOrigins {
171174
policy.AdditionalOrigins = append(policy.AdditionalOrigins, buildXdsStringMatcher(origin))
172175
}

internal/xds/translator/csrf_test.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,9 @@ import (
1717
)
1818

1919
// Envoy only lets an invalid request through in shadow mode if that request wasn't also
20-
// selected by FilterEnabled, so the enforced fraction must be the complement of the
21-
// shadow fraction for shadowing to have any effect.
20+
// selected by FilterEnabled, and it draws the two fractions independently. FilterEnabled
21+
// must therefore be the complement of the shadow fraction, and ShadowEnabled must stay at
22+
// 100% so it claims the whole non-enforced remainder rather than a fraction of it.
2223
func TestBuildXdsCSRFPolicyFractions(t *testing.T) {
2324
tests := []struct {
2425
name string
@@ -41,13 +42,13 @@ func TestBuildXdsCSRFPolicyFractions(t *testing.T) {
4142
name: "partial shadow fraction enforces the remainder",
4243
shadowFraction: &gwapiv1.Fraction{Numerator: 20},
4344
expectedFilter: &xdstype.FractionalPercent{Numerator: 80, Denominator: xdstype.FractionalPercent_HUNDRED},
44-
expectedShadow: &xdstype.FractionalPercent{Numerator: 20, Denominator: xdstype.FractionalPercent_HUNDRED},
45+
expectedShadow: &xdstype.FractionalPercent{Numerator: 100, Denominator: xdstype.FractionalPercent_HUNDRED},
4546
},
4647
{
4748
name: "custom denominator is preserved in the complement",
4849
shadowFraction: &gwapiv1.Fraction{Numerator: 25, Denominator: ptr.To(int32(1000))},
4950
expectedFilter: &xdstype.FractionalPercent{Numerator: 9750, Denominator: xdstype.FractionalPercent_TEN_THOUSAND},
50-
expectedShadow: &xdstype.FractionalPercent{Numerator: 250, Denominator: xdstype.FractionalPercent_TEN_THOUSAND},
51+
expectedShadow: &xdstype.FractionalPercent{Numerator: 100, Denominator: xdstype.FractionalPercent_HUNDRED},
5152
},
5253
}
5354

internal/xds/translator/testdata/out/xds-ir/csrf.routes.yaml

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,5 +41,4 @@
4141
numerator: 9750
4242
shadowEnabled:
4343
defaultValue:
44-
denominator: TEN_THOUSAND
45-
numerator: 250
44+
numerator: 100

0 commit comments

Comments
 (0)