Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not default the type credentials key in VCAP_SERVICES #3216

Merged
merged 1 commit into from
Apr 17, 2024
Merged

Do not default the type credentials key in VCAP_SERVICES #3216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 3 additions & 69 deletions api/repositories/service_instance_repository.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading