Add checksum to Binding

pkg/apis/servicecatalog/checksum/checksum_test.go

@@ -19,27 +19,46 @@ package checksum
 import (
 	"testing"
 
-	"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog"
-	_ "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/install"
-	"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1"
+	"k8s.io/kubernetes/pkg/api/v1"
 
+	"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog"
 	"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/checksum/unversioned"
 	checksumv1alpha1 "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/checksum/versioned/v1alpha1"
+	_ "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/install"
+	"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1"
 )
 
-func TestInstanceChecksum(t *testing.T) {
-	instanceSpec := servicecatalog.InstanceSpec{
+func TestInstanceSpecChecksum(t *testing.T) {
+	spec := servicecatalog.InstanceSpec{
 		ServiceClassName: "blorb",
 		PlanName:         "plumbus",
-		OSBGUID:          "138177saf87)87fs08f7ASfAS*7",
+		OSBGUID:          "ea6d2fc8-0bb8-11e7-af5d-0242ac110005",
+	}
+
+	unversionedChecksum := unversioned.InstanceSpecChecksum(spec)
+
+	versionedSpec := v1alpha1.InstanceSpec{}
+	v1alpha1.Convert_servicecatalog_InstanceSpec_To_v1alpha1_InstanceSpec(&spec, &versionedSpec, nil /* conversionScope */)
+	versionedChecksum := checksumv1alpha1.InstanceSpecChecksum(versionedSpec)
+
+	if e, a := unversionedChecksum, versionedChecksum; e != a {
+		t.Fatalf("versioned and unversioned checksums should match; expected %v, got %v", e, a)
 	}
+}
 
-	unversionedChecksum := unversioned.InstanceSpecChecksum(instanceSpec)
+// TestBindingChecksum tests that an internal and v1alpha1 checksum of the same object are equivalent
+func TestBindingSpecChecksum(t *testing.T) {
+	spec := servicecatalog.BindingSpec{
+		InstanceRef: v1.LocalObjectReference{Name: "test-instance"},
+		SecretName:  "test-secret",
+		OSBGUID:     "1995a7e6-d078-4ce6-9057-bcefd793634e",
+	}
 
-	versionedInstanceSpec := v1alpha1.InstanceSpec{}
-	v1alpha1.Convert_servicecatalog_InstanceSpec_To_v1alpha1_InstanceSpec(&instanceSpec, &versionedInstanceSpec, nil /* conversionScope */)
+	unversionedChecksum := unversioned.BindingSpecChecksum(spec)
 
-	versionedChecksum := checksumv1alpha1.InstanceSpecChecksum(versionedInstanceSpec)
+	versionedSpec := v1alpha1.BindingSpec{}
+	v1alpha1.Convert_servicecatalog_BindingSpec_To_v1alpha1_BindingSpec(&spec, &versionedSpec, nil /* conversionScope */)
+	versionedChecksum := checksumv1alpha1.BindingSpecChecksum(versionedSpec)
 
 	if e, a := unversionedChecksum, versionedChecksum; e != a {
 		t.Fatalf("versioned and unversioned checksums should match; expected %v, got %v", e, a)

pkg/apis/servicecatalog/checksum/unversioned/checksum.go

@@ -37,3 +37,17 @@ func InstanceSpecChecksum(spec servicecatalog.InstanceSpec) string {
 	sum := sha256.Sum256([]byte(specString))
 	return fmt.Sprintf("%x", sum)
 }
+
+func BindingSpecChecksum(spec servicecatalog.BindingSpec) string {
+	specString := ""
+	specString += fmt.Sprintf("instanceRef: %v\n", spec.InstanceRef.Name)
+
+	if spec.Parameters != nil {
+		specString += fmt.Sprintf("parameters:\n\n%v\n\n", string(spec.Parameters.Raw))
+	}
+
+	specString += fmt.Sprintf("osbGuid: %v\n", spec.OSBGUID)
+
+	sum := sha256.Sum256([]byte(specString))
+	return fmt.Sprintf("%x", sum)
+}

pkg/apis/servicecatalog/checksum/versioned/v1alpha1/checksum.go

@@ -37,3 +37,17 @@ func InstanceSpecChecksum(spec v1alpha1.InstanceSpec) string {
 	sum := sha256.Sum256([]byte(specString))
 	return fmt.Sprintf("%x", sum)
 }
+
+func BindingSpecChecksum(spec v1alpha1.BindingSpec) string {
+	specString := ""
+	specString += fmt.Sprintf("instanceRef: %v\n", spec.InstanceRef.Name)
+
+	if spec.Parameters != nil {
+		specString += fmt.Sprintf("parameters:\n\n%v\n\n", string(spec.Parameters.Raw))
+	}
+
+	specString += fmt.Sprintf("osbGuid: %v\n", spec.OSBGUID)
+
+	sum := sha256.Sum256([]byte(specString))
+	return fmt.Sprintf("%x", sum)
+}

pkg/apis/servicecatalog/types.go

@@ -254,6 +254,10 @@ type BindingSpec struct {
 	// OSBGUID is the identity of this object for use with the OSB API.
 	// Immutable.
 	OSBGUID string
+
+	// Checksum is the checksum of the BindingSpec that was last successfully
+	// reconciled against the broker.
+	Checksum *string
 }
 
 // BindingStatus represents the current status of a Binding.

pkg/apis/servicecatalog/v1alpha1/types.go

@@ -266,6 +266,10 @@ type BindingSpec struct {
 	// OSBGUID is the identity of this object for use with the OSB API.
 	// Immutable.
 	OSBGUID string `json:"osbGuid"`
+
+	// Checksum is the checksum of the BindingSpec that was last successfully
+	// reconciled against the broker.
+	Checksum *string
 }
 
 // BindingStatus represents the current status of a Binding.

pkg/apis/servicecatalog/validation/binding.go

@@ -28,16 +28,20 @@ var validateBindingName = apivalidation.NameIsDNSSubdomain
 
 // ValidateBinding validates a Binding and returns a list of errors.
 func ValidateBinding(binding *sc.Binding) field.ErrorList {
+	return internalValidateBinding(binding, true)
+}
+
+func internalValidateBinding(binding *sc.Binding, create bool) field.ErrorList {
 	allErrs := field.ErrorList{}
 	allErrs = append(allErrs, apivalidation.ValidateObjectMeta(&binding.ObjectMeta, true, /*namespace*/
 		validateBindingName,
 		field.NewPath("metadata"))...)
-	allErrs = append(allErrs, validateBindingSpec(&binding.Spec, field.NewPath("Spec"))...)
+	allErrs = append(allErrs, validateBindingSpec(&binding.Spec, field.NewPath("Spec"), true)...)
 
 	return allErrs
 }
 
-func validateBindingSpec(spec *sc.BindingSpec, fldPath *field.Path) field.ErrorList {
+func validateBindingSpec(spec *sc.BindingSpec, fldPath *field.Path, create bool) field.ErrorList {
 	allErrs := field.ErrorList{}
 
 	for _, msg := range validateInstanceName(spec.InstanceRef.Name, false /* prefix */) {
@@ -48,15 +52,16 @@ func validateBindingSpec(spec *sc.BindingSpec, fldPath *field.Path) field.ErrorL
 		allErrs = append(allErrs, field.Invalid(fldPath.Child("secretName"), spec.SecretName, msg))
 	}
 
+	if create && spec.Checksum != nil {
+		allErrs = append(allErrs, field.Forbidden(fldPath.Child("checksum"), "checksum may not be set during creation"))
+	}
+
 	return allErrs
 }
 
 // ValidateBindingUpdate checks that when changing from an older binding to a newer binding is okay.
 func ValidateBindingUpdate(new *sc.Binding, old *sc.Binding) field.ErrorList {
-	allErrs := field.ErrorList{}
-	allErrs = append(allErrs, ValidateBinding(new)...)
-	allErrs = append(allErrs, ValidateBinding(old)...)
-	return allErrs
+	return internalValidateBinding(new, false)
 }
 
 // ValidateBindingStatusUpdate checks that when changing from an older binding to a newer binding is okay.

pkg/apis/servicecatalog/validation/binding_test.go

@@ -47,6 +47,26 @@ func TestValidateBinding(t *testing.T) {
 			},
 			valid: true,
 		},
+		{
+			name: "checksum set on create",
+			binding: &servicecatalog.Binding{
+				ObjectMeta: kapi.ObjectMeta{
+					Name:      "test-binding",
+					Namespace: "test-ns",
+				},
+				Spec: servicecatalog.BindingSpec{
+					InstanceRef: v1.LocalObjectReference{
+						Name: "test-instance",
+					},
+					SecretName: "test-secret",
+					Checksum: func() *string {
+						s := "boo"
+						return &s
+					}(),
+				},
+			},
+			valid: false,
+		},
 		{
 			name: "missing namespace",
 			binding: &servicecatalog.Binding{

pkg/apis/servicecatalog/validation/instance.go

@@ -66,7 +66,7 @@ func validateInstanceSpec(spec *sc.InstanceSpec, fldPath *field.Path, create boo
 	return allErrs
 }
 
-// ValidateInstanceUpdate validates a change to the broker's spec.
+// ValidateInstanceUpdate validates a change to the Instance's spec.
 func ValidateInstanceUpdate(new *sc.Instance, old *sc.Instance) field.ErrorList {
 	return internalValidateInstance(new, false)
 }

pkg/controller/controller.go

@@ -664,6 +664,21 @@ func (c *controller) bindingUpdate(oldObj, newObj interface{}) {
 }
 
 func (c *controller) reconcileBinding(binding *v1alpha1.Binding) {
+	// Determine whether the checksum has been invalidated by a change to the
+	// object.  If the binding's checksum matches the calculated checksum,
+	// there is no work to do.
+	//
+	// We only do this if the deletion timestamp is nil, because the deletion
+	// timestamp changes the object's state in a way that we must reconcile,
+	// but does not affect the checksum.
+	if binding.Spec.Checksum != nil && binding.DeletionTimestamp == nil {
+		bindingChecksum := checksum.BindingSpecChecksum(binding.Spec)
+		if bindingChecksum == *binding.Spec.Checksum {
+			glog.V(4).Infof("Not processing event for Binding %v/%v because checksum showed there is no work to do", binding.Namespace, binding.Name)
+			return
+		}
+	}
+
 	glog.V(4).Infof("Processing Binding %v/%v", binding.Namespace, binding.Name)
 
 	instance, err := c.instanceLister.Instances(binding.Namespace).Get(binding.Spec.InstanceRef.Name)

pkg/registry/servicecatalog/binding/strategy.go

@@ -26,6 +26,7 @@ import (
 
 	"github.com/golang/glog"
 	sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog"
+	checksum "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/checksum/unversioned"
 	scv "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/validation"
 )
 
@@ -147,6 +148,25 @@ func (bindingStatusRESTStrategy) PrepareForUpdate(ctx kapi.Context, new, old run
 	}
 	// status changes are not allowed to update spec
 	newBinding.Spec = oldBinding.Spec
+
+	// TODO: unit test
+
+	foundReadyConditionTrue := false
+	for _, condition := range newBinding.Status.Conditions {
+		if condition.Type == sc.BindingConditionReady && condition.Status == sc.ConditionTrue {
+			foundReadyConditionTrue = true
+			break
+		}
+	}
+
+	if foundReadyConditionTrue {
+		glog.Infof("Found true ready condition for Binding %v/%v; updating checksum", newBinding.Namespace, newBinding.Name)
+		// This status update has a true ready condition; update the checksum if necessary
+		newBinding.Spec.Checksum = func() *string {
+			s := checksum.BindingSpecChecksum(newBinding.Spec)
+			return &s
+		}()
+	}
 }
 
 func (bindingStatusRESTStrategy) ValidateUpdate(ctx kapi.Context, new, old runtime.Object) field.ErrorList {

pkg/registry/servicecatalog/instance/strategy.go

@@ -150,6 +150,8 @@ func (instanceStatusRESTStrategy) PrepareForUpdate(ctx kapi.Context, new, old ru
 	// status changes are not allowed to update spec
 	newInstance.Spec = oldInstance.Spec
 
+	// TODO: unit test
+
 	foundReadyConditionTrue := false
 	for _, condition := range newInstance.Status.Conditions {
 		if condition.Type == sc.InstanceConditionReady && condition.Status == sc.ConditionTrue {

test/integration/clientset_test.go

@@ -533,6 +533,9 @@ func testInstanceClient(client servicecatalogclient.Interface, name string) erro
 	if e, a := readyConditionTrue, instanceServer.Status.Conditions[0]; !reflect.DeepEqual(e, a) {
 		return fmt.Errorf("Didn't get matching ready conditions:\nexpected: %v\n\ngot: %v", e, a)
 	}
+	if instanceServer.Spec.Checksum == nil {
+		return fmt.Error("Checksum should have been set after updating ready condition to true")
+	}
 
 	err = instanceClient.Delete(name, &v1.DeleteOptions{})
 	if nil != err {
@@ -692,6 +695,9 @@ func testBindingClient(client servicecatalogclient.Interface, name string) error
 	if e, a := readyConditionTrue, bindingServer.Status.Conditions[0]; !reflect.DeepEqual(e, a) {
 		return fmt.Errorf("Didn't get matching ready conditions:\nexpected: %v\n\ngot: %v", e, a)
 	}
+	if bindingServer.Spec.Checksum == nil {
+		return fmt.Error("Checksum should have been set after updating ready condition to true")
+	}
 
 	if err = bindingClient.Delete(name, &v1.DeleteOptions{}); nil != err {
 		return fmt.Errorf("broker should be deleted (%v)", err)