Add Status entry initialization in binding and instance controller (#5)

pkg/controller/controller_binding.go

@@ -20,21 +20,22 @@ import (
 	"bytes"
 	"fmt"
 	"net"
+	"reflect"
 
 	osb "github.com/kubernetes-sigs/go-open-service-broker-client/v2"
-	utilfeature "k8s.io/apiserver/pkg/util/feature"
-	"k8s.io/klog"
-
 	"github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1"
 	scfeatures "github.com/kubernetes-sigs/service-catalog/pkg/features"
 	"github.com/kubernetes-sigs/service-catalog/pkg/pretty"
+
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/util/jsonpath"
+	"k8s.io/klog"
 )
 
 const (
@@ -177,6 +178,15 @@ func (c *controller) reconcileServiceBinding(binding *v1beta1.ServiceBinding) er
 func (c *controller) reconcileServiceBindingAdd(binding *v1beta1.ServiceBinding) error {
 	pcb := pretty.NewBindingContextBuilder(binding)
 
+	if !c.isServiceBindingStatusInitialized(binding) {
+		klog.V(4).Info(pcb.Message("Initialize Status entry"))
+		if err := c.initializeServiceBindingStatus(binding); err != nil {
+			klog.Errorf(pcb.Messagef("Error initializing status: %v", err))
+			return err
+		}
+		return nil
+	}
+
 	if isServiceBindingFailed(binding) {
 		klog.V(4).Info(pcb.Message("not processing event; status showed that it has failed"))
 		return nil
@@ -763,6 +773,34 @@ func (c *controller) updateServiceBindingCondition(
 	return err
 }
 
+func (c *controller) isServiceBindingStatusInitialized(binding *v1beta1.ServiceBinding) bool {
+	emptyStatus := v1beta1.ServiceBindingStatus{}
+	if reflect.DeepEqual(binding.Status, emptyStatus) {
+		return false
+	}
+
+	return true
+}
+
+// initializeServiceBindingStatus initialize the ServiceBindingStatus.
+// In normal scenario it should be done when client is creating the ServiceBinding,
+// but right now we cannot modify the Status (sub-resource) in webhook on CREATE action.
+// As a temporary solution we are doing that in the reconcile function.
+func (c *controller) initializeServiceBindingStatus(binding *v1beta1.ServiceBinding) error {
+	updated := binding.DeepCopy()
+	updated.Status = v1beta1.ServiceBindingStatus{
+		Conditions:   []v1beta1.ServiceBindingCondition{},
+		UnbindStatus: v1beta1.ServiceBindingUnbindStatusNotRequired,
+	}
+
+	_, err := c.serviceCatalogClient.ServiceBindings(updated.Namespace).UpdateStatus(updated)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
 // recordStartOfServiceBindingOperation updates the binding to indicate
 // that there is a current operation being performed. The Status of the binding
 // is recorded in the registry.

pkg/controller/controller_binding_test.go

@@ -46,6 +46,49 @@ import (
 	clientgotesting "k8s.io/client-go/testing"
 )
 
+// TestReconcileServiceBindingNotInitializedStatus tests reconcileBinding to ensure that
+// binding Status will be initialized when it's empty.
+func TestReconcileServiceBindingNotInitializedStatus(t *testing.T) {
+	_, fakeServiceCatalogClient, fakeClusterServiceBrokerClient, testController, _ := newTestController(t, noFakeActions())
+
+	binding := &v1beta1.ServiceBinding{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       testServiceBindingName,
+			Namespace:  testNamespace,
+			Generation: 1,
+		},
+		Spec: v1beta1.ServiceBindingSpec{
+			InstanceRef: v1beta1.LocalObjectReference{Name: "test"},
+		},
+		Status: v1beta1.ServiceBindingStatus{},
+	}
+
+	expectedStatus := v1beta1.ServiceBindingStatus{
+		Conditions:   []v1beta1.ServiceBindingCondition{},
+		UnbindStatus: v1beta1.ServiceBindingUnbindStatusNotRequired,
+	}
+
+	err := reconcileServiceBinding(t, testController, binding)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	brokerActions := fakeClusterServiceBrokerClient.Actions()
+	assertNumberOfBrokerActions(t, brokerActions, 0)
+
+	actions := fakeServiceCatalogClient.Actions()
+	assertNumberOfActions(t, actions, 1)
+
+	updatedObjBinding := assertUpdateStatus(t, actions[0], binding)
+	updatedBinding, ok := updatedObjBinding.(*v1beta1.ServiceBinding)
+	if !ok {
+		t.Fatalf("cast error: want: *v1beta1.ServiceBinding, got: %T", updatedObjBinding)
+	}
+	if !reflect.DeepEqual(updatedBinding.Status, expectedStatus) {
+		t.Errorf("unexpected diff: %v", diff.ObjectReflectDiff(updatedBinding.Status, expectedStatus))
+	}
+}
+
 // TestReconcileBindingNonExistingInstance tests reconcileBinding to ensure a
 // binding fails as expected when an instance to bind to doesn't exist.
 func TestReconcileServiceBindingNonExistingServiceInstance(t *testing.T) {

pkg/controller/controller_instance.go

@@ -20,27 +20,27 @@ import (
 	stderrors "errors"
 	"fmt"
 	"net/url"
+	"reflect"
 	"sync"
 	"time"
 
 	osb "github.com/kubernetes-sigs/go-open-service-broker-client/v2"
-	"k8s.io/klog"
+	"github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1"
+	scfeatures "github.com/kubernetes-sigs/service-catalog/pkg/features"
+	"github.com/kubernetes-sigs/service-catalog/pkg/pretty"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/wait"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/util/workqueue"
-
-	"github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1"
-	scfeatures "github.com/kubernetes-sigs/service-catalog/pkg/features"
-	"github.com/kubernetes-sigs/service-catalog/pkg/pretty"
-	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/klog"
 )
 
 const (
@@ -504,6 +504,15 @@ func (c *controller) removeInstanceFromRetryMap(instance *v1beta1.ServiceInstanc
 func (c *controller) reconcileServiceInstanceAdd(instance *v1beta1.ServiceInstance) error {
 	pcb := pretty.NewInstanceContextBuilder(instance)
 
+	if !c.isServiceInstanceStatusInitialized(instance) {
+		klog.V(4).Info(pcb.Message("Initialize Status entry"))
+		if err := c.initializeServiceInstanceStatus(instance); err != nil {
+			klog.Errorf(pcb.Messagef("Error initializing status: %v", err))
+			return err
+		}
+		return nil
+	}
+
 	if isServiceInstanceProcessedAlready(instance) {
 		klog.V(4).Info(pcb.Message("Not processing event because status showed there is no work to do"))
 		return nil
@@ -1933,6 +1942,34 @@ func (c *controller) updateServiceInstanceCondition(
 	return updatedInstance, err
 }
 
+func (c *controller) isServiceInstanceStatusInitialized(instance *v1beta1.ServiceInstance) bool {
+	emptyStatus := v1beta1.ServiceInstanceStatus{}
+	if reflect.DeepEqual(instance.Status, emptyStatus) {
+		return false
+	}
+
+	return true
+}
+
+// initializeServiceInstanceStatus initialize the ServiceInstanceStatus.
+// In normal scenario it should be done when client is creating the ServiceInstance,
+// but right now we cannot modify the Status (sub-resource) in webhook on CREATE action.
+// As a temporary solution we are doing that in the reconcile function.
+func (c *controller) initializeServiceInstanceStatus(instance *v1beta1.ServiceInstance) error {
+	updated := instance.DeepCopy()
+	updated.Status = v1beta1.ServiceInstanceStatus{
+		Conditions:        []v1beta1.ServiceInstanceCondition{},
+		DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusNotRequired,
+	}
+
+	_, err := c.serviceCatalogClient.ServiceInstances(updated.Namespace).UpdateStatus(updated)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
 // prepareObservedGeneration sets the instance's observed generation
 // and clears the conditions, preparing it for any status updates that can occur
 // during the further processing.

pkg/controller/controller_instance_test.go

@@ -51,6 +51,52 @@ const (
 	lastOperationDescription = "testdescr"
 )
 
+// TestReconcileServiceInstanceNotInitializedStatus tests reconcileInstance to ensure that
+// instance Status will be initialized when it's empty.
+func TestReconcileServiceInstanceNotInitializedStatus(t *testing.T) {
+	_, fakeServiceCatalogClient, fakeClusterServiceBrokerClient, testController, _ := newTestController(t, noFakeActions())
+
+	instance := &v1beta1.ServiceInstance{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       testServiceInstanceName,
+			Generation: 1,
+		},
+		Spec: v1beta1.ServiceInstanceSpec{
+			PlanReference: v1beta1.PlanReference{
+				ClusterServiceClassExternalName: "test",
+				ClusterServicePlanExternalName:  "test",
+			},
+			ExternalID: testServiceInstanceGUID,
+		},
+		Status: v1beta1.ServiceInstanceStatus{},
+	}
+
+	expectedStatus := v1beta1.ServiceInstanceStatus{
+		Conditions:        []v1beta1.ServiceInstanceCondition{},
+		DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusNotRequired,
+	}
+
+	err := reconcileServiceInstance(t, testController, instance)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	brokerActions := fakeClusterServiceBrokerClient.Actions()
+	assertNumberOfBrokerActions(t, brokerActions, 0)
+
+	actions := fakeServiceCatalogClient.Actions()
+	assertNumberOfActions(t, actions, 1)
+
+	updatedObjInstance := assertUpdateStatus(t, actions[0], instance)
+	updatedInstance, ok := updatedObjInstance.(*v1beta1.ServiceInstance)
+	if !ok {
+		t.Fatalf("cast error: want: *v1beta1.ServiceInstance, got: %T", updatedObjInstance)
+	}
+	if !reflect.DeepEqual(updatedInstance.Status, expectedStatus) {
+		t.Errorf("unexpected diff: %v", diff.ObjectReflectDiff(updatedInstance.Status, expectedStatus))
+	}
+}
+
 // TestReconcileServiceInstanceNonExistentClusterServiceClass tests that reconcileInstance gets a failure when
 // the specified service class is not found
 func TestReconcileServiceInstanceNonExistentClusterServiceClass(t *testing.T) {
@@ -68,6 +114,10 @@ func TestReconcileServiceInstanceNonExistentClusterServiceClass(t *testing.T) {
 			},
 			ExternalID: testServiceInstanceGUID,
 		},
+		Status: v1beta1.ServiceInstanceStatus{
+			Conditions: []v1beta1.ServiceInstanceCondition{},
+			DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusNotRequired,
+		},
 	}
 
 	if err := reconcileServiceInstance(t, testController, instance); err == nil {
@@ -118,6 +168,10 @@ func TestReconcileServiceInstanceNonExistentClusterServiceClassWithK8SName(t *te
 			},
 			ExternalID: testServiceInstanceGUID,
 		},
+		Status: v1beta1.ServiceInstanceStatus{
+			Conditions: []v1beta1.ServiceInstanceCondition{},
+			DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusNotRequired,
+		},
 	}
 
 	if err := reconcileServiceInstance(t, testController, instance); err == nil {
@@ -247,6 +301,10 @@ func TestReconcileServiceInstanceNonExistentClusterServicePlan(t *testing.T) {
 			},
 			ExternalID: testServiceInstanceGUID,
 		},
+		Status: v1beta1.ServiceInstanceStatus{
+			Conditions: []v1beta1.ServiceInstanceCondition{},
+			DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusNotRequired,
+		},
 	}
 
 	if err := reconcileServiceInstance(t, testController, instance); err == nil {
@@ -309,6 +367,10 @@ func TestReconcileServiceInstanceNonExistentClusterServicePlanK8SName(t *testing
 			},
 			ExternalID: testServiceInstanceGUID,
 		},
+		Status: v1beta1.ServiceInstanceStatus{
+			Conditions: []v1beta1.ServiceInstanceCondition{},
+			DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusNotRequired,
+		},
 	}
 
 	if err := reconcileServiceInstance(t, testController, instance); err == nil {

pkg/controller/controller_test.go

@@ -877,6 +877,10 @@ func getTestServiceInstanceK8SNames() *v1beta1.ServiceInstance {
 			},
 			ExternalID: testServiceInstanceGUID,
 		},
+		Status: v1beta1.ServiceInstanceStatus{
+			Conditions: []v1beta1.ServiceInstanceCondition{},
+			DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusNotRequired,
+		},
 	}
 }
 

pkg/webhook/servicecatalog/servicebinding/mutation/handler.go

@@ -97,12 +97,6 @@ func (h *CreateUpdateHandler) mutateOnCreate(ctx context.Context, req admission.
 	if utilfeature.DefaultFeatureGate.Enabled(scfeatures.OriginatingIdentity) {
 		setServiceBindingUserInfo(req, binding)
 	}
-
-	// TODO: cannot be modified on webhook side, need to moved directly to controller
-	//binding.Status = sc.ServiceBindingStatus{
-	//	Conditions: []sc.ServiceBindingCondition{},
-	//	UnbindStatus: sc.ServiceBindingUnbindStatusNotRequired,
-	//}
 }
 
 func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, obj *sc.ServiceBinding) {

pkg/webhook/servicecatalog/serviceinstance/mutation/handler.go

@@ -93,12 +93,6 @@ func (h *CreateUpdateHandler) mutateOnCreate(ctx context.Context, req admission.
 	if utilfeature.DefaultFeatureGate.Enabled(scfeatures.OriginatingIdentity) {
 		setServiceInstanceUserInfo(req, instance)
 	}
-
-	// TODO: cannot be modified on webhook side, need to moved directly to controller
-	//instance.Status = sc.ServiceInstanceStatus{
-	//	Conditions:        []sc.ServiceInstanceCondition{},
-	//	DeprovisionStatus: sc.ServiceInstanceDeprovisionStatusNotRequired,
-	//}
 }
 
 func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, obj *sc.ServiceInstance) {