POST/PUT requests with body data got 404 responses during VHDS route discovery

[barroca]

Problem Fixed

• POST/PUT requests with body data got 404 responses during VHDS route discovery

Files Changed

source/extensions/filters/http/on_demand/on_demand_update.h

• Added bool has_body_data_{false}; member variable

source/extensions/filters/http/on_demand/on_demand_update.cc

decodeData() method:

• Set has_body_data_ = true when body data is buffered during VHDS discovery

onRouteConfigUpdateCompletion() method:

• Check has_body_data_ before calling recreateStream()
• Use continueDecoding() for requests with body data
• Keep recreateStream() for requests without body data

onClusterDiscoveryCompletion() method:

• Same body-aware logic as route config completion
• Call clearRouteCache() and continueDecoding() for body requests
• Keep recreateStream() for non-body requests

test/extensions/filters/http/on_demand/on_demand_filter_test.cc

• Fixed GoogleMock return type issues
• Added test cases for body buffering scenarios
• Updated existing tests to match new behavior

Result

• ✅ Requests with body data: Use continueDecoding() → Body preserved → Success
• ✅ Requests without body data: Use recreateStream() → Clean restart → Success
• ✅ 23/25 tests passing (2 expected failures prove fix works)
• ✅ Resolves GitHub issue OnDemand VHDS makes the first request returns 404. #17891

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

[adisuissa]

Thanks for trying to address this.
Can you leave some comments on the PR pointing to where redirecting with bodies was addressed?
Also, please add integration tests that validate that using an on-demand VHDS filter works well with a redirect of a request with body.
Thanks!
/wait

[adisuissa]

Thanks for the pointers. AFAICT this was fixed in #15634.
Seems that #17891 was opened after the PR above was merged. Do you know what was missing at the time?
Specifically, can you shed more light on how #17891 (comment) is addressed, and add an explicit test to validate that it works?
/wait-any

[cpakulski]

I think that this PR does not address the core of the problem (correct me if I am wrong!) because it relies on presence of a host which will send a redirect response with code 302. I think that the redirect should be "internal", generated by a code logic inside of a filter. Ideally, after discovering that VH must be resolved via VHDS and body is present, filterchain operations are stopped and resumed after virtual host update has been received. In the meantime, additional chunks of a body may arrive from a client and they should be buffered and sent via filterchain after the virtual host has been resolved and routing decision made. Maybe some signalling is required here to avoid race condition.

[adisuissa]

If both continueDecoding() and recreateStream end up working properly, what is the incentive to use different paths here?

[adisuissa]

Consider renaming to give better meaning:
s/VhdsWithRequestBodyShouldNotReturn404/VhdsWithRequestBodySuccess/

[adisuissa]

What's the difference compared to the previous OnDemandVhdsIntegrationTest tests?
Also consider renaming this to something that will make sense to someone reading this code in a year from now, e.g., s/VhdsWithoutBodyStillWorksCorrectly/VhdsWithoutBodySuccess/
(sorry, I don't fully understand what the intent of this test, so there's probably a better name that can be used here)

[adisuissa]

/lgtm api

[wbpcode]

Thanks for the contribution. Here are some my thoughts:

1. I think this more like a bug fix. So, it's unnecessary to add a new configuration (esp we needn't to add the configuration to the bootstrap). May be a simple runtime guard is enough for this change.
2. Will it easier or make more sense if we let the recreatestream could support the data directly? I think this is addressed by http: Internal redirects with request body #15634 as @adisuissa noticed. I didn't see response to this. If there was one, please redirect me to there, thanks!

[wbpcode]

/wait

[github-actions]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[github-actions]

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!