Adding ability to pass credentials to bind and unbind actions. #302

shawn-hurley · 2017-07-25T01:58:44Z

Implementation for #293. Just getting thoughts down. This allows you test with the the test apps and APB
https://github.com/shawn-hurley/rhscl-postgresql-apb
https://github.com/shawn-hurley/bottle-todo-demo

shawn-hurley · 2017-07-28T14:11:39Z

TESTING New Workflow

Needs to be run on a machine that spins up containers quickly

Use:
APB - https://github.com/shawn-hurley/rhscl-postgresql-apb
Application - https://github.com/shawn-hurley/bottle-todo-demo use S2I to build to pod.

The test APB will give you the ability to select which workflow you would like the APB to do.

The options are multiple or single. multiple will tell the APB to generate a new user for each bind. The credentials created at provision time should be for the postgres user. The single option will tell the APB to generate a new user, and then use that user for subsequent bind requests.

Test 1

Launch catasb point to broker built with the changes. Leave launch_apb_on_bind to false.

Use the media-wiki and RHSCL postgres images to test that the original workflow still works.

deploy postgresql apb and media-wiki apb
should be able to bind mediawiki-apb and postgres-apb
unbind should delete the secret and remove the pod preset

Test 2

Launch catasb point to broker built with the changes. set launch_apb_on_bind to true.

Use test postgres-apb and test application

deploy S2I python application and test APB Postgresql. Select the multiple user option
application should display no database connection.
Create a binding between application and PostgreSQL.
APB should launch.
New generated user and password should be created in etcd /extracted_credentials directory with key of binding id.
the secret should be created with new user and password.
pod preset should be created with new user and password.
redeploy application and reload application page. Then navigate to /todo
Should see data, can update tasks. You can create new todo's by navigating to /new and creating one
Choose to delete the binding
Should see APB launch and should see the user removed from the database.
Extracted Credentials should be deleted

Test 3

Launch catasb point to broker built with the changes. set launch_apb_on_bind to true.

Use test postgres-apb and test application

deploy S2I python application and test APB Postgresql. Select the single user option
application should display no database connection.
Create a binding between application and postgresql.
APB should launch.
same user and password should be created in etcd /extracted_credentials directory with key of binding id.
secret should be created with new user and password.
pod preset should be created with new user and password.
redeploy application and reload application page. Then navigate to /todo
Should see data, can update tasks. You can create new todo's by navigating to /new and creating one
Choose to delete the binding
Should see APB launch and should see the user removed from the database.
Extracted Credentials should be deleted

PLEASE NOTE: that if it takes too long for the APB to run the service catalog will fail to get the data and will not create the secret. This is due to lack of async binding support.

shawn-hurley · 2017-08-01T15:31:12Z

Update Bind Operation in postgres APB so that 'bind' will create credentials and unbind will revoke

shawn-hurley · 2017-08-01T15:31:14Z

Update Bind Operation in postgres APB so that 'bind' will create credentials and unbind will revoke

rthallisey

Just a simple nit

rthallisey · 2017-08-01T19:04:31Z

pkg/broker/broker.go

