Skip to content

Commit

Permalink
Do not default the type credentials key in VCAP_SERVICES
Browse files Browse the repository at this point in the history 
* `type` has no special treatment outside the servicebinding.io secret
* Remove the validation of the `type` value change from the service
  instance repository
* Whenever the `type` value changes, the binding controller recreates
  the servicebinding.io secret with the new type. Recreation is needed
  as secrets' type is immutable

fixes #3169

WIP

Do not default/validate upsi credentials `type` in service instance
repository

Co-authored-by: Georgi Sabev <[email protected]>

WIP: binding controller recreates the secret on type change

Co-authored-by: Georgi Sabev <[email protected]>

wip

Co-authored-by: Georgi Sabev <[email protected]>
  • Loading branch information
@danail-branekov @georgethebeatle
danail-branekov and georgethebeatle committed Apr 17, 2024
1 parent e969e23 commit e0fe3e0
Show file tree
Hide file tree
Showing 8 changed files with 199 additions and 208 deletions.
72 changes: 3 additions & 69 deletions api/repositories/service_instance_repository.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,12 @@ import (
"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"
Expand All @@ -28,8 +26,6 @@ const (
CFServiceInstanceGUIDLabel = "korifi.cloudfoundry.org/service-instance-guid"
ServiceInstanceResourceType = "Service Instance"
CredentialsSecretAvailableCondition = "CredentialSecretAvailable"

credentialsTypeKey = "type"
)

type NamespaceGetter interface {
Expand Down Expand Up @@ -124,24 +120,14 @@ func (r *ServiceInstanceRepo) CreateServiceInstance(ctx context.Context, authInf
return ServiceInstanceRecord{}, apierrors.FromK8sError(err, ServiceInstanceResourceType)
}

err = r.createCredentialsSecret(ctx, userClient, cfServiceInstance, defaultCredentialsType(message.Credentials))
err = r.createCredentialsSecret(ctx, userClient, cfServiceInstance, message.Credentials)
if err != nil {
return ServiceInstanceRecord{}, apierrors.FromK8sError(err, ServiceInstanceResourceType)
}

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 {
Expand Down Expand Up @@ -197,9 +183,6 @@ func (r *ServiceInstanceRepo) patchCredentialsSecret(
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,
Expand All @@ -212,17 +195,7 @@ func (r *ServiceInstanceRepo) patchCredentialsSecret(
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)
return r.createCredentialsSecret(ctx, userClient, cfServiceInstance, credentials)
}

func (r *ServiceInstanceRepo) createCredentialsSecret(
Expand All @@ -231,9 +204,6 @@ func (r *ServiceInstanceRepo) createCredentialsSecret(
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,
Expand All @@ -248,7 +218,7 @@ func (r *ServiceInstanceRepo) createCredentialsSecret(
credentialsSecret.Labels[CFServiceInstanceGUIDLabel] = cfServiceInstance.Name

var err error
credentialsSecret.Data, err = toSecretData(newCredentials)
credentialsSecret.Data, err = toSecretData(credentials)
if err != nil {
return errors.New("failed to marshal credentials for service instance")
}
Expand All @@ -270,42 +240,6 @@ func toSecretData(credentials map[string]any) (map[string][]byte, error) {
}, 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
func (r *ServiceInstanceRepo) ListServiceInstances(ctx context.Context, authInfo authorization.Info, message ListServiceInstanceMessage) ([]ServiceInstanceRecord, error) {
nsList, err := r.namespacePermissions.GetAuthorizedSpaceNamespaces(ctx, authInfo)
Expand Down
99 changes: 2 additions & 97 deletions api/repositories/service_instance_repository_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ var _ = Describe("ServiceInstanceRepository", func() {
BeforeEach(func() {
serviceInstanceTags = []string{"foo", "bar"}
serviceInstanceCredentials = map[string]any{
"type": "my-type",
"object": map[string]any{"a": "b"},
}

Expand Down Expand Up @@ -112,24 +111,6 @@ var _ = Describe("ServiceInstanceRepository", func() {
Expect(json.Unmarshal(createdSecret.Data[korifiv1alpha1.CredentialsSecretKey], &credentials)).To(Succeed())
Expect(credentials).To(Equal(serviceInstanceCredentials))
})

When("ServiceInstance credential type is not provided", func() {
BeforeEach(func() {
serviceInstanceCreateMessage.Credentials = map[string]any{
"a": "b",
}
})

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"),
}))
})
})
})

When("user does not have permissions to create ServiceInstances", func() {
Expand Down Expand Up @@ -163,7 +144,7 @@ var _ = Describe("ServiceInstanceRepository", func() {
Namespace: space.Name,
},
StringData: map[string]string{
korifiv1alpha1.CredentialsSecretKey: `{"type":"database","a": "b"}`,
korifiv1alpha1.CredentialsSecretKey: `{"a": "b"}`,
},
}
Expect(k8sClient.Create(ctx, secret)).To(Succeed())
Expand Down Expand Up @@ -259,16 +240,14 @@ var _ = Describe("ServiceInstanceRepository", func() {
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"),
"a": Equal("b"),
}))
}).Should(Succeed())
})

When("ServiceInstance credentials are provided", func() {
BeforeEach(func() {
patchMessage.Credentials = &map[string]any{
"type": "database",
"object": map[string]any{"c": "d"},
}
})
Expand All @@ -289,7 +268,6 @@ var _ = Describe("ServiceInstanceRepository", func() {
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())
Expand All @@ -305,79 +283,6 @@ var _ = Describe("ServiceInstanceRepository", func() {
})
})

When("the instance credentials modify the type", func() {
BeforeEach(func() {
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]any{
"cred-one": "val-one",
"cred-two": "val-two",
}
})

It("updates the creds and keeps the existing type", 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(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"),
}))
}).Should(Succeed())
})
})

