Replace gorilla/mux dependency

[kristiansvalland]

Due to the archiving of gorilla/mux as a result of missing maintainers, it has been considered to swap out the dependency and use our own implementation. For reference, we had a conversation on slack about this topic, and this PR is a proposed solution to what we discussed.

This PR copies the relevant files from gorilla/mux including its license, and tries to keep the resulting implementation as bare bone as possible.

It is likely that there is more code that can be removed from the implementation, as the usage of the mux.Router mainly uses Router.Handle and Route.Methods, but I did not remove the other matchers yet.

It is implied that the commits will be squashed before merge, but for now I leave them separate to keep the review process simple and to make the process of copy and edit transparent.

Disclaimer: I am not familiar with the usage of licenses and redistribution of licensed works, so I would appreciate that someone can vouch for the methodology used here.

[srenatus]

Can we move this into internal/mux/ just so no outside dependencies can happen? Or is it going to be a problem with the runtime exposing it's mux? 🤔 I'm not sure I remember the details right.

[kristiansvalland]

While I have not considered adding it to internal, I have had the same question regarding the plugins package. While the router member is private on Manager Manager.r, the package does expose the WithRouter(r *mux.Router) func(*Manager) {...} function.
Might also apply to Server with its WithRouter method as well(?).

Not exactly sure about the usage of server and runtime outside of OPA itself though.

[srenatus]

Thanks a lot for looking into this! 👏

[netlify]

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	78417eb
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/63b1a39e81cc0a00081342a4
😎 Deploy Preview	https://deploy-preview-5483--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[philipaconrad]

Thanks @kristiansvalland for looking into this, and helping us pull in this dependency. 👍

I'll echo @srenatus's concern: Can we move the mux package to internal/mux to avoid any possibility of folks depending on it? If not, let's at least list the obvious breaking API changes that would happen from doing that move, so that we can see if it's worth the hazard or not.

I'm leaning towards breaking things if necessary, but I'm open to other opinions. @srenatus, what do you think? 🤔

[kristiansvalland]

Thanks @kristiansvalland for looking into this, and helping us pull in this dependency. 👍

I'll echo @srenatus's concern: Can we move the mux package to internal/mux to avoid any possibility of folks depending on it? If not, let's at least list the obvious breaking API changes that would happen from doing that move, so that we can see if it's worth the hazard or not.

I'm leaning towards breaking things if necessary, but I'm open to other opinions. @srenatus, what do you think? 🤔

Here is what will break as far as I know if we move it into internal:

• It will break Plugin to extend the REST API? #2777

• It will break this PR that partly solves the above: Add Router to runtime.Params #3458

• Also breaks the functionality introduced by this commit by tsandall: 8cd94fc

  This allows callers to embed the OPA HTTP server inside another HTTP server (e.g., under a path prefix like /opa).

  However, would it even be possible to do this if our new "mux" package is not in "internal"? I.e. the implementer would likely want to use a gorilla/mux router as their base router, then add a subrouter on e.g. "/opa", and then that subrouter must somehow reference our "opa/mux.Router" 🤔 I think they then would need to use our opa/mux.Router for their own stuff as well as far as I understand the gorilla/mux library.

On the other hand, keeping it outside internal will likely necessitate us maintaining the whole functionality of the gorilla/mux library here, as removing features (like I have already tried doing to some extent in this PR) will limit the users that want to extend the functionality.

[srenatus]

So, technically, the move from github.com/gorilla/mux to github.com/open-policy-agent/opa/mux alone would potentially break stuff for folks.

On the detailed questions (thanks for finding out about them!):

Also breaks the functionality introduced by this commit by tsandall: 8cd94fc

This allows callers to embed the OPA HTTP server inside another HTTP server (e.g., under a path prefix like /opa).

However, would it even be possible to do this if our new "mux" package is not in "internal"? I.e. the implementer would likely want to use a gorilla/mux router as their base router, then add a subrouter on e.g. "/opa", and then that subrouter must somehow reference our "opa/mux.Router" 🤔 I think they then would need to use our opa/mux.Router for their own stuff as well as far as I understand the gorilla/mux library.

I think we shouldn't have exported *mux.Router there in the first place. The common interface used for these things is http.Handler which *mux.Router implements. Replacing how we implement that, then, by opa/internal/mux instead of gorilla/mux should be OK, too 🤞 (Note: technically, a change like that is still breaking compat.)

It will break #2777
It will break this PR that partly solves the above: #3458

This looks like it's the other way around -- we're expecting something to be set with the interface we're calling on it. From a brief look, we seem to be using Handle and HandleFunc. They're almost identical so we could do with changing the code slightly and only require on or the other. So if we changed it to expect something satisfying where gorilla diverges from the standard lib to provide its extra features, we'd need an

type Mux interface {
  Handle(pattern string, hndl http.Handler) Route
}
type Route interface {
  Methods(string...) Route
}

So we could work with old code that feeds us a gorilla mux. Basically feeding us a gorilla mux would be the only way to use that feature (unless we re-worked it), if we move our copy to internal/mux.

Ok enough of that. What are you thoughts? Viable? Worth it? @kristiansvalland @philipaconrad

[kristiansvalland]

Good points, @srenatus.
I think that it's worth investigating how all our internal dependencies on the mux package can be done through interfaces, and how those interfaces can be what we actually export for extending the server or runtime. I too think that this should be possible, and it would be nice if current implementations feeding the runtime or server with gorilla/mux would not break.

