diff --git a/Makefile.vars.mk b/Makefile.vars.mk index 0eb6f5e..0ba38ca 100644 --- a/Makefile.vars.mk +++ b/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) diff --git a/pkg/api/auth/basic.go b/pkg/api/auth/basic.go index e1624ab..0628678 100644 --- a/pkg/api/auth/basic.go +++ b/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 diff --git a/pkg/api/auth/bearer_token.go b/pkg/api/auth/bearer_token.go index 9d32692..551a8bb 100644 --- a/pkg/api/auth/bearer_token.go +++ b/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 diff --git a/pkg/brokerapi/brokerapi.go b/pkg/brokerapi/brokerapi.go index 09e6fed..62997b4 100644 --- a/pkg/brokerapi/brokerapi.go +++ b/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, diff --git a/pkg/brokerapi/brokerapi_test.go b/pkg/brokerapi/brokerapi_test.go index d492839..cd2a682 100644 --- a/pkg/brokerapi/brokerapi_test.go +++ b/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) + } + }) } } diff --git a/pkg/crossplane/client.go b/pkg/crossplane/client.go index 1a65145..f1a06fc 100644 --- a/pkg/crossplane/client.go +++ b/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 { diff --git a/pkg/crossplane/service_mariadb_database.go b/pkg/crossplane/service_mariadb_database.go index b51cf9d..11374c4 100644 --- a/pkg/crossplane/service_mariadb_database.go +++ b/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. diff --git a/testdata/crds/mysql.sql.crossplane.io_users.yaml b/testdata/crds/mysql.sql.crossplane.io_users.yaml new file mode 100644 index 0000000..008b7fa --- /dev/null +++ b/testdata/crds/mysql.sql.crossplane.io_users.yaml @@ -0,0 +1,48 @@ +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.3.0 + creationTimestamp: null + name: users.mysql.sql.crossplane.io +spec: + group: mysql.sql.crossplane.io + names: + categories: + - crossplane + - managed + - sql + kind: User + listKind: UserList + plural: users + singular: user + scope: Cluster + versions: + - additionalPrinterColumns: + - jsonPath: .status.conditions[?(@.type=='Ready')].status + name: READY + type: string + - jsonPath: .status.conditions[?(@.type=='Synced')].status + name: SYNCED + type: string + - jsonPath: .metadata.creationTimestamp + name: AGE + type: date + name: v1alpha1 + schema: + openAPIV3Schema: + description: A User represents the declarative state of a MySQL user. + properties: + apiVersion: + description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + type: string + kind: + description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + type: string + metadata: + type: object + type: object + served: true + storage: true + subresources: + status: {}