Serve /alpaca.pac to send non-DIRECT requests to alpaca #16
Conversation
|
@samuong - IMO you should protect and "require pull request reviews before merging" for your |
There was a problem hiding this comment.
Pretty tired now @camh- sorry. I've reviewed your first two commits.
Will do the rest tomorrow.
Looks really neat what you guys have built!
Remove space in `// nolint:*` Addresses: samuong#16 (review)
s/monitonically/strictly/ Addresses: samuong#16 (comment)
Test for correct type of context value. Addresses: samuong#16 (comment)
Use testify.require for test setup failures. Addresses: samuong#16 (comment)
Remove space in ``// nolint:*` Addresses: samuong#16 (comment)
Use testify.require for test setup failures. Addresses: samuong#16 (comment)
Move check of `pf.fetcher == nil` outside the handler. Addresses: samuong#16 (comment)
Use testify.require for test setup failures. Addresses: samuong#16 (comment)
|
@juliaogris @anzdaddy Changes pushed as fixup commits. PTAL, and resolve all comments that you feel are now resolved. Or continue to argue as you see fit. |
@juliaogris The only reason I haven't done that in the past is that I didn't have anyone else to review my own pull requests. You all are the only ones (other than me) who have commented on PRs, so if you're happy to be collaborators and review my code, then I'll add you to the repo. |
There was a problem hiding this comment.
As per usual: reads really well, much fun to review.
Sadly I lack a bit of context to be more than an aspirationally intelligent linter for your PR.
I understand the high level (I think) - keep alpaca running on and off corp and if there is no pac file just DIRECT everything but I'm really missing context in the middle so I don't feel in the position to approve.
Peace&love
Remove redundant parentheses. Addresses: samuong#16 (comment)
Rework handler to remove `//nolint` directive. Addresses: samuong#16 (comment)
|
@marcelocantos @juliaogris @samuong All comments resolved. I have forced-pushed my squashed series, so back to just the commits that matter without fixups. I have removed the PTAL |
Fix initialism to be all caps. Addresses: samuong#16 (comment) Addresses: samuong#16 (comment)
Add full stop to end of comment sentence. Addresses: samuong#16 (comment)
Reformat comment for 100 char line length. Addresses: samuong#16 (comment)
Use `net/http` `http.MethodGet` const for `"GET"` Addresses: samuong#16 (comment)
Don't serve upstream PAC (`/proxy.pac`). Addresses: samuong#16 (comment)
Rename PAC to UpstreamPAC. Addresses: samuong#16 (comment)
|
@samuong PTAL I've pushed fixup commits and linked them to the comments so you can see how I have addressed your comments. When you've re-reviewed, I'll run |
Change back to simpler test for consecutive IDs starting from zero. Addresses: samuong#16 (comment)
Remove PACUrl, domain and username from PACData. They are not currently used and were added when envisaging a more general PAC wrapping feature. They can be added back when that feature is added. Addresses: samuong#16 (comment)
|
PTAL @samuong. Again, fixup commits pushed which I'll squash into the main series when you're happy with it. |
|
Thanks Cam! LGTM. I'll let you squash everything before I actually approve it, since I think your squash will invalidate my approval anyway. But once you're done I'm happy to give the actual approval. |
The proxyHandler was adding a strictly increasing ID value to the request's context that is used to correlate logs. As the only http handler in the application, this was fine. However, moving to serving PAC files from alpaca directly will split up the handling of http requests over different handler for that and the proxy function. In this case, it makes sense to split off the code that adds the ID to the context into a separate piece of http middleware. This will ensure that all requests have an ID, not just those handled by the proxy handler. Add tests for the context ID. Issues: samuong#14
Add a `WrapHandler()` method to `ProxyHandler` that can be used to wrap another `http.Handler` such that any proxy requests (CONNECT methods or absolute-form URIs) are passed to `ProxyHandler.ServeHTTP` and the rest are passed to the next `http.Handler` (in the standard way that http "middleware" works). Plug in an empty `http.ServeMux` as the next handler. Any origin-form URIs requested will go there and result in a 404 at the moment. This will allow us to add paths to the mux in future commits so that alpaca can serve content itself rather than just proxy requests. This will be used to serve the PAC file that alpaca is using as well as serve a wrapped PAC file that will direct all non-direct requests to alpaca. Update the existing test cases to use the `WrapHandler()` method and ensure the next `http.Handler` does not get called (an empty mux that always returns 404). Add an additional test to ensure that the middleware passes through non-proxy requests (origin-form URIs) to the mux. Move a few imports around as a result of `goimports` way of ordering imports. Issues: samuong#14
Add a request logger handler wrapper around the `ServeMux` used to handle non-proxy http requests. At the moment there are none of these - this is in preparation for handling some origin-form requests for serving PAC files. Issues: samuong#14
Consolidate handing of the PAC URL into the `ProxyFinder` so that the one function is used as the proxy function and all logging of DIRECT proxy requests are in one place. This simplifies `main()` keeping the proxy function out of it. It will also help when looking to consolidate all request logging in one place. Issues: samuong#14
Put ProxyFinder.checkForUpdates in the HTTP path by adding a `WrapHandler` method to check for updates before passing the HTTP request to the next handler. This makes little difference to the `ProxyHandler` as it triggers a check for updates on every proxied request, but we will also want to check for updates on requests to the `ServeMux` when it is serving content later. Issues: samuong#14
Serve a PAC file that is wrapped such that the `FindProxyForURL` function returns either `DIRECT` or `PROXY localhost:port` pointing at alpaca. `DIRECT` is returned when the PAC file that alpaca is using would return `DIRECT`. `PROXY localhost:port` is returned otherwise. This allows the OS to be set up to use `http://localhost:3128/alpaca.pac` as the PAC file to use which will make it use alpaca for any URLs that alpaca would proxy but otherwise go direct if that is all alpaca is going to do anyway. This helps with the odd application that doesn't seem to like going through a proxy at all. If the system were using the same PAC file that alpaca is using, applications would make non-proxied direct connections for these same URLs. If alpaca does not have a PAC file (if the host is on a different network for instance), then the `FindProxyForURL` function returned on the `/alpaca.pac` path will just return `DIRECT` always, directing all traffic away from alpaca. Issues: samuong#14
|
Ok. Squashed and force-pushed. If you click on the "force-pushed" link where it says "camh- force-pushed the camh-:pacwrap branch" you can verify the push did not alter anything. |
|
@samuong I wondered if this was going to happen. It looks like you're not a CODEOWNER so you cannot approve this merge. Only @juliaogris or @marcelocantos can :-) If you want to raise a PR to add yourself to CODEOWNERS, I can approve it. (or you can just use your admin superpowers and merge anyway) |
* Serve wrapped PAC file pointing at alpaca * Move ProxyFinder.checkForUpdates into request path * Consolidate PAC URL, proxy finding and logging * Add request logger around handler mux * Make ProxyHandler HTTP middleware capable * Split request context id adding to separate middleware
Serve a PAC file on
/alpaca.pacthat only returnsDIRECTorPROXY localhost:<listen-port>(usually3128). If the PAC file that alpaca is using would returnDIRECTfor a URL, the wrapped PAC also returnsDIRECT. For everything else, it returns a proxy directive to send traffic to alpaca. If alpaca does not have a PAC file (wrong network, etc), then the PAC function only returnsDIRECT.This allows the system (Mac OSX, Gnome, etc) to use alpaca as the proxy, and to configure alpaca with the network proxy URL. When the system evaluates a URL with the wrapped PAC file and gets a DIRECT response, it will not proxy through alpaca and go direct instead. This helps with the odd application that doesn't seem to handle going through a proxy at all. It also means that when you're off the network that serves the PAC file, no requests go through alpaca. This makes alpaca work well as a whole-system proxy instead of just for CLI apps as originally intended.
A bit of refactoring and lint fixes made it into this PR. The most significant change would be the use of http handler wrappers (also known in some circles as http middleware). Once the code went from 1 to >1 handlers, it made sense to factor out some functionality into a handler wrapper. Some additional functionality was added that way too.
Please review this commit-by-commit and read the commit messages. I hope that makes it clearer to understand the changes and their reason. I'm happy to entertain the most nit-picky requests/reviews, so don't hold back, no matter how trivial it seems.
Fixes: #14