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

Implement "POST /v3/service_brokers" #3330

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

danail-branekov
Copy link
Member

Is there a related GitHub Issue?

#3263

What is this change about?

Implement POST /v3/service_brokers

  • Globally available brokers are only supported (space-restricted
    brokers are currently out of scope)
  • Introduce the CFServiceBroker custom resource
  • The CFServiceBroker controller sets its Ready state
  • Implement POST /v3/service_brokers
  • Only allow admin users to create brokers
  • The CFServiceBroker object is created in the root namespace (as it
    is globally available)
  • Implement GET /v3/jobs/service_broker.create~<broker-guid> to return
    COMPLETE job state once the broker is ready
  • Validate broker name uniqueness via a validating webhook

Discussion

I would like to take the opportunity of this PR to discuss an alternative way to model Korifi objects.

Currently, we have three types - the k8s object spec, the repository record, the request objects in the API (on e.g. create) and the object being returned from the API (on e.g. get). While those types are quite similar, we need boilerplate code to convert them. For example:

  1. The create CFApp payload object describes the CFApp
  2. The payload object is translated to repository message
  3. The repo create message is translated into CFApp.Spec
  4. The CFApp object is translated to repository record
  5. The record object is translated into API reponse object

The approach this PR takes to model the new ServiceBroker resource is a bit different. Instead of having so many different types, model the object into multiple small reusable type and aggregate them as necessary. Here are the details:

  1. Introduce a model package where we put small reusable types
  2. The CFResource type is a layer that is needed for API responses - it has all the common API response fields like GUID, createdAt, etc.
  3. The model.services.ServiceBroker type describes a service broker resource and only contains the fields that are specific to the service broker resource only
  4. The korifiv1alhpa1.CFServiceBrokerSpec type embeds model.services.ServiceBroker and adds a k8s-specific field to reference the credentials secret. Note that this reference only makes sense to k8s and is therefore placed in the spec type
  5. The message to the repository to create a broker (repositories.CreateServiceBrokerMessage) composes the common types
  6. The repository returns a repositories.ServiceBrokerResource object which, agains, only composes model types
  7. The create payload object - payloads.ServiceBrokerCreate composes the model objects in a way to conform to the CF API specification
  8. Not demonstrated in this PR, but when get service-broker is implemented, the resource object returned by the repository could be presented in a fairly simple manner as the service broker resource already fits the CF API service broker object. It only lacks the links section which could be added by the presenter.

So what do you think about this approach? I think it has its merits though it might look a bit confusing at first. If we agree on it, then we could use it for new resources and experiment with migrating existing ones (hopefully that should be possible in a backwards compatible manner). If not, we could always use the well known pattern.

Does this PR introduce a breaking change?

No

Acceptance Steps

See story

Tag your pair, your PM, and/or team

@cloudfoundry/wg-cf-on-k8s

@danail-branekov danail-branekov force-pushed the issues/3263-service-brokers branch 3 times, most recently from 956e01c to 05927a3 Compare June 11, 2024 08:36
* Globally available brokers are only supported (space-restricted
  brokers are currently out of scope)
* Introduce the `CFServiceBroker` custom resource
* The `CFServiceBroker` controller sets its `Ready` state
* Implement `POST /v3/service_brokers`
* Only allow admin users to create brokers
* The `CFServiceBroker` object is created in the root namespace (as it
  is globally available)
* Implement `GET /v3/jobs/service_broker.create~<broker-guid>` to return
  `COMPLETE` job state once the broker is ready
* Validate broker name uniqueness via a validating webhook

fixes #3263

Co-authored-by: Danail Branekov <danailster@gmail.com>
@georgethebeatle georgethebeatle force-pushed the issues/3263-service-brokers branch from 05927a3 to 5232293 Compare July 2, 2024 14:52
@danail-branekov danail-branekov enabled auto-merge (rebase) July 3, 2024 09:18
@danail-branekov danail-branekov merged commit aa035c8 into main Jul 3, 2024
10 of 11 checks passed
@danail-branekov danail-branekov deleted the issues/3263-service-brokers branch July 3, 2024 11:24
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.

2 participants