Implement PATCH /v3/service_instances for managed services by ybykov-a9s · Pull Request #4167 · cloudfoundry/korifi

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

+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

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

-		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.

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

+
+	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

+	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


-			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

I rearranged the test structure.

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


+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.

added check for nil first

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

 	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

Added tests. Mostly from UPSI, to cover new functionality.

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

 	//+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


 	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

 				))
 			})
 		})
-


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.

I bet my local automation did it when i touched the file. I wasn't sure if it should be included to the commit or not. No problem - removed it to decrease diff.

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

 	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.

Existing createUPServiceInstance (renamed from the original createServiceInstance) a bit above has it. So i preserved the logic.

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

+				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.

I moved defining serviceInstanceGUID to BeforeEach section. Not sure it minimizes the diff, but rather makes the logic cleaner. What do you think?

Better, but still - the describe only has two contexts and no default It. How about

Describe("Update", func(){ BeforeEach(func(){ // upsi update test setup }) JustBeforeEach(func(){ /* invokes the method under test */}) It("succeeds", func(){/* upsi update checks*/ }) When("updating a managed service instance", func(){ BeforeEach(func(){/*create+stub the broker, the managed SI, set updateRequestBody to managed SI update params }) AfterEach(func() { */managed SI cleanup */}) It("succeeds with a job redirect", func() { ... }) It("updates a managed service", func() {...}) })

With this structure, the "default" case is an UPSI update, and updating a managed service is a "corner case" that requires more setup

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

+				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)

true. thanks. changed the test.

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

+				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

It's always a dilemma - make more specific tests or combine them to minimize their number :D
In this case i would agree that your way makes more sense.

…endpoint

…iled

…heck

ybykov-a9s · 2026-02-27T11:19:15Z

@danail-branekov Thank you for your comments. Please check updates. All besides one (name parameter in createManagedServiceInstance method) I changed as you proposed. Hope now the PR looks better.

P.S.
I'm really impressed with your patience. Thank you a lot for your time.

danail-branekov

I think you are making a steady progress, there are just few things to iron out

danail-branekov · 2026-02-27T15:47:10Z

-	Credentials *map[string]any `json:"credentials,omitempty"`
-	Metadata    MetadataPatch   `json:"metadata"`
+	Name          *string                       `json:"name,omitempty"`
+	Type          string                        `json:"type"`


The PATCH /v3/service_instances/[guid] endpoint does not have Type parameter. Also the field is unused

I don't even remember what i wanted to achieve months ago. Removed it.

danail-branekov · 2026-02-27T15:54:09Z

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


That if is not necessary. Provided that p.Relationships.ServicePlan is not nil, then payload validation should have ensured that it also has Data.GUID

Also, the statement following the if block is unnecessary.

TL;DR the code should be

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

🤦
End of the Friday. Sorry for that. 😃
Fixed.

danail-branekov · 2026-02-27T15:57:28Z

 	return serviceInstance
 }

+func createManagedServiceInstanceCR(ctx context.Context, k8sClient client.Client, serviceInstanceGUID, spaceGUID, name, plan string) *korifiv1alpha1.CFServiceInstance {


I realise that you have introduced this method to be in par with the UPSI tests. We introduced that kind of utility methods a long time ago and eventually realised that they make the code unreadable instead of being helpful. Therefore we are gradually getting rid of those and replace their invocations with inlining the code making sure we are only initialising objects with data related to the concrete test.

Provided that createManagedServiceInstanceCR is used in a single place, could you please inline the code there?

Moved it as an inline block.

danail-branekov · 2026-02-27T16:02:47Z

 			})
 		})
