Save extracted credentials into a secret #775

shawn-hurley · 2018-02-19T20:26:56Z

Describe what this PR does and why we need it:
This PR will implement #768
Changes proposed in this pull request

~~Add kubernetes client methods to interact with extracted credentials in the namespace.~~
~~Add runtime methods for interacting with extracted credentials. These methods should be overridable.~~
~~Remove all dao implementation and interface methods regarding extracted credentials.~~
~~Remove all instances of interacting with dao extracted credentials in the broker package. Add back call to APB package to get extracted credentials when needed.~~
~~Add exposed method on APB that will retrieve the extracted credentials.~~
~~Update APB package to create/save/delete extracted credentials for the correct actions. this should call the correct runtime package methods.~~

rthallisey

minor suggestion

rthallisey · 2018-02-20T17:06:26Z

pkg/apb/ext_creds.go

+	creds, err := runtime.Provider.GetExtractedCredential(id, clusterConfig.Namespace)
+	if err != nil {
+		switch {
+		case err == runtime.ErrCredentialsNotFound:


runtime.ErrCredentialsNotFound is extracted credentials were not found
clients.ErrCredentialsNotFound is credentials were not found
apb.ErrExtractedCredentialsNotFound is credentials not found

Maybe instead of using different vars for a similar message, use one variable at the bottom of the call, clients.ErrCredentialsNotFound, and bubble that up to the top level and at each instance that sees the error identify where the error was seen.

// /pkg/apb.go switch { case err == runtime.ErrCredentialsNotFound: log.Debugf("APB error: extracted credential secret not found - %v", id) return nil, ErrExtractedCredentialsNotFound

So here is my thinking on why each package has its own NotFound Error.

I don't want the error to bleed across package boundaries. Right now runtime package is only called by apb. I think we should keep it that way. The same is not currently true of the client, but eventually, we need to encapsulate the client calls in the runtime package.

I think this allows a user who wants to override the runtime package calls for extracted credentials can send back the runtime error and now our APB package can use that. Does that make sense what I was trying to do?

Would love to hear thoughts on this approach.

I understand your point about which pkg calling another pkg. That part is fine with me. It just seems silly we have three different vars in three different pkgs saying almost exactly the same thing. A follow up to this could be adding an error pkg with all the error constants.

rthallisey · 2018-02-20T18:42:37Z

pkg/runtime/runtime.go

@@ -94,7 +96,14 @@ func NewRuntime() {
 		panic(err.Error())
 	}

-	Provider = &provider{coe: cluster}
+	var c ExtractedCredential
+	if extCreds == nil {


Can you explain why we should have this if/else?

if nil we will use the default extracted credentials. This should be in the godoc for the function and will add now.

I think I understand more what this is now, thanks for the comment.

There's one more thing I want to clear up. How I would create a new ExtractedCredential object without having some understanding of my runtime from the pkg/app/app.go pkg?

I want to clarify my last comment. The runtime interface is structured so that if you call NewRuntime the Runtime pkg will identify what provider you are using. If we add ExtractedCredential type to the NewRuntime call, pkg/app/app.go needs to know what runtime provider is being used in order to provide a working ExtractedCredential type. The reason for that is because ExtractedCredential is stored in a cluster specific resource, a secret. My point is I don't think we want to pass ExtractedCredential as a param to NewRuntime, rather we should add it behind the runtime interface it be initialized after the NewRuntime call.

pkg/app/app.go needs to know what runtime provider is being used in order to provide a working ExtractedCredential type. The reason for that is because ExtractedCredential is stored in a cluster-specific resource, a secret

This would certainly be a problem if we had more then k8s as a provider. Currently, we don't and even the CreateSandbox... functions relay on core k8s clients and not the provider. I don't think this is a problem currently. We are tracking a better way to override and change the runtime, which I agree we should do. But this needs to be overridable by anyone who is vendoring the project and wants to change this behavior.

Updating this to show what I mean by just using core k8s client is backed into every provider regardless of the cluster specific type.

https://github.com/openshift/ansible-service-broker/blob/master/pkg/runtime/runtime.go#L150

The sandbox will only ever use k8s client, because we only support k8s and openshift. Because of this I think it is currently a valid assumption that anything that can be done with just k8s will be supported in both the clusters that we currently support. The default extracted credentials does that.

eriknelson · 2018-02-21T02:18:02Z

Caught up with @shawn-hurley to coordinate this PR and #773 since they're in direct conflict. Plan is to bring in #773 first and then rework this PR on top of it. Coming back to review this afterwards.

rthallisey

Spoke with Shawn, in order to get the behavior we want here we're going to need to rework the runtime pkg. There's an issue to track this. In the meantime, we'll add a param to NewRuntime to allow this override.

jmrodri · 2018-02-26T17:08:54Z

docs/proposals/extracted_credentials_saved_as_secrets.md

@@ -20,7 +18,7 @@ metadata:
    <labels provided>
 ```

-[Gob encoding](https://godoc.org/encoding/gob) will allow us to save arbitrary data in the secret for a key. The secrets keys will look rational to a user who looks at the created secret. This user would need permissions to see the secret, but if someone is looking at the secret making it obvious what data is in there will be helpful. 
+To encode the credentials in a generic way, we must marshal a `map[string]interface{}` so that we can retrieve the data. This is not how I wanted the secret to look, but will allow anyone to use the credentials and will allow us to retrieve the credentials without writing our own gob parser.


jmrodri · 2018-02-26T17:10:15Z

pkg/apb/bind.go

@@ -84,6 +84,13 @@ func (e *executor) Bind(
 			return
 		}

+		labels := map[string]string{"apbAction": "bind", "apbName": instance.Spec.FQName}
+		err = runtime.Provider.CreateExtractedCredential(bindingID, clusterConfig.Namespace, creds.Credentials, labels)


bind.go change looks good.

jmrodri · 2018-02-26T17:12:26Z

pkg/apb/deprovision.go

+		if err != nil {
+			log.Errorf("unable to delete the extracted credentials - %v", err)
+			e.actionFinishedWithError(err)
+			return


Wasn't there some issue that @eriknelson was recently looking at about deleting extracted credentials that might not exist? Will this re-introduce that problem?

Yeah it will, think this is where it might make sense for us to have our own error type, and skip the action finished call. Log with INFO level.

jmrodri · 2018-02-26T17:12:48Z

pkg/apb/executor.go

-	Bind(instance *ServiceInstance, parameters *Parameters) <-chan StatusMessage
-	Unbind(instance *ServiceInstance, parameters *Parameters) <-chan StatusMessage
+	Bind(instance *ServiceInstance, parameters *Parameters, bindingID string) <-chan StatusMessage
+	Unbind(instance *ServiceInstance, parameters *Parameters, bindingID string) <-chan StatusMessage


okay, acceptable to add bindingID

jmrodri · 2018-02-26T17:15:00Z

pkg/apb/ext_creds.go

+// Please use this method with caution.
+func SetExtractedCredentials(id string, creds *ExtractedCredentials) error {
+	return runtime.Provider.CreateExtractedCredential(id, clusterConfig.Namespace, creds.Credentials, nil)
+}


👍 ext_creds.go looks good.

jmrodri · 2018-02-26T17:17:22Z

pkg/app/app.go

@@ -257,7 +257,7 @@ func CreateApp() App {
 	// Intiialize the cluster config.
 	apb.InitializeClusterConfig(app.config.GetSubConfig("openshift"))
 	if app.broker, err = broker.NewAnsibleBroker(
-		app.dao, app.registry, *app.engine, app.config.GetSubConfig("broker"),
+		app.dao, app.registry, *app.engine, app.config.GetSubConfig("broker"), app.config.GetString("openshift.namespace"),


👍 app.go

jmrodri · 2018-02-26T17:17:48Z

pkg/broker/broker.go

 }

 // NewAnsibleBroker - Creates a new ansible broker
 func NewAnsibleBroker(dao dao.Dao,
 	registry []registries.Registry,
 	engine WorkEngine,
-	brokerConfig *config.Config) (*AnsibleBroker, error) {
+	brokerConfig *config.Config,
+	namespace string) (*AnsibleBroker, error) {


that's very erik of you 😆

jmrodri · 2018-02-26T17:18:23Z

pkg/broker/broker.go

-	bindExtCreds, err := a.dao.GetExtractedCredentials(bindInstance.ID.String())
-	if err != nil && !client.IsKeyNotFound(err) {
+	bindExtCreds, err := apb.GetExtractedCredentials(bindInstance.ID.String())
+	if err != nil && err != apb.ErrExtractedCredentialsNotFound {


👍 broker.go looks good

jmrodri · 2018-02-26T17:19:58Z

pkg/clients/kubernetes.go

+		return err
+	}
+	return nil
+}


👍 client/kubernetes.go looks good

eriknelson

Just the item that @jmrodri mentioned in deprovision and unbind. Otherwise, looking good.

eriknelson · 2018-02-27T15:14:46Z

pkg/apb/ext_creds.go

@@ -41,6 +42,11 @@ const (
 	GatherCredentialsCommand = "broker-bind-creds"
 )

+var (
+	// ErrExtractedCredentialsNotFound - Extracted Credentials are not found.
+	ErrExtractedCredentialsNotFound = fmt.Errorf("credentials not found")


eriknelson · 2018-02-27T15:20:05Z

pkg/apb/unbind.go

@@ -70,6 +70,13 @@ func (e *executor) Unbind(
 			e.actionFinishedWithError(err)
 			return
 		}
+		// Delete the binding extracted credential here.
+		err = runtime.Provider.DeleteExtractedCredential(bindingID, clusterConfig.Namespace)
+		if err != nil {


I think we might want to just log this at an info level if it's a miss error instead of finishing with error here too.

eriknelson

I'm seeing:

[2018-02-27T19:10:33.086Z] [DEBUG] - executor::actionFinishedWithSuccess
[2018-02-27T19:10:33.086Z] [WARNING] - executor::actionFinishedWithSuccess was called, but the statusChan was already closed!

During what should be a failed provision.

Test:

Create new project and provision mediawiki in that project with admin/admin as the password. This is expected to hard fail because it does not accept the same vals.

We shouldn't be seeing success called in this case.

shawn-hurley · 2018-02-28T14:13:55Z

@eriknelson I have updated and tested based on our quick conversation.

eriknelson · 2018-02-28T14:42:59Z

@shawn-hurley cool, will test again this morning.

* initial commit * Fixing marshal and unmarshal of data. * fixing binding * adding documentation for creating a new runtime. * fixing provision issue when no extracted credentials occurs. * fix for unbind.go * move action hooks to base methods for provision and update.

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 19, 2018

rthallisey suggested changes Feb 20, 2018

View reviewed changes

eriknelson added the 3.10 | release-1.2 Kubernetes 1.10 | Openshift 3.10 | Broker release-1.2 label Feb 21, 2018

shawn-hurley force-pushed the save-extcreds-secret branch from d2ab460 to 3785197 Compare February 21, 2018 16:32

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2018

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 21, 2018

shawn-hurley force-pushed the save-extcreds-secret branch from 3785197 to dd46b5b Compare February 21, 2018 16:54

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2018

shawn-hurley mentioned this pull request Feb 21, 2018

APB Package should have a sub package for Errors #782

Closed

shawn-hurley added the needs-review label Feb 22, 2018

rthallisey approved these changes Feb 23, 2018

View reviewed changes

shawn-hurley mentioned this pull request Feb 26, 2018

Broker client dao #795

Merged

jmrodri approved these changes Feb 27, 2018

View reviewed changes

eriknelson suggested changes Feb 27, 2018

View reviewed changes

shawn-hurley added 6 commits February 27, 2018 12:20

initial commit

f7f2f7e

Fixing marshal and unmarshal of data.

a34a1be

fixing binding

c908141

adding documentation for creating a new runtime.

4a8d9e1

fixing provision issue when no extracted credentials occurs.

89b10cb

fix for unbind.go

144133f

shawn-hurley force-pushed the save-extcreds-secret branch from 96d4950 to 144133f Compare February 27, 2018 17:20

eriknelson suggested changes Feb 27, 2018

View reviewed changes

move action hooks to base methods for provision and update.

7568858

eriknelson approved these changes Feb 28, 2018

View reviewed changes

eriknelson merged commit ff1f14a into openshift:master Feb 28, 2018

eriknelson deleted the save-extcreds-secret branch February 28, 2018 16:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save extracted credentials into a secret #775

Save extracted credentials into a secret #775

shawn-hurley commented Feb 19, 2018 •

edited

Loading

rthallisey left a comment

rthallisey Feb 20, 2018

shawn-hurley Feb 20, 2018

rthallisey Feb 20, 2018

rthallisey Feb 20, 2018

shawn-hurley Feb 20, 2018

rthallisey Feb 20, 2018

rthallisey Feb 22, 2018

shawn-hurley Feb 23, 2018 •

edited

Loading

eriknelson commented Feb 21, 2018 •

edited

Loading

rthallisey left a comment

jmrodri Feb 26, 2018

jmrodri Feb 26, 2018

jmrodri Feb 26, 2018

eriknelson Feb 27, 2018

jmrodri Feb 26, 2018

jmrodri Feb 26, 2018

jmrodri Feb 26, 2018

jmrodri Feb 26, 2018

jmrodri Feb 26, 2018

jmrodri Feb 26, 2018

eriknelson left a comment

eriknelson Feb 27, 2018

eriknelson Feb 27, 2018

eriknelson left a comment

shawn-hurley commented Feb 28, 2018

eriknelson commented Feb 28, 2018

Save extracted credentials into a secret #775

Save extracted credentials into a secret #775

Conversation

shawn-hurley commented Feb 19, 2018 • edited Loading

rthallisey 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

shawn-hurley Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

eriknelson commented Feb 21, 2018 • edited Loading

rthallisey 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

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

Choose a reason for hiding this comment

shawn-hurley commented Feb 28, 2018

eriknelson commented Feb 28, 2018

shawn-hurley commented Feb 19, 2018 •

edited

Loading

shawn-hurley Feb 23, 2018 •

edited

Loading

eriknelson commented Feb 21, 2018 •

edited

Loading