xds: fix uint32 overflow scaling rds route runtime_fraction

[nvxbug]

routesProtoToSlice scales a route's runtime_fraction to parts-per-million by multiplying the numerator by 10000 (HUNDRED) or 100 (TEN_THOUSAND) in uint32. The numerator comes from the control plane, so a value like 429497 with a HUNDRED denominator computes 429497*10000 and wraps around to 2704. A route meant to match every request then matches roughly 0.27% of them, and the rest silently fall through to a different route action.

Per review feedback, fractional percent handling is now centralized in a new xdsresource.FractionalPercent type. NewFractionalPercent converts the proto, computes the parts-per-million value in uint64, caps it at 1000000 (100%), and rejects unsupported denominators. The four places that parsed the proto themselves now use it:

• the rds runtime_fraction path stores the precomputed PPM (fixing the overflow above)
• the eds drop policy parser stores a FractionalPercent in OverloadDropConfig
• clusterimpl reads the PPM directly, replacing its local dropRequestsPerMillion
• the fault filter's splitPct delegates to it

RELEASE NOTES:

• xds: fix a uint32 overflow when scaling a route runtime_fraction that could make a route match far fewer RPCs than configured, and centralize FractionalPercent parsing. A route runtime_fraction with an unrecognized denominator now causes the route config to be NACKed.

[codecov]

Codecov Report

❌ Patch coverage is 88.88889% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.05%. Comparing base (bf9e7cd) to head (1fb7ac3).
⚠️ Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/xds/httpfilter/fault/fault.go	50.00%	1 Missing and 1 partial ⚠️
...nternal/xds/xdsclient/xdsresource/unmarshal_rds.go	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9308      +/-   ##
==========================================
- Coverage   83.07%   83.05%   -0.02%     
==========================================
  Files         423      424       +1     
  Lines       35230    35223       -7     
==========================================
- Hits        29266    29255      -11     
+ Misses       4448     4446       -2     
- Partials     1516     1522       +6     

Files with missing lines	Coverage Δ
internal/xds/balancer/clusterimpl/clusterimpl.go	87.88% <100.00%> (-0.21%)	⬇️
...al/xds/xdsclient/xdsresource/fractional_percent.go	100.00% <100.00%> (ø)
...nternal/xds/xdsclient/xdsresource/unmarshal_eds.go	98.63% <100.00%> (+2.41%)	⬆️
internal/xds/httpfilter/fault/fault.go	72.09% <50.00%> (-2.18%)	⬇️
...nternal/xds/xdsclient/xdsresource/unmarshal_rds.go	89.88% <50.00%> (+0.18%)	⬆️

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[easwars]

I see that there are a bunch of places where fractional percent is handled/parsed/used. I'd like to centralize this into a type in the xdsresource package so that we don't have to keep fixing bugs in this.

  package xdsresource

  // FractionalPercent is the internal representation of the xDS FractionalPercent proto.
  type FractionalPercent struct {
      Numerator   uint32
      Denominator uint32
      PPM         uint32 // Pre-computed and capped at 1,000,000
  }
  
  func NewFractionalPercent(fp *v3typepb.FractionalPercent) (FractionalPercent, error) {
      if fp == nil {
          return FractionalPercent{Numerator: 0, Denominator: 100, PPM: 0}, nil
      }
  
      var den uint32
      switch fp.GetDenominator() {
      case v3typepb.FractionalPercent_HUNDRED:
          den = 100
      case v3typepb.FractionalPercent_TEN_THOUSAND:
          den = 10000
      case v3typepb.FractionalPercent_MILLION:
          den = 1000000
      default:
          return FractionalPercent{}, fmt.Errorf("unsupported denominator: %v", fp.GetDenominator())
      }
  
      num := fp.GetNumerator()
      // Perform multiplication in uint64 to prevent overflow
      ppm := uint64(num) * 1000000 / uint64(den)
      if ppm > 1000000 {
          ppm = 1000000 // Cap at 100%
      }
  
      return FractionalPercent{
          Numerator:   num,
          Denominator: den,
          PPM:         uint32(ppm),
      }, nil
  }

Then we can fix usages in the following places:

• internal/xds/xdsclient/xdsresource/unmarshal_rds.go
• internal/xds/xdsclient/xdsresource/unmarshal_eds.go
• internal/xds/balancer/clusterimpl/clusterimpl.go
• internal/xds/httpfilter/fault/fault.go

Please let me know if you are interested in making these changes. Thanks.

[nvxbug]

Done. Added FractionalPercent/NewFractionalPercent to xdsresource pretty much as you sketched it, and moved the four call sites onto it: rds runtime_fraction now stores the precomputed PPM, the eds drop policy carries a FractionalPercent in OverloadDropConfig, clusterimpl uses the PPM directly (dropRequestsPerMillion is gone), and fault's splitPct delegates to it. One behavior change worth flagging: an rds runtime_fraction with an unrecognized denominator now NACKs the route config instead of treating the numerator as parts-per-million, which matches what the eds path already did. I left ext_authz's parseFilterEnabled alone since it wasn't on your list, but can fold it in too if you'd rather. Also rebased onto master while I was at it.

[easwars]

One behavior change worth flagging: an rds runtime_fraction with an unrecognized denominator now NACKs the route config instead of treating the numerator as parts-per-million, which matches what the eds path already did

Thank you for fixing this.

I left ext_authz's parseFilterEnabled alone since it wasn't on your list, but can fold it in too if you'd rather

That would be great if you can handle it too.

[easwars]

Nit: Could you please make the subtest names look more like identifiers (using underscores or dashes instead of spaces would work). See: https://google.github.io/styleguide/go/decisions#subtest-names

Thanks.

[easwars]

Do you mind adding a TODO here to say that this validation should happen at parsing time instead and the filter configuration must be NACKed if an unrecognized denominator is seen.

I don't want to ask you to do this change as part of this PR since it would involve a bunch of refactoring in the fault filter to actually convert the proto into an internal representation at config parsing time (and perform all validation checks) and then only use the internal representation during runtime.

[easwars]

@eshitachandwani Moving it to you for second set of eyes

[eshitachandwani]

Looks good to me but I will wait till the comments from Easwar are resolved before approving and merging.