However, regarding #2777, it currently exposes the GetRouter() on the plugin.Manager struct, meaning that we are exporting the entirety of the Router's contract. In other words, we do not know which method or field the users of plugins have used on the mux.Router struct to extend OPA, so its not entirely clear or straight forward to see what the interface is that we should expose here instead. Something should be possible of course, it's just not obvious to me at the moment.
One way to deal with this is to add extendibility step-by-step in the proper way through interfaces. An example of extendibility could for instance be that the Manager exposes a method for adding handlers

func (m *Manager) AddHandler(p string, h http.Handler) { ...}

or just return our new proposed interface

// Return our mux interface allowing the uses to use the `Mux.Handle` method.
func (m *Manager) GetMux () Mux { ... }

Thus being explicit about how the plugins can extend OPA.

To summarize, what we receive (e.g. Server.WithRouter) should be easy to convert into using interfaces, but what we until now have returned (GetRouter()) is less so.

[kristiansvalland]

I tried defining the interfaces that you mentioned, @srenatus, however I now realise that it won't be as simple as I initially thought. Let's say that the user wants to pass a gorilla/mux Router to the server using the WithRouter() method,

router := gorillamux.NewRouter()
server := New()
server.WithRouter(router)

In this case, the go compiler would complain with the following

cannot use router (variable of type *"github.com/gorilla/mux".Router) as "github.com/open-policy-agent/opa/mux".Mux value in argument to server.WithRouter: *"github.com/gorilla/mux".Router does not implement "github.com/open-policy-agent/opa/mux".Mux (wrong type for method Handle)
		have Handle(path string, handler http.Handler) *"github.com/gorilla/mux".Route
		want Handle(path string, h http.Handler) "github.com/open-policy-agent/opa/mux".Route

(in the above message, I have moved the implementation into ./internal/mux, and defined the interfaces in ./mux for now).

The following is the interface

type Mux interface {
	http.Handler
	Handle(path string, h http.Handler) Route
}

type Route interface {
	Methods(methods ...string) Route
}

and

func (s *Server) WithRouter(router muxi.Mux) *Server {
	s.router = router
	return s
}

While the returned values of the Handle method on gorilla's Router also implements the interface we require, go still complains because it is strict on the return type it seems. Is this correct, or is there a way to define the interfaces and methods in go that would allow this?

If not, an option would be to break existing code passing a gorilla/mux, but enabling a rewrite through wrapper structs for gorilla/mux.Router.

[srenatus]

While the returned values of the Handle method on gorilla's Router also implements the interface we require, go still complains because it is strict on the return type it seems. Is this correct, or is there a way to define the interfaces and methods in go that would allow this?

Ugh I had forgotten about this restriction. Thanks for trying it out.

I wonder if some sort of clean break isn't the better approach here, then. But what about build tags? Perhaps we could split the go files in such a way that previous users can resort to the old behaviour; but anyone not setting the tags would get the new... 🤔

As for the other problem,

In other words, we do not know which method or field the users of plugins have used on the mux.Router struct to extend OPA, so its not entirely clear or straight forward to see what the interface is that we should expose here instead.

You're right, of course, we've given them all of it when we should have only given them what they need to do

mux.Handle("/opa", pm.GetRouter())

So there's a mismatch between intent and implementation here, I think. If we restrict stuff to http.Handler, we'll revert that, potentially breaking anyone who hadn't been using the feature as it was intended -- but that won't be their fault, since we've made it very easy to do that. Again, build flags? 😳

[kristiansvalland]

I split the relevant files into *_new and *_old in in the latest commit.

To keep the tests in internal/mux unchanged, it simplified things to create a wrapper for the mux.Route struct, as otherwise the interface it implements would need to be extended to allow for the method chaining in the test files. Still this is a possibility if you think its better to not create a wrapper.

List of unfinished work:

• Verify documentation of internal/mux is still correct after changes, and remove references to gorilla in docstrings (but keep license and copyright of course)
• Remove duplicate test files where the code has been split. Copied server_test.go into server_new_test.go and server_old_test.go
• Enable testing of "old" code by adding the "usegorillamux" build tag where needed.
• plugins/: Exactly what to expose? GetRouter() muxproto.Router?

[anderseknert]

This is great, @kristiansvalland! Thanks for helping out with this 👏

[srenatus]

Thanks again for looking into this. And especially for pushing the build tag approach as far as you've taken it. I am, however, slightly hesitant about this as a way forward 🤔 There's no way to know what it feels like to go this route without trying, but looking at where we've landed, I think we should consider other options.

Since it's been a few days, do you know if any other maintainers have adopted gorilla in the mean time? It keeps re-appearing in my news feeds... 👀

[johanneslarsson]

After reading all your comments I wonder if we shouldn't ask @kristiansvalland to take a stab on a PR implementing gin-gonic/gin instead? I think it would be a more fruitful idea. Most benchmarks (not only gin-gonic/gin themselves) indicates that there could be performance benefits from it too.

[kristiansvalland]

Thanks again for looking into this. And especially for pushing the build tag approach as far as you've taken it. I am, however, slightly hesitant about this as a way forward 🤔 There's no way to know what it feels like to go this route without trying, but looking at where we've landed, I think we should consider other options.

Since it's been a few days, do you know if any other maintainers have adopted gorilla in the mean time? It keeps re-appearing in my news feeds... 👀

@srenatus, apologies for such a late reply, I have been a bit busy with other tasks in the meantime.
I get the same feeling that this approach might not be the best path forward, as it seems unnecessary complex and can quickly lead to technical debt.

About gorilla: I have not heard or seen anything, but to be honest, I am not actively monitoring the situation either.

Regarding what @johanneslarsson says: Opting for using gin, or another actively maintained framework for that matter, could well be a good solution. We would of course take care not to expose anything that is gin-specific in how the runtime etc. can be configured.

I will close this PR for now and look into using gin in a different PR.