sso-proxy: refactor routing into hostmux, refactor oauthproxy startup#190
sso-proxy: refactor routing into hostmux, refactor oauthproxy startup#190
Conversation
| optFuncs = append(optFuncs, | ||
| SetCookieStore(opts), | ||
| SetUpstreamConfig(upstreamConfig), | ||
| SetProxyHandler(handler), |
There was a problem hiding this comment.
SetProxyHandler strikes me as a bad use of functional options, because a handler is not an optional piece of an OAuthProxy instance. My vote would be to change the signature of NewOAuthProxy to take a handler as its first argument:
oauthproxy, err := NewOAuthProxy(handler, opts, optFuncs...)There was a problem hiding this comment.
It's particularly nice for testing though :)
There was a problem hiding this comment.
Do we have a lot of tests where we don't need a handler set up? Can we paper over that in a testing helper function?
There was a problem hiding this comment.
I've added this commit to add error messages if the NewOAuthProxy does is not properly configured.
I think this is a nice compromise between the func options as well as ensuring the oauthproxy we return is functional.
jphines
force-pushed
the
sso-refactor-hostmux
branch
from
3871f6d to
978e7db
Compare
| } | ||
| } | ||
|
|
||
| func TestPing(t *testing.T) { |
There was a problem hiding this comment.
Rewritten and moved.
jphines
force-pushed
the
sso-refactor-hostmux
branch
from
b9170d8 to
b930cc5
Compare
mccutchen
left a comment
mccutchen
left a comment
There was a problem hiding this comment.
LGTM @jphines, I think this significantly improves our ability to test and further expand sso's features and functionality. Thanks for bearing with me on this long review process!
Given how significant and fundamental these changes are, how would you feel about letting them soak on our infrastructure for a bit before we land them here?
jphines
force-pushed
the
sso-refactor-hostmux
branch
3 times, most recently
from
3fbda01 to
26bdf6d
Compare
jphines
force-pushed
the
sso-refactor-hostmux
branch
from
26bdf6d to
07fa1b7
Compare
2 participants
Problem
OAuthProxy currently manages all of our routing information, upstream configs, provider information, etc. This made sense as we incrementally built SSO and wanted to make as minimal changes as possible during the development cycle.
However, SSO today is quite different and has various needs to be easier to extend.
Uncoupling routing, upstream configuration, and provider information from being closely tied to the OAuth layer will help in this uncoupling.
Solution
We two higher layer structs to help simplify the component structure as well as testing. We had a high level
hostmuxthat provides routing information using simple host matches as well as regexp host matches.We also use a new
SSOProxystruct as a constructor for use inmain.go. Pulling this constructor out ofmain.gomakes it easier to test and allows us to move more logic there.