From c8a2bcbe74424decb2de51eb0787a53626566307 Mon Sep 17 00:00:00 2001 From: Georgi Sabev Date: Fri, 23 Feb 2024 14:36:45 +0000 Subject: [PATCH] Support objects for user-provided services credentials Previously Korifi supported plain `map[string]string` for UPSI credentials. This deviates from the CF API spec where credentials can be arbitrary objects. As credentials are stored in secrets which are glorified `map[string][]byte` we had to change the way we store the credentials object: * We marshal the credentials object into a byte array and store it under the `crdentials` key in the secret. We no longer use a secret key per every credentials value. The `CFServiceInstance.Spec.SecretName` references that credentials secret. * We introduce `CFServiceInstance.Status.Credentials` and `CFServiceInstance.Status.CredentialsObservedVersion` that aremanaged by the `CFServiceInstance` controller. Once the controller reconciles the service instance, it sets the secret name and its resource version into the new status fields. * The CFServiceBindingController reconciles the CFServiceInstance credentials secret into the flat format that is then used for the servicebinding.io binding. In case of a json object value to a key, the value is represented as a json string. * Existing bindings are not affected when upgrading Korifi. However, once the user updates the service instance credentials, the binding secret is rebuilt which causes statefulset restart. Further updates to the credentials will not cause additional restarts. * We have created ADR 16 documenting how this works fixes https://github.com/cloudfoundry/korifi/issues/2900 Co-authored-by: Danail Branekov --- api/handlers/service_instance_test.go | 4 +- api/main.go | 15 +- api/payloads/service_instance.go | 12 +- api/payloads/service_instance_test.go | 27 +- .../service_instance_repository.go | 251 +++++++---- .../service_instance_repository_test.go | 231 ++++++---- .../api/v1alpha1/cfserviceinstance_types.go | 22 +- .../api/v1alpha1/zz_generated.deepcopy.go | 2 +- .../services/cfservicebinding_controller.go | 169 ++++++-- .../cfservicebinding_controller_test.go | 395 ++++++++++-------- .../services/cfserviceinstance_controller.go | 153 +++++-- .../cfserviceinstance_controller_test.go | 227 ++++++++-- .../services/credentials/credentials.go | 68 +++ .../credentials/credentials_suite_test.go | 13 + .../services/credentials/credentials_test.go | 125 ++++++ controllers/controllers/shared/index.go | 23 +- .../0016-user-provided-service-credentils.md | 57 +++ go.mod | 2 +- .../korifi/controllers/cf_roles/cf_admin.yaml | 1 + .../cf_roles/cf_space_developer.yaml | 1 + ...i.cloudfoundry.org_cfserviceinstances.yaml | 31 +- tests/e2e/apps_test.go | 4 +- tests/e2e/e2e_suite_test.go | 6 +- tests/e2e/service_instances_test.go | 9 +- 24 files changed, 1323 insertions(+), 525 deletions(-) create mode 100644 controllers/controllers/services/credentials/credentials.go create mode 100644 controllers/controllers/services/credentials/credentials_suite_test.go create mode 100644 controllers/controllers/services/credentials/credentials_test.go create mode 100644 docs/architecture-decisions/0016-user-provided-service-credentils.md diff --git a/api/handlers/service_instance_test.go b/api/handlers/service_instance_test.go index b647e1a9a..167d6857e 100644 --- a/api/handlers/service_instance_test.go +++ b/api/handlers/service_instance_test.go @@ -273,7 +273,7 @@ var _ = Describe("ServiceInstance", func() { requestValidator.DecodeAndValidateJSONPayloadStub = decodeAndValidatePayloadStub(&payloads.ServiceInstancePatch{ Name: tools.PtrTo("new-name"), Tags: &[]string{"alice", "bob"}, - Credentials: &map[string]string{"foo": "bar"}, + Credentials: &map[string]any{"foo": "bar"}, Metadata: payloads.MetadataPatch{ Annotations: map[string]*string{"ann2": tools.PtrTo("ann_val2")}, Labels: map[string]*string{"lab2": tools.PtrTo("lab_val2")}, @@ -306,7 +306,7 @@ var _ = Describe("ServiceInstance", func() { GUID: "service-instance-guid", SpaceGUID: "space-guid", Name: tools.PtrTo("new-name"), - Credentials: &map[string]string{"foo": "bar"}, + Credentials: &map[string]any{"foo": "bar"}, Tags: &[]string{"alice", "bob"}, MetadataPatch: repositories.MetadataPatch{ Annotations: map[string]*string{"ann2": tools.PtrTo("ann_val2")}, diff --git a/api/main.go b/api/main.go index 9a55605bf..328717fdc 100644 --- a/api/main.go +++ b/api/main.go @@ -45,7 +45,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/apiutil" ) -var createTimeout = time.Second * 120 +var conditionTimeout = time.Second * 120 func init() { utilruntime.Must(korifiv1alpha1.AddToScheme(scheme.Scheme)) @@ -126,14 +126,14 @@ func main() { privilegedCRClient, userClientFactory, nsPermissions, - conditions.NewConditionAwaiter[*korifiv1alpha1.CFOrg, korifiv1alpha1.CFOrgList](createTimeout), + conditions.NewConditionAwaiter[*korifiv1alpha1.CFOrg, korifiv1alpha1.CFOrgList](conditionTimeout), ) spaceRepo := repositories.NewSpaceRepo( namespaceRetriever, orgRepo, userClientFactory, nsPermissions, - conditions.NewConditionAwaiter[*korifiv1alpha1.CFSpace, korifiv1alpha1.CFSpaceList](createTimeout), + conditions.NewConditionAwaiter[*korifiv1alpha1.CFSpace, korifiv1alpha1.CFSpaceList](conditionTimeout), ) processRepo := repositories.NewProcessRepo( namespaceRetriever, @@ -147,7 +147,7 @@ func main() { namespaceRetriever, userClientFactory, nsPermissions, - conditions.NewConditionAwaiter[*korifiv1alpha1.CFApp, korifiv1alpha1.CFAppList](createTimeout), + conditions.NewConditionAwaiter[*korifiv1alpha1.CFApp, korifiv1alpha1.CFAppList](conditionTimeout), ) dropletRepo := repositories.NewDropletRepo( userClientFactory, @@ -183,18 +183,19 @@ func main() { nsPermissions, toolsregistry.NewRepositoryCreator(cfg.ContainerRegistryType), cfg.ContainerRepositoryPrefix, - conditions.NewConditionAwaiter[*korifiv1alpha1.CFPackage, korifiv1alpha1.CFPackageList](createTimeout), + conditions.NewConditionAwaiter[*korifiv1alpha1.CFPackage, korifiv1alpha1.CFPackageList](conditionTimeout), ) serviceInstanceRepo := repositories.NewServiceInstanceRepo( namespaceRetriever, userClientFactory, nsPermissions, + conditions.NewConditionAwaiter[*korifiv1alpha1.CFServiceInstance, korifiv1alpha1.CFServiceInstanceList](conditionTimeout), ) serviceBindingRepo := repositories.NewServiceBindingRepo( namespaceRetriever, userClientFactory, nsPermissions, - conditions.NewConditionAwaiter[*korifiv1alpha1.CFServiceBinding, korifiv1alpha1.CFServiceBindingList](createTimeout), + conditions.NewConditionAwaiter[*korifiv1alpha1.CFServiceBinding, korifiv1alpha1.CFServiceBindingList](conditionTimeout), ) buildpackRepo := repositories.NewBuildpackRepository(cfg.BuilderName, userClientFactory, @@ -221,7 +222,7 @@ func main() { userClientFactory, namespaceRetriever, nsPermissions, - conditions.NewConditionAwaiter[*korifiv1alpha1.CFTask, korifiv1alpha1.CFTaskList](createTimeout), + conditions.NewConditionAwaiter[*korifiv1alpha1.CFTask, korifiv1alpha1.CFTaskList](conditionTimeout), ) metricsRepo := repositories.NewMetricsRepo(userClientFactory) diff --git a/api/payloads/service_instance.go b/api/payloads/service_instance.go index 6cac9fba3..a3bc8e87c 100644 --- a/api/payloads/service_instance.go +++ b/api/payloads/service_instance.go @@ -17,7 +17,7 @@ type ServiceInstanceCreate struct { Name string `json:"name"` Type string `json:"type"` Tags []string `json:"tags"` - Credentials map[string]string `json:"credentials"` + Credentials map[string]any `json:"credentials"` Relationships *ServiceInstanceRelationships `json:"relationships"` Metadata Metadata `json:"metadata"` } @@ -74,10 +74,10 @@ func (r ServiceInstanceRelationships) Validate() error { } type ServiceInstancePatch struct { - Name *string `json:"name,omitempty"` - Tags *[]string `json:"tags,omitempty"` - Credentials *map[string]string `json:"credentials,omitempty"` - Metadata MetadataPatch `json:"metadata"` + Name *string `json:"name,omitempty"` + Tags *[]string `json:"tags,omitempty"` + Credentials *map[string]any `json:"credentials,omitempty"` + Metadata MetadataPatch `json:"metadata"` } func (p ServiceInstancePatch) Validate() error { @@ -120,7 +120,7 @@ func (p *ServiceInstancePatch) UnmarshalJSON(data []byte) error { } if v, ok := patchMap["credentials"]; ok && v == nil { - patch.Credentials = &map[string]string{} + patch.Credentials = &map[string]any{} } *p = ServiceInstancePatch(patch) diff --git a/api/payloads/service_instance_test.go b/api/payloads/service_instance_test.go index dc474625e..0e0490dc4 100644 --- a/api/payloads/service_instance_test.go +++ b/api/payloads/service_instance_test.go @@ -85,9 +85,12 @@ var _ = Describe("ServiceInstanceCreate", func() { Name: "service-instance-name", Type: "user-provided", Tags: []string{"foo", "bar"}, - Credentials: map[string]string{ + Credentials: map[string]any{ "username": "bob", "password": "float", + "object": map[string]any{ + "a": "b", + }, }, Relationships: &payloads.ServiceInstanceRelationships{ Space: &payloads.Relationship{ @@ -188,9 +191,13 @@ var _ = Describe("ServiceInstanceCreate", func() { Expect(msg.Annotations).To(HaveKeyWithValue("ann1", "val_ann1")) Expect(msg.Labels).To(HaveLen(1)) Expect(msg.Labels).To(HaveKeyWithValue("lab1", "val_lab1")) - Expect(msg.Credentials).To(HaveLen(2)) - Expect(msg.Credentials).To(HaveKeyWithValue("username", "bob")) - Expect(msg.Credentials).To(HaveKeyWithValue("password", "float")) + Expect(msg.Credentials).To(MatchAllKeys(Keys{ + "username": Equal("bob"), + "password": Equal("float"), + "object": MatchAllKeys(Keys{ + "a": Equal("b"), + }), + })) }) }) }) @@ -261,9 +268,10 @@ var _ = Describe("ServiceInstancePatch", func() { patchPayload = payloads.ServiceInstancePatch{ Name: tools.PtrTo("service-instance-name"), Tags: &[]string{"foo", "bar"}, - Credentials: &map[string]string{ - "username": "bob", - "password": "float", + Credentials: &map[string]any{ + "object": map[string]any{ + "a": "b", + }, }, Metadata: payloads.MetadataPatch{ Annotations: map[string]*string{"ann1": tools.PtrTo("val_ann1")}, @@ -316,8 +324,9 @@ var _ = Describe("ServiceInstancePatch", func() { "lab1": PointTo(Equal("val_lab1")), })) Expect(msg.Credentials).To(PointTo(MatchAllKeys(Keys{ - "username": Equal("bob"), - "password": Equal("float"), + "object": MatchAllKeys(Keys{ + "a": Equal("b"), + }), }))) }) }) diff --git a/api/repositories/service_instance_repository.go b/api/repositories/service_instance_repository.go index 1f6e4b19f..04fcd101f 100644 --- a/api/repositories/service_instance_repository.go +++ b/api/repositories/service_instance_repository.go @@ -2,27 +2,34 @@ package repositories import ( "context" + "encoding/json" + "errors" "fmt" + "reflect" "time" "code.cloudfoundry.org/korifi/api/authorization" apierrors "code.cloudfoundry.org/korifi/api/errors" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" "code.cloudfoundry.org/korifi/tools/k8s" + "golang.org/x/exp/maps" "github.com/google/uuid" 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/labels" + "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) const ( - CFServiceInstanceGUIDLabel = "korifi.cloudfoundry.org/service-instance-guid" - ServiceInstanceResourceType = "Service Instance" - serviceBindingSecretTypePrefix = "servicebinding.io/" + CFServiceInstanceGUIDLabel = "korifi.cloudfoundry.org/service-instance-guid" + ServiceInstanceResourceType = "Service Instance" + CredentialsSecretAvailableCondition = "CredentialSecretAvailable" + + credentialsTypeKey = "type" ) type NamespaceGetter interface { @@ -33,24 +40,27 @@ type ServiceInstanceRepo struct { namespaceRetriever NamespaceRetriever userClientFactory authorization.UserK8sClientFactory namespacePermissions *authorization.NamespacePermissions + awaiter ConditionAwaiter[*korifiv1alpha1.CFServiceInstance] } func NewServiceInstanceRepo( namespaceRetriever NamespaceRetriever, userClientFactory authorization.UserK8sClientFactory, namespacePermissions *authorization.NamespacePermissions, + awaiter ConditionAwaiter[*korifiv1alpha1.CFServiceInstance], ) *ServiceInstanceRepo { return &ServiceInstanceRepo{ namespaceRetriever: namespaceRetriever, userClientFactory: userClientFactory, namespacePermissions: namespacePermissions, + awaiter: awaiter, } } type CreateServiceInstanceMessage struct { Name string SpaceGUID string - Credentials map[string]string + Credentials map[string]any Type string Tags []string Labels map[string]string @@ -61,7 +71,7 @@ type PatchServiceInstanceMessage struct { GUID string SpaceGUID string Name *string - Credentials *map[string]string + Credentials *map[string]any Tags *[]string MetadataPatch } @@ -109,21 +119,12 @@ func (r *ServiceInstanceRepo) CreateServiceInstance(ctx context.Context, authInf } cfServiceInstance := message.toCFServiceInstance() - err = userClient.Create(ctx, &cfServiceInstance) + err = userClient.Create(ctx, cfServiceInstance) if err != nil { return ServiceInstanceRecord{}, apierrors.FromK8sError(err, ServiceInstanceResourceType) } - secretObj := cfServiceInstanceToSecret(cfServiceInstance) - _, err = controllerutil.CreateOrPatch(ctx, userClient, &secretObj, func() error { - secretObj.StringData = message.Credentials - if secretObj.StringData == nil { - secretObj.StringData = map[string]string{} - } - createSecretTypeFields(&secretObj) - - return nil - }) + err = r.createCredentialsSecret(ctx, userClient, cfServiceInstance, defaultCredentialsType(message.Credentials)) if err != nil { return ServiceInstanceRecord{}, apierrors.FromK8sError(err, ServiceInstanceResourceType) } @@ -131,59 +132,178 @@ func (r *ServiceInstanceRepo) CreateServiceInstance(ctx context.Context, authInf return cfServiceInstanceToServiceInstanceRecord(cfServiceInstance), nil } +func defaultCredentialsType(credentials map[string]any) map[string]any { + result := map[string]any{} + maps.Copy(result, credentials) + if _, hasCredentialsType := result[credentialsTypeKey]; !hasCredentialsType { + result[credentialsTypeKey] = korifiv1alpha1.UserProvidedType + } + + return result +} + func (r *ServiceInstanceRepo) PatchServiceInstance(ctx context.Context, authInfo authorization.Info, message PatchServiceInstanceMessage) (ServiceInstanceRecord, error) { userClient, err := r.userClientFactory.BuildClient(authInfo) if err != nil { return ServiceInstanceRecord{}, fmt.Errorf("failed to build user client: %w", err) } - var cfServiceInstance korifiv1alpha1.CFServiceInstance + cfServiceInstance := &korifiv1alpha1.CFServiceInstance{} cfServiceInstance.Namespace = message.SpaceGUID cfServiceInstance.Name = message.GUID - if err = userClient.Get(ctx, client.ObjectKeyFromObject(&cfServiceInstance), &cfServiceInstance); err != nil { + if err = userClient.Get(ctx, client.ObjectKeyFromObject(cfServiceInstance), cfServiceInstance); err != nil { return ServiceInstanceRecord{}, apierrors.FromK8sError(err, ServiceInstanceResourceType) } - err = k8s.PatchResource(ctx, userClient, &cfServiceInstance, func() { - message.Apply(&cfServiceInstance) + err = k8s.PatchResource(ctx, userClient, cfServiceInstance, func() { + message.Apply(cfServiceInstance) }) if err != nil { return ServiceInstanceRecord{}, apierrors.FromK8sError(err, ServiceInstanceResourceType) } if message.Credentials != nil { - secretObj := new(corev1.Secret) - if err = userClient.Get(ctx, client.ObjectKey{Name: cfServiceInstance.Spec.SecretName, Namespace: cfServiceInstance.Namespace}, secretObj); err != nil { + cfServiceInstance, err = r.migrateLegacyCredentials(ctx, userClient, cfServiceInstance) + if err != nil { return ServiceInstanceRecord{}, err } - - if _, ok := (*message.Credentials)["type"]; !ok { - (*message.Credentials)["type"] = string(secretObj.Data["type"]) + err = r.patchCredentialsSecret(ctx, userClient, cfServiceInstance, *message.Credentials) + if err != nil { + return ServiceInstanceRecord{}, apierrors.FromK8sError(err, ServiceInstanceResourceType) } + } - newType := (*message.Credentials)["type"] - if string(secretObj.Data["type"]) != (*message.Credentials)["type"] { - return ServiceInstanceRecord{}, apierrors.NewInvalidRequestError( - fmt.Errorf("cannot modify credential type: currently '%s': updating to '%s'", string(secretObj.Data["type"]), newType), - "Cannot change credential type. Consider creating a new Service Instance.", - ) - } + return cfServiceInstanceToServiceInstanceRecord(cfServiceInstance), nil +} - _, err = controllerutil.CreateOrPatch(ctx, userClient, secretObj, func() error { - data := map[string][]byte{} - for k, v := range *message.Credentials { - data[k] = []byte(v) - } - secretObj.Data = data +func (r *ServiceInstanceRepo) migrateLegacyCredentials(ctx context.Context, userClient client.WithWatch, cfServiceInstance *korifiv1alpha1.CFServiceInstance) (*korifiv1alpha1.CFServiceInstance, error) { + cfServiceInstance, err := r.awaiter.AwaitCondition(ctx, userClient, cfServiceInstance, CredentialsSecretAvailableCondition) + if err != nil { + return nil, err + } + err = k8s.PatchResource(ctx, userClient, cfServiceInstance, func() { + cfServiceInstance.Spec.SecretName = cfServiceInstance.Status.Credentials.Name + }) + if err != nil { + return nil, apierrors.FromK8sError(err, ServiceInstanceResourceType) + } + + return cfServiceInstance, nil +} - return nil - }) +func (r *ServiceInstanceRepo) patchCredentialsSecret( + ctx context.Context, + userClient client.Client, + cfServiceInstance *korifiv1alpha1.CFServiceInstance, + credentials map[string]any, +) error { + newCredentials := map[string]any{} + maps.Copy(newCredentials, credentials) + + credentialsSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: cfServiceInstance.Spec.SecretName, + Namespace: cfServiceInstance.Namespace, + }, + } + + err := userClient.Get(ctx, client.ObjectKeyFromObject(credentialsSecret), credentialsSecret) + if err != nil { + return err + } + + oldCredentials := fromSecretData(credentialsSecret.Data) + err = validateCredentialsChange(oldCredentials, newCredentials) + if err != nil { + return err + } + + if oldCredentialType, hasCredentialsType := oldCredentials[credentialsTypeKey]; hasCredentialsType { + newCredentials[credentialsTypeKey] = oldCredentialType + } + + return r.createCredentialsSecret(ctx, userClient, cfServiceInstance, newCredentials) +} + +func (r *ServiceInstanceRepo) createCredentialsSecret( + ctx context.Context, + userClient client.Client, + cfServiceInstance *korifiv1alpha1.CFServiceInstance, + credentials map[string]any, +) error { + newCredentials := map[string]any{} + maps.Copy(newCredentials, credentials) + + credentialsSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: cfServiceInstance.Spec.SecretName, + Namespace: cfServiceInstance.Namespace, + }, + } + + _, err := controllerutil.CreateOrPatch(ctx, userClient, credentialsSecret, func() error { + if credentialsSecret.Labels == nil { + credentialsSecret.Labels = map[string]string{} + } + credentialsSecret.Labels[CFServiceInstanceGUIDLabel] = cfServiceInstance.Name + + var err error + credentialsSecret.Data, err = toSecretData(newCredentials) if err != nil { - return ServiceInstanceRecord{}, apierrors.FromK8sError(err, ServiceInstanceResourceType) + return errors.New("failed to marshal credentials for service instance") } + + return controllerutil.SetOwnerReference(cfServiceInstance, credentialsSecret, scheme.Scheme) + }) + return err +} + +func toSecretData(credentials map[string]any) (map[string][]byte, error) { + var credentialBytes []byte + credentialBytes, err := json.Marshal(credentials) + if err != nil { + return nil, errors.New("failed to marshal credentials for service instance") } - return cfServiceInstanceToServiceInstanceRecord(cfServiceInstance), nil + return map[string][]byte{ + korifiv1alpha1.CredentialsSecretKey: credentialBytes, + }, nil +} + +func fromSecretData(credentialsSecretData map[string][]byte) map[string]any { + credentialsBytes, hasCredentials := credentialsSecretData[korifiv1alpha1.CredentialsSecretKey] + if !hasCredentials { + return nil + } + + var credentials map[string]any + err := json.Unmarshal(credentialsBytes, &credentials) + if err != nil { + return nil + } + + return credentials +} + +func validateCredentialsChange(oldCredentials, newCredentials map[string]any) error { + oldType, hasType := oldCredentials[credentialsTypeKey] + if !hasType { + oldType = korifiv1alpha1.UserProvidedType + } + + newType, hasType := newCredentials[credentialsTypeKey] + if !hasType { + return nil + } + + if !reflect.DeepEqual(oldType, newType) { + return apierrors.NewInvalidRequestError( + fmt.Errorf("cannot modify credential type: currently '%v': updating to '%v'", oldType, newType), + "Cannot change credential type. Consider creating a new Service Instance.", + ) + } + + return nil } // nolint:dupl @@ -244,8 +364,8 @@ func (r *ServiceInstanceRepo) GetServiceInstance(ctx context.Context, authInfo a return ServiceInstanceRecord{}, fmt.Errorf("failed to get namespace for service instance: %w", err) } - var serviceInstance korifiv1alpha1.CFServiceInstance - if err := userClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: guid}, &serviceInstance); err != nil { + serviceInstance := &korifiv1alpha1.CFServiceInstance{} + if err := userClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: guid}, serviceInstance); err != nil { return ServiceInstanceRecord{}, fmt.Errorf("failed to get service instance: %w", apierrors.FromK8sError(err, ServiceInstanceResourceType)) } @@ -272,9 +392,9 @@ func (r *ServiceInstanceRepo) DeleteServiceInstance(ctx context.Context, authInf return nil } -func (m CreateServiceInstanceMessage) toCFServiceInstance() korifiv1alpha1.CFServiceInstance { +func (m CreateServiceInstanceMessage) toCFServiceInstance() *korifiv1alpha1.CFServiceInstance { guid := uuid.NewString() - return korifiv1alpha1.CFServiceInstance{ + return &korifiv1alpha1.CFServiceInstance{ ObjectMeta: metav1.ObjectMeta{ Name: guid, Namespace: m.SpaceGUID, @@ -290,7 +410,7 @@ func (m CreateServiceInstanceMessage) toCFServiceInstance() korifiv1alpha1.CFSer } } -func cfServiceInstanceToServiceInstanceRecord(cfServiceInstance korifiv1alpha1.CFServiceInstance) ServiceInstanceRecord { +func cfServiceInstanceToServiceInstanceRecord(cfServiceInstance *korifiv1alpha1.CFServiceInstance) ServiceInstanceRecord { return ServiceInstanceRecord{ Name: cfServiceInstance.Spec.DisplayName, GUID: cfServiceInstance.Name, @@ -301,46 +421,15 @@ func cfServiceInstanceToServiceInstanceRecord(cfServiceInstance korifiv1alpha1.C Labels: cfServiceInstance.Labels, Annotations: cfServiceInstance.Annotations, CreatedAt: cfServiceInstance.CreationTimestamp.Time, - UpdatedAt: getLastUpdatedTime(&cfServiceInstance), - } -} - -func cfServiceInstanceToSecret(cfServiceInstance korifiv1alpha1.CFServiceInstance) corev1.Secret { - labels := make(map[string]string, 1) - labels[CFServiceInstanceGUIDLabel] = cfServiceInstance.Name - - return corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: cfServiceInstance.Name, - Namespace: cfServiceInstance.Namespace, - Labels: labels, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: korifiv1alpha1.GroupVersion.String(), - Kind: "CFServiceInstance", - Name: cfServiceInstance.Name, - UID: cfServiceInstance.UID, - }, - }, - }, + UpdatedAt: getLastUpdatedTime(cfServiceInstance), } } func returnServiceInstanceList(serviceInstanceList []korifiv1alpha1.CFServiceInstance) []ServiceInstanceRecord { serviceInstanceRecords := make([]ServiceInstanceRecord, 0, len(serviceInstanceList)) - for _, serviceInstance := range serviceInstanceList { - serviceInstanceRecords = append(serviceInstanceRecords, cfServiceInstanceToServiceInstanceRecord(serviceInstance)) + for i := range serviceInstanceList { + serviceInstanceRecords = append(serviceInstanceRecords, cfServiceInstanceToServiceInstanceRecord(&serviceInstanceList[i])) } return serviceInstanceRecords } - -func createSecretTypeFields(secret *corev1.Secret) { - userSpecifiedType, typeSpecified := secret.StringData["type"] - if typeSpecified { - secret.Type = corev1.SecretType(serviceBindingSecretTypePrefix + userSpecifiedType) - } else { - secret.StringData["type"] = korifiv1alpha1.UserProvidedType - secret.Type = serviceBindingSecretTypePrefix + korifiv1alpha1.UserProvidedType - } -} diff --git a/api/repositories/service_instance_repository_test.go b/api/repositories/service_instance_repository_test.go index a6ead6c04..3b7bdd70b 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -2,6 +2,7 @@ package repositories_test import ( "context" + "encoding/json" "errors" "fmt" "time" @@ -28,6 +29,11 @@ var _ = Describe("ServiceInstanceRepository", func() { var ( testCtx context.Context serviceInstanceRepo *repositories.ServiceInstanceRepo + conditionAwaiter *FakeAwaiter[ + *korifiv1alpha1.CFServiceInstance, + korifiv1alpha1.CFServiceInstanceList, + *korifiv1alpha1.CFServiceInstanceList, + ] org *korifiv1alpha1.CFOrg space *korifiv1alpha1.CFSpace @@ -36,7 +42,12 @@ var _ = Describe("ServiceInstanceRepository", func() { BeforeEach(func() { testCtx = context.Background() - serviceInstanceRepo = repositories.NewServiceInstanceRepo(namespaceRetriever, userClientFactory, nsPerms) + conditionAwaiter = &FakeAwaiter[ + *korifiv1alpha1.CFServiceInstance, + korifiv1alpha1.CFServiceInstanceList, + *korifiv1alpha1.CFServiceInstanceList, + ]{} + serviceInstanceRepo = repositories.NewServiceInstanceRepo(namespaceRetriever, userClientFactory, nsPerms, conditionAwaiter) org = createOrgWithCleanup(testCtx, prefixedGUID("org")) space = createSpaceWithCleanup(testCtx, org.Name, prefixedGUID("space1")) @@ -47,7 +58,7 @@ var _ = Describe("ServiceInstanceRepository", func() { var ( serviceInstanceCreateMessage repositories.CreateServiceInstanceMessage serviceInstanceTags []string - serviceInstanceCredentials map[string]string + serviceInstanceCredentials map[string]any createdServiceInstanceRecord repositories.ServiceInstanceRecord createErr error @@ -55,9 +66,9 @@ var _ = Describe("ServiceInstanceRepository", func() { BeforeEach(func() { serviceInstanceTags = []string{"foo", "bar"} - serviceInstanceCredentials = map[string]string{ - "cred-one": "val-one", - "cred-two": "val-two", + serviceInstanceCredentials = map[string]any{ + "type": "my-type", + "object": map[string]any{"a": "b"}, } serviceInstanceCreateMessage = initializeServiceInstanceCreateMessage(serviceInstanceName, space.Name, serviceInstanceTags, serviceInstanceCredentials) @@ -75,15 +86,13 @@ var _ = Describe("ServiceInstanceRepository", func() { }) JustBeforeEach(func() { + Expect(createErr).NotTo(HaveOccurred()) + secretLookupKey := types.NamespacedName{Name: createdServiceInstanceRecord.SecretName, Namespace: createdServiceInstanceRecord.SpaceGUID} createdSecret = &corev1.Secret{} Expect(k8sClient.Get(context.Background(), secretLookupKey, createdSecret)).To(Succeed()) }) - It("succeeds", func() { - Expect(createErr).NotTo(HaveOccurred()) - }) - It("creates a new ServiceInstance CR", func() { Expect(createdServiceInstanceRecord.GUID).To(MatchRegexp("^[-0-9a-f]{36}$"), "record GUID was not a 36 character guid") Expect(createdServiceInstanceRecord.SpaceGUID).To(Equal(space.Name), "SpaceGUID in record did not match input") @@ -95,58 +104,29 @@ var _ = Describe("ServiceInstanceRepository", func() { Expect(createdServiceInstanceRecord.UpdatedAt).To(PointTo(BeTemporally("~", time.Now(), timeCheckThreshold))) }) - When("ServiceInstance credentials are NOT provided", func() { - BeforeEach(func() { - serviceInstanceCreateMessage.Credentials = nil - }) - - It("creates the secret and sets the type fields to user-provided since projected bindings must have a type", func() { - Expect(createdServiceInstanceRecord.SecretName).To(Equal(createdServiceInstanceRecord.GUID)) - - Expect(createdSecret.Data).To(MatchAllKeys(Keys{ - "type": BeEquivalentTo("user-provided"), - })) - Expect(createdSecret.Type).To(Equal(corev1.SecretType("servicebinding.io/user-provided"))) - }) + It("creates the credentials secret", func() { + Expect(createdSecret.Type).To(Equal(corev1.SecretTypeOpaque)) + Expect(createdSecret.Data).To(MatchAllKeys(Keys{korifiv1alpha1.CredentialsSecretKey: Not(BeEmpty())})) + credentials := map[string]any{} + Expect(json.Unmarshal(createdSecret.Data[korifiv1alpha1.CredentialsSecretKey], &credentials)).To(Succeed()) + Expect(credentials).To(Equal(serviceInstanceCredentials)) }) - When("ServiceInstance credentials are provided", func() { - When("the instance credentials have a user-specified type", func() { - BeforeEach(func() { - serviceInstanceCredentials = map[string]string{ - "cred-one": "val-one", - "cred-two": "val-two", - "type": "mysql", - "provider": "the-cloud", - } - - serviceInstanceCreateMessage = initializeServiceInstanceCreateMessage(serviceInstanceName, space.Name, serviceInstanceTags, serviceInstanceCredentials) - }) - - It("creates the secret and does not override the type that the user specified", func() { - Expect(createdServiceInstanceRecord.SecretName).To(Equal(createdServiceInstanceRecord.GUID)) - - Expect(createdSecret.Data).To(MatchAllKeys(Keys{ - "type": BeEquivalentTo("mysql"), - "provider": BeEquivalentTo("the-cloud"), - "cred-one": BeEquivalentTo("val-one"), - "cred-two": BeEquivalentTo("val-two"), - })) - Expect(createdSecret.Type).To(Equal(corev1.SecretType("servicebinding.io/mysql"))) - }) + When("ServiceInstance credential type is not provided", func() { + BeforeEach(func() { + serviceInstanceCreateMessage.Credentials = map[string]any{ + "a": "b", + } }) - When("the instance credentials DO NOT a user-specified type", func() { - It("creates a secret and defaults type fields to 'user-provided' since projected bindings must have a type", func() { - Expect(createdServiceInstanceRecord.SecretName).To(Equal(createdServiceInstanceRecord.GUID)) - - Expect(createdSecret.Data).To(MatchAllKeys(Keys{ - "type": BeEquivalentTo("user-provided"), - "cred-one": BeEquivalentTo("val-one"), - "cred-two": BeEquivalentTo("val-two"), - })) - Expect(createdSecret.Type).To(Equal(corev1.SecretType("servicebinding.io/user-provided"))) - }) + It("defaults the credential type", func() { + Expect(createdSecret.Data).To(MatchAllKeys(Keys{korifiv1alpha1.CredentialsSecretKey: Not(BeEmpty())})) + credentials := map[string]any{} + Expect(json.Unmarshal(createdSecret.Data[korifiv1alpha1.CredentialsSecretKey], &credentials)).To(Succeed()) + Expect(credentials).To(MatchAllKeys(Keys{ + "type": Equal("user-provided"), + "a": Equal("b"), + })) }) }) }) @@ -169,18 +149,21 @@ var _ = Describe("ServiceInstanceRepository", func() { BeforeEach(func() { serviceInstanceGUID := uuid.NewString() - cfServiceInstance = createServiceInstanceCR(ctx, k8sClient, serviceInstanceGUID, space.Name, serviceInstanceName, serviceInstanceGUID) + secretName := uuid.NewString() + cfServiceInstance = createServiceInstanceCR(ctx, k8sClient, serviceInstanceGUID, space.Name, serviceInstanceName, secretName) + conditionAwaiter.AwaitConditionReturns(cfServiceInstance, nil) + Expect(k8s.Patch(ctx, k8sClient, cfServiceInstance, func() { + cfServiceInstance.Status.Credentials.Name = secretName + })).To(Succeed()) secret = &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: serviceInstanceGUID, + Name: secretName, Namespace: space.Name, }, StringData: map[string]string{ - "foo": "bar", - "type": "database", + korifiv1alpha1.CredentialsSecretKey: `{"type":"database","a": "b"}`, }, - Type: "servicebinding.io/user-provided", } Expect(k8sClient.Create(ctx, secret)).To(Succeed()) @@ -271,29 +254,83 @@ var _ = Describe("ServiceInstanceRepository", func() { It("does not change the credential secret", func() { Consistently(func(g Gomega) { g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)).To(Succeed()) - g.Expect(secret.Data).To(HaveKeyWithValue("foo", BeEquivalentTo("bar"))) + g.Expect(secret.Data).To(MatchAllKeys(Keys{korifiv1alpha1.CredentialsSecretKey: Not(BeEmpty())})) + credentials := map[string]any{} + g.Expect(json.Unmarshal(secret.Data[korifiv1alpha1.CredentialsSecretKey], &credentials)).To(Succeed()) + g.Expect(credentials).To(MatchAllKeys(Keys{ + "a": Equal("b"), + "type": Equal("database"), + })) }).Should(Succeed()) }) When("ServiceInstance credentials are provided", func() { + BeforeEach(func() { + patchMessage.Credentials = &map[string]any{ + "type": "database", + "object": map[string]any{"c": "d"}, + } + }) + + It("awaits credentials secret available condition", func() { + Expect(conditionAwaiter.AwaitConditionCallCount()).To(Equal(1)) + obj, conditionType := conditionAwaiter.AwaitConditionArgsForCall(0) + Expect(obj.GetName()).To(Equal(cfServiceInstance.Name)) + Expect(obj.GetNamespace()).To(Equal(cfServiceInstance.Namespace)) + Expect(conditionType).To(Equal(repositories.CredentialsSecretAvailableCondition)) + }) + + It("updates the creds", func() { + Expect(err).NotTo(HaveOccurred()) + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)).To(Succeed()) + g.Expect(secret.Data).To(MatchAllKeys(Keys{korifiv1alpha1.CredentialsSecretKey: Not(BeEmpty())})) + credentials := map[string]any{} + Expect(json.Unmarshal(secret.Data[korifiv1alpha1.CredentialsSecretKey], &credentials)).To(Succeed()) + Expect(credentials).To(MatchAllKeys(Keys{ + "type": Equal("database"), + "object": MatchAllKeys(Keys{"c": Equal("d")}), + })) + }).Should(Succeed()) + }) + + When("the credentials secret available condition is not met", func() { + BeforeEach(func() { + conditionAwaiter.AwaitConditionReturns(&korifiv1alpha1.CFServiceInstance{}, errors.New("timed-out")) + }) + + It("returns an error", func() { + Expect(err).To(MatchError(ContainSubstring("timed-out"))) + }) + }) + When("the instance credentials modify the type", func() { BeforeEach(func() { - patchMessage.Credentials = &map[string]string{ - "cred-one": "val-one", - "cred-two": "val-two", - "type": "mysql", - "provider": "the-cloud", + patchMessage.Credentials = &map[string]any{ + "type": "mysql", } }) It("disallows changing type", func() { Expect(err).To(MatchError(ContainSubstring("cannot modify credential"))) }) + + When("the current secret does not have a type", func() { + BeforeEach(func() { + Expect(k8s.Patch(ctx, k8sClient, secret, func() { + secret.Data[korifiv1alpha1.CredentialsSecretKey] = []byte{} + })).To(Succeed()) + }) + + It("disallows changing the default type", func() { + Expect(err).To(MatchError(ContainSubstring("cannot modify credential"))) + }) + }) }) When("the instance credentials don't specify a type", func() { BeforeEach(func() { - patchMessage.Credentials = &map[string]string{ + patchMessage.Credentials = &map[string]any{ "cred-one": "val-one", "cred-two": "val-two", } @@ -303,19 +340,21 @@ var _ = Describe("ServiceInstanceRepository", func() { Expect(err).NotTo(HaveOccurred()) Eventually(func(g Gomega) { g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)).To(Succeed()) - g.Expect(secret.Data).To(MatchAllKeys(Keys{ + g.Expect(secret.Data).To(HaveKey(korifiv1alpha1.CredentialsSecretKey)) + credentials := map[string]any{} + g.Expect(json.Unmarshal(secret.Data[korifiv1alpha1.CredentialsSecretKey], &credentials)).To(Succeed()) + g.Expect(credentials).To(MatchAllKeys(Keys{ "type": BeEquivalentTo("database"), "cred-one": BeEquivalentTo("val-one"), "cred-two": BeEquivalentTo("val-two"), })) - g.Expect(secret.Type).To(Equal(corev1.SecretType("servicebinding.io/user-provided"))) }).Should(Succeed()) }) }) When("the instance credentials pass the old type unchanged", func() { BeforeEach(func() { - patchMessage.Credentials = &map[string]string{ + patchMessage.Credentials = &map[string]any{ "type": "database", "cred-one": "val-one", "cred-two": "val-two", @@ -326,30 +365,46 @@ var _ = Describe("ServiceInstanceRepository", func() { Expect(err).NotTo(HaveOccurred()) Eventually(func(g Gomega) { g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)).To(Succeed()) - g.Expect(secret.Data).To(MatchAllKeys(Keys{ + g.Expect(secret.Data).To(HaveKey(korifiv1alpha1.CredentialsSecretKey)) + credentials := map[string]any{} + g.Expect(json.Unmarshal(secret.Data[korifiv1alpha1.CredentialsSecretKey], &credentials)).To(Succeed()) + g.Expect(credentials).To(MatchAllKeys(Keys{ "type": BeEquivalentTo("database"), "cred-one": BeEquivalentTo("val-one"), "cred-two": BeEquivalentTo("val-two"), })) - g.Expect(secret.Type).To(Equal(corev1.SecretType("servicebinding.io/user-provided"))) }).Should(Succeed()) }) }) - }) - When("ServiceInstance credentials are cleared out", func() { - BeforeEach(func() { - patchMessage.Credentials = &map[string]string{} - }) + When("the credentials secret in the spec does not match the credentials secret in the status", func() { + BeforeEach(func() { + Expect(k8sClient.Create(ctx, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: cfServiceInstance.Namespace, + Name: "foo", + }, + Data: map[string][]byte{ + korifiv1alpha1.CredentialsSecretKey: []byte(`{"type":"database"}`), + }, + })).To(Succeed()) + Expect(k8s.Patch(ctx, k8sClient, cfServiceInstance, func() { + cfServiceInstance.Status.Credentials.Name = "foo" + })).To(Succeed()) + }) - It("clears out the credentials", func() { - Expect(err).NotTo(HaveOccurred()) - Eventually(func(g Gomega) { - g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)).To(Succeed()) - g.Expect(secret.Data).To(MatchAllKeys(Keys{ - "type": BeEquivalentTo("database"), - })) - }).Should(Succeed()) + It("updates the secret in the record", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(serviceInstanceRecord.SecretName).To(Equal("foo")) + }) + + It("updates the secret in the spec", func() { + Expect(err).NotTo(HaveOccurred()) + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cfServiceInstance), cfServiceInstance)).To(Succeed()) + g.Expect(cfServiceInstance.Spec.SecretName).To(Equal("foo")) + }).Should(Succeed()) + }) }) }) }) @@ -658,7 +713,7 @@ var _ = Describe("ServiceInstanceRepository", func() { }) }) -func initializeServiceInstanceCreateMessage(serviceInstanceName string, spaceGUID string, tags []string, credentials map[string]string) repositories.CreateServiceInstanceMessage { +func initializeServiceInstanceCreateMessage(serviceInstanceName string, spaceGUID string, tags []string, credentials map[string]any) repositories.CreateServiceInstanceMessage { return repositories.CreateServiceInstanceMessage{ Name: serviceInstanceName, SpaceGUID: spaceGUID, diff --git a/controllers/api/v1alpha1/cfserviceinstance_types.go b/controllers/api/v1alpha1/cfserviceinstance_types.go index 14ecf1679..840f3b8c2 100644 --- a/controllers/api/v1alpha1/cfserviceinstance_types.go +++ b/controllers/api/v1alpha1/cfserviceinstance_types.go @@ -24,7 +24,8 @@ import ( ) const ( - UserProvidedType = "user-provided" + UserProvidedType = "user-provided" + CredentialsSecretKey = "credentials" ) // CFServiceInstanceSpec defines the desired state of CFServiceInstance @@ -53,16 +54,21 @@ type InstanceType string // CFServiceInstanceStatus defines the observed state of CFServiceInstance type CFServiceInstanceStatus struct { - // A reference to the Secret containing the credentials (same as spec.secretName). - // This is required to conform to the Kubernetes Service Bindings spec - // +optional - Binding corev1.LocalObjectReference `json:"binding"` - //+kubebuilder:validation:Optional Conditions []metav1.Condition `json:"conditions,omitempty"` // ObservedGeneration captures the latest generation of the CFServiceInstance that has been reconciled ObservedGeneration int64 `json:"observedGeneration,omitempty"` + + // A reference to the service instance secret containing the credentials + // (derived from spec.secretName). + //+kubebuilder:validation:Optional + Credentials corev1.LocalObjectReference `json:"credentials"` + + // ObservedGeneration captures the latest version of the spec.secretName that has been reconciled + // This will ensure that interested contollers are notified on instance credentials change + //+kubebuilder:validation:Optional + CredentialsObservedVersion string `json:"credentialsObservedVersion,omitempty"` } //+kubebuilder:object:root=true @@ -89,6 +95,10 @@ func (si CFServiceInstance) UniqueValidationErrorMessage() string { return fmt.Sprintf("The service instance name is taken: %s", si.Spec.DisplayName) } +func (si CFServiceInstance) StatusConditions() []metav1.Condition { + return si.Status.Conditions +} + //+kubebuilder:object:root=true // CFServiceInstanceList contains a list of CFServiceInstance diff --git a/controllers/api/v1alpha1/zz_generated.deepcopy.go b/controllers/api/v1alpha1/zz_generated.deepcopy.go index d024caa72..45553c4ab 100644 --- a/controllers/api/v1alpha1/zz_generated.deepcopy.go +++ b/controllers/api/v1alpha1/zz_generated.deepcopy.go @@ -1367,7 +1367,6 @@ func (in *CFServiceInstanceSpec) DeepCopy() *CFServiceInstanceSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *CFServiceInstanceStatus) DeepCopyInto(out *CFServiceInstanceStatus) { *out = *in - out.Binding = in.Binding if in.Conditions != nil { in, out := &in.Conditions, &out.Conditions *out = make([]metav1.Condition, len(*in)) @@ -1375,6 +1374,7 @@ func (in *CFServiceInstanceStatus) DeepCopyInto(out *CFServiceInstanceStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + out.Credentials = in.Credentials } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CFServiceInstanceStatus. diff --git a/controllers/controllers/services/cfservicebinding_controller.go b/controllers/controllers/services/cfservicebinding_controller.go index 089e1c554..2d34837d7 100644 --- a/controllers/controllers/services/cfservicebinding_controller.go +++ b/controllers/controllers/services/cfservicebinding_controller.go @@ -19,16 +19,16 @@ package services import ( "context" "fmt" - "strings" "time" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" + "code.cloudfoundry.org/korifi/controllers/controllers/services/credentials" + "code.cloudfoundry.org/korifi/controllers/controllers/shared" "code.cloudfoundry.org/korifi/tools/k8s" "github.com/go-logr/logr" servicebindingv1beta1 "github.com/servicebinding/runtime/apis/v1beta1" corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -37,6 +37,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/reconcile" ) const ( @@ -44,6 +46,7 @@ const ( VCAPServicesSecretAvailableCondition = "VCAPServicesSecretAvailable" ServiceBindingGUIDLabel = "korifi.cloudfoundry.org/service-binding-guid" ServiceCredentialBindingTypeLabel = "korifi.cloudfoundry.org/service-credential-binding-type" + ServiceBindingSecretTypePrefix = "servicebinding.io/" ) // CFServiceBindingReconciler reconciles a CFServiceBinding object @@ -64,7 +67,35 @@ func NewCFServiceBindingReconciler( func (r *CFServiceBindingReconciler) SetupWithManager(mgr ctrl.Manager) *builder.Builder { return ctrl.NewControllerManagedBy(mgr). - For(&korifiv1alpha1.CFServiceBinding{}) + For(&korifiv1alpha1.CFServiceBinding{}). + Watches( + &korifiv1alpha1.CFServiceInstance{}, + handler.EnqueueRequestsFromMapFunc(r.serviceInstanceToServiceBindings), + ) +} + +func (r *CFServiceBindingReconciler) serviceInstanceToServiceBindings(ctx context.Context, o client.Object) []reconcile.Request { + serviceInstance := o.(*korifiv1alpha1.CFServiceInstance) + + serviceBindings := korifiv1alpha1.CFServiceBindingList{} + if err := r.k8sClient.List(ctx, &serviceBindings, + client.InNamespace(serviceInstance.Namespace), + client.MatchingFields{shared.IndexServiceBindingServiceInstanceGUID: serviceInstance.Name}, + ); err != nil { + return []reconcile.Request{} + } + + requests := []reconcile.Request{} + for _, sb := range serviceBindings.Items { + requests = append(requests, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: sb.Name, + Namespace: sb.Namespace, + }, + }) + } + + return requests } //+kubebuilder:rbac:groups=korifi.cloudfoundry.org,resources=cfservicebindings,verbs=get;list;watch;create;update;patch;delete @@ -77,35 +108,41 @@ func (r *CFServiceBindingReconciler) ReconcileResource(ctx context.Context, cfSe cfServiceBinding.Status.ObservedGeneration = cfServiceBinding.Generation log.V(1).Info("set observed generation", "generation", cfServiceBinding.Status.ObservedGeneration) - instance := new(korifiv1alpha1.CFServiceInstance) - err := r.k8sClient.Get(ctx, types.NamespacedName{Name: cfServiceBinding.Spec.Service.Name, Namespace: cfServiceBinding.Namespace}, instance) + cfServiceInstance := new(korifiv1alpha1.CFServiceInstance) + err := r.k8sClient.Get(ctx, types.NamespacedName{Name: cfServiceBinding.Spec.Service.Name, Namespace: cfServiceBinding.Namespace}, cfServiceInstance) if err != nil { - // Unlike with CFApp cascading delete, CFServiceInstance delete cleans up CFServiceBindings itself as part of finalizing, - // so we do not check for deletion timestamp before returning here. - return r.handleGetError(ctx, err, cfServiceBinding, BindingSecretAvailableCondition, "ServiceInstanceNotFound", "Service instance") + return ctrl.Result{}, err } - err = controllerutil.SetControllerReference(instance, cfServiceBinding, r.scheme) + err = controllerutil.SetOwnerReference(cfServiceInstance, cfServiceBinding, r.scheme) if err != nil { log.Info("error when making the service instance owner of the service binding", "reason", err) return ctrl.Result{}, err } - secret := new(corev1.Secret) - // Note: is there a reason to fetch the secret name from the service instance spec? - err = r.k8sClient.Get(ctx, types.NamespacedName{Name: instance.Spec.SecretName, Namespace: cfServiceBinding.Namespace}, secret) - if err != nil { - return r.handleGetError(ctx, err, cfServiceBinding, BindingSecretAvailableCondition, "SecretNotFound", "Binding secret") + if cfServiceInstance.Status.Credentials.Name == "" { + meta.SetStatusCondition(&cfServiceBinding.Status.Conditions, metav1.Condition{ + Type: BindingSecretAvailableCondition, + Status: metav1.ConditionFalse, + Reason: "CredentialsSecretNotAvailable", + Message: "Service instance credentials not available yet", + ObservedGeneration: cfServiceBinding.Generation, + }) + return ctrl.Result{RequeueAfter: time.Second}, nil } - cfServiceBinding.Status.Binding.Name = instance.Spec.SecretName - meta.SetStatusCondition(&cfServiceBinding.Status.Conditions, metav1.Condition{ - Type: BindingSecretAvailableCondition, - Status: metav1.ConditionTrue, - Reason: "SecretFound", - Message: "", - ObservedGeneration: cfServiceBinding.Generation, - }) + credentialsSecret, err := r.reconcileCredentials(ctx, cfServiceInstance, cfServiceBinding) + if err != nil { + log.Error(err, "failed to reconcile credentials secret") + meta.SetStatusCondition(&cfServiceBinding.Status.Conditions, metav1.Condition{ + Type: BindingSecretAvailableCondition, + Status: metav1.ConditionFalse, + Reason: "FailedReconcilingCredentialsSecret", + Message: err.Error(), + ObservedGeneration: cfServiceBinding.Generation, + }) + return ctrl.Result{}, err + } cfApp := new(korifiv1alpha1.CFApp) err = r.k8sClient.Get(ctx, types.NamespacedName{Name: cfServiceBinding.Spec.AppRef.Name, Namespace: cfServiceBinding.Namespace}, cfApp) @@ -142,7 +179,7 @@ func (r *CFServiceBindingReconciler) ReconcileResource(ctx context.Context, cfSe }, } - desiredSBServiceBinding := generateDesiredServiceBinding(&actualSBServiceBinding, cfServiceBinding, cfApp, secret) + desiredSBServiceBinding := generateDesiredServiceBinding(&actualSBServiceBinding, cfServiceBinding, cfApp, credentialsSecret) _, err = controllerutil.CreateOrPatch(ctx, r.k8sClient, &actualSBServiceBinding, sbServiceBindingMutateFn(&actualSBServiceBinding, desiredSBServiceBinding)) if err != nil { @@ -153,27 +190,81 @@ func (r *CFServiceBindingReconciler) ReconcileResource(ctx context.Context, cfSe return ctrl.Result{}, nil } -func (r *CFServiceBindingReconciler) handleGetError(ctx context.Context, err error, cfServiceBinding *korifiv1alpha1.CFServiceBinding, conditionType, notFoundReason, objectType string) (ctrl.Result, error) { - cfServiceBinding.Status.Binding = corev1.LocalObjectReference{} - if apierrors.IsNotFound(err) { - meta.SetStatusCondition(&cfServiceBinding.Status.Conditions, metav1.Condition{ - Type: conditionType, - Status: metav1.ConditionFalse, - Reason: notFoundReason, - Message: objectType + " does not exist", - ObservedGeneration: cfServiceBinding.Generation, - }) - return ctrl.Result{RequeueAfter: 2 * time.Second}, nil +func (r *CFServiceBindingReconciler) reconcileCredentials(ctx context.Context, cfServiceInstance *korifiv1alpha1.CFServiceInstance, cfServiceBinding *korifiv1alpha1.CFServiceBinding) (*corev1.Secret, error) { + if isLegacyServiceBinding(cfServiceBinding, cfServiceInstance) { + bindingSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: cfServiceBinding.Status.Binding.Name, + Namespace: cfServiceBinding.Namespace, + }, + } + + // For legacy sevice bindings we want to keep the binding secret + // unchanged in order to avoid unexpected app restarts. See ADR 16 for more details. + err := r.k8sClient.Get(ctx, client.ObjectKeyFromObject(bindingSecret), bindingSecret) + if err != nil { + return nil, err + } + + return bindingSecret, nil + } + + credentialsSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: cfServiceInstance.Namespace, + Name: cfServiceInstance.Status.Credentials.Name, + }, + } + err := r.k8sClient.Get(ctx, client.ObjectKeyFromObject(credentialsSecret), credentialsSecret) + if err != nil { + return nil, fmt.Errorf("failed to get service instance credentials secret %q: %w", cfServiceInstance.Status.Credentials.Name, err) + } + + bindingSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: cfServiceBinding.Name, + Namespace: cfServiceBinding.Namespace, + }, + } + + _, err = controllerutil.CreateOrPatch(ctx, r.k8sClient, bindingSecret, func() error { + bindingSecret.Type, err = credentials.GetBindingSecretType(credentialsSecret) + if err != nil { + return err + } + bindingSecret.Data, err = credentials.GetBindingSecretData(credentialsSecret) + if err != nil { + return err + } + + return controllerutil.SetControllerReference(cfServiceBinding, bindingSecret, r.scheme) + }) + if err != nil { + return nil, fmt.Errorf("failed to create binding secret: %w", err) } + cfServiceBinding.Status.Binding.Name = bindingSecret.Name meta.SetStatusCondition(&cfServiceBinding.Status.Conditions, metav1.Condition{ - Type: conditionType, - Status: metav1.ConditionFalse, - Reason: "UnknownError", - Message: "Error occurred while fetching " + strings.ToLower(objectType) + ": " + err.Error(), + Type: BindingSecretAvailableCondition, + Status: metav1.ConditionTrue, + Reason: "SecretAvailable", + Message: "", ObservedGeneration: cfServiceBinding.Generation, }) - return ctrl.Result{}, err + + return bindingSecret, nil +} + +func isLegacyServiceBinding(cfServiceBinding *korifiv1alpha1.CFServiceBinding, cfServiceInstance *korifiv1alpha1.CFServiceInstance) bool { + if cfServiceBinding.Status.Binding.Name == "" { + return false + } + + // When reconciling existing legacy service bindings we make + // use of the fact that the service binding used to reference + // the secret of the sevice instance that shares the sevice + // instance name. See ADR 16 for more datails. + return cfServiceInstance.Name == cfServiceBinding.Status.Binding.Name && cfServiceInstance.Spec.SecretName == cfServiceBinding.Status.Binding.Name } func sbServiceBindingMutateFn(actualSBServiceBinding, desiredSBServiceBinding *servicebindingv1beta1.ServiceBinding) controllerutil.MutateFn { diff --git a/controllers/controllers/services/cfservicebinding_controller_test.go b/controllers/controllers/services/cfservicebinding_controller_test.go index 228cb840c..4d8f99626 100644 --- a/controllers/controllers/services/cfservicebinding_controller_test.go +++ b/controllers/controllers/services/cfservicebinding_controller_test.go @@ -1,7 +1,7 @@ package services_test import ( - "context" + "encoding/json" "fmt" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" @@ -9,6 +9,7 @@ import ( . "code.cloudfoundry.org/korifi/controllers/controllers/workloads/testutils" "code.cloudfoundry.org/korifi/tools/k8s" + "github.com/google/uuid" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" . "github.com/onsi/gomega/gstruct" @@ -26,26 +27,25 @@ var _ = Describe("CFServiceBinding", func() { cfAppGUID string desiredCFApp *korifiv1alpha1.CFApp cfServiceInstance *korifiv1alpha1.CFServiceInstance - secret *corev1.Secret - secretType string - secretProvider string + credentialsSecret *corev1.Secret cfServiceBinding *korifiv1alpha1.CFServiceBinding cfServiceBindingGUID string + credentialsData map[string]any ) BeforeEach(func() { namespace = BuildNamespaceObject(GenerateGUID()) Expect( - adminClient.Create(context.Background(), namespace), + adminClient.Create(ctx, namespace), ).To(Succeed()) cfAppGUID = GenerateGUID() desiredCFApp = BuildCFAppCRObject(cfAppGUID, namespace.Name) Expect( - adminClient.Create(context.Background(), desiredCFApp), + adminClient.Create(ctx, desiredCFApp), ).To(Succeed()) - Expect(k8s.Patch(context.Background(), adminClient, desiredCFApp, func() { + Expect(k8s.Patch(ctx, adminClient, desiredCFApp, func() { desiredCFApp.Status = korifiv1alpha1.CFAppStatus{ Conditions: nil, VCAPServicesSecretName: "foo", @@ -58,37 +58,32 @@ var _ = Describe("CFServiceBinding", func() { }) })).To(Succeed()) - secretType = "mongodb" - secretProvider = "cloud-aws" - secret = &corev1.Secret{ + credentialsData = map[string]any{ + "type": "my-type", + "provider": "my-provider", + "obj": map[string]any{ + "foo": "bar", + }, + } + credentialsSecret = &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "service-instance-secret", + Name: uuid.NewString(), Namespace: namespace.Name, }, - StringData: map[string]string{ - "type": secretType, - "provider": secretProvider, - }, } - Expect( - adminClient.Create(context.Background(), secret), - ).To(Succeed()) cfServiceInstance = &korifiv1alpha1.CFServiceInstance{ ObjectMeta: metav1.ObjectMeta{ - Name: "service-instance-guid", + Name: uuid.NewString(), Namespace: namespace.Name, }, Spec: korifiv1alpha1.CFServiceInstanceSpec{ DisplayName: "mongodb-service-instance-name", - SecretName: secret.Name, + SecretName: credentialsSecret.Name, Type: "user-provided", Tags: []string{}, }, } - Expect( - adminClient.Create(context.Background(), cfServiceInstance), - ).To(Succeed()) cfServiceBindingGUID = GenerateGUID() cfServiceBinding = &korifiv1alpha1.CFServiceBinding{ @@ -109,202 +104,244 @@ var _ = Describe("CFServiceBinding", func() { } }) - AfterEach(func() { - Expect(adminClient.Delete(context.Background(), namespace)).To(Succeed()) - }) - JustBeforeEach(func() { - Expect( - adminClient.Create(context.Background(), cfServiceBinding), - ).To(Succeed()) + Expect(adminClient.Create(ctx, cfServiceInstance)).To(Succeed()) + Expect(adminClient.Create(ctx, cfServiceBinding)).To(Succeed()) }) - It("makes the service instance owner of the service binding", func() { + It("sets binding secret not available condition", func() { Eventually(func(g Gomega) { - g.Expect(adminClient.Get(context.Background(), client.ObjectKeyFromObject(cfServiceBinding), cfServiceBinding)).To(Succeed()) - g.Expect(cfServiceBinding.GetOwnerReferences()).To(ConsistOf(HaveField("Name", cfServiceInstance.Name))) + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(cfServiceBinding), cfServiceBinding)).To(Succeed()) + g.Expect(cfServiceBinding.Status.Conditions).To(ContainElement(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(services.BindingSecretAvailableCondition), + "Status": Equal(metav1.ConditionFalse), + "Reason": Equal("CredentialsSecretNotAvailable"), + "ObservedGeneration": Equal(cfServiceBinding.Generation), + }))) }).Should(Succeed()) }) - It("resolves the secretName and updates the CFServiceBinding status", func() { - Eventually(func(g Gomega) { - updatedCFServiceBinding := new(korifiv1alpha1.CFServiceBinding) - g.Expect(adminClient.Get(context.Background(), client.ObjectKeyFromObject(cfServiceBinding), updatedCFServiceBinding)).To(Succeed()) - g.Expect(updatedCFServiceBinding.Status).To(MatchFields(IgnoreExtras, Fields{ - "Binding": MatchFields(IgnoreExtras, Fields{"Name": Equal(secret.Name)}), - "Conditions": ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal("BindingSecretAvailable"), - "Status": Equal(metav1.ConditionTrue), - "Reason": Equal("SecretFound"), - "Message": Equal(""), - })), - })) - }).Should(Succeed()) - }) + When("the credentials secret is available", func() { + BeforeEach(func() { + credentialsBytes, err := json.Marshal(credentialsData) + Expect(err).NotTo(HaveOccurred()) + credentialsSecret.Data = map[string][]byte{ + korifiv1alpha1.CredentialsSecretKey: credentialsBytes, + } + }) - It("creates a servicebinding.io ServiceBinding", func() { - Eventually(func(g Gomega) { - sbServiceBinding := servicebindingv1beta1.ServiceBinding{} - g.Expect(adminClient.Get(context.Background(), types.NamespacedName{Name: fmt.Sprintf("cf-binding-%s", cfServiceBindingGUID), Namespace: namespace.Name}, &sbServiceBinding)).To(Succeed()) - g.Expect(sbServiceBinding).To(MatchFields(IgnoreExtras, Fields{ - "ObjectMeta": MatchFields(IgnoreExtras, Fields{ - "Name": Equal(fmt.Sprintf("cf-binding-%s", cfServiceBindingGUID)), - "Namespace": Equal(namespace.Name), - "Labels": MatchKeys(IgnoreExtras, Keys{ - services.ServiceBindingGUIDLabel: Equal(cfServiceBindingGUID), - korifiv1alpha1.CFAppGUIDLabelKey: Equal(cfAppGUID), - services.ServiceCredentialBindingTypeLabel: Equal("app"), - }), - "OwnerReferences": ContainElement(MatchFields(IgnoreExtras, Fields{ - "APIVersion": Equal("korifi.cloudfoundry.org/v1alpha1"), - "Kind": Equal("CFServiceBinding"), - "Name": Equal(cfServiceBindingGUID), - })), - }), - "Spec": MatchFields(IgnoreExtras, Fields{ - "Name": Equal(secret.Name), - "Type": Equal(secretType), - "Provider": Equal(secretProvider), - "Workload": MatchFields(IgnoreExtras, Fields{ - "APIVersion": Equal("apps/v1"), - "Kind": Equal("StatefulSet"), - "Selector": PointTo(MatchFields(IgnoreExtras, Fields{ - "MatchLabels": MatchKeys(IgnoreExtras, Keys{ - korifiv1alpha1.CFAppGUIDLabelKey: Equal(cfAppGUID), - }), - })), - }), - "Service": MatchFields(IgnoreExtras, Fields{ - "APIVersion": Equal("korifi.cloudfoundry.org/v1alpha1"), - "Kind": Equal("CFServiceBinding"), - "Name": Equal(cfServiceBindingGUID), - }), - }), - })) - }).Should(Succeed()) - }) + JustBeforeEach(func() { + Expect(adminClient.Create(ctx, credentialsSecret)).To(Succeed()) + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(cfServiceInstance), cfServiceInstance)).To(Succeed()) + g.Expect(meta.IsStatusConditionTrue(cfServiceInstance.Status.Conditions, services.CredentialsSecretAvailableCondition)).To(BeTrue()) + }).Should(Succeed()) + }) - It("sets the ObservedGeneration status field", func() { - Eventually(func(g Gomega) { - g.Expect(adminClient.Get(context.Background(), client.ObjectKeyFromObject(cfServiceBinding), cfServiceBinding)).To(Succeed()) - g.Expect(cfServiceBinding.Status.ObservedGeneration).To(Equal(cfServiceBinding.Generation)) - }).Should(Succeed()) - }) + It("sets an owner reference from the service instance to the service binding", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(cfServiceBinding), cfServiceBinding)).To(Succeed()) + fmt.Fprintf(GinkgoWriter, "cfServiceInstance = %+v\n", cfServiceInstance) + + g.Expect(cfServiceBinding.OwnerReferences).To(ConsistOf(MatchFields(IgnoreExtras, Fields{ + "Name": Equal(cfServiceInstance.Name), + }))) + }).Should(Succeed()) + }) + + It("reconciles the service instance credentials secret", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(cfServiceBinding), cfServiceBinding)).To(Succeed()) + g.Expect(meta.IsStatusConditionTrue(cfServiceBinding.Status.Conditions, services.BindingSecretAvailableCondition)).To(BeTrue()) + }).Should(Succeed()) - When("the CFServiceBinding has a displayName set", func() { - var bindingName string + Expect(cfServiceBinding.Status.Binding.Name).NotTo(BeEmpty()) - BeforeEach(func() { - cfServiceBindingGUID = GenerateGUID() - bindingName = "a-custom-binding-name" - cfServiceBinding = &korifiv1alpha1.CFServiceBinding{ + bindingSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: cfServiceBindingGUID, - Namespace: namespace.Name, - }, - Spec: korifiv1alpha1.CFServiceBindingSpec{ - DisplayName: &bindingName, - Service: corev1.ObjectReference{ - Kind: "ServiceInstance", - Name: cfServiceInstance.Name, - APIVersion: "korifi.cloudfoundry.org/v1alpha1", - }, - AppRef: corev1.LocalObjectReference{ - Name: cfAppGUID, - }, + Namespace: cfServiceBinding.Namespace, + Name: cfServiceBinding.Status.Binding.Name, }, } + Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(bindingSecret), bindingSecret)).To(Succeed()) + Expect(bindingSecret.Type).To(BeEquivalentTo(services.ServiceBindingSecretTypePrefix + "my-type")) + Expect(bindingSecret.Data).To(MatchAllKeys(Keys{ + "type": Equal([]byte("my-type")), + "provider": Equal([]byte("my-provider")), + "obj": Equal([]byte(`{"foo":"bar"}`)), + })) }) - It("sets the displayName as the name on the servicebinding.io ServiceBinding", func() { + It("creates a servicebinding.io ServiceBinding", func() { Eventually(func(g Gomega) { sbServiceBinding := servicebindingv1beta1.ServiceBinding{} - g.Expect(adminClient.Get(context.Background(), types.NamespacedName{Name: fmt.Sprintf("cf-binding-%s", cfServiceBindingGUID), Namespace: namespace.Name}, &sbServiceBinding)).To(Succeed()) + g.Expect(adminClient.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("cf-binding-%s", cfServiceBindingGUID), Namespace: namespace.Name}, &sbServiceBinding)).To(Succeed()) g.Expect(sbServiceBinding).To(MatchFields(IgnoreExtras, Fields{ + "ObjectMeta": MatchFields(IgnoreExtras, Fields{ + "Name": Equal(fmt.Sprintf("cf-binding-%s", cfServiceBindingGUID)), + "Namespace": Equal(namespace.Name), + "Labels": MatchKeys(IgnoreExtras, Keys{ + services.ServiceBindingGUIDLabel: Equal(cfServiceBindingGUID), + korifiv1alpha1.CFAppGUIDLabelKey: Equal(cfAppGUID), + services.ServiceCredentialBindingTypeLabel: Equal("app"), + }), + "OwnerReferences": ContainElement(MatchFields(IgnoreExtras, Fields{ + "APIVersion": Equal("korifi.cloudfoundry.org/v1alpha1"), + "Kind": Equal("CFServiceBinding"), + "Name": Equal(cfServiceBindingGUID), + })), + }), "Spec": MatchFields(IgnoreExtras, Fields{ - "Name": Equal(bindingName), + "Name": Equal(cfServiceBinding.Name), + "Type": Equal("my-type"), + "Provider": Equal("my-provider"), + "Workload": MatchFields(IgnoreExtras, Fields{ + "APIVersion": Equal("apps/v1"), + "Kind": Equal("StatefulSet"), + "Selector": PointTo(MatchFields(IgnoreExtras, Fields{ + "MatchLabels": MatchKeys(IgnoreExtras, Keys{ + korifiv1alpha1.CFAppGUIDLabelKey: Equal(cfAppGUID), + }), + })), + }), + "Service": MatchFields(IgnoreExtras, Fields{ + "APIVersion": Equal("korifi.cloudfoundry.org/v1alpha1"), + "Kind": Equal("CFServiceBinding"), + "Name": Equal(cfServiceBindingGUID), + }), }), })) }).Should(Succeed()) }) - }) - When("the referenced secret does not exist", func() { - var otherSecret *corev1.Secret + It("sets the ObservedGeneration status field", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(cfServiceBinding), cfServiceBinding)).To(Succeed()) + g.Expect(cfServiceBinding.Status.ObservedGeneration).To(Equal(cfServiceBinding.Generation)) + }).Should(Succeed()) + }) - BeforeEach(func() { - ctx := context.Background() - otherSecret = &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "other-secret-name", - Namespace: namespace.Name, - }, - } - instance := &korifiv1alpha1.CFServiceInstance{ - ObjectMeta: metav1.ObjectMeta{ - Name: "other-service-instance-guid", - Namespace: namespace.Name, - }, - Spec: korifiv1alpha1.CFServiceInstanceSpec{ - DisplayName: "other-service-instance-name", - SecretName: otherSecret.Name, - Type: "user-provided", - Tags: []string{}, - }, - } - Expect( - adminClient.Create(ctx, instance), - ).To(Succeed()) + When("the CFServiceBinding has a displayName set", func() { + var bindingName string + + BeforeEach(func() { + cfServiceBindingGUID = GenerateGUID() + bindingName = "a-custom-binding-name" + cfServiceBinding = &korifiv1alpha1.CFServiceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: cfServiceBindingGUID, + Namespace: namespace.Name, + }, + Spec: korifiv1alpha1.CFServiceBindingSpec{ + DisplayName: &bindingName, + Service: corev1.ObjectReference{ + Kind: "ServiceInstance", + Name: cfServiceInstance.Name, + APIVersion: "korifi.cloudfoundry.org/v1alpha1", + }, + AppRef: corev1.LocalObjectReference{ + Name: cfAppGUID, + }, + }, + } + }) - cfServiceBinding.Spec.Service.Name = instance.Name + It("sets the displayName as the name on the servicebinding.io ServiceBinding", func() { + Eventually(func(g Gomega) { + sbServiceBinding := servicebindingv1beta1.ServiceBinding{} + g.Expect(adminClient.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("cf-binding-%s", cfServiceBindingGUID), Namespace: namespace.Name}, &sbServiceBinding)).To(Succeed()) + g.Expect(sbServiceBinding).To(MatchFields(IgnoreExtras, Fields{ + "Spec": MatchFields(IgnoreExtras, Fields{ + "Name": Equal(bindingName), + }), + })) + }).Should(Succeed()) + }) }) - It("updates the CFServiceBinding status", func() { - Eventually(func(g Gomega) { - updatedCFServiceBinding := new(korifiv1alpha1.CFServiceBinding) - g.Expect(adminClient.Get(context.Background(), client.ObjectKeyFromObject(cfServiceBinding), updatedCFServiceBinding)).To(Succeed()) - g.Expect(updatedCFServiceBinding.Status).To(MatchFields(IgnoreExtras, Fields{ - "Binding": MatchFields(IgnoreExtras, Fields{"Name": Equal("")}), - "Conditions": ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal("BindingSecretAvailable"), - "Status": Equal(metav1.ConditionFalse), - "Reason": Equal("SecretNotFound"), - "Message": Equal("Binding secret does not exist"), - })), - })) - }).Should(Succeed()) + When("the credentials secret does not exist", func() { + BeforeEach(func() { + cfServiceBinding.Spec.Service.Name = "does-not-exist" + }) + + It("does not set binding name in the service binding status", func() { + Consistently(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(cfServiceBinding), cfServiceBinding)).To(Succeed()) + g.Expect(cfServiceBinding.Status.Binding.Name).To(BeEmpty()) + }).Should(Succeed()) + }) }) - When("the referenced secret is created afterwards", func() { + When("the credentials change", func() { JustBeforeEach(func() { Eventually(func(g Gomega) { - updatedCFServiceBinding := new(korifiv1alpha1.CFServiceBinding) - g.Expect(adminClient.Get(context.Background(), client.ObjectKeyFromObject(cfServiceBinding), updatedCFServiceBinding)).To(Succeed()) - g.Expect(updatedCFServiceBinding.Status).To(MatchFields(IgnoreExtras, Fields{ - "Conditions": ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal("BindingSecretAvailable"), - "Status": Equal(metav1.ConditionFalse), - })), + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(cfServiceBinding), cfServiceBinding)).To(Succeed()) + g.Expect(cfServiceBinding.Status.Binding.Name).To(Equal(cfServiceBinding.Name)) + }).Should(Succeed()) + Expect(k8s.Patch(ctx, adminClient, credentialsSecret, func() { + credentialsSecret.Data = map[string][]byte{ + korifiv1alpha1.CredentialsSecretKey: []byte(`{"type":"my-type","provider": "your-provider"}`), + } + })).To(Succeed()) + }) + + It("updates the binding secret", func() { + Eventually(func(g Gomega) { + bindingSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: cfServiceBinding.Namespace, + Name: cfServiceBinding.Name, + }, + } + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(bindingSecret), bindingSecret)).To(Succeed()) + g.Expect(bindingSecret.Data).To(MatchAllKeys(Keys{ + "type": Equal([]byte("my-type")), + "provider": Equal([]byte("your-provider")), })) }).Should(Succeed()) + }) + }) - Expect(adminClient.Create(context.Background(), otherSecret)).To(Succeed()) + When("the service binding references the legacy service instance creadentials secret", func() { + BeforeEach(func() { + credentialsSecret.Name = cfServiceInstance.Name + cfServiceInstance.Spec.SecretName = cfServiceInstance.Name }) - It("resolves the secretName and updates the CFServiceBinding status", func() { + JustBeforeEach(func() { Eventually(func(g Gomega) { - updatedCFServiceBinding := new(korifiv1alpha1.CFServiceBinding) - g.Expect(adminClient.Get(context.Background(), client.ObjectKeyFromObject(cfServiceBinding), updatedCFServiceBinding)).To(Succeed()) - g.Expect(updatedCFServiceBinding.Status).To(MatchFields(IgnoreExtras, Fields{ - "Binding": MatchFields(IgnoreExtras, Fields{"Name": Equal(otherSecret.Name)}), - "Conditions": ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal("BindingSecretAvailable"), - "Status": Equal(metav1.ConditionTrue), - "Reason": Equal("SecretFound"), - "Message": Equal(""), - })), - })) + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(cfServiceBinding), cfServiceBinding)).To(Succeed()) + g.Expect(cfServiceBinding.Status.Binding.Name).NotTo(BeEmpty()) }).Should(Succeed()) + Expect(k8s.Patch(ctx, adminClient, cfServiceBinding, func() { + cfServiceBinding.Status.Binding.Name = cfServiceInstance.Name + })).To(Succeed()) + }) + + It("does not create a new binding secret", func() { + Consistently(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(cfServiceBinding), cfServiceBinding)).To(Succeed()) + g.Expect(cfServiceBinding.Status.Binding.Name).To(Equal(cfServiceInstance.Name)) + }).Should(Succeed()) + }) + + When("the referenced legacy binding secret cannot be found", func() { + JustBeforeEach(func() { + Expect(adminClient.Delete(ctx, credentialsSecret)).To(Succeed()) + }) + + It("sets credentials secret not available condition", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(cfServiceBinding), cfServiceBinding)).To(Succeed()) + + g.Expect(cfServiceBinding.Status.Conditions).To(ContainElement(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(services.BindingSecretAvailableCondition), + "Status": Equal(metav1.ConditionFalse), + "Reason": Equal("FailedReconcilingCredentialsSecret"), + "ObservedGeneration": Equal(cfServiceBinding.Generation), + }))) + }).Should(Succeed()) + }) }) }) }) diff --git a/controllers/controllers/services/cfserviceinstance_controller.go b/controllers/controllers/services/cfserviceinstance_controller.go index da050dd38..16ebbbffc 100644 --- a/controllers/controllers/services/cfserviceinstance_controller.go +++ b/controllers/controllers/services/cfserviceinstance_controller.go @@ -18,12 +18,16 @@ package services import ( "context" + "encoding/json" + "strings" "time" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" + "code.cloudfoundry.org/korifi/controllers/controllers/shared" "code.cloudfoundry.org/korifi/tools/k8s" "github.com/go-logr/logr" + "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" @@ -33,8 +37,13 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/reconcile" ) +const CredentialsSecretAvailableCondition = "CredentialSecretAvailable" + // CFServiceInstanceReconciler reconciles a CFServiceInstance object type CFServiceInstanceReconciler struct { k8sClient client.Client @@ -53,7 +62,34 @@ func NewCFServiceInstanceReconciler( func (r *CFServiceInstanceReconciler) SetupWithManager(mgr ctrl.Manager) *builder.Builder { return ctrl.NewControllerManagedBy(mgr). - For(&korifiv1alpha1.CFServiceInstance{}) + For(&korifiv1alpha1.CFServiceInstance{}). + Watches( + &corev1.Secret{}, + handler.EnqueueRequestsFromMapFunc(r.secretToServiceInstance), + ) +} + +func (r *CFServiceInstanceReconciler) secretToServiceInstance(ctx context.Context, o client.Object) []reconcile.Request { + serviceInstances := korifiv1alpha1.CFServiceInstanceList{} + if err := r.k8sClient.List(ctx, &serviceInstances, + client.InNamespace(o.GetNamespace()), + client.MatchingFields{ + shared.IndexServiceInstanceCredentialsSecretName: o.GetName(), + }); err != nil { + return []reconcile.Request{} + } + + requests := []reconcile.Request{} + for _, si := range serviceInstances.Items { + requests = append(requests, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: si.Name, + Namespace: si.Namespace, + }, + }) + } + + return requests } //+kubebuilder:rbac:groups=korifi.cloudfoundry.org,resources=cfserviceinstances,verbs=get;list;watch;create;update;patch;delete @@ -66,56 +102,107 @@ func (r *CFServiceInstanceReconciler) ReconcileResource(ctx context.Context, cfS cfServiceInstance.Status.ObservedGeneration = cfServiceInstance.Generation log.V(1).Info("set observed generation", "generation", cfServiceInstance.Status.ObservedGeneration) - secret := new(corev1.Secret) - err := r.k8sClient.Get(ctx, types.NamespacedName{Name: cfServiceInstance.Spec.SecretName, Namespace: cfServiceInstance.Namespace}, secret) + credentialsSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: cfServiceInstance.Namespace, + Name: cfServiceInstance.Spec.SecretName, + }, + } + err := r.k8sClient.Get(ctx, client.ObjectKeyFromObject(credentialsSecret), credentialsSecret) if err != nil { + meta.SetStatusCondition(&cfServiceInstance.Status.Conditions, metav1.Condition{ + Type: CredentialsSecretAvailableCondition, + Status: metav1.ConditionFalse, + Reason: "CredentialsSecretNotAvailable", + Message: "Error occurred while fetching secret: " + err.Error(), + ObservedGeneration: cfServiceInstance.Generation, + }) if apierrors.IsNotFound(err) { - cfServiceInstance.Status = bindSecretUnavailableStatus(cfServiceInstance, "SecretNotFound", "Binding secret does not exist") return ctrl.Result{RequeueAfter: 2 * time.Second}, nil } - - log.Info("failed to get secret", "reason", err) - cfServiceInstance.Status = bindSecretUnavailableStatus(cfServiceInstance, "UnknownError", "Error occurred while fetching secret: "+err.Error()) return ctrl.Result{}, err } - cfServiceInstance.Status = bindSecretAvailableStatus(cfServiceInstance) - return ctrl.Result{}, nil -} + credentialsSecret, err = r.reconcileCredentials(ctx, credentialsSecret, cfServiceInstance) + if err != nil { + log.Error(err, "failed to reconcile credentials secret") + meta.SetStatusCondition(&cfServiceInstance.Status.Conditions, metav1.Condition{ + Type: CredentialsSecretAvailableCondition, + Status: metav1.ConditionFalse, + Reason: "FailedReconcilingCredentialsSecret", + Message: err.Error(), + ObservedGeneration: cfServiceInstance.Generation, + }) + return ctrl.Result{}, err + } -func bindSecretAvailableStatus(cfServiceInstance *korifiv1alpha1.CFServiceInstance) korifiv1alpha1.CFServiceInstanceStatus { - status := korifiv1alpha1.CFServiceInstanceStatus{ - Binding: corev1.LocalObjectReference{ - Name: cfServiceInstance.Spec.SecretName, - }, - Conditions: cfServiceInstance.Status.Conditions, - ObservedGeneration: cfServiceInstance.Status.ObservedGeneration, + if err = r.validateCredentials(ctx, credentialsSecret); err != nil { + meta.SetStatusCondition(&cfServiceInstance.Status.Conditions, metav1.Condition{ + Type: CredentialsSecretAvailableCondition, + Status: metav1.ConditionFalse, + Reason: "SecretInvalid", + Message: err.Error(), + ObservedGeneration: cfServiceInstance.Generation, + }) + return ctrl.Result{}, err } - meta.SetStatusCondition(&status.Conditions, metav1.Condition{ - Type: BindingSecretAvailableCondition, + log.V(1).Info("credentials secret", "name", credentialsSecret.Name, "version", credentialsSecret.ResourceVersion) + meta.SetStatusCondition(&cfServiceInstance.Status.Conditions, metav1.Condition{ + Type: CredentialsSecretAvailableCondition, Status: metav1.ConditionTrue, Reason: "SecretFound", ObservedGeneration: cfServiceInstance.Generation, }) - - return status + cfServiceInstance.Status.Credentials = corev1.LocalObjectReference{Name: credentialsSecret.Name} + cfServiceInstance.Status.CredentialsObservedVersion = credentialsSecret.ResourceVersion + return ctrl.Result{}, nil } -func bindSecretUnavailableStatus(cfServiceInstance *korifiv1alpha1.CFServiceInstance, reason, message string) korifiv1alpha1.CFServiceInstanceStatus { - status := korifiv1alpha1.CFServiceInstanceStatus{ - Binding: corev1.LocalObjectReference{}, - Conditions: cfServiceInstance.Status.Conditions, - ObservedGeneration: cfServiceInstance.Status.ObservedGeneration, +func (r *CFServiceInstanceReconciler) reconcileCredentials(ctx context.Context, credentialsSecret *corev1.Secret, cfServiceInstance *korifiv1alpha1.CFServiceInstance) (*corev1.Secret, error) { + if !strings.HasPrefix(string(credentialsSecret.Type), ServiceBindingSecretTypePrefix) { + return credentialsSecret, nil } - meta.SetStatusCondition(&status.Conditions, metav1.Condition{ - Type: BindingSecretAvailableCondition, - Status: metav1.ConditionFalse, - Reason: reason, - Message: message, - ObservedGeneration: cfServiceInstance.Generation, + log := logr.FromContextOrDiscard(ctx) + + log.Info("migrating legacy secret", "legacy-secret-name", credentialsSecret.Name) + migratedSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: cfServiceInstance.Name + "-migrated", + Namespace: cfServiceInstance.Namespace, + }, + } + _, err := controllerutil.CreateOrPatch(ctx, r.k8sClient, migratedSecret, func() error { + migratedSecret.Type = corev1.SecretTypeOpaque + data := map[string]any{} + for k, v := range credentialsSecret.Data { + data[k] = string(v) + } + + dataBytes, err := json.Marshal(data) + if err != nil { + log.Error(err, "failed to marshal legacy credentials secret data") + return err + } + + migratedSecret.Data = map[string][]byte{ + korifiv1alpha1.CredentialsSecretKey: dataBytes, + } + return controllerutil.SetOwnerReference(cfServiceInstance, migratedSecret, r.scheme) }) + if err != nil { + log.Error(err, "failed to create migrated credentials secret") + return nil, err + } + + return migratedSecret, nil +} - return status +func (r *CFServiceInstanceReconciler) validateCredentials(ctx context.Context, credentialsSecret *corev1.Secret) error { + return errors.Wrapf( + json.Unmarshal(credentialsSecret.Data[korifiv1alpha1.CredentialsSecretKey], &map[string]any{}), + "invalid credentials secret %q", + credentialsSecret.Name, + ) } diff --git a/controllers/controllers/services/cfserviceinstance_controller_test.go b/controllers/controllers/services/cfserviceinstance_controller_test.go index bd63c6639..29e3c0884 100644 --- a/controllers/controllers/services/cfserviceinstance_controller_test.go +++ b/controllers/controllers/services/cfserviceinstance_controller_test.go @@ -2,22 +2,26 @@ package services_test import ( "context" + "encoding/json" . "github.com/onsi/gomega/gstruct" "sigs.k8s.io/controller-runtime/pkg/client" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" + "code.cloudfoundry.org/korifi/controllers/controllers/services" . "code.cloudfoundry.org/korifi/controllers/controllers/workloads/testutils" + "code.cloudfoundry.org/korifi/tools/k8s" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) var _ = Describe("CFServiceInstance", func() { var ( namespace *corev1.Namespace - secret *corev1.Secret + credentialsSecret *corev1.Secret cfServiceInstance *korifiv1alpha1.CFServiceInstance ) @@ -27,16 +31,16 @@ var _ = Describe("CFServiceInstance", func() { adminClient.Create(context.Background(), namespace), ).To(Succeed()) - secret = &corev1.Secret{ + credentialsSecret = &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "secret-name", Namespace: namespace.Name, }, - StringData: map[string]string{"foo": "bar"}, + Data: map[string][]byte{ + korifiv1alpha1.CredentialsSecretKey: []byte(`{"foo": "bar"}`), + }, } - Expect(adminClient.Create(ctx, secret)).To(Succeed()) - cfServiceInstance = &korifiv1alpha1.CFServiceInstance{ ObjectMeta: metav1.ObjectMeta{ Name: "service-instance-guid", @@ -46,7 +50,7 @@ var _ = Describe("CFServiceInstance", func() { DisplayName: "service-instance-name", Type: "user-provided", Tags: []string{}, - SecretName: secret.Name, + SecretName: credentialsSecret.Name, }, } }) @@ -56,22 +60,23 @@ var _ = Describe("CFServiceInstance", func() { }) JustBeforeEach(func() { + Expect(adminClient.Create(ctx, credentialsSecret)).To(Succeed()) Expect(adminClient.Create(context.Background(), cfServiceInstance)).To(Succeed()) }) - It("sets the BindingSecretAvailable condition to true in the CFServiceInstance status", func() { + It("sets the CredentialsSecretAvailable condition to true in the CFServiceInstance status", func() { Eventually(func(g Gomega) { - updatedCFServiceInstance := new(korifiv1alpha1.CFServiceInstance) - serviceInstanceNamespacedName := client.ObjectKeyFromObject(cfServiceInstance) - g.Expect(adminClient.Get(context.Background(), serviceInstanceNamespacedName, updatedCFServiceInstance)).To(Succeed()) + g.Expect(adminClient.Get(context.Background(), client.ObjectKeyFromObject(cfServiceInstance), cfServiceInstance)).To(Succeed()) - g.Expect(updatedCFServiceInstance.Status.Binding.Name).To(Equal("secret-name")) - g.Expect(updatedCFServiceInstance.Status.Conditions).To(ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal("BindingSecretAvailable"), - "Status": Equal(metav1.ConditionTrue), - "Reason": Equal("SecretFound"), - "Message": Equal(""), + g.Expect(cfServiceInstance.Status.Conditions).To(ContainElement(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(services.CredentialsSecretAvailableCondition), + "Status": Equal(metav1.ConditionTrue), + "Reason": Equal("SecretFound"), + "Message": Equal(""), + "ObservedGeneration": Equal(cfServiceInstance.Generation), }))) + g.Expect(cfServiceInstance.Status.Credentials.Name).To(Equal(cfServiceInstance.Spec.SecretName)) + g.Expect(cfServiceInstance.Status.CredentialsObservedVersion).NotTo(BeEmpty()) }).Should(Succeed()) }) @@ -84,49 +89,185 @@ var _ = Describe("CFServiceInstance", func() { }).Should(Succeed()) }) - When("the referenced secret does not exist", func() { + When("the credentials secret is invalid", func() { + BeforeEach(func() { + credentialsSecret.Data = map[string][]byte{} + }) + + It("sets credentials secret available condition to false", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(context.Background(), client.ObjectKeyFromObject(cfServiceInstance), cfServiceInstance)).To(Succeed()) + + g.Expect(cfServiceInstance.Status.Conditions).To(ContainElement(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(services.CredentialsSecretAvailableCondition), + "Status": Equal(metav1.ConditionFalse), + "Reason": Equal("SecretInvalid"), + "ObservedGeneration": Equal(cfServiceInstance.Generation), + }))) + }).Should(Succeed()) + }) + }) + + When("the credentials secret changes", func() { + var secretVersion string + + JustBeforeEach(func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(context.Background(), client.ObjectKeyFromObject(cfServiceInstance), cfServiceInstance)).To(Succeed()) + g.Expect(cfServiceInstance.Status.Conditions).To(ContainElement(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(services.CredentialsSecretAvailableCondition), + "Status": Equal(metav1.ConditionTrue), + "Reason": Equal("SecretFound"), + "Message": Equal(""), + }))) + secretVersion = cfServiceInstance.Status.CredentialsObservedVersion + }).Should(Succeed()) + + Expect(k8s.Patch(ctx, adminClient, credentialsSecret, func() { + credentialsSecret.StringData = map[string]string{"f": "b"} + })).To(Succeed()) + }) + + It("updates the credentials secret observed version", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(context.Background(), client.ObjectKeyFromObject(cfServiceInstance), cfServiceInstance)).To(Succeed()) + g.Expect(cfServiceInstance.Status.CredentialsObservedVersion).NotTo(Equal(secretVersion)) + }).Should(Succeed()) + }) + }) + + When("the credentials secret does not exist", func() { BeforeEach(func() { cfServiceInstance.Spec.SecretName = "other-secret-name" }) - It("sets the BindingSecretAvailable condition to false in the CFServiceInstance status", func() { + It("sets the CredentialsSecretAvailable condition to false in the CFServiceInstance status", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get( + ctx, + client.ObjectKeyFromObject(cfServiceInstance), + cfServiceInstance, + )).To(Succeed()) + g.Expect(meta.IsStatusConditionFalse( + cfServiceInstance.Status.Conditions, + services.CredentialsSecretAvailableCondition, + )).To(BeTrue()) + }).Should(Succeed()) + }) + }) + + When("the credentials secret gets deleted", func() { + var lastObservedVersion string + + JustBeforeEach(func() { Eventually(func(g Gomega) { - updatedCFServiceInstance := new(korifiv1alpha1.CFServiceInstance) - serviceInstanceNamespacedName := client.ObjectKeyFromObject(cfServiceInstance) - g.Expect(adminClient.Get(context.Background(), serviceInstanceNamespacedName, updatedCFServiceInstance)).To(Succeed()) - - g.Expect(updatedCFServiceInstance.Status.Binding).To(BeZero()) - g.Expect(updatedCFServiceInstance.Status.Conditions).To(ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal("BindingSecretAvailable"), - "Status": Equal(metav1.ConditionFalse), - "Reason": Equal("SecretNotFound"), - "Message": Equal("Binding secret does not exist"), + g.Expect(adminClient.Get( + ctx, + client.ObjectKeyFromObject(cfServiceInstance), + cfServiceInstance, + )).To(Succeed()) + g.Expect(meta.IsStatusConditionTrue( + cfServiceInstance.Status.Conditions, + services.CredentialsSecretAvailableCondition, + )).To(BeTrue()) + }).Should(Succeed()) + lastObservedVersion = cfServiceInstance.Status.CredentialsObservedVersion + + Expect(adminClient.Delete(ctx, credentialsSecret)).To(Succeed()) + }) + + It("does not change observed credentials secret", func() { + Consistently(func(g Gomega) { + g.Expect(adminClient.Get( + ctx, + client.ObjectKeyFromObject(cfServiceInstance), + cfServiceInstance, + )).To(Succeed()) + g.Expect(cfServiceInstance.Status.Credentials.Name).To(Equal(credentialsSecret.Name)) + g.Expect(cfServiceInstance.Status.CredentialsObservedVersion).To(Equal(lastObservedVersion)) + }).Should(Succeed()) + }) + }) + + Describe("legacy credentials secret reconciliation", func() { + BeforeEach(func() { + credentialsSecret = &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret-name", + Namespace: namespace.Name, + }, + Type: corev1.SecretType( + services.ServiceBindingSecretTypePrefix + "legacy", + ), + StringData: map[string]string{ + "foo": "bar", + }, + } + }) + + It("migrates the secret", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(cfServiceInstance), cfServiceInstance)).To(Succeed()) + g.Expect(cfServiceInstance.Status.Credentials.Name).To(Equal(cfServiceInstance.Name + "-migrated")) + + migratedSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: cfServiceInstance.Name + "-migrated", + Namespace: namespace.Name, + }, + } + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(migratedSecret), migratedSecret)).To(Succeed()) + g.Expect(cfServiceInstance.Status.CredentialsObservedVersion).To(Equal(migratedSecret.ResourceVersion)) + g.Expect(migratedSecret.Type).To(Equal(corev1.SecretTypeOpaque)) + g.Expect(migratedSecret.Data).To(MatchAllKeys(Keys{ + korifiv1alpha1.CredentialsSecretKey: Not(BeEmpty()), + })) + g.Expect(migratedSecret.OwnerReferences).To(ConsistOf(MatchFields(IgnoreExtras, Fields{ + "Kind": Equal("CFServiceInstance"), + "Name": Equal(cfServiceInstance.Name), }))) + + credentials := map[string]any{} + g.Expect(json.Unmarshal(migratedSecret.Data[korifiv1alpha1.CredentialsSecretKey], &credentials)).To(Succeed()) + g.Expect(credentials).To(MatchAllKeys(Keys{ + "foo": Equal("bar"), + })) + }).Should(Succeed()) + }) + + It("does not change the original credentials secret", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(cfServiceInstance), cfServiceInstance)).To(Succeed()) + g.Expect(cfServiceInstance.Status.Credentials.Name).NotTo(BeEmpty()) + + g.Expect(cfServiceInstance.Spec.SecretName).To(Equal(credentialsSecret.Name)) + + previousCredentialsVersion := credentialsSecret.ResourceVersion + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(credentialsSecret), credentialsSecret)).To(Succeed()) + g.Expect(credentialsSecret.ResourceVersion).To(Equal(previousCredentialsVersion)) }).Should(Succeed()) }) - When("the referenced secret is created afterwards", func() { + When("legacy secret cannot be migrated", func() { BeforeEach(func() { - Expect(adminClient.Create(context.Background(), &corev1.Secret{ + Expect(adminClient.Create(ctx, &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "other-secret-name", - Namespace: namespace.Name, + Name: cfServiceInstance.Name + "-migrated", + Namespace: cfServiceInstance.Namespace, }, + Type: corev1.SecretType("legacy"), })).To(Succeed()) }) - It("sets the BindingSecretAvailable condition to true in the CFServiceInstance status", func() { + It("sets credentials secret not available status condition", func() { Eventually(func(g Gomega) { - updatedCFServiceInstance := new(korifiv1alpha1.CFServiceInstance) - serviceInstanceNamespacedName := client.ObjectKeyFromObject(cfServiceInstance) - g.Expect(adminClient.Get(context.Background(), serviceInstanceNamespacedName, updatedCFServiceInstance)).To(Succeed()) - - g.Expect(updatedCFServiceInstance.Status.Binding.Name).To(Equal("other-secret-name")) - g.Expect(updatedCFServiceInstance.Status.Conditions).To(ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal("BindingSecretAvailable"), - "Status": Equal(metav1.ConditionTrue), - "Reason": Equal("SecretFound"), - "Message": Equal(""), + g.Expect(adminClient.Get(context.Background(), client.ObjectKeyFromObject(cfServiceInstance), cfServiceInstance)).To(Succeed()) + + g.Expect(cfServiceInstance.Status.Conditions).To(ContainElement(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(services.CredentialsSecretAvailableCondition), + "Status": Equal(metav1.ConditionFalse), + "Reason": Equal("FailedReconcilingCredentialsSecret"), + "ObservedGeneration": Equal(cfServiceInstance.Generation), }))) }).Should(Succeed()) }) diff --git a/controllers/controllers/services/credentials/credentials.go b/controllers/controllers/services/credentials/credentials.go new file mode 100644 index 000000000..e8b994109 --- /dev/null +++ b/controllers/controllers/services/credentials/credentials.go @@ -0,0 +1,68 @@ +package credentials + +import ( + "encoding/json" + "fmt" + + korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" + corev1 "k8s.io/api/core/v1" +) + +const ServiceBindingSecretTypePrefix = "servicebinding.io/" + +func GetBindingSecretType(credentialsSecret *corev1.Secret) (corev1.SecretType, error) { + credentials, err := getCredentials(credentialsSecret) + if err != nil { + return "", err + } + + userProvidedType, isString := credentials["type"].(string) + if isString { + return corev1.SecretType(ServiceBindingSecretTypePrefix + userProvidedType), nil + } + + return corev1.SecretType(ServiceBindingSecretTypePrefix + korifiv1alpha1.UserProvidedType), nil +} + +func GetBindingSecretData(credentialsSecret *corev1.Secret) (map[string][]byte, error) { + credentials, err := getCredentials(credentialsSecret) + if err != nil { + return nil, err + } + secretData := map[string][]byte{} + for k, v := range credentials { + secretData[k], err = toBytes(v) + if err != nil { + return nil, fmt.Errorf("failed to convert value of key %q to bytes: %w", k, err) + } + } + + return secretData, err +} + +func toBytes(value any) ([]byte, error) { + valueString, ok := value.(string) + if ok { + return []byte(valueString), nil + } + + return json.Marshal(value) +} + +func getCredentials(credentialsSecret *corev1.Secret) (map[string]any, error) { + credentials, ok := credentialsSecret.Data[korifiv1alpha1.CredentialsSecretKey] + if !ok { + return nil, fmt.Errorf( + "data of secret %q does not contain the %q key", + credentialsSecret.Name, + korifiv1alpha1.CredentialsSecretKey, + ) + } + credentialsObject := map[string]any{} + err := json.Unmarshal(credentials, &credentialsObject) + if err != nil { + return nil, fmt.Errorf("failed to unmarshal secret data: %w", err) + } + + return credentialsObject, nil +} diff --git a/controllers/controllers/services/credentials/credentials_suite_test.go b/controllers/controllers/services/credentials/credentials_suite_test.go new file mode 100644 index 000000000..72cf573be --- /dev/null +++ b/controllers/controllers/services/credentials/credentials_suite_test.go @@ -0,0 +1,13 @@ +package credentials_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestCredentials(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Credentials Suite") +} diff --git a/controllers/controllers/services/credentials/credentials_test.go b/controllers/controllers/services/credentials/credentials_test.go new file mode 100644 index 000000000..5aa010892 --- /dev/null +++ b/controllers/controllers/services/credentials/credentials_test.go @@ -0,0 +1,125 @@ +package credentials_test + +import ( + "fmt" + + korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" + "code.cloudfoundry.org/korifi/controllers/controllers/services/credentials" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + . "github.com/onsi/gomega/gstruct" + corev1 "k8s.io/api/core/v1" +) + +var _ = Describe("Credentials", func() { + var ( + err error + credentialsSecret *corev1.Secret + ) + + BeforeEach(func() { + credentialsSecret = &corev1.Secret{ + Data: map[string][]byte{ + korifiv1alpha1.CredentialsSecretKey: []byte(`{"foo":{"bar": "baz"}}`), + }, + } + }) + + Describe("GetBindingSecretType", func() { + var secretType corev1.SecretType + + JustBeforeEach(func() { + secretType, err = credentials.GetBindingSecretType(credentialsSecret) + }) + + It("returns user-provided type by default", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(secretType).To(BeEquivalentTo(credentials.ServiceBindingSecretTypePrefix + "user-provided")) + }) + + When("the type is specified in the credentials", func() { + BeforeEach(func() { + credentialsSecret.Data[korifiv1alpha1.CredentialsSecretKey] = []byte(`{"type":"my-type"}`) + }) + + It("returns the speicified type", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(secretType).To(BeEquivalentTo(credentials.ServiceBindingSecretTypePrefix + "my-type")) + }) + }) + + When("the type is not a plain string", func() { + BeforeEach(func() { + credentialsSecret.Data[korifiv1alpha1.CredentialsSecretKey] = []byte(`{"type":{"my-type":"is-not-a-string"}}`) + }) + + It("returns user-provided type", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(secretType).To(BeEquivalentTo(credentials.ServiceBindingSecretTypePrefix + "user-provided")) + }) + }) + + When("the credentials cannot be unmarshalled", func() { + BeforeEach(func() { + credentialsSecret.Data[korifiv1alpha1.CredentialsSecretKey] = []byte("invalid") + }) + + It("returns an error", func() { + Expect(err).To(MatchError(ContainSubstring("failed to unmarshal secret data"))) + }) + }) + + When("the credentials key is missing from the secret data", func() { + BeforeEach(func() { + credentialsSecret.Data = map[string][]byte{ + "foo": {}, + } + }) + + It("returns an error", func() { + Expect(err).To(MatchError(ContainSubstring( + fmt.Sprintf("does not contain the %q key", korifiv1alpha1.CredentialsSecretKey), + ))) + }) + }) + }) + + Describe("GetBindingSecretData", func() { + var bindingSecretData map[string][]byte + + JustBeforeEach(func() { + bindingSecretData, err = credentials.GetBindingSecretData(credentialsSecret) + }) + + It("converts the credentials into a flat strings map", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(bindingSecretData).To(MatchAllKeys(Keys{ + "foo": Equal([]byte(`{"bar":"baz"}`)), + })) + }) + + When("the credentials key is missing from the secret data", func() { + BeforeEach(func() { + credentialsSecret.Data = map[string][]byte{ + "foo": {}, + } + }) + + It("returns an error", func() { + Expect(err).To(MatchError(ContainSubstring( + fmt.Sprintf("does not contain the %q key", korifiv1alpha1.CredentialsSecretKey), + ))) + }) + }) + + When("the credentials cannot be unmarshalled", func() { + BeforeEach(func() { + credentialsSecret.Data[korifiv1alpha1.CredentialsSecretKey] = []byte("invalid") + }) + + It("returns an error", func() { + Expect(err).To(MatchError(ContainSubstring("failed to unmarshal secret data"))) + }) + }) + }) +}) diff --git a/controllers/controllers/shared/index.go b/controllers/controllers/shared/index.go index a613148db..9ecb75528 100644 --- a/controllers/controllers/shared/index.go +++ b/controllers/controllers/shared/index.go @@ -14,13 +14,14 @@ import ( ) const ( - IndexRouteDestinationAppName = "destinationAppName" - IndexRouteDomainQualifiedName = "domainQualifiedName" - IndexServiceBindingAppGUID = "serviceBindingAppGUID" - IndexServiceBindingServiceInstanceGUID = "serviceBindingServiceInstanceGUID" - IndexAppTasks = "appTasks" - IndexSpaceNamespaceName = "spaceNamespace" - IndexOrgNamespaceName = "orgNamespace" + IndexRouteDestinationAppName = "destinationAppName" + IndexRouteDomainQualifiedName = "domainQualifiedName" + IndexServiceInstanceCredentialsSecretName = "serviceInstanceCredentialsSecretName" + IndexServiceBindingAppGUID = "serviceBindingAppGUID" + IndexServiceBindingServiceInstanceGUID = "serviceBindingServiceInstanceGUID" + IndexAppTasks = "appTasks" + IndexSpaceNamespaceName = "spaceNamespace" + IndexOrgNamespaceName = "orgNamespace" StatusConditionReady = "Ready" ) @@ -70,6 +71,14 @@ func SetupIndexWithManager(mgr manager.Manager) error { return err } + err = mgr.GetFieldIndexer().IndexField(context.Background(), &korifiv1alpha1.CFServiceInstance{}, IndexServiceInstanceCredentialsSecretName, func(object client.Object) []string { + serviceInstance := object.(*korifiv1alpha1.CFServiceInstance) + return []string{serviceInstance.Spec.SecretName} + }) + if err != nil { + return err + } + return nil } diff --git a/docs/architecture-decisions/0016-user-provided-service-credentils.md b/docs/architecture-decisions/0016-user-provided-service-credentils.md new file mode 100644 index 000000000..33cb9653b --- /dev/null +++ b/docs/architecture-decisions/0016-user-provided-service-credentils.md @@ -0,0 +1,57 @@ +# User Provided Service Credentials + +Date: 2024-02-23 + +## Status + +Accepted + +## Context +According to the CF API, when creating or patching user provided services one can optionally specify a [credentials](https://v3-apidocs.cloudfoundry.org/version/3.159.0/index.html#optional-parameters-for-user-provided-service-instance) json object parameter. Up to Korifi `v0.11.0`, the credentials parameter was unintentionally limited to flat collection of key value pairs. In this ADR we discribe the mechanism of supporting json object credentials in a backwards compatible manner. + +What is meant by backwards compatible manner: +- Existing apps that are already bound to existing service instances continue to see the same service credentials +- Existing apps that are already bound to existing service instances do not get restarted during Korifi upgrade +- All apps continue to see servicebinding.io credentials as flat collection of key value pairs + +## Decision +* Store CF user provided service instance credentials in a dedicated secret named after the service instance with a single `credentials` key. +* Derive the servicebinding.io secret from the service instance secret, converting the credentials to key value pairs. +* Keep the [duck type](https://servicebinding.io/spec/core/1.0.0/#terminology-definition) of pre-existing service bindings unchanged to avoid undesired app restarts when Korifi is upgraded + +### Service Instance +We introduce a new `Credentials` field to `CFServiceInstance` status, where the service instance controller published the service instance credentials secret. Usually this is the secret from the `CFServiceInstance` spec. + +The service instance credentials secret is an `Opaque` secret. Its data has a single `credentials` key with the marshalled credentials object as its value. For example: + +``` +apiVersion: v1 +kind: Secret +type: Opaque +data: + credentials: '{"type":"database","user":{"name":"alice", "password": "bob"}}' +``` + +### Service Binding +The service binding controller derives the secret to put in the `.status.binding` duck type of the `CFServiceBinding` from the `.status.credentials` field of the `CFServiceInstance`. The binding secret is in the [format](https://servicebinding.io/spec/core/1.0.0/#example-secret) specified by servicebinding.io and is named after the service binding. According to that spec the secret has type `servicebinding.io/` and its data is a flat collection of key value pairs. For example: + +``` +apiVersion: v1 +kind: Secret +type: servicebinding.io/database +data: + type: database + user: '{"name":"alice", "password": "bob"}' +``` + +### Backwards Compatibility +The servicebinding.io contoller monitors the value of the `.status.binding` duck type of the `CFServiceBinding` and updates the spec of the workload statefulset to reflect any changes. The statefulset controller reacts to that change and rolls the statefulset out, causing an app restart. In order to avoid undesired restarting of apps during Korifi upgrade we make sure not to modify the value of the duck type of any pre-existing `CFServiceBinding`s. +- After a Korifi upgrade the servicebindings still reference the legacy credentials secret from the `CFServiceInstance` spec. +- The service instance controller creates a migrated secret in the new format and sets it in the `.status.credentials`. +- As long as service instance credentials do not change, the value of the `CFServiceBinding` duck type does not change as well. +- When the credentials of the legacy service instance are updated: + - The instance repo sets the `CFServiceInstance.Spec.SecretName` to the value from `CFServiceInstance.Status.Credentials` + - The instance repo applies the updated credentials to the credentials secret + - The service binding controller gets triggered by the `CFServiceInstance` update, derives the new binding secret and sets it in the `CFServiceBinding` duck type. This causes an app restart. + - From this moment on the original credentials secret (the one bearing the service instance name) becomes unused. It will eventually get deleted by owner reference once the service instance gets deleted. + diff --git a/go.mod b/go.mod index 8083ea884..10120b46a 100644 --- a/go.mod +++ b/go.mod @@ -143,7 +143,7 @@ require ( github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/opencontainers/go-digest v1.0.0 // indirect github.com/opencontainers/image-spec v1.1.0-rc5 // indirect - github.com/pkg/errors v0.9.1 // indirect + github.com/pkg/errors v0.9.1 github.com/prometheus/client_golang v1.18.0 // indirect github.com/prometheus/client_model v0.5.0 // indirect github.com/prometheus/common v0.45.0 // indirect diff --git a/helm/korifi/controllers/cf_roles/cf_admin.yaml b/helm/korifi/controllers/cf_roles/cf_admin.yaml index 4732e8fc0..bbc035602 100644 --- a/helm/korifi/controllers/cf_roles/cf_admin.yaml +++ b/helm/korifi/controllers/cf_roles/cf_admin.yaml @@ -111,6 +111,7 @@ rules: - create - patch - delete + - watch - apiGroups: - korifi.cloudfoundry.org diff --git a/helm/korifi/controllers/cf_roles/cf_space_developer.yaml b/helm/korifi/controllers/cf_roles/cf_space_developer.yaml index db3fba089..a4c597d57 100644 --- a/helm/korifi/controllers/cf_roles/cf_space_developer.yaml +++ b/helm/korifi/controllers/cf_roles/cf_space_developer.yaml @@ -94,6 +94,7 @@ rules: - create - patch - delete + - watch - apiGroups: - korifi.cloudfoundry.org diff --git a/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml b/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml index 783114a72..d27c43a80 100644 --- a/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml +++ b/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml @@ -77,19 +77,6 @@ spec: status: description: CFServiceInstanceStatus defines the observed state of CFServiceInstance properties: - binding: - description: |- - A reference to the Secret containing the credentials (same as spec.secretName). - This is required to conform to the Kubernetes Service Bindings spec - properties: - name: - description: |- - Name of the referent. - More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names - TODO: Add other useful fields. apiVersion, kind, uid? - type: string - type: object - x-kubernetes-map-type: atomic conditions: items: description: "Condition contains details for one aspect of the current @@ -159,6 +146,24 @@ spec: - type type: object type: array + credentials: + description: |- + A reference to the service instance secret containing the credentials + (derived from spec.secretName). + properties: + name: + description: |- + Name of the referent. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid? + type: string + type: object + x-kubernetes-map-type: atomic + credentialsObservedVersion: + description: |- + ObservedGeneration captures the latest version of the spec.secretName that has been reconciled + This will ensure that interested contollers are notified on instance credentials change + type: string observedGeneration: description: ObservedGeneration captures the latest generation of the CFServiceInstance that has been reconciled diff --git a/tests/e2e/apps_test.go b/tests/e2e/apps_test.go index 7285f2f53..250c20cd4 100644 --- a/tests/e2e/apps_test.go +++ b/tests/e2e/apps_test.go @@ -624,8 +624,8 @@ var _ = Describe("Apps", func() { Expect(json.Unmarshal(body, &data)).To(Succeed()) Expect(data).To(HaveLen(2)) - Expect(data[serviceInstanceGUID]).To(HaveKeyWithValue("foo", "bar"), string(body)) - Expect(data[serviceInstanceGUID]).To(HaveKeyWithValue("baz", "qux"), string(body)) + Expect(data[bindingGUID]).To(HaveKeyWithValue("foo", "bar"), string(body)) + Expect(data[bindingGUID]).To(HaveKeyWithValue("baz", "qux"), string(body)) Expect(data[bindingName]).To(HaveKeyWithValue("hello", "there"), string(body)) Expect(data[bindingName]).To(HaveKeyWithValue("secret", "stuff"), string(body)) }) diff --git a/tests/e2e/e2e_suite_test.go b/tests/e2e/e2e_suite_test.go index 8c6fd596e..397ddbff9 100644 --- a/tests/e2e/e2e_suite_test.go +++ b/tests/e2e/e2e_suite_test.go @@ -226,9 +226,9 @@ type destination struct { type serviceInstanceResource struct { resource `json:",inline"` - Tags []string `json:"tags"` - Credentials map[string]string `json:"credentials"` - InstanceType string `json:"type"` + Tags []string `json:"tags"` + Credentials map[string]any `json:"credentials"` + InstanceType string `json:"type"` } type appLogResource struct { diff --git a/tests/e2e/service_instances_test.go b/tests/e2e/service_instances_test.go index a61465a6d..d7490a27d 100644 --- a/tests/e2e/service_instances_test.go +++ b/tests/e2e/service_instances_test.go @@ -49,9 +49,8 @@ var _ = Describe("Service Instances", func() { }, }, }, - Credentials: map[string]string{ - "type": "database", - "hello": "creds", + Credentials: map[string]any{ + "object": map[string]any{"a": "b"}, }, InstanceType: "user-provided", Tags: []string{"some", "tags"}, @@ -85,8 +84,8 @@ var _ = Describe("Service Instances", func() { Annotations: map[string]string{"an-annotation": "an-annotation-value"}, }, }, - Credentials: map[string]string{ - "bye": "new-creds", + Credentials: map[string]any{ + "object-new": map[string]any{"new-a": "new-b"}, }, Tags: []string{"some", "tags"}, }).Patch("/v3/service_instances/" + existingInstanceGUID)