metaverse
Proposal: Add a way to wrap the http.Handler before serving
In order to support the likes of CORS handling (i.e. using rs/cors), it would be useful to have a way to wrap the http.Handler before it's passed in to http.ListenAndServe by server.Run.
I started implementing this in order to send through a PR, but although the change itself is reasonably straightforward, as middlewares.go is generated in an all-or-nothing manner (if it already exists, it's not touched at all), simply adding it to the template would result in breakage when re-running Truss after updating as the new WrapHTTPHandler func (or whatever we call it) wouldn't exist.
I see a few potential ways around this:
- Start issuing semantically-versioned releases (see #211), so that a version bump could be used to discourage unnoticed updates
- Release notes could be provided explaining how to upgrade
- This would also open the door for (semi-)automated project upgrades, perhaps with a new
--upgrade-from=[version]CLI argument
- Completely rework the
middlewares.go(andhooks.go?) generation to work more like thehandlers.gogeneration (i.e. check for existence of the funcs, adding the default/auto-generated func if not present) - Just go ahead and update the
middlewares.gotemplate, and let existing projects break when Truss is updated and re-run
Hi @pauln,
To respond to your potential ways around this, we do want to implement solution 1, having some kind of semantic versioning for breaking changes. And including messages about how to upgrade an old service, maybe even shell scripts.
If you or any other community member has the time, we would love contributions for solution 2, it would be very useful but currently it is not on the roadmap for us.
Currently solution 3 is what we doing, as we are still in "pre-alpha" mode.
About wrapping the http.Handler below:
I am curious what solution you were going for, could your link the branch you were working on?
I have a solution to your issue, curious of what you think.
Using a custom main.go with generated truss services.
Hi @adamryman,
The solution I was going for is similar to your custom main.go, except that I put the wrap function into middlewares.go and call it from within the //HTTP transport. block of func Run in svc/run.go so that it's handled automatically rather than requiring a custom main.go. If that sounds like an acceptable solution (in light of option 3 being the current modus operandi), I can go ahead and put together a PR for it.
@pauln
If you have already made this could you put up a PR with Proposal: in the front and link to this issue?
It sounds like a very small change and could provide a lot of utility. Currently some core members are against it because it feels outside of truss's scope. Though I think a PR could sway them if they see the simplicity. Note that I would not ask for this if it was any kind of complex change.
@zaquestion @hasLeland @hasIan
I can certainly file a PR, though the just-merged transition to stdlib log will change it (no additional complexity - the code in question has been refactored, so my change just needs to go in a different spot). I see there's also a branch revert-217-update_to_stdlib_logger though - does that imply that #217 is likely to be reverted, or is that just a convenience branch to make it easier to grab a version of Truss without that change?
Sorry about the revert branch I accidentally created that.
Though your comments about having to move where you wrap your code make me realize I probably exposed too much API in the that PR, especially with my recommendation to make a custom main.go
So I have created a PR to revert that exposed API. You can see that here: https://github.com/TuneLab/truss/pull/221
@pauln Thank you for your efforts in the proposal. My concern is with wrapping only the HTTP handler makes the HTTP transport special in a way that GRPC isn't. Truss aims to keep your business logic transport agnostic.
Truss aims to keep your business logic transport agnostic.
This is actually a core value in truss, admittedly our documentation should be updated to better expose what the core values are.
Adding this in truss would be a breach of this value. In this scenario with CORS, I would recommend a custom main.go
At TUNE
We solve CORS by using a different service for our interface to frontend called a BFF This is not a truss service and interfaces exclusively with HTTP. This was built before truss was production ready. Today we would probably use use the below solutions.
If your usecases are simple enough
Its understandable that you'd want to try and integrate your frontend and truss service directly. Several of us have done this in personal projects even. There are a few likely changes to truss https://github.com/TuneLab/truss/issues/223 https://github.com/TuneLab/truss/issues/218 that will make this nicer as well.
Support for custom main.go files were added so developers have an opportunity to take these situations into there own hands and we encourage it!
Although you've chosen to use BFF at TUNE, it'd be great if Truss supported different ways in which other developers may wish to use it. I don't see CORS as business logic; it's really a part of the HTTP transport (especially when working with microservices in the absence of something like BFF) - so it'd be great if Truss made it easy to handle without needing to implement a custom main.go which essentially replicates the generated one, but duplicates/inlines server.Run in order to make this one minor addition.
Would simply documenting the intentions behind WrapHTTPHandler (and/or renaming it to something more specific such as HandleCORS to make intentions super clear) help reconcile it with the core values of Truss? It strikes me that encouraging the use of a custom main.go strays further from the ideal of transport agnosticism, as it makes me (as a developer) have to modify transport-related code - and, as such, allows for even wilder transport-related customisations than WrapHTTPHandler does.
wrapping only the HTTP handler makes the HTTP transport special in a way that GRPC isn't
If this is a big issue, we could take a narrower approach which only allows for CORS handling (rather than generic wrapping of the HTTP handler). That would presumably introduce a new dependency (and thus also tie Truss users to a specific CORS package), though - so seems like a worse option than WrapHTTPHandler. Alternatively, if you can suggest a sensible/useful equivalent for gRPC so that HTTP isn't so "special", I can look at implementing it.
without needing to implement a custom main.go which essentially replicates the generated one, but duplicates/inlines server.Run in order to make this one minor addition.
Its seems like your adverse to copying code? As go developers this solution feels appropriate for illustrating that by wrapping the http handler directly or by adding http routes outside of the protobuf def you are shaping the service to fit your usecase specifically. We don't consider this bad or hacky, but an enviable part in meeting some technical requirements of business needs. Which is why we tried to keep the entrypoint flexible. Agreed that this is not something every or hopefully even most projects should need to do.
Additional documentation wouldn't sway my feelings on this.
wrapping only the HTTP handler makes the HTTP transport special in a way that GRPC isn't
This is the "big issue" so to speak. A concern with bringing this in would be more widespread use of WrapHTTP over WrapEndpoints purely because "we aren't using gRPC". The argument for this level of control will be answered with a custom main.go
Now that I've gotten that out..
we could take a narrower approach which only allows for CORS handling
This is an interesting idea. Admittedly I have not had any experience with using gRPC (or HTTP/2 at all) from a web browser. This makes me wonder if there are CORS equivalent restrictions? I will do some research on this and ask some frontend capable folks (which I am not (at all))
If yes, I would be interested in exploring a solution that incorporated both transports, hopefully can just be accomplished with a gokit middleware.
If no, this would change my thinking of the CORS problem and I would be inclined to agree with your assessment
I don't see CORS as business logic; it's really a part of the HTTP transport
In this case I could see adding specific support for CORS in the middlewares.go file. Off the top of my head this would be something like setting a flag and the the code could be generated commented out.
Lastly, there is a way to implement CORS functionality with a gokit endpoint middleware. By use of some functionality for working with headers and the context that truss has. @adamryman or @lelandbatey would be better at explaining this solution. IIRC this could be made easy by moving truss to the newer http+ctx functionality in the stdlib (which was not available during initial development)
This makes me wonder if there are CORS equivalent restrictions?
I don't know that there are any equivalent restrictions for "direct" gRPC, as it's not (to my knowledge) usable from a web context (where you have an "origin" to care about), but the gRPC-Web spec has a section on CORS preflight requests as it's effectively gRPC over HTTP (for use from client-side JS).
A wrapper already exists for serving gRPC-Web from Go; it already handles CORS preflight requests using the rs/cors package. Even if it's not suitable/desirable to leverage this wrapper in Truss, it should be useful as a working example.
Lastly, there is a way to implement CORS functionality with a gokit endpoint middleware
this could be made easy by moving truss to the newer http+ctx functionality in the stdlib
I'm keen to hear more about this (both the current approach and potential improvements) - please do share when you get a chance, @adamryman / @lelandbatey 😃