fix: prevent xdsIR updates from overwriting RateLimit configs from other xdsIR

[sanposhiho]

What type of PR is this?

What this PR does / why we need it:

This PR tries to prevent xdsIR updates from overwriting RateLimit configs from other xdsIR.

When there are multiple gateway classes, the gateway API runner updates r.XdsIR.Store several times with different keys.

gateway/internal/gatewayapi/runner/runner.go

Line 81 in d784c32

for _, resources := range *val {

But, the ratelimit config runner create a ratelimit config from zero every time it gets updates of XdsIR.

gateway/internal/globalratelimit/runner/runner.go

Lines 114 to 136 in d784c32

	message.HandleSubscription(message.Metadata{Runner: string(egv1a1.LogComponentGlobalRateLimitRunner), Message: "xds-ir"}, r.XdsIR.Subscribe(ctx),
	func(update message.Update[string, *ir.Xds], errChan chan error) {
	r.Logger.Info("received a notification")
	if update.Delete {
	if err := r.addNewSnapshot(ctx, nil); err != nil {
	r.Logger.Error(err, "failed to update the config snapshot")
	errChan <- err
	}
	} else {
	// Translate to ratelimit xDS Config.
	rvt, err := r.translate(update.Value)
	if err != nil {
	r.Logger.Error(err, err.Error())
	}
	// Update ratelimit xDS config cache.
	if rvt != nil {
	r.updateSnapshot(ctx, rvt.XdsResources)
	}
	}
	},
	)

As a result, an update to a XdsIR could overwrite rate limit configs generated from other XdsIRs in the ratelimit config runner.

Which issue(s) this PR fixes:

Fixes #3678

[codecov]

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 69.10%. Comparing base (d5fde3e) to head (74281cf).

Files	Patch %	Lines
internal/globalratelimit/runner/runner.go	80.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3771      +/-   ##
==========================================
+ Coverage   68.97%   69.10%   +0.13%     
==========================================
  Files         176      176              
  Lines       21731    21737       +6     
==========================================
+ Hits        14989    15022      +33     
+ Misses       5662     5628      -34     
- Partials     1080     1087       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sanposhiho]

/retest

[arkodg]

thanks for picking this one up @sanposhiho !
the approach looks good, added a comment
can you also add a test in runner_test.go where you instrument some creates and deletes for multiple keys similar to

gateway/internal/gatewayapi/runner/runner_test.go

Line 28 in 1819796

func TestRunner(t *testing.T) {

[sanposhiho]

@arkodg Addressed your points.

[arkodg]

is there a way to not maintain this cache here and instead create an API like rCache := r.getSnapshot(ctx) instead ? this will allow us to reduce one copy in memory

[sanposhiho]

Let me try; depending on the complexity that would give though, that looks like a better idea.

[sanposhiho]

The change would be like this sanposhiho@a9f0a19

So, either way, we have to have a cache to track which update.Key generates which rate limit configurations. (A cache size would be smaller than the current cache of entire configs, that's one good point though.)
The cache is required because otherwise we wouldn't know which part of snapshot we have to update. (A message.Update comes from watchable.Snapshot doesn't have a previous state of ir.)

So, I'm not very motivated to proceed that way; For me, it looks like only a little gain (less memory usage) would come with a certain downside (complexity).

[arkodg]

hey @sanposhiho thanks for writing up the quick diff, what I was suggesting was a little different, instead of holding the entire cache on the stack using rateLimitConfigsCache which incurs a constant memory hit, can we retrieve the cache snapshot using an API like
https://github.com/envoyproxy/go-control-plane/blob/1da4500d00e270d803caefbe0c20e4d3d162e586/pkg/cache/v3/snapshot.go#L124
every time we get a new new message, and then update it (add or delete key based on type of update) ?

[sanposhiho]

Sorry, what's the difference from my diff sanposhiho@a9f0a19? It fetches the previous snapshot with GetSnapshot -> GetResources, and try to rebuild a new snapshot from there already.

My point is that we, either way, need keyToRateLimitCfg cache to know which ir.Key has generated which part of snapshot so that we can calculate a correct snapshot after update.

[arkodg]

I missed that 🙈
lets go ahead with diff to keep the code complexity low and we can revisit these if/when we hit a memory bottleneck

[arkodg]

LGTM thanks !