refactor: improve Provider logic (#99)

* refactor: improve Provider logic This refactor improves the Provider.Bind logic by being clearer on what parameters are used for finding the expected keys and by introducing new types wrapping map[string]interface{} for dealing with repetitive actions. Signed-off-by: Thulio Ferraz Assis <[email protected]> * fix: apply review comments 1 Signed-off-by: Thulio Ferraz Assis <[email protected]> * fix: license headers Signed-off-by: Thulio Ferraz Assis <[email protected]> * feat: add extra Object helper methods Signed-off-by: Thulio Ferraz Assis <[email protected]> * fix: use DigStringAlt for postgresql password Signed-off-by: Thulio Ferraz Assis <[email protected]> * fix: DigStringAlt comment Signed-off-by: Thulio Ferraz Assis <[email protected]> * fix: use if instead of switch Signed-off-by: Thulio Ferraz Assis <[email protected]> * fix: silly nesting Co-authored-by: Mark Yen <[email protected]> * fix: use ifs instead of switches on providers Signed-off-by: Thulio Ferraz Assis <[email protected]> * fix: handle empty keys and key parts on Dig Signed-off-by: Thulio Ferraz Assis <[email protected]> Co-authored-by: Mark Yen <[email protected]>
kubernetes-retired · Aug 17, 2020 · 18d78ee · 18d78ee
1 parent 3783f46
commit 18d78ee
Show file tree

Hide file tree

Showing 13 changed files with 684 additions and 326 deletions.
diff --git a/pkg/broker/broker.go b/pkg/broker/broker.go
@@ -77,8 +77,8 @@ func (d *ProvisioningSettings) ForService(service string) (*ServiceProvisioningS
 type MinibrokerClient interface {
 	Init(repoURL string) error
 	ListServices() ([]osb.Service, error)
-	Provision(instanceID, serviceID, planID, namespace string, acceptsIncomplete bool, provisionParams map[string]interface{}) (string, error)
-	Bind(instanceID, serviceID, bindingID string, acceptsIncomplete bool, bindParams map[string]interface{}) (string, error)
+	Provision(instanceID, serviceID, planID, namespace string, acceptsIncomplete bool, provisionParams *minibroker.ProvisionParams) (string, error)
+	Bind(instanceID, serviceID, bindingID string, acceptsIncomplete bool, bindParams *minibroker.BindParams) (string, error)
 	Unbind(instanceID, bindingID string) error
 	GetBinding(instanceID, bindingID string) (*osb.GetBindingResponse, error)
 	Deprovision(instanceID string, acceptsIncomplete bool) (string, error)
@@ -182,7 +182,14 @@ func (b *Broker) Provision(request *osb.ProvisionRequest, _ *broker.RequestConte
 		params = request.Parameters
 	}
 
-	operationName, err := b.client.Provision(request.InstanceID, request.ServiceID, request.PlanID, namespace, request.AcceptsIncomplete, params)
+	operationName, err := b.client.Provision(
+		request.InstanceID,
+		request.ServiceID,
+		request.PlanID,
+		namespace,
+		request.AcceptsIncomplete,
+		minibroker.NewProvisionParams(params),
+	)
 	if err != nil {
 		klog.V(4).Infof("broker: failed to provision request %q: %v", request.InstanceID, err)
 		return nil, err
@@ -247,7 +254,13 @@ func (b *Broker) Bind(request *osb.BindRequest, _ *broker.RequestContext) (*brok
 	b.Lock()
 	defer b.Unlock()
 
-	operationName, err := b.client.Bind(request.InstanceID, request.ServiceID, request.BindingID, request.AcceptsIncomplete, request.Parameters)
+	operationName, err := b.client.Bind(
+		request.InstanceID,
+		request.ServiceID,
+		request.BindingID,
+		request.AcceptsIncomplete,
+		minibroker.NewBindParams(request.Parameters),
+	)
 	if err != nil {
 		klog.V(4).Infof("broker: failed to bind %q: %v", request.InstanceID, err)
 		return nil, err

diff --git a/pkg/broker/broker_test.go b/pkg/broker/broker_test.go
@@ -27,6 +27,7 @@ import (
 
 	"github.com/kubernetes-sigs/minibroker/pkg/broker"
 	"github.com/kubernetes-sigs/minibroker/pkg/broker/mocks"
+	"github.com/kubernetes-sigs/minibroker/pkg/minibroker"
 )
 
 //go:generate mockgen -destination=./mocks/mock_broker.go -package=mocks github.com/kubernetes-sigs/minibroker/pkg/broker MinibrokerClient
@@ -80,12 +81,12 @@ var _ = Describe("Broker", func() {
 
 	Describe("Provision", func() {
 		var (
-			provisionParams = map[string]interface{}{
+			provisionParams = minibroker.NewProvisionParams(map[string]interface{}{
 				"key": "value",
-			}
+			})
 			provisionRequest = &osb.ProvisionRequest{
 				ServiceID:  "redis",
-				Parameters: provisionParams,
+				Parameters: provisionParams.Object,
 			}
 			requestContext = &osbbroker.RequestContext{}
 		)
@@ -113,7 +114,7 @@ var _ = Describe("Broker", func() {
 					provisionRequest.ServiceID = service
 					provisioningSettings, found := provisioningSettings.ForService(service)
 					Expect(found).To(BeTrue())
-					params := provisioningSettings.OverrideParams
+					params := minibroker.NewProvisionParams(provisioningSettings.OverrideParams)
 
 					mbclient.EXPECT().
 						Provision(gomock.Any(), gomock.Eq(service), gomock.Any(), gomock.Eq(namespace), gomock.Any(), gomock.Eq(params))

diff --git a/pkg/broker/mocks/mock_broker.go b/pkg/broker/mocks/mock_broker.go
diff --git a/pkg/minibroker/mariadb.go b/pkg/minibroker/mariadb.go
@@ -1,5 +1,5 @@
 /*
-Copyright 2019 The Kubernetes Authors.
+Copyright 2020 The Kubernetes Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -17,15 +17,26 @@ limitations under the License.
 package minibroker
 
 import (
+	"fmt"
+	"net/url"
+
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 )
 
-const mariadbProtocolName = "mysql"
+const (
+	mariadbProtocolName = "mysql"
+	rootMariadbUsername = "root"
+)
 
 type MariadbProvider struct{}
 
-func (p MariadbProvider) Bind(services []corev1.Service, params map[string]interface{}, chartSecrets map[string]interface{}) (*Credentials, error) {
+func (p MariadbProvider) Bind(
+	services []corev1.Service,
+	_ *BindParams,
+	provisionParams *ProvisionParams,
+	chartSecrets Object,
+) (Object, error) {
 	service := services[0]
 	if len(service.Spec.Ports) == 0 {
 		return nil, errors.Errorf("no ports found")
@@ -34,58 +45,40 @@ func (p MariadbProvider) Bind(services []corev1.Service, params map[string]inter
 
 	host := buildHostFromService(service)
 
-	dbParams, ok := params["db"].(map[string]interface{})
-	if !ok {
-		dbParams = make(map[string]interface{})
+	database, err := provisionParams.DigStringOr("db.name", "")
+	if err != nil {
+		return nil, fmt.Errorf("failed to get database name: %w", err)
 	}
-
-	database := ""
-	dbVal, ok := dbParams["name"]
-	if ok {
-		database, ok = dbVal.(string)
-		if !ok {
-			return nil, errors.Errorf("db.name not a string")
-		}
+	user, err := provisionParams.DigStringOr("db.user", rootMariadbUsername)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get username: %w", err)
 	}
 
-	var user, password string
-	userVal, ok := dbParams["user"]
-	if ok {
-		user, ok = userVal.(string)
-		if !ok {
-			return nil, errors.Errorf("db.user not a string")
-		}
-
-		passwordVal, ok := chartSecrets["mariadb-password"]
-		if !ok {
-			return nil, errors.Errorf("mariadb-password not found in secret keys")
-		}
-		password, ok = passwordVal.(string)
-		if !ok {
-			return nil, errors.Errorf("password not a string")
-		}
+	var passwordKey string
+	if user == rootMariadbUsername {
+		passwordKey = "mariadb-root-password"
 	} else {
-		user = "root"
-
-		rootPassword, ok := chartSecrets["mariadb-root-password"]
-		if !ok {
-			return nil, errors.Errorf("mariadb-root-password not found in secret keys")
-		}
-		password, ok = rootPassword.(string)
-		if !ok {
-			return nil, errors.Errorf("password not a string")
-		}
+		passwordKey = "mariadb-password"
+	}
+	password, err := chartSecrets.DigString(passwordKey)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get password: %w", err)
 	}
 
-	creds := Credentials{
-		Protocol: mariadbProtocolName,
-		Port:     svcPort.Port,
-		Host:     host,
-		Username: user,
-		Password: password,
-		Database: database,
+	creds := Object{
+		"protocol": mariadbProtocolName,
+		"port":     svcPort.Port,
+		"host":     host,
+		"username": user,
+		"password": password,
+		"database": database,
+		"uri": (&url.URL{
+			Scheme: mariadbProtocolName,
+			User:   url.UserPassword(user, password),
+			Host:   fmt.Sprintf("%s:%d", host, svcPort.Port),
+			Path:   database,
+		}).String(),
 	}
-	creds.URI = buildURI(creds)
 
-	return &creds, nil
+	return creds, nil
 }
diff --git a/pkg/minibroker/minibroker.go b/pkg/minibroker/minibroker.go
@@ -1,5 +1,5 @@
 /*
-Copyright 2019 The Kubernetes Authors.
+Copyright 2020 The Kubernetes Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -284,7 +284,7 @@ func (c *Client) ListServices() ([]osb.Service, error) {
 
 // Provision a new service instance.  Returns the async operation key (if
 // acceptsIncomplete is set).
-func (c *Client) Provision(instanceID, serviceID, planID, namespace string, acceptsIncomplete bool, provisionParams map[string]interface{}) (string, error) {
+func (c *Client) Provision(instanceID, serviceID, planID, namespace string, acceptsIncomplete bool, provisionParams *ProvisionParams) (string, error) {
 	klog.V(3).Infof("minibroker: provisioning intance %q, service %q, namespace %q, params %v", instanceID, serviceID, namespace, provisionParams)
 	ctx := context.TODO()
 
@@ -369,15 +369,15 @@ func (c *Client) Provision(instanceID, serviceID, planID, namespace string, acce
 }
 
 // provisionSynchronously will provision the service instance synchronously.
-func (c *Client) provisionSynchronously(instanceID, namespace, serviceID, planID, chartName, chartVersion string, provisionParams map[string]interface{}) error {
+func (c *Client) provisionSynchronously(instanceID, namespace, serviceID, planID, chartName, chartVersion string, provisionParams *ProvisionParams) error {
 	klog.V(3).Infof("minibroker: provisioning %s/%s using helm chart %s@%s", serviceID, planID, chartName, chartVersion)
 
 	chartDef, err := c.helm.GetChart(chartName, chartVersion)
 	if err != nil {
 		return err
 	}
 
-	release, err := c.helm.ChartClient().Install(chartDef, namespace, provisionParams)
+	release, err := c.helm.ChartClient().Install(chartDef, namespace, provisionParams.Object)
 	if err != nil {
 		return err
 	}
@@ -394,7 +394,7 @@ func (c *Client) provisionSynchronously(instanceID, namespace, serviceID, planID
 		return err
 	}
 	for _, service := range services.Items {
-		err := c.labelService(service, instanceID, provisionParams)
+		err := c.labelService(service, instanceID)
 		if err != nil {
 			return err
 		}
@@ -424,7 +424,7 @@ func (c *Client) provisionSynchronously(instanceID, namespace, serviceID, planID
 	return nil
 }
 
-func (c *Client) labelService(service corev1.Service, instanceID string, params map[string]interface{}) error {
+func (c *Client) labelService(service corev1.Service, instanceID string) error {
 	ctx := context.TODO()
 
 	labeledService := service.DeepCopy()
@@ -484,7 +484,7 @@ func (c *Client) labelSecret(secret corev1.Secret, instanceID string) error {
 
 // Bind the given service instance (of the given service) asynchronously; the
 // binding operation key is returned.
-func (c *Client) Bind(instanceID, serviceID, bindingID string, acceptsIncomplete bool, bindParams map[string]interface{}) (string, error) {
+func (c *Client) Bind(instanceID, serviceID, bindingID string, acceptsIncomplete bool, bindParams *BindParams) (string, error) {
 	klog.V(3).Infof("minibroker: binding instance %q, service %q, binding %q, binding params %v", instanceID, serviceID, bindingID, bindParams)
 	config, err := c.getConfigMap(instanceID)
 	if err != nil {
@@ -501,7 +501,7 @@ func (c *Client) Bind(instanceID, serviceID, bindingID string, acceptsIncomplete
 	rawProvisionParams := config.Data[ProvisionParamsKey]
 	operationName := generateOperationName(OperationPrefixBind)
 
-	var provisionParams map[string]interface{}
+	var provisionParams *ProvisionParams
 	err = json.Unmarshal([]byte(rawProvisionParams), &provisionParams)
 	if err != nil {
 		return "", errors.Wrapf(err, "could not unmarshall provision parameters for instance %q", instanceID)
@@ -510,14 +510,28 @@ func (c *Client) Bind(instanceID, serviceID, bindingID string, acceptsIncomplete
 	if acceptsIncomplete {
 		klog.V(3).Infof("minibroker: initializing asynchronous binding %q", bindingID)
 		go func() {
-			_ = c.bindSynchronously(instanceID, serviceID, bindingID, releaseNamespace, bindParams, provisionParams)
+			_ = c.bindSynchronously(
+				instanceID,
+				serviceID,
+				bindingID,
+				releaseNamespace,
+				bindParams,
+				provisionParams,
+			)
 			klog.V(3).Infof("minibroker: asynchronously bound instance %q, service %q, binding %q", instanceID, serviceID, bindingID)
 		}()
 		return operationName, nil
 	}
 
 	klog.V(3).Infof("minibroker: initializing synchronous binding %q", bindingID)
-	if err = c.bindSynchronously(instanceID, serviceID, bindingID, releaseNamespace, bindParams, provisionParams); err != nil {
+	if err := c.bindSynchronously(
+		instanceID,
+		serviceID,
+		bindingID,
+		releaseNamespace,
+		bindParams,
+		provisionParams,
+	); err != nil {
 		return "", err
 	}
 
@@ -529,20 +543,18 @@ func (c *Client) Bind(instanceID, serviceID, bindingID string, acceptsIncomplete
 // bindSynchronously creates a new binding for the given service instance.  All
 // results are only reported via the service instance configmap (under the
 // appropriate key for the binding) for lookup by LastBindingOperationState().
-func (c *Client) bindSynchronously(instanceID, serviceID, bindingID, releaseNamespace string, bindParams, provisionParams map[string]interface{}) error {
+func (c *Client) bindSynchronously(
+	instanceID,
+	serviceID,
+	bindingID,
+	releaseNamespace string,
+	bindParams *BindParams,
+	provisionParams *ProvisionParams,
+) error {
 	ctx := context.TODO()
 
 	// Wrap most of the code in an inner function to simplify error handling
 	err := func() error {
-		// Smoosh all the params together
-		params := make(map[string]interface{}, len(bindParams)+len(provisionParams))
-		for k, v := range provisionParams {
-			params[k] = v
-		}
-		for k, v := range bindParams {
-			params[k] = v
-		}
-
 		filterByInstance := metav1.ListOptions{
 			LabelSelector: labels.SelectorFromSet(map[string]string{
 				InstanceLabel: instanceID,
@@ -569,7 +581,7 @@ func (c *Client) bindSynchronously(instanceID, serviceID, bindingID, releaseName
 			return osb.HTTPStatusCodeError{StatusCode: http.StatusNotFound}
 		}
 
-		data := make(map[string]interface{})
+		data := make(Object)
 		for _, secret := range secrets.Items {
 			for key, value := range secret.Data {
 				data[key] = string(value)
@@ -579,19 +591,24 @@ func (c *Client) bindSynchronously(instanceID, serviceID, bindingID, releaseName
 		// Apply additional provisioning logic for Service Catalog Enabled services
 		provider, ok := c.providers[serviceID]
 		if ok {
-			creds, err := provider.Bind(services.Items, params, data)
+			creds, err := provider.Bind(
+				services.Items,
+				bindParams,
+				provisionParams,
+				data,
+			)
 			if err != nil {
 				return errors.Wrapf(err, "unable to bind instance %s", instanceID)
 			}
-			for k, v := range creds.ToMap() {
+			for k, v := range creds {
 				data[k] = v
 			}
 		}
 
 		// Record the result for later fetching
 		bindingResponse := osb.GetBindingResponse{
 			Credentials: data,
-			Parameters:  bindParams,
+			Parameters:  bindParams.Object,
 		}
 		bindingResponseJSON, err := json.Marshal(bindingResponse)
 		if err != nil {

diff --git a/pkg/minibroker/minibroker_test.go b/pkg/minibroker/minibroker_test.go
@@ -1,5 +1,5 @@
 /*
-Copyright 2019 The Kubernetes Authors.
+Copyright 2020 The Kubernetes Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.