Add OAuth Proxy to UI when on OpenShift #100

jpkrohling · 2018-11-08T16:08:43Z

Closes #89.

Signed-off-by: Juraci Paixão Kröhling [email protected]

jpkrohling · 2018-11-08T16:08:50Z

This change is

jpkrohling · 2018-11-08T16:10:31Z

~~This is currently blocked by openshift/oauth-proxy#95.~~ Problem fixed in the latest commit.

Unit tests are passing, as well as end-to-end tests, which execute only on Kubernetes.

Manually testing this on OpenShift, I can see that all the objects are created as required, but I cannot login due to the issue listed above.

So, this is ready for an initial review but shouldn't be merged until the login issue is fixed.

codecov · 2018-11-08T16:11:34Z

Codecov Report

Merging #100 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #100      +/-   ##
==========================================
+ Coverage   99.43%   99.48%   +0.05%     
==========================================
  Files          21       24       +3     
  Lines         879      967      +88     
==========================================
+ Hits          874      962      +88     
  Misses          5        5

Impacted Files	Coverage Δ
pkg/account/oauth-proxy.go	`100% <100%> (ø)`
pkg/controller/production.go	`100% <100%> (ø)`	⬆️
pkg/controller/all-in-one.go	`100% <100%> (ø)`	⬆️
pkg/controller/controller.go	`100% <100%> (ø)`	⬆️
pkg/route/query.go	`100% <100%> (ø)`	⬆️
pkg/deployment/query.go	`100% <100%> (ø)`	⬆️
pkg/account/main.go	`100% <100%> (ø)`
pkg/inject/oauth-proxy.go	`100% <100%> (ø)`
pkg/service/query.go	`100% <100%> (ø)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06ceca0...c329960. Read the comment docs.

objectiser

Great job. Haven't tried it out - will do after next round of review.

README.adoc

deploy/examples/disable-oauth-proxy.yaml

objectiser · 2018-11-09T11:40:08Z

pkg/account/oauth-proxy.go

+			Name:      OAuthProxyAccountNameFor(jaeger),
+			Namespace: jaeger.Namespace,
+			Annotations: map[string]string{
+				"serviceaccounts.openshift.io/oauth-redirectreference.primary": getOAuthRedirectReference(jaeger),


Just wondering if we can make this more modular, so uses the platform parameter to govern what should be added here, and generate an error if not supported on the platform?

I started with that, but it felt weird to have if openshift all over the code base.

The idea behind the current approach is that we'd be deciding about the OAuth Proxy based on the enabled flag, and let the controller deal with setting the appropriate flag value based on the platform. The normalize operation will set it to false if not on OpenShift, and default to true if on OpenShift (leaving it as is if explicitly set).

Just thinking about future platforms - might be better to define the structure now to make it easier for other platforms to be added - as otherwise it could be quite messy to try to decouple the binary decisions between k8s and openshift at a later date.

objectiser · 2018-11-09T11:41:07Z

pkg/account/oauth-proxy.go

+}
+
+func getOAuthRedirectReference(jaeger *v1alpha1.Jaeger) string {
+	return fmt.Sprintf(`{"kind":"OAuthRedirectReference","apiVersion":"v1","reference":{"kind":"Route","name":"%s"}}`, jaeger.Name)


Same comment as above - move to a platform specific SPI?

pkg/account/oauth_proxy_test.go

objectiser · 2018-11-09T12:09:48Z

pkg/inject/oauth-proxy.go

+
+// OAuthProxy injects an appropriate OpenShift proxy into the given deployment
+func OAuthProxy(jaeger *v1alpha1.Jaeger, dep *appsv1.Deployment) *appsv1.Deployment {
+	if jaeger.Spec.Ingress.OAuthProxy == nil || !*jaeger.Spec.Ingress.OAuthProxy {


Shouldn't this also check for platform? All-in-one and production controllers don't check for platform before calling.

The normalize() operation will set the flag to false if the platform doesn't support the OAuth proxy. I should actually remove the reference to OpenShift from this godoc.

objectiser · 2018-11-09T12:13:59Z

pkg/inject/oauth-proxy_test.go

+	assert.Equal(t, "jaeger-query", dep.Spec.Template.Spec.Containers[0].Name)
+}
+
+func TestOAuthProxyContainerIsAdded(t *testing.T) {


May need to be updated if platform=openshift check added.

pkg/inject/oauth-proxy_test.go

objectiser · 2018-11-09T12:20:16Z

pkg/service/query.go

@@ -22,6 +24,9 @@ func NewQueryService(jaeger *v1alpha1.Jaeger, selector map[string]string) *v1.Se
 			Name:      GetNameForQueryService(jaeger),
 			Namespace: jaeger.Namespace,
 			Labels:    selector,
+			Annotations: map[string]string{
+				"service.alpha.openshift.io/serving-cert-secret-name": GetTLSSecretNameForQueryService(jaeger),


As mentioned above, it would be good if things like this could be moved into a platform specific SPI.

Good catch. I think this will be the only place outside of the inject package (and the normalize) that we have OpenShift-specific knowledge for the OAuth Proxy (the other feature being Routes vs. Ingresses). For now, I'll just add a check on the OAuth Proxy flag and wait for a second implementation before extracting a provider interface.

pkg/service/query_test.go

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

objectiser · 2018-11-09T15:47:01Z

Refactoring to support a platform SPI will be handled in a separate PR.

objectiser

Reviewed 5 of 20 files at r1, 17 of 18 files at r3.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jpkrohling and @objectiser)

jpkrohling requested a review from objectiser November 8, 2018 16:08

jpkrohling changed the title ~~WIP - Add OAuth Proxy to UI when on OpenShift~~ Add OAuth Proxy to UI when on OpenShift Nov 8, 2018

jpkrohling mentioned this pull request Nov 9, 2018

Add support for openshift-sar #102

Closed

objectiser reviewed Nov 9, 2018

View reviewed changes

jpkrohling force-pushed the 89-Protect-ui-with-OAuth-Proxy branch from 6c61685 to afaa92b Compare November 9, 2018 13:32

jpkrohling added 4 commits November 9, 2018 16:25

Add OAuth Proxy to UI when on OpenShift

f21592c

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Use consistent service account names for OAuth Proxy

f8b97de

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Small fixes based on code review

5c9ccca

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Move OAuth Proxy to generic security option

c329960

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

jpkrohling force-pushed the 89-Protect-ui-with-OAuth-Proxy branch from eda6ffc to c329960 Compare November 9, 2018 15:25

objectiser approved these changes Nov 9, 2018

View reviewed changes

jpkrohling mentioned this pull request Nov 9, 2018

Provider "SPI" #106

Closed

objectiser reviewed Nov 9, 2018

View reviewed changes

objectiser merged commit 3ddcd1f into jaegertracing:master Nov 9, 2018

jpkrohling mentioned this pull request Nov 9, 2018

Fix disable-oauth-proxy example #107

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OAuth Proxy to UI when on OpenShift #100

Add OAuth Proxy to UI when on OpenShift #100

jpkrohling commented Nov 8, 2018 •

edited

Loading

jpkrohling commented Nov 8, 2018

jpkrohling commented Nov 8, 2018 •

edited

Loading

codecov bot commented Nov 8, 2018 •

edited

Loading

objectiser left a comment

objectiser Nov 9, 2018

jpkrohling Nov 9, 2018

objectiser Nov 9, 2018

objectiser Nov 9, 2018

objectiser Nov 9, 2018

jpkrohling Nov 9, 2018

objectiser Nov 9, 2018

objectiser Nov 9, 2018

jpkrohling Nov 9, 2018

objectiser commented Nov 9, 2018

objectiser left a comment

Add OAuth Proxy to UI when on OpenShift #100

Add OAuth Proxy to UI when on OpenShift #100

Conversation

jpkrohling commented Nov 8, 2018 • edited Loading

jpkrohling commented Nov 8, 2018

jpkrohling commented Nov 8, 2018 • edited Loading

codecov bot commented Nov 8, 2018 • edited Loading

Codecov Report

objectiser left a 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

objectiser commented Nov 9, 2018

objectiser left a comment

Choose a reason for hiding this comment

jpkrohling commented Nov 8, 2018 •

edited

Loading

jpkrohling commented Nov 8, 2018 •

edited

Loading

codecov bot commented Nov 8, 2018 •

edited

Loading