ext_proc: remove exception throw in ext_proc configuration parsing code

[agrawroh]

Description

This PR refactors the exception throwing logic in the ext_proc filter to use absl::Status returns instead of throwing exceptions.

Fixes #37046

---

Commit Message: ext_proc: remove exception throw in ext_proc configuration parsing code

Additional Description: This change makes the error handling in ext_proc more consistent with other parts of the codebase by using Status returns instead of exceptions. The validation logic remains unchanged.

Risk Level: Low

Testing:

• Added new Unit Tests around config validation
• Existing unit tests modified to verify status returns
• Integration tests remain unchanged as external behavior is the same

Docs Changes: N/A

Release Notes: N/A

[soulxu]

/assgin @mattklein123 @yanavlasov

[agrawroh]

/assign @mattklein123 @yanavlasov

[yanjunxiang-google]

/assign @jmarantz @alyssawilk

[yanjunxiang-google]

HI, @alyssawilk , thanks for reviewing this PR.

Could you please clarify what's the eventual goal about Envoy exception throwing? Are we supposed not to throw any exception during filter configuration parsing, and instead should return a none-ok absl::Status if configuration parsing failed: https://github.com/envoyproxy/envoy/blob/77e0bfc20090ad1e636e20d2060ae562780fd98e/source/extensions/filters/http/ext_proc/config.cc#L14.

If that's the case, then this PR is not achieving the goal yet.

[yanjunxiang-google]

/wait

[alyssawilk]

so the goal is to remove exceptions from core Envoy. Envoy will always support exceptions (and have try-catch) so folks downstream don't need to rewrite filters. That said, Envoy does not support exceptions on the data plane and the exception removal project has found a non-trivial number of (uncaught) exceptions on the data plane, so the fewer exceptions Envoy has and the farther up the stack they are, the less likely uncaught data plane exceptions will be to end up being reintroduced. I think if we get to the point that all of core Envoy has no exceptions it behooves google to remove exceptions from extensions we use, just so we don't need to use them at all, but that project is not staffed or planned.

[yanjunxiang-google]

Thanks @alyssawilk for the clarification.

HI, @agrawroh ,

I think it should be easy to surface the absl::status and return here: https://github.com/envoyproxy/envoy/blob/5a241d32cc89ce9aa492a5820707fd584547d843/source/extensions/filters/http/ext_proc/config.cc#L13 , if the ext_proc configuration is wrong.

What you can do is that instead of checking the configuration in the c'tor of FilterConfig here: https://github.com/envoyproxy/envoy/blob/5a241d32cc89ce9aa492a5820707fd584547d843/source/extensions/filters/http/ext_proc/ext_proc.cc#L265, you can move that configuration check out to here: https://github.com/envoyproxy/envoy/blob/5a241d32cc89ce9aa492a5820707fd584547d843/source/extensions/filters/http/ext_proc/config.cc#L22, i.e, before the c'tor of FilterConfig.

That way, you can easily return a an absl::status for this function: ExternalProcessingFilterConfig::createFilterFactoryFromProtoTyped() if configuration check failed.

You can also move below checks to the same place: https://github.com/envoyproxy/envoy/blob/5a241d32cc89ce9aa492a5820707fd584547d843/source/extensions/filters/http/ext_proc/ext_proc.cc#L266

https://github.com/envoyproxy/envoy/blob/5a241d32cc89ce9aa492a5820707fd584547d843/source/extensions/filters/http/ext_proc/ext_proc.cc#L61

This way, we can largely make the ext_proc filter code not throwing exceptions.

[agrawroh]

/retest

[alyssawilk]

I don't know how this got 5 assignees but seems overkill. As this is pure extension code removing all but tyxia who is codeowner and can approve and merge extension code. @tyxia you can add me back in if you prefer a second pass review

[yanjunxiang-google]

I don't know how this got 5 assignees but seems overkill. As this is pure extension code removing all but tyxia who is codeowner and can approve and merge extension code. @tyxia you can add me back in if you prefer a second pass review

Thanks @alyssawilk for helping review and clarifying some of the questions we have. We will take it from now.

[yanjunxiang-google]

@agrawroh Thanks for your good work!

Only one thing left is that we should call verifyFilterConfig() here: https://github.com/envoyproxy/envoy/blob/64036b42e1581df185d02858d0e5d48b8aaaef97/source/extensions/filters/http/ext_proc/config.cc#L60, and if the configuration is bad, throw an exception to keep the existing behavior. It's a little bit unfortunate createFilterFactoryFromProtoWithServerContextTyped() does not support return an absl::status, and it's caller chain does not throw exception, so ext_proc should throw it here if configuration is bad. Please add a unit test case to cover this code change as well.

[alyssawilk]

oh one more drive by comment - if this actually removes all exceptions rather than most you should be able to remove ext proc from tools/code_format/config.yaml to make sure they aren't reintroduced. If there's still some, no worries

[agrawroh]

/retest

[yanjunxiang-google]

LGTM