Bug 1569220 - Add dashboard redirector feature #897

eriknelson · 2018-04-19T16:48:20Z

Describe what this PR does and why we need it:

This PR adds support for async provisioned dashboard urls. This is currently not possible with the OSB spec, so our solution is to add a redirect service that runs alongside the broker. The broker immediately returns a URL to the redirector service as a sort of "future" url, that will ultimately redirect to an APB's specified dashboard_url location at the conclusion of its provision run.

A few notes:

This feature requires an APB to be built on top of an apb-base that carries the dashboard redirector ansible module.
The broker author must specify in an alpha section of their APB spec that they would like to enable this feature. Example APB.
The deployment of the broker must have the route to the dashboard redirector service specified in the broker's configmap. We have a sensible default configured in the template for this.
This is entirely opt in; there should be no change to existing behavior unless the feature is explicitly opted into by an APB author.

Changes proposed in this pull request

Update bundle-lib and broker-client-go dependencies to bring in DashboardURL and Alpha feature gate support.
Update broker to check if conditions are right to use the feature.
Returns a DashboardURL on initial async provision response that points to the dashboard redirector service if the feature is enabled.
Adds dashboard-redirector binary and modifies the build files to bake it into the broker image.
Updates template to deploy the dashboard-redirector alongside the broker.

* New crd fields * Parameterize dashboard_redirector config value * Use correct service account with DR

* Add dashboard_redirector config value * Logic to return dashboard_url if settings are present

* broker-client-go && bundle-lib

* Comply with dep updates

mhrivnak · 2018-04-19T17:17:20Z

cmd/dashboard-redirector/main.go

