Implement PATCH /v3/service_instances for managed services #4167

ybykov-a9s · 2025-09-17T14:29:13Z

Is there a related GitHub Issue?

The PR is aimed to solve this Issue 4163

What is this change about?

Implementing PATCH /v3/service_instances for managed services

Does this PR introduce a breaking change?

It shouldn't be a breaking change

Acceptance Steps

The PR is still WIP

Tag your pair, your PM, and/or team

danail-branekov

Implement UPDATE in osbapi

danail-branekov

Looks promising. The most important bits of my comments are on the managed instance controller

danail-branekov · 2025-09-17T15:28:03Z

api/repositories/service_instance_repository.go

+type PatchManagedSIMessage struct {
+	GUID        string
+	SpaceGUID   string
+	PlanGUID    string


I wonder whether PlanGUID should not be a pointer? A patch may want to update the metadata, but not the plan

api/repositories/service_instance_repository.go

danail-branekov · 2025-09-17T15:46:22Z

controllers/controllers/services/instances/managed/controller.go

-		log.Error(err, "failed to provision service instance")
-		return ctrl.Result{}, fmt.Errorf("failed to provision service instance: %w", err)
-	}
+	if serviceInstance.Status.ObservedGeneration == 0 {


If I get the intention of this check correctly, the idea here is to perform provision when the CFServiceInstance has been just created, otherwise proceed to update.

The generation might drift because of whatever updates (maybe a new label appeared) that may occur while provisioning is already running.

Instead of relying on the observed generation, I would suggest to add PlanGUID to CFServiceInstance.Status. Then the plan guid on the spec would have the semantics of "desired plan", while the plan guid on the status would mean "actual plan". Then if both differ, the controller should perform a plan update. Once the update succeed, the controller should set the new plan guid on the status, thus "writing down" that this is already the actual plan.

Also, it is worth mentioning that once the instance is ready (isReady function returns true), the controller would stop listening for updates. Therefore you should change the function to also check whether the "desired plan" matches the "actual plan"

I changed the logic here as you proposed to use Status. Please check it.

controllers/controllers/services/instances/managed/controller.go

danail-branekov · 2025-09-17T15:56:34Z

controllers/controllers/services/instances/managed/controller.go

+
+	if lastOpResponse.State == "failed" {
+		meta.SetStatusCondition(&serviceInstance.Status.Conditions, metav1.Condition{
+			Type:               korifiv1alpha1.ProvisioningFailedCondition,


I think you should set UpdateFailedCondition here

Thank you for the catch.

danail-branekov · 2025-09-17T15:59:03Z

controllers/controllers/services/osbapi/client.go

+	if err != nil {
+		return UpdateResponse{}, fmt.Errorf("update request failed: %w", err)
+	}
+	if statusCode == http.StatusBadRequest || statusCode == http.StatusConflict || statusCode == http.StatusUnprocessableEntity {


According to the osbapi spec, an update operation cannot result into StatusConflict

…endpoint

…loop

…eFailedCondition

…endpoint

ybykov-a9s · 2026-01-30T09:57:40Z

As an updated summary - I applied all suggestions. Maybe used not ideal naming 😁
Updated PR also contains e2e tests with some adjustments.
@danail-branekov Please check if it looks better now.
I would like to mention that the version i showed initially was really far from being ready because I wanted to ask about the general approach. And it worked,

danail-branekov

See my comments below, significant bits are on the controller. I think you are going in the right direction.

A general style comment - we usually put a blank line between ginkgo blocks - Describe, When, It, etc to visually separate them. If you could keep that convention in the PR, that would be great.

danail-branekov · 2026-01-30T14:54:22Z

api/handlers/service_instance_test.go


-			It("returns an error", func() {
-				expectUnprocessableEntityError("nope")
+			When("getting the service instance fails with not found", func() {


Style: By introducing a separate context for managed services, the describe has now the floowing structure:

Describe("PATCH /v3/service_instances/:guid", func() { When("updating a user provided service instance", func() { // all existing tests go here When("updating a managed service instance", func() {

What I see as an issue here is that error cases (such as service instance repository returning an error) are checked in the upsi context, though being irrelevant to whether the service is upsi or not. As a matter of fact, the only difference between uspi and managed is the repository method being invoked.

Therefore, how about the following structure:

Describe("PATCH /v3/service_instances/:guid", func() { // existing (upsi) tests still here, i.e. this is the "default" case When("the service instance is managed", func() { It("patches the managed service instance", func() { It("returns HTTP 202 Accepted response", func() { When("patching the managed service instances fails", func() { It("returns the error", func() { })

This should also reduce the diff as indentation white spaces would not be there

danail-branekov · 2026-01-30T15:04:50Z

api/payloads/service_instance.go


+func (p ServiceInstancePatch) ToManagedSIPatchMessage(spaceGUID, appGUID string) repositories.PatchManagedSIMessage {
+	var planGUID *string
+	if p.Relationships.ServicePlan.Data.GUID != "" {


If I get the intention correctly, you want to check whether the plan is being updated. In that case I think you should be checking on p.Relationships.ServicePlan != nil rather than p.Relationships.ServicePlan.Data.GUID != "". If p.Relationships.ServicePlan is set, then the validation should ensure that p.Relationships.ServicePlan.Data.GUID is not empty.

danail-branekov · 2026-01-30T15:41:35Z

api/repositories/service_instance_repository.go

 	return cfServiceInstanceToRecord(*cfServiceInstance), nil
 }

+func (r *ServiceInstanceRepo) PatchManagedServiceInstance(ctx context.Context, authInfo authorization.Info, message PatchManagedSIMessage) (ServiceInstanceRecord, error) {


There are no tests for this new method. They would be pretty much the same as the ones for UPSI but I cannot think a proper way to avoid the duplication

danail-branekov · 2026-01-30T15:42:59Z

controllers/api/v1alpha1/cfserviceinstance_types.go

 	//+kubebuilder:validation:Optional
 	MaintenanceInfo MaintenanceInfo `json:"maintenanceInfo"`

+	// The service instance actual plan


I would add a note that this only makes sense for managed services (see MaintenanceInfo)

danail-branekov · 2026-01-30T15:47:05Z

controllers/controllers/services/instances/managed/controller.go


 	if isFailed(serviceInstance) {
-		return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ProvisioningFailed").WithNoRequeue()
+		return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ReconciliationFailed").WithNoRequeue()


It is not that reconciliation failed per se, it is just that the service instance is in a failed state. Maybe ServiceInstanceFailed would be a proper reason here?

danail-branekov · 2026-01-30T17:04:08Z

controllers/webhooks/networking/security_groups/validator_test.go

 				))
 			})
 		})
-


This file should not be affected by the PR at all. While I agree that removing those new lines makes sense, the generated diff is a high price to pay for such a small improvement.

danail-branekov · 2026-01-30T17:09:39Z

tests/e2e/e2e_suite_test.go

 	return serviceInstance.GUID
 }

+func createManagedServiceInstance(brokerGUID, spaceGUID string, name string) string {


Is there any particular reason for the name parameter? All references supply an inline-generated guid, therefore they do not seem to care about it. Let's not overload the method signature if that is the case.

danail-branekov · 2026-01-30T17:21:15Z

tests/e2e/service_instances_test.go

+				Patch("/v3/service_instances/" + serviceInstanceGUID)
+		})
+
+		When("updating a user-provided service instance", func() {


Test structure: The describe has two sibling contexts and there is no default case. I would recommend having just introducing the serviceInstanceGUID variable at the root of the context and default it to the upsiGUID in the BeforeEach.

Then, have a context When("the service instance is managed, where you create the managed instance and set serviceInstanceGUID to its guid. Such an approach would also minimise the diff.

danail-branekov · 2026-01-30T17:24:07Z

tests/e2e/service_instances_test.go

+				catalogResp, err := adminClient.R().SetResult(&plansResp).Get("/v3/service_plans?service_broker_guids=" + brokerGUID)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(catalogResp).To(HaveRestyStatusCode(http.StatusOK))
+				Expect(plansResp.Resources).NotTo(BeEmpty())


Maybe assert that Resource has length at least 2 (plansResp.Resources[1] is used below)

danail-branekov · 2026-01-30T17:28:11Z

tests/e2e/service_instances_test.go

+				Expect(httpErrorService).NotTo(HaveOccurred())
+				Expect(httpRespService).To(HaveRestyStatusCode(http.StatusOK))
+				Expect(
+					result.resource.Relationships["service_plan"].Data.GUID,


How about checking the service_plan relationship in It("updates a managed service"? Every new It slows down the suite execution time so we have to strike a balance

ybykov-a9s added 2 commits September 10, 2025 16:56

Split PATCH /v3/service_instances logic into UPSI and Managed

2c75f08

Implement UPDATE in osbapi

Add unit tests for managed services on PATCH /v3/service_instances

32fc9d8

ybykov-a9s mentioned this pull request Sep 17, 2025

As a Korifi user I can change a plan of a managed service instance #4163

Open

danail-branekov marked this pull request as draft September 17, 2025 15:22

danail-branekov reviewed Sep 17, 2025

View reviewed changes

ybykov-a9s and others added 6 commits October 23, 2025 16:10

Fix api/repositories/service_instance_repository.go due to suggestions

8661c74

Merge branch 'cloudfoundry:main' into extend-update-service-instance-…

ea066a9

…endpoint

Cover controllers/services/osbapi with tests

bc1663d

[controllers][managed services] Use Status.PlanGUID in reconcilation …

3ffc150

…loop

[controllers][managed services] update isFailed in order to use Updat…

2d1bd24

…eFailedCondition

[controllers][services][osbapi] Fix broker statuscodes on Update call

6651e36

ybykov-a9s force-pushed the extend-update-service-instance-endpoint branch from bdb5833 to 6651e36 Compare January 23, 2026 12:53

ybykov-a9s and others added 2 commits January 29, 2026 16:31

Merge branch 'cloudfoundry:main' into extend-update-service-instance-…

4316eb6

…endpoint

Implement e2e tests for updating managed services

5627bf1

danail-branekov reviewed Jan 30, 2026

View reviewed changes

Implement PATCH /v3/service_instances for managed services #4167

Are you sure you want to change the base?

Implement PATCH /v3/service_instances for managed services #4167

Uh oh!

Conversation

ybykov-a9s commented Sep 17, 2025

Is there a related GitHub Issue?

What is this change about?

Does this PR introduce a breaking change?

Acceptance Steps

Tag your pair, your PM, and/or team

Uh oh!

danail-branekov left a comment

Choose a reason for hiding this comment

Uh oh!

danail-branekov Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ybykov-a9s commented Jan 30, 2026

Uh oh!

danail-branekov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danail-branekov Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

danail-branekov Sep 17, 2025 •

edited

Loading

danail-branekov left a comment •

edited

Loading

danail-branekov Jan 30, 2026 •

edited

Loading