From 6fce46e89fb140eb783c30d9a3ebc03a33b82a0a Mon Sep 17 00:00:00 2001 From: Danail Branekov Date: Thu, 9 Jan 2025 11:51:44 +0000 Subject: [PATCH] Ensure credentials secret is created on async bind * When the bind operation state is `succeeded`, requeue the reconcile request. Thus we ensure that the controller would try to `bind` again, and as the bind operation has already succeeded, it would get a response containing the binding credentials * When checking the state of the last operation in asynchronous provision/bind, instance/binding controllers consider the operation to have completed when its state is either `succeeded`, or `failed`. Anything else is considered as if the operation is ongoing. Thus we protect ourselves from brokers returning nonsense operation state * The test for the instance controller is refactored so that the synchronous provisioning is the "default" case and there is a dedicated context for asynchronous provisioning. This aligns test style of instance/binding controller tests --- .../services/bindings/controller_test.go | 17 +- .../services/bindings/managed/controller.go | 8 +- .../services/instances/managed/controller.go | 8 +- .../instances/managed/controller_test.go | 257 +++++++++--------- .../controllers/services/osbapi/types.go | 14 +- 5 files changed, 160 insertions(+), 144 deletions(-) diff --git a/controllers/controllers/services/bindings/controller_test.go b/controllers/controllers/services/bindings/controller_test.go index ae8772074..0144202da 100644 --- a/controllers/controllers/services/bindings/controller_test.go +++ b/controllers/controllers/services/bindings/controller_test.go @@ -798,7 +798,7 @@ var _ = Describe("CFServiceBinding", func() { }, nil) brokerClient.GetServiceBindingLastOperationReturns(osbapi.LastOperationResponse{ - State: "in progress", + State: "in-progress-or-whatever", }, nil) }) @@ -809,6 +809,7 @@ var _ = Describe("CFServiceBinding", func() { g.Expect(binding.Status.Conditions).To(ContainElement(SatisfyAll( HasType(Equal(korifiv1alpha1.StatusConditionReady)), HasStatus(Equal(metav1.ConditionFalse)), + HasReason(Equal("BindingInProgress")), ))) }).Should(Succeed()) }) @@ -841,6 +842,7 @@ var _ = Describe("CFServiceBinding", func() { g.Expect(binding.Status.Conditions).To(ContainElement(SatisfyAll( HasType(Equal(korifiv1alpha1.StatusConditionReady)), HasStatus(Equal(metav1.ConditionFalse)), + HasReason(Equal("GetLastOperationFailed")), HasMessage(ContainSubstring("get-last-op-failed")), ))) }).Should(Succeed()) @@ -880,13 +882,24 @@ var _ = Describe("CFServiceBinding", func() { State: "succeeded", }, nil) - brokerClient.BindReturns(osbapi.BindResponse{ + brokerClient.BindReturnsOnCall(3, osbapi.BindResponse{ Credentials: map[string]any{ "foo": "bar", }, }, nil) }) + It("sets the ready condition to true", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(binding), binding)).To(Succeed()) + + g.Expect(binding.Status.Conditions).To(ContainElement(SatisfyAll( + HasType(Equal(korifiv1alpha1.StatusConditionReady)), + HasStatus(Equal(metav1.ConditionTrue)), + ))) + }).Should(Succeed()) + }) + It("creates the credentials secret", func() { Eventually(func(g Gomega) { g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(binding), binding)).To(Succeed()) diff --git a/controllers/controllers/services/bindings/managed/controller.go b/controllers/controllers/services/bindings/managed/controller.go index 1300dec35..f77354ae3 100644 --- a/controllers/controllers/services/bindings/managed/controller.go +++ b/controllers/controllers/services/bindings/managed/controller.go @@ -147,8 +147,8 @@ func (r *ManagedBindingsReconciler) processBindOperation( cfServiceBinding *korifiv1alpha1.CFServiceBinding, lastOperation osbapi.LastOperationResponse, ) (ctrl.Result, error) { - if lastOperation.State == "in progress" { - return ctrl.Result{}, k8s.NewNotReadyError().WithReason("BindingInProgress").WithRequeue() + if lastOperation.State == "succeeded" { + return ctrl.Result{Requeue: true}, nil } if lastOperation.State == "failed" { @@ -160,10 +160,10 @@ func (r *ManagedBindingsReconciler) processBindOperation( Reason: "BindingFailed", Message: lastOperation.Description, }) - return ctrl.Result{}, k8s.NewNotReadyError().WithReason("BindingFailed") + return ctrl.Result{}, k8s.NewNotReadyError().WithReason("BindingFailed").WithMessage(lastOperation.Description) } - return ctrl.Result{}, nil + return ctrl.Result{}, k8s.NewNotReadyError().WithReason("BindingInProgress").WithRequeue() } func (r *ManagedBindingsReconciler) reconcileCredentials(ctx context.Context, cfServiceBinding *korifiv1alpha1.CFServiceBinding, creds map[string]any) error { diff --git a/controllers/controllers/services/instances/managed/controller.go b/controllers/controllers/services/instances/managed/controller.go index a97860cc7..671f5dbd4 100644 --- a/controllers/controllers/services/instances/managed/controller.go +++ b/controllers/controllers/services/instances/managed/controller.go @@ -247,8 +247,8 @@ func (r *Reconciler) processProvisionOperation( serviceInstance *korifiv1alpha1.CFServiceInstance, lastOpResponse osbapi.LastOperationResponse, ) (ctrl.Result, error) { - if lastOpResponse.State == "in progress" { - return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ProvisionInProgress").WithRequeue() + if lastOpResponse.State == "succeeded" { + return ctrl.Result{}, nil } if lastOpResponse.State == "failed" { @@ -263,7 +263,7 @@ func (r *Reconciler) processProvisionOperation( return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ProvisionFailed") } - return ctrl.Result{}, nil + return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ProvisionInProgress").WithRequeue() } func (r *Reconciler) finalizeCFServiceInstance( @@ -381,7 +381,7 @@ func (r *Reconciler) pollLastOperation( return osbapi.LastOperationResponse{}, k8s.NewNotReadyError().WithCause(err).WithReason("GetLastOperationFailed") } - serviceInstance.Status.LastOperation.State = lastOpResponse.State + serviceInstance.Status.LastOperation.State = lastOpResponse.State.Value() serviceInstance.Status.LastOperation.Description = lastOpResponse.Description return lastOpResponse, nil } diff --git a/controllers/controllers/services/instances/managed/controller_test.go b/controllers/controllers/services/instances/managed/controller_test.go index 9f71d1676..ce92d8e0b 100644 --- a/controllers/controllers/services/instances/managed/controller_test.go +++ b/controllers/controllers/services/instances/managed/controller_test.go @@ -38,14 +38,7 @@ var _ = Describe("CFServiceInstance", func() { brokerClient = new(fake.BrokerClient) brokerClientFactory.CreateClientReturns(brokerClient, nil) - brokerClient.ProvisionReturns(osbapi.ProvisionResponse{ - IsAsync: true, - Operation: "operation-1", - }, nil) - - brokerClient.GetServiceInstanceLastOperationReturns(osbapi.LastOperationResponse{ - State: "succeeded", - }, nil) + brokerClient.ProvisionReturns(osbapi.ProvisionResponse{}, nil) serviceBroker = &korifiv1alpha1.CFServiceBroker{ ObjectMeta: metav1.ObjectMeta{ @@ -181,6 +174,12 @@ var _ = Describe("CFServiceInstance", func() { }) }) + It("does not check last operation", func() { + Consistently(func(g Gomega) { + g.Expect(brokerClient.GetServiceInstanceLastOperationCallCount()).To(Equal(0)) + }).Should(Succeed()) + }) + It("provisions the service", func() { Eventually(func(g Gomega) { g.Expect(brokerClient.ProvisionCallCount()).NotTo(BeZero()) @@ -197,18 +196,6 @@ var _ = Describe("CFServiceInstance", func() { }, }, })) - - g.Expect(brokerClient.GetServiceInstanceLastOperationCallCount()).To(BeNumerically(">", 0)) - _, 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", - Operation: "operation-1", - }, - })) - g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) }).Should(Succeed()) }) @@ -296,27 +283,136 @@ var _ = Describe("CFServiceInstance", func() { }) }) - When("the provisioning is synchronous", func() { + When("the provisioning is asynchronous", func() { BeforeEach(func() { - brokerClient.ProvisionReturns(osbapi.ProvisionResponse{}, nil) - }) + brokerClient.GetServiceInstanceLastOperationReturns(osbapi.LastOperationResponse{ + State: "in-progress-or-whatever", + }, nil) - It("does not check last operation", func() { - Consistently(func(g Gomega) { - g.Expect(brokerClient.GetServiceInstanceLastOperationCallCount()).To(Equal(0)) - }).Should(Succeed()) + brokerClient.ProvisionReturns(osbapi.ProvisionResponse{ + IsAsync: true, + Operation: "operation-1", + }, nil) }) - It("set sets ready condition to true", 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.ConditionTrue)), + HasStatus(Equal(metav1.ConditionFalse)), + HasReason(Equal("ProvisionInProgress")), ))) }).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("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", + 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: "provision-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.ProvisioningFailedCondition)), + HasStatus(Equal(metav1.ConditionTrue)), + HasReason(Equal("ProvisionFailed")), + HasMessage(Equal("provision-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.ProvisionCallCount()).To(BeNumerically(">=", 1)) + g.Expect(instance.Status.LastOperation).To(Equal(services.LastOperation{ + Type: "create", + State: "failed", + Description: "provision-failed", + })) + }).Should(Succeed()) + }) + }) }) When("service provisioning fails with recoverable error", func() { @@ -391,109 +487,6 @@ var _ = Describe("CFServiceInstance", func() { }) }) - 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 in progress", func() { - BeforeEach(func() { - brokerClient.GetServiceInstanceLastOperationReturns(osbapi.LastOperationResponse{ - State: "in progress", - }, 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 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)) - _, actualLastOpPayload := brokerClient.GetServiceInstanceLastOperationArgsForCall(1) - g.Expect(actualLastOpPayload).To(Equal(osbapi.GetInstanceLastOperationRequest{ - InstanceID: instance.Name, - GetLastOperationRequestParameters: osbapi.GetLastOperationRequestParameters{ - ServiceId: "service-offering-id", - PlanID: "service-plan-id", - Operation: "operation-1", - }, - })) - }).Should(Succeed()) - }) - }) - - When("the last operation is failed", func() { - BeforeEach(func() { - brokerClient.GetServiceInstanceLastOperationReturns(osbapi.LastOperationResponse{ - State: "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.ProvisioningFailedCondition)), - HasStatus(Equal(metav1.ConditionTrue)), - ))) - }).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() { BeforeEach(func() { Expect(k8s.Patch(ctx, adminClient, instance, func() { diff --git a/controllers/controllers/services/osbapi/types.go b/controllers/controllers/services/osbapi/types.go index e3cae1343..908652238 100644 --- a/controllers/controllers/services/osbapi/types.go +++ b/controllers/controllers/services/osbapi/types.go @@ -142,6 +142,16 @@ func (r UnbindResponse) IsComplete() bool { } type LastOperationResponse struct { - State string `json:"state"` - Description string `json:"description"` + State LastOperationResponseState `json:"state"` + Description string `json:"description"` +} + +type LastOperationResponseState string + +func (s LastOperationResponseState) Value() string { + if s == "succeeded" || s == "failed" { + return string(s) + } + + return "in progress" }