add localhost:9000 as a redirect URL #10895

juanvallejo · 2016-09-13T21:13:24Z

Depends on: #10819
Fixes: #10885

This patch adds https://localhost:9000 as a default redirect URI to
the webconsole oauthclient. This is done as a new oc cluster up
startup task.

$ oc cluster up

...
-- Finding server IP ...
   Using <IP> as the server IP
-- Starting OpenShift container ...
   Creating initial OpenShift configuration
   Starting OpenShift using container 'origin'
   Waiting for API server to start listening
   OpenShift server started
-- Adding default OAuthClient redirect URIs ... OK
-- Installing registry ... OK
-- Installing router ... OK
-- Importing image streams ... OK
-- Importing templates ... OK
-- Login to server ... OK
-- Creating initial project "myproject" ... OK
...

$ oc login -u system:admin
$ oc get oauthclients

NAME                              WWW-CHALLENGE   REDIRECT URIS
openshift-web-console             FALSE           https://localhost:9000

cc @fabianofranz @jwforres @csrwng

juanvallejo · 2016-09-13T21:16:12Z

pkg/bootstrap/docker/up.go

@@ -45,6 +46,9 @@ const (
 	initialProjectDisplay = "My Project"
 	initialProjectDesc    = "Initial developer project"

+	defaultRedirectClient = "oauthclient/openshift-web-console"


@fabianofranz
Although I could use the const OpenShiftWebConsoleClientID defined in pkg/cmd/server/origin/auth.go, I did not want to import server packages, but WDYT?

Hrm, yeah, that could result in bringing to oc packages that don't belong here (master.go for example imports a lot of server-side stuff).

jwforres · 2016-09-13T21:20:48Z

cc @liggitt

csrwng · 2016-09-13T21:22:08Z

@juanvallejo we currently update the config here:
https://github.com/openshift/origin/blob/master/pkg/bootstrap/docker/openshift/helper.go#L446

My preference would be to change it there rather than executing a separate patch command.

liggitt · 2016-09-13T21:44:26Z

It's not a server config update, it's an update of the API object

csrwng · 2016-09-13T21:47:54Z

ahh got it, thx @liggitt. @juanvallejo please ignore me and carry on :)

juanvallejo · 2016-09-13T21:48:50Z

:) Thanks for the feedback!

juanvallejo · 2016-09-13T21:59:56Z

[test]

liggitt · 2016-09-14T01:35:07Z

pkg/bootstrap/docker/up.go

+		return err
+	}
+
+	patch := fmt.Sprintf("{%q:[%q]}", "redirectURIs", defaultRedirectURI)


raises eyebrow... this should really be built by json-serializing a struct

actually, why not use the go client to make the call?

@csrwng

actually, why not use the go client to make the call?

Hm, I am not sure what you mean. I thought NewCmdPatch was already using the client to make the remote call?

@juanvallejo i mean not invoking the command, simply doing a client get and then an update call with your change.

@csrwng Okay, updated to not using NewCmdPatch, PTAL

csrwng · 2016-09-15T15:21:11Z

@juanvallejo actually that's not what I had in mind :)
I was thinking of something like:

client, _, err := c.Clients()
webConsoleOAuth, err := client.OAuthClients().Get("openshift-web-console")
// not shown here... check first and don't add it if it's already there
webConsoleOAuth.RedirectURIs = append(webConsoleOAuth.RedirectURIs, "http://localhost:9000")  
err = client.OAuthClients().Update(webConsoleOAuth)

However, I realized that there's no Update method on the client for OAuthClients (you could add one of course)... @liggitt was that left out on purpose?

liggitt · 2016-09-15T15:23:38Z

@liggitt was that left out on purpose?

nope

juanvallejo · 2016-09-15T19:04:59Z

@csrwng Thanks for the feedback!

However, I realized that there's no Update method on the client for OAuthClients (you could add one of course)

I have gone ahead and added an Update method to the OAuthClientInterface that receives an *oauthapi.OAuthClient and then makes a Patch request to the server, however the body of the request must contain the patch data ({"redirectURIs": ["https://localhost:9000"]}) as a json object. I was wondering if there is a way to obtain this from the OAuthClient that is passed, or would I just have to Marshal a json struct like I was doing before, and pass that as []bytes to the new Update method? Thanks!

liggitt · 2016-09-15T19:15:01Z

I have gone ahead and added an Update method to the OAuthClientInterface that receives an *oauthapi.OAuthClient and then makes a Patch request to the server

no, make Update do an update. look at the other Update client methods and model after that.

just Get, add the redirectURI, and Update

csrwng · 2016-09-15T19:18:07Z

@juanvallejo All you need to do for the update implementation is to do a put. For examples, you can see how other object updates are implemented:
https://github.com/openshift/origin/blob/master/pkg/client/oauthclientauthorization.go#L41
https://github.com/openshift/origin/blob/master/pkg/client/clusteresourcequota.go#L56

liggitt · 2016-09-15T19:42:15Z

pkg/client/oauthclient.go

@@ -17,6 +17,7 @@ type OAuthClientInterface interface {
 	Get(name string) (*oauthapi.OAuthClient, error)
 	Delete(name string) error
 	Watch(opts kapi.ListOptions) (watch.Interface, error)
+	Update(client *oauthapi.OAuthClient) (*oauthapi.OAuthClient, error)


need to update fake_oauthclient.go to implement this as well

will do, thanks!

csrwng · 2016-09-21T18:37:38Z

LGTM after a squash

juanvallejo · 2016-09-21T21:15:10Z

re[test]

juanvallejo · 2016-09-22T13:53:46Z

conformance test flaked on #9548 re[test]

liggitt

a couple nits on names and messages. once updated, this can go ahead and merge... it doesn't need to wait for #10819

liggitt · 2016-09-26T18:50:55Z

pkg/bootstrap/docker/up.go

@@ -45,6 +46,9 @@ const (
 	initialProjectDisplay = "My Project"
 	initialProjectDesc    = "Initial developer project"

+	defaultRedirectClient = "openshift-web-console"
+	defaultRedirectURI    = "https://localhost:9000"


nit: this isn't the default redirect URI, it's an additional allowed one... more a "developmentRedirectURI"

liggitt · 2016-09-26T18:55:08Z

pkg/bootstrap/docker/up.go

+		return nil
+	}
+
+	webConsoleOAuth, err := oc.OAuthClients().Get(defaultRedirectClient)


if the web console OAuth client doesn't exist, I would exit early, rather than error

if NotFound, return nil, otherwise, return err

@liggitt Thanks for clarifying! Updated to check if err is kerrors.IsNotFound

liggitt · 2016-09-26T18:56:03Z

pkg/bootstrap/docker/up.go

+	if err != nil {
+		// announce error without interrupting remaining tasks
+		suggestedCmd := fmt.Sprintf("oc patch %s -p '{%q:[%q]}'", "oauthclient/openshift-web-console", "redirectURIs", defaultRedirectURI)
+		errMsg := fmt.Sprintf("Unable to set %q as a default redirect URL for the web console.\nTo manually add it, run %q\n", defaultRedirectURI, suggestedCmd)


"unable to add development redirect URI to the openshift-web-console OAuthClient..."

juanvallejo · 2016-09-26T20:29:21Z

@liggitt Addressed review comments, PTAL

liggitt · 2016-09-26T20:55:29Z

pkg/bootstrap/docker/up.go

+			fmt.Fprintf(out, "Unable to find OAuthClient %q\n", defaultRedirectClient)
+			return nil
+		}
+		return err


if we tolerate an update failure (below), we should probably tolerate a fetch failure as well (not return the error), and print out the same suggestion

@liggitt Sounds good, it no longer returns the err but rather prints a slightly updated suggestion and returns nil

liggitt · 2016-09-26T21:12:35Z

LGTM

Fixes: openshift#10885 This patch adds `https://localhost:9000` as a default redirect URI to the webconsole oauthclient. This is done as a new `oc cluster up` startup task. ``` $ oc cluster up ... -- Finding server IP ... Using <IP> as the server IP -- Starting OpenShift container ... Creating initial OpenShift configuration Starting OpenShift using container 'origin' Waiting for API server to start listening OpenShift server started -- Adding default oAuthClient redirect URIs ... "openshift-web-console" patched -- Installing registry ... OK -- Installing router ... OK -- Importing image streams ... OK -- Importing templates ... OK -- Login to server ... OK -- Creating initial project "myproject" ... OK ... ``` ``` $ oc login -u system:admin $ oc get oauthclients NAME WWW-CHALLENGE REDIRECT URIS openshift-web-console FALSE https://localhost:9000 ```

juanvallejo · 2016-09-26T21:40:36Z

[test]

openshift-bot · 2016-09-27T08:03:15Z

Evaluated for origin test up to b183604

openshift-bot · 2016-09-27T10:17:54Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9324/)

liggitt · 2016-09-27T15:03:34Z

[merge]

openshift-bot · 2016-09-27T15:07:30Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9324/) (Image: devenv-rhel7_5084)

openshift-bot · 2016-09-27T15:07:31Z

Evaluated for origin merge up to b183604

smarterclayton · 2016-09-27T18:33:46Z

Maybe I missed this - why is "localhost" the correct value?

smarterclayton · 2016-09-27T18:34:33Z

Won't localhost be wrong if my docker daemon isn't running on my system?

liggitt · 2016-09-27T18:34:39Z

this is enabling web console development (cd $webconsole && grunt serve) out of the box against a cluster up server

smarterclayton · 2016-09-27T19:11:50Z

Can you add some Godoc to this, because nothing in here makes it obvious why it's localhost:9000, and so it's really for a very specific audience, so that usually requires some justification in the code so other people don't remove it.

Spawned from: openshift#10895 (comment) This patch adds a brief explanation for the use of "localhost:9000" as a default develppment redirect URI in the `oc cluster up` setup.

juanvallejo · 2016-09-27T19:28:57Z

@smarterclayton I was not sure if making a new pull request was the way I should do this, but please take a look: #11123

Spawned from: openshift#10895 (comment) This patch adds a brief explanation for the use of "localhost:9000" as a default develppment redirect URI in the `oc cluster up` setup.

juanvallejo reviewed Sep 13, 2016
View reviewed changes

juanvallejo force-pushed the jvallejo_add-localhost-9000-as-default-redirect-oc-cluster branch from d5e389c to 5a80c23 Compare September 13, 2016 21:58

liggitt reviewed Sep 14, 2016
View reviewed changes

juanvallejo force-pushed the jvallejo_add-localhost-9000-as-default-redirect-oc-cluster branch 2 times, most recently from 5e787cd to 4b07d66 Compare September 14, 2016 19:43

juanvallejo changed the title ~~add localhost:9000 as a default redirect URL~~ DEPENDS ON #10819: add localhost:9000 as a default redirect URL Sep 15, 2016

liggitt reviewed Sep 15, 2016

View reviewed changes

juanvallejo force-pushed the jvallejo_add-localhost-9000-as-default-redirect-oc-cluster branch 4 times, most recently from d99ce99 to dec1b11 Compare September 16, 2016 13:49

juanvallejo force-pushed the jvallejo_add-localhost-9000-as-default-redirect-oc-cluster branch from dec1b11 to 0bed3c0 Compare September 21, 2016 18:53

liggitt reviewed Sep 26, 2016

View reviewed changes

juanvallejo force-pushed the jvallejo_add-localhost-9000-as-default-redirect-oc-cluster branch from 0bed3c0 to 2367bcb Compare September 26, 2016 20:27

juanvallejo force-pushed the jvallejo_add-localhost-9000-as-default-redirect-oc-cluster branch 2 times, most recently from f89f0ca to 596047f Compare September 26, 2016 20:45

liggitt changed the title ~~DEPENDS ON #10819: add localhost:9000 as a default redirect URL~~ add localhost:9000 as a default redirect URL Sep 26, 2016

liggitt changed the title ~~add localhost:9000 as a default redirect URL~~ add localhost:9000 as a redirect URL Sep 26, 2016

liggitt reviewed Sep 26, 2016

View reviewed changes

juanvallejo force-pushed the jvallejo_add-localhost-9000-as-default-redirect-oc-cluster branch from 596047f to 056b1ce Compare September 26, 2016 21:03

juanvallejo force-pushed the jvallejo_add-localhost-9000-as-default-redirect-oc-cluster branch from 056b1ce to b183604 Compare September 26, 2016 21:33

openshift-bot merged commit 53d71cb into openshift:master Sep 27, 2016

juanvallejo deleted the jvallejo_add-localhost-9000-as-default-redirect-oc-cluster branch September 27, 2016 18:35

juanvallejo mentioned this pull request Sep 27, 2016

Add godoc to justify use of developmentRedirectURI #11123

Merged

add localhost:9000 as a redirect URL #10895

add localhost:9000 as a redirect URL #10895

Conversation

juanvallejo commented Sep 13, 2016 • edited by liggitt Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwforres commented Sep 13, 2016

csrwng commented Sep 13, 2016

liggitt commented Sep 13, 2016

csrwng commented Sep 13, 2016

juanvallejo commented Sep 13, 2016

juanvallejo commented Sep 13, 2016

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

csrwng commented Sep 15, 2016

liggitt commented Sep 15, 2016

juanvallejo commented Sep 15, 2016

liggitt commented Sep 15, 2016

csrwng commented Sep 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csrwng commented Sep 21, 2016

juanvallejo commented Sep 21, 2016

juanvallejo commented Sep 22, 2016

liggitt 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

juanvallejo commented Sep 26, 2016

liggitt Sep 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Sep 26, 2016

juanvallejo commented Sep 26, 2016

openshift-bot commented Sep 27, 2016

openshift-bot commented Sep 27, 2016

liggitt commented Sep 27, 2016

openshift-bot commented Sep 27, 2016 • edited Loading

openshift-bot commented Sep 27, 2016

smarterclayton commented Sep 27, 2016

smarterclayton commented Sep 27, 2016

liggitt commented Sep 27, 2016 • edited Loading

smarterclayton commented Sep 27, 2016

juanvallejo commented Sep 27, 2016

juanvallejo commented Sep 13, 2016 •

edited by liggitt

Loading

liggitt Sep 26, 2016 •

edited

Loading

openshift-bot commented Sep 27, 2016 •

edited

Loading

liggitt commented Sep 27, 2016 •

edited

Loading