make username available for APB in the serviceInstance #832

johnkim76 · 2018-03-12T18:24:32Z

Describe what this PR does and why we need it:

username information was requested to be available for APBs

Changes proposed in this pull request

pass the username info from the broker to the APB during it's executions as a parameter in extra_vars

Addresses the following Issue

Issue #674

johnkim76 · 2018-03-12T18:25:27Z

(3) AWS Broker RFE: make serviceInstance details available to the playbook #674

shawn-hurley · 2018-03-12T19:26:14Z

I think that if we are planning on passing this down to the APB we should pass it down for all methods, not just provision IMO.

johnkim76 · 2018-03-12T19:32:29Z

@shawn-hurley ok, I will update. I wasn't sure if it was necessary to do so for all methods, but I can make the changes.

jmrodri

I have one question for @eriknelson and @shawn-hurley , the rest looks goods.

jmrodri · 2018-03-12T19:22:48Z

pkg/handler/handler.go

@@ -274,6 +274,7 @@ func (h handler) provision(w http.ResponseWriter, r *http.Request, params map[st
 		return
 	}

+	var username = "default"


this is a little weird. I would've left it empty and if it is still empty when we call the apb, we don't pass anything to the apb.

I can update this...

jmrodri · 2018-03-12T19:24:18Z

pkg/apb/types.go

@@ -224,6 +224,8 @@ const (
 	ClusterKey = "cluster"
 	// NamespaceKey parameter name passed to APBs
 	NamespaceKey = "namespace"
+	// UsernameKey parameter name passed to APBs
+	UsernameKey = "username"


@eriknelson @shawn-hurley is this supposed to be _apb_username or is username okay?

Would it make more sense place the username here and set it with the others here and NOT put it in the ServiceInstance? I put it in the serviceInstance so that it retains the username info in the instance... but I can change it.

@johnkim76 I think it's okay on the ServiceInstance, Since an instance should never have multiple users requesting it.

Consider the scenario where the broker is set to auto_esclate UserA and UserB have enough access in the target namespace. UserA creates a service postgresql and then UserB creates a binding for that service. That would be two users requesting the same service instance.

I agree @johnkim76 that this key should be _apb_requesting_user and should behave similarly those that you posted above.

jmrodri · 2018-03-12T22:52:36Z

I think that if we are planning on passing this down to the APB we should pass it down for all methods, not just provision IMO.
@shawn-hurley good catch.

shawn-hurley · 2018-03-13T13:08:49Z

pkg/apb/types.go

@@ -224,6 +224,8 @@ const (
 	ClusterKey = "cluster"
 	// NamespaceKey parameter name passed to APBs
 	NamespaceKey = "namespace"
+	// UsernameKey parameter name passed to APBs
+	UsernameKey = "username"


Consider the scenario where the broker is set to auto_esclate UserA and UserB have enough access in the target namespace. UserA creates a service postgresql and then UserB creates a binding for that service. That would be two users requesting the same service instance.

I agree @johnkim76 that this key should be _apb_requesting_user and should behave similarly those that you posted above.

shawn-hurley · 2018-03-13T13:11:29Z

pkg/broker/broker.go

@@ -485,7 +485,7 @@ func (a AnsibleBroker) Catalog() (*CatalogResponse, error) {
 }

 // Provision  - will provision a service
-func (a AnsibleBroker) Provision(instanceUUID uuid.UUID, req *ProvisionRequest, async bool,
+func (a AnsibleBroker) Provision(instanceUUID uuid.UUID, req *ProvisionRequest, async bool, username string,


You will need to do the same for all the other actions to truely pass this for every method.

@eriknelson @jmrodri What do you guys think of having him add the requesting user UserInfo to the provision request?

@shawn-hurley ProvisionRequest sounds better, but it usually maps to the OSB ProvisionRequest. Unless we add it as part of the parameters, then I'd be okay with that.

shawn-hurley · 2018-03-13T13:12:07Z

pkg/handler/handler.go

@@ -289,11 +290,13 @@ func (h handler) provision(w http.ResponseWriter, r *http.Request, params map[st
 			writeResponse(w, status, broker.ErrorResponse{Description: err.Error()})
 			return
 		}
+
+		username = userInfo.Username


I would suggest that you may need to add logic because it could be the case that we do not have Username but UID from the UserInfo of the requesting user.

eriknelson

I was just posting about how Username feels shoehorned into a ServiceInstance because it's really the only way to pass the data through to something like a ProvisionJob: https://github.com/openshift/ansible-service-broker/blob/master/pkg/broker/broker.go#L636
when I re-read the discussion and saw @shawn-hurley advocating for the _apb_requesting_user. I think that's the right way to go. I'm concerned about continuing to add fields to the ServiceInstance proper that maybe don't belong there, especially in light of bundle-lib when we are expecting these types to be generically used by bundle-lib consumers.

mhrivnak

Please also add this new field to the service bundle contract doc, including the broker version where it will first be available.

shawn-hurley

This is very close 👍

I have just one question about what to do on deprovision, unbind, update.

shawn-hurley · 2018-03-21T13:27:37Z

pkg/broker/broker.go

@@ -660,7 +663,7 @@ func (a AnsibleBroker) Provision(instanceUUID uuid.UUID, req *ProvisionRequest,

 // Deprovision - will deprovision a service.
 func (a AnsibleBroker) Deprovision(
-	instance apb.ServiceInstance, planID string, skipApbExecution bool, async bool,
+	instance apb.ServiceInstance, planID string, skipApbExecution bool, async bool, userInfo UserInfo,


I don't see the update of the parameters[requestingUserKey] = getRequestingUser(userInfo) for Deprovision or Unbind or Update.

I think we need to do this be a separate user could request those actions and if we want to alert the APBs of the users that are interacting with them, I think that we should update it. I wonder if we should re-name this field then to _apb_last_requesting_user?

I have updated as you advised, to override the userInfo in the Deprovision, Unbind, and Update

shawn-hurley · 2018-03-21T15:23:05Z

pkg/handler/handler.go

@@ -646,8 +646,8 @@ func (h handler) unbind(w http.ResponseWriter, r *http.Request, params map[strin
 		return
 	}

+	userInfo, ok := r.Context().Value(UserInfoContext).(broker.UserInfo)


We need to make sure it is clear that this could be empty. If the service catalog does not have the correct feature gate on we will not get this header. I think this should be fine but is a documentation on the service bundle contract.

mhrivnak · 2018-03-21T20:01:24Z

docs/service-bundle.md

+
+The requesting username of the [actions](#actions) _provision_, _deprovision_, _bind_, _unbind_, and _update_ will available in the `_apb_last_requesting_user` parameter. The parameter value will be the `UID` of the requesting user if the `username` is not available (e.g. auto escalation). Also, the user info of the latest request will always overwrite the existing value.
+
+This feature will be available in v3.10


I think this should be a broker version, right?

As a nit-pick, I think it makes sense for these to be present-tense. "This field is new in version X.Y." Or even past tense: "This field was introduced in version X.Y."

Users aren't likely to care about this field or read about it until after its release.

release-1.2?

oh, ok, so I'll make those updates

I think I'd go with just the version number rather than a branch name, but either way I think that's the best number to convey.

eriknelson · 2018-03-21T20:33:19Z

pkg/broker/broker.go

@@ -708,6 +711,13 @@ func (a AnsibleBroker) Deprovision(
 		instance.Parameters = &params
 	}

+	// Override the lastRequestingUserKey value in the instance.Parameters
+	if instance.Parameters != nil {
+		params := *instance.Parameters


So Parameters here is already a reference type, you don't have to deref and then copy the address back to Parameters. I'd update these on each to just be a one liner:

(*instance.Parameters)[lastRequestingUserKey] = getLastRequestingUser(userInfo)

https://play.golang.org/p/wG6632X68h7

thanks, I will update

override userInfo for Deprovision, Update, Unbind

shawn-hurley · 2018-03-22T14:41:13Z

docs/service-bundle.md

@@ -127,12 +129,18 @@ Example of JSON document passed to __bind__:
  provision action for this service instance. Details are below in the Output
  section.

+#### Last Request User
+
+The requesting username of the [actions](#actions) _provision_, _deprovision_, _bind_, _unbind_, and _update_ is available in the `_apb_last_requesting_user` parameter. The parameter is set to the `UID` if the requesting `username` is empty (e.g. auto escalation). Also, the user info of the latest request will always overwrite the existing value.


I think we should note that this could be completely empty as it is not a requirement to send the requesting user.

Sorry @johnkim76 one more change?

thanks, will update

eriknelson

@johnkim76 Ah, I see why you were updating the params that way, we were just doing in other places as well.

Ack, looks good 👍

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 12, 2018

johnkim76 requested a review from jmrodri March 12, 2018 18:30

jmrodri requested a review from shawn-hurley March 12, 2018 19:20

jmrodri added the feature label Mar 12, 2018

jmrodri approved these changes Mar 12, 2018

View reviewed changes

shawn-hurley requested changes Mar 13, 2018

View reviewed changes

eriknelson suggested changes Mar 16, 2018

View reviewed changes

johnkim76 force-pushed the provide_apb_info branch from d88f8e5 to e89d5b7 Compare March 16, 2018 15:45

mhrivnak requested changes Mar 19, 2018

View reviewed changes

johnkim76 force-pushed the provide_apb_info branch from e89d5b7 to 5d6e261 Compare March 21, 2018 12:39

shawn-hurley reviewed Mar 21, 2018

View reviewed changes

shawn-hurley approved these changes Mar 21, 2018

View reviewed changes

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 21, 2018

johnkim76 force-pushed the provide_apb_info branch from 1ce8d65 to d560b2c Compare March 21, 2018 16:14

mhrivnak requested changes Mar 21, 2018

View reviewed changes

eriknelson suggested changes Mar 21, 2018

View reviewed changes

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 21, 2018

jkim added 4 commits March 22, 2018 09:36

make UserInfo available to APBs

6d9b151

requestingUser -> lastRequestingUser

5c0af89

override userInfo for Deprovision, Update, Unbind

update service_bundle.md

f464249

update

8d1f842

johnkim76 force-pushed the provide_apb_info branch from 92e3234 to 8d1f842 Compare March 22, 2018 13:37

shawn-hurley reviewed Mar 22, 2018

View reviewed changes

add that the user info may be empty

d2fde1a

eriknelson approved these changes Mar 22, 2018

View reviewed changes

update parameter description

07f818a

mhrivnak approved these changes Mar 22, 2018

View reviewed changes

shawn-hurley merged commit 41c78cb into openshift:master Mar 22, 2018

johnkim76 mentioned this pull request Mar 23, 2018

make serviceInstance details available to the playbook #674

Closed

make username available for APB in the serviceInstance #832

make username available for APB in the serviceInstance #832

johnkim76 commented Mar 12, 2018 •

edited

johnkim76 commented Mar 12, 2018

shawn-hurley commented Mar 12, 2018

johnkim76 commented Mar 12, 2018

jmrodri left a comment

jmrodri Mar 12, 2018

johnkim76 Mar 12, 2018

jmrodri Mar 12, 2018

johnkim76 Mar 12, 2018

jmrodri Mar 12, 2018

shawn-hurley Mar 13, 2018

jmrodri commented Mar 12, 2018

shawn-hurley Mar 13, 2018

shawn-hurley Mar 13, 2018

jmrodri Mar 14, 2018

shawn-hurley Mar 13, 2018

eriknelson left a comment

mhrivnak left a comment

shawn-hurley left a comment

shawn-hurley Mar 21, 2018

johnkim76 Mar 21, 2018

shawn-hurley Mar 21, 2018

mhrivnak Mar 21, 2018

mhrivnak Mar 21, 2018

eriknelson Mar 21, 2018

johnkim76 Mar 21, 2018

mhrivnak Mar 21, 2018

eriknelson Mar 21, 2018

johnkim76 Mar 21, 2018

shawn-hurley Mar 22, 2018 •

edited

johnkim76 Mar 22, 2018

eriknelson left a comment


		The requesting username of the [actions](#actions) _provision_, _deprovision_, _bind_, _unbind_, and _update_ will available in the `_apb_last_requesting_user` parameter. The parameter value will be the `UID` of the requesting user if the `username` is not available (e.g. auto escalation). Also, the user info of the latest request will always overwrite the existing value.

		This feature will be available in v3.10

make username available for APB in the serviceInstance #832

make username available for APB in the serviceInstance #832

Conversation

johnkim76 commented Mar 12, 2018 • edited

Describe what this PR does and why we need it:

Changes proposed in this pull request

Addresses the following Issue

johnkim76 commented Mar 12, 2018

shawn-hurley commented Mar 12, 2018

johnkim76 commented Mar 12, 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

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

jmrodri commented Mar 12, 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

eriknelson left a comment

Choose a reason for hiding this comment

mhrivnak left a comment

Choose a reason for hiding this comment

shawn-hurley 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

Choose a reason for hiding this comment

shawn-hurley Mar 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriknelson left a comment

Choose a reason for hiding this comment

johnkim76 commented Mar 12, 2018 •

edited

shawn-hurley Mar 22, 2018 •

edited