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.

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

Support objects for user-provided services credentials #3140

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

georgethebeatle
Copy link
Member

@georgethebeatle georgethebeatle commented Feb 23, 2024

Is there a related GitHub Issue?

#2900

What is this change about?

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

Does this PR introduce a breaking change?

No

Acceptance Steps

See #2900

Tag your pair, your PM, and/or team

@cloudfoundry/wg-cf-on-k8s

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 #2900

Co-authored-by: Danail Branekov <danailster@gmail.com>
@tcdowney
Copy link
Member

tcdowney commented Mar 6, 2024

I think this looks like it should work, but I'm wondering if we have any automated tests that push an app that actually uses servicebindings.io bindings? Like a Java Spring app that uses https://github.com/spring-cloud/spring-cloud-bindings.

There are some important keys in there like the name of the binding and especially the type that I think are still wired through correctly, but are now becoming fairly brittle.

https://github.com/cloudfoundry-samples/spring-music is one I've used to validate service bindings are working.

@georgethebeatle
Copy link
Member Author

Hey @tcdowney, thanks for taking a look, we just validated that spring-music works with this change. We thought it might be useful to deploy spring-music on the acceptance cluster, but realised that we are already deploying postfacto which is consuming a database as well. This should be good enough, right? If you think that spring boot is an important use case we might drop in a chore to deploy spring-music next to postfacto.

@tcdowney
Copy link
Member

tcdowney commented Mar 8, 2024

Hey @tcdowney, thanks for taking a look, we just validated that spring-music works with this change. We thought it might be useful to deploy spring-music on the acceptance cluster, but realised that we are already deploying postfacto which is consuming a database as well. This should be good enough, right? If you think that spring boot is an important use case we might drop in a chore to deploy spring-music next to postfacto.

Cool thank you for manually validating Spring Music! I think a chore would be good because Spring apps are one of the few app types that automatically get Cloud Native Service Binding support through https://github.com/spring-cloud/spring-cloud-bindings so we can be sure we're still creating them in a way that apps can use.

I don't think Postfacto is the best example cause I don't think we're actually even using service bindings for it. I think we're just constructing DATABASE_URL manually in its manifest:
https://github.com/eirini-forks/postfacto-release/blob/9dbfdee6dcd63787158b173dcec2468fb025eea8/manifest.yml#L6

@georgethebeatle
Copy link
Member Author

@tcdowney created the chore: #3161

@danail-branekov danail-branekov merged commit da2d312 into main Mar 11, 2024
11 checks passed
@danail-branekov danail-branekov deleted the issues/2900-upsi-credentials-object branch March 11, 2024 14:50
georgethebeatle added a commit that referenced this pull request May 2, 2024
Adopt PR #3140

This PR deals with user-provided service instance credentials which is
somehow related to managed services credentials

Co-authored-by: Danail Branekov <danailster@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Korifi API users should be able to provide JSON objects as user provided service parameters
3 participants