Implement instances last operation

Previously we were setting the ProvisioningFailedCondition to true if
the broker returned error on provision, no matter the error. The problem
with doing this is that the error can be recoverable, such as the
network flaking, but the instance is sent to a state from which it
cannot recover (When the ProvisioningFailedCondition is set the
reconciler gives up early in the reconciliation loop).

This change fixes the problem described above by defining a new
UnrecoverableError type in the osbapi client. The controller would then set the
ProvisioningFailedCondition only if the error it received is an
UnrecoverableError.

Also LastOperation field is added in CFServiceInstance and there is a
logic implemented in the controllers about the different states and this
last operation information is propagated to the presenter logic

According to the OSBAPI spec unrecoverable perovision error codes are
400, 409 and 422.

Co-authored-by: Danail Branekov <danailster@gmail.com>

WIP

Co-authored-by: Danail Branekov <danailster@gmail.com>

WIP

Co-authored-by: Danail Branekov <danailster@gmail.com>

api/presenter/service_instance.go

@@ -45,11 +45,6 @@ type ServiceInstanceLinks struct {
 }
 
 func ForServiceInstance(serviceInstanceRecord repositories.ServiceInstanceRecord, baseURL url.URL, includes ...model.IncludedResource) ServiceInstanceResponse {
-	lastOperationType := "update"
-	if serviceInstanceRecord.UpdatedAt == nil || serviceInstanceRecord.CreatedAt.Equal(*serviceInstanceRecord.UpdatedAt) {
-		lastOperationType = "create"
-	}
-
 	return ServiceInstanceResponse{
 		Name: serviceInstanceRecord.Name,
 		GUID: serviceInstanceRecord.GUID,
@@ -58,9 +53,9 @@ func ForServiceInstance(serviceInstanceRecord repositories.ServiceInstanceRecord
 		LastOperation: lastOperation{
 			CreatedAt:   formatTimestamp(&serviceInstanceRecord.CreatedAt),
 			UpdatedAt:   formatTimestamp(serviceInstanceRecord.UpdatedAt),
-			Description: "Operation succeeded",
-			State:       "succeeded",
-			Type:        lastOperationType,
+			Description: serviceInstanceRecord.LastOperation.Description,
+			State:       serviceInstanceRecord.LastOperation.State,
+			Type:        serviceInstanceRecord.LastOperation.Type,
 		},
 		CreatedAt:     formatTimestamp(&serviceInstanceRecord.CreatedAt),
 		UpdatedAt:     formatTimestamp(serviceInstanceRecord.UpdatedAt),

api/presenter/service_instance_test.go

@@ -7,6 +7,7 @@ import (
 
 	"code.cloudfoundry.org/korifi/api/presenter"
 	"code.cloudfoundry.org/korifi/api/repositories"
+	"code.cloudfoundry.org/korifi/model/services"
 	. "code.cloudfoundry.org/korifi/tests/matchers"
 	"code.cloudfoundry.org/korifi/tools"
 	. "github.com/onsi/ginkgo/v2"
@@ -36,6 +37,11 @@ var _ = Describe("Service Instance", func() {
 			Labels: map[string]string{
 				"foo": "bar",
 			},
+			LastOperation: services.LastOperation{
+				Type:        "update",
+				State:       "succeeded",
+				Description: "Operation succeeded",
+			},
 			Annotations: map[string]string{
 				"one": "two",
 			},
@@ -104,16 +110,6 @@ var _ = Describe("Service Instance", func() {
 		}`))
 	})
 
-	When("create and update times are the same", func() {
-		BeforeEach(func() {
-			record.UpdatedAt = &record.CreatedAt
-		})
-
-		It("sets last operation type to create", func() {
-			Expect(output).To(MatchJSONPath("$.last_operation.type", "create"))
-		})
-	})
-
 	When("labels is nil", func() {
 		BeforeEach(func() {
 			record.Labels = nil

api/repositories/service_instance_repository.go

@@ -14,6 +14,7 @@ import (
 	"code.cloudfoundry.org/korifi/api/repositories/compare"
 	korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
 	"code.cloudfoundry.org/korifi/model"
+	"code.cloudfoundry.org/korifi/model/services"
 	"code.cloudfoundry.org/korifi/tools"
 	"code.cloudfoundry.org/korifi/tools/k8s"
 
@@ -169,19 +170,20 @@ type DeleteServiceInstanceMessage struct {
 }
 
 type ServiceInstanceRecord struct {
-	Name        string
-	GUID        string
-	SpaceGUID   string
-	PlanGUID    string
-	SecretName  string
-	Tags        []string
-	Type        string
-	Labels      map[string]string
-	Annotations map[string]string
-	CreatedAt   time.Time
-	UpdatedAt   *time.Time
-	DeletedAt   *time.Time
-	Ready       bool
+	Name          string
+	GUID          string
+	SpaceGUID     string
+	PlanGUID      string
+	SecretName    string
+	Tags          []string
+	Type          string
+	Labels        map[string]string
+	Annotations   map[string]string
+	CreatedAt     time.Time
+	UpdatedAt     *time.Time
+	DeletedAt     *time.Time
+	LastOperation services.LastOperation
+	Ready         bool
 }
 
 func (r ServiceInstanceRecord) Relationships() map[string]string {
@@ -545,19 +547,20 @@ func (r *ServiceInstanceRepo) removeBindingsFinalizer(ctx context.Context, userC
 
 func cfServiceInstanceToRecord(cfServiceInstance korifiv1alpha1.CFServiceInstance) ServiceInstanceRecord {
 	return ServiceInstanceRecord{
-		Name:        cfServiceInstance.Spec.DisplayName,
-		GUID:        cfServiceInstance.Name,
-		SpaceGUID:   cfServiceInstance.Namespace,
-		PlanGUID:    cfServiceInstance.Spec.PlanGUID,
-		SecretName:  cfServiceInstance.Spec.SecretName,
-		Tags:        cfServiceInstance.Spec.Tags,
-		Type:        string(cfServiceInstance.Spec.Type),
-		Labels:      cfServiceInstance.Labels,
-		Annotations: cfServiceInstance.Annotations,
-		CreatedAt:   cfServiceInstance.CreationTimestamp.Time,
-		UpdatedAt:   getLastUpdatedTime(&cfServiceInstance),
-		DeletedAt:   golangTime(cfServiceInstance.DeletionTimestamp),
-		Ready:       isInstanceReady(cfServiceInstance),
+		Name:          cfServiceInstance.Spec.DisplayName,
+		GUID:          cfServiceInstance.Name,
+		SpaceGUID:     cfServiceInstance.Namespace,
+		PlanGUID:      cfServiceInstance.Spec.PlanGUID,
+		SecretName:    cfServiceInstance.Spec.SecretName,
+		Tags:          cfServiceInstance.Spec.Tags,
+		Type:          string(cfServiceInstance.Spec.Type),
+		Labels:        cfServiceInstance.Labels,
+		Annotations:   cfServiceInstance.Annotations,
+		CreatedAt:     cfServiceInstance.CreationTimestamp.Time,
+		UpdatedAt:     getLastUpdatedTime(&cfServiceInstance),
+		DeletedAt:     golangTime(cfServiceInstance.DeletionTimestamp),
+		LastOperation: cfServiceInstance.Status.LastOperation,
+		Ready:         isInstanceReady(cfServiceInstance),
 	}
 }
 

api/repositories/service_instance_repository_test.go

@@ -13,6 +13,7 @@ import (
 	"code.cloudfoundry.org/korifi/api/repositories/fakeawaiter"
 	korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
 	"code.cloudfoundry.org/korifi/model"
+	"code.cloudfoundry.org/korifi/model/services"
 	"code.cloudfoundry.org/korifi/tests/matchers"
 	"code.cloudfoundry.org/korifi/tools"
 	"code.cloudfoundry.org/korifi/tools/k8s"
@@ -306,6 +307,50 @@ var _ = Describe("ServiceInstanceRepository", func() {
 		})
 	})
 
+	Describe("instance record last operation", func() {
+		var (
+			cfServiceInstance     *korifiv1alpha1.CFServiceInstance
+			serviceInstanceRecord repositories.ServiceInstanceRecord
+		)
+
+		BeforeEach(func() {
+			createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space.Name)
+
+			cfServiceInstance = &korifiv1alpha1.CFServiceInstance{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      uuid.NewString(),
+					Namespace: space.Name,
+				},
+				Spec: korifiv1alpha1.CFServiceInstanceSpec{
+					Type: korifiv1alpha1.ManagedType,
+				},
+			}
+			Expect(k8sClient.Create(ctx, cfServiceInstance)).To(Succeed())
+
+			Expect(k8s.Patch(ctx, k8sClient, cfServiceInstance, func() {
+				cfServiceInstance.Status.LastOperation = services.LastOperation{
+					Type:        "create",
+					State:       "failed",
+					Description: "failed due to error",
+				}
+			})).To(Succeed())
+		})
+
+		JustBeforeEach(func() {
+			var err error
+			serviceInstanceRecord, err = serviceInstanceRepo.GetServiceInstance(ctx, authInfo, cfServiceInstance.Name)
+			Expect(err).NotTo(HaveOccurred())
+		})
+
+		It("returns  last operation", func() {
+			Expect(serviceInstanceRecord.LastOperation).To(Equal(services.LastOperation{
+				Type:        "create",
+				State:       "failed",
+				Description: "failed due to error",
+			}))
+		})
+	})
+
 	Describe("GetDeletedAt", func() {
 		var (
 			cfServiceInstance *korifiv1alpha1.CFServiceInstance

controllers/api/v1alpha1/cfserviceinstance_types.go

@@ -19,6 +19,7 @@ package v1alpha1
 import (
 	"fmt"
 
+	"code.cloudfoundry.org/korifi/model/services"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	runtime "k8s.io/apimachinery/pkg/runtime"
@@ -79,6 +80,9 @@ type CFServiceInstanceStatus struct {
 	// This will ensure that interested contollers are notified on instance credentials change
 	//+kubebuilder:validation:Optional
 	CredentialsObservedVersion string `json:"credentialsObservedVersion,omitempty"`
+
+	//+kubebuilder:validation:Optional
+	LastOperation services.LastOperation `json:"last_operation"`
 }
 
 //+kubebuilder:object:root=true

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

@@ -26,6 +26,7 @@ import (
 	korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
 	"code.cloudfoundry.org/korifi/controllers/controllers/services/osbapi"
 	"code.cloudfoundry.org/korifi/controllers/controllers/shared"
+	"code.cloudfoundry.org/korifi/model/services"
 	"code.cloudfoundry.org/korifi/tools"
 	"code.cloudfoundry.org/korifi/tools/k8s"
 
@@ -116,7 +117,7 @@ func (r *Reconciler) isManaged(object client.Object) bool {
 }
 
 //+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;atch
+//+kubebuilder:rbac:groups=korifi.cloudfoundry.org,resources=cfserviceinstances/status,verbs=get;update;patch
 //+kubebuilder:rbac:groups=korifi.cloudfoundry.org,resources=cfserviceinstances/finalizers,verbs=update
 
 func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *korifiv1alpha1.CFServiceInstance) (ctrl.Result, error) {
@@ -174,6 +175,7 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor
 		return r.pollProvisionOperation(ctx, serviceInstance, serviceInstanceAssets, osbapiClient, provisionResponse.Operation)
 	}
 
+	serviceInstance.Status.LastOperation.State = "succeeded"
 	return ctrl.Result{}, nil
 }
 
@@ -224,6 +226,11 @@ func (r *Reconciler) provisionServiceInstance(
 		return osbapi.ServiceInstanceOperationResponse{}, err
 	}
 
+	serviceInstance.Status.LastOperation = services.LastOperation{
+		Type:  "create",
+		State: "initial",
+	}
+
 	var provisionResponse osbapi.ServiceInstanceOperationResponse
 	provisionResponse, err = osbapiClient.Provision(ctx, osbapi.InstanceProvisionPayload{
 		InstanceID: serviceInstance.Name,
@@ -239,6 +246,7 @@ func (r *Reconciler) provisionServiceInstance(
 		log.Error(err, "failed to provision service")
 
 		if osbapi.IsUnrecoveralbeError(err) {
+			serviceInstance.Status.LastOperation.State = "failed"
 			meta.SetStatusCondition(&serviceInstance.Status.Conditions, metav1.Condition{
 				Type:               korifiv1alpha1.ProvisioningFailedCondition,
 				Status:             metav1.ConditionTrue,
@@ -279,6 +287,9 @@ func (r *Reconciler) pollProvisionOperation(
 		return ctrl.Result{}, k8s.NewNotReadyError().WithCause(err).WithReason("GetLastOperationFailed")
 	}
 
+	serviceInstance.Status.LastOperation.State = lastOpResponse.State
+	serviceInstance.Status.LastOperation.Description = lastOpResponse.Description
+
 	if lastOpResponse.State == "in progress" {
 		return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ProvisionInProgress").WithRequeue()
 	}

controllers/controllers/services/instances/managed/controller_test.go

@@ -138,6 +138,7 @@ var _ = Describe("CFServiceInstance", func() {
 				},
 			},
 		}
+
 		Expect(adminClient.Create(ctx, instance)).To(Succeed())
 	})
 
@@ -211,6 +212,17 @@ var _ = Describe("CFServiceInstance", func() {
 		}).Should(Succeed())
 	})
 
+	It("sets succeeded state in instance last operation", func() {
+		Eventually(func(g Gomega) {
+			g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed())
+			g.Expect(brokerClient.ProvisionCallCount()).To(BeNumerically(">=", 1))
+			g.Expect(instance.Status.LastOperation).To(Equal(services.LastOperation{
+				Type:  "create",
+				State: "succeeded",
+			}))
+		}).Should(Succeed())
+	})
+
 	When("the service instance parameters are not set", func() {
 		BeforeEach(func() {
 			Expect(k8s.PatchResource(ctx, adminClient, instance, func() {
@@ -330,6 +342,17 @@ var _ = Describe("CFServiceInstance", func() {
 				}))
 			}).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.ProvisionCallCount()).To(BeNumerically(">=", 1))
+				g.Expect(instance.Status.LastOperation).To(Equal(services.LastOperation{
+					Type:  "create",
+					State: "initial",
+				}))
+			}).Should(Succeed())
+		})
 	})
 
 	When("service provisioning fails with unrecoverable error", func() {
@@ -355,6 +378,17 @@ var _ = Describe("CFServiceInstance", func() {
 				))
 			}).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.ProvisionCallCount()).To(BeNumerically(">=", 1))
+				g.Expect(instance.Status.LastOperation).To(Equal(services.LastOperation{
+					Type:  "create",
+					State: "failed",
+				}))
+			}).Should(Succeed())
+		})
 	})
 
 	When("getting service last operation fails", func() {
@@ -392,6 +426,17 @@ 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.ProvisionCallCount()).To(BeNumerically(">=", 1))
+				g.Expect(instance.Status.LastOperation).To(Equal(services.LastOperation{
+					Type:  "create",
+					State: "in progress",
+				}))
+			}).Should(Succeed())
+		})
+
 		It("keeps checking last operation", func() {
 			Eventually(func(g Gomega) {
 				g.Expect(brokerClient.GetServiceInstanceLastOperationCallCount()).To(BeNumerically(">", 1))
@@ -436,6 +481,17 @@ var _ = Describe("CFServiceInstance", func() {
 				)))
 			}).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.ProvisionCallCount()).To(BeNumerically(">=", 1))
+				g.Expect(instance.Status.LastOperation).To(Equal(services.LastOperation{
+					Type:  "create",
+					State: "failed",
+				}))
+			}).Should(Succeed())
+		})
 	})
 
 	When("the instance has become ready", func() {