Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ABI vNEXT: generic vs specialized context functions #9

Open
PiotrSikora opened this issue Aug 18, 2020 · 3 comments
Open

ABI vNEXT: generic vs specialized context functions #9

PiotrSikora opened this issue Aug 18, 2020 · 3 comments

Comments

@PiotrSikora
Copy link
Contributor

PiotrSikora commented Aug 18, 2020

The current version of the ABI defines generic proxy_on_context_create() and proxy_on_context_finalize() functions which are used for VM and plugin lifecycle, as well as for per-stream lifecycle, which makes "context" a bit overloaded and confusing.

For VM context:

proxy_on_context_create(VmContext, ...)
proxy_on_vm_start(...)
proxy_on_context_finalize(...)

For plugin context:

proxy_on_context_create(PluginContext, ...)
proxy_on_plugin_start(...)
proxy_on_context_finalize(...)

For HTTP per-stream context:

proxy_on_context_create(HttpContext, ...)
proxy_on_http_request_headers(...)
proxy_on_http_request_body(...)
proxy_on_http_response_headers(...)
proxy_on_http_response_body(...)
proxy_on_context_finalize(...)

Instead, we could have specialized "finalize" functions for each context type, and create context implicitly as part of each "start" function:

For VM context:

proxy_on_vm_start(...)
proxy_on_vm_shutdown(...)

For plugin context:

proxy_on_plugin_start(...)
proxy_on_plugin_shutdown(...)

For HTTP per-stream context:

proxy_on_http_request_headers(...)
proxy_on_http_request_body(...)
proxy_on_http_response_headers(...)
proxy_on_http_response_body(...)
proxy_on_http_finalize(...)

In case proxy_on_http_finalize() returns Action::Pause, the host would wait until proxy_resume() is called before calling proxy_on_http_finalize() again (which matches the behavior of other callbacks that have the ability to pause/resume processing).

@mattklein123 @htuch @yskopets @yuval-k @gbrail @jplevyak @kyessenov any thoughts?

@htuch
Copy link

htuch commented Aug 26, 2020

Seems a slam dunk interface improvement.

@mattklein123
Copy link
Collaborator

Huge +1 from me. I think this is one of the things that I initially commented on.

@kyessenov
Copy link

+1 as a user. Using the same callback is really problematic when one plugin serves in different roles (which might be common to save on resources by reusing the VM/module).

I think this will need pausable access log extensions in envoy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants