From 7b17f40495fb02f72d0b05eba1eb8e4528066952 Mon Sep 17 00:00:00 2001 From: staebler Date: Tue, 22 Aug 2017 15:10:21 -0400 Subject: [PATCH 1/4] Use generation instead of checksum for bindings and instances --- .../servicecatalog/checksum/checksum_test.go | 67 --------- .../checksum/unversioned/checksum.go | 90 ------------ .../checksum/versioned/v1alpha1/checksum.go | 90 ------------ pkg/apis/servicecatalog/types.go | 12 +- .../v1alpha1/types.generated.go | 130 ++++++------------ pkg/apis/servicecatalog/v1alpha1/types.go | 12 +- .../v1alpha1/zz_generated.conversion.go | 8 +- .../v1alpha1/zz_generated.deepcopy.go | 10 -- .../servicecatalog/zz_generated.deepcopy.go | 10 -- pkg/controller/controller_binding.go | 21 +-- pkg/controller/controller_binding_test.go | 4 - pkg/controller/controller_instance.go | 31 +++-- pkg/controller/controller_instance_test.go | 16 +-- pkg/openapi/openapi_generated.go | 20 +-- .../servicecatalog/binding/strategy.go | 39 ++---- .../servicecatalog/binding/strategy_test.go | 99 +++++-------- .../servicecatalog/instance/strategy.go | 39 ++---- .../servicecatalog/instance/strategy_test.go | 96 +++++-------- test/integration/clientset_test.go | 6 - 19 files changed, 204 insertions(+), 596 deletions(-) delete mode 100644 pkg/apis/servicecatalog/checksum/checksum_test.go delete mode 100644 pkg/apis/servicecatalog/checksum/unversioned/checksum.go delete mode 100644 pkg/apis/servicecatalog/checksum/versioned/v1alpha1/checksum.go diff --git a/pkg/apis/servicecatalog/checksum/checksum_test.go b/pkg/apis/servicecatalog/checksum/checksum_test.go deleted file mode 100644 index 6d6c0abd8a3..00000000000 --- a/pkg/apis/servicecatalog/checksum/checksum_test.go +++ /dev/null @@ -1,67 +0,0 @@ -/* -Copyright 2017 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package checksum - -import ( - "testing" - - "k8s.io/client-go/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 TestServiceInstanceSpecChecksum(t *testing.T) { - spec := servicecatalog.ServiceInstanceSpec{ - ServiceClassName: "blorb", - PlanName: "plumbus", - ExternalID: "ea6d2fc8-0bb8-11e7-af5d-0242ac110005", - } - - unversionedChecksum := unversioned.ServiceInstanceSpecChecksum(spec) - - versionedSpec := v1alpha1.ServiceInstanceSpec{} - v1alpha1.Convert_servicecatalog_ServiceInstanceSpec_To_v1alpha1_ServiceInstanceSpec(&spec, &versionedSpec, nil /* conversionScope */) - versionedChecksum := checksumv1alpha1.ServiceInstanceSpecChecksum(versionedSpec) - - if e, a := unversionedChecksum, versionedChecksum; e != a { - t.Fatalf("versioned and unversioned checksums should match; expected %v, got %v", e, a) - } -} - -// TestServiceInstanceCredentialChecksum tests that an internal and v1alpha1 checksum of the same object are equivalent -func TestServiceInstanceCredentialSpecChecksum(t *testing.T) { - spec := servicecatalog.ServiceInstanceCredentialSpec{ - ServiceInstanceRef: v1.LocalObjectReference{Name: "test-instance"}, - SecretName: "test-secret", - ExternalID: "1995a7e6-d078-4ce6-9057-bcefd793634e", - } - - unversionedChecksum := unversioned.ServiceInstanceCredentialSpecChecksum(spec) - - versionedSpec := v1alpha1.ServiceInstanceCredentialSpec{} - v1alpha1.Convert_servicecatalog_ServiceInstanceCredentialSpec_To_v1alpha1_ServiceInstanceCredentialSpec(&spec, &versionedSpec, nil /* conversionScope */) - versionedChecksum := checksumv1alpha1.ServiceInstanceCredentialSpecChecksum(versionedSpec) - - if e, a := unversionedChecksum, versionedChecksum; e != a { - t.Fatalf("versioned and unversioned checksums should match; expected %v, got %v", e, a) - } -} - diff --git a/pkg/apis/servicecatalog/checksum/unversioned/checksum.go b/pkg/apis/servicecatalog/checksum/unversioned/checksum.go deleted file mode 100644 index 3233eb1d2ee..00000000000 --- a/pkg/apis/servicecatalog/checksum/unversioned/checksum.go +++ /dev/null @@ -1,90 +0,0 @@ -/* -Copyright 2017 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package unversioned - -import ( - "crypto/sha256" - "fmt" - - "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog" -) - -// ServiceInstanceSpecChecksum calculates a checksum of the given InstanceSpec based -// on the following fields; -// -// - ServiceClassName -// - PlanName -// - Parameters -// - ParametersFrom -// - ExternalID -func ServiceInstanceSpecChecksum(spec servicecatalog.ServiceInstanceSpec) string { - specString := "" - specString += fmt.Sprintf("serviceClassName: %v\n", spec.ServiceClassName) - specString += fmt.Sprintf("planName: %v\n", spec.PlanName) - - if spec.Parameters != nil { - specString += fmt.Sprintf("parameters:\n\n%v\n\n", string(spec.Parameters.Raw)) - } - if spec.ParametersFrom != nil { - specString += "parametersFrom: \n" - for _, p := range spec.ParametersFrom { - specString += fmt.Sprintf("%v\n", parametersFromChecksum(p)) - } - } - - specString += fmt.Sprintf("externalID: %v\n", spec.ExternalID) - - sum := sha256.Sum256([]byte(specString)) - return fmt.Sprintf("%x", sum) -} - -// ServiceInstanceCredentialSpecChecksum calculates a checksum of the given ServiceInstanceCredentialSpec based on -// the following fields: -// -// - ServiceInstanceRef.Name -// - Parameters -// - ExternalID -func ServiceInstanceCredentialSpecChecksum(spec servicecatalog.ServiceInstanceCredentialSpec) string { - specString := "" - specString += fmt.Sprintf("instanceRef: %v\n", spec.ServiceInstanceRef.Name) - - if spec.Parameters != nil { - specString += fmt.Sprintf("parameters:\n\n%v\n\n", string(spec.Parameters.Raw)) - } - if spec.ParametersFrom != nil { - specString += "parametersFrom: \n" - for _, p := range spec.ParametersFrom { - specString += fmt.Sprintf("%v\n", parametersFromChecksum(p)) - } - } - - specString += fmt.Sprintf("externalID: %v\n", spec.ExternalID) - - sum := sha256.Sum256([]byte(specString)) - return fmt.Sprintf("%x", sum) -} - -func parametersFromChecksum(parameters servicecatalog.ParametersFromSource) string { - specString := "" - - if parameters.SecretKeyRef != nil { - specString += fmt.Sprintf("secretKeyRef: %v[%v]\n", parameters.SecretKeyRef.Name, parameters.SecretKeyRef.Key) - } - - sum := sha256.Sum256([]byte(specString)) - return fmt.Sprintf("%x", sum) -} diff --git a/pkg/apis/servicecatalog/checksum/versioned/v1alpha1/checksum.go b/pkg/apis/servicecatalog/checksum/versioned/v1alpha1/checksum.go deleted file mode 100644 index e78beecd530..00000000000 --- a/pkg/apis/servicecatalog/checksum/versioned/v1alpha1/checksum.go +++ /dev/null @@ -1,90 +0,0 @@ -/* -Copyright 2017 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package unversioned - -import ( - "crypto/sha256" - "fmt" - - "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1" -) - -// ServiceInstanceSpecChecksum calculates a checksum of the given InstanceSpec based -// on the following fields; -// -// - ServiceClassName -// - PlanName -// - Parameters -// - ParametersFrom -// - ExternalID -func ServiceInstanceSpecChecksum(spec v1alpha1.ServiceInstanceSpec) string { - specString := "" - specString += fmt.Sprintf("serviceClassName: %v\n", spec.ServiceClassName) - specString += fmt.Sprintf("planName: %v\n", spec.PlanName) - - if spec.Parameters != nil { - specString += fmt.Sprintf("parameters:\n\n%v\n\n", string(spec.Parameters.Raw)) - } - if spec.ParametersFrom != nil { - specString += "parametersFrom: \n" - for _, p := range spec.ParametersFrom { - specString += fmt.Sprintf("%v\n", parametersFromChecksum(p)) - } - } - - specString += fmt.Sprintf("externalID: %v\n", spec.ExternalID) - - sum := sha256.Sum256([]byte(specString)) - return fmt.Sprintf("%x", sum) -} - -// ServiceInstanceCredentialSpecChecksum calculates a checksum of the given ServiceInstanceCredentialSpec based on -// the following fields: -// -// - ServiceInstanceRef.Name -// - Parameters -// - ExternalID -func ServiceInstanceCredentialSpecChecksum(spec v1alpha1.ServiceInstanceCredentialSpec) string { - specString := "" - specString += fmt.Sprintf("instanceRef: %v\n", spec.ServiceInstanceRef.Name) - - if spec.Parameters != nil { - specString += fmt.Sprintf("parameters:\n\n%v\n\n", string(spec.Parameters.Raw)) - } - if spec.ParametersFrom != nil { - specString += "parametersFrom: \n" - for _, p := range spec.ParametersFrom { - specString += fmt.Sprintf("%v\n", parametersFromChecksum(p)) - } - } - - specString += fmt.Sprintf("externalID: %v\n", spec.ExternalID) - - sum := sha256.Sum256([]byte(specString)) - return fmt.Sprintf("%x", sum) -} - -func parametersFromChecksum(parameters v1alpha1.ParametersFromSource) string { - specString := "" - - if parameters.SecretKeyRef != nil { - specString += fmt.Sprintf("secretKeyRef: %v[%v]\n", parameters.SecretKeyRef.Name, parameters.SecretKeyRef.Key) - } - - sum := sha256.Sum256([]byte(specString)) - return fmt.Sprintf("%x", sum) -} diff --git a/pkg/apis/servicecatalog/types.go b/pkg/apis/servicecatalog/types.go index 4c789dd10f2..a9f7906adc9 100644 --- a/pkg/apis/servicecatalog/types.go +++ b/pkg/apis/servicecatalog/types.go @@ -357,9 +357,9 @@ type ServiceInstanceStatus struct { // the service instance. DashboardURL *string - // Checksum is the checksum of the ServiceInstanceSpec that was last successfully - // reconciled against the broker. - Checksum *string + // ReconciledGeneration is the generation of the instance that was last + // successfully reconciled. + ReconciledGeneration int64 } // ServiceInstanceCondition contains condition information about an Instance. @@ -454,9 +454,9 @@ type ServiceInstanceCredentialSpec struct { type ServiceInstanceCredentialStatus struct { Conditions []ServiceInstanceCredentialCondition - // Checksum is the checksum of the ServiceInstanceCredentialSpec that was last successfully - // reconciled against the broker. - Checksum *string + // ReconciledGeneration is the generation of the ServiceInstanceCredential that was last + // successfully reconciled. + ReconciledGeneration int64 } // ServiceInstanceCredentialCondition condition information for a ServiceInstanceCredential. diff --git a/pkg/apis/servicecatalog/v1alpha1/types.generated.go b/pkg/apis/servicecatalog/v1alpha1/types.generated.go index dc3613c5fa7..940b15f6a8e 100644 --- a/pkg/apis/servicecatalog/v1alpha1/types.generated.go +++ b/pkg/apis/servicecatalog/v1alpha1/types.generated.go @@ -5655,12 +5655,11 @@ func (x *ServiceInstanceStatus) CodecEncodeSelf(e *codec1978.Encoder) { const yyr2 bool = false yyq2[2] = x.LastOperation != nil yyq2[3] = x.DashboardURL != nil - yyq2[4] = x.Checksum != nil var yynn2 int if yyr2 || yy2arr2 { r.EncodeArrayStart(5) } else { - yynn2 = 2 + yynn2 = 3 for _, b := range yyq2 { if b { yynn2++ @@ -5787,37 +5786,21 @@ func (x *ServiceInstanceStatus) CodecEncodeSelf(e *codec1978.Encoder) { } if yyr2 || yy2arr2 { z.EncSendContainerState(codecSelfer_containerArrayElem1234) - if yyq2[4] { - if x.Checksum == nil { - r.EncodeNil() - } else { - yy20 := *x.Checksum - yym21 := z.EncBinary() - _ = yym21 - if false { - } else { - r.EncodeString(codecSelferC_UTF81234, string(yy20)) - } - } + yym20 := z.EncBinary() + _ = yym20 + if false { } else { - r.EncodeNil() + r.EncodeInt(int64(x.ReconciledGeneration)) } } else { - if yyq2[4] { - z.EncSendContainerState(codecSelfer_containerMapKey1234) - r.EncodeString(codecSelferC_UTF81234, string("checksum")) - z.EncSendContainerState(codecSelfer_containerMapValue1234) - if x.Checksum == nil { - r.EncodeNil() - } else { - yy22 := *x.Checksum - yym23 := z.EncBinary() - _ = yym23 - if false { - } else { - r.EncodeString(codecSelferC_UTF81234, string(yy22)) - } - } + z.EncSendContainerState(codecSelfer_containerMapKey1234) + r.EncodeString(codecSelferC_UTF81234, string("ReconciledGeneration")) + z.EncSendContainerState(codecSelfer_containerMapValue1234) + yym21 := z.EncBinary() + _ = yym21 + if false { + } else { + r.EncodeInt(int64(x.ReconciledGeneration)) } } if yyr2 || yy2arr2 { @@ -5937,20 +5920,16 @@ func (x *ServiceInstanceStatus) codecDecodeSelfFromMap(l int, d *codec1978.Decod *((*string)(x.DashboardURL)) = r.DecodeString() } } - case "checksum": + case "ReconciledGeneration": if r.TryDecodeAsNil() { - if x.Checksum != nil { - x.Checksum = nil - } + x.ReconciledGeneration = 0 } else { - if x.Checksum == nil { - x.Checksum = new(string) - } + yyv12 := &x.ReconciledGeneration yym13 := z.DecBinary() _ = yym13 if false { } else { - *((*string)(x.Checksum)) = r.DecodeString() + *((*int64)(yyv12)) = int64(r.DecodeInt(64)) } } default: @@ -6075,18 +6054,14 @@ func (x *ServiceInstanceStatus) codecDecodeSelfFromArray(l int, d *codec1978.Dec } z.DecSendContainerState(codecSelfer_containerArrayElem1234) if r.TryDecodeAsNil() { - if x.Checksum != nil { - x.Checksum = nil - } + x.ReconciledGeneration = 0 } else { - if x.Checksum == nil { - x.Checksum = new(string) - } + yyv23 := &x.ReconciledGeneration yym24 := z.DecBinary() _ = yym24 if false { } else { - *((*string)(x.Checksum)) = r.DecodeString() + *((*int64)(yyv23)) = int64(r.DecodeInt(64)) } } for { @@ -7686,12 +7661,11 @@ func (x *ServiceInstanceCredentialStatus) CodecEncodeSelf(e *codec1978.Encoder) var yyq2 [2]bool _, _, _ = yysep2, yyq2, yy2arr2 const yyr2 bool = false - yyq2[1] = x.Checksum != nil var yynn2 int if yyr2 || yy2arr2 { r.EncodeArrayStart(2) } else { - yynn2 = 1 + yynn2 = 2 for _, b := range yyq2 { if b { yynn2++ @@ -7729,37 +7703,21 @@ func (x *ServiceInstanceCredentialStatus) CodecEncodeSelf(e *codec1978.Encoder) } if yyr2 || yy2arr2 { z.EncSendContainerState(codecSelfer_containerArrayElem1234) - if yyq2[1] { - if x.Checksum == nil { - r.EncodeNil() - } else { - yy7 := *x.Checksum - yym8 := z.EncBinary() - _ = yym8 - if false { - } else { - r.EncodeString(codecSelferC_UTF81234, string(yy7)) - } - } + yym7 := z.EncBinary() + _ = yym7 + if false { } else { - r.EncodeNil() + r.EncodeInt(int64(x.ReconciledGeneration)) } } else { - if yyq2[1] { - z.EncSendContainerState(codecSelfer_containerMapKey1234) - r.EncodeString(codecSelferC_UTF81234, string("checksum")) - z.EncSendContainerState(codecSelfer_containerMapValue1234) - if x.Checksum == nil { - r.EncodeNil() - } else { - yy9 := *x.Checksum - yym10 := z.EncBinary() - _ = yym10 - if false { - } else { - r.EncodeString(codecSelferC_UTF81234, string(yy9)) - } - } + z.EncSendContainerState(codecSelfer_containerMapKey1234) + r.EncodeString(codecSelferC_UTF81234, string("ReconciledGeneration")) + z.EncSendContainerState(codecSelfer_containerMapValue1234) + yym8 := z.EncBinary() + _ = yym8 + if false { + } else { + r.EncodeInt(int64(x.ReconciledGeneration)) } } if yyr2 || yy2arr2 { @@ -7835,20 +7793,16 @@ func (x *ServiceInstanceCredentialStatus) codecDecodeSelfFromMap(l int, d *codec h.decSliceServiceInstanceCredentialCondition((*[]ServiceInstanceCredentialCondition)(yyv4), d) } } - case "checksum": + case "ReconciledGeneration": if r.TryDecodeAsNil() { - if x.Checksum != nil { - x.Checksum = nil - } + x.ReconciledGeneration = 0 } else { - if x.Checksum == nil { - x.Checksum = new(string) - } + yyv6 := &x.ReconciledGeneration yym7 := z.DecBinary() _ = yym7 if false { } else { - *((*string)(x.Checksum)) = r.DecodeString() + *((*int64)(yyv6)) = int64(r.DecodeInt(64)) } } default: @@ -7899,18 +7853,14 @@ func (x *ServiceInstanceCredentialStatus) codecDecodeSelfFromArray(l int, d *cod } z.DecSendContainerState(codecSelfer_containerArrayElem1234) if r.TryDecodeAsNil() { - if x.Checksum != nil { - x.Checksum = nil - } + x.ReconciledGeneration = 0 } else { - if x.Checksum == nil { - x.Checksum = new(string) - } + yyv11 := &x.ReconciledGeneration yym12 := z.DecBinary() _ = yym12 if false { } else { - *((*string)(x.Checksum)) = r.DecodeString() + *((*int64)(yyv11)) = int64(r.DecodeInt(64)) } } for { diff --git a/pkg/apis/servicecatalog/v1alpha1/types.go b/pkg/apis/servicecatalog/v1alpha1/types.go index fea8ad061c3..2bc963e4ee7 100644 --- a/pkg/apis/servicecatalog/v1alpha1/types.go +++ b/pkg/apis/servicecatalog/v1alpha1/types.go @@ -361,9 +361,9 @@ type ServiceInstanceStatus struct { // the service instance. DashboardURL *string `json:"dashboardURL,omitempty"` - // Checksum is the checksum of the ServiceInstanceSpec that was last successfully - // reconciled against the broker. - Checksum *string `json:"checksum,omitempty"` + // ReconciledGeneration is the generation of the instance that was last + // successfully reconciled. + ReconciledGeneration int64 } // ServiceInstanceCondition contains condition information about an Instance. @@ -458,9 +458,9 @@ type ServiceInstanceCredentialSpec struct { type ServiceInstanceCredentialStatus struct { Conditions []ServiceInstanceCredentialCondition `json:"conditions"` - // Checksum is the checksum of the ServiceInstanceCredentialSpec that was last successfully - // reconciled against the broker. - Checksum *string `json:"checksum,omitempty"` + // ReconciledGeneration is the generation of the ServiceInstanceCredential that was last + // successfully reconciled. + ReconciledGeneration int64 } // ServiceInstanceCredentialCondition condition information for a ServiceInstanceCredential. diff --git a/pkg/apis/servicecatalog/v1alpha1/zz_generated.conversion.go b/pkg/apis/servicecatalog/v1alpha1/zz_generated.conversion.go index 07ab07c0d5e..5f14f407a8b 100644 --- a/pkg/apis/servicecatalog/v1alpha1/zz_generated.conversion.go +++ b/pkg/apis/servicecatalog/v1alpha1/zz_generated.conversion.go @@ -573,7 +573,7 @@ func Convert_servicecatalog_ServiceInstanceCredentialSpec_To_v1alpha1_ServiceIns func autoConvert_v1alpha1_ServiceInstanceCredentialStatus_To_servicecatalog_ServiceInstanceCredentialStatus(in *ServiceInstanceCredentialStatus, out *servicecatalog.ServiceInstanceCredentialStatus, s conversion.Scope) error { out.Conditions = *(*[]servicecatalog.ServiceInstanceCredentialCondition)(unsafe.Pointer(&in.Conditions)) - out.Checksum = (*string)(unsafe.Pointer(in.Checksum)) + out.ReconciledGeneration = in.ReconciledGeneration return nil } @@ -588,7 +588,7 @@ func autoConvert_servicecatalog_ServiceInstanceCredentialStatus_To_v1alpha1_Serv } else { out.Conditions = *(*[]ServiceInstanceCredentialCondition)(unsafe.Pointer(&in.Conditions)) } - out.Checksum = (*string)(unsafe.Pointer(in.Checksum)) + out.ReconciledGeneration = in.ReconciledGeneration return nil } @@ -656,7 +656,7 @@ func autoConvert_v1alpha1_ServiceInstanceStatus_To_servicecatalog_ServiceInstanc out.AsyncOpInProgress = in.AsyncOpInProgress out.LastOperation = (*string)(unsafe.Pointer(in.LastOperation)) out.DashboardURL = (*string)(unsafe.Pointer(in.DashboardURL)) - out.Checksum = (*string)(unsafe.Pointer(in.Checksum)) + out.ReconciledGeneration = in.ReconciledGeneration return nil } @@ -674,7 +674,7 @@ func autoConvert_servicecatalog_ServiceInstanceStatus_To_v1alpha1_ServiceInstanc out.AsyncOpInProgress = in.AsyncOpInProgress out.LastOperation = (*string)(unsafe.Pointer(in.LastOperation)) out.DashboardURL = (*string)(unsafe.Pointer(in.DashboardURL)) - out.Checksum = (*string)(unsafe.Pointer(in.Checksum)) + out.ReconciledGeneration = in.ReconciledGeneration return nil } diff --git a/pkg/apis/servicecatalog/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/servicecatalog/v1alpha1/zz_generated.deepcopy.go index c3616a7b5cd..54b08af3f3d 100644 --- a/pkg/apis/servicecatalog/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/servicecatalog/v1alpha1/zz_generated.deepcopy.go @@ -424,11 +424,6 @@ func DeepCopy_v1alpha1_ServiceInstanceCredentialStatus(in interface{}, out inter } } } - if in.Checksum != nil { - in, out := &in.Checksum, &out.Checksum - *out = new(string) - **out = **in - } return nil } } @@ -504,11 +499,6 @@ func DeepCopy_v1alpha1_ServiceInstanceStatus(in interface{}, out interface{}, c *out = new(string) **out = **in } - if in.Checksum != nil { - in, out := &in.Checksum, &out.Checksum - *out = new(string) - **out = **in - } return nil } } diff --git a/pkg/apis/servicecatalog/zz_generated.deepcopy.go b/pkg/apis/servicecatalog/zz_generated.deepcopy.go index 2d69408800c..d9ecc80739c 100644 --- a/pkg/apis/servicecatalog/zz_generated.deepcopy.go +++ b/pkg/apis/servicecatalog/zz_generated.deepcopy.go @@ -424,11 +424,6 @@ func DeepCopy_servicecatalog_ServiceInstanceCredentialStatus(in interface{}, out } } } - if in.Checksum != nil { - in, out := &in.Checksum, &out.Checksum - *out = new(string) - **out = **in - } return nil } } @@ -504,11 +499,6 @@ func DeepCopy_servicecatalog_ServiceInstanceStatus(in interface{}, out interface *out = new(string) **out = **in } - if in.Checksum != nil { - in, out := &in.Checksum, &out.Checksum - *out = new(string) - **out = **in - } return nil } } diff --git a/pkg/controller/controller_binding.go b/pkg/controller/controller_binding.go index dd2a68d52f5..4d4bdafd812 100644 --- a/pkg/controller/controller_binding.go +++ b/pkg/controller/controller_binding.go @@ -22,7 +22,6 @@ import ( "github.com/golang/glog" osb "github.com/pmorie/go-open-service-broker-client/v2" - checksum "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/checksum/versioned/v1alpha1" "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -104,18 +103,17 @@ func (c *controller) reconcileServiceInstanceCredential(binding *v1alpha1.Servic return err } - // 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. + // Determine whether there is a new generation of the object. If the binding's + // generation does not match the reconciled generation, then there is a new + // generation, indicating that changes have been made to the binding's spec. // // 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.Status.Checksum != nil && binding.DeletionTimestamp == nil { - bindingChecksum := checksum.ServiceInstanceCredentialSpecChecksum(binding.Spec) - if bindingChecksum == *binding.Status.Checksum { + // but does not affect the generation. + if binding.Status.ReconciledGeneration != 0 && binding.DeletionTimestamp == nil { + if binding.Status.ReconciledGeneration == binding.Generation { glog.V(4).Infof( - "Not processing event for ServiceInstanceCredential %v/%v because checksum showed there is no work to do", + "Not processing event for ServiceInstanceCredential %v/%v because reconciled generation showed there is no work to do", binding.Namespace, binding.Name, ) @@ -320,6 +318,11 @@ func (c *controller) reconcileServiceInstanceCredential(binding *v1alpha1.Servic c.recorder.Event(binding, api.EventTypeWarning, errorInjectingBindResultReason, s) return err } + + // Create/Update for InstanceCredential has completed successful, so set Status.ReconciledGeneration to the + // Generation used. + toUpdate.Status.ReconciledGeneration = toUpdate.Generation + c.setServiceInstanceCredentialCondition( toUpdate, v1alpha1.ServiceInstanceCredentialConditionReady, diff --git a/pkg/controller/controller_binding_test.go b/pkg/controller/controller_binding_test.go index 0ae7f4018ea..cc8cc3057b3 100644 --- a/pkg/controller/controller_binding_test.go +++ b/pkg/controller/controller_binding_test.go @@ -26,7 +26,6 @@ import ( "time" scmeta "github.com/kubernetes-incubator/service-catalog/pkg/api/meta" - checksum "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/checksum/versioned/v1alpha1" "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1" osb "github.com/pmorie/go-open-service-broker-client/v2" fakeosb "github.com/pmorie/go-open-service-broker-client/v2/fake" @@ -889,9 +888,6 @@ func TestReconcileServiceInstanceCredentialDeleteFailedServiceInstanceCredential binding.ObjectMeta.DeletionTimestamp = &metav1.Time{} binding.ObjectMeta.Finalizers = []string{v1alpha1.FinalizerServiceCatalog} - checksum := checksum.ServiceInstanceCredentialSpecChecksum(binding.Spec) - binding.Status.Checksum = &checksum - fakeCatalogClient.AddReactor("get", "serviceinstancecredentials", func(action clientgotesting.Action) (bool, runtime.Object, error) { return true, binding, nil }) diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index 042fbeba2ad..5c4f5fa1df8 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -22,7 +22,6 @@ import ( "github.com/golang/glog" osb "github.com/pmorie/go-open-service-broker-client/v2" - checksum "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/checksum/versioned/v1alpha1" "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1" "github.com/kubernetes-incubator/service-catalog/pkg/brokerapi" "k8s.io/apimachinery/pkg/api/errors" @@ -169,7 +168,7 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1alpha1.ServiceIn // // TODO: the above logic changes slightly once we handle orphan // mitigation. - if !instance.Status.AsyncOpInProgress && instance.Status.Checksum == nil { + if !instance.Status.AsyncOpInProgress && instance.Status.ReconciledGeneration == 0 { finalizers.Delete(finalizerToken) // Clear the finalizer return c.updateServiceInstanceFinalizers(instance, finalizers.List()) @@ -355,15 +354,16 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance return c.pollServiceInstanceInternal(instance) } - // If there's no async op in progress, determine whether the checksum - // has been invalidated by a change to the object. If the instance's - // checksum matches the calculated checksum, there is no work to do. - // If there's an async op in progress, we need to keep polling, hence - // do not bail if checksum hasn't changed. + // If there's no async op in progress, determine whether there is a new + // generation of the object. If the instance's generation does not match + // the reconciled generation, then there is a new generation, indicating + // that changes have been made to the instance's spec. If there is an + // async op in progress, we need to keep polling, hence do not bail if + // there is not a new generation. // // 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. + // but does not affect the generation. // // Note: currently the instance spec is immutable because we do not yet // support plan or parameter updates. This logic is currently meant only @@ -371,11 +371,10 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance // communicating with the broker. In the future the same logic will // result in an instance that requires update being processed by the // controller. - if instance.Status.Checksum != nil && instance.DeletionTimestamp == nil { - instanceChecksum := checksum.ServiceInstanceSpecChecksum(instance.Spec) - if instanceChecksum == *instance.Status.Checksum { + if instance.Status.ReconciledGeneration != 0 && instance.DeletionTimestamp == nil { + if instance.Status.ReconciledGeneration == instance.Generation { glog.V(4).Infof( - "Not processing event for ServiceInstance %v/%v because checksum showed there is no work to do", + "Not processing event for ServiceInstance %v/%v because reconciled generation showed there is no work to do", instance.Namespace, instance.Name, ) @@ -546,6 +545,10 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance } else { glog.V(5).Infof("Successfully provisioned ServiceInstance %v/%v of ServiceClass %v at ServiceBroker %v: response: %+v", instance.Namespace, instance.Name, serviceClass.Name, brokerName, response) + // Create/Update for Instance has completed successful, so set Status.ReconciledGeneration to the + // Generation used. + toUpdate.Status.ReconciledGeneration = toUpdate.Generation + // TODO: process response setServiceInstanceCondition( toUpdate, @@ -718,6 +721,10 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se c.recorder.Event(instance, api.EventTypeNormal, successDeprovisionReason, successDeprovisionMessage) glog.V(5).Infof("Successfully deprovisioned ServiceInstance %v/%v of ServiceClass %v at ServiceBroker %v", instance.Namespace, instance.Name, serviceClass.Name, brokerName) } else { + // Create/Update for InstanceCredential has completed successful, so set Status.ReconciledGeneration to the + // Generation used. + toUpdate.Status.ReconciledGeneration = toUpdate.Generation + c.updateServiceInstanceCondition( toUpdate, v1alpha1.ServiceInstanceConditionReady, diff --git a/pkg/controller/controller_instance_test.go b/pkg/controller/controller_instance_test.go index cb0da0dfdb7..1ab9064dc5e 100644 --- a/pkg/controller/controller_instance_test.go +++ b/pkg/controller/controller_instance_test.go @@ -28,7 +28,6 @@ import ( osb "github.com/pmorie/go-open-service-broker-client/v2" fakeosb "github.com/pmorie/go-open-service-broker-client/v2/fake" - checksum "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/checksum/versioned/v1alpha1" "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -745,10 +744,9 @@ func TestReconcileServiceInstanceDelete(t *testing.T) { instance := getTestServiceInstance() instance.ObjectMeta.DeletionTimestamp = &metav1.Time{} instance.ObjectMeta.Finalizers = []string{v1alpha1.FinalizerServiceCatalog} - // we only invoke the broker client to deprovision if we have a checksum set + // we only invoke the broker client to deprovision if we have a recondiled generation set // as that implies a previous success. - checksum := checksum.ServiceInstanceSpecChecksum(instance.Spec) - instance.Status.Checksum = &checksum + instance.Status.ReconciledGeneration = 1 fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) { return true, instance, nil @@ -812,10 +810,9 @@ func TestReconcileServiceInstanceDeleteAsynchronous(t *testing.T) { instance := getTestServiceInstance() instance.ObjectMeta.DeletionTimestamp = &metav1.Time{} instance.ObjectMeta.Finalizers = []string{v1alpha1.FinalizerServiceCatalog} - // we only invoke the broker client to deprovision if we have a checksum set + // we only invoke the broker client to deprovision if we have a recondiled generation set // as that implies a previous success. - checksum := checksum.ServiceInstanceSpecChecksum(instance.Spec) - instance.Status.Checksum = &checksum + instance.Status.ReconciledGeneration = 1 fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) { return true, instance, nil @@ -880,10 +877,9 @@ func TestReconcileServiceInstanceDeleteFailedInstance(t *testing.T) { instance.ObjectMeta.DeletionTimestamp = &metav1.Time{} instance.ObjectMeta.Finalizers = []string{v1alpha1.FinalizerServiceCatalog} - // we only invoke the broker client to deprovision if we have a checksum set + // we only invoke the broker client to deprovision if we have a recondiled generation set // as that implies a previous success. - checksum := checksum.ServiceInstanceSpecChecksum(instance.Spec) - instance.Status.Checksum = &checksum + instance.Status.ReconciledGeneration = 1 fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) { return true, instance, nil diff --git a/pkg/openapi/openapi_generated.go b/pkg/openapi/openapi_generated.go index a53e75e72b8..db24d6d1ade 100644 --- a/pkg/openapi/openapi_generated.go +++ b/pkg/openapi/openapi_generated.go @@ -775,15 +775,15 @@ func GetOpenAPIDefinitions(ref openapi.ReferenceCallback) map[string]openapi.Ope }, }, }, - "checksum": { + "ReconciledGeneration": { SchemaProps: spec.SchemaProps{ - Description: "Checksum is the checksum of the ServiceInstanceCredentialSpec that was last successfully reconciled against the broker.", - Type: []string{"string"}, - Format: "", + Description: "ReconciledGeneration is the generation of the ServiceInstanceCredential that was last successfully reconciled.", + Type: []string{"integer"}, + Format: "int64", }, }, }, - Required: []string{"conditions"}, + Required: []string{"conditions", "ReconciledGeneration"}, }, }, Dependencies: []string{ @@ -923,15 +923,15 @@ func GetOpenAPIDefinitions(ref openapi.ReferenceCallback) map[string]openapi.Ope Format: "", }, }, - "checksum": { + "ReconciledGeneration": { SchemaProps: spec.SchemaProps{ - Description: "Checksum is the checksum of the ServiceInstanceSpec that was last successfully reconciled against the broker.", - Type: []string{"string"}, - Format: "", + Description: "ReconciledGeneration is the generation of the instance that was last successfully reconciled.", + Type: []string{"integer"}, + Format: "int64", }, }, }, - Required: []string{"conditions", "asyncOpInProgress"}, + Required: []string{"conditions", "asyncOpInProgress", "ReconciledGeneration"}, }, }, Dependencies: []string{ diff --git a/pkg/registry/servicecatalog/binding/strategy.go b/pkg/registry/servicecatalog/binding/strategy.go index 53339ae31b4..0cf8031c87d 100644 --- a/pkg/registry/servicecatalog/binding/strategy.go +++ b/pkg/registry/servicecatalog/binding/strategy.go @@ -19,6 +19,7 @@ package binding // this was copied from where else and edited to fit our objects import ( + //apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" @@ -28,7 +29,6 @@ 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" ) @@ -97,6 +97,7 @@ func (bindingRESTStrategy) PrepareForCreate(ctx genericapirequest.Context, obj r // Fill in the first entry set to "creating"? binding.Status.Conditions = []sc.ServiceInstanceCredentialCondition{} binding.Finalizers = []string{sc.FinalizerServiceCatalog} + binding.Generation = 1 } func (bindingRESTStrategy) Validate(ctx genericapirequest.Context, obj runtime.Object) field.ErrorList { @@ -120,8 +121,20 @@ func (bindingRESTStrategy) PrepareForUpdate(ctx genericapirequest.Context, new, if !ok { glog.Fatal("received a non-binding object to update from") } - newServiceInstanceCredential.Spec = oldServiceInstanceCredential.Spec newServiceInstanceCredential.Status = oldServiceInstanceCredential.Status + + // TODO: We currently don't handle any changes to the spec in the + // reconciler. Once we do that, this check needs to be removed and + // proper validation of allowed changes needs to be implemented in + // ValidateUpdate. Also, the check for whether the generation needs + // to be updated needs to be un-commented. + newServiceInstanceCredential.Spec = oldServiceInstanceCredential.Spec + + // Spec updates bump the generation so that we can distinguish between + // spec changes and other changes to the object. + //if !apiequality.Semantic.DeepEqual(oldServiceInstanceCredential.Spec, newServiceInstanceCredential.Spec) { + // newServiceInstanceCredential.Generation = oldServiceInstanceCredential.Generation + 1 + //} } func (bindingRESTStrategy) ValidateUpdate(ctx genericapirequest.Context, new, old runtime.Object) field.ErrorList { @@ -148,28 +161,6 @@ func (bindingStatusRESTStrategy) PrepareForUpdate(ctx genericapirequest.Context, } // status changes are not allowed to update spec newServiceInstanceCredential.Spec = oldServiceInstanceCredential.Spec - - foundReadyConditionTrue := false - for _, condition := range newServiceInstanceCredential.Status.Conditions { - if condition.Type == sc.ServiceInstanceCredentialConditionReady && condition.Status == sc.ConditionTrue { - foundReadyConditionTrue = true - break - } - } - - if foundReadyConditionTrue { - glog.Infof("Found true ready condition for ServiceInstanceCredential %v/%v; updating checksum", newServiceInstanceCredential.Namespace, newServiceInstanceCredential.Name) - // This status update has a true ready condition; update the checksum if necessary - newServiceInstanceCredential.Status.Checksum = func() *string { - s := checksum.ServiceInstanceCredentialSpecChecksum(newServiceInstanceCredential.Spec) - return &s - }() - return - } - - // if the ready condition is not true, the value of the checksum should - // not change. - newServiceInstanceCredential.Status.Checksum = oldServiceInstanceCredential.Status.Checksum } func (bindingStatusRESTStrategy) ValidateUpdate(ctx genericapirequest.Context, new, old runtime.Object) field.ErrorList { diff --git a/pkg/registry/servicecatalog/binding/strategy_test.go b/pkg/registry/servicecatalog/binding/strategy_test.go index 8a6cadc05fd..2a76ccbe5e9 100644 --- a/pkg/registry/servicecatalog/binding/strategy_test.go +++ b/pkg/registry/servicecatalog/binding/strategy_test.go @@ -22,11 +22,14 @@ import ( "k8s.io/client-go/pkg/api/v1" "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog" - checksum "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/checksum/unversioned" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func bindingWithFalseReadyCondition() *servicecatalog.ServiceInstanceCredential { +func instanceCredentialWithOldSpec() *servicecatalog.ServiceInstanceCredential { return &servicecatalog.ServiceInstanceCredential{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + }, Spec: servicecatalog.ServiceInstanceCredentialSpec{ ServiceInstanceRef: v1.LocalObjectReference{ Name: "some-string", @@ -43,74 +46,48 @@ func bindingWithFalseReadyCondition() *servicecatalog.ServiceInstanceCredential } } -func bindingWithTrueReadyCondition() *servicecatalog.ServiceInstanceCredential { - return &servicecatalog.ServiceInstanceCredential{ - Spec: servicecatalog.ServiceInstanceCredentialSpec{ - ServiceInstanceRef: v1.LocalObjectReference{ - Name: "some-string", - }, - }, - Status: servicecatalog.ServiceInstanceCredentialStatus{ - Conditions: []servicecatalog.ServiceInstanceCredentialCondition{ - { - Type: servicecatalog.ServiceInstanceCredentialConditionReady, - Status: servicecatalog.ConditionTrue, - }, - }, - }, - } -} +// TODO: Un-comment "spec-change" test case when there is a field +// in the spec to which the reconciler allows a change. + +//func instanceCredentialWithNewSpec() *servicecatalog.ServiceInstanceCredential { +// ic := instanceCredentialWithOldSpec() +// ic.Spec.ServiceInstanceRef = v1.LocalObjectReference{ +// Name: "new-string", +// } +// return ic +//} -func TestValidateUpdateStatusPrepareForUpdate(t *testing.T) { +// TestInstanceCredentialUpdate tests that generation is incremented correctly when the +// spec of a ServiceInstanceCredential is updated. +func TestInstanceCredentialUpdate(t *testing.T) { cases := []struct { - name string - old *servicecatalog.ServiceInstanceCredential - newer *servicecatalog.ServiceInstanceCredential - shouldChecksum bool - checksumShouldBeSet bool + name string + older *servicecatalog.ServiceInstanceCredential + newer *servicecatalog.ServiceInstanceCredential + shouldGenerationIncrement bool }{ { - name: "not ready -> not ready", - old: bindingWithFalseReadyCondition(), - newer: bindingWithFalseReadyCondition(), - shouldChecksum: false, - checksumShouldBeSet: false, - }, - { - name: "not ready -> not ready, checksum already set", - old: func() *servicecatalog.ServiceInstanceCredential { - b := bindingWithFalseReadyCondition() - cs := "22081-9471-471" - b.Status.Checksum = &cs - return b - }(), - newer: bindingWithFalseReadyCondition(), - shouldChecksum: false, - checksumShouldBeSet: true, - }, - { - name: "not ready -> ready", - old: bindingWithFalseReadyCondition(), - newer: bindingWithTrueReadyCondition(), - shouldChecksum: true, + name: "no spec change", + older: instanceCredentialWithOldSpec(), + newer: instanceCredentialWithOldSpec(), }, + //{ + // name: "spec change", + // older: instanceCredentialWithOldSpec(), + // newer: instanceCredentialWithOldSpec(), + // shouldGenerationIncrement: true, + //}, } for _, tc := range cases { - strategy := bindingStatusUpdateStrategy - strategy.PrepareForUpdate(nil /* api context */, tc.newer, tc.old) + bindingRESTStrategies.PrepareForUpdate(nil, tc.newer, tc.older) - if tc.shouldChecksum { - if tc.newer.Status.Checksum == nil { - t.Errorf("%v: Checksum should have been set", tc.name) - continue - } - - if e, a := checksum.ServiceInstanceCredentialSpecChecksum(tc.newer.Spec), *tc.newer.Status.Checksum; e != a { - t.Errorf("%v: Checksum was incorrect; expected %v got %v", tc.name, e, a) - } - } else if tc.checksumShouldBeSet != (tc.newer.Status.Checksum != nil) { - t.Errorf("%v: expected checksum to be populated, but was nil", tc.name) + expectedGeneration := tc.older.Generation + if tc.shouldGenerationIncrement { + expectedGeneration = expectedGeneration + 1 + } + if e, a := expectedGeneration, tc.newer.Generation; e != a { + t.Errorf("%v: expected %v, got %v for generation", tc.name, e, a) } } } diff --git a/pkg/registry/servicecatalog/instance/strategy.go b/pkg/registry/servicecatalog/instance/strategy.go index 27565373577..51788ec8b97 100644 --- a/pkg/registry/servicecatalog/instance/strategy.go +++ b/pkg/registry/servicecatalog/instance/strategy.go @@ -19,6 +19,7 @@ package instance // this was copied from where else and edited to fit our objects import ( + //apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" @@ -28,7 +29,6 @@ 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" ) @@ -98,6 +98,7 @@ func (instanceRESTStrategy) PrepareForCreate(ctx genericapirequest.Context, obj // Fill in the first entry set to "creating"? instance.Status.Conditions = []sc.ServiceInstanceCondition{} instance.Finalizers = []string{sc.FinalizerServiceCatalog} + instance.Generation = 1 } func (instanceRESTStrategy) Validate(ctx genericapirequest.Context, obj runtime.Object) field.ErrorList { @@ -122,14 +123,21 @@ func (instanceRESTStrategy) PrepareForUpdate(ctx genericapirequest.Context, new, glog.Fatal("received a non-instance object to update from") } + // Do not allow any updates to the Status field while updating the Spec + newServiceInstance.Status = oldServiceInstance.Status + // TODO: We currently don't handle any changes to the spec in the // reconciler. Once we do that, this check needs to be removed and // proper validation of allowed changes needs to be implemented in - // ValidateUpdate + // ValidateUpdate. Also, the check for whether the generation needs + // to be updated needs to be un-commented. newServiceInstance.Spec = oldServiceInstance.Spec - // Do not allow any updates to the Status field while updating the Spec - newServiceInstance.Status = oldServiceInstance.Status + // Spec updates bump the generation so that we can distinguish between + // spec changes and other changes to the object. + //if !apiequality.Semantic.DeepEqual(oldServiceInstance.Spec, newServiceInstance.Spec) { + // newServiceInstance.Generation = oldServiceInstance.Generation + 1 + //} } func (instanceRESTStrategy) ValidateUpdate(ctx genericapirequest.Context, new, old runtime.Object) field.ErrorList { @@ -156,29 +164,6 @@ func (instanceStatusRESTStrategy) PrepareForUpdate(ctx genericapirequest.Context } // Status changes are not allowed to update spec newServiceInstance.Spec = oldServiceInstance.Spec - - foundReadyConditionTrue := false - for _, condition := range newServiceInstance.Status.Conditions { - if condition.Type == sc.ServiceInstanceConditionReady && condition.Status == sc.ConditionTrue { - foundReadyConditionTrue = true - break - } - } - - if foundReadyConditionTrue { - glog.Infof("Found true ready condition for ServiceInstance %v/%v; updating checksum", newServiceInstance.Namespace, newServiceInstance.Name) - // This status update has a true ready condition; update the checksum - // if necessary - newServiceInstance.Status.Checksum = func() *string { - s := checksum.ServiceInstanceSpecChecksum(newServiceInstance.Spec) - return &s - }() - return - } - - // if the ready condition is not true, the value of the checksum should - // not change. - newServiceInstance.Status.Checksum = oldServiceInstance.Status.Checksum } func (instanceStatusRESTStrategy) ValidateUpdate(ctx genericapirequest.Context, new, old runtime.Object) field.ErrorList { diff --git a/pkg/registry/servicecatalog/instance/strategy_test.go b/pkg/registry/servicecatalog/instance/strategy_test.go index 90499a13a54..0aab1e63956 100644 --- a/pkg/registry/servicecatalog/instance/strategy_test.go +++ b/pkg/registry/servicecatalog/instance/strategy_test.go @@ -20,11 +20,14 @@ import ( "testing" "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog" - checksum "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/checksum/unversioned" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func instanceWithFalseReadyCondition() *servicecatalog.ServiceInstance { +func instanceWithOldSpec() *servicecatalog.ServiceInstance { return &servicecatalog.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + }, Spec: servicecatalog.ServiceInstanceSpec{ ServiceClassName: "test-serviceclass", PlanName: "test-plan", @@ -40,73 +43,46 @@ func instanceWithFalseReadyCondition() *servicecatalog.ServiceInstance { } } -func instanceWithTrueReadyCondition() *servicecatalog.ServiceInstance { - return &servicecatalog.ServiceInstance{ - Spec: servicecatalog.ServiceInstanceSpec{ - ServiceClassName: "test-serviceclass", - PlanName: "test-plan", - }, - Status: servicecatalog.ServiceInstanceStatus{ - Conditions: []servicecatalog.ServiceInstanceCondition{ - { - Type: servicecatalog.ServiceInstanceConditionReady, - Status: servicecatalog.ConditionTrue, - }, - }, - }, - } -} +// TODO: Un-comment "spec-change" test case when there is a field +// in the spec to which the reconciler allows a change. + +//func instanceWithNewSpec() *servicecatalog.ServiceInstance { +// i := instanceWithOldSpec() +// i.Spec.ServiceClassName = "new-serviceclass" +// return i +//} -func TestValidateUpdateStatusPrepareForUpdate(t *testing.T) { +// TestInstanceUpdate tests that generation is incremented correctly when the +// spec of a Instance is updated. +func TestInstanceUpdate(t *testing.T) { cases := []struct { - name string - old *servicecatalog.ServiceInstance - newer *servicecatalog.ServiceInstance - shouldChecksum bool - checksumShouldBeSet bool + name string + older *servicecatalog.ServiceInstance + newer *servicecatalog.ServiceInstance + shouldGenerationIncrement bool }{ { - name: "not ready -> not ready", - old: instanceWithFalseReadyCondition(), - newer: instanceWithFalseReadyCondition(), - shouldChecksum: false, - checksumShouldBeSet: false, - }, - { - name: "not ready -> not ready, checksum already set", - old: func() *servicecatalog.ServiceInstance { - i := instanceWithFalseReadyCondition() - cs := "22081-9471-471" - i.Status.Checksum = &cs - return i - }(), - newer: instanceWithFalseReadyCondition(), - shouldChecksum: false, - checksumShouldBeSet: true, - }, - { - name: "not ready -> ready", - old: instanceWithFalseReadyCondition(), - newer: instanceWithTrueReadyCondition(), - shouldChecksum: true, + name: "no spec change", + older: instanceWithOldSpec(), + newer: instanceWithOldSpec(), }, + //{ + // name: "spec change", + // older: instanceWithOldSpec(), + // newer: instanceWithNewSpec(), + // shouldGenerationIncrement: true, + //}, } for _, tc := range cases { - strategy := instanceStatusUpdateStrategy - strategy.PrepareForUpdate(nil /* api context */, tc.newer, tc.old) + instanceRESTStrategies.PrepareForUpdate(nil, tc.newer, tc.older) - if tc.shouldChecksum { - if tc.newer.Status.Checksum == nil { - t.Errorf("%v: Checksum should have been set", tc.name) - continue - } - - if e, a := checksum.ServiceInstanceSpecChecksum(tc.newer.Spec), *tc.newer.Status.Checksum; e != a { - t.Errorf("%v: Checksum was incorrect; expected %v got %v", tc.name, e, a) - } - } else if tc.checksumShouldBeSet != (tc.newer.Status.Checksum != nil) { - t.Errorf("%v: expected checksum to be populated, but was nil", tc.name) + expectedGeneration := tc.older.Generation + if tc.shouldGenerationIncrement { + expectedGeneration = expectedGeneration + 1 + } + if e, a := expectedGeneration, tc.newer.Generation; e != a { + t.Errorf("%v: expected %v, got %v for generation", tc.name, e, a) } } } diff --git a/test/integration/clientset_test.go b/test/integration/clientset_test.go index 595ea96f48a..c4a7ac36234 100644 --- a/test/integration/clientset_test.go +++ b/test/integration/clientset_test.go @@ -627,9 +627,6 @@ func testInstanceClient(sType server.StorageType, client servicecatalogclient.In 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.Status.Checksum == nil { - return fmt.Errorf("Checksum should have been set after updating ready condition to true") - } // delete the instance, set its finalizers to nil, update it, then ensure it is actually // deleted @@ -798,9 +795,6 @@ func testBindingClient(sType server.StorageType, client servicecatalogclient.Int 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.Status.Checksum == nil { - return fmt.Errorf("Checksum should have been set after updating ready condition to true") - } if err = bindingClient.Delete(name, &metav1.DeleteOptions{}); nil != err { return fmt.Errorf("binding delete failed (%s)", err) From b204d6aacaa6080c119bc14f676e473533f7fcb3 Mon Sep 17 00:00:00 2001 From: staebler Date: Wed, 30 Aug 2017 12:23:37 -0400 Subject: [PATCH 2/4] Changes from review feedback. --- pkg/controller/controller_binding.go | 4 ++-- pkg/controller/controller_binding_test.go | 3 +++ pkg/controller/controller_instance.go | 4 ++-- pkg/registry/servicecatalog/instance/strategy.go | 4 ++++ 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/pkg/controller/controller_binding.go b/pkg/controller/controller_binding.go index 4d4bdafd812..681e103d5df 100644 --- a/pkg/controller/controller_binding.go +++ b/pkg/controller/controller_binding.go @@ -319,8 +319,8 @@ func (c *controller) reconcileServiceInstanceCredential(binding *v1alpha1.Servic return err } - // Create/Update for InstanceCredential has completed successful, so set Status.ReconciledGeneration to the - // Generation used. + // The bind operation completed successful, so set + // Status.ReconciledGeneration to the Generation used. toUpdate.Status.ReconciledGeneration = toUpdate.Generation c.setServiceInstanceCredentialCondition( diff --git a/pkg/controller/controller_binding_test.go b/pkg/controller/controller_binding_test.go index cc8cc3057b3..bb0dd1d4c19 100644 --- a/pkg/controller/controller_binding_test.go +++ b/pkg/controller/controller_binding_test.go @@ -888,6 +888,9 @@ func TestReconcileServiceInstanceCredentialDeleteFailedServiceInstanceCredential binding.ObjectMeta.DeletionTimestamp = &metav1.Time{} binding.ObjectMeta.Finalizers = []string{v1alpha1.FinalizerServiceCatalog} + binding.ObjectMeta.Generation = 1 + binding.Status.ReconciledGeneration = 1 + fakeCatalogClient.AddReactor("get", "serviceinstancecredentials", func(action clientgotesting.Action) (bool, runtime.Object, error) { return true, binding, nil }) diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index 5c4f5fa1df8..dc1e38fbc14 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -545,8 +545,8 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance } else { glog.V(5).Infof("Successfully provisioned ServiceInstance %v/%v of ServiceClass %v at ServiceBroker %v: response: %+v", instance.Namespace, instance.Name, serviceClass.Name, brokerName, response) - // Create/Update for Instance has completed successful, so set Status.ReconciledGeneration to the - // Generation used. + // Create/Update for Instance has completed successful, so set + // Status.ReconciledGeneration to the Generation used. toUpdate.Status.ReconciledGeneration = toUpdate.Generation // TODO: process response diff --git a/pkg/registry/servicecatalog/instance/strategy.go b/pkg/registry/servicecatalog/instance/strategy.go index 51788ec8b97..aa29a37c274 100644 --- a/pkg/registry/servicecatalog/instance/strategy.go +++ b/pkg/registry/servicecatalog/instance/strategy.go @@ -19,6 +19,8 @@ package instance // this was copied from where else and edited to fit our objects import ( + "fmt" + //apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" @@ -114,6 +116,8 @@ func (instanceRESTStrategy) AllowUnconditionalUpdate() bool { } func (instanceRESTStrategy) PrepareForUpdate(ctx genericapirequest.Context, new, old runtime.Object) { + fmt.Printf("Instance PrepareForUpdate") + newServiceInstance, ok := new.(*sc.ServiceInstance) if !ok { glog.Fatal("received a non-instance object to update to") From df61ab7c75a2024798dd3715a7191bc875f8f5e9 Mon Sep 17 00:00:00 2001 From: staebler Date: Wed, 30 Aug 2017 16:37:48 -0400 Subject: [PATCH 3/4] Fix comment and left-over print statement. --- pkg/controller/controller_binding.go | 2 +- pkg/controller/controller_instance.go | 6 +++--- pkg/registry/servicecatalog/instance/strategy.go | 4 ---- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/pkg/controller/controller_binding.go b/pkg/controller/controller_binding.go index 681e103d5df..965a65e5533 100644 --- a/pkg/controller/controller_binding.go +++ b/pkg/controller/controller_binding.go @@ -319,7 +319,7 @@ func (c *controller) reconcileServiceInstanceCredential(binding *v1alpha1.Servic return err } - // The bind operation completed successful, so set + // The bind operation completed successfully, so set // Status.ReconciledGeneration to the Generation used. toUpdate.Status.ReconciledGeneration = toUpdate.Generation diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index dc1e38fbc14..55002f25bd6 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -545,7 +545,7 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance } else { glog.V(5).Infof("Successfully provisioned ServiceInstance %v/%v of ServiceClass %v at ServiceBroker %v: response: %+v", instance.Namespace, instance.Name, serviceClass.Name, brokerName, response) - // Create/Update for Instance has completed successful, so set + // Create/Update for Instance has completed successfully, so set // Status.ReconciledGeneration to the Generation used. toUpdate.Status.ReconciledGeneration = toUpdate.Generation @@ -721,8 +721,8 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se c.recorder.Event(instance, api.EventTypeNormal, successDeprovisionReason, successDeprovisionMessage) glog.V(5).Infof("Successfully deprovisioned ServiceInstance %v/%v of ServiceClass %v at ServiceBroker %v", instance.Namespace, instance.Name, serviceClass.Name, brokerName) } else { - // Create/Update for InstanceCredential has completed successful, so set Status.ReconciledGeneration to the - // Generation used. + // Create/Update for InstanceCredential has completed successfully, + // so set Status.ReconciledGeneration to the Generation used. toUpdate.Status.ReconciledGeneration = toUpdate.Generation c.updateServiceInstanceCondition( diff --git a/pkg/registry/servicecatalog/instance/strategy.go b/pkg/registry/servicecatalog/instance/strategy.go index aa29a37c274..51788ec8b97 100644 --- a/pkg/registry/servicecatalog/instance/strategy.go +++ b/pkg/registry/servicecatalog/instance/strategy.go @@ -19,8 +19,6 @@ package instance // this was copied from where else and edited to fit our objects import ( - "fmt" - //apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" @@ -116,8 +114,6 @@ func (instanceRESTStrategy) AllowUnconditionalUpdate() bool { } func (instanceRESTStrategy) PrepareForUpdate(ctx genericapirequest.Context, new, old runtime.Object) { - fmt.Printf("Instance PrepareForUpdate") - newServiceInstance, ok := new.(*sc.ServiceInstance) if !ok { glog.Fatal("received a non-instance object to update to") From 8fd96d90c30dc1d26522460c64864dcf728826cc Mon Sep 17 00:00:00 2001 From: staebler Date: Wed, 30 Aug 2017 21:13:05 -0400 Subject: [PATCH 4/4] Code review feedback. --- pkg/apis/servicecatalog/types.go | 16 ++-- .../v1alpha1/types.generated.go | 8 +- pkg/apis/servicecatalog/v1alpha1/types.go | 20 +++-- pkg/controller/controller_binding.go | 2 +- pkg/controller/controller_binding_test.go | 74 ++++++++++++++++--- pkg/controller/controller_instance.go | 2 +- pkg/controller/controller_instance_test.go | 16 ++-- pkg/controller/controller_test.go | 12 ++- pkg/openapi/openapi_generated.go | 14 ++-- .../servicecatalog/binding/strategy.go | 11 ++- .../servicecatalog/instance/strategy.go | 11 ++- 11 files changed, 132 insertions(+), 54 deletions(-) diff --git a/pkg/apis/servicecatalog/types.go b/pkg/apis/servicecatalog/types.go index a9f7906adc9..8afaae82303 100644 --- a/pkg/apis/servicecatalog/types.go +++ b/pkg/apis/servicecatalog/types.go @@ -114,8 +114,9 @@ const ( type ServiceBrokerStatus struct { Conditions []ServiceBrokerCondition - // ReconciledGeneration is the generation of the broker that was last - // successfully reconciled. + // ReconciledGeneration is the 'Generation' of the serviceBrokerSpec that + // was last processed by the controller. The reconciled generation is updated + // even if the controller failed to process the spec. ReconciledGeneration int64 } @@ -357,8 +358,9 @@ type ServiceInstanceStatus struct { // the service instance. DashboardURL *string - // ReconciledGeneration is the generation of the instance that was last - // successfully reconciled. + // ReconciledGeneration is the 'Generation' of the serviceInstanceSpec that + // was last processed by the controller. The reconciled generation is updated + // even if the controller failed to process the spec. ReconciledGeneration int64 } @@ -454,8 +456,10 @@ type ServiceInstanceCredentialSpec struct { type ServiceInstanceCredentialStatus struct { Conditions []ServiceInstanceCredentialCondition - // ReconciledGeneration is the generation of the ServiceInstanceCredential that was last - // successfully reconciled. + // ReconciledGeneration is the 'Generation' of the + // serviceInstanceCredentialSpec that was last processed by the controller. + // The reconciled generation is updated even if the controller failed to + // process the spec. ReconciledGeneration int64 } diff --git a/pkg/apis/servicecatalog/v1alpha1/types.generated.go b/pkg/apis/servicecatalog/v1alpha1/types.generated.go index 940b15f6a8e..2fb63aad5df 100644 --- a/pkg/apis/servicecatalog/v1alpha1/types.generated.go +++ b/pkg/apis/servicecatalog/v1alpha1/types.generated.go @@ -5794,7 +5794,7 @@ func (x *ServiceInstanceStatus) CodecEncodeSelf(e *codec1978.Encoder) { } } else { z.EncSendContainerState(codecSelfer_containerMapKey1234) - r.EncodeString(codecSelferC_UTF81234, string("ReconciledGeneration")) + r.EncodeString(codecSelferC_UTF81234, string("reconciledGeneration")) z.EncSendContainerState(codecSelfer_containerMapValue1234) yym21 := z.EncBinary() _ = yym21 @@ -5920,7 +5920,7 @@ func (x *ServiceInstanceStatus) codecDecodeSelfFromMap(l int, d *codec1978.Decod *((*string)(x.DashboardURL)) = r.DecodeString() } } - case "ReconciledGeneration": + case "reconciledGeneration": if r.TryDecodeAsNil() { x.ReconciledGeneration = 0 } else { @@ -7711,7 +7711,7 @@ func (x *ServiceInstanceCredentialStatus) CodecEncodeSelf(e *codec1978.Encoder) } } else { z.EncSendContainerState(codecSelfer_containerMapKey1234) - r.EncodeString(codecSelferC_UTF81234, string("ReconciledGeneration")) + r.EncodeString(codecSelferC_UTF81234, string("reconciledGeneration")) z.EncSendContainerState(codecSelfer_containerMapValue1234) yym8 := z.EncBinary() _ = yym8 @@ -7793,7 +7793,7 @@ func (x *ServiceInstanceCredentialStatus) codecDecodeSelfFromMap(l int, d *codec h.decSliceServiceInstanceCredentialCondition((*[]ServiceInstanceCredentialCondition)(yyv4), d) } } - case "ReconciledGeneration": + case "reconciledGeneration": if r.TryDecodeAsNil() { x.ReconciledGeneration = 0 } else { diff --git a/pkg/apis/servicecatalog/v1alpha1/types.go b/pkg/apis/servicecatalog/v1alpha1/types.go index 2bc963e4ee7..f5c8e40ec8c 100644 --- a/pkg/apis/servicecatalog/v1alpha1/types.go +++ b/pkg/apis/servicecatalog/v1alpha1/types.go @@ -114,8 +114,9 @@ const ( type ServiceBrokerStatus struct { Conditions []ServiceBrokerCondition `json:"conditions"` - // ReconciledGeneration is the generation of the broker that was last - // successfully reconciled. + // ReconciledGeneration is the 'Generation' of the serviceBrokerSpec that + // was last processed by the controller. The reconciled generation is updated + // even if the controller failed to process the spec. ReconciledGeneration int64 `json:"reconciledGeneration"` } @@ -361,9 +362,10 @@ type ServiceInstanceStatus struct { // the service instance. DashboardURL *string `json:"dashboardURL,omitempty"` - // ReconciledGeneration is the generation of the instance that was last - // successfully reconciled. - ReconciledGeneration int64 + // ReconciledGeneration is the 'Generation' of the serviceInstanceSpec that + // was last processed by the controller. The reconciled generation is updated + // even if the controller failed to process the spec. + ReconciledGeneration int64 `json:"reconciledGeneration"` } // ServiceInstanceCondition contains condition information about an Instance. @@ -458,9 +460,11 @@ type ServiceInstanceCredentialSpec struct { type ServiceInstanceCredentialStatus struct { Conditions []ServiceInstanceCredentialCondition `json:"conditions"` - // ReconciledGeneration is the generation of the ServiceInstanceCredential that was last - // successfully reconciled. - ReconciledGeneration int64 + // ReconciledGeneration is the 'Generation' of the + // serviceInstanceCredentialSpec that was last processed by the controller. + // The reconciled generation is updated even if the controller failed to + // process the spec. + ReconciledGeneration int64 `json:"reconciledGeneration"` } // ServiceInstanceCredentialCondition condition information for a ServiceInstanceCredential. diff --git a/pkg/controller/controller_binding.go b/pkg/controller/controller_binding.go index 965a65e5533..3796621ed25 100644 --- a/pkg/controller/controller_binding.go +++ b/pkg/controller/controller_binding.go @@ -110,7 +110,7 @@ func (c *controller) reconcileServiceInstanceCredential(binding *v1alpha1.Servic // 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 generation. - if binding.Status.ReconciledGeneration != 0 && binding.DeletionTimestamp == nil { + if binding.DeletionTimestamp == nil { if binding.Status.ReconciledGeneration == binding.Generation { glog.V(4).Infof( "Not processing event for ServiceInstanceCredential %v/%v because reconciled generation showed there is no work to do", diff --git a/pkg/controller/controller_binding_test.go b/pkg/controller/controller_binding_test.go index bb0dd1d4c19..4a848dd2099 100644 --- a/pkg/controller/controller_binding_test.go +++ b/pkg/controller/controller_binding_test.go @@ -45,7 +45,10 @@ func TestReconcileServiceInstanceCredentialNonExistingServiceInstance(t *testing _, fakeCatalogClient, fakeServiceBrokerClient, testController, _ := newTestController(t, noFakeActions()) binding := &v1alpha1.ServiceInstanceCredential{ - ObjectMeta: metav1.ObjectMeta{Name: testServiceInstanceCredentialName}, + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceInstanceCredentialName, + Generation: 1, + }, Spec: v1alpha1.ServiceInstanceCredentialSpec{ ServiceInstanceRef: v1.LocalObjectReference{Name: "nothere"}, ExternalID: bindingGUID, @@ -98,7 +101,11 @@ func TestReconcileServiceInstanceCredentialNonExistingServiceClass(t *testing.T) sharedInformers.ServiceInstances().Informer().GetStore().Add(instance) binding := &v1alpha1.ServiceInstanceCredential{ - ObjectMeta: metav1.ObjectMeta{Name: testServiceInstanceCredentialName, Namespace: testNamespace}, + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceInstanceCredentialName, + Namespace: testNamespace, + Generation: 1, + }, Spec: v1alpha1.ServiceInstanceCredentialSpec{ ServiceInstanceRef: v1.LocalObjectReference{Name: testServiceInstanceName}, ExternalID: bindingGUID, @@ -154,7 +161,11 @@ func TestReconcileServiceInstanceCredentialWithSecretConflict(t *testing.T) { sharedInformers.ServiceInstances().Informer().GetStore().Add(getTestServiceInstanceWithStatus(v1alpha1.ConditionTrue)) binding := &v1alpha1.ServiceInstanceCredential{ - ObjectMeta: metav1.ObjectMeta{Name: testServiceInstanceCredentialName, Namespace: testNamespace}, + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceInstanceCredentialName, + Namespace: testNamespace, + Generation: 1, + }, Spec: v1alpha1.ServiceInstanceCredentialSpec{ ServiceInstanceRef: v1.LocalObjectReference{Name: testServiceInstanceName}, ExternalID: bindingGUID, @@ -229,7 +240,11 @@ func TestReconcileServiceInstanceCredentialWithParameters(t *testing.T) { sharedInformers.ServiceInstances().Informer().GetStore().Add(getTestServiceInstanceWithStatus(v1alpha1.ConditionTrue)) binding := &v1alpha1.ServiceInstanceCredential{ - ObjectMeta: metav1.ObjectMeta{Name: testServiceInstanceCredentialName, Namespace: testNamespace}, + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceInstanceCredentialName, + Namespace: testNamespace, + Generation: 1, + }, Spec: v1alpha1.ServiceInstanceCredentialSpec{ ServiceInstanceRef: v1.LocalObjectReference{Name: testServiceInstanceName}, ExternalID: bindingGUID, @@ -337,7 +352,11 @@ func TestReconcileServiceInstanceCredentialNonbindableServiceClass(t *testing.T) sharedInformers.ServiceInstances().Informer().GetStore().Add(getTestNonbindableServiceInstance()) binding := &v1alpha1.ServiceInstanceCredential{ - ObjectMeta: metav1.ObjectMeta{Name: testServiceInstanceCredentialName, Namespace: testNamespace}, + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceInstanceCredentialName, + Namespace: testNamespace, + Generation: 1, + }, Spec: v1alpha1.ServiceInstanceCredentialSpec{ ServiceInstanceRef: v1.LocalObjectReference{Name: testServiceInstanceName}, ExternalID: bindingGUID, @@ -402,7 +421,11 @@ func TestReconcileServiceInstanceCredentialNonbindableServiceClassBindablePlan(t }()) binding := &v1alpha1.ServiceInstanceCredential{ - ObjectMeta: metav1.ObjectMeta{Name: testServiceInstanceCredentialName, Namespace: testNamespace}, + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceInstanceCredentialName, + Namespace: testNamespace, + Generation: 1, + }, Spec: v1alpha1.ServiceInstanceCredentialSpec{ ServiceInstanceRef: v1.LocalObjectReference{Name: testServiceInstanceName}, ExternalID: bindingGUID, @@ -482,7 +505,11 @@ func TestReconcileServiceInstanceCredentialBindableServiceClassNonbindablePlan(t sharedInformers.ServiceInstances().Informer().GetStore().Add(getTestServiceInstanceBindableServiceNonbindablePlan()) binding := &v1alpha1.ServiceInstanceCredential{ - ObjectMeta: metav1.ObjectMeta{Name: testServiceInstanceCredentialName, Namespace: testNamespace}, + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceInstanceCredentialName, + Namespace: testNamespace, + Generation: 1, + }, Spec: v1alpha1.ServiceInstanceCredentialSpec{ ServiceInstanceRef: v1.LocalObjectReference{Name: testServiceInstanceName}, ExternalID: bindingGUID, @@ -524,7 +551,11 @@ func TestReconcileServiceInstanceCredentialFailsWithServiceInstanceAsyncOngoing( sharedInformers.ServiceInstances().Informer().GetStore().Add(getTestServiceInstanceAsyncProvisioning("")) binding := &v1alpha1.ServiceInstanceCredential{ - ObjectMeta: metav1.ObjectMeta{Name: testServiceInstanceCredentialName, Namespace: testNamespace}, + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceInstanceCredentialName, + Namespace: testNamespace, + Generation: 1, + }, Spec: v1alpha1.ServiceInstanceCredentialSpec{ ServiceInstanceRef: v1.LocalObjectReference{Name: testServiceInstanceName}, ExternalID: bindingGUID, @@ -581,7 +612,11 @@ func TestReconcileServiceInstanceCredentialServiceInstanceNotReady(t *testing.T) sharedInformers.ServiceInstances().Informer().GetStore().Add(getTestServiceInstance()) binding := &v1alpha1.ServiceInstanceCredential{ - ObjectMeta: metav1.ObjectMeta{Name: testServiceInstanceCredentialName, Namespace: testNamespace}, + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceInstanceCredentialName, + Namespace: testNamespace, + Generation: 1, + }, Spec: v1alpha1.ServiceInstanceCredentialSpec{ ServiceInstanceRef: v1.LocalObjectReference{Name: testServiceInstanceName}, ExternalID: bindingGUID, @@ -626,7 +661,11 @@ func TestReconcileServiceInstanceCredentialNamespaceError(t *testing.T) { sharedInformers.ServiceInstances().Informer().GetStore().Add(getTestServiceInstance()) binding := &v1alpha1.ServiceInstanceCredential{ - ObjectMeta: metav1.ObjectMeta{Name: testServiceInstanceCredentialName, Namespace: testNamespace}, + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceInstanceCredentialName, + Namespace: testNamespace, + Generation: 1, + }, Spec: v1alpha1.ServiceInstanceCredentialSpec{ ServiceInstanceRef: v1.LocalObjectReference{Name: testServiceInstanceName}, ExternalID: bindingGUID, @@ -672,6 +711,7 @@ func TestReconcileServiceInstanceCredentialDelete(t *testing.T) { Namespace: testNamespace, DeletionTimestamp: &metav1.Time{}, Finalizers: []string{v1alpha1.FinalizerServiceCatalog}, + Generation: 1, }, Spec: v1alpha1.ServiceInstanceCredentialSpec{ ServiceInstanceRef: v1.LocalObjectReference{Name: testServiceInstanceName}, @@ -966,7 +1006,11 @@ func TestReconcileServiceInstanceCredentialWithServiceBrokerError(t *testing.T) sharedInformers.ServiceInstances().Informer().GetStore().Add(getTestServiceInstanceWithStatus(v1alpha1.ConditionTrue)) binding := &v1alpha1.ServiceInstanceCredential{ - ObjectMeta: metav1.ObjectMeta{Name: testServiceInstanceCredentialName, Namespace: testNamespace}, + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceInstanceCredentialName, + Namespace: testNamespace, + Generation: 1, + }, Spec: v1alpha1.ServiceInstanceCredentialSpec{ ServiceInstanceRef: v1.LocalObjectReference{Name: testServiceInstanceName}, ExternalID: bindingGUID, @@ -1009,7 +1053,11 @@ func TestReconcileServiceInstanceCredentialWithServiceBrokerHTTPError(t *testing sharedInformers.ServiceInstances().Informer().GetStore().Add(getTestServiceInstanceWithStatus(v1alpha1.ConditionTrue)) binding := &v1alpha1.ServiceInstanceCredential{ - ObjectMeta: metav1.ObjectMeta{Name: testServiceInstanceCredentialName, Namespace: testNamespace}, + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceInstanceCredentialName, + Namespace: testNamespace, + Generation: 1, + }, Spec: v1alpha1.ServiceInstanceCredentialSpec{ ServiceInstanceRef: v1.LocalObjectReference{Name: testServiceInstanceName}, ExternalID: bindingGUID, @@ -1353,6 +1401,7 @@ func TestReconcileUnbindingWithServiceBrokerError(t *testing.T) { Name: testServiceInstanceCredentialName, Namespace: testNamespace, DeletionTimestamp: &t1, + Generation: 1, }, Spec: v1alpha1.ServiceInstanceCredentialSpec{ ServiceInstanceRef: v1.LocalObjectReference{Name: testServiceInstanceName}, @@ -1400,6 +1449,7 @@ func TestReconcileUnbindingWithServiceBrokerHTTPError(t *testing.T) { Name: testServiceInstanceCredentialName, Namespace: testNamespace, DeletionTimestamp: &t1, + Generation: 1, }, Spec: v1alpha1.ServiceInstanceCredentialSpec{ ServiceInstanceRef: v1.LocalObjectReference{Name: testServiceInstanceName}, diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index 55002f25bd6..adba1d5c634 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -371,7 +371,7 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance // communicating with the broker. In the future the same logic will // result in an instance that requires update being processed by the // controller. - if instance.Status.ReconciledGeneration != 0 && instance.DeletionTimestamp == nil { + if instance.DeletionTimestamp == nil { if instance.Status.ReconciledGeneration == instance.Generation { glog.V(4).Infof( "Not processing event for ServiceInstance %v/%v because reconciled generation showed there is no work to do", diff --git a/pkg/controller/controller_instance_test.go b/pkg/controller/controller_instance_test.go index 1ab9064dc5e..0b1df473aba 100644 --- a/pkg/controller/controller_instance_test.go +++ b/pkg/controller/controller_instance_test.go @@ -49,7 +49,10 @@ func TestReconcileServiceInstanceNonExistentServiceClass(t *testing.T) { _, fakeCatalogClient, fakeServiceBrokerClient, testController, _ := newTestController(t, noFakeActions()) instance := &v1alpha1.ServiceInstance{ - ObjectMeta: metav1.ObjectMeta{Name: testServiceInstanceName}, + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceInstanceName, + Generation: 1, + }, Spec: v1alpha1.ServiceInstanceSpec{ ServiceClassName: "nothere", PlanName: "nothere", @@ -183,7 +186,10 @@ func TestReconcileServiceInstanceNonExistentServicePlan(t *testing.T) { sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) instance := &v1alpha1.ServiceInstance{ - ObjectMeta: metav1.ObjectMeta{Name: testServiceInstanceName}, + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceInstanceName, + Generation: 1, + }, Spec: v1alpha1.ServiceInstanceSpec{ ServiceClassName: testServiceClassName, PlanName: "nothere", @@ -744,7 +750,7 @@ func TestReconcileServiceInstanceDelete(t *testing.T) { instance := getTestServiceInstance() instance.ObjectMeta.DeletionTimestamp = &metav1.Time{} instance.ObjectMeta.Finalizers = []string{v1alpha1.FinalizerServiceCatalog} - // we only invoke the broker client to deprovision if we have a recondiled generation set + // we only invoke the broker client to deprovision if we have a reconciled generation set // as that implies a previous success. instance.Status.ReconciledGeneration = 1 @@ -810,7 +816,7 @@ func TestReconcileServiceInstanceDeleteAsynchronous(t *testing.T) { instance := getTestServiceInstance() instance.ObjectMeta.DeletionTimestamp = &metav1.Time{} instance.ObjectMeta.Finalizers = []string{v1alpha1.FinalizerServiceCatalog} - // we only invoke the broker client to deprovision if we have a recondiled generation set + // we only invoke the broker client to deprovision if we have a reconciled generation set // as that implies a previous success. instance.Status.ReconciledGeneration = 1 @@ -877,7 +883,7 @@ func TestReconcileServiceInstanceDeleteFailedInstance(t *testing.T) { instance.ObjectMeta.DeletionTimestamp = &metav1.Time{} instance.ObjectMeta.Finalizers = []string{v1alpha1.FinalizerServiceCatalog} - // we only invoke the broker client to deprovision if we have a recondiled generation set + // we only invoke the broker client to deprovision if we have a reconciled generation set // as that implies a previous success. instance.Status.ReconciledGeneration = 1 diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 78176cef7d9..2d601f338df 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -396,7 +396,11 @@ func getTestCatalog() *osb.CatalogResponse { // instance referencing the result of getTestServiceClass() func getTestServiceInstance() *v1alpha1.ServiceInstance { return &v1alpha1.ServiceInstance{ - ObjectMeta: metav1.ObjectMeta{Name: testServiceInstanceName, Namespace: testNamespace}, + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceInstanceName, + Namespace: testNamespace, + Generation: 1, + }, Spec: v1alpha1.ServiceInstanceSpec{ ServiceClassName: testServiceClassName, PlanName: testPlanName, @@ -503,7 +507,11 @@ func getTestServiceInstanceAsyncDeprovisioningWithFinalizer(operation string) *v // binding referencing the result of getTestServiceInstance() func getTestServiceInstanceCredential() *v1alpha1.ServiceInstanceCredential { return &v1alpha1.ServiceInstanceCredential{ - ObjectMeta: metav1.ObjectMeta{Name: testServiceInstanceCredentialName, Namespace: testNamespace}, + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceInstanceCredentialName, + Namespace: testNamespace, + Generation: 1, + }, Spec: v1alpha1.ServiceInstanceCredentialSpec{ ServiceInstanceRef: v1.LocalObjectReference{Name: testServiceInstanceName}, ExternalID: bindingGUID, diff --git a/pkg/openapi/openapi_generated.go b/pkg/openapi/openapi_generated.go index db24d6d1ade..ac7dbf42402 100644 --- a/pkg/openapi/openapi_generated.go +++ b/pkg/openapi/openapi_generated.go @@ -322,7 +322,7 @@ func GetOpenAPIDefinitions(ref openapi.ReferenceCallback) map[string]openapi.Ope }, "reconciledGeneration": { SchemaProps: spec.SchemaProps{ - Description: "ReconciledGeneration is the generation of the broker that was last successfully reconciled.", + Description: "ReconciledGeneration is the 'Generation' of the serviceBrokerSpec that was last processed by the controller. The reconciled generation is updated even if the controller failed to process the spec.", Type: []string{"integer"}, Format: "int64", }, @@ -775,15 +775,15 @@ func GetOpenAPIDefinitions(ref openapi.ReferenceCallback) map[string]openapi.Ope }, }, }, - "ReconciledGeneration": { + "reconciledGeneration": { SchemaProps: spec.SchemaProps{ - Description: "ReconciledGeneration is the generation of the ServiceInstanceCredential that was last successfully reconciled.", + Description: "ReconciledGeneration is the 'Generation' of the serviceInstanceCredentialSpec that was last processed by the controller. The reconciled generation is updated even if the controller failed to process the spec.", Type: []string{"integer"}, Format: "int64", }, }, }, - Required: []string{"conditions", "ReconciledGeneration"}, + Required: []string{"conditions", "reconciledGeneration"}, }, }, Dependencies: []string{ @@ -923,15 +923,15 @@ func GetOpenAPIDefinitions(ref openapi.ReferenceCallback) map[string]openapi.Ope Format: "", }, }, - "ReconciledGeneration": { + "reconciledGeneration": { SchemaProps: spec.SchemaProps{ - Description: "ReconciledGeneration is the generation of the instance that was last successfully reconciled.", + Description: "ReconciledGeneration is the 'Generation' of the serviceInstanceSpec that was last processed by the controller. The reconciled generation is updated even if the controller failed to process the spec.", Type: []string{"integer"}, Format: "int64", }, }, }, - Required: []string{"conditions", "asyncOpInProgress", "ReconciledGeneration"}, + Required: []string{"conditions", "asyncOpInProgress", "reconciledGeneration"}, }, }, Dependencies: []string{ diff --git a/pkg/registry/servicecatalog/binding/strategy.go b/pkg/registry/servicecatalog/binding/strategy.go index 0cf8031c87d..41a24aaab25 100644 --- a/pkg/registry/servicecatalog/binding/strategy.go +++ b/pkg/registry/servicecatalog/binding/strategy.go @@ -19,7 +19,7 @@ package binding // this was copied from where else and edited to fit our objects import ( - //apiequality "k8s.io/apimachinery/pkg/api/equality" + apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" @@ -132,9 +132,12 @@ func (bindingRESTStrategy) PrepareForUpdate(ctx genericapirequest.Context, new, // Spec updates bump the generation so that we can distinguish between // spec changes and other changes to the object. - //if !apiequality.Semantic.DeepEqual(oldServiceInstanceCredential.Spec, newServiceInstanceCredential.Spec) { - // newServiceInstanceCredential.Generation = oldServiceInstanceCredential.Generation + 1 - //} + // + // Note that since we do not currently handle any changes to the spec, + // the generation will never be incremented + if !apiequality.Semantic.DeepEqual(oldServiceInstanceCredential.Spec, newServiceInstanceCredential.Spec) { + newServiceInstanceCredential.Generation = oldServiceInstanceCredential.Generation + 1 + } } func (bindingRESTStrategy) ValidateUpdate(ctx genericapirequest.Context, new, old runtime.Object) field.ErrorList { diff --git a/pkg/registry/servicecatalog/instance/strategy.go b/pkg/registry/servicecatalog/instance/strategy.go index 51788ec8b97..7fdaed176a8 100644 --- a/pkg/registry/servicecatalog/instance/strategy.go +++ b/pkg/registry/servicecatalog/instance/strategy.go @@ -19,7 +19,7 @@ package instance // this was copied from where else and edited to fit our objects import ( - //apiequality "k8s.io/apimachinery/pkg/api/equality" + apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" @@ -135,9 +135,12 @@ func (instanceRESTStrategy) PrepareForUpdate(ctx genericapirequest.Context, new, // Spec updates bump the generation so that we can distinguish between // spec changes and other changes to the object. - //if !apiequality.Semantic.DeepEqual(oldServiceInstance.Spec, newServiceInstance.Spec) { - // newServiceInstance.Generation = oldServiceInstance.Generation + 1 - //} + // + // Note that since we do not currently handle any changes to the spec, + // the generation will never be incremented + if !apiequality.Semantic.DeepEqual(oldServiceInstance.Spec, newServiceInstance.Spec) { + newServiceInstance.Generation = oldServiceInstance.Generation + 1 + } } func (instanceRESTStrategy) ValidateUpdate(ctx genericapirequest.Context, new, old runtime.Object) field.ErrorList {