Repository navigation
egress: return a retryable 503 instead of a 504 when a credential provider is slow - #2787
Conversation
a hung provider call ran into the ext_proc message_timeout and came back as a 504, same cap as the policy fetch so it's a retryable 503
Benjamin Elder (BenTheElder)
added
kind/bug
Abhishek Sharma (abhisheksharma2411)
left a comment
Abhishek Sharma (abhisheksharma2411)
left a comment
There was a problem hiding this comment.
Makes sense, and wrapping the whole injections loop rather than each FetchSecret looks deliberately right - the ext_proc budget is per message, not per call, so a shared budget is the thing that actually has to fit.
One thing I wondered about while reading. injectionTimeout is 4s against defaultExtProcMessageTimeout of 5s, which matches policyFetchTimeout. But the message timeout isn't always the default: dataplane.go:77 calls
xdsSrv.SetExtProcMessageTimeout(parkCfg.Budget + 5*time.Second)when parking is enabled, so the effective timeout grows with the park budget while these two constants stay at 4s. In that configuration a credential fetch that takes, say, 6s gets cut off at 4s and answered 503, where Envoy would have waited. Retryable rather than broken, so not a big deal - but it's the mirror of the flake you're fixing.
The repo already has the derive-rather-than-hardcode shape for this: drainTimeoutMargin exists specifically to mirror "the +5s Envoy ext_proc MessageTimeout margin". Would deriving both fetch timeouts from the effective message timeout be worth a follow-up? policyFetchTimeout has the same coupling, so this PR isn't making anything worse either way.
|
Abhishek Sharma (@abhisheksharma2411) Thanks for checking, but I don't think this applies to egress. SetExtProcMessageTimeout is on the xDS server, which only configures the ingress envoy. An egress-only atenet doesn't run it: substrate/cmd/atenet/internal/router/router.go Lines 274 to 281 in cc3354c The egress envoy has message_timeout: 5s in its static config, and parking doesn't apply there: So the real coupling is the yaml vs the two go constants. We can't really derive one from the other. |
|
You're right, and I should have checked that rather than assuming the xDS path applied here. // The ingress control plane — the xDS server — configures the *ingress*
// dataplane. The egress gateway is statically configured, so an egress-only
// instance does not run it.
if s.cfg.Mode.ServesIngress() {So Agreed the real coupling is the yaml against the two Go constants, and that isn't something you can derive. Nothing blocking from me. |
fixes a flake in TestActorEgressCredentialInjection/https on the envoy lane:
https://github.com/agent-substrate/substrate/actions/runs/38982841766/job/117247440573
The test restarts k8s-credential-provider and then makes the first injected request. The gateway's FetchSecret to the new pod had no deadline, so a slow first call ran past the ext_proc message_timeout (5s) and envoy answered 504 with an empty body (failed at 5.18s). The test already retries 503 while the gateway reconnects to the provider, but not 504.
The policy fetch already handles this by staying under the message timeout (4s):
substrate/cmd/atenet/internal/router/egress/policycache.go
Lines 38 to 41 in cc3354c
This puts the same 4s cap on resolving a request's credentials (provider fetch and actor JWT mint), so a hung provider is the documented retryable 503.
agentgateway has no outer message timeout, so it waits on a slow provider instead of returning 504 and doesn't hit this, otherwise I would've proposed alignment.
AI-assisted.