+	logrus.Infof("DashboardURL found: %s, 301 redirecting", si.DashboardURL)
+	var redirectURL string
+	if !strings.HasPrefix("http", si.DashboardURL) {
+		redirectURL = fmt.Sprintf("http://%s", si.DashboardURL)


What's the use case behind this? Is it possible to just force whatever external entity is supplying the URL to supply a URL with a valid scheme?

Aside from the risk of assuming non-tls http, it's possible a service would have a management dashboard available through other non-http means, such as ssh.

If there is a use case that requires making this assumption, it would be helpful to add an in-line comment about it.

What's the use case behind this? Is it possible to just force whatever external entity is supplying the URL to supply a URL with a valid scheme?

When an APB passes back something without a scheme; this is more friendly and allows that case to pass and assume http rather than explicitly fail.

Aside from the risk of assuming non-tls http

It seems to me the worst case scenario here is that http is assumed when not supported by the service. Many will force a redirect to https.

it's possible a service would have a management dashboard available through other non-http means, such as ssh.

Interesting point.

I'm anticipating there will be APB authors that unintentionally do not provide a scheme and the intention is to help them with a reasonable assumption. Alternatively, we can lock this down, warn, and default to not providing the dashboard url if the scheme is missing. As I'm thinking about it, the latter might not be a bad idea since the majority of authors will probably get that service URL dynamically from some API and store it in a var to later provide to our module, greater increasing the likelyhood of having the scheme handled by the remote system.

mhrivnak

Could we get a tiny README or similar for the redirector that explains what it does and how to use it? Even just two or three sentences explaining how to make a request to it (GET request with "id" as a query parameter containing...) and where it gets its data would be a great start.

mhrivnak · 2018-04-19T17:35:30Z

cmd/dashboard-redirector/main.go

+	logrus.Infof("Got request for service instance %s, looking up dashboard_url", id)
+
+	si, err := crdDao.GetServiceInstance(id)
+	if err != nil {


What happens if the service instance doesn't exist? Would that result in err having a value here? Or would si just have the default zero value?

If it's the former, I think the StatusInternalServerError below is incorrect in that case and should be a 404, since the instance would have literally been "not found".

If it's the latter, then the log message "Successfully loaded" would be misleading.

Updated to check if it's actually an is not found error and will return the correct code in that case, otherwise will return an internal error.

👍 I'd change the value for errMsg too just to make it clear that all the software worked correctly, and your instance just doesn't exist. But that's a minor suggestion.

mhrivnak · 2018-04-19T17:38:57Z

templates/deploy-ansible-service-broker.template.yaml

+- apiVersion: v1
+  kind: Service
+  metadata:
+    name: dr


For someone looking at this Service and wondering what it is, could this be a touch more descriptive? It'll save us from answering the question "What is the 'dr' service?" a hundred times. ;)

mhrivnak · 2018-04-19T17:48:29Z

cmd/dashboard-redirector/main.go

+	var errMsg string
+	logrus.Info("Checking for form")
+
+	id := r.FormValue("id")


Is there any security risk in handing out the dashboard URL to anyone who knows the instance ID? Or is there some implied authorization happening before the request makes it this far?

Discussed in person, decided to file an issue here to address if/when the feature graduates from alpha.

* Use url pkg to check scheme and URL structure * Use correct return codes

eriknelson · 2018-04-19T20:30:54Z

@mhrivnak Think I addressed your requests for change, let me know if I missed anything!

shawn-hurley · 2018-04-19T20:37:00Z

pkg/broker/broker.go

+			return false
+		}
+		if a.brokerConfig.DashboardRedirector == "" {
+			warnMsg := fmt.Sprintf("Attempting to provision %v", spec.FQName) +


I personally prefer log.Warningf and then you can remove the string concatenation.

Do you have a preferred alternative to adding the strings? I really can't stand 130 char error messages, and that's all I care about :)

You could do one of the following, use tick marks:

warnMsg := `Attempting to provision %v, which has dashboard redirect enabled, but no dashboard_redirector route was found in the broker's configmap. Deploying without a dashboard_url.` log.Warningf(warnMsg, spec.FQName)

You could keep the concatenation but remove the sprintf using the Warningf version:

warnMsg1 := "Attempting to provision %v, which has dashboard redirect enabled" + "but no dashboard_redirector route was found in the broker's " + "configmap. Deploying without a dashboard_url." log.Warningf(warnMsg1, "FQName")

Or you could reword the message to be shorter:

// short message, no fqname log.Warning("Deploying without a dashboard_url, no dashboard_redirector route found") // short message with fqname log.Warningf("Deploying without a dashboard_url, no dashboard_redirector route found for %v", spec.FQName)

shawn-hurley · 2018-04-19T20:40:30Z

pkg/broker/broker.go

+
+	var response *ProvisionResponse
+
+	dashboardRedirectEnabled := func() bool {


should we make this it's own helper function below and not a function defined in a function?

Also do we need the dashboard url for GetServiceInstance

Also do we need the dashboard url for GetServiceInstance

Good catch. We have a GET service instance endpoint, but I'm not sure that there is actually anything calling it? Ironically in the future, it will probably be the spec support that actually implements the workaround this PR is trying to address.

I think I'm going to add a helper that encapsulates getProvisionResponse given a service instance, and we can hide the details there. I prefer inline function defs like this because I'm trying to define what it means to be enabled at a higher level when the conditionals start to get more complex than a simple boolean expression in the conditional. I prefer inline because it's close to the call site.

but I'm not sure that there is actually anything calling it?

👍

I prefer inline function defs like this because I'm trying to define what it means to be enabled at a higher level when the conditionals start to get more complex than a simple boolean expression in the conditional. I prefer inline because it's close to the call site.

I personally worry about to many inline functions. I believe that at a certain point they make it harder to follow the function. With that said I this one makes sense if it is all encapsulated in another helper function 👍 from me

shawn-hurley · 2018-04-19T20:41:26Z

pkg/broker/job_state_subscriber.go

@@ -44,6 +44,22 @@ func (jss *JobStateSubscriber) Notify(msg JobMsg) {
 			return
 		}
 	}
+	//TODO: Need to get the service instance
+	//TODO: NOTE: THIS NEEDS TO BREAK OUT TO OWN SUBSCRIBER


I think if we have the time, this might be something we should do.

I don't think it needs a different subscriber, but it could definitely use a helper function. It feels weird where it is now.

func (jss *JobStateSubscriber) handleDashboardURL(msg JobMsg) { // Update with dashboard URL. if msg.DashboardURL != "" { instance, err := jss.dao.GetServiceInstance(msg.InstanceUUID) if err != nil { log.Errorf("Error after job succeeded : %v", err) return } instance.DashboardURL = msg.DashboardURL err = jss.dao.SetServiceInstance(msg.InstanceUUID, instance) if err != nil { log.Errorf("Error after job succeeded : %v", err) return } }

The Notify would then look cleaner:

// Notify external API to notify this subscriber of a change in the Job func (jss *JobStateSubscriber) Notify(msg JobMsg) { log.Debugf("JobStateSubscriber Notify : msg state %v ", msg.State) id := msg.InstanceUUID if isBinding(msg) { id = msg.BindingUUID } if _, err := jss.dao.SetState(id, msg.State); err != nil { log.Errorf("Error JobStateSubscriber failed to set state after action %v completed with state %s err: %v", msg.State.Method, msg.State.State, err) return } if msg.State.State == apb.StateSucceeded { if err := jss.handleSucceeded(msg); err != nil { log.Errorf("Error after job succeeded : %v", err) return } } handleDashboardURL(msg) return }

shawn-hurley · 2018-04-19T20:44:16Z

templates/deploy-ansible-service-broker.template.yaml

@@ -167,6 +184,36 @@ objects:
            secret:
              secretName: asb-auth-secret

+- apiVersion: v1


I think before we merge we should get this working as a container inside the asb pod IMO

My belief is that this is so we don't have to expose a public facing route in order for the broker to be able to talk to it. Is that right?

@djzager that's is true, but the broker in this case never talks to the redirector. It's for external clients (OpenShift UI).

mhrivnak · 2018-04-19T21:04:46Z

cmd/dashboard-redirector/README.md

+
+Currently, the OSB spec does not allow for the brokers to return a `dashboard_url`
+at the conclusion of an async provision. This is problematic because brokers
+(and bundles, in our case) may not actually know their `dashboard_url` if its


s/its/it's/

mhrivnak · 2018-04-19T21:06:35Z

cmd/dashboard-redirector/README.md

+As a workaround, our broker runs this "dashboard-redirector" as a simple
+service that accepts a service instance ID, and looks up the service instance
+in our DAO. If the `DashboardURL` is present on the service instance, the
+dashboard-redirector will return a 302 redirect to that location. Otherwise,


mhrivnak · 2018-04-19T21:09:43Z

cmd/dashboard-redirector/main.go

+	logrus.Infof("Got request for service instance %s, looking up dashboard_url", id)
+
+	si, err := crdDao.GetServiceInstance(id)
+	if err != nil {


👍 I'd change the value for errMsg too just to make it clear that all the software worked correctly, and your instance just doesn't exist. But that's a minor suggestion.

dymurray · 2018-04-20T12:50:34Z

Still trying to get the DR container in the same pod as ASB.... routes are not cooperating but will push here when its done.

jmrodri

+1 to all the others comments on this.

jmrodri · 2018-04-20T13:30:44Z

pkg/broker/broker.go

+			return false
+		}
+		if a.brokerConfig.DashboardRedirector == "" {
+			warnMsg := fmt.Sprintf("Attempting to provision %v", spec.FQName) +


You could do one of the following, use tick marks:

warnMsg := `Attempting to provision %v, which has dashboard redirect enabled, but no dashboard_redirector route was found in the broker's configmap. Deploying without a dashboard_url.` log.Warningf(warnMsg, spec.FQName)

You could keep the concatenation but remove the sprintf using the Warningf version:

warnMsg1 := "Attempting to provision %v, which has dashboard redirect enabled" + "but no dashboard_redirector route was found in the broker's " + "configmap. Deploying without a dashboard_url." log.Warningf(warnMsg1, "FQName")

Or you could reword the message to be shorter:

// short message, no fqname log.Warning("Deploying without a dashboard_url, no dashboard_redirector route found") // short message with fqname log.Warningf("Deploying without a dashboard_url, no dashboard_redirector route found for %v", spec.FQName)

jmrodri · 2018-04-20T13:40:06Z

cmd/dashboard-redirector/main.go

+	if err != nil {
+		panic(fmt.Sprintf("Sanity check failed! -> %v", err))
+	} else {
+		logrus.Info("Sanity check passed! Loaded specs: %v", specs)


What does this print out?

@jmrodri don't the ticks include the newlines? If so, gotta be honest I don't think any of those options really help :P Guess I'll throw this in my pile of go complaints.

@eriknelson yeah the tick marks will include the new lines. Kind of like the python doc strings.

eriknelson · 2018-04-20T13:55:36Z

@mhrivnak Think I hit got your second round of comments.

@shawn-hurley I'm not sure I understand why we need another subscriber? Could you elaborate on that?

I just tested @dymurray's changes to the template and can confirm it's working in a single pod. Turns out there should only be a single service and multiple ports declared. Makes sense since the containers share the same port space of the virtual ip (svc).

eriknelson · 2018-04-20T13:59:50Z

@fabianvf think these template additions probably need to make their way into openshift-ansible.

fabianvf · 2018-04-20T14:04:18Z

@eriknelson could you break the template changes out to a 3.10 aligned Trello card?

dymurray · 2018-04-23T13:58:48Z

@fabianvf @eriknelson https://trello.com/c/b9eEU5Lk

I am working on a PR that I will need some help testing.

dymurray · 2018-04-23T14:49:39Z

openshift/openshift-ansible#8086

mhrivnak · 2018-04-23T17:18:29Z

cmd/dashboard-redirector/main.go

+			errMsg = fmt.Sprintf("Something went wrong trying to load service instance [%s] -> %s", id, err)
+			http.Error(w, errMsg, http.StatusInternalServerError)
+		}
+		logrus.Errorf(errMsg, id, err.Error())


One last nit to pick: here a 404 gets logged as at "Error", but below it gets logged at "Info". It would be better to have them at the same level, and I'd suggest Info. It's not a problematic or exceptional case by any means for the service instance to not be found.

djzager

LGTM. One comment.

djzager · 2018-04-24T13:21:22Z

templates/deploy-ansible-service-broker.template.yaml

@@ -517,6 +546,8 @@ objects:
            properties:
              specId:
                type: string
+              dashboardUrl:
+                type: string
              context:
                type: object
                properties:


Adding here, since I can't add it where it belongs. On line 554.

properties: plateform: type: string namespace: type: string

Should we fix this?

Shawn Hurley and others added 14 commits April 17, 2018 15:36

adding dashboard url

428cf1e

Add dashUrl return for Provision Response, Hardcoded

2bfb8a3

Add template for DR

247f56b

Dashboard redirector

27528f5

Updates

0b4f81d

Merge branch 'dashboard-redirector' into feature-add-dashboard-url

a15b44e

Add DR to template

d638631

Fix tests

98abc14

Update to tmp. use dashboard branches

d95feb6

Update deployment template

37c3095

* New crd fields * Parameterize dashboard_redirector config value * Use correct service account with DR

Support dashboard_redirector

0c5d814

* Add dashboard_redirector config value * Logic to return dashboard_url if settings are present

Put serviceAccount in the right place for dr

85c590b

Update deps

6ac92e0

* broker-client-go && bundle-lib

Updates to pass make check

96bfa1a

* Comply with dep updates

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 19, 2018

eriknelson requested review from shawn-hurley and jmrodri April 19, 2018 16:48

eriknelson added the needs-review label Apr 19, 2018

mhrivnak reviewed Apr 19, 2018

View reviewed changes

mhrivnak requested changes Apr 19, 2018

View reviewed changes

dymurray changed the title ~~Add dashboard redirector feature~~ Bug 1569220 - Add dashboard redirector feature Apr 19, 2018

eriknelson added 3 commits April 19, 2018 16:11

Add dashboard-redirector README

4a93fbf

Update the dr route to be more descriptive

6055ba7

Address PR feedback

7aa5f31

* Use url pkg to check scheme and URL structure * Use correct return codes

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 19, 2018

shawn-hurley requested changes Apr 19, 2018

View reviewed changes

mhrivnak requested changes Apr 19, 2018

View reviewed changes

README fixes

daeb47e

Set correct errMsg when si is not found

6c786f8

djzager mentioned this pull request Apr 20, 2018

Update Broker APB with dashboard redirector automationbroker/automation-broker-apb#10

Open

Move dashboard redirector container within asb pod

8a018b7

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 20, 2018

jmrodri approved these changes Apr 20, 2018

View reviewed changes

dymurray approved these changes Apr 23, 2018

View reviewed changes

Breakout dashboard helper function

6a74ee8

mhrivnak requested changes Apr 23, 2018

View reviewed changes

Log at info level for 404s

f054515

mhrivnak approved these changes Apr 23, 2018

View reviewed changes

shawn-hurley approved these changes Apr 23, 2018

View reviewed changes

djzager approved these changes Apr 24, 2018

View reviewed changes

djzager mentioned this pull request Apr 24, 2018

Update templates for dashboard redirector feature automationbroker/automation-broker-apb#11

Closed

jmrodri merged commit 50d8490 into master Apr 24, 2018

eriknelson deleted the feature-add-dashboard-url branch April 24, 2018 15:35


		var response *ProvisionResponse

		dashboardRedirectEnabled := func() bool {

Bug 1569220 - Add dashboard redirector feature #897

Bug 1569220 - Add dashboard redirector feature #897

Conversation

eriknelson commented Apr 19, 2018 • edited by jmrodri

Describe what this PR does and why we need it:

Changes proposed in this pull request

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhrivnak 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

eriknelson commented Apr 19, 2018

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

shawn-hurley Apr 20, 2018 • edited

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

dymurray commented Apr 20, 2018

jmrodri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriknelson Apr 20, 2018 • edited

Choose a reason for hiding this comment

jmrodri Apr 20, 2018 • edited

Choose a reason for hiding this comment

eriknelson commented Apr 20, 2018 • edited

eriknelson commented Apr 20, 2018 • edited

fabianvf commented Apr 20, 2018

dymurray commented Apr 23, 2018

dymurray commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djzager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriknelson commented Apr 19, 2018 •

edited by jmrodri

shawn-hurley Apr 20, 2018 •

edited

eriknelson Apr 20, 2018 •

edited

jmrodri Apr 20, 2018 •

edited

eriknelson commented Apr 20, 2018 •

edited

eriknelson commented Apr 20, 2018 •

edited