@@ -678,6 +677,9 @@ func (a AnsibleBroker) Bind(instanceUUID uuid.UUID, bindingUUID uuid.UUID, req *
 	params[planParameterKey] = req.PlanID

 	// Create a BindingInstance with a reference to the serviceinstance.
+	//
+	// Create a BindingInstance with a reference to the serviceinstance.


Looks like an extra comment

rthallisey · 2017-08-02T12:21:49Z

LGTM. I haven't tested yet.

jmrodri

basically some formatting changes.

jmrodri · 2017-08-03T03:18:39Z

docs/config.md

@@ -100,6 +100,8 @@ The broker config section will tell the broker what functionality should be enab
 and disabled. It will also tell the broker where to find files on disk that will
 enable the full functionality.

+*Note: with the absence of async bind, setting launch_apb_on_bind to true can cause the bind action to timeout and will span a retry. The broker will handle with with 409 Conflicts because it is the same bind request with different parameters.*


jmrodri · 2017-08-03T03:21:42Z

pkg/apb/unbind.go

 	log.Notice("============================================================")
 	log.Notice("                       UNBINDING                              ")
 	log.Notice("============================================================")

 	// podName, err
 	_, err := ExecuteApb(
 		"unbind", clusterConfig, instance.Spec,
-		instance.Context, instance.Parameters, log,
+		instance.Context, parameters, log,
 	)


jmrodri · 2017-08-03T03:23:27Z

pkg/apb/unbind.go

@@ -10,15 +10,15 @@ import (
 // github.com/op/go-logging, which is used all over the broker
 // Maybe apb defines its own interface and accepts that optionally
 // Little looser, but still not great
-func Unbind(instance *ServiceInstance, clusterConfig ClusterConfig, log *logging.Logger) error {
+func Unbind(instance *ServiceInstance, clusterConfig ClusterConfig, log *logging.Logger, parameters *Parameters) error {


It would be nice to use the same order as Bind:

func Bind( instance *ServiceInstance, parameters *Parameters, clusterConfig ClusterConfig, log *logging.Logger, ) (string, *ExtractedCredentials, error) {

jmrodri · 2017-08-03T03:26:58Z

pkg/broker/broker.go

@@ -751,31 +734,28 @@ func (a AnsibleBroker) Bind(instanceUUID uuid.UUID, bindingUUID uuid.UUID, req *
 			return nil, err
 		}
 	} else {
-		a.log.Warning("Broker configured to *NOT* launch and run APB bind")
+		a.log.Warningf("Broker configured to *NOT* launch and run APB bind")


I like using the f version, but do we need to change it if we're not passing in variables. I think it's ok, but I wouldn't change them unless we're passing in a variable.

jmrodri · 2017-08-03T03:27:33Z

pkg/broker/broker.go

-		a.log.Error("No extracted credentials found from provision or bind")
-		a.log.Error("Instance ID: %s", instanceUUID.String())
+		a.log.Errorf("No extracted credentials found from provision or bind instance ID: %s",
+			instanceUUID.String())


This is a prime example of requiring the change as we are passing the variable in.

jmrodri · 2017-08-03T03:35:05Z

pkg/broker/broker.go

-		return nil, err
+	if bindExtCreds != nil {
+		err = a.dao.DeleteExtractedCredentials(bindingUUID.String())
+		if err != nil {


is it possible we could leak bindings?

I see 4 cases on 777 we will attempt to get the credentials.

etcd/dao error that is something other than creds could not be found

we return the error and error out to service catalog which will re-try.

etcd/dao error that is creds not found

we don't need to attempt to delete anything that is not there

etcd/dao finds creds and delete succeeds

happy path

etcd/dao finds creds and delete creds fails.

we will error out which will bubble up to service catalog which will re-try.

Can you think of another path?

I think we're covered by service cat retries.

jmrodri · 2017-08-03T03:36:29Z

pkg/handler/handler.go

 	resp, err := h.broker.Unbind(instanceUUID, bindingUUID, planID)
-
 	if errors.IsNotFound(err) {
 		writeResponse(w, http.StatusGone, resp)


any reason we deleted the blank lines? they were single lines which isn't egregious and I think it makes it more readable.

No good reason, I will add it back

jmrodri · 2017-08-03T03:39:15Z

scripts/prep_local_devel_env.sh

@@ -113,7 +113,7 @@ echo -e "Wrote \n${BROKER_SVC_ACCT_TOKEN}\n to: ${SVC_ACCT_TOKEN_FILE}\n"
 ###
 # Fetch the tls.crt for the asb deployment
 ###
-TLS_CRT_DATA=`oc get secret asb-tls -o jsonpath='{ .data.tls\.crt }'`
+TLS_CRT_DATA=`oc get secret asb-tls -o jsonpath='{ .data.tls\.crt}'`


What this intentional? we should be consistent and we currently aren't. I would not make this change as part of this PR. Here are other ones in the same file:

[jesusr@speed3 ansible-service-broker{authimpl}]$ grep jsonpath scripts/prep_local_devel_env.sh BROKER_SVC_ACCT_SECRET_NAME=`oc get serviceaccount asb -n ansible-service-broker -o jsonpath='{.secrets[0].name}'` SVC_ACCT_CA_CRT_DATA=`oc get secret ${BROKER_SVC_ACCT_SECRET_NAME} -n ${ASB_PROJECT} -o jsonpath='{ .data.service-ca\.crt }'` BROKER_SVC_ACCT_TOKEN=`oc get secret ${BROKER_SVC_ACCT_SECRET_NAME} -n ${ASB_PROJECT} -o jsonpath='{ .data.token }'` TLS_CRT_DATA=`oc get secret asb-tls -o jsonpath='{ .data.tls\.crt }'` TLS_KEY_DATA=`oc get secret asb-tls -o jsonpath='{ .data.tls\.key }'` etcd_route=`oc get route asb-etcd -n ${ASB_PROJECT} -o=jsonpath=\'\{.spec.host\}\'`

I had to fix this and they you submitted the patch, I must have chosen the wrong one on the merge. I will add the space back my fault.

jmrodri · 2017-08-03T03:39:40Z

pkg/broker/broker.go

 	}

 	err = a.dao.DeleteBindInstance(bindingUUID.String())
 	if err != nil {
 		return nil, err
 	}
-


I would have left this blank line as well.

jmrodri · 2017-08-03T03:41:23Z

pkg/broker/broker.go

+	params[provisionCredentialsKey] = provExtCreds.Credentials
+	if bindExtCreds != nil {
+		params[bindCredentialsKey] = bindExtCreds.Credentials
+	}


overall this looks good.

eriknelson

Questions around APBs that return only provision credentials, or only bind credentials. Unbind is written expecting both to be present
Return err if serviceInstance is not found on unbind
Waning on no launch APB in unbind
Spoke with @shawn-hurley in IRC, thought it was a little weird to key into /extracted_credentials sourcing from two different places (instance_ids and binding_ids). Would be useful to know where they came from, he came up with an additional namespace: /extracted_credentials/prov & /extracted_credentials/bind

Overall I like this a lot. Solves a problem we've been wondering about in the past: how should the broker deal with two different sets of binding credentials? Answer: it shouldn't. Delegate that decision to the APBs and remain dumb.

eriknelson · 2017-08-04T15:24:30Z

docs/config.md

@@ -100,6 +100,8 @@ The broker config section will tell the broker what functionality should be enab
 and disabled. It will also tell the broker where to find files on disk that will
 enable the full functionality.

+*Note: with the absence of async bind, setting launch_apb_on_bind to true can cause the bind action to timeout and will span a retry. The broker will handle with with 409 Conflicts because it is the same bind request with different parameters.*


eriknelson · 2017-08-04T15:40:20Z

pkg/broker/broker.go

+			a.log.Errorf("Could not persist extracted credentials - %v", err)
+			return nil, err
+		}
+		return &BindResponse{Credentials: bindExtCreds.Credentials}, nil


This is better than our original thinking. Keeps the broker dumb and delegates the responsibility for what needs to be in the bind response entirely to the APBs, while still allowing them to include provision credentials since they're passed in as a param during the bind action. Nice job 👍

eriknelson · 2017-08-04T15:43:54Z

pkg/broker/broker.go

@@ -789,14 +769,41 @@ func (a AnsibleBroker) Unbind(
 		return nil, errors.New(errMsg)
 	}

+	params := make(apb.Parameters)
+	provExtCreds, err := a.dao.GetExtractedCredentials(instanceUUID.String())


This reads like any APB that has been bound is required to have credentials stashed for both provision and bind. What if I have a bindable APB that only returns credentials during a bind operation? This will error out unless the provision object is some kind of empty object, which I also don't think we should be doing.

eriknelson · 2017-08-04T15:48:23Z

pkg/broker/broker.go

+	params[provisionCredentialsKey] = provExtCreds.Credentials
+	if bindExtCreds != nil {
+		params[bindCredentialsKey] = bindExtCreds.Credentials
+	}
 	serviceInstance, err := a.getServiceInstance(instanceUUID)
 	if err != nil {
 		a.log.Debugf("Service instance with id %s does not exist", instanceUUID.String())


Just noticing this; if this branch is taken it's a hard error and we need to return w/ error code. We'll potentially hit an NPE continuing with a nil serviceInstance.

This would also be quite bad; if someone is trying to unbind from a service that we don't know exists, we're in a real corrupt state.

eriknelson · 2017-08-04T15:49:28Z

pkg/broker/broker.go

+		if err != nil {
+			return nil, err
+		}
+	}


I would add a warning to indicate the APB was not run in the logs, making sure we are clear that nothing was launched in case somebody expects otherwise.

eriknelson · 2017-08-04T15:50:42Z

pkg/broker/broker.go

-		return nil, err
+	if bindExtCreds != nil {
+		err = a.dao.DeleteExtractedCredentials(bindingUUID.String())
+		if err != nil {


I think we're covered by service cat retries.

but should still handle bind and unbind.

…hift#302) * adding ability to pass credentials to bind and unbind actions. * fixing unbind. and updating name of params to spec compliant * update style, making note of issues with lack of async support * config fix up * removing duplicate comment. * addressing comments * addressing issues where prov credentials not sent. but should still handle bind and unbind. * adding warning for not finding any credentials at unbind

shawn-hurley force-pushed the bind-wkflw branch 2 times, most recently from 0a1443c to 4e61fc8 Compare July 28, 2017 12:59

shawn-hurley changed the title ~~[WIP] Adding ability to pass credentials to bind and unbind actions.~~ Adding ability to pass credentials to bind and unbind actions. Jul 28, 2017

shawn-hurley and others added 3 commits August 1, 2017 09:03

adding ability to pass credentials to bind and unbind actions.

a2e0354

fixing unbind. and updating name of params to spec compliant

d1b3231

update style, making note of issues with lack of async support

1a08f5d

shawn-hurley force-pushed the bind-wkflw branch from 4e61fc8 to d5eed8c Compare August 1, 2017 13:08

config fix up

9578928

shawn-hurley force-pushed the bind-wkflw branch from d5eed8c to 9578928 Compare August 1, 2017 15:34

shawn-hurley requested review from jmrodri, eriknelson and rthallisey August 1, 2017 18:15

rthallisey suggested changes Aug 1, 2017

View reviewed changes

removing duplicate comment.

63fb106

rthallisey approved these changes Aug 2, 2017

View reviewed changes

jmrodri suggested changes Aug 3, 2017

View reviewed changes

addressing comments

baccbe3

eriknelson suggested changes Aug 4, 2017

View reviewed changes

addressing issues where prov credentials not sent.

5d92733

but should still handle bind and unbind.

eriknelson approved these changes Aug 4, 2017

View reviewed changes

jmrodri approved these changes Aug 4, 2017

View reviewed changes

adding warning for not finding any credentials at unbind

8a2fb67

eriknelson merged commit f569628 into openshift:master Aug 4, 2017

Adding ability to pass credentials to bind and unbind actions. #302

Adding ability to pass credentials to bind and unbind actions. #302

shawn-hurley commented Jul 25, 2017

shawn-hurley commented Jul 28, 2017 •

edited

shawn-hurley commented Aug 1, 2017

shawn-hurley commented Aug 1, 2017

rthallisey left a comment

rthallisey Aug 1, 2017

rthallisey commented Aug 2, 2017

jmrodri left a comment

jmrodri Aug 3, 2017

eriknelson Aug 4, 2017

jmrodri Aug 3, 2017

jmrodri Aug 3, 2017

jmrodri Aug 3, 2017

jmrodri Aug 3, 2017

jmrodri Aug 3, 2017

shawn-hurley Aug 3, 2017

eriknelson Aug 4, 2017

jmrodri Aug 3, 2017

shawn-hurley Aug 3, 2017

jmrodri Aug 3, 2017

shawn-hurley Aug 3, 2017

jmrodri Aug 3, 2017

jmrodri Aug 3, 2017

eriknelson left a comment •

edited

eriknelson Aug 4, 2017

eriknelson Aug 4, 2017

eriknelson Aug 4, 2017

eriknelson Aug 4, 2017

eriknelson Aug 4, 2017

eriknelson Aug 4, 2017

Adding ability to pass credentials to bind and unbind actions. #302

Adding ability to pass credentials to bind and unbind actions. #302

Conversation

shawn-hurley commented Jul 25, 2017

shawn-hurley commented Jul 28, 2017 • edited

TESTING New Workflow

Test 1

Test 2

Test 3

shawn-hurley commented Aug 1, 2017

shawn-hurley commented Aug 1, 2017

rthallisey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rthallisey commented Aug 2, 2017

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

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

eriknelson left a comment • 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

shawn-hurley commented Jul 28, 2017 •

edited

eriknelson left a comment •

edited