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

support for envoy ext_proc in OPA #6639

Open
rshriram opened this issue Mar 19, 2024 · 15 comments · May be fixed by open-policy-agent/opa-envoy-plugin#614
Open

support for envoy ext_proc in OPA #6639

rshriram opened this issue Mar 19, 2024 · 15 comments · May be fixed by open-policy-agent/opa-envoy-plugin#614
Labels
feature-request help wanted int-envoy Issues related to the opa-envoy-plugin

Comments

@rshriram
Copy link

OPA currently supports Envoy's ext_authz protocol (unary gRPC). Envoy now has a more extensible and stream based protocol for extensibility (ext_proc) that provides a structured way to interpose on request headers/body etc. This feature request is to add support for the Envoy ext_proc protocol to OPA's envoy plugin. The main benefit of this feature is that as an end user, I will have to use only one extensibility protocol in Envoy that can be used for multiple purposes: authorization, body transformation or any kind of traffic mutation.

@ashutosh-narkar
Copy link
Member

Thanks for filing this request.

The main benefit of this feature is that as an end user, I will have to use only one extensibility protocol in Envoy that can be used for multiple purposes: authorization, body transformation or any kind of traffic mutation.

Currently you would have to use the ext_authz filter plus other filters to make this work but ext_proc can do all this. Is this correct?

From the ext_proc docs this is a WIP but most functionality seems to be implemented.

@ashutosh-narkar ashutosh-narkar added the int-envoy Issues related to the opa-envoy-plugin label Mar 19, 2024
@rshriram
Copy link
Author

Currently, the user would have to do ext_authz filter (and implement ext_authz protocol on their side) for authz, and then ext_proc filter for body transformation etc (and implement ext_proc protocol on their side). This is going to lead to toil and a situation where they refuse to use one or other due to additional work involved.

ext_proc is marked as production ready. What part of it is WIP?

@ashutosh-narkar
Copy link
Member

Thanks for the context.

ext_proc is marked as production ready. What part of it is WIP?

From https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/ext_proc_filter

This filter is a work in progress. Most of the major bits of functionality are complete. The updated list of supported features and implementation status may be found on the reference page.

From https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/ext_proc/v3/ext_proc.proto#envoy-v3-api-msg-extensions-filters-http-ext-proc-v3-externalprocessor

Current Implementation Status: All options and processing modes are implemented except for the following:
“async mode” is not implemented.

@rshriram
Copy link
Author

Async mode is completely irrelevant for OPA. async is an enhancement for observability use cases. Could we get some estimate on how long it would take to support this?

@ashutosh-narkar
Copy link
Member

This seems like a good addition. If you'd like to contribute the feature that would be great! We'd be happy to help with any questions, reviews etc. We could also leave this open for sometime for other folks in the community wanting this feature as well to chime in and perhaps submit a contribution.

Copy link

stale bot commented Apr 25, 2024

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.

@stale stale bot added the inactive label Apr 25, 2024
@tjons
Copy link
Contributor

tjons commented Aug 7, 2024

I'm going to work on this one next. I'm still in the process of designing it but I think it will require some refactoring of the overall OPA envoy plugin, as the server implementing the ExtProc protocol will likely need to run on another port alongside the ExtAuth protocol. I'll attach a design document in the next few days and we ideally can discuss the approach before I begin implementation, as it will not be a small change.

cc @ashutosh-narkar

@stale stale bot removed the inactive label Aug 7, 2024
@pweiber
Copy link

pweiber commented Aug 14, 2024

Hi guys, I've been working in the background on this to try to assist rshriram and speed things up. However, after seeing tjons' comments, I tried to come up with a solution, but I haven't been successful so far, likely due to my limited familiarity with the codebase.

I put together some code for the ext_proc server, which I've run individually and received some responses from. While there's still some work to be done, I hope this code might be of some help and could potentially be integrated with the refactoring that tjons is planning. I based my code on the current authz implementation. Please let me know if this code is useful or if it might not be relevant. The branch is available here: https://github.com/pweiber/opa-envoy-plugin/tree/ext_proc

cc @tjons

@tjons
Copy link
Contributor

tjons commented Aug 15, 2024

@pweiber awesome, sounds good. I've been a little busy this week but should have some time to hack on this issue this weekend - I'll take a look at what you have and see whether we can incorporate it into what I had been planning... at a very brief glance, it looks good :)

@rshriram
Copy link
Author

@tjons is there any update?

@tjons
Copy link
Contributor

tjons commented Sep 12, 2024

@rshriram I apologize for the lack of forward progress. I'm in the middle of a job transition and will look to pick this up again sometime next month once I've started my new role. If others are able to make progress, please do - I'm not territorial about this, just want to see it completed.

@anderseknert
Copy link
Member

Sorrry for interrupting / going off topic, but great to hear that @tjons! And all the best with your new role 🚀

@tjons
Copy link
Contributor

tjons commented Sep 12, 2024

Thanks @anderseknert!

Copy link

stale bot commented Oct 17, 2024

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.

@stale stale bot added the inactive label Oct 17, 2024
@pweiber
Copy link

pweiber commented Nov 13, 2024

Hi all, I believe that in terms of functionality, I was able to tackle most of the features related to the ext_proc.

Regarding the refactoring I tried some approaches but keeping the code inside the internal and changing the plugin factory code seemed the best way to be able to run multiple plugins at the same time and using different ports, I believe we can discuss more if the current solution is not that good.

I also added into the examples folder the config and the client that I used for the testing, if not needed I can just delete that before the merge.

There is still some logging I need to change and create more unit tests for the new files but I believe that we can start validating the implementation, I'll create a draft PR for now so the reviewers can begin taking a look into it and I'll try to fix whatever is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request help wanted int-envoy Issues related to the opa-envoy-plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants