feat #524 Support mapping additional request paths to different upstream URLs #526

chirino · 2020-04-08T18:08:42Z

No description provided.

golangcibot · 2020-04-08T18:10:06Z

server_upstream_paths_test.go

+	proxyServer := httptest.NewServer(proxy.router)
+	defer proxyServer.Close()
+
+	http.Get(proxyServer.URL + "/auth_all/white_listed/admin")


Error return value of http.Get is not checked (from errcheck)

golangcibot · 2020-04-08T18:10:06Z

server_upstream_paths_test.go

+	http.Get(proxyServer.URL + "/auth_all/white_listed/admin")
+	require.Equal(t, []int64{0, 1, 0}, counters())
+
+	http.Get(proxyServer.URL + "/auth_all/white_listed/other")


Error return value of http.Get is not checked (from errcheck)

golangcibot · 2020-04-08T18:10:06Z

server_upstream_paths_test.go

+	http.Get(proxyServer.URL + "/auth_all/white_listed/other")
+	require.Equal(t, []int64{1, 1, 0}, counters())
+
+	http.Get(proxyServer.URL + "/auth_all/white_listed/data")


Error return value of http.Get is not checked (from errcheck)

golangcibot · 2020-04-08T18:10:07Z

cli.go

+			if err != nil {
+				return fmt.Errorf("invalid upstream-url-paths %s, %s", x, err)
+			}
+			config.UpstreamPaths = append(config.UpstreamPaths, path)


append only allowed to cuddle with appended value (from wsl)

golangcibot · 2020-04-08T18:10:07Z

cli.go

@@ -220,6 +222,42 @@ func parseCLIOptions(cx *cli.Context, config *Config) (err error) {
 			config.Resources = append(config.Resources, resource)
 		}
 	}
+	if cx.IsSet("upstream-url-paths") {


if statements should only be cuddled with assignments (from wsl)

golangcibot · 2020-04-08T18:10:12Z

server.go

+		for _, x := range r.config.UpstreamPaths {
+			path := x
+			fmt.Printf("%s => %s\n", path.URL, path.Upstream)
+			upstreamUrl, err := url.Parse(path.Upstream)


var upstreamUrl should be upstreamURL (from golint)

golangcibot · 2020-04-08T18:10:12Z

server.go

 // createUpstreamProxy create a reverse http proxy from the upstream
-func (r *oauthProxy) createUpstreamProxy(upstream *url.URL) error {
+func (r *oauthProxy) createUpstreamProxy(upstream *url.URL) (*goproxy.ProxyHttpServer, error) {


Function 'createUpstreamProxy' is too long (72 > 60) (from funlen)

golangcibot · 2020-04-08T18:10:12Z

cli.go

+			return r, errors.New("config pair, should be (uri|upstream-url)=value")
+		}
+		switch kp[0] {
+		case "uri":


string uri has 2 occurrences, make it a constant (from goconst)

golangcibot · 2020-04-08T18:10:12Z

server_upstream_paths_test.go

+
+	http.Get(proxyServer.URL + "/auth_all/white_listed/data")
+	require.Equal(t, []int64{1, 1, 1}, counters())
+


unnecessary trailing newline (from whitespace)

golangcibot · 2020-04-08T18:10:13Z

cli.go

+	}
+	for _, x := range strings.Split(resource, "|") {
+		kp := strings.Split(x, "=")
+		if len(kp) != 2 {


mnd: Magic number: 2, in detected (from gomnd)

golangcibot · 2020-04-08T19:27:40Z

server.go

@@ -155,7 +155,7 @@ func createLogger(config *Config) (*zap.Logger, error) {
 // createReverseProxy creates a reverse proxy
 func (r *oauthProxy) createReverseProxy() error {
 	r.log.Info("enabled reverse proxy mode, upstream url", zap.String("url", r.config.Upstream))
-	if err := r.createUpstreamProxy(r.endpoint); err != nil {
+	if err := r.createDefaultUpstreamProxy(); err != nil {


if statements should only be cuddled with assignments (from wsl)

golangcibot · 2020-04-08T19:27:41Z

server_upstream_paths_test.go

+	proxyServer := httptest.NewServer(proxy.router)
+	defer proxyServer.Close()
+
+	_, err = http.Get(proxyServer.URL + "/auth_all/white_listed/admin")


response body must be closed (from bodyclose)

golangcibot · 2020-04-08T19:27:41Z

server_upstream_paths_test.go

+	require.NoError(t, err)
+	require.Equal(t, []int64{0, 1, 0}, counters())
+
+	_, err = http.Get(proxyServer.URL + "/auth_all/white_listed/other")


response body must be closed (from bodyclose)

golangcibot · 2020-04-08T19:27:41Z

server_upstream_paths_test.go

+	require.NoError(t, err)
+	require.Equal(t, []int64{1, 1, 0}, counters())
+
+	_, err = http.Get(proxyServer.URL + "/auth_all/white_listed/data")


response body must be closed (from bodyclose)

chirino · 2020-04-08T19:36:28Z

all the white space reviews by the golangcibot are super annoying. Is there some CLI tool that could fix these automatically for me?

…t upstream URLs

stianst · 2020-04-21T09:36:42Z

Could you provide some description of what this PR is about?

chirino · 2020-04-29T12:56:07Z

This PR allows one keycloack gatekeeper be the security gateway to multiple upstream http servers. This is a common setup when you app is broken down into multiple microservices which that all want to share the same security policies. It would not be very DRY to have to repeat gatekeeper configuration per microservice.

stianst · 2020-04-29T14:34:27Z

The idea with Gatekeeper is that it should be deployed alongside the application, and should not be a gateway to multiple applications/services. If you deploy Gatekeeper separate you are opening up for applications to be accessed insecurely when someone gets inside the network boundaries.

stianst · 2020-04-29T14:35:19Z

For separate micro services these should also be using separate client-ids as you do not want a single token to be able to invoke all micro services, but rather control what can invoke what.

stianst

-1

stianst · 2020-04-29T14:37:43Z

That being said there is discussions and potential that we will allow plugin Gatekeeper into a general purpose proxy/gateway, such as HAProxy/OpenShift Router/Envoy, through a gRPC remoting interface.

stianst · 2020-04-30T06:13:01Z

We should continue the discussion on this one in the issue:
#524

stianst · 2020-05-08T12:11:52Z

After thinking about this some more we should support this to make it easy to migrate from other proxies, then recommend using mtls to prevent ability to bypass security by invoking upstream directly.

JoelSpeed · 2020-05-11T10:43:55Z

I haven't reviewed this fully yet, just a quick scan. I also don't have too much context on this project and how it's configured yet, but, from experience with OAuth2-Proxy, I would suggest we don't have a flag for this and instead force users to use a configuration file.

Overloading of strings in flags makes for complicated parsing code and also makes documenting the feature difficult. We have to make up our own formats for separating the different parts of the option and users will make mistakes configuring this that will be hard to understand.

I would much prefer to see this as a structured object within configuration only rather than a flag with complex parsing.

For context, I raised some discussion around this oauth2-proxy/oauth2-proxy#532 as the complexity of configuration has been one of the major problems with the project since I took it over

golangcibot reviewed Apr 8, 2020

View reviewed changes

chirino force-pushed the upstream-paths branch from 4d9b550 to 8cd4f0e Compare April 8, 2020 19:26

golangcibot reviewed Apr 8, 2020

View reviewed changes

feat louketo#524 Support mapping additional request paths to differen…

f5798cb

…t upstream URLs

chirino force-pushed the upstream-paths branch from 8cd4f0e to f5798cb Compare April 8, 2020 19:38

abstractj added the status/discuss Needs discussion label Apr 29, 2020

stianst suggested changes Apr 29, 2020

View reviewed changes

stianst requested review from stianst and abstractj May 8, 2020 12:10

abstractj requested a review from JoelSpeed May 8, 2020 14:23

abstractj self-assigned this May 8, 2020

abstractj removed the status/discuss Needs discussion label May 8, 2020

abstractj removed the request for review from stianst June 15, 2020 18:49

abstractj mentioned this pull request Jun 25, 2020

Support mapping additional request paths to different upstream URLs. #524

Open

abstractj removed their assignment Aug 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat #524 Support mapping additional request paths to different upstream URLs #526

feat #524 Support mapping additional request paths to different upstream URLs #526

chirino commented Apr 8, 2020

golangcibot Apr 8, 2020

golangcibot Apr 8, 2020

golangcibot Apr 8, 2020

golangcibot Apr 8, 2020

golangcibot Apr 8, 2020

golangcibot Apr 8, 2020

golangcibot Apr 8, 2020

golangcibot Apr 8, 2020

golangcibot Apr 8, 2020

golangcibot Apr 8, 2020

golangcibot Apr 8, 2020

golangcibot Apr 8, 2020

golangcibot Apr 8, 2020

golangcibot Apr 8, 2020

chirino commented Apr 8, 2020 •

edited

stianst commented Apr 21, 2020

chirino commented Apr 29, 2020

stianst commented Apr 29, 2020 •

edited

stianst commented Apr 29, 2020

stianst left a comment

stianst commented Apr 29, 2020

stianst commented Apr 30, 2020

stianst commented May 8, 2020

JoelSpeed commented May 11, 2020


		http.Get(proxyServer.URL + "/auth_all/white_listed/data")
		require.Equal(t, []int64{1, 1, 1}, counters())

feat #524 Support mapping additional request paths to different upstream URLs #526

Are you sure you want to change the base?

feat #524 Support mapping additional request paths to different upstream URLs #526

Conversation

chirino commented Apr 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chirino commented Apr 8, 2020 • edited

stianst commented Apr 21, 2020

chirino commented Apr 29, 2020

stianst commented Apr 29, 2020 • edited

stianst commented Apr 29, 2020

stianst left a comment

Choose a reason for hiding this comment

stianst commented Apr 29, 2020

stianst commented Apr 30, 2020

stianst commented May 8, 2020

JoelSpeed commented May 11, 2020

chirino commented Apr 8, 2020 •

edited

stianst commented Apr 29, 2020 •

edited