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

Adding support for envoy ext_proc in OPA #614

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

pweiber
Copy link

@pweiber pweiber commented Nov 13, 2024

This PR resolves open-policy-agent/opa#6639 by adding the support for the usage of envoy external processor(ext_proc).

This PR also includes a plugin refactoring to be able to run different plugins in different ports based on the config file, for example:
authz at :9191 and ext_proc at :9292.

Consider this PR as a first draft for the functionality so we can validate the feature first.

All suggestions are welcome and I'll try to fix the issues found as soon as possible.

plugin/plugin.go Outdated
ExtProcPluginName = "envoy_ext_proc_grpc"
)

// New method for AuthZFactory.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this change would be break backward compatibility, correct? The rest of the changes seem backward compatible. Is there a way to either keep the old one and if not we can deprecate it in favor of a more flexible implementation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure to be honest, if we consider the changed structure It'll start the authz plugin with "envoy_ext_authz_grpc" but the wrapper for the v2 is still there, unless it requires the naming to be "envoy.ext_authz.grpc" then yes it'll break.

If it can use the same NewAuthZ internal method what we can do is something like this:

plugin.go

type Factory struct{}

func (Factory) New(m *plugins.Manager, config interface{}) plugins.Plugin {
	return internal.NewAuthZ(m, config.(*internal.Config))
}

func (Factory) Validate(m *plugins.Manager, configBytes []byte) (interface{}, error) {
	return internal.Validate(m, configBytes)
}

and for the main.go we move back to use the Factory

runtime.RegisterPlugin("envoy.ext_authz.grpc", plugin.Factory{})

What do you think? I'm not sure how to test it, so if you can provide me with some guidance we can check if it works

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant is if we keep the Factory implementation for the envoy.ext_authz.grpc unchanged and add a new one for envoy_ext_proc_grpc, won't that work? Then based on how the OPA-Envoy plugin is configured, the "right" factory functions will be called. I haven't tried to do this but would be good to know if this works.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh ok got it, makes sense. Yes I think it should work, tomorrow is a holiday here so I'll try that on Monday and get back to you.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it worked, I'll adjust the logs and work on the tests for the new files, let me know if you have any other concerns about the backward compatibility

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest seemed ok to me. Thanks for working on this!

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

Successfully merging this pull request may close these issues.

support for envoy ext_proc in OPA
2 participants