From d155864806ce5c81a42ece4286263b9890d7458f Mon Sep 17 00:00:00 2001 From: staebler Date: Tue, 5 Sep 2017 16:41:09 -0400 Subject: [PATCH 1/2] Allow updates to ServiceInstance. --- pkg/apis/servicecatalog/types.go | 12 +- .../v1alpha1/types.generated.go | 197 +++-- pkg/apis/servicecatalog/v1alpha1/types.go | 6 + .../v1alpha1/zz_generated.conversion.go | 2 + .../servicecatalog/validation/instance.go | 48 +- .../validation/instance_test.go | 205 ++++- pkg/controller/controller_broker.go | 8 + pkg/controller/controller_instance.go | 288 +++++-- pkg/controller/controller_instance_test.go | 753 +++++++++++++++++- pkg/controller/controller_test.go | 61 +- pkg/openapi/openapi_generated.go | 9 +- .../servicecatalog/instance/strategy.go | 45 +- .../servicecatalog/instance/strategy_test.go | 96 ++- test/integration/clientset_test.go | 84 +- test/integration/controller_test.go | 118 ++- 15 files changed, 1571 insertions(+), 361 deletions(-) diff --git a/pkg/apis/servicecatalog/types.go b/pkg/apis/servicecatalog/types.go index 179525e6e07..c8fb37f3b50 100644 --- a/pkg/apis/servicecatalog/types.go +++ b/pkg/apis/servicecatalog/types.go @@ -374,12 +374,6 @@ type ExtraValue []string // +genclient=true // ServiceInstance represents a provisioned instance of a ServiceClass. -// Currently, the spec field cannot be changed once a ServiceInstance is -// created. Spec changes submitted by users will be ignored. -// -// In the future, this will be allowed and will represent the intention that -// the ServiceInstance should have the plan and/or parameters updated at the -// ServiceBroker. type ServiceInstance struct { metav1.TypeMeta metav1.ObjectMeta @@ -449,6 +443,12 @@ type ServiceInstanceSpec struct { // end-user. User-provided values for this field are not saved. // +optional UserInfo *UserInfo + + // UpdateRequests is a strictly increasing, non-negative integer counter that + // can be manually incremented by a user to manually trigger an update. This + // allows for parameters to be updated with any out-of-band changes that have + // been made to the secrets from which the parameters are sourced. + UpdateRequests int64 } // ServiceInstanceStatus represents the current status of an Instance. diff --git a/pkg/apis/servicecatalog/v1alpha1/types.generated.go b/pkg/apis/servicecatalog/v1alpha1/types.generated.go index 9d455074a30..4547a891bb2 100644 --- a/pkg/apis/servicecatalog/v1alpha1/types.generated.go +++ b/pkg/apis/servicecatalog/v1alpha1/types.generated.go @@ -6856,7 +6856,7 @@ func (x *ServiceInstanceSpec) CodecEncodeSelf(e *codec1978.Encoder) { } else { yysep2 := !z.EncBinary() yy2arr2 := z.EncBasicHandle().StructToArray - var yyq2 [8]bool + var yyq2 [9]bool _, _, _ = yysep2, yyq2, yy2arr2 const yyr2 bool = false yyq2[1] = x.ExternalServicePlanName != "" @@ -6867,9 +6867,9 @@ func (x *ServiceInstanceSpec) CodecEncodeSelf(e *codec1978.Encoder) { yyq2[7] = x.UserInfo != nil var yynn2 int if yyr2 || yy2arr2 { - r.EncodeArrayStart(8) + r.EncodeArrayStart(9) } else { - yynn2 = 2 + yynn2 = 3 for _, b := range yyq2 { if b { yynn2++ @@ -7082,6 +7082,25 @@ func (x *ServiceInstanceSpec) CodecEncodeSelf(e *codec1978.Encoder) { } } } + if yyr2 || yy2arr2 { + z.EncSendContainerState(codecSelfer_containerArrayElem1234) + yym28 := z.EncBinary() + _ = yym28 + if false { + } else { + r.EncodeInt(int64(x.UpdateRequests)) + } + } else { + z.EncSendContainerState(codecSelfer_containerMapKey1234) + r.EncodeString(codecSelferC_UTF81234, string("updateRequests")) + z.EncSendContainerState(codecSelfer_containerMapValue1234) + yym29 := z.EncBinary() + _ = yym29 + if false { + } else { + r.EncodeInt(int64(x.UpdateRequests)) + } + } if yyr2 || yy2arr2 { z.EncSendContainerState(codecSelfer_containerArrayEnd1234) } else { @@ -7243,6 +7262,18 @@ func (x *ServiceInstanceSpec) codecDecodeSelfFromMap(l int, d *codec1978.Decoder } x.UserInfo.CodecDecodeSelf(d) } + case "updateRequests": + if r.TryDecodeAsNil() { + x.UpdateRequests = 0 + } else { + yyv17 := &x.UpdateRequests + yym18 := z.DecBinary() + _ = yym18 + if false { + } else { + *((*int64)(yyv17)) = int64(r.DecodeInt(64)) + } + } default: z.DecStructFieldNotFound(-1, yys3) } // end switch yys3 @@ -7254,16 +7285,16 @@ func (x *ServiceInstanceSpec) codecDecodeSelfFromArray(l int, d *codec1978.Decod var h codecSelfer1234 z, r := codec1978.GenHelperDecoder(d) _, _, _ = h, z, r - var yyj17 int - var yyb17 bool - var yyhl17 bool = l >= 0 - yyj17++ - if yyhl17 { - yyb17 = yyj17 > l + var yyj19 int + var yyb19 bool + var yyhl19 bool = l >= 0 + yyj19++ + if yyhl19 { + yyb19 = yyj19 > l } else { - yyb17 = r.CheckBreak() + yyb19 = r.CheckBreak() } - if yyb17 { + if yyb19 { z.DecSendContainerState(codecSelfer_containerArrayEnd1234) return } @@ -7271,21 +7302,21 @@ func (x *ServiceInstanceSpec) codecDecodeSelfFromArray(l int, d *codec1978.Decod if r.TryDecodeAsNil() { x.ExternalServiceClassName = "" } else { - yyv18 := &x.ExternalServiceClassName - yym19 := z.DecBinary() - _ = yym19 + yyv20 := &x.ExternalServiceClassName + yym21 := z.DecBinary() + _ = yym21 if false { } else { - *((*string)(yyv18)) = r.DecodeString() + *((*string)(yyv20)) = r.DecodeString() } } - yyj17++ - if yyhl17 { - yyb17 = yyj17 > l + yyj19++ + if yyhl19 { + yyb19 = yyj19 > l } else { - yyb17 = r.CheckBreak() + yyb19 = r.CheckBreak() } - if yyb17 { + if yyb19 { z.DecSendContainerState(codecSelfer_containerArrayEnd1234) return } @@ -7293,21 +7324,21 @@ func (x *ServiceInstanceSpec) codecDecodeSelfFromArray(l int, d *codec1978.Decod if r.TryDecodeAsNil() { x.ExternalServicePlanName = "" } else { - yyv20 := &x.ExternalServicePlanName - yym21 := z.DecBinary() - _ = yym21 + yyv22 := &x.ExternalServicePlanName + yym23 := z.DecBinary() + _ = yym23 if false { } else { - *((*string)(yyv20)) = r.DecodeString() + *((*string)(yyv22)) = r.DecodeString() } } - yyj17++ - if yyhl17 { - yyb17 = yyj17 > l + yyj19++ + if yyhl19 { + yyb19 = yyj19 > l } else { - yyb17 = r.CheckBreak() + yyb19 = r.CheckBreak() } - if yyb17 { + if yyb19 { z.DecSendContainerState(codecSelfer_containerArrayEnd1234) return } @@ -7322,13 +7353,13 @@ func (x *ServiceInstanceSpec) codecDecodeSelfFromArray(l int, d *codec1978.Decod } x.ServiceClassRef.CodecDecodeSelf(d) } - yyj17++ - if yyhl17 { - yyb17 = yyj17 > l + yyj19++ + if yyhl19 { + yyb19 = yyj19 > l } else { - yyb17 = r.CheckBreak() + yyb19 = r.CheckBreak() } - if yyb17 { + if yyb19 { z.DecSendContainerState(codecSelfer_containerArrayEnd1234) return } @@ -7343,13 +7374,13 @@ func (x *ServiceInstanceSpec) codecDecodeSelfFromArray(l int, d *codec1978.Decod } x.ServicePlanRef.CodecDecodeSelf(d) } - yyj17++ - if yyhl17 { - yyb17 = yyj17 > l + yyj19++ + if yyhl19 { + yyb19 = yyj19 > l } else { - yyb17 = r.CheckBreak() + yyb19 = r.CheckBreak() } - if yyb17 { + if yyb19 { z.DecSendContainerState(codecSelfer_containerArrayEnd1234) return } @@ -7362,23 +7393,23 @@ func (x *ServiceInstanceSpec) codecDecodeSelfFromArray(l int, d *codec1978.Decod if x.Parameters == nil { x.Parameters = new(pkg4_runtime.RawExtension) } - yym25 := z.DecBinary() - _ = yym25 + yym27 := z.DecBinary() + _ = yym27 if false { } else if z.HasExtensions() && z.DecExt(x.Parameters) { - } else if !yym25 && z.IsJSONHandle() { + } else if !yym27 && z.IsJSONHandle() { z.DecJSONUnmarshal(x.Parameters) } else { z.DecFallback(x.Parameters, false) } } - yyj17++ - if yyhl17 { - yyb17 = yyj17 > l + yyj19++ + if yyhl19 { + yyb19 = yyj19 > l } else { - yyb17 = r.CheckBreak() + yyb19 = r.CheckBreak() } - if yyb17 { + if yyb19 { z.DecSendContainerState(codecSelfer_containerArrayEnd1234) return } @@ -7386,21 +7417,21 @@ func (x *ServiceInstanceSpec) codecDecodeSelfFromArray(l int, d *codec1978.Decod if r.TryDecodeAsNil() { x.ParametersFrom = nil } else { - yyv26 := &x.ParametersFrom - yym27 := z.DecBinary() - _ = yym27 + yyv28 := &x.ParametersFrom + yym29 := z.DecBinary() + _ = yym29 if false { } else { - h.decSliceParametersFromSource((*[]ParametersFromSource)(yyv26), d) + h.decSliceParametersFromSource((*[]ParametersFromSource)(yyv28), d) } } - yyj17++ - if yyhl17 { - yyb17 = yyj17 > l + yyj19++ + if yyhl19 { + yyb19 = yyj19 > l } else { - yyb17 = r.CheckBreak() + yyb19 = r.CheckBreak() } - if yyb17 { + if yyb19 { z.DecSendContainerState(codecSelfer_containerArrayEnd1234) return } @@ -7408,21 +7439,21 @@ func (x *ServiceInstanceSpec) codecDecodeSelfFromArray(l int, d *codec1978.Decod if r.TryDecodeAsNil() { x.ExternalID = "" } else { - yyv28 := &x.ExternalID - yym29 := z.DecBinary() - _ = yym29 + yyv30 := &x.ExternalID + yym31 := z.DecBinary() + _ = yym31 if false { } else { - *((*string)(yyv28)) = r.DecodeString() + *((*string)(yyv30)) = r.DecodeString() } } - yyj17++ - if yyhl17 { - yyb17 = yyj17 > l + yyj19++ + if yyhl19 { + yyb19 = yyj19 > l } else { - yyb17 = r.CheckBreak() + yyb19 = r.CheckBreak() } - if yyb17 { + if yyb19 { z.DecSendContainerState(codecSelfer_containerArrayEnd1234) return } @@ -7437,18 +7468,40 @@ func (x *ServiceInstanceSpec) codecDecodeSelfFromArray(l int, d *codec1978.Decod } x.UserInfo.CodecDecodeSelf(d) } + yyj19++ + if yyhl19 { + yyb19 = yyj19 > l + } else { + yyb19 = r.CheckBreak() + } + if yyb19 { + z.DecSendContainerState(codecSelfer_containerArrayEnd1234) + return + } + z.DecSendContainerState(codecSelfer_containerArrayElem1234) + if r.TryDecodeAsNil() { + x.UpdateRequests = 0 + } else { + yyv33 := &x.UpdateRequests + yym34 := z.DecBinary() + _ = yym34 + if false { + } else { + *((*int64)(yyv33)) = int64(r.DecodeInt(64)) + } + } for { - yyj17++ - if yyhl17 { - yyb17 = yyj17 > l + yyj19++ + if yyhl19 { + yyb19 = yyj19 > l } else { - yyb17 = r.CheckBreak() + yyb19 = r.CheckBreak() } - if yyb17 { + if yyb19 { break } z.DecSendContainerState(codecSelfer_containerArrayElem1234) - z.DecStructFieldNotFound(yyj17-1, "") + z.DecStructFieldNotFound(yyj19-1, "") } z.DecSendContainerState(codecSelfer_containerArrayEnd1234) } @@ -12422,7 +12475,7 @@ func (x codecSelfer1234) decSliceServiceInstance(v *[]ServiceInstance, d *codec1 yyrg1 := len(yyv1) > 0 yyv21 := yyv1 - yyrl1, yyrt1 = z.DecInferLen(yyl1, z.DecBasicHandle().MaxInitLen, 464) + yyrl1, yyrt1 = z.DecInferLen(yyl1, z.DecBasicHandle().MaxInitLen, 472) if yyrt1 { if yyrl1 <= cap(yyv1) { yyv1 = yyv1[:yyrl1] diff --git a/pkg/apis/servicecatalog/v1alpha1/types.go b/pkg/apis/servicecatalog/v1alpha1/types.go index 9ab60dc3212..d792b1b4669 100644 --- a/pkg/apis/servicecatalog/v1alpha1/types.go +++ b/pkg/apis/servicecatalog/v1alpha1/types.go @@ -448,6 +448,12 @@ type ServiceInstanceSpec struct { // end-user. User-provided values for this field are not saved. // +optional UserInfo *UserInfo `json:"userInfo,omitempty"` + + // UpdateRequests is a strictly increasing, non-negative integer counter that + // can be manually incremented by a user to manually trigger an update. This + // allows for parameters to be updated with any out-of-band changes that have + // been made to the secrets from which the parameters are sourced. + UpdateRequests int64 `json:"updateRequests"` } // ServiceInstanceStatus represents the current status of an Instance. diff --git a/pkg/apis/servicecatalog/v1alpha1/zz_generated.conversion.go b/pkg/apis/servicecatalog/v1alpha1/zz_generated.conversion.go index 2a59bb33502..2d4992b0e51 100644 --- a/pkg/apis/servicecatalog/v1alpha1/zz_generated.conversion.go +++ b/pkg/apis/servicecatalog/v1alpha1/zz_generated.conversion.go @@ -735,6 +735,7 @@ func autoConvert_v1alpha1_ServiceInstanceSpec_To_servicecatalog_ServiceInstanceS out.ParametersFrom = *(*[]servicecatalog.ParametersFromSource)(unsafe.Pointer(&in.ParametersFrom)) out.ExternalID = in.ExternalID out.UserInfo = (*servicecatalog.UserInfo)(unsafe.Pointer(in.UserInfo)) + out.UpdateRequests = in.UpdateRequests return nil } @@ -752,6 +753,7 @@ func autoConvert_servicecatalog_ServiceInstanceSpec_To_v1alpha1_ServiceInstanceS out.ParametersFrom = *(*[]ParametersFromSource)(unsafe.Pointer(&in.ParametersFrom)) out.ExternalID = in.ExternalID out.UserInfo = (*UserInfo)(unsafe.Pointer(in.UserInfo)) + out.UpdateRequests = in.UpdateRequests return nil } diff --git a/pkg/apis/servicecatalog/validation/instance.go b/pkg/apis/servicecatalog/validation/instance.go index b01c6860615..d34009aac35 100644 --- a/pkg/apis/servicecatalog/validation/instance.go +++ b/pkg/apis/servicecatalog/validation/instance.go @@ -107,6 +107,8 @@ func validateServiceInstanceSpec(spec *sc.ServiceInstanceSpec, fldPath *field.Pa } } + allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(spec.UpdateRequests, fldPath.Child("updateRequests"))...) + return allErrs } @@ -205,6 +207,12 @@ func validateServiceInstanceCreate(instance *sc.ServiceInstance) field.ErrorList if instance.Status.ReconciledGeneration >= instance.Generation { allErrs = append(allErrs, field.Invalid(field.NewPath("status").Child("reconciledGeneration"), instance.Status.ReconciledGeneration, "reconciledGeneration must be less than generation on create")) } + if instance.Spec.ServiceClassRef != nil { + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec").Child("serviceClassRef"), "serviceClassRef must not be present on create")) + } + if instance.Spec.ServicePlanRef != nil { + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec").Child("servicePlanRef"), "servicePlanRef must not be present on create")) + } return allErrs } @@ -217,6 +225,14 @@ func validateServiceInstanceUpdate(instance *sc.ServiceInstance) field.ErrorList } else if instance.Status.ReconciledGeneration > instance.Generation { allErrs = append(allErrs, field.Invalid(field.NewPath("status").Child("reconciledGeneration"), instance.Status.ReconciledGeneration, "reconciledGeneration must not be greater than generation")) } + if instance.Status.CurrentOperation != "" { + if instance.Spec.ServiceClassRef == nil { + allErrs = append(allErrs, field.Required(field.NewPath("spec").Child("servceClassRef"), "serviceClassRef is required when currentOperation is present")) + } + if instance.Spec.ServicePlanRef == nil { + allErrs = append(allErrs, field.Required(field.NewPath("spec").Child("servicePlanRef"), "servicePlanRef is required when currentOperation is present")) + } + } return allErrs } @@ -228,14 +244,26 @@ func internalValidateServiceInstanceUpdateAllowed(new *sc.ServiceInstance, old * if old.Generation != new.Generation && old.Status.ReconciledGeneration != old.Generation { errors = append(errors, field.Forbidden(field.NewPath("spec"), "Another update for this service instance is in progress")) } + if old.Spec.ExternalServicePlanName != new.Spec.ExternalServicePlanName && new.Spec.ServicePlanRef != nil { + errors = append(errors, field.Forbidden(field.NewPath("spec").Child("servicePlanRef"), "servicePlanRef must not be present when externalServicePlanName is being changed")) + } return errors } // ValidateServiceInstanceUpdate validates a change to the Instance's spec. func ValidateServiceInstanceUpdate(new *sc.ServiceInstance, old *sc.ServiceInstance) field.ErrorList { allErrs := field.ErrorList{} + allErrs = append(allErrs, internalValidateServiceInstanceUpdateAllowed(new, old)...) allErrs = append(allErrs, internalValidateServiceInstance(new, false)...) + + allErrs = append(allErrs, apivalidation.ValidateImmutableField(new.Spec.ExternalServiceClassName, old.Spec.ExternalServiceClassName, field.NewPath("spec").Child("externalServiceClassName"))...) + allErrs = append(allErrs, apivalidation.ValidateImmutableField(new.Spec.ExternalID, old.Spec.ExternalID, field.NewPath("spec").Child("externalID"))...) + + if new.Spec.UpdateRequests < old.Spec.UpdateRequests { + allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("updateRequests"), new.Spec.UpdateRequests, "new updateRequests value must not be less than the old one")) + } + return allErrs } @@ -247,9 +275,23 @@ func internalValidateServiceInstanceStatusUpdateAllowed(new *sc.ServiceInstance, } func internalValidateServiceInstanceReferencesUpdateAllowed(new *sc.ServiceInstance, old *sc.ServiceInstance) field.ErrorList { - errors := field.ErrorList{} - // TODO what would be errors? - return errors + allErrs := field.ErrorList{} + if new.Status.CurrentOperation != "" { + allErrs = append(allErrs, field.Forbidden(field.NewPath("status").Child("currentOperation"), "cannot update references when currentOperation is present")) + } + if new.Spec.ServiceClassRef == nil { + allErrs = append(allErrs, field.Required(field.NewPath("spec").Child("servceClassRef"), "serviceClassRef is required when updating references")) + } + if new.Spec.ServicePlanRef == nil { + allErrs = append(allErrs, field.Required(field.NewPath("spec").Child("servicePlanRef"), "servicePlanRef is required when updating references")) + } + if old.Spec.ServiceClassRef != nil { + allErrs = append(allErrs, apivalidation.ValidateImmutableField(new.Spec.ServiceClassRef, old.Spec.ServiceClassRef, field.NewPath("spec").Child("serviceClassRef"))...) + } + if old.Spec.ServicePlanRef != nil { + allErrs = append(allErrs, apivalidation.ValidateImmutableField(new.Spec.ServicePlanRef, old.Spec.ServicePlanRef, field.NewPath("spec").Child("servicePlanRef"))...) + } + return allErrs } // ValidateServiceInstanceStatusUpdate checks that when changing from an older diff --git a/pkg/apis/servicecatalog/validation/instance_test.go b/pkg/apis/servicecatalog/validation/instance_test.go index 7af8dfb36ba..12aaa369409 100644 --- a/pkg/apis/servicecatalog/validation/instance_test.go +++ b/pkg/apis/servicecatalog/validation/instance_test.go @@ -22,15 +22,17 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + apiv1 "k8s.io/client-go/pkg/api/v1" "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog" ) -func validServiceInstance() *servicecatalog.ServiceInstance { +func validServiceInstanceForCreate() *servicecatalog.ServiceInstance { return &servicecatalog.ServiceInstance{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-instance", - Namespace: "test-ns", + Name: "test-instance", + Namespace: "test-ns", + Generation: 1, }, Spec: servicecatalog.ServiceInstanceSpec{ ExternalServiceClassName: "test-serviceclass", @@ -39,6 +41,13 @@ func validServiceInstance() *servicecatalog.ServiceInstance { } } +func validServiceInstance() *servicecatalog.ServiceInstance { + instance := validServiceInstanceForCreate() + instance.Spec.ServiceClassRef = &apiv1.ObjectReference{} + instance.Spec.ServicePlanRef = &apiv1.ObjectReference{} + return instance +} + func validServiceInstanceWithInProgressProvision() *servicecatalog.ServiceInstance { instance := validServiceInstance() instance.Generation = 2 @@ -385,22 +394,15 @@ func TestValidateServiceInstance(t *testing.T) { valid: false, }, { - name: "valid create", - instance: func() *servicecatalog.ServiceInstance { - i := validServiceInstance() - i.Generation = 1 - i.Status.ReconciledGeneration = 0 - return i - }(), - create: true, - valid: true, + name: "valid create", + instance: validServiceInstanceForCreate(), + create: true, + valid: true, }, { name: "create with operation in-progress", instance: func() *servicecatalog.ServiceInstance { - i := validServiceInstance() - i.Generation = 1 - i.Status.ReconciledGeneration = 0 + i := validServiceInstanceForCreate() i.Status.CurrentOperation = servicecatalog.ServiceInstanceOperationProvision return i }(), @@ -410,8 +412,7 @@ func TestValidateServiceInstance(t *testing.T) { { name: "create with invalid reconciled generation", instance: func() *servicecatalog.ServiceInstance { - i := validServiceInstance() - i.Generation = 1 + i := validServiceInstanceForCreate() i.Status.ReconciledGeneration = 1 return i }(), @@ -422,13 +423,32 @@ func TestValidateServiceInstance(t *testing.T) { name: "update with invalid reconciled generation", instance: func() *servicecatalog.ServiceInstance { i := validServiceInstance() - i.Generation = 1 i.Status.ReconciledGeneration = 2 return i }(), create: false, valid: false, }, + { + name: "in-progress operation with missing service class ref", + instance: func() *servicecatalog.ServiceInstance { + i := validServiceInstanceWithInProgressProvision() + i.Spec.ServiceClassRef = nil + return i + }(), + create: false, + valid: false, + }, + { + name: "in-progress operation with missing service plan ref", + instance: func() *servicecatalog.ServiceInstance { + i := validServiceInstanceWithInProgressProvision() + i.Spec.ServicePlanRef = nil + return i + }(), + create: false, + valid: false, + }, } for _, tc := range cases { @@ -520,6 +540,74 @@ func TestInternalValidateServiceInstanceUpdateAllowed(t *testing.T) { } } +func TestInternalValidateServiceInstanceUpdateAllowedForPlanChange(t *testing.T) { + cases := []struct { + name string + oldPlan string + newPlan string + newPlanRef *apiv1.ObjectReference + valid bool + }{ + { + name: "valid plan change", + oldPlan: "old-plan", + newPlan: "new-plan", + newPlanRef: nil, + valid: true, + }, + { + name: "plan ref not cleared", + oldPlan: "old-plan", + newPlan: "new-plan", + newPlanRef: &apiv1.ObjectReference{}, + valid: false, + }, + { + name: "no plan change", + oldPlan: "plan-name", + newPlan: "plan-name", + newPlanRef: &apiv1.ObjectReference{}, + valid: true, + }, + } + + for _, tc := range cases { + oldInstance := &servicecatalog.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-instance", + Namespace: "test-ns", + }, + Spec: servicecatalog.ServiceInstanceSpec{ + ExternalServiceClassName: "test-serviceclass", + ExternalServicePlanName: tc.oldPlan, + ServiceClassRef: &apiv1.ObjectReference{}, + ServicePlanRef: &apiv1.ObjectReference{}, + }, + } + + newInstance := &servicecatalog.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-instance", + Namespace: "test-ns", + }, + Spec: servicecatalog.ServiceInstanceSpec{ + ExternalServiceClassName: "test-serviceclass", + ExternalServicePlanName: tc.newPlan, + ServiceClassRef: &apiv1.ObjectReference{}, + ServicePlanRef: tc.newPlanRef, + }, + } + + errs := internalValidateServiceInstanceUpdateAllowed(newInstance, oldInstance) + if len(errs) != 0 && tc.valid { + t.Errorf("%v: unexpected error: %v", tc.name, errs) + continue + } else if len(errs) == 0 && !tc.valid { + t.Errorf("%v: unexpected success", tc.name) + } + } +} + func TestValidateServiceInstanceStatusUpdate(t *testing.T) { now := metav1.Now() cases := []struct { @@ -656,6 +744,8 @@ func TestValidateServiceInstanceStatusUpdate(t *testing.T) { Spec: servicecatalog.ServiceInstanceSpec{ ExternalServiceClassName: "test-serviceclass", ExternalServicePlanName: "test-plan", + ServiceClassRef: &apiv1.ObjectReference{}, + ServicePlanRef: &apiv1.ObjectReference{}, }, Status: *tc.old, } @@ -669,6 +759,8 @@ func TestValidateServiceInstanceStatusUpdate(t *testing.T) { Spec: servicecatalog.ServiceInstanceSpec{ ExternalServiceClassName: "test-serviceclass", ExternalServicePlanName: "test-plan", + ServiceClassRef: &apiv1.ObjectReference{}, + ServicePlanRef: &apiv1.ObjectReference{}, }, Status: *tc.new, } @@ -690,3 +782,80 @@ func TestValidateServiceInstanceStatusUpdate(t *testing.T) { } } } + +func TestValidateServiceInstanceReferencesUpdate(t *testing.T) { + cases := []struct { + name string + old *servicecatalog.ServiceInstance + new *servicecatalog.ServiceInstance + valid bool + }{ + { + name: "valid class and plan update", + old: func() *servicecatalog.ServiceInstance { + i := validServiceInstance() + i.Spec.ServiceClassRef = nil + i.Spec.ServicePlanRef = nil + return i + }(), + new: validServiceInstance(), + valid: true, + }, + { + name: "invalid class update", + old: validServiceInstance(), + new: func() *servicecatalog.ServiceInstance { + i := validServiceInstance() + i.Spec.ServiceClassRef = &apiv1.ObjectReference{ + Name: "new-class-name", + } + return i + }(), + valid: false, + }, + { + name: "direct update to plan ref", + old: validServiceInstance(), + new: func() *servicecatalog.ServiceInstance { + i := validServiceInstance() + i.Spec.ServicePlanRef = &apiv1.ObjectReference{ + Name: "new-plan-name", + } + return i + }(), + valid: false, + }, + { + name: "valid plan update from name change", + old: func() *servicecatalog.ServiceInstance { + i := validServiceInstance() + i.Spec.ServicePlanRef = nil + return i + }(), + new: func() *servicecatalog.ServiceInstance { + i := validServiceInstance() + i.Spec.ServicePlanRef = &apiv1.ObjectReference{ + Name: "new-plan-name", + } + return i + }(), + valid: true, + }, + { + name: "in-progress operation", + old: validServiceInstanceWithInProgressProvision(), + new: validServiceInstanceWithInProgressProvision(), + valid: false, + }, + } + + for _, tc := range cases { + errs := ValidateServiceInstanceReferencesUpdate(tc.new, tc.old) + if len(errs) != 0 && tc.valid { + t.Errorf("%v: unexpected error: %v", tc.name, errs) + continue + } else if len(errs) == 0 && !tc.valid { + t.Errorf("%v: unexpected success", tc.name) + } + } +} diff --git a/pkg/controller/controller_broker.go b/pkg/controller/controller_broker.go index 3ae83651442..45d4d9b3832 100644 --- a/pkg/controller/controller_broker.go +++ b/pkg/controller/controller_broker.go @@ -86,6 +86,8 @@ const ( errorFindingNamespaceServiceInstanceReason string = "ErrorFindingNamespaceForInstance" errorProvisionCallFailedReason string = "ProvisionCallFailed" errorErrorCallingProvisionReason string = "ErrorCallingProvision" + errorUpdateInstanceCallFailedReason string = "UpdateInstanceCallFailed" + errorErrorCallingUpdateInstanceReason string = "ErrorCallingUpdateInstance" errorDeprovisionCalledReason string = "DeprovisionCallFailed" errorDeprovisionBlockedByCredentialsReason string = "DeprovisionBlockedByExistingCredentials" errorBindCallReason string = "BindCallFailed" @@ -107,6 +109,8 @@ const ( successInjectedBindResultMessage string = "Injected bind result" successDeprovisionReason string = "DeprovisionedSuccessfully" successDeprovisionMessage string = "The instance was deprovisioned successfully" + successUpdateInstanceReason string = "InstanceUpdatedSuccessfully" + successUpdateInstanceMessage string = "The instance was updated successfully" successProvisionReason string = "ProvisionedSuccessfully" successProvisionMessage string = "The instance was provisioned successfully" successFetchedCatalogReason string = "FetchedCatalog" @@ -117,6 +121,8 @@ const ( successOrphanMitigationReason string = "OrphanMitigationSuccessful" asyncProvisioningReason string = "Provisioning" asyncProvisioningMessage string = "The instance is being provisioned asynchronously" + asyncUpdatingInstanceReason string = "UpdatingInstance" + asyncUpdatingInstanceMessage string = "The instance is being updated asynchronously" asyncDeprovisioningReason string = "Deprovisioning" asyncDeprovisioningMessage string = "The instance is being deprovisioned asynchronously" bindingInFlightReason string = "BindingRequestInFlight" @@ -125,6 +131,8 @@ const ( unbindingInFlightMessage string = "Unbind request for ServiceInstanceCredential in-flight to Broker" provisioningInFlightReason string = "ProvisionRequestInFlight" provisioningInFlightMessage string = "Provision request for ServiceInstance in-flight to Broker" + instanceUpdatingInFlightReason string = "UpdateInstanceRequestInFlight" + instanceUpdatingInFlightMessage string = "Update request for ServiceInstance in-flight to Broker" deprovisioningInFlightReason string = "DeprovisionRequestInFlight" deprovisioningInFlightMessage string = "Deprovision request for ServiceInstance in-flight to Broker" ) diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index 80fc29e6eca..a3b6cc6b358 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -615,25 +615,9 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance UserInfo: instance.Spec.UserInfo, } - request := &osb.ProvisionRequest{ - AcceptsIncomplete: true, - InstanceID: instance.Spec.ExternalID, - ServiceID: serviceClass.Spec.ExternalID, - PlanID: servicePlan.Spec.ExternalID, - Parameters: parameters, - OrganizationGUID: string(ns.UID), - SpaceGUID: string(ns.UID), - } - - // osb client handles whether or not to really send this based - // on the version of the client. - request.Context = map[string]interface{}{ - "platform": ContextProfilePlatformKubernetes, - "namespace": instance.Namespace, - } - + var originatingIdentity *osb.AlphaOriginatingIdentity if utilfeature.DefaultFeatureGate.Enabled(scfeatures.OriginatingIdentity) { - originatingIdentity, err := buildOriginatingIdentity(instance.Spec.UserInfo) + originatingIdentity, err = buildOriginatingIdentity(instance.Spec.UserInfo) if err != nil { s := fmt.Sprintf(`Error building originating identity headers for provisioning ServiceInstance "%v/%v": %v`, instance.Namespace, instance.Name, err) glog.Warning(s) @@ -652,11 +636,67 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance return err } - request.OriginatingIdentity = originatingIdentity + } + + var ( + isProvisioning bool + provisionRequest *osb.ProvisionRequest + updateRequest *osb.UpdateInstanceRequest + currentOperation v1alpha1.ServiceInstanceOperation + provisionOrUpdateText string + provisionedOrUpdatedText string + provisioningOrUpdatingText string + ) + if toUpdate.Status.ReconciledGeneration == 0 { + isProvisioning = true + // osb client handles whether or not to really send this based + // on the version of the client. + requestContext := map[string]interface{}{ + "platform": ContextProfilePlatformKubernetes, + "namespace": instance.Namespace, + } + provisionRequest = &osb.ProvisionRequest{ + AcceptsIncomplete: true, + InstanceID: instance.Spec.ExternalID, + ServiceID: serviceClass.Spec.ExternalID, + PlanID: servicePlan.Spec.ExternalID, + Parameters: parameters, + OrganizationGUID: string(ns.UID), + SpaceGUID: string(ns.UID), + Context: requestContext, + OriginatingIdentity: originatingIdentity, + } + currentOperation = v1alpha1.ServiceInstanceOperationProvision + provisionOrUpdateText = "provision" + provisionedOrUpdatedText = "provisioned" + provisioningOrUpdatingText = "provisioning" + } else { + isProvisioning = false + updateRequest = &osb.UpdateInstanceRequest{ + AcceptsIncomplete: true, + InstanceID: instance.Spec.ExternalID, + ServiceID: serviceClass.Spec.ExternalID, + OriginatingIdentity: originatingIdentity, + } + // Only send the plan ID if the plan name has changed from what the Broker has + if toUpdate.Status.ExternalProperties == nil || + toUpdate.Status.InProgressProperties.ExternalServicePlanName != toUpdate.Status.ExternalProperties.ExternalServicePlanName { + planID := servicePlan.Spec.ExternalID + updateRequest.PlanID = &planID + } + // Only send the parameters if they have changed from what the Broker has + if toUpdate.Status.ExternalProperties == nil || + toUpdate.Status.InProgressProperties.ParametersChecksum != toUpdate.Status.ExternalProperties.ParametersChecksum { + updateRequest.Parameters = parameters + } + currentOperation = v1alpha1.ServiceInstanceOperationUpdate + provisionOrUpdateText = "update" + provisionedOrUpdatedText = "updated" + provisioningOrUpdatingText = "updating" } if toUpdate.Status.CurrentOperation == "" { - toUpdate, err = c.recordStartOfServiceInstanceOperation(toUpdate, v1alpha1.ServiceInstanceOperationProvision) + toUpdate, err = c.recordStartOfServiceInstanceOperation(toUpdate, currentOperation) if err != nil { // There has been an update to the instance. Start reconciliation // over with a fresh view of the instance. @@ -664,18 +704,29 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance } } - glog.V(4).Infof("Provisioning a new ServiceInstance %v/%v of ServiceClass %v at ClusterServiceBroker %v", instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName) - response, err := brokerClient.ProvisionInstance(request) + var provisionResponse *osb.ProvisionResponse + var updateResponse *osb.UpdateInstanceResponse + if isProvisioning { + glog.V(4).Infof("Provisioning a new ServiceInstance %v/%v of ServiceClass %v at ClusterServiceBroker %v", instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName) + provisionResponse, err = brokerClient.ProvisionInstance(provisionRequest) + } else { + glog.V(4).Infof("Updating ServiceInstance %v/%v of ServiceClass %v at ClusterServiceBroker %v", instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName) + updateResponse, err = brokerClient.UpdateInstance(updateRequest) + } if err != nil { // There are two buckets of errors to handle: // 1. Errors that represent a failure response from the broker // 2. All other errors if httpErr, ok := osb.IsHTTPError(err); ok { + reason := errorProvisionCallFailedReason + if !isProvisioning { + reason = errorUpdateInstanceCallFailedReason + } // An error from the broker represents a permanent failure and // should not be retried; set the Failed condition. - s := fmt.Sprintf("Error provisioning ServiceInstance \"%s/%s\" of ServiceClass %q at ClusterServiceBroker %q: %s", instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName, httpErr) + s := fmt.Sprintf("Error %v ServiceInstance \"%s/%s\" of ServiceClass %q at ClusterServiceBroker %q: %s", provisioningOrUpdatingText, instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName, httpErr) glog.Warning(s) - c.recorder.Event(instance, apiv1.EventTypeWarning, errorProvisionCallFailedReason, s) + c.recorder.Event(instance, apiv1.EventTypeWarning, reason, s) setServiceInstanceCondition( toUpdate, @@ -687,8 +738,8 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance toUpdate, v1alpha1.ServiceInstanceConditionReady, v1alpha1.ConditionFalse, - errorProvisionCallFailedReason, - "ClusterServiceBroker returned a failure for provision call; operation will not be retried: "+s) + reason, + fmt.Sprintf("ClusterServiceBroker returned a failure for %v call; operation will not be retried: %v", provisionOrUpdateText, s)) if shouldStartOrphanMitigation(httpErr.StatusCode) { setServiceInstanceStartOrphanMitigation(toUpdate) @@ -709,9 +760,13 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance return nil } - s := fmt.Sprintf("Error provisioning ServiceInstance \"%s/%s\" of ServiceClass %q at ClusterServiceBroker %q: %s", instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName, err) + reason := errorErrorCallingProvisionReason + if !isProvisioning { + reason = errorErrorCallingUpdateInstanceReason + } + s := fmt.Sprintf("Error %v ServiceInstance \"%s/%s\" of ServiceClass %q at ClusterServiceBroker %q: %s", provisioningOrUpdatingText, instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName, err) glog.Warning(s) - c.recorder.Event(instance, apiv1.EventTypeWarning, errorErrorCallingProvisionReason, s) + c.recorder.Event(instance, apiv1.EventTypeWarning, reason, s) urlErr, ok := err.(*url.Error) if ok && urlErr.Timeout() { @@ -742,8 +797,8 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance toUpdate, v1alpha1.ServiceInstanceConditionReady, v1alpha1.ConditionFalse, - errorErrorCallingProvisionReason, - "Provision call failed and will be retried: "+s) + reason, + fmt.Sprintf("The %v call failed and will be retried: %v", provisionOrUpdateText, s)) if !time.Now().Before(toUpdate.Status.OperationStartTime.Time.Add(c.reconciliationRetryDuration)) { s := fmt.Sprintf(`Stopping reconciliation retries on ServiceInstance "%v/%v" because too much time has elapsed`, instance.Namespace, instance.Name) @@ -768,8 +823,8 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance return err } - if response.DashboardURL != nil && *response.DashboardURL != "" { - url := *response.DashboardURL + if isProvisioning && provisionResponse.DashboardURL != nil && *provisionResponse.DashboardURL != "" { + url := *provisionResponse.DashboardURL toUpdate.Status.DashboardURL = &url } @@ -778,10 +833,26 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance // and we need to add it to the polling queue. ClusterServiceBroker can // optionally return 'Operation' that will then need to be // passed back to the broker during polling of last_operation. - if response.Async { - glog.V(5).Infof("Received asynchronous provisioning response for ServiceInstance %v/%v of ServiceClass %v at ClusterServiceBroker %v: response: %+v", instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName, response) - if response.OperationKey != nil && *response.OperationKey != "" { - key := string(*response.OperationKey) + var response interface{} + async := false + if isProvisioning { + response = provisionResponse + async = provisionResponse.Async + } else { + response = updateResponse + async = updateResponse.Async + } + if async { + glog.V(5).Infof("Received asynchronous %v response for ServiceInstance %v/%v of ServiceClass %v at ClusterServiceBroker %v: response: %+v", provisioningOrUpdatingText, instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName, response) + + var operationKey *osb.OperationKey + if isProvisioning { + operationKey = provisionResponse.OperationKey + } else { + operationKey = updateResponse.OperationKey + } + if operationKey != nil && *operationKey != "" { + key := string(*operationKey) toUpdate.Status.LastOperation = &key } @@ -789,12 +860,18 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance // no other operations against it can start. toUpdate.Status.AsyncOpInProgress = true + reason := asyncProvisioningReason + message := asyncProvisioningMessage + if !isProvisioning { + reason = asyncUpdatingInstanceReason + message = asyncUpdatingInstanceMessage + } setServiceInstanceCondition( toUpdate, v1alpha1.ServiceInstanceConditionReady, v1alpha1.ConditionFalse, - asyncProvisioningReason, - asyncProvisioningMessage, + reason, + message, ) if _, err := c.updateServiceInstanceStatus(toUpdate); err != nil { return err @@ -804,9 +881,15 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance return err } - c.recorder.Eventf(instance, apiv1.EventTypeNormal, asyncProvisioningReason, asyncProvisioningMessage) + c.recorder.Eventf(instance, apiv1.EventTypeNormal, reason, message) } else { - glog.V(5).Infof("Successfully provisioned ServiceInstance %v/%v of ServiceClass %v at ClusterServiceBroker %v: response: %+v", instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName, response) + reason := successProvisionReason + message := successProvisionMessage + if !isProvisioning { + reason = successUpdateInstanceReason + message = successUpdateInstanceMessage + } + glog.V(5).Infof("Successfully %v ServiceInstance %v/%v of ServiceClass %v at ClusterServiceBroker %v: response: %+v", provisionedOrUpdatedText, instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName, response) toUpdate.Status.ExternalProperties = toUpdate.Status.InProgressProperties c.clearServiceInstanceCurrentOperation(toUpdate) @@ -816,14 +899,14 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance toUpdate, v1alpha1.ServiceInstanceConditionReady, v1alpha1.ConditionTrue, - successProvisionReason, - successProvisionMessage, + reason, + message, ) if _, err := c.updateServiceInstanceStatus(toUpdate); err != nil { return err } - c.recorder.Eventf(instance, apiv1.EventTypeNormal, successProvisionReason, successProvisionMessage) + c.recorder.Eventf(instance, apiv1.EventTypeNormal, reason, message) } return nil } @@ -1033,12 +1116,16 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se var message string var reason string - if deleting { + switch { + case deleting: reason = asyncDeprovisioningReason message = asyncDeprovisioningMessage - } else { + case instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationProvision: reason = asyncProvisioningReason message = asyncProvisioningMessage + case instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationUpdate: + reason = asyncUpdatingInstanceReason + message = asyncUpdatingInstanceMessage } if response.Description != nil { @@ -1098,6 +1185,30 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se } glog.V(4).Infof("last operation not completed (still in progress) for %v/%v", instance.Namespace, instance.Name) case osb.StateSucceeded: + var ( + readyStatus v1alpha1.ConditionStatus + message string + reason string + actionText string + ) + switch { + case deleting: + readyStatus = v1alpha1.ConditionFalse + reason = successDeprovisionReason + message = successDeprovisionMessage + actionText = "deprovisioned" + case instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationProvision: + readyStatus = v1alpha1.ConditionTrue + reason = successProvisionReason + message = successProvisionMessage + actionText = "provisioned" + case instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationUpdate: + readyStatus = v1alpha1.ConditionTrue + reason = successUpdateInstanceReason + message = successUpdateInstanceMessage + actionText = "updated" + } + // Update the instance to reflect that an async operation is no longer // in progress. clone, err := api.Scheme.DeepCopy(instance) @@ -1105,47 +1216,33 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se return err } toUpdate := clone.(*v1alpha1.ServiceInstance) + toUpdate.Status.ExternalProperties = toUpdate.Status.InProgressProperties c.clearServiceInstanceCurrentOperation(toUpdate) - // If we were asynchronously deleting a Service Instance, finish - // the finalizers. - if deleting { - setServiceInstanceCondition( - toUpdate, - v1alpha1.ServiceInstanceConditionReady, - v1alpha1.ConditionFalse, - successDeprovisionReason, - successDeprovisionMessage, - ) - - if !mitigatingOrphan { - // Clear the finalizer - if finalizers := sets.NewString(toUpdate.Finalizers...); finalizers.Has(v1alpha1.FinalizerServiceCatalog) { - finalizers.Delete(v1alpha1.FinalizerServiceCatalog) - toUpdate.Finalizers = finalizers.List() - } - } + setServiceInstanceCondition( + toUpdate, + v1alpha1.ServiceInstanceConditionReady, + readyStatus, + reason, + message, + ) - if _, err := c.updateServiceInstanceStatus(toUpdate); err != nil { - return err + if deleting && !mitigatingOrphan { + // Clear the finalizer + if finalizers := sets.NewString(toUpdate.Finalizers...); finalizers.Has(v1alpha1.FinalizerServiceCatalog) { + finalizers.Delete(v1alpha1.FinalizerServiceCatalog) + toUpdate.Finalizers = finalizers.List() } + } - c.recorder.Event(instance, apiv1.EventTypeNormal, successDeprovisionReason, successDeprovisionMessage) - glog.V(5).Infof("Successfully deprovisioned ServiceInstance %v/%v of ServiceClass %v at ClusterServiceBroker %v", instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName) - } else { - setServiceInstanceCondition( - toUpdate, - v1alpha1.ServiceInstanceConditionReady, - v1alpha1.ConditionTrue, - successProvisionReason, - successProvisionMessage, - ) - if _, err := c.updateServiceInstanceStatus(toUpdate); err != nil { - return err - } + if _, err := c.updateServiceInstanceStatus(toUpdate); err != nil { + return err } + c.recorder.Event(instance, apiv1.EventTypeNormal, reason, message) + glog.V(5).Infof("Successfully %v ServiceInstance %v/%v of ServiceClass %v at ClusterServiceBroker %v", actionText, instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName) + err = c.finishPollingServiceInstance(instance) if err != nil { return err @@ -1155,7 +1252,16 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se if response.Description != nil { description = *response.Description } - s := fmt.Sprintf("Error deprovisioning ServiceInstance \"%s/%s\" of ServiceClass %q at ClusterServiceBroker %q: %q", instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName, description) + actionText := "" + switch { + case deleting: + actionText = "deprovisioning" + case instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationProvision: + actionText = "provisioning" + case instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationUpdate: + actionText = "updating" + } + s := fmt.Sprintf("Error %s ServiceInstance \"%s/%s\" of ServiceClass %q at ClusterServiceBroker %q: %q", actionText, instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName, description) c.recorder.Event(instance, apiv1.EventTypeWarning, errorDeprovisionCalledReason, s) clone, err := api.Scheme.DeepCopy(instance) @@ -1165,13 +1271,24 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se toUpdate := clone.(*v1alpha1.ServiceInstance) c.clearServiceInstanceCurrentOperation(toUpdate) - readyCond := v1alpha1.ConditionFalse - reason := errorProvisionCallFailedReason - msg := "Provision call failed: " + s - if deleting { + var ( + readyCond v1alpha1.ConditionStatus + reason string + msg string + ) + switch { + case deleting: readyCond = v1alpha1.ConditionUnknown reason = errorDeprovisionCalledReason - msg = "Deprovision call failed:" + s + msg = "Deprovision call failed: " + s + case instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationProvision: + readyCond = v1alpha1.ConditionFalse + reason = errorProvisionCallFailedReason + msg = "Provision call failed: " + s + case instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationUpdate: + readyCond = v1alpha1.ConditionFalse + reason = errorUpdateInstanceCallFailedReason + msg = "Update call failed: " + s } setServiceInstanceCondition( toUpdate, @@ -1449,6 +1566,9 @@ func (c *controller) recordStartOfServiceInstanceOperation(toUpdate *v1alpha1.Se case v1alpha1.ServiceInstanceOperationProvision: reason = provisioningInFlightReason message = provisioningInFlightMessage + case v1alpha1.ServiceInstanceOperationUpdate: + reason = instanceUpdatingInFlightReason + message = instanceUpdatingInFlightMessage case v1alpha1.ServiceInstanceOperationDeprovision: reason = deprovisioningInFlightReason message = deprovisioningInFlightMessage diff --git a/pkg/controller/controller_instance_test.go b/pkg/controller/controller_instance_test.go index 0508f2f4333..4b23be4bcfd 100644 --- a/pkg/controller/controller_instance_test.go +++ b/pkg/controller/controller_instance_test.go @@ -3151,6 +3151,128 @@ func TestResolveReferencesNoServiceClass(t *testing.T) { } } +// TestReconcileServiceInstanceUpdateParameters tests updating a +// ServiceInstance with new paramaters +func TestReconcileServiceInstanceUpdateParameters(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeClusterServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + UpdateInstanceReaction: &fakeosb.UpdateInstanceReaction{ + Response: &osb.UpdateInstanceResponse{}, + }, + }) + + sharedInformers.ClusterServiceBrokers().Informer().GetStore().Add(getTestClusterServiceBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + sharedInformers.ServicePlans().Informer().GetStore().Add(getTestServicePlan()) + + instance := getTestServiceInstanceWithRefs() + instance.Generation = 2 + instance.Status.ReconciledGeneration = 1 + + oldParameters := map[string]interface{}{ + "args": map[string]interface{}{ + "first": "first-arg", + "second": "second-arg", + }, + "name": "test-param", + } + oldParametersMarshaled, err := MarshalRawParameters(oldParameters) + if err != nil { + t.Fatalf("Failed to marshal parameters: %v", err) + } + oldParametersRaw := &runtime.RawExtension{ + Raw: oldParametersMarshaled, + } + + oldParametersChecksum, err := generateChecksumOfParameters(oldParameters) + if err != nil { + t.Fatalf("Failed to generate parameters checksum: %v", err) + } + + instance.Status.ExternalProperties = &v1alpha1.ServiceInstancePropertiesState{ + ExternalServicePlanName: testServicePlanName, + Parameters: oldParametersRaw, + ParametersChecksum: oldParametersChecksum, + } + + parameters := instanceParameters{Name: "test-param", Args: make(map[string]string)} + parameters.Args["first"] = "first-arg" + parameters.Args["second"] = "new-second-arg" + + b, err := json.Marshal(parameters) + if err != nil { + t.Fatalf("Failed to marshal parameters %v : %v", parameters, err) + } + instance.Spec.Parameters = &runtime.RawExtension{Raw: b} + + if err = testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("This should not fail : %v", err) + } + + brokerActions := fakeClusterServiceBrokerClient.Actions() + assertNumberOfClusterServiceBrokerActions(t, brokerActions, 1) + assertUpdateInstance(t, brokerActions[0], &osb.UpdateInstanceRequest{ + AcceptsIncomplete: true, + InstanceID: instanceGUID, + ServiceID: serviceClassGUID, + PlanID: nil, // no change to plan + Parameters: map[string]interface{}{ + "args": map[string]interface{}{ + "first": "first-arg", + "second": "new-second-arg", + }, + "name": "test-param", + }, + }) + + expectedParameters := map[string]interface{}{ + "args": map[string]interface{}{ + "first": "first-arg", + "second": "second-arg", + }, + "name": "test-param", + } + expectedParametersChecksum, err := generateChecksumOfParameters(expectedParameters) + if err != nil { + t.Fatalf("Failed to generate parameters checksum: %v", err) + } + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 2) + + updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertServiceInstanceOperationInProgressWithParameters(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, testServicePlanName, expectedParameters, expectedParametersChecksum, instance) + + updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) + assertServiceInstanceOperationSuccessWithParameters(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, testServicePlanName, expectedParameters, expectedParametersChecksum, instance) + + updateObject, ok := updatedServiceInstance.(*v1alpha1.ServiceInstance) + if !ok { + t.Fatalf("couldn't convert to *v1alpha1.ServiceInstance") + } + + // Verify parameters are what we'd expect them to be, basically name, map with two values in it. + if len(updateObject.Spec.Parameters.Raw) == 0 { + t.Fatalf("Parameters was unexpectedly empty") + } + + // verify no kube resources created + // One single action comes from getting namespace uid + kubeActions := fakeKubeClient.Actions() + if err := checkKubeClientActions(kubeActions, []kubeClientAction{ + {verb: "get", resourceName: "namespaces", checkType: checkGetActionType}, + }); err != nil { + t.Fatal(err) + } + + events := getRecordedEvents(testController) + assertNumEvents(t, events, 1) + + expectedEvent := apiv1.EventTypeNormal + " " + successUpdateInstanceReason + " " + "The instance was updated successfully" + if e, a := expectedEvent, events[0]; e != a { + t.Fatalf("Received unexpected event: %v\nExpected: %v", a, e) + } +} + // TestResolveReferencesNoServicePlan tests that resolveReferences fails // with the expected failure case when no ServicePlan exists func TestResolveReferencesNoServicePlan(t *testing.T) { @@ -3224,75 +3346,618 @@ func TestResolveReferencesNoServicePlan(t *testing.T) { } } -// TestResolveReferences tests that resolveReferences works -// correctly and resolves references. -func TestResolveReferencesWorks(t *testing.T) { - fakeKubeClient, fakeCatalogClient, _, testController, _ := newTestController(t, noFakeActions()) +// TestReconcileServiceInstanceUpdatePlan tests updating a +// ServiceInstance with a new plan +func TestReconcileServiceInstanceUpdatePlan(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeClusterServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + UpdateInstanceReaction: &fakeosb.UpdateInstanceReaction{ + Response: &osb.UpdateInstanceResponse{}, + }, + }) - instance := getTestServiceInstance() + sharedInformers.ClusterServiceBrokers().Informer().GetStore().Add(getTestClusterServiceBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + sharedInformers.ServicePlans().Informer().GetStore().Add(getTestServicePlan()) - sc := getTestServiceClass() - var scItems []v1alpha1.ServiceClass - scItems = append(scItems, *sc) - fakeCatalogClient.AddReactor("list", "serviceclasses", func(action clientgotesting.Action) (bool, runtime.Object, error) { - return true, &v1alpha1.ServiceClassList{Items: scItems}, nil - }) - sp := getTestServicePlan() - var spItems []v1alpha1.ServicePlan - spItems = append(spItems, *sp) - fakeCatalogClient.AddReactor("list", "serviceplans", func(action clientgotesting.Action) (bool, runtime.Object, error) { - return true, &v1alpha1.ServicePlanList{Items: spItems}, nil - }) + instance := getTestServiceInstanceWithRefs() + instance.Generation = 2 + instance.Status.ReconciledGeneration = 1 - updatedInstance, err := testController.resolveReferences(instance) + oldParameters := map[string]interface{}{ + "args": map[string]interface{}{ + "first": "first-arg", + "second": "second-arg", + }, + "name": "test-param", + } + oldParametersMarshaled, err := MarshalRawParameters(oldParameters) if err != nil { - t.Fatalf("Should not have failed, but failed with: %q", err) + t.Fatalf("Failed to marshal parameters: %v", err) + } + oldParametersRaw := &runtime.RawExtension{ + Raw: oldParametersMarshaled, } - if updatedInstance.Spec.ServiceClassRef == nil || updatedInstance.Spec.ServiceClassRef.Name != serviceClassGUID { - t.Fatalf("Did not find expected ServiceClassRef, expected %q got %+v", serviceClassGUID, updatedInstance.Spec.ServiceClassRef) + oldParametersChecksum, err := generateChecksumOfParameters(oldParameters) + if err != nil { + t.Fatalf("Failed to generate parameters checksum: %v", err) } - if updatedInstance.Spec.ServicePlanRef == nil || updatedInstance.Spec.ServicePlanRef.Name != planGUID { - t.Fatalf("Did not find expected ServicePlanRef, expected %q got %+v", planGUID, updatedInstance.Spec.ServicePlanRef.Name) + instance.Status.ExternalProperties = &v1alpha1.ServiceInstancePropertiesState{ + ExternalServicePlanName: "old-plan-name", + Parameters: oldParametersRaw, + ParametersChecksum: oldParametersChecksum, } - // We should get the following actions: - // list call for ServiceClass - // list call for ServicePlan - // updating references - actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 3) + parameters := instanceParameters{Name: "test-param", Args: make(map[string]string)} + parameters.Args["first"] = "first-arg" + parameters.Args["second"] = "second-arg" - listRestrictions := clientgotesting.ListRestrictions{ - Labels: labels.Everything(), - Fields: fields.OneTermEqualSelector("spec.externalName", instance.Spec.ExternalServiceClassName), + b, err := json.Marshal(parameters) + if err != nil { + t.Fatalf("Failed to marshal parameters %v : %v", parameters, err) } - assertList(t, actions[0], &v1alpha1.ServiceClass{}, listRestrictions) + instance.Spec.Parameters = &runtime.RawExtension{Raw: b} - listRestrictions = clientgotesting.ListRestrictions{ - Labels: labels.Everything(), - Fields: fields.ParseSelectorOrDie("spec.externalName=test-plan,spec.clusterServiceBrokerName=test-broker,spec.serviceClassRef.name=SCGUID"), + if err = testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("This should not fail : %v", err) } - assertList(t, actions[1], &v1alpha1.ServicePlan{}, listRestrictions) - updatedServiceInstance := assertUpdateReference(t, actions[2], instance) + brokerActions := fakeClusterServiceBrokerClient.Actions() + assertNumberOfClusterServiceBrokerActions(t, brokerActions, 1) + expectedPlanID := planGUID + assertUpdateInstance(t, brokerActions[0], &osb.UpdateInstanceRequest{ + AcceptsIncomplete: true, + InstanceID: instanceGUID, + ServiceID: serviceClassGUID, + PlanID: &expectedPlanID, + Parameters: nil, // no change to parameters + }) + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 2) + + updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertServiceInstanceOperationInProgressWithParameters(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, testServicePlanName, oldParameters, oldParametersChecksum, instance) + + updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) + assertServiceInstanceOperationSuccessWithParameters(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, testServicePlanName, oldParameters, oldParametersChecksum, instance) + updateObject, ok := updatedServiceInstance.(*v1alpha1.ServiceInstance) if !ok { t.Fatalf("couldn't convert to *v1alpha1.ServiceInstance") } - if updateObject.Spec.ServiceClassRef == nil || updateObject.Spec.ServiceClassRef.Name != serviceClassGUID { - t.Fatalf("ServiceClassRef was not resolved correctly during reconcile") + + // Verify parameters are what we'd expect them to be, basically name, map with two values in it. + if len(updateObject.Spec.Parameters.Raw) == 0 { + t.Fatalf("Parameters was unexpectedly empty") } - if updateObject.Spec.ServicePlanRef == nil || updateObject.Spec.ServicePlanRef.Name != planGUID { - t.Fatalf("ServicePlanRef was not resolved correctly during reconcile") + + // verify no kube resources created + // One single action comes from getting namespace uid + kubeActions := fakeKubeClient.Actions() + if err := checkKubeClientActions(kubeActions, []kubeClientAction{ + {verb: "get", resourceName: "namespaces", checkType: checkGetActionType}, + }); err != nil { + t.Fatal(err) + } + + events := getRecordedEvents(testController) + assertNumEvents(t, events, 1) + + expectedEvent := apiv1.EventTypeNormal + " " + successUpdateInstanceReason + " " + "The instance was updated successfully" + if e, a := expectedEvent, events[0]; e != a { + t.Fatalf("Received unexpected event: %v\nExpected: %v", a, e) + } +} + +// TestReconcileServiceInstanceWithUpdateCallFailure tests that when the update +// call to the broker fails, the ready condition becomes false, and the +// failure condition is not set. +func TestReconcileServiceInstanceWithUpdateCallFailure(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeClusterServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + UpdateInstanceReaction: &fakeosb.UpdateInstanceReaction{ + Error: errors.New("fake update failure"), + }, + }) + + sharedInformers.ClusterServiceBrokers().Informer().GetStore().Add(getTestClusterServiceBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + sharedInformers.ServicePlans().Informer().GetStore().Add(getTestServicePlan()) + + instance := getTestServiceInstanceWithRefs() + instance.Generation = 2 + instance.Status.ReconciledGeneration = 1 + + instance.Status.ExternalProperties = &v1alpha1.ServiceInstancePropertiesState{ + ExternalServicePlanName: "old-plan-name", + } + + if err := testController.reconcileServiceInstance(instance); err == nil { + t.Fatalf("Should not be able to make the ServiceInstance.") } + brokerActions := fakeClusterServiceBrokerClient.Actions() + assertNumberOfClusterServiceBrokerActions(t, brokerActions, 1) + expectedPlanID := planGUID + assertUpdateInstance(t, brokerActions[0], &osb.UpdateInstanceRequest{ + AcceptsIncomplete: true, + InstanceID: instanceGUID, + ServiceID: serviceClassGUID, + PlanID: &expectedPlanID, + }) + // verify no kube resources created // One single action comes from getting namespace uid kubeActions := fakeKubeClient.Actions() - assertNumberOfActions(t, kubeActions, 0) + if err := checkKubeClientActions(kubeActions, []kubeClientAction{ + {verb: "get", resourceName: "namespaces", checkType: checkGetActionType}, + }); err != nil { + t.Fatal(err) + } + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 2) + + updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, testServicePlanName, instance) + + updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) + assertServiceInstanceRequestRetriableError(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, errorErrorCallingUpdateInstanceReason, testServicePlanName, instance) events := getRecordedEvents(testController) - assertNumEvents(t, events, 0) + assertNumEvents(t, events, 1) + + expectedEvent := apiv1.EventTypeWarning + " " + errorErrorCallingUpdateInstanceReason + " " + "Error updating ServiceInstance \"test-ns/test-instance\" of ServiceClass \"test-serviceclass\" at ClusterServiceBroker \"test-broker\": fake update failure" + if e, a := expectedEvent, events[0]; e != a { + t.Fatalf("Received unexpected event: %v\nExpected: %v", a, e) + } +} + +// TestReconcileServiceInstanceWithUpdateFailure tests that when the provision +// call to the broker fails with an HTTP error, the ready condition becomes +// false, and the failure condition is set. +func TestReconcileServiceInstanceWithUpdateFailure(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeClusterServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + UpdateInstanceReaction: &fakeosb.UpdateInstanceReaction{ + Error: osb.HTTPStatusCodeError{ + StatusCode: http.StatusConflict, + ErrorMessage: strPtr("OutOfQuota"), + Description: strPtr("You're out of quota!"), + }, + }, + }) + + sharedInformers.ClusterServiceBrokers().Informer().GetStore().Add(getTestClusterServiceBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + sharedInformers.ServicePlans().Informer().GetStore().Add(getTestServicePlan()) + + instance := getTestServiceInstanceWithRefs() + instance.Generation = 2 + instance.Status.ReconciledGeneration = 1 + + instance.Status.ExternalProperties = &v1alpha1.ServiceInstancePropertiesState{ + ExternalServicePlanName: "old-plan-name", + } + + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + brokerActions := fakeClusterServiceBrokerClient.Actions() + assertNumberOfClusterServiceBrokerActions(t, brokerActions, 1) + expectedPlanID := planGUID + assertUpdateInstance(t, brokerActions[0], &osb.UpdateInstanceRequest{ + AcceptsIncomplete: true, + InstanceID: instanceGUID, + ServiceID: serviceClassGUID, + PlanID: &expectedPlanID, + }) + + // verify one kube action occurred + kubeActions := fakeKubeClient.Actions() + if err := checkKubeClientActions(kubeActions, []kubeClientAction{ + {verb: "get", resourceName: "namespaces", checkType: checkGetActionType}, + }); err != nil { + t.Fatal(err) + } + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 2) + + updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, testServicePlanName, instance) + + updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) + assertServiceInstanceRequestFailingError(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, errorUpdateInstanceCallFailedReason, "ClusterServiceBrokerReturnedFailure", instance) + + events := getRecordedEvents(testController) + assertNumEvents(t, events, 1) + + expectedEvent := apiv1.EventTypeWarning + " " + errorUpdateInstanceCallFailedReason + " " + "Error updating ServiceInstance \"test-ns/test-instance\" of ServiceClass \"test-serviceclass\" at ClusterServiceBroker \"test-broker\": Status: 409; ErrorMessage: OutOfQuota; Description: You're out of quota!; ResponseError: " + if e, a := expectedEvent, events[0]; e != a { + t.Fatalf("Received unexpected event: %v\nExpected: %v", a, e) + } +} + +// TestResolveReferences tests that resolveReferences works +// correctly and resolves references. +func TestResolveReferencesWorks(t *testing.T) { + fakeKubeClient, fakeCatalogClient, _, testController, _ := newTestController(t, noFakeActions()) + + instance := getTestServiceInstance() + + sc := getTestServiceClass() + var scItems []v1alpha1.ServiceClass + scItems = append(scItems, *sc) + fakeCatalogClient.AddReactor("list", "serviceclasses", func(action clientgotesting.Action) (bool, runtime.Object, error) { + return true, &v1alpha1.ServiceClassList{Items: scItems}, nil + }) + sp := getTestServicePlan() + var spItems []v1alpha1.ServicePlan + spItems = append(spItems, *sp) + fakeCatalogClient.AddReactor("list", "serviceplans", func(action clientgotesting.Action) (bool, runtime.Object, error) { + return true, &v1alpha1.ServicePlanList{Items: spItems}, nil + }) + + updatedInstance, err := testController.resolveReferences(instance) + if err != nil { + t.Fatalf("Should not have failed, but failed with: %q", err) + } + + if updatedInstance.Spec.ServiceClassRef == nil || updatedInstance.Spec.ServiceClassRef.Name != serviceClassGUID { + t.Fatalf("Did not find expected ServiceClassRef, expected %q got %+v", serviceClassGUID, updatedInstance.Spec.ServiceClassRef) + } + + if updatedInstance.Spec.ServicePlanRef == nil || updatedInstance.Spec.ServicePlanRef.Name != planGUID { + t.Fatalf("Did not find expected ServicePlanRef, expected %q got %+v", planGUID, updatedInstance.Spec.ServicePlanRef.Name) + } + + // We should get the following actions: + // list call for ServiceClass + // list call for ServicePlan + // updating references + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 3) + + listRestrictions := clientgotesting.ListRestrictions{ + Labels: labels.Everything(), + Fields: fields.OneTermEqualSelector("spec.externalName", instance.Spec.ExternalServiceClassName), + } + assertList(t, actions[0], &v1alpha1.ServiceClass{}, listRestrictions) + + listRestrictions = clientgotesting.ListRestrictions{ + Labels: labels.Everything(), + Fields: fields.ParseSelectorOrDie("spec.externalName=test-plan,spec.clusterServiceBrokerName=test-broker,spec.serviceClassRef.name=SCGUID"), + } + assertList(t, actions[1], &v1alpha1.ServicePlan{}, listRestrictions) + + updatedServiceInstance := assertUpdateReference(t, actions[2], instance) + updateObject, ok := updatedServiceInstance.(*v1alpha1.ServiceInstance) + if !ok { + t.Fatalf("couldn't convert to *v1alpha1.ServiceInstance") + } + if updateObject.Spec.ServiceClassRef == nil || updateObject.Spec.ServiceClassRef.Name != serviceClassGUID { + t.Fatalf("ServiceClassRef was not resolved correctly during reconcile") + } + if updateObject.Spec.ServicePlanRef == nil || updateObject.Spec.ServicePlanRef.Name != planGUID { + t.Fatalf("ServicePlanRef was not resolved correctly during reconcile") + } + + // verify no kube resources created + // One single action comes from getting namespace uid + kubeActions := fakeKubeClient.Actions() + assertNumberOfActions(t, kubeActions, 0) + + events := getRecordedEvents(testController) + assertNumEvents(t, events, 0) +} + +// TestResolveReferencesForPlanChange tests that resolveReferences updates the +// ServicePlanRef when the plan is changed. +func TestResolveReferencesForPlanChange(t *testing.T) { + fakeKubeClient, fakeCatalogClient, _, testController, sharedInformers := newTestController(t, noFakeActions()) + + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + + instance := getTestServiceInstanceWithRefs() + + newPlanID := "new-plan-id" + newPlanName := "new-plan-name" + + sp := &v1alpha1.ServicePlan{ + ObjectMeta: metav1.ObjectMeta{Name: newPlanID}, + Spec: v1alpha1.ServicePlanSpec{ + ExternalID: newPlanID, + ExternalName: newPlanName, + Bindable: truePtr(), + }, + } + var spItems []v1alpha1.ServicePlan + spItems = append(spItems, *sp) + fakeCatalogClient.AddReactor("list", "serviceplans", func(action clientgotesting.Action) (bool, runtime.Object, error) { + return true, &v1alpha1.ServicePlanList{Items: spItems}, nil + }) + + instance.Spec.ExternalServicePlanName = newPlanName + instance.Spec.ServicePlanRef = nil + + updatedInstance, err := testController.resolveReferences(instance) + if err != nil { + t.Fatalf("Should not have failed, but failed with: %q", err) + } + + if updatedInstance.Spec.ServiceClassRef == nil || updatedInstance.Spec.ServiceClassRef.Name != serviceClassGUID { + t.Fatalf("Did not find expected ServiceClassRef, expected %q got %+v", serviceClassGUID, updatedInstance.Spec.ServiceClassRef) + } + + if updatedInstance.Spec.ServicePlanRef == nil || updatedInstance.Spec.ServicePlanRef.Name != newPlanID { + t.Fatalf("Did not find expected ServicePlanRef, expected %q got %+v", newPlanID, updatedInstance.Spec.ServicePlanRef.Name) + } + + // We should get the following actions: + // list call for ServicePlan + // updating references + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 2) + + listRestrictions := clientgotesting.ListRestrictions{ + Labels: labels.Everything(), + Fields: fields.ParseSelectorOrDie("spec.externalName=new-plan-name,spec.clusterServiceBrokerName=test-broker,spec.serviceClassRef.name=SCGUID"), + } + assertList(t, actions[0], &v1alpha1.ServicePlan{}, listRestrictions) + + updatedServiceInstance := assertUpdateReference(t, actions[1], instance) + updateObject, ok := updatedServiceInstance.(*v1alpha1.ServiceInstance) + if !ok { + t.Fatalf("couldn't convert to *v1alpha1.ServiceInstance") + } + if updateObject.Spec.ServiceClassRef == nil || updateObject.Spec.ServiceClassRef.Name != serviceClassGUID { + t.Fatalf("ServiceClassRef was not resolved correctly during reconcile") + } + if updateObject.Spec.ServicePlanRef == nil || updateObject.Spec.ServicePlanRef.Name != newPlanID { + t.Fatalf("ServicePlanRef was not resolved correctly during reconcile") + } + + // verify no kube resources created + // One single action comes from getting namespace uid + kubeActions := fakeKubeClient.Actions() + assertNumberOfActions(t, kubeActions, 0) + + events := getRecordedEvents(testController) + assertNumEvents(t, events, 0) +} + +// TestReconcileServiceInstanceUpdateAsynchronous tests updating a ServiceInstance +// when the request results in an async response. Resulting status will indicate +// not ready and polling in progress. +func TestReconcileServiceInstanceUpdateAsynchronous(t *testing.T) { + key := osb.OperationKey(testOperation) + fakeKubeClient, fakeCatalogClient, fakeClusterServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + UpdateInstanceReaction: &fakeosb.UpdateInstanceReaction{ + Response: &osb.UpdateInstanceResponse{ + Async: true, + OperationKey: &key, + }, + }, + }) + + addGetNamespaceReaction(fakeKubeClient) + + sharedInformers.ClusterServiceBrokers().Informer().GetStore().Add(getTestClusterServiceBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + sharedInformers.ServicePlans().Informer().GetStore().Add(getTestServicePlan()) + + instance := getTestServiceInstanceWithRefs() + instance.Generation = 2 + instance.Status.ReconciledGeneration = 1 + + instance.Status.ExternalProperties = &v1alpha1.ServiceInstancePropertiesState{ + ExternalServicePlanName: "old-plan-name", + } + + instanceKey := testNamespace + "/" + testServiceInstanceName + if testController.pollingQueue.NumRequeues(instanceKey) != 0 { + t.Fatalf("Expected polling queue to not have any record of test instance") + } + + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("This should not fail : %v", err) + } + + brokerActions := fakeClusterServiceBrokerClient.Actions() + assertNumberOfClusterServiceBrokerActions(t, brokerActions, 1) + expectedPlanID := planGUID + assertUpdateInstance(t, brokerActions[0], &osb.UpdateInstanceRequest{ + AcceptsIncomplete: true, + InstanceID: instanceGUID, + ServiceID: serviceClassGUID, + PlanID: &expectedPlanID, + }) + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 2) + + updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, testServicePlanName, instance) + + updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) + assertServiceInstanceAsyncInProgress(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, testOperation, testServicePlanName, instance) + + // verify no kube resources created. + // One single action comes from getting namespace uid + kubeActions := fakeKubeClient.Actions() + if e, a := 1, len(kubeActions); e != a { + t.Fatalf("Unexpected number of actions: expected %v, got %v", e, a) + } + + if testController.pollingQueue.NumRequeues(instanceKey) != 1 { + t.Fatalf("Expected polling queue to have a record of seeing test instance once") + } +} + +// TestPollServiceInstanceAsyncInProgressUpdating tests polling an instance that +// is already in process of updating (background/asynchronously) and is still in +// progress (should be re-polled) +func TestPollServiceInstanceAsyncInProgressUpdating(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeClusterServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + PollLastOperationReaction: &fakeosb.PollLastOperationReaction{ + Response: &osb.LastOperationResponse{ + State: osb.StateInProgress, + Description: strPtr(lastOperationDescription), + }, + }, + }) + + sharedInformers.ClusterServiceBrokers().Informer().GetStore().Add(getTestClusterServiceBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + sharedInformers.ServicePlans().Informer().GetStore().Add(getTestServicePlan()) + + instance := getTestServiceInstanceAsyncUpdating(testOperation) + instanceKey := testNamespace + "/" + testServiceInstanceName + + if testController.pollingQueue.NumRequeues(instanceKey) != 0 { + t.Fatalf("Expected polling queue to not have any record of test instance") + } + + err := testController.pollServiceInstanceInternal(instance) + if err != nil { + t.Fatalf("pollServiceInstanceInternal failed: %s", err) + } + + if testController.pollingQueue.NumRequeues(instanceKey) != 1 { + t.Fatalf("Expected polling queue to have record of seeing test instance once") + } + + brokerActions := fakeClusterServiceBrokerClient.Actions() + assertNumberOfClusterServiceBrokerActions(t, brokerActions, 1) + operationKey := osb.OperationKey(testOperation) + assertPollLastOperation(t, brokerActions[0], &osb.LastOperationRequest{ + InstanceID: instanceGUID, + ServiceID: strPtr(serviceClassGUID), + PlanID: strPtr(planGUID), + OperationKey: &operationKey, + }) + + // there should have been 1 action to update the status with the last operation description + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + + updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertServiceInstanceAsyncInProgress(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, testOperation, testServicePlanName, instance) + assertServiceInstanceConditionHasLastOperationDescription(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, lastOperationDescription) + + // verify no kube resources created. + // No actions + kubeActions := fakeKubeClient.Actions() + assertNumberOfActions(t, kubeActions, 0) +} + +// TestPollServiceInstanceAsyncSuccessUpdating tests polling an instance that is +// already in process of updating (background/ asynchronously) and is found to be +// ready +func TestPollServiceInstanceAsyncSuccessUpdating(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeClusterServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + PollLastOperationReaction: &fakeosb.PollLastOperationReaction{ + Response: &osb.LastOperationResponse{ + State: osb.StateSucceeded, + Description: strPtr(lastOperationDescription), + }, + }, + }) + + sharedInformers.ClusterServiceBrokers().Informer().GetStore().Add(getTestClusterServiceBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + sharedInformers.ServicePlans().Informer().GetStore().Add(getTestServicePlan()) + + instance := getTestServiceInstanceAsyncUpdating(testOperation) + instanceKey := testNamespace + "/" + testServiceInstanceName + + if testController.pollingQueue.NumRequeues(instanceKey) != 0 { + t.Fatalf("Expected polling queue to not have any record of test instance") + } + + err := testController.pollServiceInstanceInternal(instance) + if err != nil { + t.Fatalf("pollServiceInstanceInternal failed: %s", err) + } + + if testController.pollingQueue.NumRequeues(instanceKey) != 0 { + t.Fatalf("Expected polling queue to not have any record of test instance as polling should have completed") + } + + brokerActions := fakeClusterServiceBrokerClient.Actions() + assertNumberOfClusterServiceBrokerActions(t, brokerActions, 1) + operationKey := osb.OperationKey(testOperation) + assertPollLastOperation(t, brokerActions[0], &osb.LastOperationRequest{ + InstanceID: instanceGUID, + ServiceID: strPtr(serviceClassGUID), + PlanID: strPtr(planGUID), + OperationKey: &operationKey, + }) + + // verify no kube resources created. + // No actions + kubeActions := fakeKubeClient.Actions() + assertNumberOfActions(t, kubeActions, 0) + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + + updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, testServicePlanName, instance) +} + +// TestPollServiceInstanceAsyncFailureUpdating tests polling an instance where +// update was in process asynchronously but has an updated status of failed to +// update. +func TestPollServiceInstanceAsyncFailureUpdating(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeClusterServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + PollLastOperationReaction: &fakeosb.PollLastOperationReaction{ + Response: &osb.LastOperationResponse{ + State: osb.StateFailed, + }, + }, + }) + + sharedInformers.ClusterServiceBrokers().Informer().GetStore().Add(getTestClusterServiceBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + sharedInformers.ServicePlans().Informer().GetStore().Add(getTestServicePlan()) + + instance := getTestServiceInstanceAsyncUpdating(testOperation) + instanceKey := testNamespace + "/" + testServiceInstanceName + + if testController.pollingQueue.NumRequeues(instanceKey) != 0 { + t.Fatalf("Expected polling queue to not have any record of test instance") + } + + err := testController.pollServiceInstanceInternal(instance) + if err != nil { + t.Fatalf("pollServiceInstanceInternal failed: %s", err) + } + + if testController.pollingQueue.NumRequeues(instanceKey) != 0 { + t.Fatalf("Expected polling queue to not have any record of test instance as polling should have completed") + } + + brokerActions := fakeClusterServiceBrokerClient.Actions() + assertNumberOfClusterServiceBrokerActions(t, brokerActions, 1) + operationKey := osb.OperationKey(testOperation) + assertPollLastOperation(t, brokerActions[0], &osb.LastOperationRequest{ + InstanceID: instanceGUID, + ServiceID: strPtr(serviceClassGUID), + PlanID: strPtr(planGUID), + OperationKey: &operationKey, + }) + + // verify no kube resources created. + // No actions + kubeActions := fakeKubeClient.Actions() + assertNumberOfActions(t, kubeActions, 0) + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + + updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertServiceInstanceRequestFailingError(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, errorUpdateInstanceCallFailedReason, errorUpdateInstanceCallFailedReason, instance) } diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 30363cf15bd..8b2ff86d31f 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -557,9 +557,6 @@ func getTestServiceInstanceWithFailedStatus() *v1alpha1.ServiceInstance { // getTestServiceInstanceAsync returns an instance in async mode func getTestServiceInstanceAsyncProvisioning(operation string) *v1alpha1.ServiceInstance { instance := getTestServiceInstanceWithRefs() - if operation != "" { - instance.Status.LastOperation = &operation - } operationStartTime := metav1.NewTime(time.Now().Add(-1 * time.Hour)) instance.Status = v1alpha1.ServiceInstanceStatus{ @@ -583,13 +580,42 @@ func getTestServiceInstanceAsyncProvisioning(operation string) *v1alpha1.Service return instance } -func getTestServiceInstanceAsyncDeprovisioning(operation string) *v1alpha1.ServiceInstance { +// getTestServiceInstanceAsyncUpdating returns an instance for which there is an +// in-progress async update +func getTestServiceInstanceAsyncUpdating(operation string) *v1alpha1.ServiceInstance { instance := getTestServiceInstanceWithRefs() instance.Generation = 2 + + operationStartTime := metav1.NewTime(time.Now().Add(-1 * time.Hour)) + instance.Status = v1alpha1.ServiceInstanceStatus{ + ReconciledGeneration: 1, + Conditions: []v1alpha1.ServiceInstanceCondition{{ + Type: v1alpha1.ServiceInstanceConditionReady, + Status: v1alpha1.ConditionFalse, + Message: "Updating", + LastTransitionTime: metav1.NewTime(time.Now().Add(-5 * time.Minute)), + }}, + AsyncOpInProgress: true, + OperationStartTime: &operationStartTime, + CurrentOperation: v1alpha1.ServiceInstanceOperationUpdate, + InProgressProperties: &v1alpha1.ServiceInstancePropertiesState{ + ExternalServicePlanName: testServicePlanName, + }, + ExternalProperties: &v1alpha1.ServiceInstancePropertiesState{ + ExternalServicePlanName: "old-plan-name", + }, + } if operation != "" { instance.Status.LastOperation = &operation } + return instance +} + +func getTestServiceInstanceAsyncDeprovisioning(operation string) *v1alpha1.ServiceInstance { + instance := getTestServiceInstanceWithRefs() + instance.Generation = 2 + operationStartTime := metav1.NewTime(time.Now().Add(-1 * time.Hour)) instance.Status = v1alpha1.ServiceInstanceStatus{ Conditions: []v1alpha1.ServiceInstanceCondition{{ @@ -1659,6 +1685,8 @@ func assertServiceInstanceOperationInProgressWithParameters(t *testing.T, obj ru switch operation { case v1alpha1.ServiceInstanceOperationProvision: reason = provisioningInFlightReason + case v1alpha1.ServiceInstanceOperationUpdate: + reason = instanceUpdatingInFlightReason case v1alpha1.ServiceInstanceOperationDeprovision: reason = deprovisioningInFlightReason } @@ -1691,6 +1719,9 @@ func assertServiceInstanceOperationSuccessWithParameters(t *testing.T, obj runti case v1alpha1.ServiceInstanceOperationProvision: reason = successProvisionReason readyStatus = v1alpha1.ConditionTrue + case v1alpha1.ServiceInstanceOperationUpdate: + reason = successUpdateInstanceReason + readyStatus = v1alpha1.ConditionTrue case v1alpha1.ServiceInstanceOperationDeprovision: reason = successDeprovisionReason readyStatus = v1alpha1.ConditionFalse @@ -1717,7 +1748,7 @@ func assertServiceInstanceOperationSuccessWithParameters(t *testing.T, obj runti func assertServiceInstanceRequestFailingError(t *testing.T, obj runtime.Object, operation v1alpha1.ServiceInstanceOperation, readyReason string, failureReason string, originalInstance *v1alpha1.ServiceInstance) { var readyStatus v1alpha1.ConditionStatus switch operation { - case v1alpha1.ServiceInstanceOperationProvision: + case v1alpha1.ServiceInstanceOperationProvision, v1alpha1.ServiceInstanceOperationUpdate: readyStatus = v1alpha1.ConditionFalse case v1alpha1.ServiceInstanceOperationDeprovision: readyStatus = v1alpha1.ConditionUnknown @@ -1751,7 +1782,7 @@ func assertServiceInstanceRequestRetriableError(t *testing.T, obj runtime.Object func assertServiceInstanceRequestRetriableErrorWithParameters(t *testing.T, obj runtime.Object, operation v1alpha1.ServiceInstanceOperation, reason string, planName string, inProgressParameters map[string]interface{}, inProgressParametersChecksum string, originalInstance *v1alpha1.ServiceInstance) { var readyStatus v1alpha1.ConditionStatus switch operation { - case v1alpha1.ServiceInstanceOperationProvision: + case v1alpha1.ServiceInstanceOperationProvision, v1alpha1.ServiceInstanceOperationUpdate: readyStatus = v1alpha1.ConditionFalse case v1alpha1.ServiceInstanceOperationDeprovision: readyStatus = v1alpha1.ConditionUnknown @@ -1761,7 +1792,7 @@ func assertServiceInstanceRequestRetriableErrorWithParameters(t *testing.T, obj assertServiceInstanceOperationStartTimeSet(t, obj, true) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) switch operation { - case v1alpha1.ServiceInstanceOperationProvision: + case v1alpha1.ServiceInstanceOperationProvision, v1alpha1.ServiceInstanceOperationUpdate: assertServiceInstanceInProgressPropertiesPlanName(t, obj, planName) assertServiceInstanceInProgressPropertiesParameters(t, obj, inProgressParameters, inProgressParametersChecksum) case v1alpha1.ServiceInstanceOperationDeprovision: @@ -1775,6 +1806,8 @@ func assertServiceInstanceAsyncInProgress(t *testing.T, obj runtime.Object, oper switch operation { case v1alpha1.ServiceInstanceOperationProvision: reason = asyncProvisioningReason + case v1alpha1.ServiceInstanceOperationUpdate: + reason = asyncUpdatingInstanceReason case v1alpha1.ServiceInstanceOperationDeprovision: reason = asyncDeprovisioningReason } @@ -1784,7 +1817,7 @@ func assertServiceInstanceAsyncInProgress(t *testing.T, obj runtime.Object, oper assertServiceInstanceOperationStartTimeSet(t, obj, true) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) switch operation { - case v1alpha1.ServiceInstanceOperationProvision: + case v1alpha1.ServiceInstanceOperationProvision, v1alpha1.ServiceInstanceOperationUpdate: assertServiceInstanceInProgressPropertiesPlanName(t, obj, planName) assertServiceInstanceInProgressPropertiesParameters(t, obj, nil, "") case v1alpha1.ServiceInstanceOperationDeprovision: @@ -1802,6 +1835,8 @@ func assertServiceInstanceConditionHasLastOperationDescription(t *testing.T, obj switch operation { case v1alpha1.ServiceInstanceOperationProvision: expected = fmt.Sprintf("%s (%s)", asyncProvisioningMessage, lastOperationDescription) + case v1alpha1.ServiceInstanceOperationUpdate: + expected = fmt.Sprintf("%s (%s)", asyncUpdatingInstanceMessage, lastOperationDescription) case v1alpha1.ServiceInstanceOperationDeprovision: expected = fmt.Sprintf("%s (%s)", asyncDeprovisioningMessage, lastOperationDescription) } @@ -2317,6 +2352,16 @@ func assertProvision(t *testing.T, action fakeosb.Action, request *osb.Provision } } +func assertUpdateInstance(t *testing.T, action fakeosb.Action, request *osb.UpdateInstanceRequest) { + if e, a := fakeosb.UpdateInstance, action.Type; e != a { + fatalf(t, "unexpected action type; expected %v, got %v", e, a) + } + + if e, a := request, action.Request; !reflect.DeepEqual(e, a) { + fatalf(t, "unexpected diff in update instance request: %v\nexpected %+v\ngot %+v", diff.ObjectReflectDiff(e, a), e, a) + } +} + func assertDeprovision(t *testing.T, action fakeosb.Action, request *osb.DeprovisionRequest) { if e, a := fakeosb.DeprovisionInstance, action.Type; e != a { fatalf(t, "unexpected action type; expected %v, got %v", e, a) diff --git a/pkg/openapi/openapi_generated.go b/pkg/openapi/openapi_generated.go index 567fb9faf7d..cce41e0fe16 100644 --- a/pkg/openapi/openapi_generated.go +++ b/pkg/openapi/openapi_generated.go @@ -1038,8 +1038,15 @@ func GetOpenAPIDefinitions(ref openapi.ReferenceCallback) map[string]openapi.Ope Ref: ref("github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1.UserInfo"), }, }, + "updateRequests": { + SchemaProps: spec.SchemaProps{ + Description: "UpdateRequests is a strictly increasing, non-negative integer counter that can be manually incremented by a user to manually trigger an update. This allows for parameters to be updated with any out-of-band changes that have been made to the secrets from which the parameters are sourced.", + Type: []string{"integer"}, + Format: "int64", + }, + }, }, - Required: []string{"externalServiceClassName", "externalID"}, + Required: []string{"externalServiceClassName", "externalID", "updateRequests"}, }, }, Dependencies: []string{ diff --git a/pkg/registry/servicecatalog/instance/strategy.go b/pkg/registry/servicecatalog/instance/strategy.go index 38f2e0a9966..409ed16bee2 100644 --- a/pkg/registry/servicecatalog/instance/strategy.go +++ b/pkg/registry/servicecatalog/instance/strategy.go @@ -153,23 +153,19 @@ func (instanceRESTStrategy) PrepareForUpdate(ctx genericapirequest.Context, new, newServiceInstance.Spec.ServiceClassRef = oldServiceInstance.Spec.ServiceClassRef newServiceInstance.Spec.ServicePlanRef = oldServiceInstance.Spec.ServicePlanRef - // 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. - newServiceInstance.Spec = oldServiceInstance.Spec + // Clear out the ServicePlanRef so that it is resolved during reconciliation + if newServiceInstance.Spec.ExternalServicePlanName != oldServiceInstance.Spec.ExternalServicePlanName { + newServiceInstance.Spec.ServicePlanRef = nil + } // Spec updates bump the generation so that we can distinguish between // spec changes and other changes to the object. - // - // 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) { if utilfeature.DefaultFeatureGate.Enabled(scfeatures.OriginatingIdentity) { setServiceInstanceUserInfo(newServiceInstance, ctx) } newServiceInstance.Generation = oldServiceInstance.Generation + 1 + setServiceInstanceReadyFalseCondition(newServiceInstance) } } @@ -279,3 +275,34 @@ func setServiceInstanceUserInfo(instance *sc.ServiceInstance, ctx genericapirequ } } } + +func setServiceInstanceReadyFalseCondition(instance *sc.ServiceInstance) { + newCondition := sc.ServiceInstanceCondition{ + Type: sc.ServiceInstanceConditionReady, + Status: sc.ConditionFalse, + Reason: "UpdateInitiated", + Message: "Update initiated on ServiceInstance", + } + + if len(instance.Status.Conditions) == 0 { + newCondition.LastTransitionTime = metav1.Now() + instance.Status.Conditions = []sc.ServiceInstanceCondition{newCondition} + return + } + + for i, cond := range instance.Status.Conditions { + if cond.Type == sc.ServiceInstanceConditionReady { + if cond.Status != newCondition.Status { + newCondition.LastTransitionTime = metav1.Now() + } else { + newCondition.LastTransitionTime = cond.LastTransitionTime + } + + instance.Status.Conditions[i] = newCondition + return + } + } + + newCondition.LastTransitionTime = metav1.Now() + instance.Status.Conditions = append(instance.Status.Conditions, newCondition) +} diff --git a/pkg/registry/servicecatalog/instance/strategy_test.go b/pkg/registry/servicecatalog/instance/strategy_test.go index 08403b4b043..7c90f3df489 100644 --- a/pkg/registry/servicecatalog/instance/strategy_test.go +++ b/pkg/registry/servicecatalog/instance/strategy_test.go @@ -27,6 +27,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apiserver/pkg/authentication/user" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/client-go/pkg/api/v1" ) func getTestInstance() *servicecatalog.ServiceInstance { @@ -37,6 +38,8 @@ func getTestInstance() *servicecatalog.ServiceInstance { Spec: servicecatalog.ServiceInstanceSpec{ ExternalServiceClassName: "test-serviceclass", ExternalServicePlanName: "test-plan", + ServiceClassRef: &v1.ObjectReference{}, + ServicePlanRef: &v1.ObjectReference{}, UserInfo: &servicecatalog.UserInfo{ Username: "some-user", }, @@ -60,44 +63,79 @@ func contextWithUserName(userName string) genericapirequest.Context { return genericapirequest.WithUser(ctx, userInfo) } -// TODO: Un-comment "spec-change" test case when there is a field -// in the spec to which the reconciler allows a change. - -// TestInstanceUpdate tests that generation is incremented correctly when the -// spec of a Instance is updated. +// TestInstanceUpdate tests that updates to the spec of an Instance. func TestInstanceUpdate(t *testing.T) { cases := []struct { - name string - older *servicecatalog.ServiceInstance - newer *servicecatalog.ServiceInstance - shouldGenerationIncrement bool + name string + older *servicecatalog.ServiceInstance + newer *servicecatalog.ServiceInstance + shouldSpecUpdate bool + shouldPlanRefClear bool }{ { name: "no spec change", older: getTestInstance(), newer: getTestInstance(), }, - // { - // name: "spec change", - // older: getTestInstance(), - // newer: func() *servicecatalog.ServiceInstance { - // i := getTestInstance() - // i.Spec.ServiceClassName = "new-serviceclass" - // return i - // }, - // shouldGenerationIncrement: true, - // }, + { + name: "UpdateRequest increment", + older: func() *servicecatalog.ServiceInstance { + i := getTestInstance() + i.Spec.UpdateRequests = 1 + return i + }(), + newer: func() *servicecatalog.ServiceInstance { + i := getTestInstance() + i.Spec.UpdateRequests = 2 + return i + }(), + shouldSpecUpdate: true, + }, + { + name: "plan change", + older: getTestInstance(), + newer: func() *servicecatalog.ServiceInstance { + i := getTestInstance() + i.Spec.ExternalServicePlanName = "new-test-plan" + return i + }(), + shouldSpecUpdate: true, + shouldPlanRefClear: true, + }, } for _, tc := range cases { instanceRESTStrategies.PrepareForUpdate(nil, tc.newer, tc.older) expectedGeneration := tc.older.Generation - if tc.shouldGenerationIncrement { + expectedReadyCondition := servicecatalog.ConditionTrue + if tc.shouldSpecUpdate { expectedGeneration = expectedGeneration + 1 + expectedReadyCondition = servicecatalog.ConditionFalse } if e, a := expectedGeneration, tc.newer.Generation; e != a { t.Errorf("%v: expected %v, got %v for generation", tc.name, e, a) + continue + } + if e, a := 1, len(tc.newer.Status.Conditions); e != a { + t.Errorf("%v: unexpected number of conditions: expected %v, got %v", tc.name, e, a) + continue + } + if e, a := servicecatalog.ServiceInstanceConditionReady, tc.newer.Status.Conditions[0].Type; e != a { + t.Errorf("%v: unexpected condition type: expected %v, got %v", tc.name, e, a) + continue + } + if e, a := expectedReadyCondition, tc.newer.Status.Conditions[0].Status; e != a { + t.Errorf("%v: unexpected ready condition status: expected %v, got %v", tc.name, e, a) + } + if tc.shouldPlanRefClear { + if tc.newer.Spec.ServicePlanRef != nil { + t.Errorf("%v: expected ServicePlanRef to be nil", tc.name) + } + } else { + if tc.newer.Spec.ServicePlanRef == nil { + t.Errorf("%v: expected ServicePlanRef to not be nil", tc.name) + } } } } @@ -118,17 +156,15 @@ func TestInstanceUserInfo(t *testing.T) { t.Errorf("unexpected user info in created spec: expected %v, got %v", e, a) } - // TODO: Un-comment the following portion of this test when there is a field - // in the spec to which the reconciler allows a change. - - // updaterUserName := "updater" - // updatedInstance := getTestInstance() - // updateContext := contextWithUserName(updaterUserName) - // instanceRESTStrategies.PrepareForUpdate(updateContext, updatedInstance, createdInstance) + updaterUserName := "updater" + updatedInstance := getTestInstance() + updatedInstance.Spec.UpdateRequests = updatedInstance.Spec.UpdateRequests + 1 + updateContext := contextWithUserName(updaterUserName) + instanceRESTStrategies.PrepareForUpdate(updateContext, updatedInstance, createdInstance) - // if e, a := updaterUserName, updatedInstance.Spec.UserInfo.Username; e != a { - // t.Errorf("unexpected user info in updated spec: expected %v, got %v", e, a) - // } + if e, a := updaterUserName, updatedInstance.Spec.UserInfo.Username; e != a { + t.Errorf("unexpected user info in updated spec: expected %v, got %v", e, a) + } deleterUserName := "deleter" deletedInstance := getTestInstance() diff --git a/test/integration/clientset_test.go b/test/integration/clientset_test.go index d47c4bc3186..469f3df13a7 100644 --- a/test/integration/clientset_test.go +++ b/test/integration/clientset_test.go @@ -882,7 +882,8 @@ func testInstanceClient(sType server.StorageType, client servicecatalogclient.In return fmt.Errorf("Didn't get back 'secondvalue' value for key 'second' in Values map was %+v", parameters) } - // update the instance's conditions + // update the instance's conditions, and set the ReconciledGeneration so that + // spec updates are not rejected readyConditionTrue := v1alpha1.ServiceInstanceCondition{ Type: v1alpha1.ServiceInstanceConditionReady, Status: v1alpha1.ConditionTrue, @@ -890,7 +891,8 @@ func testInstanceClient(sType server.StorageType, client servicecatalogclient.In Message: "ConditionMessage", } instanceServer.Status = v1alpha1.ServiceInstanceStatus{ - Conditions: []v1alpha1.ServiceInstanceCondition{readyConditionTrue}, + ReconciledGeneration: instanceServer.Generation, + Conditions: []v1alpha1.ServiceInstanceCondition{readyConditionTrue}, } _, err = instanceClient.UpdateStatus(instanceServer) @@ -907,95 +909,63 @@ func testInstanceClient(sType server.StorageType, client servicecatalogclient.In return fmt.Errorf("Didn't get matching ready conditions:\nexpected: %v\n\ngot: %v", e, a) } - // Update the ServiceClassRef + // Update the instance references classRef := &v1.ObjectReference{Name: "service-class-ref"} instanceServer.Spec.ServiceClassRef = classRef + planRef := &v1.ObjectReference{Name: "service-plan-ref"} + instanceServer.Spec.ServicePlanRef = planRef returnedInstance, err := instanceClient.UpdateReferences(instanceServer) if err != nil { return fmt.Errorf("Error updating instance references: %v", err) } oldGeneration := instanceServer.Generation // check the returned object we got back from the reference subresource - if returnedInstance.Spec.ServiceClassRef == nil { - return fmt.Errorf("ServiceClassRef was not updated, instance: %+v", returnedInstance) - } - if returnedInstance.Spec.ServicePlanRef != nil { - return fmt.Errorf("ServicePlanRef was unexpectedly updated, instance: %+v", returnedInstance) - } if e, a := classRef, returnedInstance.Spec.ServiceClassRef; !reflect.DeepEqual(e, a) { return fmt.Errorf("ServiceClassRef was not set correctly, expected: %v got: %v", e, a) } + if e, a := planRef, returnedInstance.Spec.ServicePlanRef; !reflect.DeepEqual(e, a) { + return fmt.Errorf("ServiceClassRef was not set correctly, expected: %v got: %v", e, a) + } if oldGeneration != returnedInstance.Generation { return fmt.Errorf("Generation was changed, expected: %q got: %q", oldGeneration, returnedInstance.Generation) } - // re-fetch the instance by name and check its conditions + // re-fetch the instance by name and check that the service class ref was changed instanceServer, err = instanceClient.Get(name, metav1.GetOptions{}) if err != nil { return fmt.Errorf("error getting instance (%s)", err) } - if instanceServer.Spec.ServiceClassRef == nil { - return fmt.Errorf("ServiceClassRef was not updated, instance: %+v", instanceServer) - } - if instanceServer.Spec.ServicePlanRef != nil { - return fmt.Errorf("ServicePlanRef was unexpectedly updated, instance: %+v", instanceServer) - } if e, a := classRef, instanceServer.Spec.ServiceClassRef; !reflect.DeepEqual(e, a) { return fmt.Errorf("ServiceClassRef was not set correctly, expected: %v got: %v", e, a) } + if e, a := planRef, instanceServer.Spec.ServicePlanRef; !reflect.DeepEqual(e, a) { + return fmt.Errorf("ServiceClassRef was not set correctly, expected: %v got: %v", e, a) + } if oldGeneration != instanceServer.Generation { return fmt.Errorf("Generation was changed, expected: %q got: %q", oldGeneration, instanceServer.Generation) } - // Update the ServicePlanRef - planRef := &v1.ObjectReference{Name: "service-plan-ref"} - instanceServer.Spec.ServicePlanRef = planRef - returnedInstance, err = instanceClient.UpdateReferences(instanceServer) - if err != nil { - return fmt.Errorf("Error updating instance references: %v", err) - } - oldGeneration = instanceServer.Generation - - // check the object returned from the reference endpoint - if returnedInstance.Spec.ServicePlanRef == nil { - return fmt.Errorf("ServicePlanRef was not updated, instance: %+v", returnedInstance) - } - if e, a := planRef, returnedInstance.Spec.ServicePlanRef; !reflect.DeepEqual(e, a) { - return fmt.Errorf("ServicePlanRef was not set correctly, expected: %v got: %v", e, a) - } - // Make sure ServiceClassRef was not changed - if returnedInstance.Spec.ServiceClassRef == nil { - return fmt.Errorf("ServiceClassRef was cleared, instance: %+v", returnedInstance) - } - if e, a := classRef, returnedInstance.Spec.ServiceClassRef; !reflect.DeepEqual(e, a) { - return fmt.Errorf("ServiceClassRef was modified unexpectedly, expected: %v got: %v", e, a) - } + // update the instance's spec + updateRequests := instanceServer.Spec.UpdateRequests + 1 + expectedGeneration := instanceServer.Generation + 1 + instanceServer.Spec.UpdateRequests = updateRequests - if oldGeneration != returnedInstance.Generation { - return fmt.Errorf("Generation was changed, expected: %q got: %q", oldGeneration, returnedInstance.Generation) + _, err = instanceClient.Update(instanceServer) + if err != nil { + return fmt.Errorf("Error updating instance: %v", err) } - // re-fetch the instance by name and check its conditions + // re-fetch the instance by name and check that spec changes were accepted and + // caused a bump to the generation instanceServer, err = instanceClient.Get(name, metav1.GetOptions{}) if err != nil { return fmt.Errorf("error getting instance (%s)", err) } - if instanceServer.Spec.ServicePlanRef == nil { - return fmt.Errorf("ServicePlanRef was not updated, instance: %+v", instanceServer) - } - if e, a := planRef, instanceServer.Spec.ServicePlanRef; !reflect.DeepEqual(e, a) { - return fmt.Errorf("ServicePlanRef was not set correctly, expected: %v got: %v", e, a) + if e, a := updateRequests, instanceServer.Spec.UpdateRequests; e != a { + return fmt.Errorf("unexpected UpdateRequests: expected: %v, got %v", e, a) } - // Make sure ServiceClassRef was not changed - if instanceServer.Spec.ServiceClassRef == nil { - return fmt.Errorf("ServiceClassRef was cleared, instance: %+v", instanceServer) - } - if e, a := classRef, instanceServer.Spec.ServiceClassRef; !reflect.DeepEqual(e, a) { - return fmt.Errorf("ServiceClassRef was modified unexpectedly, expected: %v got: %v", e, a) - } - - if oldGeneration != instanceServer.Generation { - return fmt.Errorf("Generation was changed, expected: %q got: %q", oldGeneration, instanceServer.Generation) + if e, a := expectedGeneration, instanceServer.Generation; e != a { + return fmt.Errorf("unexpected generation: expected %v, got %v", e, a) } // delete the instance, set its finalizers to nil, update it, then ensure it is actually diff --git a/test/integration/controller_test.go b/test/integration/controller_test.go index 12f7b8107cb..ff66214c5cb 100644 --- a/test/integration/controller_test.go +++ b/test/integration/controller_test.go @@ -79,6 +79,7 @@ func truePtr() *bool { // - add Broker // - verify ServiceClasses added // - provision Instance +// - update Instance // - make Binding // - unbind // - deprovision @@ -95,6 +96,11 @@ func TestBasicFlowsSync(t *testing.T) { Async: false, }, }, + UpdateInstanceReaction: &fakeosb.UpdateInstanceReaction{ + Response: &osb.UpdateInstanceResponse{ + Async: false, + }, + }, BindReaction: &fakeosb.BindReaction{ Response: &osb.BindResponse{ Credentials: map[string]interface{}{ @@ -179,6 +185,28 @@ func TestBasicFlowsSync(t *testing.T) { ) } + // Update Instance + updateRequests := retInst.Spec.UpdateRequests + 1 + retInst.Spec.UpdateRequests = updateRequests + if _, err := client.ServiceInstances(testNamespace).Update(retInst); err != nil { + t.Fatalf("error updating Instance: %v", err) + } + + if err := util.WaitForInstanceCondition(client, testNamespace, testInstanceName, v1alpha1.ServiceInstanceCondition{ + Type: v1alpha1.ServiceInstanceConditionReady, + Status: v1alpha1.ConditionTrue, + }); err != nil { + t.Fatalf("error waiting for instance to become ready: %v", err) + } + + retInst, err = client.ServiceInstances(instance.Namespace).Get(instance.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("error getting instance %s/%s back", instance.Namespace, instance.Name) + } + if e, a := updateRequests, retInst.Spec.UpdateRequests; e != a { + t.Fatalf("unexpected updateRequets in instance spec: expected %v, got %v", e, a) + } + // Binding test begins here //----------------- @@ -264,6 +292,11 @@ func TestBasicFlowsAsync(t *testing.T) { State: osb.StateSucceeded, }, }, + UpdateInstanceReaction: &fakeosb.UpdateInstanceReaction{ + Response: &osb.UpdateInstanceResponse{ + Async: true, + }, + }, BindReaction: &fakeosb.BindReaction{ Response: &osb.BindResponse{ Credentials: map[string]interface{}{ @@ -348,6 +381,28 @@ func TestBasicFlowsAsync(t *testing.T) { ) } + // Update Instance + updateRequests := retInst.Spec.UpdateRequests + 1 + retInst.Spec.UpdateRequests = updateRequests + if _, err := client.ServiceInstances(testNamespace).Update(retInst); err != nil { + t.Fatalf("error updating Instance: %v", err) + } + + if err := util.WaitForInstanceCondition(client, testNamespace, testInstanceName, v1alpha1.ServiceInstanceCondition{ + Type: v1alpha1.ServiceInstanceConditionReady, + Status: v1alpha1.ConditionTrue, + }); err != nil { + t.Fatalf("error waiting for instance to become ready: %v", err) + } + + retInst, err = client.ServiceInstances(instance.Namespace).Get(instance.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("error getting instance %s/%s back", instance.Namespace, instance.Name) + } + if e, a := updateRequests, retInst.Spec.UpdateRequests; e != a { + t.Fatalf("unexpected updateRequets in instance spec: expected %v, got %v", e, a) + } + // Binding test begins here //----------------- @@ -732,6 +787,11 @@ func TestBasicFlowsWithOriginatingIdentity(t *testing.T) { Async: false, }, }, + UpdateInstanceReaction: &fakeosb.UpdateInstanceReaction{ + Response: &osb.UpdateInstanceResponse{ + Async: false, + }, + }, BindReaction: &fakeosb.BindReaction{ Response: &osb.BindResponse{ Credentials: map[string]interface{}{ @@ -824,35 +884,35 @@ func TestBasicFlowsWithOriginatingIdentity(t *testing.T) { } // Update Instance - // TODO: Un-comment the update instance part of this test when we support updates - // catalogClient, err = changeUsernameForCatalogClient(catalogClient, catalogClientConfig, testUpdaterUsername) - // if err != nil { - // t.Fatalf("could not change the username for the catalog client: %v", err) - // } - - // client = catalogClient.ServicecatalogV1alpha1() - - // if _, err := client.ServiceInstances(testNamespace).Update(retInst); err != nil { - // t.Fatalf("error updating Instance: %v", err) - // } - - // if err := util.WaitForInstanceCondition(client, testNamespace, testInstanceName, v1alpha1.ServiceInstanceCondition{ - // Type: v1alpha1.ServiceInstanceConditionReady, - // Status: v1alpha1.ConditionTrue, - // }); err != nil { - // t.Fatalf("error waiting for instance to become ready: %v", err) - // } - - // retInst, err = client.ServiceInstances(instance.Namespace).Get(instance.Name, metav1.GetOptions{}) - // if err != nil { - // t.Fatalf("error getting instance %s/%s back", instance.Namespace, instance.Name) - // } - // if retInst.Spec.UserInfo == nil { - // t.Fatalf("instance spec does not include creating user info") - // } - // if e, a := testUpdaterUsername, retInst.Spec.UserInfo.Username; e != a { - // t.Fatalf("unexpected updating user name in instance spec: expected %v, got %v", e, a) - // } + catalogClient, err = changeUsernameForCatalogClient(catalogClient, catalogClientConfig, testUpdaterUsername) + if err != nil { + t.Fatalf("could not change the username for the catalog client: %v", err) + } + + client = catalogClient.ServicecatalogV1alpha1() + + retInst.Spec.UpdateRequests = retInst.Spec.UpdateRequests + 1 + if _, err := client.ServiceInstances(testNamespace).Update(retInst); err != nil { + t.Fatalf("error updating Instance: %v", err) + } + + if err := util.WaitForInstanceCondition(client, testNamespace, testInstanceName, v1alpha1.ServiceInstanceCondition{ + Type: v1alpha1.ServiceInstanceConditionReady, + Status: v1alpha1.ConditionTrue, + }); err != nil { + t.Fatalf("error waiting for instance to become ready: %v", err) + } + + retInst, err = client.ServiceInstances(instance.Namespace).Get(instance.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("error getting instance %s/%s back", instance.Namespace, instance.Name) + } + if retInst.Spec.UserInfo == nil { + t.Fatalf("instance spec does not include creating user info") + } + if e, a := testUpdaterUsername, retInst.Spec.UserInfo.Username; e != a { + t.Fatalf("unexpected updating user name in instance spec: expected %v, got %v", e, a) + } // Binding test begins here //----------------- From 65ba4556c6d829220c25434be113a4ade69f0599 Mon Sep 17 00:00:00 2001 From: staebler Date: Wed, 4 Oct 2017 20:29:08 -0400 Subject: [PATCH 2/2] Only do orphan mitigation when provisioning. --- pkg/controller/controller_instance.go | 52 ++++++++++++++-------- pkg/controller/controller_instance_test.go | 4 +- 2 files changed, 36 insertions(+), 20 deletions(-) diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index a3b6cc6b358..c2764d29581 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -741,7 +741,7 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance reason, fmt.Sprintf("ClusterServiceBroker returned a failure for %v call; operation will not be retried: %v", provisionOrUpdateText, s)) - if shouldStartOrphanMitigation(httpErr.StatusCode) { + if isProvisioning && shouldStartOrphanMitigation(httpErr.StatusCode) { setServiceInstanceStartOrphanMitigation(toUpdate) if _, err := c.updateServiceInstanceStatus(toUpdate); err != nil { @@ -770,21 +770,36 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance urlErr, ok := err.(*url.Error) if ok && urlErr.Timeout() { + var ( + reason string + message string + ) + if isProvisioning { + reason = errorErrorCallingProvisionReason + } else { + reason = errorErrorCallingUpdateInstanceReason + } + message = "Communication with the ClusterServiceBroker timed out; operation will not be retried: " + s // Communication to the broker timed out. Treat as terminal failure and // begin orphan mitigation. setServiceInstanceCondition( toUpdate, v1alpha1.ServiceInstanceConditionReady, v1alpha1.ConditionFalse, - errorErrorCallingProvisionReason, - "Communication with the ClusterServiceBroker timed out; operation will not be retried: "+s) + reason, + message) setServiceInstanceCondition( toUpdate, v1alpha1.ServiceInstanceConditionFailed, v1alpha1.ConditionTrue, - errorErrorCallingProvisionReason, - "Communication with the ClusterServiceBroker timed out; operation will not be retried: "+s) - setServiceInstanceStartOrphanMitigation(toUpdate) + reason, + message) + + if isProvisioning { + setServiceInstanceStartOrphanMitigation(toUpdate) + } else { + c.clearServiceInstanceCurrentOperation(toUpdate) + } if _, err := c.updateServiceInstanceStatus(toUpdate); err != nil { return err @@ -931,6 +946,7 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se // operation we're polling for. This is more readable than checking the // status in various places. mitigatingOrphan := instance.Status.OrphanMitigationInProgress + provisioning := instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationProvision && !mitigatingOrphan deleting := false if instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationDeprovision || mitigatingOrphan { deleting = true @@ -956,7 +972,7 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se s) } - if deleting { + if !provisioning { c.clearServiceInstanceCurrentOperation(toUpdate) } else { setServiceInstanceStartOrphanMitigation(toUpdate) @@ -1082,7 +1098,7 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se s) } - if deleting { + if !provisioning { c.clearServiceInstanceCurrentOperation(toUpdate) } else { setServiceInstanceStartOrphanMitigation(toUpdate) @@ -1120,10 +1136,10 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se case deleting: reason = asyncDeprovisioningReason message = asyncDeprovisioningMessage - case instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationProvision: + case provisioning: reason = asyncProvisioningReason message = asyncProvisioningMessage - case instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationUpdate: + default: reason = asyncUpdatingInstanceReason message = asyncUpdatingInstanceMessage } @@ -1160,7 +1176,7 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se s) } - if deleting { + if !provisioning { c.clearServiceInstanceCurrentOperation(toUpdate) } else { setServiceInstanceStartOrphanMitigation(toUpdate) @@ -1197,12 +1213,12 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se reason = successDeprovisionReason message = successDeprovisionMessage actionText = "deprovisioned" - case instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationProvision: + case provisioning: readyStatus = v1alpha1.ConditionTrue reason = successProvisionReason message = successProvisionMessage actionText = "provisioned" - case instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationUpdate: + default: readyStatus = v1alpha1.ConditionTrue reason = successUpdateInstanceReason message = successUpdateInstanceMessage @@ -1256,9 +1272,9 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se switch { case deleting: actionText = "deprovisioning" - case instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationProvision: + case provisioning: actionText = "provisioning" - case instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationUpdate: + default: actionText = "updating" } s := fmt.Sprintf("Error %s ServiceInstance \"%s/%s\" of ServiceClass %q at ClusterServiceBroker %q: %q", actionText, instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName, description) @@ -1281,11 +1297,11 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se readyCond = v1alpha1.ConditionUnknown reason = errorDeprovisionCalledReason msg = "Deprovision call failed: " + s - case instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationProvision: + case provisioning: readyCond = v1alpha1.ConditionFalse reason = errorProvisionCallFailedReason msg = "Provision call failed: " + s - case instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationUpdate: + default: readyCond = v1alpha1.ConditionFalse reason = errorUpdateInstanceCallFailedReason msg = "Update call failed: " + s @@ -1336,7 +1352,7 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se s) } - if deleting { + if !provisioning { c.clearServiceInstanceCurrentOperation(toUpdate) } else { setServiceInstanceStartOrphanMitigation(toUpdate) diff --git a/pkg/controller/controller_instance_test.go b/pkg/controller/controller_instance_test.go index 4b23be4bcfd..2665058409b 100644 --- a/pkg/controller/controller_instance_test.go +++ b/pkg/controller/controller_instance_test.go @@ -3569,7 +3569,7 @@ func TestReconcileServiceInstanceWithUpdateFailure(t *testing.T) { assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, testServicePlanName, instance) updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) - assertServiceInstanceRequestFailingError(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, errorUpdateInstanceCallFailedReason, "ClusterServiceBrokerReturnedFailure", instance) + assertServiceInstanceRequestFailingErrorNoOrphanMitigation(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, errorUpdateInstanceCallFailedReason, "ClusterServiceBrokerReturnedFailure", instance) events := getRecordedEvents(testController) assertNumEvents(t, events, 1) @@ -3959,5 +3959,5 @@ func TestPollServiceInstanceAsyncFailureUpdating(t *testing.T) { assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceRequestFailingError(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, errorUpdateInstanceCallFailedReason, errorUpdateInstanceCallFailedReason, instance) + assertServiceInstanceRequestFailingErrorNoOrphanMitigation(t, updatedServiceInstance, v1alpha1.ServiceInstanceOperationUpdate, errorUpdateInstanceCallFailedReason, errorUpdateInstanceCallFailedReason, instance) }