+
+		When("Patching a managed service instance", func() {


This context is exercising the new method PatchManagedServiceInstance and alters the behaviour of the parent describe JustBeforeEach. Generally, we prefer a describe per method with a single JustBeforeEach.

Could you change the structure of those test to something like

Describe("PatchUserProvidedServiceInstance", // patch upsi tests Describe("PatchManagedServiceInstance", //patch managed service tests

Mmm. OK. done.

danail-branekov · 2026-02-27T16:10:43Z

@@ -148,7 +148,7 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor
 	}

 	if isFailed(serviceInstance) {


The method only checks whether ProvisioningFailedCondition status condition is true. Therefore it makes sense to rename the method to isProvisioningFailed or similar

Also, the reason of the not ready error below probably makes sense to remain ProvisioningFailed rather than the generic ServiceInstanceFailed

isFailed was an original name of the method. But I agree that a new name is better. Updated.

danail-branekov · 2026-02-27T16:20:51Z

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


In order to make the method more readable and concise, could you please extract the code into utility methods, for example

func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *korifiv1alpha1.CFServiceInstance) (ctrl.Result, error) { // getting the service, validation, assets, etc boilerplate here if !serviceInstance.Status.Provisioned { return r.reconcileProvisionedServiceInstance(...) // old provisioning code here, set the status.Provisioned field } return r.reconcileUpdatedServiceInstance(...) // new code here }

danail-branekov · 2026-02-27T16:29:01Z

+		serviceBroker   *korifiv1alpha1.CFServiceBroker
+		serviceOffering *korifiv1alpha1.CFServiceOffering
+		servicePlan     *korifiv1alpha1.CFServicePlan
+		servicePlan2    *korifiv1alpha1.CFServicePlan


servicePlan2 could be local to When("the service instance is being updated" as it is only used there

danail-branekov · 2026-02-27T16:40:24Z

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


Better, but still - the describe only has two contexts and no default It. How about

Describe("Update", func(){ BeforeEach(func(){ // upsi update test setup }) JustBeforeEach(func(){ /* invokes the method under test */}) It("succeeds", func(){/* upsi update checks*/ }) When("updating a managed service instance", func(){ BeforeEach(func(){/*create+stub the broker, the managed SI, set updateRequestBody to managed SI update params }) AfterEach(func() { */managed SI cleanup */}) It("succeeds with a job redirect", func() { ... }) It("updates a managed service", func() {...}) })

With this structure, the "default" case is an UPSI update, and updating a managed service is a "corner case" that requires more setup

danail-branekov · 2026-02-27T16:43:37Z

P.S. I'm really impressed with your patience. Thank you a lot for your time.

You are most welcome. Guiding contributors is part of the maintainers' job description

ybykov-a9s · 2026-03-02T13:24:45Z

@danail-branekov I made all the changes you've requested including the latest of your suggestions here, which Github doesn't allow me to comment for unknown reason.

Please check it.

…endpoint

danail-branekov · 2026-03-04T12:49:25Z

@ybykov-a9s Tehre are a couple of CI failures with the current state of the PR:

A couple of linter errors on code duplication - you could suppress those via the nolint:dupl comment - example. You could run the make lint target to simulate what the CI is doing locally.
PatchManagedServiceInstance [BeforeEach] is failing - sounds like a metadata.name for the cfservice instance the before each creates is not set. I believe the reason is that serviceInstanceGUID variable is not initialised before creating the CFServiceInstance. You could run ./scripts/run-tests.sh api/repositories to run repositories tests only, or make -C api test to run all api tests the way CI would

danail-branekov · 2026-03-04T13:03:27Z

+			}
+
+			Expect(k8sClient.Create(ctx, cfServiceInstance)).To(Succeed())
+			conditionAwaiter.AwaitConditionReturns(cfServiceInstance, nil)


The condition awaiter is not used when patching the managed service instance, so stubbing it here does not really make sense

danail-branekov · 2026-03-04T13:07:53Z

In addition to the comments above, this PR should enable updating the service instance plan from the cli. Therefore, you could also introduce a smoke test that exercises cf update-service <service> -p <new-plan> here

We could have that as a follow-up PR though

ybykov-a9s · 2026-03-05T09:14:21Z

@danail-branekov

I fixed linter issues as you suggested. TBH never ran lint tests to make it faster, but it's not a solution :-)
Also fixed PatchManagedServiceInstance [BeforeEach] . Donno how could i miss it because I always run tests.
Removed conditionAwaiter as you suggested after I checked what it does.

I agree to introduce a smoke tests for changing a plan as a follow-up PR. It would require for me some time to learn Korifi smoke tests in general. And it would delay merging this PR.

danail-branekov

Today I spent some time checking out how the update works. I am afraid that I found out that when updating the service instance (via cf update-service managed-service -p sample-2), no communication with the broker is performed.

While investigating the issue I took the liberty to come up with a diff that addresses that (it also addresses the comments below):

diff --git a/controllers/controllers/services/instances/managed/controller.go b/controllers/controllers/services/instances/managed/controller.go
index bcf440a5..c2f52bcb 100644
--- a/controllers/controllers/services/instances/managed/controller.go
+++ b/controllers/controllers/services/instances/managed/controller.go
@@ -143,10 +143,6 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor
 
 	serviceInstance.Status.UpgradeAvailable = serviceInstance.Status.MaintenanceInfo.Version != serviceInstanceAssets.ServicePlan.Spec.MaintenanceInfo.Version
 
-	if isReady(serviceInstance) {
-		return ctrl.Result{}, nil
-	}
-
 	if isProvisioningFailed(serviceInstance) {
 		return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ServiceInstanceFailed").WithNoRequeue()
 	}
@@ -220,7 +216,6 @@ func (r *Reconciler) reconcileUpdatedServiceInstance( //nolint:dupl
 		return r.processUpdateOperation(serviceInstance, lastOpResponse)
 	}
 
-	serviceInstance.Status.Provisioned = true
 	serviceInstance.Status.MaintenanceInfo = serviceInstanceAssets.ServicePlan.Spec.MaintenanceInfo
 	serviceInstance.Status.LastOperation.State = "succeeded"
 	return ctrl.Result{}, nil
@@ -584,7 +579,3 @@ func (r *Reconciler) getNamespace(ctx context.Context, namespaceName string) (*c
 func isProvisioningFailed(instance *korifiv1alpha1.CFServiceInstance) bool {
 	return meta.IsStatusConditionTrue(instance.Status.Conditions, korifiv1alpha1.ProvisioningFailedCondition)
 }
-
-func isReady(instance *korifiv1alpha1.CFServiceInstance) bool {
-	return meta.IsStatusConditionTrue(instance.Status.Conditions, korifiv1alpha1.StatusConditionReady)
-}
diff --git a/controllers/controllers/services/instances/managed/controller_test.go b/controllers/controllers/services/instances/managed/controller_test.go
index 256641fb..d1537a79 100644
--- a/controllers/controllers/services/instances/managed/controller_test.go
+++ b/controllers/controllers/services/instances/managed/controller_test.go
@@ -793,21 +793,11 @@ var _ = Describe("CFServiceInstance", func() {
 		JustBeforeEach(func() {
 			Eventually(func(g Gomega) {
 				g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed())
-				g.Expect(instance.Status.Conditions).To(ContainElement(SatisfyAll(
-					HasType(Equal(korifiv1alpha1.StatusConditionReady)),
-					HasStatus(Equal(metav1.ConditionTrue)),
-				)))
 				g.Expect(instance.Status.Provisioned).To(BeTrue())
 			}).Should(Succeed())
 
 			Expect(k8s.Patch(ctx, adminClient, instance, func() {
 				instance.Spec.PlanGUID = servicePlan2.Name
-				meta.SetStatusCondition(&instance.Status.Conditions, metav1.Condition{
-					Type:    korifiv1alpha1.StatusConditionReady,
-					Status:  metav1.ConditionFalse,
-					Reason:  "UpdateRequested",
-					Message: "managed service instance update is requested",
-				})
 			})).To(Succeed())
 		})
 
@@ -823,6 +813,20 @@ var _ = Describe("CFServiceInstance", func() {
 			}).Should(Succeed())
 		})
 
+		It("sends an update request to the broker", func() {
+			Eventually(func(g Gomega) {
+				g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">=", 1))
+				_, actualUpdatePayload := brokerClient.UpdateArgsForCall(brokerClient.UpdateCallCount() - 1)
+				g.Expect(actualUpdatePayload).To(Equal(osbapi.UpdatePayload{
+					InstanceID: instance.Name,
+					UpdateRequest: osbapi.UpdateRequest{
+						ServiceId: "service-offering-id",
+						PlanID:    "service-plan-id-2",
+					},
+				}))
+			}).Should(Succeed())
+		})
+
 		When("service update fails with recoverable error", func() {
 			BeforeEach(func() {
 				brokerClient.UpdateReturns(osbapi.UpdateResponse{}, errors.New("update-failed"))
@@ -831,7 +835,7 @@ var _ = Describe("CFServiceInstance", func() {
 			It("keeps trying to update the instance", func() {
 				Eventually(func(g Gomega) {
 					g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">=", 1))
-					_, updatePayload := brokerClient.UpdateArgsForCall(1)
+					_, updatePayload := brokerClient.UpdateArgsForCall(brokerClient.UpdateCallCount() - 1)
 					g.Expect(updatePayload).To(Equal(osbapi.UpdatePayload{
 						InstanceID: instance.Name,
 						UpdateRequest: osbapi.UpdateRequest{

While this diff ensures the broker's update method is invoked, it breaks existing tests. The reason is that when a k8s object is being created, it could be reconciled a couple of times, hence a provisioned service could yield a noop update with the broker. While this should not be an issue from OSBAPI spec point of view (still, no idea how real-life brokers react to noop-updates), this has the consequence that when a service instance is created, its last operation is reported as update rather than create.

If we are sure brokers do not mind noop-updates, we could probably ignore the effect above as an "usability issue" within this PR. Still, existing tests should be adapted.

If we want to solve the issue and avoid noop updates with the broker, I guess we need to replicate fields that can be updated to the status. Thus the values on the spec would have the semantic of "desired state", while the ones in the status would mean "actual state". The user sets the field on the spec, while the controller communicates the "desired state" with the broker and updates the status ("actual state") depending on the outcome.
Having those fields in the status would let the controller only perform broker updates that are noop. The criteria of an update with the broker would be desired-state != actual_state

Now, the question is what fields from the spec we should put on the status. According to the docs, those are plan, tags and parameters. I think that for the scope of this PR we could just add the plan guid

danail-branekov · 2026-03-05T11:21:30Z

Now that the ready state is not terminal (i.e. the service instance can be updated), we should no longer return if the instance is ready.

As a matter of fact this check returns whenever the service instance has been provisioned. As a result, the reconciler sees the plan change, but as the service is ready, it does not talk to the broker.

The isReady check should be removed

danail-branekov · 2026-03-05T11:23:39Z

+		return r.processUpdateOperation(serviceInstance, lastOpResponse)
+	}
+
+	serviceInstance.Status.Provisioned = true


Status.Provisioned should be only set when the instance is provisioned. Setting it here as well does not make sense.

danail-branekov · 2026-03-05T11:25:28Z

+		JustBeforeEach(func() {
+			Eventually(func(g Gomega) {
+				g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed())
+				g.Expect(instance.Status.Conditions).To(ContainElement(SatisfyAll(


The idea of this eventually should be to wait for the service to become provisioned before updating it. Provided that the ready state is no longer the terminal one, this check is now flaky. Instead, here you could just wait for the Status.Provisioned field to become true

danail-branekov · 2026-03-05T11:29:08Z

+
+			Expect(k8s.Patch(ctx, adminClient, instance, func() {
+				instance.Spec.PlanGUID = servicePlan2.Name
+				meta.SetStatusCondition(&instance.Status.Conditions, metav1.Condition{


Setting the status here jumps over the isReady check in the controller thus hiding the bug that the controller is not talking to the broker on update. When the service instance is being updated, the instance repository just sets Spec.PlanGUID to the new value and assumes that the controller kicks in. Setting the status from the test code in JustBeforeEach (the function that simulates the user updating the service instance) makes no sense here.

ybykov-a9s · 2026-03-30T09:19:05Z

@danail-branekov A huge thank you for your review. I applied changes you proposed. I also fixed broken tests you mentioned. Added a logic which prevents no-op broker requests. It shouldn't be an issue for the broker, but IHMO it's better to avoid such behaviour. I also added some logic which leaves last operation create for service instance creation. TBH it would be disappointing to see something like update successful after a successful creation operation.

I also added a smoke-test you mentioned.

danail-branekov · 2026-04-02T06:26:01Z

+
+	// The generation at which the instance was last successfully reconciled with the broker.
+	//+kubebuilder:validation:Optional
+	BrokerReconciledGeneration int64 `json:"brokerReconciledGeneration,omitempty"`


I had a look at the usages of this field and from what I see, it is only used in the controller to decide whether to perform update.

From my point of view this pattern is quite imperative and does not convey the intention. Also, relying on generation to filter out noop broker updates would probably not work - a generation could change in case e.g. a label is added to the CFServiceInstance - that is not a change a broker is interested in.

Would it not be better if we had a needsUpdate function in the controller instead that would (for now) return true only if spec.PlanGUID != status.PlanGUID. Then, the controller proceeds with updating the instance with the broker only if needsUpdate returns true
Such an approach would also remove setting the field all over the place and would be easier to reason about.

Speaking of avoiding unnecessary updates with the broker - you could introduce a test in the controller tests that verify that no broker communication is performed when something irrelevant has changed, e.g. a label has been added to the CFServiceInstance

danail-branekov · 2026-04-02T06:29:13Z

 	}

-	if !planVisible {
+	if !planVisible && !serviceInstance.Status.Provisioned {


I guess the intention here is not to set failed status if the service has been provisioned but the plan has been disabled after that. Sounds reasonable, I did not see a test about that case though.

Provided the check only makes sense for unprovisioned services, I would suggest moving it to reconcileProvisionedServiceInstance

Actually, we need to check for plan visibility on update as well. If the new desired plan is not visible, then the update should fail

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

ybykov-a9s and others added 14 commits February 23, 2026 14:31

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

6080d7d

…endpoint

Update managed service payloads instance checks

519c99e

Improve managed service instance handler tests

ab0a5e1

Cover repositories/PatchManagedServiceInstance with tests

10a5434

Update cfcerviceinstance_types comments for PlanGUID

0a1ac06

Update cfcerviceinstance_types comments for PlanGUID

5e0bd7a

Update managed service controller failing reason to ServiceInstanceFa…

a3c00fc

…iled

Update provisioned status field for managed instance

309185d

Clean up setObservedGeneration method in managed service controller

b1a9a0d

Managed controller allow consequent retries on UpdateFailedCondition

c471fba

Managed controller refine plan change test

5bd8b84

Managed controller tests refine test names

cc66a02

Managed controller tests remove extraneous ServicePlan2 creation check

ee03e3c

Managed controller tests remove extraneous ObservedGeneration field c…

7808778

…heck

combine some managed service update e2e plans

2932729

danail-branekov reviewed Feb 27, 2026

View reviewed changes

ybykov-a9s added 11 commits March 2, 2026 08:44

Revert some unneeded changes in api/payloads

6afbcac

Simplify ToManagedSIPatchMessage method in api/payloads

afa5310

Revert some unneeded changes in api/handlers

735ea65

Inline tests api/repositories managed service instance creation

b2db415

Split api/repositories patch service instance tests

138672c

Add api/repositories patch service instance api forbidden test

b2aceec

Rename isFailed method in controllers/managed

70adff8

Simplify controllers/managed Reconciler code

8507a89

controllers/managed stop reconsiling on UpdateFailed

73118cd

controllers/managed move servicePlan2 tests var to its scope

95d3b40

Rearrange e2e tests for updating managed services

f1ee843

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

9aa1147

…endpoint

ybykov-a9s marked this pull request as ready for review March 3, 2026 15:14

danail-branekov reviewed Mar 4, 2026

View reviewed changes

ybykov-a9s added 3 commits March 5, 2026 08:30

Suppress linter errors in a managed controller

97e98f4

Fix repo service instance unit tests

ee8aa6a

Remove unneeded await check from repositories test

1509139

danail-branekov reviewed Mar 5, 2026

View reviewed changes

ybykov-a9s added 2 commits March 30, 2026 11:10

Apply proposed fixes for managed services and their tests

4a14aab

Introduce a smoke-test for 'cf update-service' command

d506b76

danail-branekov reviewed Apr 2, 2026

View reviewed changes

		@@ -148,7 +148,7 @@ func (r Reconciler) ReconcileResource(ctx context.Context, serviceInstance kor
		}

		if isFailed(serviceInstance) {

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!

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

danail-branekov Sep 17, 2025 •

edited

Loading

danail-branekov left a comment •

edited

Loading

danail-branekov Jan 30, 2026 •

edited

Loading