Merge pull request #82 from vshn/fix/mariadb-unbind-race-condition

Fix race condition in MariaDB unbinding

Makefile.vars.mk

@@ -35,6 +35,8 @@ CROSSPLANE_CRDS = $(addprefix $(TESTDATA_CRD_DIR)/, apiextensions.crossplane.io_
 					pkg.crossplane.io_providerrevisions.yaml \
 					pkg.crossplane.io_providers.yaml)
 
+PROVIDERSQL_VERSION = v0.2.1
+
 # Image URL to use all building/pushing image targets
 DOCKER_IMG ?= docker.io/vshn/crossplane-service-broker:$(IMG_TAG)
 QUAY_IMG ?= quay.io/vshn/crossplane-service-broker:$(IMG_TAG)

pkg/api/auth/basic.go

@@ -13,11 +13,11 @@ import (
 // UserPropertyName allows to query the HTTP context for the current user's name.
 // See Context() on http.Request.
 //
-//   func(w http.ResponseWriter, r *http.Request) {
-//     user := r.Context().Value(auth.UserPropertyName);
-//     fmt.Fprintf(w, "This is an authenticated request")
-//     fmt.Fprintf(w, "User name: '%s'\n", user)
-//   }
+//	func(w http.ResponseWriter, r *http.Request) {
+//	  user := r.Context().Value(auth.UserPropertyName);
+//	  fmt.Fprintf(w, "This is an authenticated request")
+//	  fmt.Fprintf(w, "User name: '%s'\n", user)
+//	}
 const UserPropertyName contextKey = "user"
 
 // Basic represents a mux middleware that, given a http.Request, checks whether the Authorization header
@@ -26,7 +26,7 @@ type Basic struct {
 	Credentials []Credential
 }
 
-// Credential represents one user's ``username and password'' combination.
+// Credential represents one user's “username and password” combination.
 type Credential struct {
 	username []byte
 	password []byte

pkg/api/auth/bearer_token.go

@@ -15,14 +15,14 @@ type BearerToken struct {
 // TokenPropertyName allows to query the HTTP context for the current user's JWT token.
 // See Context() on http.Request.
 //
-//   func(w http.ResponseWriter, r *http.Request) {
-//     claims := req.Context().Value(auth.TokenPropertyName).(*jwt.Claims)
-//     if n, ok := claims.Number("deadline"); !ok {
-//       fmt.Fprintln(w, "no deadline")
-//     } else {
-//       fmt.Fprintln(w, "deadline at", (*jwt.NumericTime)(&n))
-//     }
-//   }
+//	func(w http.ResponseWriter, r *http.Request) {
+//	  claims := req.Context().Value(auth.TokenPropertyName).(*jwt.Claims)
+//	  if n, ok := claims.Number("deadline"); !ok {
+//	    fmt.Fprintln(w, "no deadline")
+//	  } else {
+//	    fmt.Fprintln(w, "deadline at", (*jwt.NumericTime)(&n))
+//	  }
+//	}
 const TokenPropertyName = "bearer-token"
 
 // Handler represents a mux.MiddlewareFunc

pkg/brokerapi/brokerapi.go

@@ -30,7 +30,8 @@ func New(cp *crossplane.Crossplane, logger lager.Logger, pc crossplane.PlanUpdat
 }
 
 // Services gets the catalog of services offered by the service broker
-//   GET /v2/catalog
+//
+//	GET /v2/catalog
 func (b BrokerAPI) Services(ctx context.Context) ([]domain.Service, error) {
 	rctx := reqcontext.NewReqContext(ctx, b.logger, nil)
 	rctx.Logger.Info("get-catalog")
@@ -40,7 +41,8 @@ func (b BrokerAPI) Services(ctx context.Context) ([]domain.Service, error) {
 }
 
 // Provision creates a new service instance
-//   PUT /v2/service_instances/{instance_id}
+//
+//	PUT /v2/service_instances/{instance_id}
 func (b BrokerAPI) Provision(ctx context.Context, instanceID string, details domain.ProvisionDetails, asyncAllowed bool) (domain.ProvisionedServiceSpec, error) {
 	rctx := reqcontext.NewReqContext(ctx, b.logger, lager.Data{
 		"instance-id": instanceID,
@@ -58,7 +60,8 @@ func (b BrokerAPI) Provision(ctx context.Context, instanceID string, details dom
 }
 
 // Deprovision deletes an existing service instance
-//  DELETE /v2/service_instances/{instance_id}
+//
+//	DELETE /v2/service_instances/{instance_id}
 func (b BrokerAPI) Deprovision(ctx context.Context, instanceID string, details domain.DeprovisionDetails, asyncAllowed bool) (domain.DeprovisionServiceSpec, error) {
 	rctx := reqcontext.NewReqContext(ctx, b.logger, lager.Data{
 		"instance-id": instanceID,
@@ -72,7 +75,8 @@ func (b BrokerAPI) Deprovision(ctx context.Context, instanceID string, details d
 }
 
 // GetInstance fetches information about a service instance
-//   GET /v2/service_instances/{instance_id}
+//
+//	GET /v2/service_instances/{instance_id}
 func (b BrokerAPI) GetInstance(ctx context.Context, instanceID string, details domain.FetchInstanceDetails) (domain.GetInstanceDetailsSpec, error) {
 	rctx := reqcontext.NewReqContext(ctx, b.logger, lager.Data{
 		"instance-id": instanceID,
@@ -84,7 +88,8 @@ func (b BrokerAPI) GetInstance(ctx context.Context, instanceID string, details d
 }
 
 // Update modifies an existing service instance
-//  PATCH /v2/service_instances/{instance_id}
+//
+//	PATCH /v2/service_instances/{instance_id}
 func (b BrokerAPI) Update(ctx context.Context, instanceID string, details domain.UpdateDetails, asyncAllowed bool) (domain.UpdateServiceSpec, error) {
 	rctx := reqcontext.NewReqContext(ctx, b.logger, lager.Data{
 		"instance-id": instanceID,
@@ -106,7 +111,8 @@ func (b BrokerAPI) Update(ctx context.Context, instanceID string, details domain
 }
 
 // LastOperation fetches last operation state for a service instance
-//   GET /v2/service_instances/{instance_id}/last_operation
+//
+//	GET /v2/service_instances/{instance_id}/last_operation
 func (b BrokerAPI) LastOperation(ctx context.Context, instanceID string, details domain.PollDetails) (domain.LastOperation, error) {
 	rctx := reqcontext.NewReqContext(ctx, b.logger, lager.Data{
 		"instance-id": instanceID,
@@ -120,7 +126,8 @@ func (b BrokerAPI) LastOperation(ctx context.Context, instanceID string, details
 }
 
 // Bind creates a new service binding
-//   PUT /v2/service_instances/{instance_id}/service_bindings/{binding_id}
+//
+//	PUT /v2/service_instances/{instance_id}/service_bindings/{binding_id}
 func (b BrokerAPI) Bind(ctx context.Context, instanceID, bindingID string, details domain.BindDetails, asyncAllowed bool) (domain.Binding, error) {
 	rctx := reqcontext.NewReqContext(ctx, b.logger, lager.Data{
 		"instance-id": instanceID,
@@ -135,7 +142,8 @@ func (b BrokerAPI) Bind(ctx context.Context, instanceID, bindingID string, detai
 }
 
 // Unbind deletes an existing service binding
-//   DELETE /v2/service_instances/{instance_id}/service_bindings/{binding_id}
+//
+//	DELETE /v2/service_instances/{instance_id}/service_bindings/{binding_id}
 func (b BrokerAPI) Unbind(ctx context.Context, instanceID, bindingID string, details domain.UnbindDetails, asyncAllowed bool) (domain.UnbindSpec, error) {
 	rctx := reqcontext.NewReqContext(ctx, b.logger, lager.Data{
 		"instance-id": instanceID,
@@ -150,7 +158,9 @@ func (b BrokerAPI) Unbind(ctx context.Context, instanceID, bindingID string, det
 }
 
 // GetBinding fetches an existing service binding
-//   GET /v2/service_instances/{instance_id}/service_bindings/{binding_id}
+//
+//	GET /v2/service_instances/{instance_id}/service_bindings/{binding_id}
+//
 // TODO(mw): adjust to use details.PlanID when https://github.com/pivotal-cf/brokerapi/pull/138 is merged.
 func (b BrokerAPI) GetBinding(ctx context.Context, instanceID, bindingID string, details domain.FetchBindingDetails) (domain.GetBindingSpec, error) {
 	rctx := reqcontext.NewReqContext(ctx, b.logger, lager.Data{
@@ -164,7 +174,8 @@ func (b BrokerAPI) GetBinding(ctx context.Context, instanceID, bindingID string,
 }
 
 // LastBindingOperation fetches last operation state for a service binding
-//   GET /v2/service_instances/{instance_id}/service_bindings/{binding_id}/last_operation
+//
+//	GET /v2/service_instances/{instance_id}/service_bindings/{binding_id}/last_operation
 func (b BrokerAPI) LastBindingOperation(ctx context.Context, instanceID, bindingID string, details domain.PollDetails) (domain.LastOperation, error) {
 	rctx := reqcontext.NewReqContext(ctx, b.logger, lager.Data{
 		"instance-id": instanceID,

pkg/brokerapi/brokerapi_test.go

@@ -7,6 +7,7 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"testing"
 
 	"code.cloudfoundry.org/lager"
@@ -20,8 +21,11 @@ import (
 	"github.com/stretchr/testify/suite"
 	"github.com/vshn/crossplane-service-broker/pkg/api/auth"
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
+	"github.com/crossplane/crossplane-runtime/pkg/resource/unstructured/composite"
 	cintegration "github.com/crossplane/crossplane-runtime/pkg/test/integration"
 	"github.com/vshn/crossplane-service-broker/pkg/crossplane"
 	"github.com/vshn/crossplane-service-broker/pkg/integration"
@@ -1726,11 +1730,31 @@ func (ts *EnvTestSuite) TestBrokerAPI_Unbind() {
 			resources: func() (func(c client.Client) error, []client.Object) {
 				servicePlan := integration.NewTestServicePlan("1", "1-1", crossplane.MariaDBDatabaseService)
 				instance := integration.NewTestInstance("1-1-1", servicePlan, crossplane.MariaDBDatabaseService, "", "1")
+
+				userInstance := integration.NewTestMariaDBUserInstance("1-1-1", "binding-1")
+
+				gvk := schema.GroupVersionKind{
+					Group:   "mysql.sql.crossplane.io",
+					Version: "v1alpha1",
+					Kind:    "User",
+				}
+				sqlUser := composite.New(composite.WithGroupVersionKind(gvk))
+				sqlUser.SetName("binding-1-user")
+
+				userInstance.SetResourceReferences([]corev1.ObjectReference{
+					{
+						Kind:       sqlUser.GetKind(),
+						Name:       sqlUser.GetName(),
+						APIVersion: sqlUser.GetAPIVersion(),
+					},
+				})
+
 				objs := []client.Object{
 					integration.NewTestService("1", crossplane.MariaDBDatabaseService),
 					servicePlan.Composition,
 					instance,
-					integration.NewTestMariaDBUserInstance("1-1-1", "binding-1"),
+					userInstance,
+					sqlUser,
 					integration.NewTestSecret(integration.TestNamespace, "binding-1-password", map[string]string{
 						xrv1.ResourceCredentialsSecretPasswordKey: "supersecret",
 					}),
@@ -1750,17 +1774,14 @@ func (ts *EnvTestSuite) TestBrokerAPI_Unbind() {
 
 	for _, tt := range tests {
 		ts.Run(tt.name, func() {
+			c := ts.Manager.GetClient()
 			fn, objs := tt.resources()
-			ts.Require().NoError(integration.CreateObjects(tt.args.ctx, objs)(ts.Manager.GetClient()))
+			ts.Require().NoError(integration.CreateObjects(tt.args.ctx, objs)(c))
 			defer func() {
-				// if wantErr == nil, secret must be gone and would error here if we still would try to remove it again
-				if tt.wantErr == nil {
-					objs = objs[:len(objs)-1]
-				}
-				ts.Require().NoError(integration.RemoveObjects(tt.args.ctx, objs)(ts.Manager.GetClient()))
+				ts.Require().NoError(integration.RemoveObjects(tt.args.ctx, objs)(c))
 			}()
 			if fn != nil {
-				ts.Require().NoError(fn(ts.Manager.GetClient()))
+				ts.Require().NoError(fn(c))
 			}
 
 			got, err := bAPI.Unbind(tt.args.ctx, tt.args.instanceID, tt.args.bindingID, tt.args.details, false)
@@ -1771,6 +1792,34 @@ func (ts *EnvTestSuite) TestBrokerAPI_Unbind() {
 
 			ts.Assert().NoError(err)
 			ts.Assert().Equal(*tt.want, got)
+
+			// Check that neither binding nor secret exist anymore or are marked for deletion
+			userInstance := integration.NewTestMariaDBUserInstance("", "")
+			err = c.Get(ctx, client.ObjectKey{
+				Name:      tt.args.bindingID,
+				Namespace: integration.TestNamespace,
+			}, userInstance)
+			if err != nil {
+				ts.Assert().True(apierrors.IsNotFound(err))
+				ts.Assert().NoError(err)
+			} else {
+				ts.Assert().NotNil(userInstance.GetDeletionTimestamp())
+			}
+
+			secret := integration.NewTestSecret("", "", map[string]string{})
+			err = c.Get(ctx, client.ObjectKey{
+				Name:      fmt.Sprintf("%s-password", tt.args.bindingID),
+				Namespace: integration.TestNamespace,
+			}, secret)
+			if err != nil {
+				ts.Assert().True(apierrors.IsNotFound(err))
+			} else if secret.GetDeletionTimestamp() == nil {
+				ts.Assert().NotEmpty(secret.GetOwnerReferences())
+				ts.Assert().True(*secret.GetOwnerReferences()[0].BlockOwnerDeletion)
+				ts.Assert().Equal("binding-1-user", secret.GetOwnerReferences()[0].Name)
+				ts.Assert().NotEmpty(secret.GetOwnerReferences()[0].UID)
+			}
+
 		})
 	}
 }

pkg/crossplane/client.go

@@ -170,8 +170,9 @@ func (cp Crossplane) Plan(rctx *reqcontext.ReqContext, planID string) (*Plan, er
 // The `ok` parameter is *only* set to true if no error is returned and the instance already exists.
 // Normal errors are returned as-is.
 // FIXME(mw): is it correct to return `false, errInstanceNotFound` if PlanNameLabel does not match? And how should that be handled?
-//            Ported from PoC code as-is, and errInstanceNotFound is handled as instance really not found, however as the ID exists, how
-//            can we speak about having no instance with that name? It's a UUID after all.
+//
+//	Ported from PoC code as-is, and errInstanceNotFound is handled as instance really not found, however as the ID exists, how
+//	can we speak about having no instance with that name? It's a UUID after all.
 func (cp Crossplane) Instance(rctx *reqcontext.ReqContext, id string, plan *Plan) (inst *Instance, ok bool, err error) {
 	gvk, err := plan.GVK()
 	if err != nil {

pkg/crossplane/service_mariadb_database.go

@@ -3,9 +3,9 @@ package crossplane
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"strconv"
-	"time"
 
 	"code.cloudfoundry.org/lager"
 	xrv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
@@ -16,8 +16,10 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/utils/pointer"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -108,25 +110,59 @@ func (msb MariadbDatabaseServiceBinder) Bind(ctx context.Context, bindingID stri
 
 // Unbind deletes the created User and Grant.
 func (msb MariadbDatabaseServiceBinder) Unbind(ctx context.Context, bindingID string) error {
+
+	if err := msb.markCredentialsForDeletion(ctx, bindingID); err != nil {
+		return fmt.Errorf("could not mark credentials for deletion: %w", err)
+	}
+
 	cmp := composite.New(composite.WithGroupVersionKind(mariaDBUserGroupVersionKind))
 	cmp.SetName(bindingID)
-	if err := msb.cp.client.Delete(ctx, cmp, client.PropagationPolicy(metav1.DeletePropagationForeground)); err != nil {
-		return err
+	return msb.cp.client.Delete(ctx, cmp, client.PropagationPolicy(metav1.DeletePropagationForeground))
+}
+
+func (msb MariadbDatabaseServiceBinder) markCredentialsForDeletion(ctx context.Context, bindingID string) error {
+	cmp := composite.New(composite.WithGroupVersionKind(mariaDBUserGroupVersionKind))
+	if err := msb.cp.client.Get(ctx, types.NamespacedName{Name: bindingID}, cmp); err != nil {
+		return fmt.Errorf("could not get binding: %w", err)
+	}
+
+	userRef := corev1.ObjectReference{}
+	for _, r := range cmp.GetResourceReferences() {
+		if r.Kind == "User" {
+			userRef = r
+		}
+	}
+	if userRef.Kind == "" {
+		return errors.New("unable to find User object in composite")
 	}
 
-	// TODO: figure out a better way to delete the password secret
-	//       option a) use Watch on resourceRefs of composite and wait until User/Grant are both deleted
-	//       option b) https://github.com/crossplane/crossplane/issues/1612 is implemented by crossplane
-	//                 and wait for the composite to disappear before removing the secret
-	// If we delete the secret too quickly, the provider-sql can't deprovision the user
-	time.Sleep(5 * time.Second)
 	secret := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      fmt.Sprintf(secretName, bindingID),
+			Name:      fmt.Sprintf(secretName, cmp.GetName()),
 			Namespace: msb.cp.config.Namespace,
 		},
 	}
-	return msb.cp.client.Delete(ctx, secret)
+	if err := msb.cp.client.Get(ctx, types.NamespacedName{Name: fmt.Sprintf(secretName, cmp.GetName()), Namespace: msb.cp.config.Namespace}, secret); err != nil {
+		return fmt.Errorf("failed to fetch secret: %w", err)
+	}
+
+	user := &unstructured.Unstructured{}
+	user.SetAPIVersion(userRef.APIVersion)
+	user.SetKind(userRef.Kind)
+	user.SetName(userRef.Name)
+	if err := msb.cp.client.Get(ctx, types.NamespacedName{Name: user.GetName()}, user); err != nil {
+		return fmt.Errorf("failed to fetch user: %w", err)
+	}
+
+	ref := metav1.OwnerReference{
+		APIVersion:         user.GetAPIVersion(),
+		Kind:               user.GetKind(),
+		Name:               user.GetName(),
+		UID:                user.GetUID(),
+		BlockOwnerDeletion: pointer.BoolPtr(true),
+	}
+	secret.SetOwnerReferences([]metav1.OwnerReference{ref})
+	return msb.cp.client.Update(ctx, secret)
 }
 
 // Deprovisionable always returns nil for MariadbDatabase instances.