From 2c75f08c088eb7a036eb8cadb1ee4233ecc4c3f1 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Wed, 10 Sep 2025 16:49:07 +0200 Subject: [PATCH 01/47] Split PATCH /v3/service_instances logic into UPSI and Managed Implement UPDATE in osbapi --- .../fake/cfservice_instance_repository.go | 159 +++++++++++++----- api/handlers/service_instance.go | 19 ++- api/handlers/service_instance_test.go | 10 +- api/payloads/service_instance.go | 29 +++- api/payloads/service_instance_test.go | 2 +- .../service_instance_repository.go | 59 ++++++- .../service_instance_repository_test.go | 6 +- .../api/v1alpha1/cfserviceinstance_types.go | 1 + .../services/instances/managed/controller.go | 111 ++++++++++-- .../controllers/services/osbapi/client.go | 34 ++++ .../services/osbapi/clientfactory.go | 1 + .../services/osbapi/fake/broker_client.go | 79 +++++++++ .../controllers/services/osbapi/types.go | 15 ++ tests/e2e/apps_test.go | 8 +- tests/e2e/e2e_suite_test.go | 2 +- tests/e2e/service_bindings_test.go | 6 +- tests/e2e/service_instances_test.go | 18 +- tests/e2e/spaces_test.go | 2 +- 18 files changed, 474 insertions(+), 87 deletions(-) diff --git a/api/handlers/fake/cfservice_instance_repository.go b/api/handlers/fake/cfservice_instance_repository.go index 85dab06a4..9cf21f066 100644 --- a/api/handlers/fake/cfservice_instance_repository.go +++ b/api/handlers/fake/cfservice_instance_repository.go @@ -101,18 +101,33 @@ type CFServiceInstanceRepository struct { result1 repositories.ListResult[repositories.ServiceInstanceRecord] result2 error } - PatchServiceInstanceStub func(context.Context, authorization.Info, repositories.PatchServiceInstanceMessage) (repositories.ServiceInstanceRecord, error) - patchServiceInstanceMutex sync.RWMutex - patchServiceInstanceArgsForCall []struct { + PatchManagedServiceInstanceStub func(context.Context, authorization.Info, repositories.PatchManagedSIMessage) (repositories.ServiceInstanceRecord, error) + patchManagedServiceInstanceMutex sync.RWMutex + patchManagedServiceInstanceArgsForCall []struct { arg1 context.Context arg2 authorization.Info - arg3 repositories.PatchServiceInstanceMessage + arg3 repositories.PatchManagedSIMessage } - patchServiceInstanceReturns struct { + patchManagedServiceInstanceReturns struct { result1 repositories.ServiceInstanceRecord result2 error } - patchServiceInstanceReturnsOnCall map[int]struct { + patchManagedServiceInstanceReturnsOnCall map[int]struct { + result1 repositories.ServiceInstanceRecord + result2 error + } + PatchUserProvidedServiceInstanceStub func(context.Context, authorization.Info, repositories.PatchUPSIMessage) (repositories.ServiceInstanceRecord, error) + patchUserProvidedServiceInstanceMutex sync.RWMutex + patchUserProvidedServiceInstanceArgsForCall []struct { + arg1 context.Context + arg2 authorization.Info + arg3 repositories.PatchUPSIMessage + } + patchUserProvidedServiceInstanceReturns struct { + result1 repositories.ServiceInstanceRecord + result2 error + } + patchUserProvidedServiceInstanceReturnsOnCall map[int]struct { result1 repositories.ServiceInstanceRecord result2 error } @@ -516,18 +531,84 @@ func (fake *CFServiceInstanceRepository) ListServiceInstancesReturnsOnCall(i int }{result1, result2} } -func (fake *CFServiceInstanceRepository) PatchServiceInstance(arg1 context.Context, arg2 authorization.Info, arg3 repositories.PatchServiceInstanceMessage) (repositories.ServiceInstanceRecord, error) { - fake.patchServiceInstanceMutex.Lock() - ret, specificReturn := fake.patchServiceInstanceReturnsOnCall[len(fake.patchServiceInstanceArgsForCall)] - fake.patchServiceInstanceArgsForCall = append(fake.patchServiceInstanceArgsForCall, struct { +func (fake *CFServiceInstanceRepository) PatchManagedServiceInstance(arg1 context.Context, arg2 authorization.Info, arg3 repositories.PatchManagedSIMessage) (repositories.ServiceInstanceRecord, error) { + fake.patchManagedServiceInstanceMutex.Lock() + ret, specificReturn := fake.patchManagedServiceInstanceReturnsOnCall[len(fake.patchManagedServiceInstanceArgsForCall)] + fake.patchManagedServiceInstanceArgsForCall = append(fake.patchManagedServiceInstanceArgsForCall, struct { + arg1 context.Context + arg2 authorization.Info + arg3 repositories.PatchManagedSIMessage + }{arg1, arg2, arg3}) + stub := fake.PatchManagedServiceInstanceStub + fakeReturns := fake.patchManagedServiceInstanceReturns + fake.recordInvocation("PatchManagedServiceInstance", []interface{}{arg1, arg2, arg3}) + fake.patchManagedServiceInstanceMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *CFServiceInstanceRepository) PatchManagedServiceInstanceCallCount() int { + fake.patchManagedServiceInstanceMutex.RLock() + defer fake.patchManagedServiceInstanceMutex.RUnlock() + return len(fake.patchManagedServiceInstanceArgsForCall) +} + +func (fake *CFServiceInstanceRepository) PatchManagedServiceInstanceCalls(stub func(context.Context, authorization.Info, repositories.PatchManagedSIMessage) (repositories.ServiceInstanceRecord, error)) { + fake.patchManagedServiceInstanceMutex.Lock() + defer fake.patchManagedServiceInstanceMutex.Unlock() + fake.PatchManagedServiceInstanceStub = stub +} + +func (fake *CFServiceInstanceRepository) PatchManagedServiceInstanceArgsForCall(i int) (context.Context, authorization.Info, repositories.PatchManagedSIMessage) { + fake.patchManagedServiceInstanceMutex.RLock() + defer fake.patchManagedServiceInstanceMutex.RUnlock() + argsForCall := fake.patchManagedServiceInstanceArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 +} + +func (fake *CFServiceInstanceRepository) PatchManagedServiceInstanceReturns(result1 repositories.ServiceInstanceRecord, result2 error) { + fake.patchManagedServiceInstanceMutex.Lock() + defer fake.patchManagedServiceInstanceMutex.Unlock() + fake.PatchManagedServiceInstanceStub = nil + fake.patchManagedServiceInstanceReturns = struct { + result1 repositories.ServiceInstanceRecord + result2 error + }{result1, result2} +} + +func (fake *CFServiceInstanceRepository) PatchManagedServiceInstanceReturnsOnCall(i int, result1 repositories.ServiceInstanceRecord, result2 error) { + fake.patchManagedServiceInstanceMutex.Lock() + defer fake.patchManagedServiceInstanceMutex.Unlock() + fake.PatchManagedServiceInstanceStub = nil + if fake.patchManagedServiceInstanceReturnsOnCall == nil { + fake.patchManagedServiceInstanceReturnsOnCall = make(map[int]struct { + result1 repositories.ServiceInstanceRecord + result2 error + }) + } + fake.patchManagedServiceInstanceReturnsOnCall[i] = struct { + result1 repositories.ServiceInstanceRecord + result2 error + }{result1, result2} +} + +func (fake *CFServiceInstanceRepository) PatchUserProvidedServiceInstance(arg1 context.Context, arg2 authorization.Info, arg3 repositories.PatchUPSIMessage) (repositories.ServiceInstanceRecord, error) { + fake.patchUserProvidedServiceInstanceMutex.Lock() + ret, specificReturn := fake.patchUserProvidedServiceInstanceReturnsOnCall[len(fake.patchUserProvidedServiceInstanceArgsForCall)] + fake.patchUserProvidedServiceInstanceArgsForCall = append(fake.patchUserProvidedServiceInstanceArgsForCall, struct { arg1 context.Context arg2 authorization.Info - arg3 repositories.PatchServiceInstanceMessage + arg3 repositories.PatchUPSIMessage }{arg1, arg2, arg3}) - stub := fake.PatchServiceInstanceStub - fakeReturns := fake.patchServiceInstanceReturns - fake.recordInvocation("PatchServiceInstance", []interface{}{arg1, arg2, arg3}) - fake.patchServiceInstanceMutex.Unlock() + stub := fake.PatchUserProvidedServiceInstanceStub + fakeReturns := fake.patchUserProvidedServiceInstanceReturns + fake.recordInvocation("PatchUserProvidedServiceInstance", []interface{}{arg1, arg2, arg3}) + fake.patchUserProvidedServiceInstanceMutex.Unlock() if stub != nil { return stub(arg1, arg2, arg3) } @@ -537,46 +618,46 @@ func (fake *CFServiceInstanceRepository) PatchServiceInstance(arg1 context.Conte return fakeReturns.result1, fakeReturns.result2 } -func (fake *CFServiceInstanceRepository) PatchServiceInstanceCallCount() int { - fake.patchServiceInstanceMutex.RLock() - defer fake.patchServiceInstanceMutex.RUnlock() - return len(fake.patchServiceInstanceArgsForCall) +func (fake *CFServiceInstanceRepository) PatchUserProvidedServiceInstanceCallCount() int { + fake.patchUserProvidedServiceInstanceMutex.RLock() + defer fake.patchUserProvidedServiceInstanceMutex.RUnlock() + return len(fake.patchUserProvidedServiceInstanceArgsForCall) } -func (fake *CFServiceInstanceRepository) PatchServiceInstanceCalls(stub func(context.Context, authorization.Info, repositories.PatchServiceInstanceMessage) (repositories.ServiceInstanceRecord, error)) { - fake.patchServiceInstanceMutex.Lock() - defer fake.patchServiceInstanceMutex.Unlock() - fake.PatchServiceInstanceStub = stub +func (fake *CFServiceInstanceRepository) PatchUserProvidedServiceInstanceCalls(stub func(context.Context, authorization.Info, repositories.PatchUPSIMessage) (repositories.ServiceInstanceRecord, error)) { + fake.patchUserProvidedServiceInstanceMutex.Lock() + defer fake.patchUserProvidedServiceInstanceMutex.Unlock() + fake.PatchUserProvidedServiceInstanceStub = stub } -func (fake *CFServiceInstanceRepository) PatchServiceInstanceArgsForCall(i int) (context.Context, authorization.Info, repositories.PatchServiceInstanceMessage) { - fake.patchServiceInstanceMutex.RLock() - defer fake.patchServiceInstanceMutex.RUnlock() - argsForCall := fake.patchServiceInstanceArgsForCall[i] +func (fake *CFServiceInstanceRepository) PatchUserProvidedServiceInstanceArgsForCall(i int) (context.Context, authorization.Info, repositories.PatchUPSIMessage) { + fake.patchUserProvidedServiceInstanceMutex.RLock() + defer fake.patchUserProvidedServiceInstanceMutex.RUnlock() + argsForCall := fake.patchUserProvidedServiceInstanceArgsForCall[i] return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 } -func (fake *CFServiceInstanceRepository) PatchServiceInstanceReturns(result1 repositories.ServiceInstanceRecord, result2 error) { - fake.patchServiceInstanceMutex.Lock() - defer fake.patchServiceInstanceMutex.Unlock() - fake.PatchServiceInstanceStub = nil - fake.patchServiceInstanceReturns = struct { +func (fake *CFServiceInstanceRepository) PatchUserProvidedServiceInstanceReturns(result1 repositories.ServiceInstanceRecord, result2 error) { + fake.patchUserProvidedServiceInstanceMutex.Lock() + defer fake.patchUserProvidedServiceInstanceMutex.Unlock() + fake.PatchUserProvidedServiceInstanceStub = nil + fake.patchUserProvidedServiceInstanceReturns = struct { result1 repositories.ServiceInstanceRecord result2 error }{result1, result2} } -func (fake *CFServiceInstanceRepository) PatchServiceInstanceReturnsOnCall(i int, result1 repositories.ServiceInstanceRecord, result2 error) { - fake.patchServiceInstanceMutex.Lock() - defer fake.patchServiceInstanceMutex.Unlock() - fake.PatchServiceInstanceStub = nil - if fake.patchServiceInstanceReturnsOnCall == nil { - fake.patchServiceInstanceReturnsOnCall = make(map[int]struct { +func (fake *CFServiceInstanceRepository) PatchUserProvidedServiceInstanceReturnsOnCall(i int, result1 repositories.ServiceInstanceRecord, result2 error) { + fake.patchUserProvidedServiceInstanceMutex.Lock() + defer fake.patchUserProvidedServiceInstanceMutex.Unlock() + fake.PatchUserProvidedServiceInstanceStub = nil + if fake.patchUserProvidedServiceInstanceReturnsOnCall == nil { + fake.patchUserProvidedServiceInstanceReturnsOnCall = make(map[int]struct { result1 repositories.ServiceInstanceRecord result2 error }) } - fake.patchServiceInstanceReturnsOnCall[i] = struct { + fake.patchUserProvidedServiceInstanceReturnsOnCall[i] = struct { result1 repositories.ServiceInstanceRecord result2 error }{result1, result2} diff --git a/api/handlers/service_instance.go b/api/handlers/service_instance.go index 0a57e89d1..6c7eb46f9 100644 --- a/api/handlers/service_instance.go +++ b/api/handlers/service_instance.go @@ -30,7 +30,8 @@ const ( type CFServiceInstanceRepository interface { CreateUserProvidedServiceInstance(context.Context, authorization.Info, repositories.CreateUPSIMessage) (repositories.ServiceInstanceRecord, error) CreateManagedServiceInstance(context.Context, authorization.Info, repositories.CreateManagedSIMessage) (repositories.ServiceInstanceRecord, error) - PatchServiceInstance(context.Context, authorization.Info, repositories.PatchServiceInstanceMessage) (repositories.ServiceInstanceRecord, error) + PatchUserProvidedServiceInstance(context.Context, authorization.Info, repositories.PatchUPSIMessage) (repositories.ServiceInstanceRecord, error) + PatchManagedServiceInstance(context.Context, authorization.Info, repositories.PatchManagedSIMessage) (repositories.ServiceInstanceRecord, error) ListServiceInstances(context.Context, authorization.Info, repositories.ListServiceInstanceMessage) (repositories.ListResult[repositories.ServiceInstanceRecord], error) GetServiceInstance(context.Context, authorization.Info, string) (repositories.ServiceInstanceRecord, error) GetServiceInstanceCredentials(context.Context, authorization.Info, string) (map[string]any, error) @@ -185,10 +186,20 @@ func (h *ServiceInstance) patch(r *http.Request) (*routing.Response, error) { return nil, apierrors.LogAndReturn(logger, apierrors.ForbiddenAsNotFound(err), "failed to get service instance") } - patchMessage := payload.ToServiceInstancePatchMessage(serviceInstance.SpaceGUID, serviceInstance.GUID) - serviceInstance, err = h.serviceInstanceRepo.PatchServiceInstance(r.Context(), authInfo, patchMessage) + if payload.Type == korifiv1alpha1.ManagedType { + patchMessage := payload.ToManagedSIPatchMessage(serviceInstance.SpaceGUID, serviceInstance.GUID) + serviceInstance, err = h.serviceInstanceRepo.PatchManagedServiceInstance(r.Context(), authInfo, patchMessage) + if err != nil { + return nil, apierrors.LogAndReturn(logger, err, "failed to patch managed service instance") + } + + return routing.NewResponse(http.StatusOK).WithBody(presenter.ForServiceInstance(serviceInstance, h.serverURL)), nil + } + + patchMessage := payload.ToUPSIPatchMessage(serviceInstance.SpaceGUID, serviceInstance.GUID) + serviceInstance, err = h.serviceInstanceRepo.PatchUserProvidedServiceInstance(r.Context(), authInfo, patchMessage) if err != nil { - return nil, apierrors.LogAndReturn(logger, err, "failed to patch service instance") + return nil, apierrors.LogAndReturn(logger, err, "failed to patch user provided service instance") } return routing.NewResponse(http.StatusOK).WithBody(presenter.ForServiceInstance(serviceInstance, h.serverURL)), nil diff --git a/api/handlers/service_instance_test.go b/api/handlers/service_instance_test.go index 431e3f057..457c98e26 100644 --- a/api/handlers/service_instance_test.go +++ b/api/handlers/service_instance_test.go @@ -846,7 +846,7 @@ var _ = Describe("ServiceInstance", func() { }, }) - serviceInstanceRepo.PatchServiceInstanceReturns(repositories.ServiceInstanceRecord{ + serviceInstanceRepo.PatchUserProvidedServiceInstanceReturns(repositories.ServiceInstanceRecord{ Name: "new-name", GUID: "service-instance-guid", }, nil) @@ -865,10 +865,10 @@ var _ = Describe("ServiceInstance", func() { Expect(actualAuthInfo).To(Equal(authInfo)) Expect(actualGUID).To(Equal("service-instance-guid")) - Expect(serviceInstanceRepo.PatchServiceInstanceCallCount()).To(Equal(1)) - _, actualAuthInfo, patchMessage := serviceInstanceRepo.PatchServiceInstanceArgsForCall(0) + Expect(serviceInstanceRepo.PatchUserProvidedServiceInstanceCallCount()).To(Equal(1)) + _, actualAuthInfo, patchMessage := serviceInstanceRepo.PatchUserProvidedServiceInstanceArgsForCall(0) Expect(actualAuthInfo).To(Equal(authInfo)) - Expect(patchMessage).To(Equal(repositories.PatchServiceInstanceMessage{ + Expect(patchMessage).To(Equal(repositories.PatchUPSIMessage{ GUID: "service-instance-guid", SpaceGUID: "space-guid", Name: tools.PtrTo("new-name"), @@ -927,7 +927,7 @@ var _ = Describe("ServiceInstance", func() { When("patching the service instances fails", func() { BeforeEach(func() { - serviceInstanceRepo.PatchServiceInstanceReturns(repositories.ServiceInstanceRecord{}, errors.New("oops")) + serviceInstanceRepo.PatchUserProvidedServiceInstanceReturns(repositories.ServiceInstanceRecord{}, errors.New("oops")) }) It("returns the error", func() { diff --git a/api/payloads/service_instance.go b/api/payloads/service_instance.go index 9f92158e3..7f4c3f810 100644 --- a/api/payloads/service_instance.go +++ b/api/payloads/service_instance.go @@ -171,10 +171,12 @@ func (g *ServiceInstanceGet) DecodeFromURLValues(values url.Values) error { } type ServiceInstancePatch struct { - Name *string `json:"name,omitempty"` - Tags *[]string `json:"tags,omitempty"` - Credentials *map[string]any `json:"credentials,omitempty"` - Metadata MetadataPatch `json:"metadata"` + Name *string `json:"name,omitempty"` + Type string `json:"type"` + Tags *[]string `json:"tags,omitempty"` + Credentials *map[string]any `json:"credentials,omitempty"` + Relationships *ServiceInstanceRelationships `json:"relationships,omitempty"` + Metadata MetadataPatch `json:"metadata"` } func (p ServiceInstancePatch) Validate() error { @@ -183,9 +185,24 @@ func (p ServiceInstancePatch) Validate() error { ) } -func (p ServiceInstancePatch) ToServiceInstancePatchMessage(spaceGUID, appGUID string) repositories.PatchServiceInstanceMessage { - return repositories.PatchServiceInstanceMessage{ +func (p ServiceInstancePatch) ToUPSIPatchMessage(spaceGUID, appGUID string) repositories.PatchUPSIMessage { + return repositories.PatchUPSIMessage{ + SpaceGUID: spaceGUID, + GUID: appGUID, + Name: p.Name, + Credentials: p.Credentials, + Tags: p.Tags, + MetadataPatch: repositories.MetadataPatch{ + Labels: p.Metadata.Labels, + Annotations: p.Metadata.Annotations, + }, + } +} + +func (p ServiceInstancePatch) ToManagedSIPatchMessage(spaceGUID, appGUID string) repositories.PatchManagedSIMessage { + return repositories.PatchManagedSIMessage{ SpaceGUID: spaceGUID, + PlanGUID: p.Relationships.ServicePlan.Data.GUID, GUID: appGUID, Name: p.Name, Credentials: p.Credentials, diff --git a/api/payloads/service_instance_test.go b/api/payloads/service_instance_test.go index 589274008..0863f019e 100644 --- a/api/payloads/service_instance_test.go +++ b/api/payloads/service_instance_test.go @@ -533,7 +533,7 @@ var _ = Describe("ServiceInstancePatch", func() { Context("ToServiceInstancePatchMessage", func() { It("converts to repo message correctly", func() { - msg := serviceInstancePatch.ToServiceInstancePatchMessage("space-guid", "app-guid") + msg := serviceInstancePatch.ToUPSIPatchMessage("space-guid", "app-guid") Expect(msg.SpaceGUID).To(Equal("space-guid")) Expect(msg.GUID).To(Equal("app-guid")) Expect(msg.Name).To(PointTo(Equal("service-instance-name"))) diff --git a/api/repositories/service_instance_repository.go b/api/repositories/service_instance_repository.go index 3d1f62eee..0644a3a6b 100644 --- a/api/repositories/service_instance_repository.go +++ b/api/repositories/service_instance_repository.go @@ -68,7 +68,7 @@ type CreateManagedSIMessage struct { Annotations map[string]string } -type PatchServiceInstanceMessage struct { +type PatchUPSIMessage struct { GUID string SpaceGUID string Name *string @@ -77,7 +77,27 @@ type PatchServiceInstanceMessage struct { MetadataPatch } -func (p PatchServiceInstanceMessage) Apply(cfServiceInstance *korifiv1alpha1.CFServiceInstance) { +type PatchManagedSIMessage struct { + GUID string + SpaceGUID string + PlanGUID string + Name *string + Credentials *map[string]any + Tags *[]string + MetadataPatch +} + +func (p PatchUPSIMessage) Apply(cfServiceInstance *korifiv1alpha1.CFServiceInstance) { + if p.Name != nil { + cfServiceInstance.Spec.DisplayName = *p.Name + } + if p.Tags != nil { + cfServiceInstance.Spec.Tags = *p.Tags + } + p.MetadataPatch.Apply(cfServiceInstance) +} + +func (p PatchManagedSIMessage) Apply(cfServiceInstance *korifiv1alpha1.CFServiceInstance) { if p.Name != nil { cfServiceInstance.Spec.DisplayName = *p.Name } @@ -274,7 +294,40 @@ func (r *ServiceInstanceRepo) servicePlanVisible(ctx context.Context, planGUID s return slices.Contains(servicePlan.Spec.Visibility.Organizations, space.Namespace), nil } -func (r *ServiceInstanceRepo) PatchServiceInstance(ctx context.Context, authInfo authorization.Info, message PatchServiceInstanceMessage) (ServiceInstanceRecord, error) { +func (r *ServiceInstanceRepo) PatchUserProvidedServiceInstance(ctx context.Context, authInfo authorization.Info, message PatchUPSIMessage) (ServiceInstanceRecord, error) { + cfServiceInstance := &korifiv1alpha1.CFServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: message.SpaceGUID, + Name: message.GUID, + }, + } + if err := r.klient.Get(ctx, cfServiceInstance); err != nil { + return ServiceInstanceRecord{}, apierrors.FromK8sError(err, ServiceInstanceResourceType) + } + + err := r.klient.Patch(ctx, cfServiceInstance, func() error { + message.Apply(cfServiceInstance) + return nil + }) + if err != nil { + return ServiceInstanceRecord{}, apierrors.FromK8sError(err, ServiceInstanceResourceType) + } + + if message.Credentials != nil { + cfServiceInstance, err = r.migrateLegacyCredentials(ctx, cfServiceInstance) + if err != nil { + return ServiceInstanceRecord{}, err + } + err = r.patchCredentialsSecret(ctx, cfServiceInstance, *message.Credentials) + if err != nil { + return ServiceInstanceRecord{}, apierrors.FromK8sError(err, ServiceInstanceResourceType) + } + } + + return cfServiceInstanceToRecord(*cfServiceInstance), nil +} + +func (r *ServiceInstanceRepo) PatchManagedServiceInstance(ctx context.Context, authInfo authorization.Info, message PatchManagedSIMessage) (ServiceInstanceRecord, error) { cfServiceInstance := &korifiv1alpha1.CFServiceInstance{ ObjectMeta: metav1.ObjectMeta{ Namespace: message.SpaceGUID, diff --git a/api/repositories/service_instance_repository_test.go b/api/repositories/service_instance_repository_test.go index 35401f69f..c06e9c27a 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -504,7 +504,7 @@ var _ = Describe("ServiceInstanceRepository", func() { cfServiceInstance *korifiv1alpha1.CFServiceInstance secret *corev1.Secret serviceInstanceRecord repositories.ServiceInstanceRecord - patchMessage repositories.PatchServiceInstanceMessage + patchMessage repositories.PatchUPSIMessage err error ) @@ -528,7 +528,7 @@ var _ = Describe("ServiceInstanceRepository", func() { } Expect(k8sClient.Create(ctx, secret)).To(Succeed()) - patchMessage = repositories.PatchServiceInstanceMessage{ + patchMessage = repositories.PatchUPSIMessage{ GUID: cfServiceInstance.Name, SpaceGUID: space.Name, Name: tools.PtrTo("new-name"), @@ -542,7 +542,7 @@ var _ = Describe("ServiceInstanceRepository", func() { }) JustBeforeEach(func() { - serviceInstanceRecord, err = serviceInstanceRepo.PatchServiceInstance(ctx, authInfo, patchMessage) + serviceInstanceRecord, err = serviceInstanceRepo.PatchUserProvidedServiceInstance(ctx, authInfo, patchMessage) }) When("authorized in the space", func() { diff --git a/controllers/api/v1alpha1/cfserviceinstance_types.go b/controllers/api/v1alpha1/cfserviceinstance_types.go index 020a65eb1..f25d3ef3c 100644 --- a/controllers/api/v1alpha1/cfserviceinstance_types.go +++ b/controllers/api/v1alpha1/cfserviceinstance_types.go @@ -33,6 +33,7 @@ const ( DeprovisionWithoutBrokerAnnotation = "korifi.cloudfoundry.org/deprovision-without-broker" ProvisioningFailedCondition = "ProvisioningFailed" + UpdateFailedCondition = "UpdateFailed" DeprovisioningFailedCondition = "DeprovisioningFailed" ) diff --git a/controllers/controllers/services/instances/managed/controller.go b/controllers/controllers/services/instances/managed/controller.go index 3b73f6403..192908d47 100644 --- a/controllers/controllers/services/instances/managed/controller.go +++ b/controllers/controllers/services/instances/managed/controller.go @@ -122,7 +122,6 @@ func (r *Reconciler) isManaged(object client.Object) bool { func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *korifiv1alpha1.CFServiceInstance) (ctrl.Result, error) { log := logr.FromContextOrDiscard(ctx) - serviceInstance.Status.ObservedGeneration = serviceInstance.Generation log.V(1).Info("set observed generation", "generation", serviceInstance.Status.ObservedGeneration) if !serviceInstance.GetDeletionTimestamp().IsZero() { @@ -166,18 +165,33 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor serviceInstance.Spec.ServiceLabel = tools.PtrTo(serviceInstanceAssets.ServiceOffering.Spec.Name) } - provisionResponse, err := r.provisionServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient) - if err != nil { - 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 { + provisionResponse, err := r.provisionServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient) + if err != nil { + log.Error(err, "failed to provision service instance") + return ctrl.Result{}, fmt.Errorf("failed to provision service instance: %w", err) + } - if provisionResponse.IsAsync { - lastOpResponse, err := r.pollLastOperation(ctx, serviceInstance, serviceInstanceAssets, osbapiClient, provisionResponse.Operation) + if provisionResponse.IsAsync { + lastOpResponse, err := r.pollLastOperation(ctx, serviceInstance, serviceInstanceAssets, osbapiClient, provisionResponse.Operation) + if err != nil { + return ctrl.Result{}, err + } + return r.processProvisionOperation(serviceInstance, lastOpResponse) + } + } else { + updateResponse, err := r.updateServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient) if err != nil { - return ctrl.Result{}, err + log.Error(err, "failed to update service instance") + return ctrl.Result{}, fmt.Errorf("failed to update service instance: %w", err) + } + if updateResponse.IsAsync { + lastOpResponse, err := r.pollLastOperation(ctx, serviceInstance, serviceInstanceAssets, osbapiClient, updateResponse.Operation) + if err != nil { + return ctrl.Result{}, err + } + return r.processUpdateOperation(serviceInstance, lastOpResponse) } - return r.processProvisionOperation(serviceInstance, lastOpResponse) } serviceInstance.Status.MaintenanceInfo = serviceInstanceAssets.ServicePlan.Spec.MaintenanceInfo @@ -249,6 +263,7 @@ func (r *Reconciler) processProvisionOperation( lastOpResponse osbapi.LastOperationResponse, ) (ctrl.Result, error) { if lastOpResponse.State == "succeeded" { + setObservedGeneration(serviceInstance) return ctrl.Result{}, nil } @@ -261,16 +276,88 @@ func (r *Reconciler) processProvisionOperation( Reason: "ProvisionFailed", Message: lastOpResponse.Description, }) + setObservedGeneration(serviceInstance) return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ProvisionFailed") } return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ProvisionInProgress").WithRequeue() } +func (r *Reconciler) updateServiceInstance( + ctx context.Context, + serviceInstance *korifiv1alpha1.CFServiceInstance, + assets osbapi.ServiceInstanceAssets, + osbapiClient osbapi.BrokerClient, +) (osbapi.UpdateResponse, error) { + log := logr.FromContextOrDiscard(ctx).WithName("update-service-instance") + + serviceInstance.Status.LastOperation = korifiv1alpha1.LastOperation{ + Type: "update", + State: "in progress", + } + + var updateResponse osbapi.UpdateResponse + var err error + updateResponse, err = osbapiClient.Update(ctx, osbapi.UpdatePayload{ + InstanceID: serviceInstance.Name, + UpdateRequest: osbapi.UpdateRequest{ + ServiceId: assets.ServiceOffering.Spec.BrokerCatalog.ID, + PlanID: assets.ServicePlan.Spec.BrokerCatalog.ID, + }, + }) + if err != nil { + log.Error(err, "failed to update service") + + if osbapi.IsUnrecoveralbeError(err) { + serviceInstance.Status.LastOperation.State = "failed" + meta.SetStatusCondition(&serviceInstance.Status.Conditions, metav1.Condition{ + Type: korifiv1alpha1.UpdateFailedCondition, + Status: metav1.ConditionTrue, + ObservedGeneration: serviceInstance.Generation, + LastTransitionTime: metav1.NewTime(time.Now()), + Reason: "UpdateFailed", + Message: err.Error(), + }) + return osbapi.UpdateResponse{}, + k8s.NewNotReadyError().WithReason("UpdateFailed") + } + + return osbapi.UpdateResponse{}, err + } + + return updateResponse, nil +} + +func (r *Reconciler) processUpdateOperation( + serviceInstance *korifiv1alpha1.CFServiceInstance, + lastOpResponse osbapi.LastOperationResponse, +) (ctrl.Result, error) { + if lastOpResponse.State == "succeeded" { + setObservedGeneration(serviceInstance) + return ctrl.Result{}, nil + } + + if lastOpResponse.State == "failed" { + meta.SetStatusCondition(&serviceInstance.Status.Conditions, metav1.Condition{ + Type: korifiv1alpha1.ProvisioningFailedCondition, + Status: metav1.ConditionTrue, + ObservedGeneration: serviceInstance.Generation, + LastTransitionTime: metav1.NewTime(time.Now()), + Reason: "UpdateFailed", + Message: lastOpResponse.Description, + }) + setObservedGeneration(serviceInstance) + return ctrl.Result{}, k8s.NewNotReadyError().WithReason("UpdateFailed") + } + + return ctrl.Result{}, k8s.NewNotReadyError().WithReason("UpdateInProgress").WithRequeue() +} + func (r *Reconciler) finalize( ctx context.Context, serviceInstance *korifiv1alpha1.CFServiceInstance, ) (ctrl.Result, error) { + setObservedGeneration(serviceInstance) if !controllerutil.ContainsFinalizer(serviceInstance, korifiv1alpha1.CFServiceInstanceFinalizerName) { return ctrl.Result{}, nil } @@ -478,3 +565,7 @@ func isFailed(instance *korifiv1alpha1.CFServiceInstance) bool { func isReady(instance *korifiv1alpha1.CFServiceInstance) bool { return meta.IsStatusConditionTrue(instance.Status.Conditions, korifiv1alpha1.StatusConditionReady) } + +func setObservedGeneration(instance *korifiv1alpha1.CFServiceInstance) { + instance.Status.ObservedGeneration = instance.Generation +} diff --git a/controllers/controllers/services/osbapi/client.go b/controllers/controllers/services/osbapi/client.go index 5bbc9d89b..391b3a929 100644 --- a/controllers/controllers/services/osbapi/client.go +++ b/controllers/controllers/services/osbapi/client.go @@ -110,6 +110,40 @@ func (c *Client) Provision(ctx context.Context, payload ProvisionPayload) (Provi return response, nil } +func (c *Client) Update(ctx context.Context, payload UpdatePayload) (UpdateResponse, error) { + statusCode, respBytes, err := c.newBrokerRequester(). + forBroker(c.broker). + async(). + sendRequest( + ctx, + "/v2/service_instances/"+payload.InstanceID, + http.MethodPatch, + nil, + payload.UpdateRequest, + ) + if err != nil { + return UpdateResponse{}, fmt.Errorf("update request failed: %w", err) + } + if statusCode == http.StatusBadRequest || statusCode == http.StatusConflict || statusCode == http.StatusUnprocessableEntity { + return UpdateResponse{}, UnrecoverableError{Status: statusCode} + } + + if statusCode >= 300 { + return UpdateResponse{}, fmt.Errorf("update request failed with status code: %d", statusCode) + } + + response := UpdateResponse{ + IsAsync: statusCode == http.StatusAccepted, + } + + err = json.Unmarshal(respBytes, &response) + if err != nil { + return UpdateResponse{}, fmt.Errorf("failed to unmarshal response: %w", err) + } + + return response, nil +} + func (c *Client) Deprovision(ctx context.Context, payload DeprovisionPayload) (ProvisionResponse, error) { statusCode, respBytes, err := c.newBrokerRequester(). forBroker(c.broker). diff --git a/controllers/controllers/services/osbapi/clientfactory.go b/controllers/controllers/services/osbapi/clientfactory.go index b829cd4dc..be8df1a4f 100644 --- a/controllers/controllers/services/osbapi/clientfactory.go +++ b/controllers/controllers/services/osbapi/clientfactory.go @@ -17,6 +17,7 @@ import ( //counterfeiter:generate -o fake -fake-name BrokerClient code.cloudfoundry.org/korifi/controllers/controllers/services/osbapi.BrokerClient type BrokerClient interface { Provision(context.Context, ProvisionPayload) (ProvisionResponse, error) + Update(context.Context, UpdatePayload) (UpdateResponse, error) Deprovision(context.Context, DeprovisionPayload) (ProvisionResponse, error) GetServiceInstanceLastOperation(context.Context, GetInstanceLastOperationRequest) (LastOperationResponse, error) GetCatalog(context.Context) (Catalog, error) diff --git a/controllers/controllers/services/osbapi/fake/broker_client.go b/controllers/controllers/services/osbapi/fake/broker_client.go index 0c145a277..045b21f94 100644 --- a/controllers/controllers/services/osbapi/fake/broker_client.go +++ b/controllers/controllers/services/osbapi/fake/broker_client.go @@ -120,6 +120,20 @@ type BrokerClient struct { result1 osbapi.UnbindResponse result2 error } + UpdateStub func(context.Context, osbapi.UpdatePayload) (osbapi.UpdateResponse, error) + updateMutex sync.RWMutex + updateArgsForCall []struct { + arg1 context.Context + arg2 osbapi.UpdatePayload + } + updateReturns struct { + result1 osbapi.UpdateResponse + result2 error + } + updateReturnsOnCall map[int]struct { + result1 osbapi.UpdateResponse + result2 error + } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } @@ -643,6 +657,71 @@ func (fake *BrokerClient) UnbindReturnsOnCall(i int, result1 osbapi.UnbindRespon }{result1, result2} } +func (fake *BrokerClient) Update(arg1 context.Context, arg2 osbapi.UpdatePayload) (osbapi.UpdateResponse, error) { + fake.updateMutex.Lock() + ret, specificReturn := fake.updateReturnsOnCall[len(fake.updateArgsForCall)] + fake.updateArgsForCall = append(fake.updateArgsForCall, struct { + arg1 context.Context + arg2 osbapi.UpdatePayload + }{arg1, arg2}) + stub := fake.UpdateStub + fakeReturns := fake.updateReturns + fake.recordInvocation("Update", []interface{}{arg1, arg2}) + fake.updateMutex.Unlock() + if stub != nil { + return stub(arg1, arg2) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *BrokerClient) UpdateCallCount() int { + fake.updateMutex.RLock() + defer fake.updateMutex.RUnlock() + return len(fake.updateArgsForCall) +} + +func (fake *BrokerClient) UpdateCalls(stub func(context.Context, osbapi.UpdatePayload) (osbapi.UpdateResponse, error)) { + fake.updateMutex.Lock() + defer fake.updateMutex.Unlock() + fake.UpdateStub = stub +} + +func (fake *BrokerClient) UpdateArgsForCall(i int) (context.Context, osbapi.UpdatePayload) { + fake.updateMutex.RLock() + defer fake.updateMutex.RUnlock() + argsForCall := fake.updateArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *BrokerClient) UpdateReturns(result1 osbapi.UpdateResponse, result2 error) { + fake.updateMutex.Lock() + defer fake.updateMutex.Unlock() + fake.UpdateStub = nil + fake.updateReturns = struct { + result1 osbapi.UpdateResponse + result2 error + }{result1, result2} +} + +func (fake *BrokerClient) UpdateReturnsOnCall(i int, result1 osbapi.UpdateResponse, result2 error) { + fake.updateMutex.Lock() + defer fake.updateMutex.Unlock() + fake.UpdateStub = nil + if fake.updateReturnsOnCall == nil { + fake.updateReturnsOnCall = make(map[int]struct { + result1 osbapi.UpdateResponse + result2 error + }) + } + fake.updateReturnsOnCall[i] = struct { + result1 osbapi.UpdateResponse + result2 error + }{result1, result2} +} + func (fake *BrokerClient) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() diff --git a/controllers/controllers/services/osbapi/types.go b/controllers/controllers/services/osbapi/types.go index 524798769..95e5bcdb3 100644 --- a/controllers/controllers/services/osbapi/types.go +++ b/controllers/controllers/services/osbapi/types.go @@ -83,6 +83,21 @@ type ProvisionResponse struct { Operation string `json:"operation,omitempty"` } +type UpdatePayload struct { + InstanceID string + UpdateRequest +} + +type UpdateRequest struct { + ServiceId string `json:"service_id"` + PlanID string `json:"plan_id,omitempty"` +} + +type UpdateResponse struct { + IsAsync bool + Operation string `json:"operation,omitempty"` +} + type GetBindingRequest struct { InstanceID string BindingID string diff --git a/tests/e2e/apps_test.go b/tests/e2e/apps_test.go index bf35bf69e..c1ab7abf0 100644 --- a/tests/e2e/apps_test.go +++ b/tests/e2e/apps_test.go @@ -638,14 +638,14 @@ var _ = Describe("Apps", func() { "foo": "bar", "baz": "qux", } - serviceInstanceGUID = createServiceInstance(space1GUID, generateGUID("service-instance"), credentials) + serviceInstanceGUID = createUPServiceInstance(space1GUID, generateGUID("service-instance"), credentials) bindingGUID = createUPSIServiceBinding(appGUID, serviceInstanceGUID, "") moreCredentials := map[string]string{ "hello": "there", "secret": "stuff", } - secondServiceInstanceGUID = createServiceInstance(space1GUID, generateGUID("service-instance"), moreCredentials) + secondServiceInstanceGUID = createUPServiceInstance(space1GUID, generateGUID("service-instance"), moreCredentials) bindingName = "custom-named-binding" namedBindingGUID = createUPSIServiceBinding(appGUID, secondServiceInstanceGUID, bindingName) @@ -729,10 +729,10 @@ var _ = Describe("Apps", func() { "foo": "var", }) instanceName = generateGUID("service-instance") - instanceGUID = createServiceInstance(space1GUID, instanceName, nil) + instanceGUID = createUPServiceInstance(space1GUID, instanceName, nil) bindingGUID = createUPSIServiceBinding(appGUID, instanceGUID, "") instanceName2 = generateGUID("service-instance") - instanceGUID2 = createServiceInstance(space1GUID, instanceName2, nil) + instanceGUID2 = createUPServiceInstance(space1GUID, instanceName2, nil) bindingGUID2 = createUPSIServiceBinding(appGUID, instanceGUID2, "") }) diff --git a/tests/e2e/e2e_suite_test.go b/tests/e2e/e2e_suite_test.go index 774b83360..ea94cd875 100644 --- a/tests/e2e/e2e_suite_test.go +++ b/tests/e2e/e2e_suite_test.go @@ -731,7 +731,7 @@ func getProcess(appGUID, processType string) processResource { return process } -func createServiceInstance(spaceGUID, name string, credentials map[string]string) string { +func createUPServiceInstance(spaceGUID, name string, credentials map[string]string) string { GinkgoHelper() var serviceInstance typedResource diff --git a/tests/e2e/service_bindings_test.go b/tests/e2e/service_bindings_test.go index 08f116daa..2ab45aee3 100644 --- a/tests/e2e/service_bindings_test.go +++ b/tests/e2e/service_bindings_test.go @@ -24,7 +24,7 @@ var _ = Describe("Service Bindings", func() { BeforeEach(func() { spaceGUID = createSpace(generateGUID("space1"), commonTestOrgGUID) appGUID = createBuildpackApp(spaceGUID, generateGUID("app")) - upsiGUID = createServiceInstance(spaceGUID, generateGUID("service-instance"), nil) + upsiGUID = createUPServiceInstance(spaceGUID, generateGUID("service-instance"), nil) }) Describe("POST /v3/service_credential_bindings/{guid}", func() { @@ -150,7 +150,7 @@ var _ = Describe("Service Bindings", func() { BeforeEach(func() { bindingGUID = createUPSIServiceBinding(appGUID, upsiGUID, "") - anotherInstanceGUID = createServiceInstance(spaceGUID, generateGUID("another-service-instance"), nil) + anotherInstanceGUID = createUPServiceInstance(spaceGUID, generateGUID("another-service-instance"), nil) anotherBindingGUID = createUPSIServiceBinding(appGUID, anotherInstanceGUID, "") result = resourceListWithInclusion{} @@ -178,7 +178,7 @@ var _ = Describe("Service Bindings", func() { "foo": "val1", "bar": "val2", } - upsiGUID = createServiceInstance(spaceGUID, uuid.NewString(), credentials) + upsiGUID = createUPServiceInstance(spaceGUID, uuid.NewString(), credentials) bindingGUID = createUPSIServiceBinding(appGUID, upsiGUID, "") result = credentialsResponse{} }) diff --git a/tests/e2e/service_instances_test.go b/tests/e2e/service_instances_test.go index 9305100ee..b648900f7 100644 --- a/tests/e2e/service_instances_test.go +++ b/tests/e2e/service_instances_test.go @@ -18,15 +18,19 @@ var _ = Describe("Service Instances", func() { upsiGUID string upsiWithCredsGUID string upsiName string - httpResp *resty.Response - httpError error + // managedName string + // managedGUID string + httpResp *resty.Response + httpError error ) BeforeEach(func() { spaceGUID = createSpace(generateGUID("space1"), commonTestOrgGUID) - upsiName = generateGUID("service-instance") - upsiWithCredsGUID = generateGUID("service-instance-creds") - upsiGUID = createServiceInstance(spaceGUID, upsiName, nil) + upsiName = generateGUID("upsi-service-instance") + upsiWithCredsGUID = generateGUID("upsi-service-instance-creds") + upsiGUID = createUPServiceInstance(spaceGUID, upsiName, nil) + // managedName = generateGUID("managed-service-instance") + // managedGUID = createManagedServiceInstance(spaceGUID, managedName) }) AfterEach(func() { @@ -53,7 +57,7 @@ var _ = Describe("Service Instances", func() { var result map[string]any BeforeEach(func() { - upsiWithCredsGUID = createServiceInstance(spaceGUID, generateGUID("service-instance2"), map[string]string{"a": "b"}) + upsiWithCredsGUID = createUPServiceInstance(spaceGUID, generateGUID("service-instance2"), map[string]string{"a": "b"}) }) JustBeforeEach(func() { @@ -256,7 +260,7 @@ var _ = Describe("Service Instances", func() { BeforeEach(func() { anotherSpaceGUID = createSpace(generateGUID("space1"), commonTestOrgGUID) - anotherInstanceGUID = createServiceInstance(anotherSpaceGUID, generateGUID("service-instance"), nil) + anotherInstanceGUID = createUPServiceInstance(anotherSpaceGUID, generateGUID("service-instance"), nil) }) JustBeforeEach(func() { diff --git a/tests/e2e/spaces_test.go b/tests/e2e/spaces_test.go index 58f5a0fbb..a4ec0656a 100644 --- a/tests/e2e/spaces_test.go +++ b/tests/e2e/spaces_test.go @@ -179,7 +179,7 @@ var _ = Describe("Spaces", func() { spaceGUID = createSpace(generateGUID("space"), commonTestOrgGUID) resultErr = cfErrs{} serviceName = uuid.NewString() - serviceGUID = createServiceInstance(spaceGUID, serviceName, map[string]string{}) + serviceGUID = createUPServiceInstance(spaceGUID, serviceName, map[string]string{}) app1Name = generateGUID("manifested-app-1") app2Name = generateGUID("manifested-app-2") serviceBindingName = uuid.NewString() From 32fc9d8dd8770e348f4a83b47fb6950751222578 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 15 Sep 2025 16:24:56 +0200 Subject: [PATCH 02/47] Add unit tests for managed services on PATCH /v3/service_instances --- api/handlers/service_instance.go | 3 +- api/handlers/service_instance_test.go | 224 +++++++++++++++++--------- api/presenter/job.go | 1 + 3 files changed, 151 insertions(+), 77 deletions(-) diff --git a/api/handlers/service_instance.go b/api/handlers/service_instance.go index 6c7eb46f9..3929f7109 100644 --- a/api/handlers/service_instance.go +++ b/api/handlers/service_instance.go @@ -193,7 +193,8 @@ func (h *ServiceInstance) patch(r *http.Request) (*routing.Response, error) { return nil, apierrors.LogAndReturn(logger, err, "failed to patch managed service instance") } - return routing.NewResponse(http.StatusOK).WithBody(presenter.ForServiceInstance(serviceInstance, h.serverURL)), nil + return routing.NewResponse(http.StatusAccepted). + WithHeader("Location", presenter.JobURLForRedirects(serviceInstance.GUID, presenter.ManagedServiceInstancePatchOperation, h.serverURL)), nil } patchMessage := payload.ToUPSIPatchMessage(serviceInstance.SpaceGUID, serviceInstance.GUID) diff --git a/api/handlers/service_instance_test.go b/api/handlers/service_instance_test.go index 457c98e26..9a5686f89 100644 --- a/api/handlers/service_instance_test.go +++ b/api/handlers/service_instance_test.go @@ -835,103 +835,175 @@ var _ = Describe("ServiceInstance", func() { }) Describe("PATCH /v3/service_instances/:guid", func() { - BeforeEach(func() { - requestValidator.DecodeAndValidateJSONPayloadStub = decodeAndValidatePayloadStub(&payloads.ServiceInstancePatch{ - Name: tools.PtrTo("new-name"), - Tags: &[]string{"alice", "bob"}, - Credentials: &map[string]any{"foo": "bar"}, - Metadata: payloads.MetadataPatch{ - Annotations: map[string]*string{"ann2": tools.PtrTo("ann_val2")}, - Labels: map[string]*string{"lab2": tools.PtrTo("lab_val2")}, - }, - }) - serviceInstanceRepo.PatchUserProvidedServiceInstanceReturns(repositories.ServiceInstanceRecord{ - Name: "new-name", - GUID: "service-instance-guid", - }, nil) + When("updating a user provided service instance", func() { + BeforeEach(func() { + requestValidator.DecodeAndValidateJSONPayloadStub = decodeAndValidatePayloadStub(&payloads.ServiceInstancePatch{ + Name: tools.PtrTo("new-name"), + Tags: &[]string{"alice", "bob"}, + Credentials: &map[string]any{"foo": "bar"}, + Metadata: payloads.MetadataPatch{ + Annotations: map[string]*string{"ann2": tools.PtrTo("ann_val2")}, + Labels: map[string]*string{"lab2": tools.PtrTo("lab_val2")}, + }, + }) - reqPath += "/service-instance-guid" - reqMethod = http.MethodPatch - }) + serviceInstanceRepo.PatchUserProvidedServiceInstanceReturns(repositories.ServiceInstanceRecord{ + Name: "new-name", + GUID: "service-instance-guid", + }, nil) - It("patches the service instance", func() { - Expect(requestValidator.DecodeAndValidateJSONPayloadCallCount()).To(Equal(1)) - actualReq, _ := requestValidator.DecodeAndValidateJSONPayloadArgsForCall(0) - Expect(bodyString(actualReq)).To(Equal("the-json-body")) + reqPath += "/service-instance-guid" + reqMethod = http.MethodPatch + }) + It("patches the user provided service instance", func() { + Expect(requestValidator.DecodeAndValidateJSONPayloadCallCount()).To(Equal(1)) + actualReq, _ := requestValidator.DecodeAndValidateJSONPayloadArgsForCall(0) + Expect(bodyString(actualReq)).To(Equal("the-json-body")) - Expect(serviceInstanceRepo.GetServiceInstanceCallCount()).To(Equal(1)) - _, actualAuthInfo, actualGUID := serviceInstanceRepo.GetServiceInstanceArgsForCall(0) - Expect(actualAuthInfo).To(Equal(authInfo)) - Expect(actualGUID).To(Equal("service-instance-guid")) + Expect(serviceInstanceRepo.GetServiceInstanceCallCount()).To(Equal(1)) + _, actualAuthInfo, actualGUID := serviceInstanceRepo.GetServiceInstanceArgsForCall(0) + Expect(actualAuthInfo).To(Equal(authInfo)) + Expect(actualGUID).To(Equal("service-instance-guid")) - Expect(serviceInstanceRepo.PatchUserProvidedServiceInstanceCallCount()).To(Equal(1)) - _, actualAuthInfo, patchMessage := serviceInstanceRepo.PatchUserProvidedServiceInstanceArgsForCall(0) - Expect(actualAuthInfo).To(Equal(authInfo)) - Expect(patchMessage).To(Equal(repositories.PatchUPSIMessage{ - GUID: "service-instance-guid", - SpaceGUID: "space-guid", - Name: tools.PtrTo("new-name"), - Credentials: &map[string]any{"foo": "bar"}, - Tags: &[]string{"alice", "bob"}, - MetadataPatch: repositories.MetadataPatch{ - Annotations: map[string]*string{"ann2": tools.PtrTo("ann_val2")}, - Labels: map[string]*string{"lab2": tools.PtrTo("lab_val2")}, - }, - })) + Expect(serviceInstanceRepo.PatchUserProvidedServiceInstanceCallCount()).To(Equal(1)) + _, actualAuthInfo, patchMessage := serviceInstanceRepo.PatchUserProvidedServiceInstanceArgsForCall(0) + Expect(actualAuthInfo).To(Equal(authInfo)) + Expect(patchMessage).To(Equal(repositories.PatchUPSIMessage{ + GUID: "service-instance-guid", + SpaceGUID: "space-guid", + Name: tools.PtrTo("new-name"), + Credentials: &map[string]any{"foo": "bar"}, + Tags: &[]string{"alice", "bob"}, + MetadataPatch: repositories.MetadataPatch{ + Annotations: map[string]*string{"ann2": tools.PtrTo("ann_val2")}, + Labels: map[string]*string{"lab2": tools.PtrTo("lab_val2")}, + }, + })) - Expect(rr).To(HaveHTTPStatus(http.StatusOK)) - Expect(rr).To(HaveHTTPHeaderWithValue("Content-Type", "application/json")) - Expect(rr).To(HaveHTTPBody(SatisfyAll( - MatchJSONPath("$.guid", "service-instance-guid"), - MatchJSONPath("$.name", "new-name"), - MatchJSONPath("$.links.self.href", "https://api.example.org/v3/service_instances/service-instance-guid"), - ))) - }) + Expect(rr).To(HaveHTTPStatus(http.StatusOK)) + Expect(rr).To(HaveHTTPHeaderWithValue("Content-Type", "application/json")) + Expect(rr).To(HaveHTTPBody(SatisfyAll( + MatchJSONPath("$.guid", "service-instance-guid"), + MatchJSONPath("$.name", "new-name"), + MatchJSONPath("$.links.self.href", "https://api.example.org/v3/service_instances/service-instance-guid"), + ))) + }) - When("decoding the payload fails", func() { - BeforeEach(func() { - requestValidator.DecodeAndValidateJSONPayloadReturns(apierrors.NewUnprocessableEntityError(nil, "nope")) + When("decoding the payload fails", func() { + BeforeEach(func() { + requestValidator.DecodeAndValidateJSONPayloadReturns(apierrors.NewUnprocessableEntityError(nil, "nope")) + }) + + It("returns an error", func() { + expectUnprocessableEntityError("nope") + }) }) - It("returns an error", func() { - expectUnprocessableEntityError("nope") + When("getting the service instance fails with not found", func() { + BeforeEach(func() { + serviceInstanceRepo.GetServiceInstanceReturns( + repositories.ServiceInstanceRecord{}, + apierrors.NewNotFoundError(nil, repositories.ServiceInstanceResourceType), + ) + }) + + It("returns 404 Not Found", func() { + expectNotFoundError("Service Instance") + }) }) - }) - When("getting the service instance fails with not found", func() { - BeforeEach(func() { - serviceInstanceRepo.GetServiceInstanceReturns( - repositories.ServiceInstanceRecord{}, - apierrors.NewNotFoundError(nil, repositories.ServiceInstanceResourceType), - ) + When("getting the service instance fails with forbidden", func() { + BeforeEach(func() { + serviceInstanceRepo.GetServiceInstanceReturns( + repositories.ServiceInstanceRecord{}, + apierrors.NewForbiddenError(nil, repositories.ServiceInstanceResourceType), + ) + }) + + It("returns 404 Not Found", func() { + expectNotFoundError("Service Instance") + }) }) - It("returns 404 Not Found", func() { - expectNotFoundError("Service Instance") + When("patching the user provided service instances fails", func() { + BeforeEach(func() { + serviceInstanceRepo.PatchUserProvidedServiceInstanceReturns(repositories.ServiceInstanceRecord{}, errors.New("oops")) + }) + + It("returns the error", func() { + expectUnknownError() + }) }) }) - - When("getting the service instance fails with forbidden", func() { + When("updating a managed service instance", func() { BeforeEach(func() { - serviceInstanceRepo.GetServiceInstanceReturns( - repositories.ServiceInstanceRecord{}, - apierrors.NewForbiddenError(nil, repositories.ServiceInstanceResourceType), - ) - }) + requestValidator.DecodeAndValidateJSONPayloadStub = decodeAndValidatePayloadStub(&payloads.ServiceInstancePatch{ + Name: tools.PtrTo("new-name"), + Type: "managed", + Tags: &[]string{"alice", "bob"}, + Credentials: &map[string]any{"foo": "bar"}, + Relationships: &payloads.ServiceInstanceRelationships{ + ServicePlan: &payloads.Relationship{ + Data: &payloads.RelationshipData{ + GUID: "plan-guid", + }, + }, + }, + Metadata: payloads.MetadataPatch{ + Annotations: map[string]*string{"ann2": tools.PtrTo("ann_val2")}, + Labels: map[string]*string{"lab2": tools.PtrTo("lab_val2")}, + }, + }) - It("returns 404 Not Found", func() { - expectNotFoundError("Service Instance") + serviceInstanceRepo.GetServiceInstanceReturns(repositories.ServiceInstanceRecord{ + GUID: "service-instance-guid", + SpaceGUID: "space-guid", + Type: korifiv1alpha1.ManagedType, + }, nil) + serviceInstanceRepo.PatchManagedServiceInstanceReturns(repositories.ServiceInstanceRecord{ + Name: "new-name", + GUID: "service-instance-guid", + }, nil) + + reqPath += "/service-instance-guid" + reqMethod = http.MethodPatch }) - }) - When("patching the service instances fails", func() { - BeforeEach(func() { - serviceInstanceRepo.PatchUserProvidedServiceInstanceReturns(repositories.ServiceInstanceRecord{}, errors.New("oops")) + It("patches the managed service instance", func() { + Expect(requestValidator.DecodeAndValidateJSONPayloadCallCount()).To(Equal(1)) + actualReq, _ := requestValidator.DecodeAndValidateJSONPayloadArgsForCall(0) + Expect(bodyString(actualReq)).To(Equal("the-json-body")) + + Expect(serviceInstanceRepo.PatchManagedServiceInstanceCallCount()).To(Equal(1)) + _, actualAuthInfo, patchMessage := serviceInstanceRepo.PatchManagedServiceInstanceArgsForCall(0) + Expect(actualAuthInfo).To(Equal(authInfo)) + Expect(patchMessage).To(Equal(repositories.PatchManagedSIMessage{ + GUID: "service-instance-guid", + SpaceGUID: "space-guid", + PlanGUID: "plan-guid", + Name: tools.PtrTo("new-name"), + Credentials: &map[string]any{"foo": "bar"}, + Tags: &[]string{"alice", "bob"}, + MetadataPatch: repositories.MetadataPatch{ + Annotations: map[string]*string{"ann2": tools.PtrTo("ann_val2")}, + Labels: map[string]*string{"lab2": tools.PtrTo("lab_val2")}, + }, + })) }) + It("returns HTTP 202 Accepted response", func() { + Expect(rr).To(HaveHTTPStatus(http.StatusAccepted)) + Expect(rr).To(HaveHTTPHeaderWithValue("Location", + ContainSubstring("/v3/jobs/managed_service_instance.patch~service-instance-guid"))) + }) + When("patching the managed service instances fails", func() { + BeforeEach(func() { + serviceInstanceRepo.PatchManagedServiceInstanceReturns(repositories.ServiceInstanceRecord{}, errors.New("oops")) + }) - It("returns the error", func() { - expectUnknownError() + It("returns the error", func() { + expectUnknownError() + }) }) }) }) diff --git a/api/presenter/job.go b/api/presenter/job.go index d65e1c925..07a94366f 100644 --- a/api/presenter/job.go +++ b/api/presenter/job.go @@ -31,6 +31,7 @@ const ( ManagedServiceInstanceResourceType = "managed_service_instance" ManagedServiceBindingResourceType = "managed_service_binding" ManagedServiceInstanceCreateOperation = ManagedServiceInstanceResourceType + ".create" + ManagedServiceInstancePatchOperation = ManagedServiceInstanceResourceType + ".patch" ManagedServiceInstanceDeleteOperation = ManagedServiceInstanceResourceType + ".delete" ManagedServiceBindingCreateOperation = ManagedServiceBindingResourceType + ".create" ManagedServiceBindingDeleteOperation = ManagedServiceBindingResourceType + ".delete" From 8661c745a79f35d9b343dd648ac5d0180306f817 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Thu, 23 Oct 2025 16:10:17 +0200 Subject: [PATCH 03/47] Fix api/repositories/service_instance_repository.go due to suggestions --- api/handlers/service_instance_test.go | 12 ++++----- api/payloads/service_instance.go | 15 ++++++----- .../service_instance_repository.go | 25 ++++++------------- 3 files changed, 22 insertions(+), 30 deletions(-) diff --git a/api/handlers/service_instance_test.go b/api/handlers/service_instance_test.go index 9a5686f89..d7ade2dfe 100644 --- a/api/handlers/service_instance_test.go +++ b/api/handlers/service_instance_test.go @@ -835,7 +835,6 @@ var _ = Describe("ServiceInstance", func() { }) Describe("PATCH /v3/service_instances/:guid", func() { - When("updating a user provided service instance", func() { BeforeEach(func() { requestValidator.DecodeAndValidateJSONPayloadStub = decodeAndValidatePayloadStub(&payloads.ServiceInstancePatch{ @@ -979,12 +978,11 @@ var _ = Describe("ServiceInstance", func() { _, actualAuthInfo, patchMessage := serviceInstanceRepo.PatchManagedServiceInstanceArgsForCall(0) Expect(actualAuthInfo).To(Equal(authInfo)) Expect(patchMessage).To(Equal(repositories.PatchManagedSIMessage{ - GUID: "service-instance-guid", - SpaceGUID: "space-guid", - PlanGUID: "plan-guid", - Name: tools.PtrTo("new-name"), - Credentials: &map[string]any{"foo": "bar"}, - Tags: &[]string{"alice", "bob"}, + GUID: "service-instance-guid", + SpaceGUID: "space-guid", + PlanGUID: tools.PtrTo("plan-guid"), + Name: tools.PtrTo("new-name"), + Tags: &[]string{"alice", "bob"}, MetadataPatch: repositories.MetadataPatch{ Annotations: map[string]*string{"ann2": tools.PtrTo("ann_val2")}, Labels: map[string]*string{"lab2": tools.PtrTo("lab_val2")}, diff --git a/api/payloads/service_instance.go b/api/payloads/service_instance.go index 7f4c3f810..35ece4971 100644 --- a/api/payloads/service_instance.go +++ b/api/payloads/service_instance.go @@ -200,13 +200,16 @@ func (p ServiceInstancePatch) ToUPSIPatchMessage(spaceGUID, appGUID string) repo } func (p ServiceInstancePatch) ToManagedSIPatchMessage(spaceGUID, appGUID string) repositories.PatchManagedSIMessage { + var planGUID *string + if p.Relationships.ServicePlan.Data.GUID != "" { + planGUID = &p.Relationships.ServicePlan.Data.GUID + } return repositories.PatchManagedSIMessage{ - SpaceGUID: spaceGUID, - PlanGUID: p.Relationships.ServicePlan.Data.GUID, - GUID: appGUID, - Name: p.Name, - Credentials: p.Credentials, - Tags: p.Tags, + SpaceGUID: spaceGUID, + PlanGUID: planGUID, + GUID: appGUID, + Name: p.Name, + Tags: p.Tags, MetadataPatch: repositories.MetadataPatch{ Labels: p.Metadata.Labels, Annotations: p.Metadata.Annotations, diff --git a/api/repositories/service_instance_repository.go b/api/repositories/service_instance_repository.go index 0644a3a6b..f75d1a1f9 100644 --- a/api/repositories/service_instance_repository.go +++ b/api/repositories/service_instance_repository.go @@ -78,12 +78,11 @@ type PatchUPSIMessage struct { } type PatchManagedSIMessage struct { - GUID string - SpaceGUID string - PlanGUID string - Name *string - Credentials *map[string]any - Tags *[]string + GUID string + SpaceGUID string + PlanGUID *string + Name *string + Tags *[]string MetadataPatch } @@ -104,6 +103,9 @@ func (p PatchManagedSIMessage) Apply(cfServiceInstance *korifiv1alpha1.CFService if p.Tags != nil { cfServiceInstance.Spec.Tags = *p.Tags } + if p.PlanGUID != nil { + cfServiceInstance.Spec.PlanGUID = *p.PlanGUID + } p.MetadataPatch.Apply(cfServiceInstance) } @@ -346,17 +348,6 @@ func (r *ServiceInstanceRepo) PatchManagedServiceInstance(ctx context.Context, a return ServiceInstanceRecord{}, apierrors.FromK8sError(err, ServiceInstanceResourceType) } - if message.Credentials != nil { - cfServiceInstance, err = r.migrateLegacyCredentials(ctx, cfServiceInstance) - if err != nil { - return ServiceInstanceRecord{}, err - } - err = r.patchCredentialsSecret(ctx, cfServiceInstance, *message.Credentials) - if err != nil { - return ServiceInstanceRecord{}, apierrors.FromK8sError(err, ServiceInstanceResourceType) - } - } - return cfServiceInstanceToRecord(*cfServiceInstance), nil } From bc1663d5eb32b139cfcfdfaa324a3154c4922759 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 22 Dec 2025 16:09:47 +0100 Subject: [PATCH 04/47] Cover controllers/services/osbapi with tests --- .../services/osbapi/client_test.go | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/controllers/controllers/services/osbapi/client_test.go b/controllers/controllers/services/osbapi/client_test.go index a27dd7210..a1f846698 100644 --- a/controllers/controllers/services/osbapi/client_test.go +++ b/controllers/controllers/services/osbapi/client_test.go @@ -236,6 +236,99 @@ var _ = Describe("OSBAPI Client", func() { }) }) + Describe("Update", func() { + var ( + updateResp osbapi.UpdateResponse + updateErr error + ) + + BeforeEach(func() { + brokerServer = brokerServer.WithResponse( + "/v2/service_instances/{id}", + nil, + http.StatusOK, + ) + }) + + JustBeforeEach(func() { + updateResp, updateErr = brokerClient.Update(ctx, osbapi.UpdatePayload{ + InstanceID: "my-service-instance", + UpdateRequest: osbapi.UpdateRequest{ + ServiceId: "service-guid", + PlanID: "plan-guid", + }, + }) + }) + It("updates the service synchronously", func() { + Expect(updateErr).NotTo(HaveOccurred()) + Expect(updateResp).To(Equal(osbapi.UpdateResponse{})) + }) + It("sends async update request to broker", func() { + Expect(updateErr).NotTo(HaveOccurred()) + requests := brokerServer.ServedRequests() + + Expect(requests).To(HaveLen(1)) + + Expect(requests[0].Method).To(Equal(http.MethodPatch)) + Expect(requests[0].URL.Path).To(Equal("/v2/service_instances/my-service-instance")) + + Expect(requests[0].URL.Query()).To(BeEquivalentTo(map[string][]string{ + "accepts_incomplete": {"true"}, + })) + }) + When("the broker accepts the update request", func() { + BeforeEach(func() { + brokerServer = brokerServer.WithResponse( + "/v2/service_instances/{id}", + map[string]any{ + "operation": "update_op1", + }, + http.StatusAccepted, + ) + }) + + It("updatres the service asynchronously", func() { + Expect(updateErr).NotTo(HaveOccurred()) + Expect(updateResp).To(Equal(osbapi.UpdateResponse{ + IsAsync: true, + Operation: "update_op1", + })) + }) + }) + When("the update request fails with 400 BadRequest error", func() { + BeforeEach(func() { + brokerServer = brokerServer.WithResponse("/v2/service_instances/{id}", nil, http.StatusBadRequest) + }) + + It("returns an unrecoverable error", func() { + Expect(updateErr).To(Equal(osbapi.UnrecoverableError{Status: http.StatusBadRequest})) + }) + }) + + When("the provision request fails with 422 Unprocessable entity error", func() { + BeforeEach(func() { + brokerServer = brokerServer.WithResponse("/v2/service_instances/{id}", nil, http.StatusUnprocessableEntity) + }) + + It("returns an unrecoverable error", func() { + Expect(updateErr).To(Equal(osbapi.UnrecoverableError{Status: http.StatusUnprocessableEntity})) + }) + }) + When("the update request fails", func() { + BeforeEach(func() { + brokerServer = brokerServer.WithResponse( + "/v2/service_instances/{id}", + nil, + http.StatusTeapot, + ) + }) + + It("returns an error", func() { + Expect(updateErr).To(MatchError(ContainSubstring("update request failed"))) + }) + }) + }) + Describe("Deprovision", func() { var ( deprovisionResp osbapi.ProvisionResponse From 3ffc150a0f78122301ffd718d58cf80ec9d5bba5 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Tue, 30 Dec 2025 17:43:32 +0100 Subject: [PATCH 05/47] [controllers][managed services] Use Status.PlanGUID in reconcilation loop --- .../api/v1alpha1/cfserviceinstance_types.go | 4 + .../services/instances/managed/controller.go | 8 +- .../instances/managed/controller_test.go | 290 +++++++++++++++++- ...i.cloudfoundry.org_cfserviceinstances.yaml | 3 + 4 files changed, 301 insertions(+), 4 deletions(-) diff --git a/controllers/api/v1alpha1/cfserviceinstance_types.go b/controllers/api/v1alpha1/cfserviceinstance_types.go index f25d3ef3c..bb469d5c3 100644 --- a/controllers/api/v1alpha1/cfserviceinstance_types.go +++ b/controllers/api/v1alpha1/cfserviceinstance_types.go @@ -91,6 +91,10 @@ type CFServiceInstanceStatus struct { //+kubebuilder:validation:Optional MaintenanceInfo MaintenanceInfo `json:"maintenanceInfo"` + // The service instance actual plan + //+kubebuilder:validation:Optional + PlanGUID string `json:"planGuid"` + // True if there is an upgrade available for for the service instance (i.e. the plan has a new version). Only makes seense for managed service instances //+kubebuilder:validation:Optional UpgradeAvailable bool `json:"upgradeAvailable"` diff --git a/controllers/controllers/services/instances/managed/controller.go b/controllers/controllers/services/instances/managed/controller.go index 192908d47..0892f8b1f 100644 --- a/controllers/controllers/services/instances/managed/controller.go +++ b/controllers/controllers/services/instances/managed/controller.go @@ -122,6 +122,7 @@ func (r *Reconciler) isManaged(object client.Object) bool { func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *korifiv1alpha1.CFServiceInstance) (ctrl.Result, error) { log := logr.FromContextOrDiscard(ctx) + serviceInstance.Status.ObservedGeneration = serviceInstance.Generation log.V(1).Info("set observed generation", "generation", serviceInstance.Status.ObservedGeneration) if !serviceInstance.GetDeletionTimestamp().IsZero() { @@ -147,7 +148,7 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor } if isFailed(serviceInstance) { - return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ProvisioningFailed").WithNoRequeue() + return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ReconciliationFailed").WithNoRequeue() } planVisible, err := r.isServicePlanVisible(ctx, serviceInstance, serviceInstanceAssets.ServicePlan) @@ -165,7 +166,7 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor serviceInstance.Spec.ServiceLabel = tools.PtrTo(serviceInstanceAssets.ServiceOffering.Spec.Name) } - if serviceInstance.Status.ObservedGeneration == 0 { + if serviceInstance.Status.PlanGUID == "" { provisionResponse, err := r.provisionServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient) if err != nil { log.Error(err, "failed to provision service instance") @@ -194,6 +195,7 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor } } + serviceInstance.Status.PlanGUID = serviceInstance.Spec.PlanGUID serviceInstance.Status.MaintenanceInfo = serviceInstanceAssets.ServicePlan.Spec.MaintenanceInfo serviceInstance.Status.LastOperation.State = "succeeded" return ctrl.Result{}, nil @@ -339,7 +341,7 @@ func (r *Reconciler) processUpdateOperation( if lastOpResponse.State == "failed" { meta.SetStatusCondition(&serviceInstance.Status.Conditions, metav1.Condition{ - Type: korifiv1alpha1.ProvisioningFailedCondition, + Type: korifiv1alpha1.UpdateFailedCondition, Status: metav1.ConditionTrue, ObservedGeneration: serviceInstance.Generation, LastTransitionTime: metav1.NewTime(time.Now()), diff --git a/controllers/controllers/services/instances/managed/controller_test.go b/controllers/controllers/services/instances/managed/controller_test.go index a04f2e713..ecffc22de 100644 --- a/controllers/controllers/services/instances/managed/controller_test.go +++ b/controllers/controllers/services/instances/managed/controller_test.go @@ -29,6 +29,7 @@ var _ = Describe("CFServiceInstance", func() { instance *korifiv1alpha1.CFServiceInstance serviceBroker *korifiv1alpha1.CFServiceBroker servicePlan *korifiv1alpha1.CFServicePlan + servicePlan2 *korifiv1alpha1.CFServicePlan ) BeforeEach(func() { @@ -102,6 +103,29 @@ var _ = Describe("CFServiceInstance", func() { } Expect(adminClient.Create(ctx, servicePlan)).To(Succeed()) + servicePlan2 = &korifiv1alpha1.CFServicePlan{ + ObjectMeta: metav1.ObjectMeta{ + Name: uuid.NewString(), + Namespace: rootNamespace, + Labels: map[string]string{ + korifiv1alpha1.RelServiceBrokerGUIDLabel: serviceBroker.Name, + korifiv1alpha1.RelServiceOfferingGUIDLabel: serviceOffering.Name, + }, + }, + Spec: korifiv1alpha1.CFServicePlanSpec{ + Visibility: korifiv1alpha1.ServicePlanVisibility{ + Type: "public", + }, + BrokerCatalog: korifiv1alpha1.ServicePlanBrokerCatalog{ + ID: "service-plan-id-2", + }, + MaintenanceInfo: korifiv1alpha1.MaintenanceInfo{ + Version: "1.2.3", + }, + }, + } + Expect(adminClient.Create(ctx, servicePlan2)).To(Succeed()) + instance = &korifiv1alpha1.CFServiceInstance{ ObjectMeta: metav1.ObjectMeta{ Name: uuid.NewString(), @@ -116,7 +140,6 @@ var _ = Describe("CFServiceInstance", func() { PlanGUID: servicePlan.Name, }, } - Expect(adminClient.Create(ctx, instance)).To(Succeed()) }) @@ -127,6 +150,13 @@ var _ = Describe("CFServiceInstance", func() { }).Should(Succeed()) }) + It("sets the PlanGUID status field", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) + g.Expect(instance.Status.PlanGUID).To(Equal(instance.Spec.PlanGUID)) + }).Should(Succeed()) + }) + It("sets the Ready condition to True", func() { Eventually(func(g Gomega) { g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) @@ -753,6 +783,264 @@ var _ = Describe("CFServiceInstance", func() { }) }) + When("updates instance", func() { + BeforeEach(func() { + brokerClient.UpdateReturns(osbapi.UpdateResponse{}, nil) + }) + 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.PlanGUID).To(Equal(servicePlan.Name)) + }).Should(Succeed()) + + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(servicePlan2), servicePlan2)).To(Succeed()) + }).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()) + }) + + It("sets the ObservedGeneration status field", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) + g.Expect(instance.Status.ObservedGeneration).To(Equal(instance.Generation)) + }).Should(Succeed()) + }) + + It("sets the PlanGUID status field", 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.PlanGUID).To(Equal(instance.Spec.PlanGUID)) + g.Expect(instance.Status.PlanGUID).To(Equal(servicePlan2.Name)) + }).Should(Succeed()) + }) + + It("sets the Ready condition to True", 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)), + ))) + }).Should(Succeed()) + }) + + When("service update fails with recoverable error", func() { + BeforeEach(func() { + brokerClient.UpdateReturns(osbapi.UpdateResponse{}, errors.New("update-failed")) + }) + + It("keeps trying to update the instance", func() { + Eventually(func(g Gomega) { + g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">=", 1)) + _, updatePayload := brokerClient.UpdateArgsForCall(1) + g.Expect(updatePayload).To(Equal(osbapi.UpdatePayload{ + InstanceID: instance.Name, + UpdateRequest: osbapi.UpdateRequest{ + ServiceId: "service-offering-id", + PlanID: "service-plan-id-2", + }, + })) + }).Should(Succeed()) + }) + + It("sets initial state in instance last operation", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) + g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">=", 1)) + g.Expect(instance.Status.LastOperation).To(Equal(korifiv1alpha1.LastOperation{ + Type: "update", + State: "in progress", + })) + }).Should(Succeed()) + }) + }) + + When("service update fails with unrecoverable error", func() { + BeforeEach(func() { + brokerClient.UpdateReturns(osbapi.UpdateResponse{}, osbapi.UnrecoverableError{Status: http.StatusBadRequest}) + }) + + It("fails the instance", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) + + g.Expect(instance.Status.Conditions).To(ContainElements( + SatisfyAll( + HasType(Equal(korifiv1alpha1.StatusConditionReady)), + HasStatus(Equal(metav1.ConditionFalse)), + ), + SatisfyAll( + HasType(Equal(korifiv1alpha1.UpdateFailedCondition)), + HasStatus(Equal(metav1.ConditionTrue)), + HasReason(Equal("UpdateFailed")), + HasMessage(ContainSubstring("The server responded with status: 400")), + ), + )) + }).Should(Succeed()) + }) + + It("sets failed state in instance last operation", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) + g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">=", 1)) + g.Expect(instance.Status.LastOperation).To(Equal(korifiv1alpha1.LastOperation{ + Type: "update", + State: "failed", + })) + }).Should(Succeed()) + }) + }) + + When("the update is asynchronous", func() { + BeforeEach(func() { + brokerClient.GetServiceInstanceLastOperationReturns(osbapi.LastOperationResponse{ + State: "in-progress-or-whatever", + }, nil) + + brokerClient.UpdateReturns(osbapi.UpdateResponse{ + IsAsync: true, + Operation: "operation-1", + }, nil) + }) + + It("set sets ready condition to false", 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.ConditionFalse)), + HasReason(Equal("UpdateInProgress")), + ))) + }).Should(Succeed()) + }) + + It("sets in progress state in instance last operation", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) + g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">=", 1)) + g.Expect(instance.Status.LastOperation).To(Equal(korifiv1alpha1.LastOperation{ + Type: "update", + State: "in progress", + })) + }).Should(Succeed()) + }) + + It("continuously checks the last operation", func() { + Eventually(func(g Gomega) { + g.Expect(brokerClient.GetServiceInstanceLastOperationCallCount()).To(BeNumerically(">", 1)) + _, lastOp := brokerClient.GetServiceInstanceLastOperationArgsForCall(brokerClient.GetServiceInstanceLastOperationCallCount() - 1) + g.Expect(lastOp).To(Equal(osbapi.GetInstanceLastOperationRequest{ + InstanceID: instance.Name, + GetLastOperationRequestParameters: osbapi.GetLastOperationRequestParameters{ + ServiceId: "service-offering-id", + PlanID: "service-plan-id-2", + Operation: "operation-1", + }, + })) + }).Should(Succeed()) + }) + + When("getting service last operation fails", func() { + BeforeEach(func() { + brokerClient.GetServiceInstanceLastOperationReturns(osbapi.LastOperationResponse{}, errors.New("get-last-op-failed")) + }) + + It("sets the ready condition to false", 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.ConditionFalse)), + ))) + }).Should(Succeed()) + }) + }) + + When("the last operation is succeeded", func() { + BeforeEach(func() { + brokerClient.GetServiceInstanceLastOperationReturns(osbapi.LastOperationResponse{ + State: "succeeded", + }, nil) + }) + + It("sets the ready condition to true", 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)), + ))) + }).Should(Succeed()) + }) + }) + + When("the last operation is failed", func() { + BeforeEach(func() { + brokerClient.GetServiceInstanceLastOperationReturns(osbapi.LastOperationResponse{ + State: "failed", + Description: "update-failed", + }, nil) + }) + + It("sets the ready condition to false", 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.ConditionFalse)), + ))) + }).Should(Succeed()) + }) + + It("sets the failed condition", 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.UpdateFailedCondition)), + HasStatus(Equal(metav1.ConditionTrue)), + HasReason(Equal("UpdateFailed")), + HasMessage(Equal("update-failed")), + ))) + }).Should(Succeed()) + }) + + It("sets failed state in instance last operation", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) + g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">=", 1)) + g.Expect(instance.Status.LastOperation).To(Equal(korifiv1alpha1.LastOperation{ + Type: "update", + State: "failed", + Description: "update-failed", + })) + }).Should(Succeed()) + }) + }) + }) + }) + When("the service instance is user-provided", func() { BeforeEach(func() { Expect(k8s.PatchResource(ctx, adminClient, instance, func() { diff --git a/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml b/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml index 245f143b9..99cffd440 100644 --- a/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml +++ b/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml @@ -217,6 +217,9 @@ spec: the CFServiceInstance that has been reconciled format: int64 type: integer + planGuid: + description: The service instance actual plan + type: string upgradeAvailable: description: True if there is an upgrade available for for the service instance (i.e. the plan has a new version). Only makes seense for From 2d1bd24e0ad63560cf04521deb9a477408346dc3 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Fri, 23 Jan 2026 13:34:48 +0100 Subject: [PATCH 06/47] [controllers][managed services] update isFailed in order to use UpdateFailedCondition --- .../controllers/services/instances/managed/controller.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/controllers/controllers/services/instances/managed/controller.go b/controllers/controllers/services/instances/managed/controller.go index 0892f8b1f..e8ca20c5d 100644 --- a/controllers/controllers/services/instances/managed/controller.go +++ b/controllers/controllers/services/instances/managed/controller.go @@ -561,7 +561,8 @@ func (r *Reconciler) getNamespace(ctx context.Context, namespaceName string) (*c } func isFailed(instance *korifiv1alpha1.CFServiceInstance) bool { - return meta.IsStatusConditionTrue(instance.Status.Conditions, korifiv1alpha1.ProvisioningFailedCondition) + return meta.IsStatusConditionTrue(instance.Status.Conditions, korifiv1alpha1.ProvisioningFailedCondition) || + meta.IsStatusConditionTrue(instance.Status.Conditions, korifiv1alpha1.UpdateFailedCondition) } func isReady(instance *korifiv1alpha1.CFServiceInstance) bool { From 6651e36945c7e7b35784daae7977171a8eb8bb69 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Fri, 23 Jan 2026 13:52:47 +0100 Subject: [PATCH 07/47] [controllers][services][osbapi] Fix broker statuscodes on Update call --- controllers/controllers/services/osbapi/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/controllers/services/osbapi/client.go b/controllers/controllers/services/osbapi/client.go index 391b3a929..d58912694 100644 --- a/controllers/controllers/services/osbapi/client.go +++ b/controllers/controllers/services/osbapi/client.go @@ -124,7 +124,7 @@ func (c *Client) Update(ctx context.Context, payload UpdatePayload) (UpdateRespo if err != nil { return UpdateResponse{}, fmt.Errorf("update request failed: %w", err) } - if statusCode == http.StatusBadRequest || statusCode == http.StatusConflict || statusCode == http.StatusUnprocessableEntity { + if statusCode == http.StatusBadRequest || statusCode == http.StatusUnprocessableEntity { return UpdateResponse{}, UnrecoverableError{Status: statusCode} } From 5627bf1f76e0124eeeb04b57739002c921da6789 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Fri, 30 Jan 2026 10:49:12 +0100 Subject: [PATCH 08/47] Implement e2e tests for updating managed services --- api/handlers/job.go | 1 + api/handlers/service_instance.go | 4 +- api/main.go | 1 + api/presenter/job.go | 2 +- .../security_groups/validator_test.go | 2 - tests/assets/sample-broker-golang/main.go | 42 ++++- tests/e2e/e2e_suite_test.go | 40 +++++ tests/e2e/service_bindings_test.go | 4 +- tests/e2e/service_instances_test.go | 166 +++++++++++------- 9 files changed, 180 insertions(+), 82 deletions(-) diff --git a/api/handlers/job.go b/api/handlers/job.go index 17271f92f..1b762a540 100644 --- a/api/handlers/job.go +++ b/api/handlers/job.go @@ -32,6 +32,7 @@ const ( ServiceBrokerDeleteJobType = "service_broker.delete" ManagedServiceInstanceDeleteJobType = "managed_service_instance.delete" ManagedServiceInstanceCreateJobType = "managed_service_instance.create" + ManagedServiceInstanceUpdateJobType = "managed_service_instance.update" ManagedServiceBindingCreateJobType = "managed_service_binding.create" ManagedServiceBindingDeleteJobType = "managed_service_binding.delete" JobTimeoutDuration = 120.0 diff --git a/api/handlers/service_instance.go b/api/handlers/service_instance.go index 3929f7109..7222fed6f 100644 --- a/api/handlers/service_instance.go +++ b/api/handlers/service_instance.go @@ -186,7 +186,7 @@ func (h *ServiceInstance) patch(r *http.Request) (*routing.Response, error) { return nil, apierrors.LogAndReturn(logger, apierrors.ForbiddenAsNotFound(err), "failed to get service instance") } - if payload.Type == korifiv1alpha1.ManagedType { + if serviceInstance.Type == korifiv1alpha1.ManagedType { patchMessage := payload.ToManagedSIPatchMessage(serviceInstance.SpaceGUID, serviceInstance.GUID) serviceInstance, err = h.serviceInstanceRepo.PatchManagedServiceInstance(r.Context(), authInfo, patchMessage) if err != nil { @@ -194,7 +194,7 @@ func (h *ServiceInstance) patch(r *http.Request) (*routing.Response, error) { } return routing.NewResponse(http.StatusAccepted). - WithHeader("Location", presenter.JobURLForRedirects(serviceInstance.GUID, presenter.ManagedServiceInstancePatchOperation, h.serverURL)), nil + WithHeader("Location", presenter.JobURLForRedirects(serviceInstance.GUID, presenter.ManagedServiceInstanceUpdateOperation, h.serverURL)), nil } patchMessage := payload.ToUPSIPatchMessage(serviceInstance.SpaceGUID, serviceInstance.GUID) diff --git a/api/main.go b/api/main.go index 71f33bf64..4c28ca559 100644 --- a/api/main.go +++ b/api/main.go @@ -421,6 +421,7 @@ func main() { handlers.ServiceBrokerCreateJobType: serviceBrokerRepo, handlers.ServiceBrokerUpdateJobType: serviceBrokerRepo, handlers.ManagedServiceInstanceCreateJobType: serviceInstanceRepo, + handlers.ManagedServiceInstanceUpdateJobType: serviceInstanceRepo, handlers.ManagedServiceBindingCreateJobType: serviceBindingRepo, }, routeRepo, diff --git a/api/presenter/job.go b/api/presenter/job.go index 07a94366f..b7fd7fdd9 100644 --- a/api/presenter/job.go +++ b/api/presenter/job.go @@ -31,7 +31,7 @@ const ( ManagedServiceInstanceResourceType = "managed_service_instance" ManagedServiceBindingResourceType = "managed_service_binding" ManagedServiceInstanceCreateOperation = ManagedServiceInstanceResourceType + ".create" - ManagedServiceInstancePatchOperation = ManagedServiceInstanceResourceType + ".patch" + ManagedServiceInstanceUpdateOperation = ManagedServiceInstanceResourceType + ".update" ManagedServiceInstanceDeleteOperation = ManagedServiceInstanceResourceType + ".delete" ManagedServiceBindingCreateOperation = ManagedServiceBindingResourceType + ".create" ManagedServiceBindingDeleteOperation = ManagedServiceBindingResourceType + ".delete" diff --git a/controllers/webhooks/networking/security_groups/validator_test.go b/controllers/webhooks/networking/security_groups/validator_test.go index 5472edaf5..fb777367d 100644 --- a/controllers/webhooks/networking/security_groups/validator_test.go +++ b/controllers/webhooks/networking/security_groups/validator_test.go @@ -438,7 +438,6 @@ var _ = Describe("CFSecurityGroupValidatingWebhook", func() { )) }) }) - }) Describe("ValidateDelete", func() { @@ -468,5 +467,4 @@ var _ = Describe("CFSecurityGroupValidatingWebhook", func() { }) }) }) - }) diff --git a/tests/assets/sample-broker-golang/main.go b/tests/assets/sample-broker-golang/main.go index 1a28185fb..85250ab03 100644 --- a/tests/assets/sample-broker-golang/main.go +++ b/tests/assets/sample-broker-golang/main.go @@ -27,6 +27,7 @@ func main() { http.HandleFunc("GET /v2/catalog", getCatalogHandler) http.HandleFunc("PUT /v2/service_instances/{id}", provisionServiceInstanceHandler) + http.HandleFunc("PATCH /v2/service_instances/{id}", updateServiceInstanceHandler) http.HandleFunc("DELETE /v2/service_instances/{id}", deprovisionServiceInstanceHandler) http.HandleFunc("GET /v2/service_instances/{id}/last_operation", getLastOperationHandler) @@ -62,16 +63,28 @@ func getCatalogHandler(w http.ResponseWriter, r *http.Request) { Name: "sample-service", Id: "edfd6e50-aa59-4688-b5bf-b21e2ab27cdb", Description: "A sample service that does nothing", - Plans: []osbapi.Plan{{ - Id: "ebf1c1df-fefb-479b-9231-ddf700a37b58", - Name: "sample", - Description: "Sample plan", - Free: true, - Bindable: true, - MaintenanceInfo: osbapi.MaintenanceInfo{ - Version: "1.2.3", + Plans: []osbapi.Plan{ + { + Id: "ebf1c1df-fefb-479b-9231-ddf700a37b58", + Name: "sample", + Description: "Sample plan", + Free: true, + Bindable: true, + MaintenanceInfo: osbapi.MaintenanceInfo{ + Version: "1.2.3", + }, }, - }}, + { + Id: "003c9135-2d40-426d-9099-d4c1b807cec1", + Name: "sample-2", + Description: "Sample plan 2", + Free: true, + Bindable: true, + MaintenanceInfo: osbapi.MaintenanceInfo{ + Version: "1.2.3", + }, + }, + }, }}, } @@ -96,6 +109,17 @@ func provisionServiceInstanceHandler(w http.ResponseWriter, r *http.Request) { asyncOperation(w, fmt.Sprintf("provision-%s", r.PathValue("id")), "{}") } +func updateServiceInstanceHandler(w http.ResponseWriter, r *http.Request) { + logRequest(r) + + if status, err := checkCredentials(w, r); err != nil { + respond(w, status, fmt.Sprintf("Credentials check failed: %v", err)) + return + } + + asyncOperation(w, fmt.Sprintf("update-%s", r.PathValue("id")), "{}") +} + func deprovisionServiceInstanceHandler(w http.ResponseWriter, r *http.Request) { logRequest(r) diff --git a/tests/e2e/e2e_suite_test.go b/tests/e2e/e2e_suite_test.go index 54b2b6775..3591242e5 100644 --- a/tests/e2e/e2e_suite_test.go +++ b/tests/e2e/e2e_suite_test.go @@ -757,6 +757,46 @@ func createUPServiceInstance(spaceGUID, name string, credentials map[string]stri return serviceInstance.GUID } +func createManagedServiceInstance(brokerGUID, spaceGUID string, name string) string { + GinkgoHelper() + + var serviceInstance typedResource + var plansResp resourceList[resource] + 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()) + + resp, err := adminClient.R(). + SetBody(typedResource{ + Type: "managed", + resource: resource{ + Name: name, + Relationships: relationships{ + "space": { + Data: resource{GUID: spaceGUID}, + }, + "service_plan": { + Data: resource{GUID: plansResp.Resources[0].GUID}, + }, + }, + }, + }). + SetResult(&serviceInstance). + Post("/v3/service_instances") + + Expect(err).NotTo(HaveOccurred()) + Expect(resp).To(SatisfyAll( + HaveRestyStatusCode(http.StatusAccepted), + HaveRestyHeaderWithValue("Location", ContainSubstring("/v3/jobs/managed_service_instance.create~")), + )) + jobURL := resp.Header().Get("Location") + expectJobCompletes(resp) + + return strings.Split(jobURL, "~")[1] +} + func listServiceInstances(names ...string) resourceList[serviceInstanceResource] { GinkgoHelper() diff --git a/tests/e2e/service_bindings_test.go b/tests/e2e/service_bindings_test.go index 2ab45aee3..c5f14102b 100644 --- a/tests/e2e/service_bindings_test.go +++ b/tests/e2e/service_bindings_test.go @@ -60,7 +60,7 @@ var _ = Describe("Service Bindings", func() { BeforeEach(func() { brokerGUID = createBroker(serviceBrokerURL) - instanceGUID = createManagedServiceInstance(brokerGUID, spaceGUID) + instanceGUID = createManagedServiceInstance(brokerGUID, spaceGUID, generateGUID("managed-service-instance")) }) AfterEach(func() { @@ -121,7 +121,7 @@ var _ = Describe("Service Bindings", func() { BeforeEach(func() { brokerGUID = createBroker(serviceBrokerURL) - instanceGUID := createManagedServiceInstance(brokerGUID, spaceGUID) + instanceGUID := createManagedServiceInstance(brokerGUID, spaceGUID, generateGUID("managed-service-instance")) bindingGUID = createManagedServiceBinding(appGUID, instanceGUID, "") }) diff --git a/tests/e2e/service_instances_test.go b/tests/e2e/service_instances_test.go index b648900f7..7050723d1 100644 --- a/tests/e2e/service_instances_test.go +++ b/tests/e2e/service_instances_test.go @@ -2,11 +2,9 @@ package e2e_test import ( "net/http" - "strings" "code.cloudfoundry.org/korifi/tests/helpers/broker" "github.com/go-resty/resty/v2" - "github.com/google/uuid" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" . "github.com/onsi/gomega/gstruct" @@ -18,10 +16,8 @@ var _ = Describe("Service Instances", func() { upsiGUID string upsiWithCredsGUID string upsiName string - // managedName string - // managedGUID string - httpResp *resty.Response - httpError error + httpResp *resty.Response + httpError error ) BeforeEach(func() { @@ -29,8 +25,6 @@ var _ = Describe("Service Instances", func() { upsiName = generateGUID("upsi-service-instance") upsiWithCredsGUID = generateGUID("upsi-service-instance-creds") upsiGUID = createUPServiceInstance(spaceGUID, upsiName, nil) - // managedName = generateGUID("managed-service-instance") - // managedGUID = createManagedServiceInstance(spaceGUID, managedName) }) AfterEach(func() { @@ -169,9 +163,24 @@ var _ = Describe("Service Instances", func() { }) Describe("Update", func() { + var ( + updateRequestBody serviceInstanceResource + serviceInstanceGUID string + brokerGUID string + plansResp resourceList[resource] + result serviceInstanceResource + ) + JustBeforeEach(func() { httpResp, httpError = adminClient.R(). - SetBody(serviceInstanceResource{ + SetBody(updateRequestBody). + Patch("/v3/service_instances/" + serviceInstanceGUID) + }) + + When("updating a user-provided service instance", func() { + BeforeEach(func() { + serviceInstanceGUID = upsiGUID + updateRequestBody = serviceInstanceResource{ resource: resource{ Name: "new-instance-name", Metadata: &metadata{ @@ -183,21 +192,91 @@ var _ = Describe("Service Instances", func() { "object-new": map[string]any{"new-a": "new-b"}, }, Tags: []string{"some", "tags"}, - }).Patch("/v3/service_instances/" + upsiGUID) + } + }) + + It("succeeds", func() { + Expect(httpError).NotTo(HaveOccurred()) + Expect(httpResp).To(HaveRestyStatusCode(http.StatusOK)) + + serviceInstances := listServiceInstances("new-instance-name") + Expect(serviceInstances.Resources).To(HaveLen(1)) + + serviceInstance := serviceInstances.Resources[0] + Expect(serviceInstance.Name).To(Equal("new-instance-name")) + Expect(serviceInstance.Metadata.Labels).To(HaveKeyWithValue("a-label", "a-label-value")) + Expect(serviceInstance.Metadata.Annotations).To(HaveKeyWithValue("an-annotation", "an-annotation-value")) + Expect(serviceInstance.Tags).To(ConsistOf("some", "tags")) + }) }) - It("succeeds", func() { - Expect(httpError).NotTo(HaveOccurred()) - Expect(httpResp).To(HaveRestyStatusCode(http.StatusOK)) + When("updating a managed service instance", func() { + BeforeEach(func() { + brokerGUID = createBroker(serviceBrokerURL) + + 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()) - serviceInstances := listServiceInstances("new-instance-name") - Expect(serviceInstances.Resources).To(HaveLen(1)) + serviceInstanceGUID = createManagedServiceInstance(brokerGUID, spaceGUID, generateGUID("managed-service-instance")) - serviceInstance := serviceInstances.Resources[0] - Expect(serviceInstance.Name).To(Equal("new-instance-name")) - Expect(serviceInstance.Metadata.Labels).To(HaveKeyWithValue("a-label", "a-label-value")) - Expect(serviceInstance.Metadata.Annotations).To(HaveKeyWithValue("an-annotation", "an-annotation-value")) - Expect(serviceInstance.Tags).To(ConsistOf("some", "tags")) + updateRequestBody = serviceInstanceResource{ + resource: resource{ + Name: "new-managed-instance-name", + Metadata: &metadata{ + Labels: map[string]string{"a-label": "a-label-value"}, + Annotations: map[string]string{"an-annotation": "an-annotation-value"}, + }, + Relationships: relationships{ + "service_plan": { + Data: resource{GUID: plansResp.Resources[1].GUID}, + }, + }, + }, + Tags: []string{"some", "tags"}, + } + }) + + AfterEach(func() { + broker.NewDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete() + }) + + It("succeeds with a job redirect", func() { + Expect(httpError).NotTo(HaveOccurred()) + Expect(httpResp).To(HaveRestyStatusCode(http.StatusAccepted)) + + Expect(httpResp).To(SatisfyAll( + HaveRestyStatusCode(http.StatusAccepted), + HaveRestyHeaderWithValue("Location", ContainSubstring("/v3/jobs/managed_service_instance.update~")), + )) + expectJobCompletes(httpResp) + }) + It("updates a managed service", func() { + Expect(httpError).NotTo(HaveOccurred()) + Expect(httpResp).To(HaveRestyStatusCode(http.StatusAccepted)) + + expectJobCompletes(httpResp) + + serviceInstances := listServiceInstances("new-managed-instance-name") + Expect(serviceInstances.Resources).To(HaveLen(1)) + + serviceInstance := serviceInstances.Resources[0] + Expect(serviceInstance.Name).To(Equal("new-managed-instance-name")) + Expect(serviceInstance.Metadata.Labels).To(HaveKeyWithValue("a-label", "a-label-value")) + Expect(serviceInstance.Metadata.Annotations).To(HaveKeyWithValue("an-annotation", "an-annotation-value")) + Expect(serviceInstance.Tags).To(ConsistOf("some", "tags")) + }) + It("changes a plan", func() { + expectJobCompletes(httpResp) + + httpRespService, httpErrorService := adminClient.R().SetResult(&result).Get("/v3/service_instances/" + serviceInstanceGUID) + Expect(httpErrorService).NotTo(HaveOccurred()) + Expect(httpRespService).To(HaveRestyStatusCode(http.StatusOK)) + Expect( + result.resource.Relationships["service_plan"].Data.GUID, + ).To(Equal(plansResp.Resources[1].GUID)) + }) }) }) @@ -230,8 +309,7 @@ var _ = Describe("Service Instances", func() { BeforeEach(func() { brokerGUID = createBroker(serviceBrokerURL) - - serviceInstanceGUID = createManagedServiceInstance(brokerGUID, spaceGUID) + serviceInstanceGUID = createManagedServiceInstance(brokerGUID, spaceGUID, generateGUID("managed-service-instance")) }) AfterEach(func() { @@ -282,47 +360,3 @@ var _ = Describe("Service Instances", func() { }) }) }) - -func createManagedServiceInstance(brokerGUID, spaceGUID string) string { - GinkgoHelper() - - var plansResp resourceList[resource] - 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()) - - createPayload := serviceInstanceResource{ - resource: resource{ - Name: uuid.NewString(), - Relationships: relationships{ - "space": { - Data: resource{ - GUID: spaceGUID, - }, - }, - "service_plan": { - Data: resource{ - GUID: plansResp.Resources[0].GUID, - }, - }, - }, - }, - InstanceType: "managed", - } - - var result serviceInstanceResource - httpResp, httpError := adminClient.R(). - SetBody(createPayload). - SetResult(&result). - Post("/v3/service_instances") - Expect(httpError).NotTo(HaveOccurred()) - Expect(httpResp).To(SatisfyAll( - HaveRestyStatusCode(http.StatusAccepted), - HaveRestyHeaderWithValue("Location", ContainSubstring("/v3/jobs/managed_service_instance.create~")), - )) - jobURL := httpResp.Header().Get("Location") - expectJobCompletes(httpResp) - - return strings.Split(jobURL, "~")[1] -} From 519c99e9a509957abc98a680ef83fcfabb2a1990 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 23 Feb 2026 14:36:43 +0100 Subject: [PATCH 09/47] Update managed service payloads instance checks --- api/payloads/service_instance.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/api/payloads/service_instance.go b/api/payloads/service_instance.go index 35ece4971..fcebed806 100644 --- a/api/payloads/service_instance.go +++ b/api/payloads/service_instance.go @@ -201,7 +201,10 @@ func (p ServiceInstancePatch) ToUPSIPatchMessage(spaceGUID, appGUID string) repo func (p ServiceInstancePatch) ToManagedSIPatchMessage(spaceGUID, appGUID string) repositories.PatchManagedSIMessage { var planGUID *string - if p.Relationships.ServicePlan.Data.GUID != "" { + if p.Relationships.ServicePlan != nil { + if p.Relationships.ServicePlan.Data.GUID != "" { + planGUID = &p.Relationships.ServicePlan.Data.GUID + } planGUID = &p.Relationships.ServicePlan.Data.GUID } return repositories.PatchManagedSIMessage{ From ab0a5e1a40137294e4eaff6a866d31432570adbe Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 23 Feb 2026 14:36:56 +0100 Subject: [PATCH 10/47] Improve managed service instance handler tests --- api/handlers/service_instance_test.go | 168 +++++++++++++------------- 1 file changed, 82 insertions(+), 86 deletions(-) diff --git a/api/handlers/service_instance_test.go b/api/handlers/service_instance_test.go index d7ade2dfe..ce6a2e919 100644 --- a/api/handlers/service_instance_test.go +++ b/api/handlers/service_instance_test.go @@ -835,104 +835,103 @@ var _ = Describe("ServiceInstance", func() { }) Describe("PATCH /v3/service_instances/:guid", func() { - When("updating a user provided service instance", func() { - BeforeEach(func() { - requestValidator.DecodeAndValidateJSONPayloadStub = decodeAndValidatePayloadStub(&payloads.ServiceInstancePatch{ - Name: tools.PtrTo("new-name"), - Tags: &[]string{"alice", "bob"}, - Credentials: &map[string]any{"foo": "bar"}, - Metadata: payloads.MetadataPatch{ - Annotations: map[string]*string{"ann2": tools.PtrTo("ann_val2")}, - Labels: map[string]*string{"lab2": tools.PtrTo("lab_val2")}, - }, - }) + BeforeEach(func() { + requestValidator.DecodeAndValidateJSONPayloadStub = decodeAndValidatePayloadStub(&payloads.ServiceInstancePatch{ + Name: tools.PtrTo("new-name"), + Tags: &[]string{"alice", "bob"}, + Credentials: &map[string]any{"foo": "bar"}, + Metadata: payloads.MetadataPatch{ + Annotations: map[string]*string{"ann2": tools.PtrTo("ann_val2")}, + Labels: map[string]*string{"lab2": tools.PtrTo("lab_val2")}, + }, + }) - serviceInstanceRepo.PatchUserProvidedServiceInstanceReturns(repositories.ServiceInstanceRecord{ - Name: "new-name", - GUID: "service-instance-guid", - }, nil) + serviceInstanceRepo.PatchUserProvidedServiceInstanceReturns(repositories.ServiceInstanceRecord{ + Name: "new-name", + GUID: "service-instance-guid", + }, nil) - reqPath += "/service-instance-guid" - reqMethod = http.MethodPatch - }) - It("patches the user provided service instance", func() { - Expect(requestValidator.DecodeAndValidateJSONPayloadCallCount()).To(Equal(1)) - actualReq, _ := requestValidator.DecodeAndValidateJSONPayloadArgsForCall(0) - Expect(bodyString(actualReq)).To(Equal("the-json-body")) + reqPath += "/service-instance-guid" + reqMethod = http.MethodPatch + }) - Expect(serviceInstanceRepo.GetServiceInstanceCallCount()).To(Equal(1)) - _, actualAuthInfo, actualGUID := serviceInstanceRepo.GetServiceInstanceArgsForCall(0) - Expect(actualAuthInfo).To(Equal(authInfo)) - Expect(actualGUID).To(Equal("service-instance-guid")) + It("patches the service instance", func() { + Expect(requestValidator.DecodeAndValidateJSONPayloadCallCount()).To(Equal(1)) + actualReq, _ := requestValidator.DecodeAndValidateJSONPayloadArgsForCall(0) + Expect(bodyString(actualReq)).To(Equal("the-json-body")) - Expect(serviceInstanceRepo.PatchUserProvidedServiceInstanceCallCount()).To(Equal(1)) - _, actualAuthInfo, patchMessage := serviceInstanceRepo.PatchUserProvidedServiceInstanceArgsForCall(0) - Expect(actualAuthInfo).To(Equal(authInfo)) - Expect(patchMessage).To(Equal(repositories.PatchUPSIMessage{ - GUID: "service-instance-guid", - SpaceGUID: "space-guid", - Name: tools.PtrTo("new-name"), - Credentials: &map[string]any{"foo": "bar"}, - Tags: &[]string{"alice", "bob"}, - MetadataPatch: repositories.MetadataPatch{ - Annotations: map[string]*string{"ann2": tools.PtrTo("ann_val2")}, - Labels: map[string]*string{"lab2": tools.PtrTo("lab_val2")}, - }, - })) + Expect(serviceInstanceRepo.GetServiceInstanceCallCount()).To(Equal(1)) + _, actualAuthInfo, actualGUID := serviceInstanceRepo.GetServiceInstanceArgsForCall(0) + Expect(actualAuthInfo).To(Equal(authInfo)) + Expect(actualGUID).To(Equal("service-instance-guid")) - Expect(rr).To(HaveHTTPStatus(http.StatusOK)) - Expect(rr).To(HaveHTTPHeaderWithValue("Content-Type", "application/json")) - Expect(rr).To(HaveHTTPBody(SatisfyAll( - MatchJSONPath("$.guid", "service-instance-guid"), - MatchJSONPath("$.name", "new-name"), - MatchJSONPath("$.links.self.href", "https://api.example.org/v3/service_instances/service-instance-guid"), - ))) - }) + Expect(serviceInstanceRepo.PatchUserProvidedServiceInstanceCallCount()).To(Equal(1)) + _, actualAuthInfo, patchMessage := serviceInstanceRepo.PatchUserProvidedServiceInstanceArgsForCall(0) + Expect(actualAuthInfo).To(Equal(authInfo)) + Expect(patchMessage).To(Equal(repositories.PatchUPSIMessage{ + GUID: "service-instance-guid", + SpaceGUID: "space-guid", + Name: tools.PtrTo("new-name"), + Credentials: &map[string]any{"foo": "bar"}, + Tags: &[]string{"alice", "bob"}, + MetadataPatch: repositories.MetadataPatch{ + Annotations: map[string]*string{"ann2": tools.PtrTo("ann_val2")}, + Labels: map[string]*string{"lab2": tools.PtrTo("lab_val2")}, + }, + })) - When("decoding the payload fails", func() { - BeforeEach(func() { - requestValidator.DecodeAndValidateJSONPayloadReturns(apierrors.NewUnprocessableEntityError(nil, "nope")) - }) + Expect(rr).To(HaveHTTPStatus(http.StatusOK)) + Expect(rr).To(HaveHTTPHeaderWithValue("Content-Type", "application/json")) + Expect(rr).To(HaveHTTPBody(SatisfyAll( + MatchJSONPath("$.guid", "service-instance-guid"), + MatchJSONPath("$.name", "new-name"), + MatchJSONPath("$.links.self.href", "https://api.example.org/v3/service_instances/service-instance-guid"), + ))) + }) - It("returns an error", func() { - expectUnprocessableEntityError("nope") - }) + When("decoding the payload fails", func() { + BeforeEach(func() { + requestValidator.DecodeAndValidateJSONPayloadReturns(apierrors.NewUnprocessableEntityError(nil, "nope")) }) - When("getting the service instance fails with not found", func() { - BeforeEach(func() { - serviceInstanceRepo.GetServiceInstanceReturns( - repositories.ServiceInstanceRecord{}, - apierrors.NewNotFoundError(nil, repositories.ServiceInstanceResourceType), - ) - }) + It("returns an error", func() { + expectUnprocessableEntityError("nope") + }) + }) - It("returns 404 Not Found", func() { - expectNotFoundError("Service Instance") - }) + When("getting the service instance fails with not found", func() { + BeforeEach(func() { + serviceInstanceRepo.GetServiceInstanceReturns( + repositories.ServiceInstanceRecord{}, + apierrors.NewNotFoundError(nil, repositories.ServiceInstanceResourceType), + ) }) - When("getting the service instance fails with forbidden", func() { - BeforeEach(func() { - serviceInstanceRepo.GetServiceInstanceReturns( - repositories.ServiceInstanceRecord{}, - apierrors.NewForbiddenError(nil, repositories.ServiceInstanceResourceType), - ) - }) + It("returns 404 Not Found", func() { + expectNotFoundError("Service Instance") + }) + }) - It("returns 404 Not Found", func() { - expectNotFoundError("Service Instance") - }) + When("getting the service instance fails with forbidden", func() { + BeforeEach(func() { + serviceInstanceRepo.GetServiceInstanceReturns( + repositories.ServiceInstanceRecord{}, + apierrors.NewForbiddenError(nil, repositories.ServiceInstanceResourceType), + ) }) - When("patching the user provided service instances fails", func() { - BeforeEach(func() { - serviceInstanceRepo.PatchUserProvidedServiceInstanceReturns(repositories.ServiceInstanceRecord{}, errors.New("oops")) - }) + It("returns 404 Not Found", func() { + expectNotFoundError("Service Instance") + }) + }) - It("returns the error", func() { - expectUnknownError() - }) + When("patching the service instances fails", func() { + BeforeEach(func() { + serviceInstanceRepo.PatchUserProvidedServiceInstanceReturns(repositories.ServiceInstanceRecord{}, errors.New("oops")) + }) + + It("returns the error", func() { + expectUnknownError() }) }) When("updating a managed service instance", func() { @@ -964,9 +963,6 @@ var _ = Describe("ServiceInstance", func() { Name: "new-name", GUID: "service-instance-guid", }, nil) - - reqPath += "/service-instance-guid" - reqMethod = http.MethodPatch }) It("patches the managed service instance", func() { @@ -992,7 +988,7 @@ var _ = Describe("ServiceInstance", func() { It("returns HTTP 202 Accepted response", func() { Expect(rr).To(HaveHTTPStatus(http.StatusAccepted)) Expect(rr).To(HaveHTTPHeaderWithValue("Location", - ContainSubstring("/v3/jobs/managed_service_instance.patch~service-instance-guid"))) + ContainSubstring("/v3/jobs/managed_service_instance.update~service-instance-guid"))) }) When("patching the managed service instances fails", func() { BeforeEach(func() { From 10a5434aee3dd2c4288cbb3fe8e69fcd75895fab Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 23 Feb 2026 16:47:27 +0100 Subject: [PATCH 11/47] Cover repositories/PatchManagedServiceInstance with tests --- api/repositories/repositories_suite_test.go | 22 ++- .../service_instance_repository_test.go | 127 +++++++++++++++++- 2 files changed, 142 insertions(+), 7 deletions(-) diff --git a/api/repositories/repositories_suite_test.go b/api/repositories/repositories_suite_test.go index 610e4d78d..2b1fdb8ad 100644 --- a/api/repositories/repositories_suite_test.go +++ b/api/repositories/repositories_suite_test.go @@ -366,7 +366,7 @@ func createAppCR(ctx context.Context, k8sClient client.Client, appName, appGUID, return toReturn } -func createServiceInstanceCR(ctx context.Context, k8sClient client.Client, serviceInstanceGUID, spaceGUID, name, secretName string) *korifiv1alpha1.CFServiceInstance { +func createUserProvidedServiceInstanceCR(ctx context.Context, k8sClient client.Client, serviceInstanceGUID, spaceGUID, name, secretName string) *korifiv1alpha1.CFServiceInstance { serviceInstance := &korifiv1alpha1.CFServiceInstance{ ObjectMeta: metav1.ObjectMeta{ Name: serviceInstanceGUID, @@ -386,6 +386,26 @@ func createServiceInstanceCR(ctx context.Context, k8sClient client.Client, servi return serviceInstance } +func createManagedServiceInstanceCR(ctx context.Context, k8sClient client.Client, serviceInstanceGUID, spaceGUID, name, plan string) *korifiv1alpha1.CFServiceInstance { + serviceInstance := &korifiv1alpha1.CFServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: serviceInstanceGUID, + Namespace: spaceGUID, + Labels: map[string]string{"a-label": "a-label-value"}, + Annotations: map[string]string{"an-annotation": "an-annotation-value"}, + }, + Spec: korifiv1alpha1.CFServiceInstanceSpec{ + DisplayName: name, + Type: "managed", + Tags: []string{"database", "mysql"}, + PlanGUID: plan, + }, + } + Expect(k8sClient.Create(ctx, serviceInstance)).To(Succeed()) + + return serviceInstance +} + func createApp(space string) *korifiv1alpha1.CFApp { return createAppWithGUID(space, uuid.NewString()) } diff --git a/api/repositories/service_instance_repository_test.go b/api/repositories/service_instance_repository_test.go index c06e9c27a..6f485650c 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -503,15 +503,16 @@ var _ = Describe("ServiceInstanceRepository", func() { var ( cfServiceInstance *korifiv1alpha1.CFServiceInstance secret *corev1.Secret + serviceInstanceGUID string serviceInstanceRecord repositories.ServiceInstanceRecord patchMessage repositories.PatchUPSIMessage err error ) BeforeEach(func() { - serviceInstanceGUID := uuid.NewString() + serviceInstanceGUID = uuid.NewString() secretName := uuid.NewString() - cfServiceInstance = createServiceInstanceCR(ctx, k8sClient, serviceInstanceGUID, space.Name, serviceInstanceName, secretName) + cfServiceInstance = createUserProvidedServiceInstanceCR(ctx, k8sClient, serviceInstanceGUID, space.Name, serviceInstanceName, secretName) conditionAwaiter.AwaitConditionReturns(cfServiceInstance, nil) Expect(k8s.Patch(ctx, k8sClient, cfServiceInstance, func() { cfServiceInstance.Status.Credentials.Name = secretName @@ -686,6 +687,120 @@ var _ = Describe("ServiceInstanceRepository", func() { }) }) }) + + When("Patching a managed service instance", func() { + var ( + patchMessage repositories.PatchManagedSIMessage + ) + + BeforeEach(func() { + serviceInstancePlanGUID := uuid.NewString() + cfServiceInstance = createManagedServiceInstanceCR(ctx, k8sClient, serviceInstanceGUID, space.Name, serviceInstanceName, serviceInstancePlanGUID) + conditionAwaiter.AwaitConditionReturns(cfServiceInstance, nil) + + Expect(k8sClient.Create(ctx, cfServiceInstance)).To(Succeed()) + + patchMessage = repositories.PatchManagedSIMessage{ + GUID: cfServiceInstance.Name, + SpaceGUID: space.Name, + Name: tools.PtrTo("new-name"), + PlanGUID: tools.PtrTo("new-plan-guid"), + Tags: &[]string{"new"}, + MetadataPatch: repositories.MetadataPatch{ + Labels: map[string]*string{"new-label": tools.PtrTo("new-label-value")}, + Annotations: map[string]*string{"new-annotation": tools.PtrTo("new-annotation-value")}, + }, + } + }) + + JustBeforeEach(func() { + serviceInstanceRecord, err = serviceInstanceRepo.PatchManagedServiceInstance(ctx, authInfo, patchMessage) + }) + + When("authorized in the space", func() { + BeforeEach(func() { + createRoleBinding(ctx, userName, orgUserRole.Name, org.Name) + createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space.Name) + }) + + It("returns the updated record", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(serviceInstanceRecord.Name).To(Equal("new-name")) + Expect(serviceInstanceRecord.Tags).To(ConsistOf("new")) + Expect(serviceInstanceRecord.Labels).To(HaveKeyWithValue("a-label", "a-label-value")) + Expect(serviceInstanceRecord.Labels).To(HaveKeyWithValue("new-label", "new-label-value")) + Expect(serviceInstanceRecord.Annotations).To(HaveLen(2)) + Expect(serviceInstanceRecord.Annotations).To(HaveKeyWithValue("an-annotation", "an-annotation-value")) + Expect(serviceInstanceRecord.Annotations).To(HaveKeyWithValue("new-annotation", "new-annotation-value")) + Expect(serviceInstanceRecord.Relationships()).To(HaveKeyWithValue("service_plan", "new-plan-guid")) + }) + + It("updates the service instance", func() { + Expect(err).NotTo(HaveOccurred()) + serviceInstance := new(korifiv1alpha1.CFServiceInstance) + + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cfServiceInstance), serviceInstance)).To(Succeed()) + g.Expect(serviceInstance.Spec.DisplayName).To(Equal("new-name")) + g.Expect(serviceInstance.Spec.Tags).To(ConsistOf("new")) + g.Expect(serviceInstance.Spec.PlanGUID).To(Equal("new-plan-guid")) + g.Expect(serviceInstance.Labels).To(HaveKeyWithValue("a-label", "a-label-value")) + g.Expect(serviceInstance.Labels).To(HaveKeyWithValue("new-label", "new-label-value")) + g.Expect(serviceInstance.Annotations).To(HaveLen(2)) + g.Expect(serviceInstance.Annotations).To(HaveKeyWithValue("an-annotation", "an-annotation-value")) + g.Expect(serviceInstance.Annotations).To(HaveKeyWithValue("new-annotation", "new-annotation-value")) + }).Should(Succeed()) + }) + + When("tags is an empty list", func() { + BeforeEach(func() { + patchMessage.Tags = &[]string{} + }) + + It("clears the tags", func() { + Expect(err).NotTo(HaveOccurred()) + serviceInstance := new(korifiv1alpha1.CFServiceInstance) + + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cfServiceInstance), serviceInstance)).To(Succeed()) + g.Expect(serviceInstance.Spec.Tags).To(BeEmpty()) + }).Should(Succeed()) + }) + }) + + When("tags is nil", func() { + BeforeEach(func() { + patchMessage.Tags = nil + }) + + It("preserves the tags", func() { + Expect(err).NotTo(HaveOccurred()) + serviceInstance := new(korifiv1alpha1.CFServiceInstance) + + Consistently(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cfServiceInstance), serviceInstance)).To(Succeed()) + g.Expect(serviceInstance.Spec.Tags).To(ConsistOf("database", "mysql")) + }).Should(Succeed()) + }) + }) + + When("the plan GUID is nil", func() { + BeforeEach(func() { + patchMessage.PlanGUID = nil + }) + + It("preserves the plan GUID", func() { + Expect(err).NotTo(HaveOccurred()) + serviceInstance := new(korifiv1alpha1.CFServiceInstance) + + Consistently(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cfServiceInstance), serviceInstance)).To(Succeed()) + g.Expect(serviceInstance.Spec.PlanGUID).To(Equal("plan-guid")) + }).Should(Succeed()) + }) + }) + }) + }) }) Describe("ListServiceInstances", func() { @@ -815,8 +930,8 @@ var _ = Describe("ServiceInstanceRepository", func() { BeforeEach(func() { space2 = createSpaceWithCleanup(ctx, org.Name, prefixedGUID("space2")) - serviceInstance = createServiceInstanceCR(ctx, k8sClient, prefixedGUID("service-instance"), space.Name, "the-service-instance", prefixedGUID("secret")) - createServiceInstanceCR(ctx, k8sClient, prefixedGUID("service-instance"), space2.Name, "some-other-service-instance", prefixedGUID("secret")) + serviceInstance = createUserProvidedServiceInstanceCR(ctx, k8sClient, prefixedGUID("service-instance"), space.Name, "the-service-instance", prefixedGUID("secret")) + createUserProvidedServiceInstanceCR(ctx, k8sClient, prefixedGUID("service-instance"), space2.Name, "some-other-service-instance", prefixedGUID("secret")) getGUID = serviceInstance.Name }) @@ -902,7 +1017,7 @@ var _ = Describe("ServiceInstanceRepository", func() { BeforeEach(func() { createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space.Name) createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space2.Name) - createServiceInstanceCR(ctx, k8sClient, getGUID, space2.Name, "the-service-instance", prefixedGUID("secret")) + createUserProvidedServiceInstanceCR(ctx, k8sClient, getGUID, space2.Name, "the-service-instance", prefixedGUID("secret")) }) It("returns a error", func() { @@ -1013,7 +1128,7 @@ var _ = Describe("ServiceInstanceRepository", func() { ) BeforeEach(func() { - serviceInstance = createServiceInstanceCR(ctx, k8sClient, prefixedGUID("service-instance"), space.Name, "the-service-instance", prefixedGUID("secret")) + serviceInstance = createUserProvidedServiceInstanceCR(ctx, k8sClient, prefixedGUID("service-instance"), space.Name, "the-service-instance", prefixedGUID("secret")) deleteMessage = repositories.DeleteServiceInstanceMessage{ GUID: serviceInstance.Name, From 0a1ac06d1be23f003cae77a0b7e9626368c861e7 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 23 Feb 2026 16:53:11 +0100 Subject: [PATCH 12/47] Update cfcerviceinstance_types comments for PlanGUID --- controllers/api/v1alpha1/cfserviceinstance_types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/api/v1alpha1/cfserviceinstance_types.go b/controllers/api/v1alpha1/cfserviceinstance_types.go index bb469d5c3..1109a720b 100644 --- a/controllers/api/v1alpha1/cfserviceinstance_types.go +++ b/controllers/api/v1alpha1/cfserviceinstance_types.go @@ -91,7 +91,7 @@ type CFServiceInstanceStatus struct { //+kubebuilder:validation:Optional MaintenanceInfo MaintenanceInfo `json:"maintenanceInfo"` - // The service instance actual plan + // The service instance actual plan. Only makes sense for managed service instances //+kubebuilder:validation:Optional PlanGUID string `json:"planGuid"` From 5e0bd7aef04ff6c006d3dc68e54b7a70ac4d1dc1 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 23 Feb 2026 17:42:07 +0100 Subject: [PATCH 13/47] Update cfcerviceinstance_types comments for PlanGUID --- .../crds/korifi.cloudfoundry.org_cfserviceinstances.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml b/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml index 064130312..5298f2055 100644 --- a/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml +++ b/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml @@ -218,7 +218,8 @@ spec: format: int64 type: integer planGuid: - description: The service instance actual plan + description: The service instance actual plan. Only makes sense for + managed service instances type: string upgradeAvailable: description: True if there is an upgrade available for for the service From a3c00fce727c8f46bc4f9d7ba80d19f24f650453 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 23 Feb 2026 17:43:43 +0100 Subject: [PATCH 14/47] Update managed service controller failing reason to ServiceInstanceFailed --- api/repositories/service_instance_repository_test.go | 4 +--- .../controllers/services/instances/managed/controller.go | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/api/repositories/service_instance_repository_test.go b/api/repositories/service_instance_repository_test.go index 6f485650c..703ec5193 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -689,9 +689,7 @@ var _ = Describe("ServiceInstanceRepository", func() { }) When("Patching a managed service instance", func() { - var ( - patchMessage repositories.PatchManagedSIMessage - ) + var patchMessage repositories.PatchManagedSIMessage BeforeEach(func() { serviceInstancePlanGUID := uuid.NewString() diff --git a/controllers/controllers/services/instances/managed/controller.go b/controllers/controllers/services/instances/managed/controller.go index e8ca20c5d..8a95ce5ce 100644 --- a/controllers/controllers/services/instances/managed/controller.go +++ b/controllers/controllers/services/instances/managed/controller.go @@ -148,7 +148,7 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor } if isFailed(serviceInstance) { - return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ReconciliationFailed").WithNoRequeue() + return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ServiceInstanceFailed").WithNoRequeue() } planVisible, err := r.isServicePlanVisible(ctx, serviceInstance, serviceInstanceAssets.ServicePlan) From 309185d616d35c54fb0c878488456cd0c5da906e Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Tue, 24 Feb 2026 16:16:35 +0100 Subject: [PATCH 15/47] Update provisioned status field for managed instance --- controllers/api/v1alpha1/cfserviceinstance_types.go | 4 ++-- .../services/instances/managed/controller.go | 4 ++-- .../services/instances/managed/controller_test.go | 10 +++++----- .../korifi.cloudfoundry.org_cfserviceinstances.yaml | 8 ++++---- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/controllers/api/v1alpha1/cfserviceinstance_types.go b/controllers/api/v1alpha1/cfserviceinstance_types.go index 1109a720b..420596acb 100644 --- a/controllers/api/v1alpha1/cfserviceinstance_types.go +++ b/controllers/api/v1alpha1/cfserviceinstance_types.go @@ -91,9 +91,9 @@ type CFServiceInstanceStatus struct { //+kubebuilder:validation:Optional MaintenanceInfo MaintenanceInfo `json:"maintenanceInfo"` - // The service instance actual plan. Only makes sense for managed service instances + // True if the service instance has been successfully provisioned. Only makes seense for managed service instances //+kubebuilder:validation:Optional - PlanGUID string `json:"planGuid"` + Provisioned bool `json:"provisioned"` // True if there is an upgrade available for for the service instance (i.e. the plan has a new version). Only makes seense for managed service instances //+kubebuilder:validation:Optional diff --git a/controllers/controllers/services/instances/managed/controller.go b/controllers/controllers/services/instances/managed/controller.go index 8a95ce5ce..530332a4f 100644 --- a/controllers/controllers/services/instances/managed/controller.go +++ b/controllers/controllers/services/instances/managed/controller.go @@ -166,7 +166,7 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor serviceInstance.Spec.ServiceLabel = tools.PtrTo(serviceInstanceAssets.ServiceOffering.Spec.Name) } - if serviceInstance.Status.PlanGUID == "" { + if !serviceInstance.Status.Provisioned { provisionResponse, err := r.provisionServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient) if err != nil { log.Error(err, "failed to provision service instance") @@ -195,7 +195,7 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor } } - serviceInstance.Status.PlanGUID = serviceInstance.Spec.PlanGUID + serviceInstance.Status.Provisioned = true serviceInstance.Status.MaintenanceInfo = serviceInstanceAssets.ServicePlan.Spec.MaintenanceInfo serviceInstance.Status.LastOperation.State = "succeeded" return ctrl.Result{}, nil diff --git a/controllers/controllers/services/instances/managed/controller_test.go b/controllers/controllers/services/instances/managed/controller_test.go index ecffc22de..c5ea4104c 100644 --- a/controllers/controllers/services/instances/managed/controller_test.go +++ b/controllers/controllers/services/instances/managed/controller_test.go @@ -150,10 +150,10 @@ var _ = Describe("CFServiceInstance", func() { }).Should(Succeed()) }) - It("sets the PlanGUID status field", func() { + It("sets the Provisioned status field", func() { Eventually(func(g Gomega) { g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) - g.Expect(instance.Status.PlanGUID).To(Equal(instance.Spec.PlanGUID)) + g.Expect(instance.Status.Provisioned).To(BeTrue()) }).Should(Succeed()) }) @@ -794,7 +794,7 @@ var _ = Describe("CFServiceInstance", func() { HasType(Equal(korifiv1alpha1.StatusConditionReady)), HasStatus(Equal(metav1.ConditionTrue)), ))) - g.Expect(instance.Status.PlanGUID).To(Equal(servicePlan.Name)) + g.Expect(instance.Status.Provisioned).To(BeTrue()) }).Should(Succeed()) Eventually(func(g Gomega) { @@ -826,8 +826,8 @@ var _ = Describe("CFServiceInstance", func() { HasType(Equal(korifiv1alpha1.StatusConditionReady)), HasStatus(Equal(metav1.ConditionTrue)), ))) - g.Expect(instance.Status.PlanGUID).To(Equal(instance.Spec.PlanGUID)) - g.Expect(instance.Status.PlanGUID).To(Equal(servicePlan2.Name)) + g.Expect(instance.Status.Provisioned).To(BeTrue()) + g.Expect(instance.Spec.PlanGUID).To(Equal(servicePlan2.Name)) }).Should(Succeed()) }) diff --git a/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml b/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml index 5298f2055..49d751232 100644 --- a/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml +++ b/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml @@ -217,10 +217,10 @@ spec: the CFServiceInstance that has been reconciled format: int64 type: integer - planGuid: - description: The service instance actual plan. Only makes sense for - managed service instances - type: string + provisioned: + description: True if the service instance has been successfully provisioned. + Only makes seense for managed service instances + type: boolean upgradeAvailable: description: True if there is an upgrade available for for the service instance (i.e. the plan has a new version). Only makes seense for From b1a9a0d613c5cae671a18ad4ead2b7c3826459a7 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Wed, 25 Feb 2026 10:54:11 +0100 Subject: [PATCH 16/47] Clean up setObservedGeneration method in managed service controller --- .../controllers/services/instances/managed/controller.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/controllers/controllers/services/instances/managed/controller.go b/controllers/controllers/services/instances/managed/controller.go index 530332a4f..85dca872e 100644 --- a/controllers/controllers/services/instances/managed/controller.go +++ b/controllers/controllers/services/instances/managed/controller.go @@ -265,7 +265,6 @@ func (r *Reconciler) processProvisionOperation( lastOpResponse osbapi.LastOperationResponse, ) (ctrl.Result, error) { if lastOpResponse.State == "succeeded" { - setObservedGeneration(serviceInstance) return ctrl.Result{}, nil } @@ -278,7 +277,6 @@ func (r *Reconciler) processProvisionOperation( Reason: "ProvisionFailed", Message: lastOpResponse.Description, }) - setObservedGeneration(serviceInstance) return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ProvisionFailed") } @@ -335,7 +333,6 @@ func (r *Reconciler) processUpdateOperation( lastOpResponse osbapi.LastOperationResponse, ) (ctrl.Result, error) { if lastOpResponse.State == "succeeded" { - setObservedGeneration(serviceInstance) return ctrl.Result{}, nil } @@ -348,7 +345,6 @@ func (r *Reconciler) processUpdateOperation( Reason: "UpdateFailed", Message: lastOpResponse.Description, }) - setObservedGeneration(serviceInstance) return ctrl.Result{}, k8s.NewNotReadyError().WithReason("UpdateFailed") } @@ -359,7 +355,6 @@ func (r *Reconciler) finalize( ctx context.Context, serviceInstance *korifiv1alpha1.CFServiceInstance, ) (ctrl.Result, error) { - setObservedGeneration(serviceInstance) if !controllerutil.ContainsFinalizer(serviceInstance, korifiv1alpha1.CFServiceInstanceFinalizerName) { return ctrl.Result{}, nil } @@ -568,7 +563,3 @@ func isFailed(instance *korifiv1alpha1.CFServiceInstance) bool { func isReady(instance *korifiv1alpha1.CFServiceInstance) bool { return meta.IsStatusConditionTrue(instance.Status.Conditions, korifiv1alpha1.StatusConditionReady) } - -func setObservedGeneration(instance *korifiv1alpha1.CFServiceInstance) { - instance.Status.ObservedGeneration = instance.Generation -} From c471fba7ff85194aba6e2fcc0363b37c4b771a72 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Thu, 26 Feb 2026 16:40:03 +0100 Subject: [PATCH 17/47] Managed controller allow consequent retries on UpdateFailedCondition --- .../controllers/services/instances/managed/controller.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/controllers/controllers/services/instances/managed/controller.go b/controllers/controllers/services/instances/managed/controller.go index 85dca872e..2458c4ef5 100644 --- a/controllers/controllers/services/instances/managed/controller.go +++ b/controllers/controllers/services/instances/managed/controller.go @@ -556,8 +556,7 @@ func (r *Reconciler) getNamespace(ctx context.Context, namespaceName string) (*c } func isFailed(instance *korifiv1alpha1.CFServiceInstance) bool { - return meta.IsStatusConditionTrue(instance.Status.Conditions, korifiv1alpha1.ProvisioningFailedCondition) || - meta.IsStatusConditionTrue(instance.Status.Conditions, korifiv1alpha1.UpdateFailedCondition) + return meta.IsStatusConditionTrue(instance.Status.Conditions, korifiv1alpha1.ProvisioningFailedCondition) } func isReady(instance *korifiv1alpha1.CFServiceInstance) bool { From 5bd8b849bab8b9bd49e7f0e901cdb0d41b1977da Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Thu, 26 Feb 2026 16:41:01 +0100 Subject: [PATCH 18/47] Managed controller refine plan change test --- .../instances/managed/controller_test.go | 57 ++++++++++--------- 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/controllers/controllers/services/instances/managed/controller_test.go b/controllers/controllers/services/instances/managed/controller_test.go index c5ea4104c..f39161842 100644 --- a/controllers/controllers/services/instances/managed/controller_test.go +++ b/controllers/controllers/services/instances/managed/controller_test.go @@ -25,11 +25,12 @@ import ( var _ = Describe("CFServiceInstance", func() { var ( - brokerClient *fake.BrokerClient - instance *korifiv1alpha1.CFServiceInstance - serviceBroker *korifiv1alpha1.CFServiceBroker - servicePlan *korifiv1alpha1.CFServicePlan - servicePlan2 *korifiv1alpha1.CFServicePlan + brokerClient *fake.BrokerClient + instance *korifiv1alpha1.CFServiceInstance + serviceBroker *korifiv1alpha1.CFServiceBroker + serviceOffering *korifiv1alpha1.CFServiceOffering + servicePlan *korifiv1alpha1.CFServicePlan + servicePlan2 *korifiv1alpha1.CFServicePlan ) BeforeEach(func() { @@ -103,29 +104,6 @@ var _ = Describe("CFServiceInstance", func() { } Expect(adminClient.Create(ctx, servicePlan)).To(Succeed()) - servicePlan2 = &korifiv1alpha1.CFServicePlan{ - ObjectMeta: metav1.ObjectMeta{ - Name: uuid.NewString(), - Namespace: rootNamespace, - Labels: map[string]string{ - korifiv1alpha1.RelServiceBrokerGUIDLabel: serviceBroker.Name, - korifiv1alpha1.RelServiceOfferingGUIDLabel: serviceOffering.Name, - }, - }, - Spec: korifiv1alpha1.CFServicePlanSpec{ - Visibility: korifiv1alpha1.ServicePlanVisibility{ - Type: "public", - }, - BrokerCatalog: korifiv1alpha1.ServicePlanBrokerCatalog{ - ID: "service-plan-id-2", - }, - MaintenanceInfo: korifiv1alpha1.MaintenanceInfo{ - Version: "1.2.3", - }, - }, - } - Expect(adminClient.Create(ctx, servicePlan2)).To(Succeed()) - instance = &korifiv1alpha1.CFServiceInstance{ ObjectMeta: metav1.ObjectMeta{ Name: uuid.NewString(), @@ -786,6 +764,29 @@ var _ = Describe("CFServiceInstance", func() { When("updates instance", func() { BeforeEach(func() { brokerClient.UpdateReturns(osbapi.UpdateResponse{}, nil) + + servicePlan2 = &korifiv1alpha1.CFServicePlan{ + ObjectMeta: metav1.ObjectMeta{ + Name: uuid.NewString(), + Namespace: rootNamespace, + Labels: map[string]string{ + korifiv1alpha1.RelServiceBrokerGUIDLabel: serviceBroker.Name, + korifiv1alpha1.RelServiceOfferingGUIDLabel: serviceOffering.Name, + }, + }, + Spec: korifiv1alpha1.CFServicePlanSpec{ + Visibility: korifiv1alpha1.ServicePlanVisibility{ + Type: "public", + }, + BrokerCatalog: korifiv1alpha1.ServicePlanBrokerCatalog{ + ID: "service-plan-id-2", + }, + MaintenanceInfo: korifiv1alpha1.MaintenanceInfo{ + Version: "1.2.3", + }, + }, + } + Expect(adminClient.Create(ctx, servicePlan2)).To(Succeed()) }) JustBeforeEach(func() { Eventually(func(g Gomega) { From cc66a02f0711a10c43a421d2f4d68985f27ca709 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Thu, 26 Feb 2026 16:42:14 +0100 Subject: [PATCH 19/47] Managed controller tests refine test names --- .../controllers/services/instances/managed/controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/controllers/services/instances/managed/controller_test.go b/controllers/controllers/services/instances/managed/controller_test.go index f39161842..1fe7eba67 100644 --- a/controllers/controllers/services/instances/managed/controller_test.go +++ b/controllers/controllers/services/instances/managed/controller_test.go @@ -761,7 +761,7 @@ var _ = Describe("CFServiceInstance", func() { }) }) - When("updates instance", func() { + When("the service instance is being updated", func() { BeforeEach(func() { brokerClient.UpdateReturns(osbapi.UpdateResponse{}, nil) From ee03e3cc2c2f3043145b3af4b173c3a5612219a2 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Thu, 26 Feb 2026 17:46:15 +0100 Subject: [PATCH 20/47] Managed controller tests remove extraneous ServicePlan2 creation check --- .../services/instances/managed/controller_test.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/controllers/controllers/services/instances/managed/controller_test.go b/controllers/controllers/services/instances/managed/controller_test.go index 1fe7eba67..5e33209fa 100644 --- a/controllers/controllers/services/instances/managed/controller_test.go +++ b/controllers/controllers/services/instances/managed/controller_test.go @@ -64,7 +64,7 @@ var _ = Describe("CFServiceInstance", func() { } Expect(adminClient.Create(ctx, namespace)).To(Succeed()) - serviceOffering := &korifiv1alpha1.CFServiceOffering{ + serviceOffering = &korifiv1alpha1.CFServiceOffering{ ObjectMeta: metav1.ObjectMeta{ Name: uuid.NewString(), Namespace: rootNamespace, @@ -798,10 +798,6 @@ var _ = Describe("CFServiceInstance", func() { g.Expect(instance.Status.Provisioned).To(BeTrue()) }).Should(Succeed()) - Eventually(func(g Gomega) { - g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(servicePlan2), servicePlan2)).To(Succeed()) - }).Should(Succeed()) - Expect(k8s.Patch(ctx, adminClient, instance, func() { instance.Spec.PlanGUID = servicePlan2.Name meta.SetStatusCondition(&instance.Status.Conditions, metav1.Condition{ From 780877887cedca8cad9afc75eebb9d20b6e2e72c Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Thu, 26 Feb 2026 17:56:58 +0100 Subject: [PATCH 21/47] Managed controller tests remove extraneous ObservedGeneration field check --- .../services/instances/managed/controller_test.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/controllers/controllers/services/instances/managed/controller_test.go b/controllers/controllers/services/instances/managed/controller_test.go index 5e33209fa..080cb6b05 100644 --- a/controllers/controllers/services/instances/managed/controller_test.go +++ b/controllers/controllers/services/instances/managed/controller_test.go @@ -809,13 +809,6 @@ var _ = Describe("CFServiceInstance", func() { })).To(Succeed()) }) - It("sets the ObservedGeneration status field", func() { - Eventually(func(g Gomega) { - g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) - g.Expect(instance.Status.ObservedGeneration).To(Equal(instance.Generation)) - }).Should(Succeed()) - }) - It("sets the PlanGUID status field", func() { Eventually(func(g Gomega) { g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) From 4e7355c688e92eb4dc0266a309035a06def8d0d9 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Thu, 26 Feb 2026 17:57:40 +0100 Subject: [PATCH 22/47] Managed controller tests refine test names --- .../controllers/services/instances/managed/controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/controllers/services/instances/managed/controller_test.go b/controllers/controllers/services/instances/managed/controller_test.go index 080cb6b05..43ff6788f 100644 --- a/controllers/controllers/services/instances/managed/controller_test.go +++ b/controllers/controllers/services/instances/managed/controller_test.go @@ -809,7 +809,7 @@ var _ = Describe("CFServiceInstance", func() { })).To(Succeed()) }) - It("sets the PlanGUID status field", func() { + It("updates the PlanGUID status field", func() { Eventually(func(g Gomega) { g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) g.Expect(instance.Status.Conditions).To(ContainElement(SatisfyAll( From 0f1e6dbcf0a142c7571bd42587bb7f3ed33b018c Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Thu, 26 Feb 2026 17:59:45 +0100 Subject: [PATCH 23/47] Managed controller tests remove extraneous Ready condition check --- .../services/instances/managed/controller_test.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/controllers/controllers/services/instances/managed/controller_test.go b/controllers/controllers/services/instances/managed/controller_test.go index 43ff6788f..2f34a13fb 100644 --- a/controllers/controllers/services/instances/managed/controller_test.go +++ b/controllers/controllers/services/instances/managed/controller_test.go @@ -821,16 +821,6 @@ var _ = Describe("CFServiceInstance", func() { }).Should(Succeed()) }) - It("sets the Ready condition to True", 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)), - ))) - }).Should(Succeed()) - }) - When("service update fails with recoverable error", func() { BeforeEach(func() { brokerClient.UpdateReturns(osbapi.UpdateResponse{}, errors.New("update-failed")) From ea3d8db25d6c46942cb719cb41b53b08e49f825d Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Thu, 26 Feb 2026 18:14:13 +0100 Subject: [PATCH 24/47] Managed controller tests refine test names --- .../controllers/services/instances/managed/controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/controllers/services/instances/managed/controller_test.go b/controllers/controllers/services/instances/managed/controller_test.go index 2f34a13fb..fd074d8f6 100644 --- a/controllers/controllers/services/instances/managed/controller_test.go +++ b/controllers/controllers/services/instances/managed/controller_test.go @@ -840,7 +840,7 @@ var _ = Describe("CFServiceInstance", func() { }).Should(Succeed()) }) - It("sets initial state in instance last operation", func() { + It("sets in progress state in instance last operation", func() { Eventually(func(g Gomega) { g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">=", 1)) From a6f9f034a137564976f316a17d9b4a58d4ede11f Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Thu, 26 Feb 2026 18:17:40 +0100 Subject: [PATCH 25/47] revert unneeded newline change in webhook tests --- .../webhooks/networking/security_groups/validator_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/controllers/webhooks/networking/security_groups/validator_test.go b/controllers/webhooks/networking/security_groups/validator_test.go index fb777367d..e44b2e79e 100644 --- a/controllers/webhooks/networking/security_groups/validator_test.go +++ b/controllers/webhooks/networking/security_groups/validator_test.go @@ -467,4 +467,5 @@ var _ = Describe("CFSecurityGroupValidatingWebhook", func() { }) }) }) + }) From df83936fb94029f893a38a99c4343e35521266ad Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Thu, 26 Feb 2026 18:19:29 +0100 Subject: [PATCH 26/47] revert unneeded newline change in webhook tests --- .../webhooks/networking/security_groups/validator_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/controllers/webhooks/networking/security_groups/validator_test.go b/controllers/webhooks/networking/security_groups/validator_test.go index e44b2e79e..5472edaf5 100644 --- a/controllers/webhooks/networking/security_groups/validator_test.go +++ b/controllers/webhooks/networking/security_groups/validator_test.go @@ -438,6 +438,7 @@ var _ = Describe("CFSecurityGroupValidatingWebhook", func() { )) }) }) + }) Describe("ValidateDelete", func() { From e46a39b3dca5cb6bf56c4450b8f38179bc0c8697 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Fri, 27 Feb 2026 10:30:13 +0100 Subject: [PATCH 27/47] e2e managed services update tests: move declaring serviceInstanceGUID to BeforeEach --- tests/e2e/service_instances_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/e2e/service_instances_test.go b/tests/e2e/service_instances_test.go index 7050723d1..3c142d68c 100644 --- a/tests/e2e/service_instances_test.go +++ b/tests/e2e/service_instances_test.go @@ -171,6 +171,10 @@ var _ = Describe("Service Instances", func() { result serviceInstanceResource ) + BeforeEach(func() { + serviceInstanceGUID = upsiGUID + }) + JustBeforeEach(func() { httpResp, httpError = adminClient.R(). SetBody(updateRequestBody). @@ -179,7 +183,6 @@ var _ = Describe("Service Instances", func() { When("updating a user-provided service instance", func() { BeforeEach(func() { - serviceInstanceGUID = upsiGUID updateRequestBody = serviceInstanceResource{ resource: resource{ Name: "new-instance-name", From 6c99cb7eff4a721869d454e405d460e7d0d6b04d Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Fri, 27 Feb 2026 11:28:43 +0100 Subject: [PATCH 28/47] e2e add check OSB plans number assertion --- tests/e2e/service_instances_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e/service_instances_test.go b/tests/e2e/service_instances_test.go index 3c142d68c..1bc408c6d 100644 --- a/tests/e2e/service_instances_test.go +++ b/tests/e2e/service_instances_test.go @@ -220,7 +220,7 @@ var _ = Describe("Service Instances", func() { 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()) + Expect(plansResp.Resources).To(HaveLen(2)) serviceInstanceGUID = createManagedServiceInstance(brokerGUID, spaceGUID, generateGUID("managed-service-instance")) From 2932729d419e4e5de301f938ac2651912575f280 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Fri, 27 Feb 2026 11:59:16 +0100 Subject: [PATCH 29/47] combine some managed service update e2e plans --- tests/e2e/service_instances_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/e2e/service_instances_test.go b/tests/e2e/service_instances_test.go index 1bc408c6d..43ced5213 100644 --- a/tests/e2e/service_instances_test.go +++ b/tests/e2e/service_instances_test.go @@ -269,9 +269,6 @@ var _ = Describe("Service Instances", func() { Expect(serviceInstance.Metadata.Labels).To(HaveKeyWithValue("a-label", "a-label-value")) Expect(serviceInstance.Metadata.Annotations).To(HaveKeyWithValue("an-annotation", "an-annotation-value")) Expect(serviceInstance.Tags).To(ConsistOf("some", "tags")) - }) - It("changes a plan", func() { - expectJobCompletes(httpResp) httpRespService, httpErrorService := adminClient.R().SetResult(&result).Get("/v3/service_instances/" + serviceInstanceGUID) Expect(httpErrorService).NotTo(HaveOccurred()) From 6afbcac8935153021d028a56f89221c303326e30 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 2 Mar 2026 08:44:38 +0100 Subject: [PATCH 30/47] Revert some unneeded changes in api/payloads --- api/payloads/service_instance.go | 1 - 1 file changed, 1 deletion(-) diff --git a/api/payloads/service_instance.go b/api/payloads/service_instance.go index fcebed806..a1769f71d 100644 --- a/api/payloads/service_instance.go +++ b/api/payloads/service_instance.go @@ -172,7 +172,6 @@ func (g *ServiceInstanceGet) DecodeFromURLValues(values url.Values) error { type ServiceInstancePatch struct { Name *string `json:"name,omitempty"` - Type string `json:"type"` Tags *[]string `json:"tags,omitempty"` Credentials *map[string]any `json:"credentials,omitempty"` Relationships *ServiceInstanceRelationships `json:"relationships,omitempty"` From afa5310473aff8f9bdad8e6cac51a5c5a03addf4 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 2 Mar 2026 09:15:15 +0100 Subject: [PATCH 31/47] Simplify ToManagedSIPatchMessage method in api/payloads --- api/payloads/service_instance.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/api/payloads/service_instance.go b/api/payloads/service_instance.go index a1769f71d..9022b7a15 100644 --- a/api/payloads/service_instance.go +++ b/api/payloads/service_instance.go @@ -201,9 +201,6 @@ func (p ServiceInstancePatch) ToUPSIPatchMessage(spaceGUID, appGUID string) repo func (p ServiceInstancePatch) ToManagedSIPatchMessage(spaceGUID, appGUID string) repositories.PatchManagedSIMessage { var planGUID *string if p.Relationships.ServicePlan != nil { - if p.Relationships.ServicePlan.Data.GUID != "" { - planGUID = &p.Relationships.ServicePlan.Data.GUID - } planGUID = &p.Relationships.ServicePlan.Data.GUID } return repositories.PatchManagedSIMessage{ From 735ea653214960bd533f1c88eb3e3c5635189277 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 2 Mar 2026 09:34:28 +0100 Subject: [PATCH 32/47] Revert some unneeded changes in api/handlers --- api/handlers/service_instance_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/api/handlers/service_instance_test.go b/api/handlers/service_instance_test.go index ce6a2e919..650361b3b 100644 --- a/api/handlers/service_instance_test.go +++ b/api/handlers/service_instance_test.go @@ -938,7 +938,6 @@ var _ = Describe("ServiceInstance", func() { BeforeEach(func() { requestValidator.DecodeAndValidateJSONPayloadStub = decodeAndValidatePayloadStub(&payloads.ServiceInstancePatch{ Name: tools.PtrTo("new-name"), - Type: "managed", Tags: &[]string{"alice", "bob"}, Credentials: &map[string]any{"foo": "bar"}, Relationships: &payloads.ServiceInstanceRelationships{ From b2db4155d152153217111f8c3a39ffb58d19722f Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 2 Mar 2026 10:43:33 +0100 Subject: [PATCH 33/47] Inline tests api/repositories managed service instance creation --- api/repositories/repositories_suite_test.go | 20 ------------------- .../service_instance_repository_test.go | 20 +++++++++++++++---- 2 files changed, 16 insertions(+), 24 deletions(-) diff --git a/api/repositories/repositories_suite_test.go b/api/repositories/repositories_suite_test.go index 2b1fdb8ad..2ac313c50 100644 --- a/api/repositories/repositories_suite_test.go +++ b/api/repositories/repositories_suite_test.go @@ -386,26 +386,6 @@ func createUserProvidedServiceInstanceCR(ctx context.Context, k8sClient client.C return serviceInstance } -func createManagedServiceInstanceCR(ctx context.Context, k8sClient client.Client, serviceInstanceGUID, spaceGUID, name, plan string) *korifiv1alpha1.CFServiceInstance { - serviceInstance := &korifiv1alpha1.CFServiceInstance{ - ObjectMeta: metav1.ObjectMeta{ - Name: serviceInstanceGUID, - Namespace: spaceGUID, - Labels: map[string]string{"a-label": "a-label-value"}, - Annotations: map[string]string{"an-annotation": "an-annotation-value"}, - }, - Spec: korifiv1alpha1.CFServiceInstanceSpec{ - DisplayName: name, - Type: "managed", - Tags: []string{"database", "mysql"}, - PlanGUID: plan, - }, - } - Expect(k8sClient.Create(ctx, serviceInstance)).To(Succeed()) - - return serviceInstance -} - func createApp(space string) *korifiv1alpha1.CFApp { return createAppWithGUID(space, uuid.NewString()) } diff --git a/api/repositories/service_instance_repository_test.go b/api/repositories/service_instance_repository_test.go index 703ec5193..4a8d75cb5 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -692,11 +692,23 @@ var _ = Describe("ServiceInstanceRepository", func() { var patchMessage repositories.PatchManagedSIMessage BeforeEach(func() { - serviceInstancePlanGUID := uuid.NewString() - cfServiceInstance = createManagedServiceInstanceCR(ctx, k8sClient, serviceInstanceGUID, space.Name, serviceInstanceName, serviceInstancePlanGUID) - conditionAwaiter.AwaitConditionReturns(cfServiceInstance, nil) + cfserviceInstance := &korifiv1alpha1.CFServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: serviceInstanceGUID, + Namespace: space.Name, + Labels: map[string]string{"a-label": "a-label-value"}, + Annotations: map[string]string{"an-annotation": "an-annotation-value"}, + }, + Spec: korifiv1alpha1.CFServiceInstanceSpec{ + DisplayName: serviceInstanceName, + Type: "managed", + Tags: []string{"database", "mysql"}, + PlanGUID: uuid.NewString(), + }, + } - Expect(k8sClient.Create(ctx, cfServiceInstance)).To(Succeed()) + Expect(k8sClient.Create(ctx, cfserviceInstance)).To(Succeed()) + conditionAwaiter.AwaitConditionReturns(cfServiceInstance, nil) patchMessage = repositories.PatchManagedSIMessage{ GUID: cfServiceInstance.Name, From 138672c64cdf877f4206d5ca9a5aaf3327e29879 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 2 Mar 2026 11:10:22 +0100 Subject: [PATCH 34/47] Split api/repositories patch service instance tests --- .../service_instance_repository_test.go | 200 +++++++++--------- 1 file changed, 102 insertions(+), 98 deletions(-) diff --git a/api/repositories/service_instance_repository_test.go b/api/repositories/service_instance_repository_test.go index 4a8d75cb5..52df4809a 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -499,18 +499,17 @@ var _ = Describe("ServiceInstanceRepository", func() { }) }) - Describe("PatchServiceInstance", func() { + Describe("PatchUserProvidedServiceInstance", func() { var ( cfServiceInstance *korifiv1alpha1.CFServiceInstance secret *corev1.Secret - serviceInstanceGUID string serviceInstanceRecord repositories.ServiceInstanceRecord patchMessage repositories.PatchUPSIMessage err error ) BeforeEach(func() { - serviceInstanceGUID = uuid.NewString() + serviceInstanceGUID := uuid.NewString() secretName := uuid.NewString() cfServiceInstance = createUserProvidedServiceInstanceCR(ctx, k8sClient, serviceInstanceGUID, space.Name, serviceInstanceName, secretName) conditionAwaiter.AwaitConditionReturns(cfServiceInstance, nil) @@ -687,127 +686,132 @@ var _ = Describe("ServiceInstanceRepository", func() { }) }) }) + }) + + Describe("PatchManagedServiceInstance", func() { + var ( + cfServiceInstance *korifiv1alpha1.CFServiceInstance + serviceInstanceGUID string + serviceInstanceRecord repositories.ServiceInstanceRecord + patchMessage repositories.PatchManagedSIMessage + err error + ) + BeforeEach(func() { + cfServiceInstance = &korifiv1alpha1.CFServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: serviceInstanceGUID, + Namespace: space.Name, + Labels: map[string]string{"a-label": "a-label-value"}, + Annotations: map[string]string{"an-annotation": "an-annotation-value"}, + }, + Spec: korifiv1alpha1.CFServiceInstanceSpec{ + DisplayName: serviceInstanceName, + Type: "managed", + Tags: []string{"database", "mysql"}, + PlanGUID: uuid.NewString(), + }, + } + + Expect(k8sClient.Create(ctx, cfServiceInstance)).To(Succeed()) + conditionAwaiter.AwaitConditionReturns(cfServiceInstance, nil) + + patchMessage = repositories.PatchManagedSIMessage{ + GUID: cfServiceInstance.Name, + SpaceGUID: space.Name, + Name: tools.PtrTo("new-name"), + PlanGUID: tools.PtrTo("new-plan-guid"), + Tags: &[]string{"new"}, + MetadataPatch: repositories.MetadataPatch{ + Labels: map[string]*string{"new-label": tools.PtrTo("new-label-value")}, + Annotations: map[string]*string{"new-annotation": tools.PtrTo("new-annotation-value")}, + }, + } + }) - When("Patching a managed service instance", func() { - var patchMessage repositories.PatchManagedSIMessage + JustBeforeEach(func() { + serviceInstanceRecord, err = serviceInstanceRepo.PatchManagedServiceInstance(ctx, authInfo, patchMessage) + }) + When("authorized in the space", func() { BeforeEach(func() { - cfserviceInstance := &korifiv1alpha1.CFServiceInstance{ - ObjectMeta: metav1.ObjectMeta{ - Name: serviceInstanceGUID, - Namespace: space.Name, - Labels: map[string]string{"a-label": "a-label-value"}, - Annotations: map[string]string{"an-annotation": "an-annotation-value"}, - }, - Spec: korifiv1alpha1.CFServiceInstanceSpec{ - DisplayName: serviceInstanceName, - Type: "managed", - Tags: []string{"database", "mysql"}, - PlanGUID: uuid.NewString(), - }, - } + createRoleBinding(ctx, userName, orgUserRole.Name, org.Name) + createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space.Name) + }) - Expect(k8sClient.Create(ctx, cfserviceInstance)).To(Succeed()) - conditionAwaiter.AwaitConditionReturns(cfServiceInstance, nil) - - patchMessage = repositories.PatchManagedSIMessage{ - GUID: cfServiceInstance.Name, - SpaceGUID: space.Name, - Name: tools.PtrTo("new-name"), - PlanGUID: tools.PtrTo("new-plan-guid"), - Tags: &[]string{"new"}, - MetadataPatch: repositories.MetadataPatch{ - Labels: map[string]*string{"new-label": tools.PtrTo("new-label-value")}, - Annotations: map[string]*string{"new-annotation": tools.PtrTo("new-annotation-value")}, - }, - } + It("returns the updated record", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(serviceInstanceRecord.Name).To(Equal("new-name")) + Expect(serviceInstanceRecord.Tags).To(ConsistOf("new")) + Expect(serviceInstanceRecord.Labels).To(HaveKeyWithValue("a-label", "a-label-value")) + Expect(serviceInstanceRecord.Labels).To(HaveKeyWithValue("new-label", "new-label-value")) + Expect(serviceInstanceRecord.Annotations).To(HaveLen(2)) + Expect(serviceInstanceRecord.Annotations).To(HaveKeyWithValue("an-annotation", "an-annotation-value")) + Expect(serviceInstanceRecord.Annotations).To(HaveKeyWithValue("new-annotation", "new-annotation-value")) + Expect(serviceInstanceRecord.Relationships()).To(HaveKeyWithValue("service_plan", "new-plan-guid")) }) - JustBeforeEach(func() { - serviceInstanceRecord, err = serviceInstanceRepo.PatchManagedServiceInstance(ctx, authInfo, patchMessage) + It("updates the service instance", func() { + Expect(err).NotTo(HaveOccurred()) + serviceInstance := new(korifiv1alpha1.CFServiceInstance) + + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cfServiceInstance), serviceInstance)).To(Succeed()) + g.Expect(serviceInstance.Spec.DisplayName).To(Equal("new-name")) + g.Expect(serviceInstance.Spec.Tags).To(ConsistOf("new")) + g.Expect(serviceInstance.Spec.PlanGUID).To(Equal("new-plan-guid")) + g.Expect(serviceInstance.Labels).To(HaveKeyWithValue("a-label", "a-label-value")) + g.Expect(serviceInstance.Labels).To(HaveKeyWithValue("new-label", "new-label-value")) + g.Expect(serviceInstance.Annotations).To(HaveLen(2)) + g.Expect(serviceInstance.Annotations).To(HaveKeyWithValue("an-annotation", "an-annotation-value")) + g.Expect(serviceInstance.Annotations).To(HaveKeyWithValue("new-annotation", "new-annotation-value")) + }).Should(Succeed()) }) - When("authorized in the space", func() { + When("tags is an empty list", func() { BeforeEach(func() { - createRoleBinding(ctx, userName, orgUserRole.Name, org.Name) - createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space.Name) - }) - - It("returns the updated record", func() { - Expect(err).NotTo(HaveOccurred()) - Expect(serviceInstanceRecord.Name).To(Equal("new-name")) - Expect(serviceInstanceRecord.Tags).To(ConsistOf("new")) - Expect(serviceInstanceRecord.Labels).To(HaveKeyWithValue("a-label", "a-label-value")) - Expect(serviceInstanceRecord.Labels).To(HaveKeyWithValue("new-label", "new-label-value")) - Expect(serviceInstanceRecord.Annotations).To(HaveLen(2)) - Expect(serviceInstanceRecord.Annotations).To(HaveKeyWithValue("an-annotation", "an-annotation-value")) - Expect(serviceInstanceRecord.Annotations).To(HaveKeyWithValue("new-annotation", "new-annotation-value")) - Expect(serviceInstanceRecord.Relationships()).To(HaveKeyWithValue("service_plan", "new-plan-guid")) + patchMessage.Tags = &[]string{} }) - It("updates the service instance", func() { + It("clears the tags", func() { Expect(err).NotTo(HaveOccurred()) serviceInstance := new(korifiv1alpha1.CFServiceInstance) Eventually(func(g Gomega) { g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cfServiceInstance), serviceInstance)).To(Succeed()) - g.Expect(serviceInstance.Spec.DisplayName).To(Equal("new-name")) - g.Expect(serviceInstance.Spec.Tags).To(ConsistOf("new")) - g.Expect(serviceInstance.Spec.PlanGUID).To(Equal("new-plan-guid")) - g.Expect(serviceInstance.Labels).To(HaveKeyWithValue("a-label", "a-label-value")) - g.Expect(serviceInstance.Labels).To(HaveKeyWithValue("new-label", "new-label-value")) - g.Expect(serviceInstance.Annotations).To(HaveLen(2)) - g.Expect(serviceInstance.Annotations).To(HaveKeyWithValue("an-annotation", "an-annotation-value")) - g.Expect(serviceInstance.Annotations).To(HaveKeyWithValue("new-annotation", "new-annotation-value")) + g.Expect(serviceInstance.Spec.Tags).To(BeEmpty()) }).Should(Succeed()) }) + }) - When("tags is an empty list", func() { - BeforeEach(func() { - patchMessage.Tags = &[]string{} - }) - - It("clears the tags", func() { - Expect(err).NotTo(HaveOccurred()) - serviceInstance := new(korifiv1alpha1.CFServiceInstance) - - Eventually(func(g Gomega) { - g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cfServiceInstance), serviceInstance)).To(Succeed()) - g.Expect(serviceInstance.Spec.Tags).To(BeEmpty()) - }).Should(Succeed()) - }) + When("tags is nil", func() { + BeforeEach(func() { + patchMessage.Tags = nil }) - When("tags is nil", func() { - BeforeEach(func() { - patchMessage.Tags = nil - }) - - It("preserves the tags", func() { - Expect(err).NotTo(HaveOccurred()) - serviceInstance := new(korifiv1alpha1.CFServiceInstance) + It("preserves the tags", func() { + Expect(err).NotTo(HaveOccurred()) + serviceInstance := new(korifiv1alpha1.CFServiceInstance) - Consistently(func(g Gomega) { - g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cfServiceInstance), serviceInstance)).To(Succeed()) - g.Expect(serviceInstance.Spec.Tags).To(ConsistOf("database", "mysql")) - }).Should(Succeed()) - }) + Consistently(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cfServiceInstance), serviceInstance)).To(Succeed()) + g.Expect(serviceInstance.Spec.Tags).To(ConsistOf("database", "mysql")) + }).Should(Succeed()) }) + }) - When("the plan GUID is nil", func() { - BeforeEach(func() { - patchMessage.PlanGUID = nil - }) + When("the plan GUID is nil", func() { + BeforeEach(func() { + patchMessage.PlanGUID = nil + }) - It("preserves the plan GUID", func() { - Expect(err).NotTo(HaveOccurred()) - serviceInstance := new(korifiv1alpha1.CFServiceInstance) + It("preserves the plan GUID", func() { + Expect(err).NotTo(HaveOccurred()) + serviceInstance := new(korifiv1alpha1.CFServiceInstance) - Consistently(func(g Gomega) { - g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cfServiceInstance), serviceInstance)).To(Succeed()) - g.Expect(serviceInstance.Spec.PlanGUID).To(Equal("plan-guid")) - }).Should(Succeed()) - }) + Consistently(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cfServiceInstance), serviceInstance)).To(Succeed()) + g.Expect(serviceInstance.Spec.PlanGUID).To(Equal("plan-guid")) + }).Should(Succeed()) }) }) }) From b2aceecb419e1c68146f8f49d9f9886fdd4165a9 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 2 Mar 2026 11:27:09 +0100 Subject: [PATCH 35/47] Add api/repositories patch service instance api forbidden test --- api/repositories/service_instance_repository_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/api/repositories/service_instance_repository_test.go b/api/repositories/service_instance_repository_test.go index 52df4809a..3239e853a 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -732,6 +732,10 @@ var _ = Describe("ServiceInstanceRepository", func() { serviceInstanceRecord, err = serviceInstanceRepo.PatchManagedServiceInstance(ctx, authInfo, patchMessage) }) + It("returns a Forbidden error", func() { + Expect(err).To(BeAssignableToTypeOf(apierrors.ForbiddenError{})) + }) + When("authorized in the space", func() { BeforeEach(func() { createRoleBinding(ctx, userName, orgUserRole.Name, org.Name) From 70adff888a7b45106ad54f9698ca0fad133a457a Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 2 Mar 2026 12:29:53 +0100 Subject: [PATCH 36/47] Rename isFailed method in controllers/managed --- .../controllers/services/instances/managed/controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/controllers/services/instances/managed/controller.go b/controllers/controllers/services/instances/managed/controller.go index 2458c4ef5..dd5773e75 100644 --- a/controllers/controllers/services/instances/managed/controller.go +++ b/controllers/controllers/services/instances/managed/controller.go @@ -147,7 +147,7 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor return ctrl.Result{}, nil } - if isFailed(serviceInstance) { + if isProvisioningFailed(serviceInstance) { return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ServiceInstanceFailed").WithNoRequeue() } @@ -555,7 +555,7 @@ func (r *Reconciler) getNamespace(ctx context.Context, namespaceName string) (*c return namespace, nil } -func isFailed(instance *korifiv1alpha1.CFServiceInstance) bool { +func isProvisioningFailed(instance *korifiv1alpha1.CFServiceInstance) bool { return meta.IsStatusConditionTrue(instance.Status.Conditions, korifiv1alpha1.ProvisioningFailedCondition) } From 8507a899756fe40d5b9fdb0c8149bc2cc9cbec01 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 2 Mar 2026 12:56:47 +0100 Subject: [PATCH 37/47] Simplify controllers/managed Reconciler code --- .../services/instances/managed/controller.go | 67 +++++++++++++------ 1 file changed, 46 insertions(+), 21 deletions(-) diff --git a/controllers/controllers/services/instances/managed/controller.go b/controllers/controllers/services/instances/managed/controller.go index dd5773e75..4a424b8fc 100644 --- a/controllers/controllers/services/instances/managed/controller.go +++ b/controllers/controllers/services/instances/managed/controller.go @@ -167,32 +167,57 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor } if !serviceInstance.Status.Provisioned { - provisionResponse, err := r.provisionServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient) + return r.reconcileProvisionedServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient) + } + + return r.reconcileUpdatedServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient) +} + +func (r *Reconciler) reconcileProvisionedServiceInstance( + ctx context.Context, + serviceInstance *korifiv1alpha1.CFServiceInstance, + serviceInstanceAssets osbapi.ServiceInstanceAssets, + osbapiClient osbapi.BrokerClient, +) (ctrl.Result, error) { + log := logr.FromContextOrDiscard(ctx) + provisionResponse, err := r.provisionServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient) + if err != nil { + log.Error(err, "failed to provision service instance") + return ctrl.Result{}, fmt.Errorf("failed to provision service instance: %w", err) + } + + if provisionResponse.IsAsync { + lastOpResponse, err := r.pollLastOperation(ctx, serviceInstance, serviceInstanceAssets, osbapiClient, provisionResponse.Operation) if err != nil { - log.Error(err, "failed to provision service instance") - return ctrl.Result{}, fmt.Errorf("failed to provision service instance: %w", err) + return ctrl.Result{}, err } + return r.processProvisionOperation(serviceInstance, lastOpResponse) + } - if provisionResponse.IsAsync { - lastOpResponse, err := r.pollLastOperation(ctx, serviceInstance, serviceInstanceAssets, osbapiClient, provisionResponse.Operation) - if err != nil { - return ctrl.Result{}, err - } - return r.processProvisionOperation(serviceInstance, lastOpResponse) - } - } else { - updateResponse, err := r.updateServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient) + serviceInstance.Status.Provisioned = true + serviceInstance.Status.MaintenanceInfo = serviceInstanceAssets.ServicePlan.Spec.MaintenanceInfo + serviceInstance.Status.LastOperation.State = "succeeded" + return ctrl.Result{}, nil +} + +func (r *Reconciler) reconcileUpdatedServiceInstance( + ctx context.Context, + serviceInstance *korifiv1alpha1.CFServiceInstance, + serviceInstanceAssets osbapi.ServiceInstanceAssets, + osbapiClient osbapi.BrokerClient, +) (ctrl.Result, error) { + log := logr.FromContextOrDiscard(ctx) + updateResponse, err := r.updateServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient) + if err != nil { + log.Error(err, "failed to update service instance") + return ctrl.Result{}, fmt.Errorf("failed to update service instance: %w", err) + } + if updateResponse.IsAsync { + lastOpResponse, err := r.pollLastOperation(ctx, serviceInstance, serviceInstanceAssets, osbapiClient, updateResponse.Operation) if err != nil { - log.Error(err, "failed to update service instance") - return ctrl.Result{}, fmt.Errorf("failed to update service instance: %w", err) - } - if updateResponse.IsAsync { - lastOpResponse, err := r.pollLastOperation(ctx, serviceInstance, serviceInstanceAssets, osbapiClient, updateResponse.Operation) - if err != nil { - return ctrl.Result{}, err - } - return r.processUpdateOperation(serviceInstance, lastOpResponse) + return ctrl.Result{}, err } + return r.processUpdateOperation(serviceInstance, lastOpResponse) } serviceInstance.Status.Provisioned = true From 73118cdf83f0a3a223a05e67916c87ce8128bb4b Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 2 Mar 2026 13:26:24 +0100 Subject: [PATCH 38/47] controllers/managed stop reconsiling on UpdateFailed --- .../controllers/services/instances/managed/controller.go | 3 ++- .../services/instances/managed/controller_test.go | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/controllers/controllers/services/instances/managed/controller.go b/controllers/controllers/services/instances/managed/controller.go index 4a424b8fc..a0f68a67d 100644 --- a/controllers/controllers/services/instances/managed/controller.go +++ b/controllers/controllers/services/instances/managed/controller.go @@ -344,7 +344,8 @@ func (r *Reconciler) updateServiceInstance( Message: err.Error(), }) return osbapi.UpdateResponse{}, - k8s.NewNotReadyError().WithReason("UpdateFailed") + k8s.NewNotReadyError().WithReason("UpdateFailed").WithNoRequeue() + } return osbapi.UpdateResponse{}, err diff --git a/controllers/controllers/services/instances/managed/controller_test.go b/controllers/controllers/services/instances/managed/controller_test.go index fd074d8f6..21f0b5a84 100644 --- a/controllers/controllers/services/instances/managed/controller_test.go +++ b/controllers/controllers/services/instances/managed/controller_test.go @@ -3,6 +3,7 @@ package managed_test import ( "errors" "net/http" + "time" "github.com/google/uuid" "sigs.k8s.io/controller-runtime/pkg/client" @@ -885,6 +886,11 @@ var _ = Describe("CFServiceInstance", func() { State: "failed", })) }).Should(Succeed()) + + currentCallCount := brokerClient.UpdateCallCount() + Consistently(func(g Gomega) { + g.Expect(brokerClient.UpdateCallCount()).To(Equal(currentCallCount)) + }).WithTimeout(time.Second).Should(Succeed()) }) }) From 95d3b40670a83b68c0369e90e6fd2e915c45d215 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 2 Mar 2026 13:58:13 +0100 Subject: [PATCH 39/47] controllers/managed move servicePlan2 tests var to its scope --- .../controllers/services/instances/managed/controller_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/controllers/controllers/services/instances/managed/controller_test.go b/controllers/controllers/services/instances/managed/controller_test.go index 21f0b5a84..256641fbc 100644 --- a/controllers/controllers/services/instances/managed/controller_test.go +++ b/controllers/controllers/services/instances/managed/controller_test.go @@ -31,7 +31,6 @@ var _ = Describe("CFServiceInstance", func() { serviceBroker *korifiv1alpha1.CFServiceBroker serviceOffering *korifiv1alpha1.CFServiceOffering servicePlan *korifiv1alpha1.CFServicePlan - servicePlan2 *korifiv1alpha1.CFServicePlan ) BeforeEach(func() { @@ -763,6 +762,8 @@ var _ = Describe("CFServiceInstance", func() { }) When("the service instance is being updated", func() { + var servicePlan2 *korifiv1alpha1.CFServicePlan + BeforeEach(func() { brokerClient.UpdateReturns(osbapi.UpdateResponse{}, nil) From f1ee84385abaf2eff327f0826803b3cd14ae21b0 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 2 Mar 2026 14:16:02 +0100 Subject: [PATCH 40/47] Rearrange e2e tests for updating managed services --- tests/e2e/service_instances_test.go | 51 +++++++++++++---------------- 1 file changed, 23 insertions(+), 28 deletions(-) diff --git a/tests/e2e/service_instances_test.go b/tests/e2e/service_instances_test.go index 43ced5213..243ba8eee 100644 --- a/tests/e2e/service_instances_test.go +++ b/tests/e2e/service_instances_test.go @@ -173,6 +173,19 @@ var _ = Describe("Service Instances", func() { BeforeEach(func() { serviceInstanceGUID = upsiGUID + updateRequestBody = serviceInstanceResource{ + resource: resource{ + Name: "new-instance-name", + Metadata: &metadata{ + Labels: map[string]string{"a-label": "a-label-value"}, + Annotations: map[string]string{"an-annotation": "an-annotation-value"}, + }, + }, + Credentials: map[string]any{ + "object-new": map[string]any{"new-a": "new-b"}, + }, + Tags: []string{"some", "tags"}, + } }) JustBeforeEach(func() { @@ -181,36 +194,18 @@ var _ = Describe("Service Instances", func() { Patch("/v3/service_instances/" + serviceInstanceGUID) }) - When("updating a user-provided service instance", func() { - BeforeEach(func() { - updateRequestBody = serviceInstanceResource{ - resource: resource{ - Name: "new-instance-name", - Metadata: &metadata{ - Labels: map[string]string{"a-label": "a-label-value"}, - Annotations: map[string]string{"an-annotation": "an-annotation-value"}, - }, - }, - Credentials: map[string]any{ - "object-new": map[string]any{"new-a": "new-b"}, - }, - Tags: []string{"some", "tags"}, - } - }) - - It("succeeds", func() { - Expect(httpError).NotTo(HaveOccurred()) - Expect(httpResp).To(HaveRestyStatusCode(http.StatusOK)) + It("succeeds", func() { + Expect(httpError).NotTo(HaveOccurred()) + Expect(httpResp).To(HaveRestyStatusCode(http.StatusOK)) - serviceInstances := listServiceInstances("new-instance-name") - Expect(serviceInstances.Resources).To(HaveLen(1)) + serviceInstances := listServiceInstances("new-instance-name") + Expect(serviceInstances.Resources).To(HaveLen(1)) - serviceInstance := serviceInstances.Resources[0] - Expect(serviceInstance.Name).To(Equal("new-instance-name")) - Expect(serviceInstance.Metadata.Labels).To(HaveKeyWithValue("a-label", "a-label-value")) - Expect(serviceInstance.Metadata.Annotations).To(HaveKeyWithValue("an-annotation", "an-annotation-value")) - Expect(serviceInstance.Tags).To(ConsistOf("some", "tags")) - }) + serviceInstance := serviceInstances.Resources[0] + Expect(serviceInstance.Name).To(Equal("new-instance-name")) + Expect(serviceInstance.Metadata.Labels).To(HaveKeyWithValue("a-label", "a-label-value")) + Expect(serviceInstance.Metadata.Annotations).To(HaveKeyWithValue("an-annotation", "an-annotation-value")) + Expect(serviceInstance.Tags).To(ConsistOf("some", "tags")) }) When("updating a managed service instance", func() { From 97e98f48e73f380f6b1cfb3844df3a8a88904620 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Thu, 5 Mar 2026 08:30:27 +0100 Subject: [PATCH 41/47] Suppress linter errors in a managed controller --- .../controllers/services/instances/managed/controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/controllers/services/instances/managed/controller.go b/controllers/controllers/services/instances/managed/controller.go index a0f68a67d..bcf440a52 100644 --- a/controllers/controllers/services/instances/managed/controller.go +++ b/controllers/controllers/services/instances/managed/controller.go @@ -173,7 +173,7 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor return r.reconcileUpdatedServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient) } -func (r *Reconciler) reconcileProvisionedServiceInstance( +func (r *Reconciler) reconcileProvisionedServiceInstance( //nolint:dupl ctx context.Context, serviceInstance *korifiv1alpha1.CFServiceInstance, serviceInstanceAssets osbapi.ServiceInstanceAssets, @@ -200,7 +200,7 @@ func (r *Reconciler) reconcileProvisionedServiceInstance( return ctrl.Result{}, nil } -func (r *Reconciler) reconcileUpdatedServiceInstance( +func (r *Reconciler) reconcileUpdatedServiceInstance( //nolint:dupl ctx context.Context, serviceInstance *korifiv1alpha1.CFServiceInstance, serviceInstanceAssets osbapi.ServiceInstanceAssets, From ee8aa6ad14f75552d3909e4426fe22002f4f339e Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Thu, 5 Mar 2026 08:46:55 +0100 Subject: [PATCH 42/47] Fix repo service instance unit tests --- api/repositories/service_instance_repository_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/api/repositories/service_instance_repository_test.go b/api/repositories/service_instance_repository_test.go index 3239e853a..ce67efb86 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -697,6 +697,7 @@ var _ = Describe("ServiceInstanceRepository", func() { err error ) BeforeEach(func() { + serviceInstanceGUID = uuid.NewString() cfServiceInstance = &korifiv1alpha1.CFServiceInstance{ ObjectMeta: metav1.ObjectMeta{ Name: serviceInstanceGUID, @@ -708,7 +709,7 @@ var _ = Describe("ServiceInstanceRepository", func() { DisplayName: serviceInstanceName, Type: "managed", Tags: []string{"database", "mysql"}, - PlanGUID: uuid.NewString(), + PlanGUID: "plan-guid", }, } From 1509139ac8b65f71f696d33ae5fee5317a5ca69a Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Thu, 5 Mar 2026 09:05:24 +0100 Subject: [PATCH 43/47] Remove unneeded await check from repositories test --- api/repositories/service_instance_repository_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/api/repositories/service_instance_repository_test.go b/api/repositories/service_instance_repository_test.go index ce67efb86..d8132f707 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -714,7 +714,6 @@ var _ = Describe("ServiceInstanceRepository", func() { } Expect(k8sClient.Create(ctx, cfServiceInstance)).To(Succeed()) - conditionAwaiter.AwaitConditionReturns(cfServiceInstance, nil) patchMessage = repositories.PatchManagedSIMessage{ GUID: cfServiceInstance.Name, From 4a14aabf3fc57f801a043da7cd3e124e901dacf8 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 30 Mar 2026 11:10:51 +0200 Subject: [PATCH 44/47] Apply proposed fixes for managed services and their tests --- .../api/v1alpha1/cfserviceinstance_types.go | 8 ++++ .../services/instances/managed/controller.go | 39 ++++++++++++------- .../instances/managed/controller_test.go | 30 ++++++++------ 3 files changed, 52 insertions(+), 25 deletions(-) diff --git a/controllers/api/v1alpha1/cfserviceinstance_types.go b/controllers/api/v1alpha1/cfserviceinstance_types.go index 420596acb..4398ff67e 100644 --- a/controllers/api/v1alpha1/cfserviceinstance_types.go +++ b/controllers/api/v1alpha1/cfserviceinstance_types.go @@ -91,10 +91,18 @@ type CFServiceInstanceStatus struct { //+kubebuilder:validation:Optional MaintenanceInfo MaintenanceInfo `json:"maintenanceInfo"` + // The plan guid of the service instance. Only makes seense for managed service instances + //+kubebuilder:validation:Optional + PlanGUID string `json:"planGuid"` + // True if the service instance has been successfully provisioned. Only makes seense for managed service instances //+kubebuilder:validation:Optional Provisioned bool `json:"provisioned"` + // The generation at which the instance was last successfully reconciled with the broker. + //+kubebuilder:validation:Optional + BrokerReconciledGeneration int64 `json:"brokerReconciledGeneration,omitempty"` + // True if there is an upgrade available for for the service instance (i.e. the plan has a new version). Only makes seense for managed service instances //+kubebuilder:validation:Optional UpgradeAvailable bool `json:"upgradeAvailable"` diff --git a/controllers/controllers/services/instances/managed/controller.go b/controllers/controllers/services/instances/managed/controller.go index bcf440a52..adeacbd63 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() } @@ -157,19 +153,24 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor return ctrl.Result{}, err } - if !planVisible { + if !planVisible && !serviceInstance.Status.Provisioned { return ctrl.Result{}, k8s.NewNotReadyError().WithMessage("The service plan is disabled").WithReason("InvalidServicePlan").WithNoRequeue() } if serviceInstance.Spec.ServiceLabel == nil { serviceInstance.Spec.ServiceLabel = tools.PtrTo(serviceInstanceAssets.ServiceOffering.Spec.Name) + return ctrl.Result{}, nil } if !serviceInstance.Status.Provisioned { return r.reconcileProvisionedServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient) } + if serviceInstance.Generation == serviceInstance.Status.BrokerReconciledGeneration { + return ctrl.Result{}, nil + } + return r.reconcileUpdatedServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient) } @@ -191,10 +192,12 @@ func (r *Reconciler) reconcileProvisionedServiceInstance( //nolint:dupl if err != nil { return ctrl.Result{}, err } - return r.processProvisionOperation(serviceInstance, lastOpResponse) + return r.processProvisionOperation(serviceInstance, lastOpResponse, serviceInstanceAssets) } serviceInstance.Status.Provisioned = true + serviceInstance.Status.BrokerReconciledGeneration = serviceInstance.Generation + serviceInstance.Status.PlanGUID = serviceInstance.Spec.PlanGUID serviceInstance.Status.MaintenanceInfo = serviceInstanceAssets.ServicePlan.Spec.MaintenanceInfo serviceInstance.Status.LastOperation.State = "succeeded" return ctrl.Result{}, nil @@ -217,12 +220,15 @@ func (r *Reconciler) reconcileUpdatedServiceInstance( //nolint:dupl if err != nil { return ctrl.Result{}, err } - return r.processUpdateOperation(serviceInstance, lastOpResponse) + return r.processUpdateOperation(serviceInstance, lastOpResponse, serviceInstanceAssets) } - serviceInstance.Status.Provisioned = true - serviceInstance.Status.MaintenanceInfo = serviceInstanceAssets.ServicePlan.Spec.MaintenanceInfo + serviceInstance.Status.BrokerReconciledGeneration = serviceInstance.Generation serviceInstance.Status.LastOperation.State = "succeeded" + if serviceInstance.Spec.PlanGUID != serviceInstance.Status.PlanGUID { + serviceInstance.Status.PlanGUID = serviceInstance.Spec.PlanGUID + serviceInstance.Status.MaintenanceInfo = serviceInstanceAssets.ServicePlan.Spec.MaintenanceInfo + } return ctrl.Result{}, nil } @@ -288,8 +294,13 @@ func (r *Reconciler) provisionServiceInstance( func (r *Reconciler) processProvisionOperation( serviceInstance *korifiv1alpha1.CFServiceInstance, lastOpResponse osbapi.LastOperationResponse, + assets osbapi.ServiceInstanceAssets, ) (ctrl.Result, error) { if lastOpResponse.State == "succeeded" { + serviceInstance.Status.Provisioned = true + serviceInstance.Status.BrokerReconciledGeneration = serviceInstance.Generation + serviceInstance.Status.PlanGUID = serviceInstance.Spec.PlanGUID + serviceInstance.Status.MaintenanceInfo = assets.ServicePlan.Spec.MaintenanceInfo return ctrl.Result{}, nil } @@ -357,8 +368,14 @@ func (r *Reconciler) updateServiceInstance( func (r *Reconciler) processUpdateOperation( serviceInstance *korifiv1alpha1.CFServiceInstance, lastOpResponse osbapi.LastOperationResponse, + assets osbapi.ServiceInstanceAssets, ) (ctrl.Result, error) { if lastOpResponse.State == "succeeded" { + serviceInstance.Status.BrokerReconciledGeneration = serviceInstance.Generation + if serviceInstance.Spec.PlanGUID != serviceInstance.Status.PlanGUID { + serviceInstance.Status.PlanGUID = serviceInstance.Spec.PlanGUID + serviceInstance.Status.MaintenanceInfo = assets.ServicePlan.Spec.MaintenanceInfo + } return ctrl.Result{}, nil } @@ -584,7 +601,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 256641fbc..e6685aa9b 100644 --- a/controllers/controllers/services/instances/managed/controller_test.go +++ b/controllers/controllers/services/instances/managed/controller_test.go @@ -543,6 +543,8 @@ var _ = Describe("CFServiceInstance", func() { When("the instance has become ready", func() { BeforeEach(func() { Expect(k8s.Patch(ctx, adminClient, instance, func() { + instance.Status.Provisioned = true + instance.Status.PlanGUID = instance.Spec.PlanGUID instance.Status.MaintenanceInfo.Version = "1.2.3" meta.SetStatusCondition(&instance.Status.Conditions, metav1.Condition{ Type: korifiv1alpha1.StatusConditionReady, @@ -793,21 +795,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 +815,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")) @@ -830,8 +836,8 @@ 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) + g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">", 1)) + _, updatePayload := brokerClient.UpdateArgsForCall(brokerClient.UpdateCallCount() - 1) g.Expect(updatePayload).To(Equal(osbapi.UpdatePayload{ InstanceID: instance.Name, UpdateRequest: osbapi.UpdateRequest{ From d506b76d8019613eb05c256c1961dd14ba917266 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 30 Mar 2026 11:11:53 +0200 Subject: [PATCH 45/47] Introduce a smoke-test for 'cf update-service' command --- tests/smoke/services_test.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/smoke/services_test.go b/tests/smoke/services_test.go index a7c0b07c5..65deab4fd 100644 --- a/tests/smoke/services_test.go +++ b/tests/smoke/services_test.go @@ -71,4 +71,27 @@ var _ = Describe("Services", func() { )) }) }) + + Describe("cf update-service", func() { + var serviceName string + + BeforeEach(func() { + serviceName = uuid.NewString() + session := helpers.Cf("create-service", "sample-service", "sample", serviceName, "-b", brokerName) + Expect(session).To(Exit(0)) + }) + + It("updates the managed service plan", func() { + session := helpers.Cf("update-service", serviceName, "-p", "sample-2") + Expect(session).To(Exit(0)) + + session = helpers.Cf("services") + Expect(session).To(Exit(0)) + + lines := it.MustCollect(it.LinesString(session.Out)) + Expect(lines).To(ContainElement( + matchSubstrings(serviceName, "sample-service", "sample-2", brokerName), + )) + }) + }) }) From 3aa15b060737e395180c7fc49e3de16742d675e6 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Mon, 20 Apr 2026 15:08:09 +0200 Subject: [PATCH 46/47] Skip broker updates for metadata-only instance changes --- .../api/v1alpha1/cfserviceinstance_types.go | 4 -- .../services/instances/managed/controller.go | 54 +++++++++++++------ .../instances/managed/controller_test.go | 54 ++++++++++++++++++- ...i.cloudfoundry.org_cfserviceinstances.yaml | 4 ++ 4 files changed, 95 insertions(+), 21 deletions(-) diff --git a/controllers/api/v1alpha1/cfserviceinstance_types.go b/controllers/api/v1alpha1/cfserviceinstance_types.go index 4398ff67e..e45add546 100644 --- a/controllers/api/v1alpha1/cfserviceinstance_types.go +++ b/controllers/api/v1alpha1/cfserviceinstance_types.go @@ -99,10 +99,6 @@ type CFServiceInstanceStatus struct { //+kubebuilder:validation:Optional Provisioned bool `json:"provisioned"` - // The generation at which the instance was last successfully reconciled with the broker. - //+kubebuilder:validation:Optional - BrokerReconciledGeneration int64 `json:"brokerReconciledGeneration,omitempty"` - // True if there is an upgrade available for for the service instance (i.e. the plan has a new version). Only makes seense for managed service instances //+kubebuilder:validation:Optional UpgradeAvailable bool `json:"upgradeAvailable"` diff --git a/controllers/controllers/services/instances/managed/controller.go b/controllers/controllers/services/instances/managed/controller.go index adeacbd63..905ccb684 100644 --- a/controllers/controllers/services/instances/managed/controller.go +++ b/controllers/controllers/services/instances/managed/controller.go @@ -115,6 +115,26 @@ func (r *Reconciler) isManaged(object client.Object) bool { return serviceInstance.Spec.Type == korifiv1alpha1.ManagedType } +func needsUpdate(instance *korifiv1alpha1.CFServiceInstance) bool { + return instance.Spec.PlanGUID != instance.Status.PlanGUID +} + +func (r *Reconciler) ensurePlanVisible(ctx context.Context, serviceInstance *korifiv1alpha1.CFServiceInstance, servicePlan *korifiv1alpha1.CFServicePlan) error { + log := logr.FromContextOrDiscard(ctx) + + planVisible, err := r.isServicePlanVisible(ctx, serviceInstance, servicePlan) + if err != nil { + log.Error(err, "failed to check service plan visibility") + return err + } + + if !planVisible { + return k8s.NewNotReadyError().WithMessage("The service plan is disabled").WithReason("InvalidServicePlan").WithNoRequeue() + } + + return nil +} + //+kubebuilder:rbac:groups=korifi.cloudfoundry.org,resources=cfserviceinstances,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=korifi.cloudfoundry.org,resources=cfserviceinstances/status,verbs=get;update;patch //+kubebuilder:rbac:groups=korifi.cloudfoundry.org,resources=cfserviceinstances/finalizers,verbs=update @@ -147,17 +167,6 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ServiceInstanceFailed").WithNoRequeue() } - planVisible, err := r.isServicePlanVisible(ctx, serviceInstance, serviceInstanceAssets.ServicePlan) - if err != nil { - log.Error(err, "failed to check service plan visibility") - return ctrl.Result{}, err - } - - if !planVisible && !serviceInstance.Status.Provisioned { - return ctrl.Result{}, - k8s.NewNotReadyError().WithMessage("The service plan is disabled").WithReason("InvalidServicePlan").WithNoRequeue() - } - if serviceInstance.Spec.ServiceLabel == nil { serviceInstance.Spec.ServiceLabel = tools.PtrTo(serviceInstanceAssets.ServiceOffering.Spec.Name) return ctrl.Result{}, nil @@ -167,7 +176,13 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor return r.reconcileProvisionedServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient) } - if serviceInstance.Generation == serviceInstance.Status.BrokerReconciledGeneration { + // Backfill status.PlanGUID for legacy provisioned instances without it (one-time migration) + if serviceInstance.Status.PlanGUID == "" { + serviceInstance.Status.PlanGUID = serviceInstance.Spec.PlanGUID + return ctrl.Result{}, nil + } + + if !needsUpdate(serviceInstance) { return ctrl.Result{}, nil } @@ -181,6 +196,11 @@ func (r *Reconciler) reconcileProvisionedServiceInstance( //nolint:dupl osbapiClient osbapi.BrokerClient, ) (ctrl.Result, error) { log := logr.FromContextOrDiscard(ctx) + + if err := r.ensurePlanVisible(ctx, serviceInstance, serviceInstanceAssets.ServicePlan); err != nil { + return ctrl.Result{}, err + } + provisionResponse, err := r.provisionServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient) if err != nil { log.Error(err, "failed to provision service instance") @@ -196,7 +216,7 @@ func (r *Reconciler) reconcileProvisionedServiceInstance( //nolint:dupl } serviceInstance.Status.Provisioned = true - serviceInstance.Status.BrokerReconciledGeneration = serviceInstance.Generation + serviceInstance.Status.PlanGUID = serviceInstance.Spec.PlanGUID serviceInstance.Status.MaintenanceInfo = serviceInstanceAssets.ServicePlan.Spec.MaintenanceInfo serviceInstance.Status.LastOperation.State = "succeeded" @@ -210,6 +230,11 @@ func (r *Reconciler) reconcileUpdatedServiceInstance( //nolint:dupl osbapiClient osbapi.BrokerClient, ) (ctrl.Result, error) { log := logr.FromContextOrDiscard(ctx) + + if err := r.ensurePlanVisible(ctx, serviceInstance, serviceInstanceAssets.ServicePlan); err != nil { + return ctrl.Result{}, err + } + updateResponse, err := r.updateServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient) if err != nil { log.Error(err, "failed to update service instance") @@ -223,7 +248,6 @@ func (r *Reconciler) reconcileUpdatedServiceInstance( //nolint:dupl return r.processUpdateOperation(serviceInstance, lastOpResponse, serviceInstanceAssets) } - serviceInstance.Status.BrokerReconciledGeneration = serviceInstance.Generation serviceInstance.Status.LastOperation.State = "succeeded" if serviceInstance.Spec.PlanGUID != serviceInstance.Status.PlanGUID { serviceInstance.Status.PlanGUID = serviceInstance.Spec.PlanGUID @@ -298,7 +322,6 @@ func (r *Reconciler) processProvisionOperation( ) (ctrl.Result, error) { if lastOpResponse.State == "succeeded" { serviceInstance.Status.Provisioned = true - serviceInstance.Status.BrokerReconciledGeneration = serviceInstance.Generation serviceInstance.Status.PlanGUID = serviceInstance.Spec.PlanGUID serviceInstance.Status.MaintenanceInfo = assets.ServicePlan.Spec.MaintenanceInfo return ctrl.Result{}, nil @@ -371,7 +394,6 @@ func (r *Reconciler) processUpdateOperation( assets osbapi.ServiceInstanceAssets, ) (ctrl.Result, error) { if lastOpResponse.State == "succeeded" { - serviceInstance.Status.BrokerReconciledGeneration = serviceInstance.Generation if serviceInstance.Spec.PlanGUID != serviceInstance.Status.PlanGUID { serviceInstance.Status.PlanGUID = serviceInstance.Spec.PlanGUID serviceInstance.Status.MaintenanceInfo = assets.ServicePlan.Spec.MaintenanceInfo diff --git a/controllers/controllers/services/instances/managed/controller_test.go b/controllers/controllers/services/instances/managed/controller_test.go index e6685aa9b..d2670d557 100644 --- a/controllers/controllers/services/instances/managed/controller_test.go +++ b/controllers/controllers/services/instances/managed/controller_test.go @@ -811,7 +811,7 @@ var _ = Describe("CFServiceInstance", func() { HasStatus(Equal(metav1.ConditionTrue)), ))) g.Expect(instance.Status.Provisioned).To(BeTrue()) - g.Expect(instance.Spec.PlanGUID).To(Equal(servicePlan2.Name)) + g.Expect(instance.Status.PlanGUID).To(Equal(servicePlan2.Name)) }).Should(Succeed()) }) @@ -829,6 +829,58 @@ var _ = Describe("CFServiceInstance", func() { }).Should(Succeed()) }) + It("does not call the broker when only metadata changes", func() { + initialUpdateCallCount := brokerClient.UpdateCallCount() + + Expect(k8s.Patch(ctx, adminClient, instance, func() { + if instance.ObjectMeta.Labels == nil { + instance.ObjectMeta.Labels = map[string]string{} + } + instance.ObjectMeta.Labels["test-label"] = "test-value" + })).To(Succeed()) + + Consistently(func(g Gomega) { + g.Expect(brokerClient.UpdateCallCount()).To(Equal(initialUpdateCallCount)) + }).WithTimeout(2 * time.Second).Should(Succeed()) + }) + + When("the new plan is not visible", func() { + BeforeEach(func() { + Expect(k8s.PatchResource(ctx, adminClient, servicePlan2, func() { + servicePlan2.Spec.Visibility = korifiv1alpha1.ServicePlanVisibility{ + Type: korifiv1alpha1.AdminServicePlanVisibilityType, + } + })).To(Succeed()) + }) + + It("fails the instance with InvalidServicePlan", 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.ConditionFalse)), + HasReason(Equal("InvalidServicePlan")), + HasMessage(Equal("The service plan is disabled")), + ))) + }).Should(Succeed()) + }) + + It("does not send an update request to the broker", 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.ConditionFalse)), + HasReason(Equal("InvalidServicePlan")), + ))) + }).Should(Succeed()) + + Consistently(func(g Gomega) { + g.Expect(brokerClient.UpdateCallCount()).To(BeZero()) + }).WithTimeout(time.Second).Should(Succeed()) + }) + }) + When("service update fails with recoverable error", func() { BeforeEach(func() { brokerClient.UpdateReturns(osbapi.UpdateResponse{}, errors.New("update-failed")) diff --git a/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml b/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml index 6fb6d7d07..b6a2a8e94 100644 --- a/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml +++ b/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml @@ -217,6 +217,10 @@ spec: the CFServiceInstance that has been reconciled format: int64 type: integer + planGuid: + description: The plan guid of the service instance. Only makes seense + for managed service instances + type: string provisioned: description: True if the service instance has been successfully provisioned. Only makes seense for managed service instances From 27a56b6663877ff38d87b9205f13e7510188c1f5 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Tue, 9 Jun 2026 16:38:43 +0200 Subject: [PATCH 47/47] Apply suggections to tests --- .../services/instances/managed/controller.go | 12 + .../instances/managed/controller_test.go | 407 +++++++++--------- 2 files changed, 216 insertions(+), 203 deletions(-) diff --git a/controllers/controllers/services/instances/managed/controller.go b/controllers/controllers/services/instances/managed/controller.go index 905ccb684..2bb631b78 100644 --- a/controllers/controllers/services/instances/managed/controller.go +++ b/controllers/controllers/services/instances/managed/controller.go @@ -186,6 +186,10 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor return ctrl.Result{}, nil } + if isUpdateFailed(serviceInstance) { + return ctrl.Result{}, k8s.NewNotReadyError().WithReason("UpdateFailed").WithNoRequeue() + } + return r.reconcileUpdatedServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient) } @@ -620,6 +624,14 @@ func (r *Reconciler) getNamespace(ctx context.Context, namespaceName string) (*c return namespace, nil } +func isUpdateFailed(instance *korifiv1alpha1.CFServiceInstance) bool { + cond := meta.FindStatusCondition(instance.Status.Conditions, korifiv1alpha1.UpdateFailedCondition) + if cond == nil { + return false + } + return cond.Status == metav1.ConditionTrue && cond.ObservedGeneration == instance.Generation +} + func isProvisioningFailed(instance *korifiv1alpha1.CFServiceInstance) bool { return meta.IsStatusConditionTrue(instance.Status.Conditions, korifiv1alpha1.ProvisioningFailedCondition) } diff --git a/controllers/controllers/services/instances/managed/controller_test.go b/controllers/controllers/services/instances/managed/controller_test.go index d2670d557..4fefd2d0a 100644 --- a/controllers/controllers/services/instances/managed/controller_test.go +++ b/controllers/controllers/services/instances/managed/controller_test.go @@ -763,7 +763,7 @@ var _ = Describe("CFServiceInstance", func() { }) }) - When("the service instance is being updated", func() { + Describe("service instance update", func() { var servicePlan2 *korifiv1alpha1.CFServicePlan BeforeEach(func() { @@ -798,99 +798,44 @@ var _ = Describe("CFServiceInstance", func() { g.Expect(instance.Status.Provisioned).To(BeTrue()) }).Should(Succeed()) - Expect(k8s.Patch(ctx, adminClient, instance, func() { - instance.Spec.PlanGUID = servicePlan2.Name - })).To(Succeed()) - }) - - It("updates the PlanGUID status field", 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()) - g.Expect(instance.Status.PlanGUID).To(Equal(servicePlan2.Name)) - }).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()) - }) - - It("does not call the broker when only metadata changes", func() { - initialUpdateCallCount := brokerClient.UpdateCallCount() - Expect(k8s.Patch(ctx, adminClient, instance, func() { if instance.ObjectMeta.Labels == nil { instance.ObjectMeta.Labels = map[string]string{} } instance.ObjectMeta.Labels["test-label"] = "test-value" })).To(Succeed()) + }) + It("does not perform broker update", func() { Consistently(func(g Gomega) { - g.Expect(brokerClient.UpdateCallCount()).To(Equal(initialUpdateCallCount)) + g.Expect(brokerClient.UpdateCallCount()).To(BeZero()) }).WithTimeout(2 * time.Second).Should(Succeed()) }) - When("the new plan is not visible", func() { - BeforeEach(func() { - Expect(k8s.PatchResource(ctx, adminClient, servicePlan2, func() { - servicePlan2.Spec.Visibility = korifiv1alpha1.ServicePlanVisibility{ - Type: korifiv1alpha1.AdminServicePlanVisibilityType, - } + When("the plan is updated", func() { + JustBeforeEach(func() { + Expect(k8s.Patch(ctx, adminClient, instance, func() { + instance.Spec.PlanGUID = servicePlan2.Name })).To(Succeed()) }) - It("fails the instance with InvalidServicePlan", func() { + It("updates the PlanGUID status field", 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.ConditionFalse)), - HasReason(Equal("InvalidServicePlan")), - HasMessage(Equal("The service plan is disabled")), - ))) - }).Should(Succeed()) - }) - - It("does not send an update request to the broker", 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.ConditionFalse)), - HasReason(Equal("InvalidServicePlan")), + HasStatus(Equal(metav1.ConditionTrue)), ))) + g.Expect(instance.Status.Provisioned).To(BeTrue()) + g.Expect(instance.Status.PlanGUID).To(Equal(servicePlan2.Name)) }).Should(Succeed()) - - Consistently(func(g Gomega) { - g.Expect(brokerClient.UpdateCallCount()).To(BeZero()) - }).WithTimeout(time.Second).Should(Succeed()) - }) - }) - - When("service update fails with recoverable error", func() { - BeforeEach(func() { - brokerClient.UpdateReturns(osbapi.UpdateResponse{}, errors.New("update-failed")) }) - It("keeps trying to update the instance", func() { + It("sends an update request to the broker", func() { Eventually(func(g Gomega) { - g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">", 1)) - _, updatePayload := brokerClient.UpdateArgsForCall(brokerClient.UpdateCallCount() - 1) - g.Expect(updatePayload).To(Equal(osbapi.UpdatePayload{ + 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", @@ -900,188 +845,244 @@ var _ = Describe("CFServiceInstance", func() { }).Should(Succeed()) }) - It("sets in progress state in instance last operation", func() { - Eventually(func(g Gomega) { - g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) - g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">=", 1)) - g.Expect(instance.Status.LastOperation).To(Equal(korifiv1alpha1.LastOperation{ - Type: "update", - State: "in progress", - })) - }).Should(Succeed()) - }) - }) - - When("service update fails with unrecoverable error", func() { - BeforeEach(func() { - brokerClient.UpdateReturns(osbapi.UpdateResponse{}, osbapi.UnrecoverableError{Status: http.StatusBadRequest}) - }) - - It("fails the instance", func() { - Eventually(func(g Gomega) { - g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) + When("the new plan is not visible", func() { + BeforeEach(func() { + Expect(k8s.PatchResource(ctx, adminClient, servicePlan2, func() { + servicePlan2.Spec.Visibility = korifiv1alpha1.ServicePlanVisibility{ + Type: korifiv1alpha1.AdminServicePlanVisibilityType, + } + })).To(Succeed()) + }) - g.Expect(instance.Status.Conditions).To(ContainElements( - SatisfyAll( + It("fails the instance with InvalidServicePlan", 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.ConditionFalse)), - ), - SatisfyAll( - HasType(Equal(korifiv1alpha1.UpdateFailedCondition)), - HasStatus(Equal(metav1.ConditionTrue)), - HasReason(Equal("UpdateFailed")), - HasMessage(ContainSubstring("The server responded with status: 400")), - ), - )) - }).Should(Succeed()) - }) - - It("sets failed state in instance last operation", func() { - Eventually(func(g Gomega) { - g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) - g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">=", 1)) - g.Expect(instance.Status.LastOperation).To(Equal(korifiv1alpha1.LastOperation{ - Type: "update", - State: "failed", - })) - }).Should(Succeed()) - - currentCallCount := brokerClient.UpdateCallCount() - Consistently(func(g Gomega) { - g.Expect(brokerClient.UpdateCallCount()).To(Equal(currentCallCount)) - }).WithTimeout(time.Second).Should(Succeed()) - }) - }) - - When("the update is asynchronous", func() { - BeforeEach(func() { - brokerClient.GetServiceInstanceLastOperationReturns(osbapi.LastOperationResponse{ - State: "in-progress-or-whatever", - }, nil) - - brokerClient.UpdateReturns(osbapi.UpdateResponse{ - IsAsync: true, - Operation: "operation-1", - }, nil) - }) - - It("set sets ready condition to false", 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.ConditionFalse)), - HasReason(Equal("UpdateInProgress")), - ))) - }).Should(Succeed()) - }) + HasReason(Equal("InvalidServicePlan")), + HasMessage(Equal("The service plan is disabled")), + ))) + }).Should(Succeed()) + }) - It("sets in progress state in instance last operation", func() { - Eventually(func(g Gomega) { - g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) - g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">=", 1)) - g.Expect(instance.Status.LastOperation).To(Equal(korifiv1alpha1.LastOperation{ - Type: "update", - State: "in progress", - })) - }).Should(Succeed()) - }) + It("does not send an update request to the broker", 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.ConditionFalse)), + HasReason(Equal("InvalidServicePlan")), + ))) + }).Should(Succeed()) - It("continuously checks the last operation", func() { - Eventually(func(g Gomega) { - g.Expect(brokerClient.GetServiceInstanceLastOperationCallCount()).To(BeNumerically(">", 1)) - _, lastOp := brokerClient.GetServiceInstanceLastOperationArgsForCall(brokerClient.GetServiceInstanceLastOperationCallCount() - 1) - g.Expect(lastOp).To(Equal(osbapi.GetInstanceLastOperationRequest{ - InstanceID: instance.Name, - GetLastOperationRequestParameters: osbapi.GetLastOperationRequestParameters{ - ServiceId: "service-offering-id", - PlanID: "service-plan-id-2", - Operation: "operation-1", - }, - })) - }).Should(Succeed()) + Consistently(func(g Gomega) { + g.Expect(brokerClient.UpdateCallCount()).To(BeZero()) + }).WithTimeout(time.Second).Should(Succeed()) + }) }) - When("getting service last operation fails", func() { + When("service update fails with recoverable error", func() { BeforeEach(func() { - brokerClient.GetServiceInstanceLastOperationReturns(osbapi.LastOperationResponse{}, errors.New("get-last-op-failed")) + brokerClient.UpdateReturns(osbapi.UpdateResponse{}, errors.New("update-failed")) }) - It("sets the ready condition to false", func() { + It("keeps trying to update the instance", func() { Eventually(func(g Gomega) { - g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) + g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">", 1)) + _, updatePayload := brokerClient.UpdateArgsForCall(brokerClient.UpdateCallCount() - 1) + g.Expect(updatePayload).To(Equal(osbapi.UpdatePayload{ + InstanceID: instance.Name, + UpdateRequest: osbapi.UpdateRequest{ + ServiceId: "service-offering-id", + PlanID: "service-plan-id-2", + }, + })) + }).Should(Succeed()) + }) - g.Expect(instance.Status.Conditions).To(ContainElement(SatisfyAll( - HasType(Equal(korifiv1alpha1.StatusConditionReady)), - HasStatus(Equal(metav1.ConditionFalse)), - ))) + It("sets in progress state in instance last operation", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) + g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">=", 1)) + g.Expect(instance.Status.LastOperation).To(Equal(korifiv1alpha1.LastOperation{ + Type: "update", + State: "in progress", + })) }).Should(Succeed()) }) }) - When("the last operation is succeeded", func() { + When("service update fails with unrecoverable error", func() { BeforeEach(func() { - brokerClient.GetServiceInstanceLastOperationReturns(osbapi.LastOperationResponse{ - State: "succeeded", - }, nil) + brokerClient.UpdateReturns(osbapi.UpdateResponse{}, osbapi.UnrecoverableError{Status: http.StatusBadRequest}) }) - It("sets the ready condition to true", func() { + It("fails the instance", 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.Conditions).To(ContainElements( + SatisfyAll( + HasType(Equal(korifiv1alpha1.StatusConditionReady)), + HasStatus(Equal(metav1.ConditionFalse)), + ), + SatisfyAll( + HasType(Equal(korifiv1alpha1.UpdateFailedCondition)), + HasStatus(Equal(metav1.ConditionTrue)), + HasReason(Equal("UpdateFailed")), + HasMessage(ContainSubstring("The server responded with status: 400")), + ), + )) }).Should(Succeed()) }) + + It("sets failed state in instance last operation", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) + g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">=", 1)) + g.Expect(instance.Status.LastOperation).To(Equal(korifiv1alpha1.LastOperation{ + Type: "update", + State: "failed", + })) + }).Should(Succeed()) + + Consistently(func(g Gomega) { + g.Expect(brokerClient.UpdateCallCount()).To(Equal(1)) + }).WithTimeout(time.Second).Should(Succeed()) + }) }) - When("the last operation is failed", func() { + When("the update is asynchronous", func() { BeforeEach(func() { brokerClient.GetServiceInstanceLastOperationReturns(osbapi.LastOperationResponse{ - State: "failed", - Description: "update-failed", + State: "in-progress-or-whatever", + }, nil) + + brokerClient.UpdateReturns(osbapi.UpdateResponse{ + IsAsync: true, + Operation: "operation-1", }, nil) }) - It("sets the ready condition to false", func() { + It("set sets ready condition to false", 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.ConditionFalse)), + HasReason(Equal("UpdateInProgress")), ))) }).Should(Succeed()) }) - It("sets the failed condition", func() { + It("sets in progress state in instance last operation", 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.UpdateFailedCondition)), - HasStatus(Equal(metav1.ConditionTrue)), - HasReason(Equal("UpdateFailed")), - HasMessage(Equal("update-failed")), - ))) + g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">=", 1)) + g.Expect(instance.Status.LastOperation).To(Equal(korifiv1alpha1.LastOperation{ + Type: "update", + State: "in progress", + })) }).Should(Succeed()) }) - It("sets failed state in instance last operation", func() { + It("continuously checks the last operation", func() { Eventually(func(g Gomega) { - g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) - g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">=", 1)) - g.Expect(instance.Status.LastOperation).To(Equal(korifiv1alpha1.LastOperation{ - Type: "update", - State: "failed", - Description: "update-failed", + g.Expect(brokerClient.GetServiceInstanceLastOperationCallCount()).To(BeNumerically(">", 1)) + _, lastOp := brokerClient.GetServiceInstanceLastOperationArgsForCall(brokerClient.GetServiceInstanceLastOperationCallCount() - 1) + g.Expect(lastOp).To(Equal(osbapi.GetInstanceLastOperationRequest{ + InstanceID: instance.Name, + GetLastOperationRequestParameters: osbapi.GetLastOperationRequestParameters{ + ServiceId: "service-offering-id", + PlanID: "service-plan-id-2", + Operation: "operation-1", + }, })) }).Should(Succeed()) }) + + When("getting service last operation fails", func() { + BeforeEach(func() { + brokerClient.GetServiceInstanceLastOperationReturns(osbapi.LastOperationResponse{}, errors.New("get-last-op-failed")) + }) + + It("sets the ready condition to false", 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.ConditionFalse)), + ))) + }).Should(Succeed()) + }) + }) + + When("the last operation is succeeded", func() { + BeforeEach(func() { + brokerClient.GetServiceInstanceLastOperationReturns(osbapi.LastOperationResponse{ + State: "succeeded", + }, nil) + }) + + It("sets the ready condition to true", 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)), + ))) + }).Should(Succeed()) + }) + }) + + When("the last operation is failed", func() { + BeforeEach(func() { + brokerClient.GetServiceInstanceLastOperationReturns(osbapi.LastOperationResponse{ + State: "failed", + Description: "update-failed", + }, nil) + }) + + It("sets the ready condition to false", 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.ConditionFalse)), + ))) + }).Should(Succeed()) + }) + + It("sets the failed condition", 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.UpdateFailedCondition)), + HasStatus(Equal(metav1.ConditionTrue)), + HasReason(Equal("UpdateFailed")), + HasMessage(Equal("update-failed")), + ))) + }).Should(Succeed()) + }) + + It("sets failed state in instance last operation", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) + g.Expect(brokerClient.UpdateCallCount()).To(BeNumerically(">=", 1)) + g.Expect(instance.Status.LastOperation).To(Equal(korifiv1alpha1.LastOperation{ + Type: "update", + State: "failed", + Description: "update-failed", + })) + }).Should(Succeed()) + }) + }) }) }) })