From 67b0af68b6ca304d944cd8d6d4963cbd68e4efe1 Mon Sep 17 00:00:00 2001 From: staebler Date: Tue, 5 Sep 2017 16:41:09 -0400 Subject: [PATCH] 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 | 11 + pkg/controller/controller_broker.go | 8 + pkg/controller/controller_instance.go | 288 ++++++--- pkg/controller/controller_instance_test.go | 590 ++++++++++++++++++ pkg/controller/controller_test.go | 61 +- pkg/openapi/openapi_generated.go | 9 +- .../servicecatalog/instance/strategy.go | 42 +- .../servicecatalog/instance/strategy_test.go | 69 +- test/integration/clientset_test.go | 29 +- test/integration/controller_test.go | 118 +++- 14 files changed, 1199 insertions(+), 243 deletions(-) diff --git a/pkg/apis/servicecatalog/types.go b/pkg/apis/servicecatalog/types.go index fe3410d8c71..beb4cf4810c 100644 --- a/pkg/apis/servicecatalog/types.go +++ b/pkg/apis/servicecatalog/types.go @@ -370,12 +370,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 @@ -441,6 +435,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 b738a3d0d12..662b925a798 100644 --- a/pkg/apis/servicecatalog/v1alpha1/types.generated.go +++ b/pkg/apis/servicecatalog/v1alpha1/types.generated.go @@ -6782,7 +6782,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 != "" @@ -6793,9 +6793,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++ @@ -7008,6 +7008,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 { @@ -7169,6 +7188,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 @@ -7180,16 +7211,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 } @@ -7197,21 +7228,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 } @@ -7219,21 +7250,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 } @@ -7248,13 +7279,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 } @@ -7269,13 +7300,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 } @@ -7288,23 +7319,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 } @@ -7312,21 +7343,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 } @@ -7334,21 +7365,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 } @@ -7363,18 +7394,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) } @@ -12214,7 +12267,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 63f142f5eb0..bdece9429a2 100644 --- a/pkg/apis/servicecatalog/v1alpha1/types.go +++ b/pkg/apis/servicecatalog/v1alpha1/types.go @@ -440,6 +440,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 65fb1919f62..83e7f53cd64 100644 --- a/pkg/apis/servicecatalog/v1alpha1/zz_generated.conversion.go +++ b/pkg/apis/servicecatalog/v1alpha1/zz_generated.conversion.go @@ -733,6 +733,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 } @@ -750,6 +751,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 0e342fb511f..2066b8c54c3 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 } @@ -234,8 +236,17 @@ func internalValidateServiceInstanceUpdateAllowed(new *sc.ServiceInstance, old * // 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"), old.Spec.UpdateRequests, "new updateRequests value must not be less than the old one")) + } + return allErrs } diff --git a/pkg/controller/controller_broker.go b/pkg/controller/controller_broker.go index 9d85cc6780f..546c08b7cc0 100644 --- a/pkg/controller/controller_broker.go +++ b/pkg/controller/controller_broker.go @@ -85,6 +85,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" @@ -104,6 +106,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" @@ -113,6 +117,8 @@ const ( successUnboundReason string = "UnboundSuccessfully" 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" @@ -121,6 +127,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 d45f3719c86..33806c42faa 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -616,25 +616,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) @@ -653,11 +637,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. @@ -665,18 +705,29 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance } } - glog.V(4).Infof("Provisioning a new ServiceInstance %v/%v of ServiceClass %v at ServiceBroker %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 ServiceBroker %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 ServiceBroker %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 ServiceBroker %q: %s", instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName, httpErr) + s := fmt.Sprintf("Error %v ServiceInstance \"%s/%s\" of ServiceClass %q at ServiceBroker %q: %s", provisioningOrUpdatingText, instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName, httpErr) glog.Warning(s) - c.recorder.Event(instance, api.EventTypeWarning, errorProvisionCallFailedReason, s) + c.recorder.Event(instance, api.EventTypeWarning, reason, s) setServiceInstanceCondition( toUpdate, @@ -688,8 +739,8 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance toUpdate, v1alpha1.ServiceInstanceConditionReady, v1alpha1.ConditionFalse, - errorProvisionCallFailedReason, - "ServiceBroker returned a failure for provision call; operation will not be retried: "+s) + reason, + fmt.Sprintf("ServiceBroker returned a failure for %v call; operation will not be retried: %v", provisionOrUpdateText, s)) c.clearServiceInstanceCurrentOperation(toUpdate) if _, err := c.updateServiceInstanceStatus(toUpdate); err != nil { return err @@ -697,16 +748,20 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance return nil } - s := fmt.Sprintf("Error provisioning ServiceInstance \"%s/%s\" of ServiceClass %q at ServiceBroker %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 ServiceBroker %q: %s", provisioningOrUpdatingText, instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName, err) glog.Warning(s) - c.recorder.Event(instance, api.EventTypeWarning, errorErrorCallingProvisionReason, s) + c.recorder.Event(instance, api.EventTypeWarning, reason, s) setServiceInstanceCondition( 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) @@ -731,8 +786,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 } @@ -741,10 +796,26 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance // and we need to add it to the polling queue. ServiceBroker 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 ServiceBroker %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 ServiceBroker %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 } @@ -752,12 +823,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 @@ -767,9 +844,15 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance return err } - c.recorder.Eventf(instance, api.EventTypeNormal, asyncProvisioningReason, asyncProvisioningMessage) + c.recorder.Eventf(instance, api.EventTypeNormal, reason, message) } else { - glog.V(5).Infof("Successfully provisioned ServiceInstance %v/%v of ServiceClass %v at ServiceBroker %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 ServiceBroker %v: response: %+v", provisionedOrUpdatedText, instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName, response) toUpdate.Status.ExternalProperties = toUpdate.Status.InProgressProperties c.clearServiceInstanceCurrentOperation(toUpdate) @@ -779,14 +862,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, api.EventTypeNormal, successProvisionReason, successProvisionMessage) + c.recorder.Eventf(instance, api.EventTypeNormal, reason, message) } return nil } @@ -802,14 +885,6 @@ func (c *controller) pollServiceInstanceInternal(instance *v1alpha1.ServiceInsta } func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, servicePlan *v1alpha1.ServicePlan, brokerName string, brokerClient osb.Client, instance *v1alpha1.ServiceInstance) error { - // There are some conditions that are different if we're - // deleting, this is more readable than checking the - // timestamps in various places. - deleting := false - if instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationDeprovision { - deleting = true - } - // OperationStartTime must be set because we are polling an in-progress // operation. If it is not set, this is a logical error. Let's bail out. if instance.Status.OperationStartTime == nil { @@ -876,7 +951,7 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se // If the operation was for delete and we receive a http.StatusGone, // this is considered a success as per the spec, so mark as deleted // and remove any finalizers. - if osb.IsGoneError(err) && deleting { + if osb.IsGoneError(err) && instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationDeprovision { clone, err := api.Scheme.DeepCopy(instance) if err != nil { return err @@ -971,12 +1046,16 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se var message string var reason string - if deleting { + switch instance.Status.CurrentOperation { + case v1alpha1.ServiceInstanceOperationDeprovision: reason = asyncDeprovisioningReason message = asyncDeprovisioningMessage - } else { + case v1alpha1.ServiceInstanceOperationProvision: reason = asyncProvisioningReason message = asyncProvisioningMessage + case v1alpha1.ServiceInstanceOperationUpdate: + reason = asyncUpdatingInstanceReason + message = asyncUpdatingInstanceMessage } if response.Description != nil { @@ -1027,6 +1106,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 instance.Status.CurrentOperation { + case v1alpha1.ServiceInstanceOperationDeprovision: + readyStatus = v1alpha1.ConditionFalse + reason = successDeprovisionReason + message = successDeprovisionMessage + actionText = "deprovisioned" + case v1alpha1.ServiceInstanceOperationProvision: + readyStatus = v1alpha1.ConditionTrue + reason = successProvisionReason + message = successProvisionMessage + actionText = "provisioned" + case 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) @@ -1034,45 +1137,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, - ) + setServiceInstanceCondition( + toUpdate, + v1alpha1.ServiceInstanceConditionReady, + readyStatus, + reason, + message, + ) + if instance.Status.CurrentOperation == v1alpha1.ServiceInstanceOperationDeprovision { // Clear the finalizer if finalizers := sets.NewString(toUpdate.Finalizers...); finalizers.Has(v1alpha1.FinalizerServiceCatalog) { finalizers.Delete(v1alpha1.FinalizerServiceCatalog) toUpdate.Finalizers = finalizers.List() } + } - if _, err := c.updateServiceInstanceStatus(toUpdate); err != nil { - return err - } - - c.recorder.Event(instance, api.EventTypeNormal, successDeprovisionReason, successDeprovisionMessage) - glog.V(5).Infof("Successfully deprovisioned ServiceInstance %v/%v of ServiceClass %v at ServiceBroker %v", instance.Namespace, instance.Name, serviceClass.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, api.EventTypeNormal, reason, message) + glog.V(5).Infof("Successfully %v ServiceInstance %v/%v of ServiceClass %v at ServiceBroker %v", actionText, instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName) + err = c.finishPollingServiceInstance(instance) if err != nil { return err @@ -1082,7 +1173,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 ServiceBroker %q: %q", instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName, description) + actionText := "" + switch instance.Status.CurrentOperation { + case v1alpha1.ServiceInstanceOperationProvision: + actionText = "provisioning" + case v1alpha1.ServiceInstanceOperationUpdate: + actionText = "updating" + case v1alpha1.ServiceInstanceOperationDeprovision: + actionText = "deprovisioning" + } + s := fmt.Sprintf("Error %s ServiceInstance \"%s/%s\" of ServiceClass %q at ServiceBroker %q: %q", actionText, instance.Namespace, instance.Name, serviceClass.Spec.ExternalName, brokerName, description) c.recorder.Event(instance, api.EventTypeWarning, errorDeprovisionCalledReason, s) clone, err := api.Scheme.DeepCopy(instance) @@ -1092,13 +1192,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 instance.Status.CurrentOperation { + case v1alpha1.ServiceInstanceOperationProvision: + readyCond = v1alpha1.ConditionFalse + reason = errorProvisionCallFailedReason + msg = "Provision call failed: " + s + case v1alpha1.ServiceInstanceOperationUpdate: + readyCond = v1alpha1.ConditionFalse + reason = errorUpdateInstanceCallFailedReason + msg = "Update call failed: " + s + case v1alpha1.ServiceInstanceOperationDeprovision: readyCond = v1alpha1.ConditionUnknown reason = errorDeprovisionCalledReason - msg = "Deprovision call failed:" + s + msg = "Deprovision call failed: " + s } setServiceInstanceCondition( toUpdate, @@ -1269,6 +1380,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 f6b2392e58f..2175cfb7bad 100644 --- a/pkg/controller/controller_instance_test.go +++ b/pkg/controller/controller_instance_test.go @@ -2514,3 +2514,593 @@ func TestReconcileServiceInstanceWithSecretParameters(t *testing.T) { t.Fatalf("Received unexpected event: %v", a) } } + +// TestReconcileServiceInstanceUpdateParameters tests updating a +// ServiceInstance with new paramaters +func TestReconcileServiceInstanceUpdateParameters(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + UpdateInstanceReaction: &fakeosb.UpdateInstanceReaction{ + Response: &osb.UpdateInstanceResponse{}, + }, + }) + + sharedInformers.ServiceBrokers().Informer().GetStore().Add(getTestServiceBroker()) + 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 := fakeServiceBrokerClient.Actions() + assertNumberOfServiceBrokerActions(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 := api.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) + } +} + +// TestReconcileServiceInstanceUpdatePlan tests updating a +// ServiceInstance with a new plan +func TestReconcileServiceInstanceUpdatePlan(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + UpdateInstanceReaction: &fakeosb.UpdateInstanceReaction{ + Response: &osb.UpdateInstanceResponse{}, + }, + }) + + sharedInformers.ServiceBrokers().Informer().GetStore().Add(getTestServiceBroker()) + 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: "old-plan-name", + Parameters: oldParametersRaw, + ParametersChecksum: oldParametersChecksum, + } + + parameters := instanceParameters{Name: "test-param", Args: make(map[string]string)} + parameters.Args["first"] = "first-arg" + parameters.Args["second"] = "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 := fakeServiceBrokerClient.Actions() + assertNumberOfServiceBrokerActions(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") + } + + // 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 := api.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, fakeServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + UpdateInstanceReaction: &fakeosb.UpdateInstanceReaction{ + Error: errors.New("fake update failure"), + }, + }) + + sharedInformers.ServiceBrokers().Informer().GetStore().Add(getTestServiceBroker()) + 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 := fakeServiceBrokerClient.Actions() + assertNumberOfServiceBrokerActions(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() + 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, 1) + + expectedEvent := api.EventTypeWarning + " " + errorErrorCallingUpdateInstanceReason + " " + "Error updating ServiceInstance \"test-ns/test-instance\" of ServiceClass \"test-serviceclass\" at ServiceBroker \"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, fakeServiceBrokerClient, 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.ServiceBrokers().Informer().GetStore().Add(getTestServiceBroker()) + 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 := fakeServiceBrokerClient.Actions() + assertNumberOfServiceBrokerActions(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, "ServiceBrokerReturnedFailure", instance) + + events := getRecordedEvents(testController) + assertNumEvents(t, events, 1) + + expectedEvent := api.EventTypeWarning + " " + errorUpdateInstanceCallFailedReason + " " + "Error updating ServiceInstance \"test-ns/test-instance\" of ServiceClass \"test-serviceclass\" at ServiceBroker \"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) + } +} + +// 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, fakeServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + UpdateInstanceReaction: &fakeosb.UpdateInstanceReaction{ + Response: &osb.UpdateInstanceResponse{ + Async: true, + OperationKey: &key, + }, + }, + }) + + addGetNamespaceReaction(fakeKubeClient) + + sharedInformers.ServiceBrokers().Informer().GetStore().Add(getTestServiceBroker()) + 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 := fakeServiceBrokerClient.Actions() + assertNumberOfServiceBrokerActions(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, fakeServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + PollLastOperationReaction: &fakeosb.PollLastOperationReaction{ + Response: &osb.LastOperationResponse{ + State: osb.StateInProgress, + Description: strPtr(lastOperationDescription), + }, + }, + }) + + sharedInformers.ServiceBrokers().Informer().GetStore().Add(getTestServiceBroker()) + 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 := fakeServiceBrokerClient.Actions() + assertNumberOfServiceBrokerActions(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, fakeServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + PollLastOperationReaction: &fakeosb.PollLastOperationReaction{ + Response: &osb.LastOperationResponse{ + State: osb.StateSucceeded, + Description: strPtr(lastOperationDescription), + }, + }, + }) + + sharedInformers.ServiceBrokers().Informer().GetStore().Add(getTestServiceBroker()) + 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 := fakeServiceBrokerClient.Actions() + assertNumberOfServiceBrokerActions(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, fakeServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + PollLastOperationReaction: &fakeosb.PollLastOperationReaction{ + Response: &osb.LastOperationResponse{ + State: osb.StateFailed, + }, + }, + }) + + sharedInformers.ServiceBrokers().Informer().GetStore().Add(getTestServiceBroker()) + 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 := fakeServiceBrokerClient.Actions() + assertNumberOfServiceBrokerActions(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 04a659faff0..02a7acda58d 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -535,9 +535,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{ @@ -561,13 +558,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{{ @@ -1596,6 +1622,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 } @@ -1627,6 +1655,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 @@ -1652,7 +1683,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 @@ -1674,7 +1705,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 @@ -1684,7 +1715,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: @@ -1698,6 +1729,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 } @@ -1707,7 +1740,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: @@ -1725,6 +1758,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) } @@ -2200,6 +2235,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 befc0a8ff90..ffb1813a6e2 100644 --- a/pkg/openapi/openapi_generated.go +++ b/pkg/openapi/openapi_generated.go @@ -1031,8 +1031,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 236e2877098..3900f7115a3 100644 --- a/pkg/registry/servicecatalog/instance/strategy.go +++ b/pkg/registry/servicecatalog/instance/strategy.go @@ -134,23 +134,14 @@ func (instanceRESTStrategy) PrepareForUpdate(ctx genericapirequest.Context, new, // Do not allow any updates to the Status field while updating the Spec newServiceInstance.Status = oldServiceInstance.Status - // TODO: We currently don't handle any changes to the spec in the - // reconciler. Once we do that, this check needs to be removed and - // proper validation of allowed changes needs to be implemented in - // ValidateUpdate. Also, the check for whether the generation needs - // to be updated needs to be un-commented. - newServiceInstance.Spec = oldServiceInstance.Spec - // 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) } } @@ -227,3 +218,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..f64632d4ea4 100644 --- a/pkg/registry/servicecatalog/instance/strategy_test.go +++ b/pkg/registry/servicecatalog/instance/strategy_test.go @@ -60,44 +60,59 @@ 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. 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 }{ { 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: "spec change", + 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, + }, } 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", 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", 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", e, a) } } } @@ -118,17 +133,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() + updatedInstance.Spec.UpdateRequests = updatedInstance.Spec.UpdateRequests + 1 + updateContext := contextWithUserName(updaterUserName) + instanceRESTStrategies.PrepareForUpdate(updateContext, updatedInstance, createdInstance) - // updaterUserName := "updater" - // updatedInstance := getTestInstance() - // 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 15e872a2fc7..9ffa5b0b8cd 100644 --- a/test/integration/clientset_test.go +++ b/test/integration/clientset_test.go @@ -873,7 +873,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, @@ -881,7 +882,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) @@ -898,6 +900,29 @@ 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 instance's spec + updateRequests := instanceServer.Spec.UpdateRequests + 1 + expectedGeneration := instanceServer.Generation + 1 + instanceServer.Spec.UpdateRequests = updateRequests + + _, err = instanceClient.Update(instanceServer) + if err != nil { + return fmt.Errorf("Error updating instance: %v", err) + } + + // 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 e, a := updateRequests, instanceServer.Spec.UpdateRequests; e != a { + return fmt.Errorf("unexpected UpdateRequests: expected: %v, got %v", e, a) + } + 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 // deleted if err := instanceClient.Delete(name, &metav1.DeleteOptions{}); err != nil { diff --git a/test/integration/controller_test.go b/test/integration/controller_test.go index e1feda4ab46..84356097530 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{}{ @@ -178,6 +184,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 //----------------- @@ -263,6 +291,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{}{ @@ -346,6 +379,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 //----------------- @@ -728,6 +783,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{}{ @@ -819,35 +879,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 //-----------------