When("the instance credentials pass the old type unchanged", func() {
BeforeEach(func() {
patchMessage.Credentials = &map[string]any{
"type": "database",
"cred-one": "val-one",
"cred-two": "val-two",
}
})

It("updates the creds and keeps the existing type", 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(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"),
}))
}).Should(Succeed())
})
})

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{
Expand Down
14 changes: 13 additions & 1 deletion controllers/controllers/services/bindings/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,10 @@ import (
"code.cloudfoundry.org/korifi/tools/k8s"

"github.com/go-logr/logr"
"github.com/pkg/errors"
servicebindingv1beta1 "github.com/servicebinding/runtime/apis/v1beta1"
corev1 "k8s.io/api/core/v1"
k8serrors "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"
Expand Down Expand Up @@ -133,6 +135,16 @@ func (r *CFServiceBindingReconciler) ReconcileResource(ctx context.Context, cfSe

credentialsSecret, err := r.reconcileCredentials(ctx, cfServiceInstance, cfServiceBinding)
if err != nil {
if k8serrors.IsInvalid(err) {
err = r.k8sClient.Delete(ctx, &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: cfServiceBinding.Name,
Namespace: cfServiceBinding.Namespace,
},
})
return ctrl.Result{Requeue: true}, errors.Wrap(err, "failed to delete outdated binding secret")
}

log.Error(err, "failed to reconcile credentials secret")
meta.SetStatusCondition(&cfServiceBinding.Status.Conditions, metav1.Condition{
Type: BindingSecretAvailableCondition,
Expand Down Expand Up @@ -242,7 +254,7 @@ func (r *CFServiceBindingReconciler) reconcileCredentials(ctx context.Context, c
return controllerutil.SetControllerReference(cfServiceBinding, bindingSecret, r.scheme)
})
if err != nil {
return nil, fmt.Errorf("failed to create binding secret: %w", err)
return nil, errors.Wrap(err, "failed to create binding secret")
}

cfServiceBinding.Status.Binding.Name = bindingSecret.Name
Expand Down
Loading

0 comments on commit e0fe3e0

Please sign in to comment.