Skip to content

Commit

Permalink
[WIP] Pass correct plan ID in deprovision request (for both deleting …
Browse files Browse the repository at this point in the history 
…and orphan mitigation) (openshift#1847)

* In deprovisioning pass the plan id that was used during the last provision/update operation
* Comment out t.Parallel() as a temporary fix for flaky tests caused by openshift#1848
  • Loading branch information
@nilebox
nilebox committed Mar 19, 2018
1 parent 74f73c0 commit 5e1e90d
Show file tree
Hide file tree
Showing 9 changed files with 139 additions and 164 deletions.
6 changes: 3 additions & 3 deletions pkg/apis/servicecatalog/validation/instance.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,13 +140,13 @@ func validateServiceInstanceStatus(status *sc.ServiceInstanceStatus, fldPath *fi
}

switch status.CurrentOperation {
case sc.ServiceInstanceOperationProvision, sc.ServiceInstanceOperationUpdate:
case sc.ServiceInstanceOperationProvision, sc.ServiceInstanceOperationUpdate, sc.ServiceInstanceOperationDeprovision:
if status.InProgressProperties == nil {
allErrs = append(allErrs, field.Required(fldPath.Child("inProgressProperties"), `inProgressProperties is required when currentOperation is "Provision" or "Update"`))
allErrs = append(allErrs, field.Required(fldPath.Child("inProgressProperties"), `inProgressProperties is required when currentOperation is "Provision", "Update" or "Deprovision"`))
}
default:
if status.InProgressProperties != nil {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("inProgressProperties"), `inProgressProperties must not be present when currentOperation is neither "Provision" nor "Update"`))
allErrs = append(allErrs, field.Forbidden(fldPath.Child("inProgressProperties"), `inProgressProperties must not be present when currentOperation is not "Provision", "Update" or "Deprovision"`))
}
}

Expand Down
11 changes: 1 addition & 10 deletions pkg/apis/servicecatalog/validation/instance_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ func validServiceInstanceWithInProgressDeprovision() *servicecatalog.ServiceInst
instance.Status.CurrentOperation = servicecatalog.ServiceInstanceOperationDeprovision
now := metav1.Now()
instance.Status.OperationStartTime = &now
instance.Status.InProgressProperties = validServiceInstancePropertiesState()
instance.Status.ExternalProperties = validServiceInstancePropertiesState()
return instance
}
Expand Down Expand Up @@ -221,7 +222,6 @@ func TestValidateServiceInstance(t *testing.T) {
instance: func() *servicecatalog.ServiceInstance {
i := validServiceInstanceWithInProgressProvision()
i.Status.CurrentOperation = servicecatalog.ServiceInstanceOperationDeprovision
i.Status.InProgressProperties = nil
return i
}(),
valid: true,
Expand Down Expand Up @@ -319,15 +319,6 @@ func TestValidateServiceInstance(t *testing.T) {
}(),
valid: false,
},
{
name: "in-progress deprovision with present InProgressProperties",
instance: func() *servicecatalog.ServiceInstance {
i := validServiceInstanceWithInProgressProvision()
i.Status.CurrentOperation = servicecatalog.ServiceInstanceOperationDeprovision
return i
}(),
valid: false,
},
{
name: "in-progress properties with no external plan name",
instance: func() *servicecatalog.ServiceInstance {
Expand Down
37 changes: 24 additions & 13 deletions pkg/controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -264,16 +264,9 @@ func (e *operationError) Error() string { return e.message }
// is nil. This will happen when deleting a ServiceInstance that previously
// had an update to a non-existent plan.
func (c *controller) getClusterServiceClassPlanAndClusterServiceBroker(instance *v1beta1.ServiceInstance) (*v1beta1.ClusterServiceClass, *v1beta1.ClusterServicePlan, string, osb.Client, error) {
pcb := pretty.NewContextBuilder(pretty.ServiceInstance, instance.Namespace, instance.Name)
serviceClass, err := c.serviceClassLister.Get(instance.Spec.ClusterServiceClassRef.Name)
serviceClass, brokerName, brokerClient, err := c.getClusterServiceClassAndClusterServiceBroker(instance)
if err != nil {
return nil, nil, "", nil, &operationError{
reason: errorNonexistentClusterServiceClassReason,
message: fmt.Sprintf(
"The instance references a non-existent ClusterServiceClass (K8S: %q ExternalName: %q)",
instance.Spec.ClusterServiceClassRef.Name, instance.Spec.ClusterServiceClassExternalName,
),
}
return nil, nil, "", nil, err
}

var servicePlan *v1beta1.ClusterServicePlan
Expand All @@ -290,10 +283,28 @@ func (c *controller) getClusterServiceClassPlanAndClusterServiceBroker(instance
}
}
}
return serviceClass, servicePlan, brokerName, brokerClient, nil
}

// getClusterServiceClassAndClusterServiceBroker is a sequence of operations that's done in couple of
// places so this method fetches the Service Class and creates
// a brokerClient to use for that method given an ServiceInstance.
func (c *controller) getClusterServiceClassAndClusterServiceBroker(instance *v1beta1.ServiceInstance) (*v1beta1.ClusterServiceClass, string, osb.Client, error) {
pcb := pretty.NewContextBuilder(pretty.ServiceInstance, instance.Namespace, instance.Name)
serviceClass, err := c.serviceClassLister.Get(instance.Spec.ClusterServiceClassRef.Name)
if err != nil {
return nil, "", nil, &operationError{
reason: errorNonexistentClusterServiceClassReason,
message: fmt.Sprintf(
"The instance references a non-existent ClusterServiceClass (K8S: %q ExternalName: %q)",
instance.Spec.ClusterServiceClassRef.Name, instance.Spec.ClusterServiceClassExternalName,
),
}
}

broker, err := c.brokerLister.Get(serviceClass.Spec.ClusterServiceBrokerName)
if err != nil {
return nil, nil, "", nil, &operationError{
return nil, "", nil, &operationError{
reason: errorNonexistentClusterServiceBrokerReason,
message: fmt.Sprintf(
"The instance references a non-existent broker %q",
Expand All @@ -305,7 +316,7 @@ func (c *controller) getClusterServiceClassPlanAndClusterServiceBroker(instance

authConfig, err := getAuthCredentialsFromClusterServiceBroker(c.kubeClient, broker)
if err != nil {
return nil, nil, "", nil, &operationError{
return nil, "", nil, &operationError{
reason: errorAuthCredentialsReason,
message: fmt.Sprintf(
"Error getting broker auth credentials for broker %q: %s",
Expand All @@ -318,10 +329,10 @@ func (c *controller) getClusterServiceClassPlanAndClusterServiceBroker(instance
glog.V(4).Info(pcb.Messagef("Creating client for ClusterServiceBroker %v, URL: %v", broker.Name, broker.Spec.URL))
brokerClient, err := c.brokerClientCreateFunc(clientConfig)
if err != nil {
return nil, nil, "", nil, err
return nil, "", nil, err
}

return serviceClass, servicePlan, broker.Name, brokerClient, nil
return serviceClass, broker.Name, brokerClient, nil
}

// getClusterServiceClassPlanAndClusterServiceBrokerForServiceBinding is a sequence of operations that's
Expand Down
93 changes: 50 additions & 43 deletions pkg/controller/controller_instance.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package controller

import (
stderrors "errors"
"fmt"
"net/url"

Expand Down Expand Up @@ -69,8 +70,6 @@ const (
errorOrphanMitigationFailedReason string = "OrphanMitigationFailed"
errorInvalidDeprovisionStatusReason string = "InvalidDeprovisionStatus"
errorInvalidDeprovisionStatusMessage string = "The deprovision status is invalid"
errorUnknownServicePlanReason string = "UnknownServicePlan"
errorUnknownServicePlanMessage string = "The ServicePlan is not known"

asyncProvisioningReason string = "Provisioning"
asyncProvisioningMessage string = "The instance is being provisioned asynchronously"
Expand Down Expand Up @@ -542,25 +541,26 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns
return c.handleServiceInstanceReconciliationError(instance, err)
}

serviceClass, servicePlan, brokerName, brokerClient, err := c.getClusterServiceClassPlanAndClusterServiceBroker(instance)
serviceClass, brokerName, brokerClient, err := c.getClusterServiceClassAndClusterServiceBroker(instance)
if err != nil {
return c.handleServiceInstanceReconciliationError(instance, err)
}

request, err := c.prepareDeprovisionRequest(instance, serviceClass, servicePlan)
request, inProgressProperties, err := c.prepareDeprovisionRequest(instance, serviceClass)
if err != nil {
return c.handleServiceInstanceReconciliationError(instance, err)
}

if instance.DeletionTimestamp == nil {
// Orphan mitigation
if instance.Status.OperationStartTime == nil {
// if mitigating an orphan, set the operation start time if unset
now := metav1.Now()
instance.Status.OperationStartTime = &now
}
} else {
if instance.Status.CurrentOperation != v1beta1.ServiceInstanceOperationDeprovision {
instance, err = c.recordStartOfServiceInstanceOperation(instance, v1beta1.ServiceInstanceOperationDeprovision, nil)
instance, err = c.recordStartOfServiceInstanceOperation(instance, v1beta1.ServiceInstanceOperationDeprovision, inProgressProperties)
if err != nil {
// There has been an update to the instance. Start reconciliation
// over with a fresh view of the instance.
Expand Down Expand Up @@ -1176,18 +1176,17 @@ func (c *controller) recordStartOfServiceInstanceOperation(toUpdate *v1beta1.Ser
toUpdate.Status.CurrentOperation = operation
now := metav1.Now()
toUpdate.Status.OperationStartTime = &now
toUpdate.Status.InProgressProperties = inProgressProperties
reason := ""
message := ""
switch operation {
case v1beta1.ServiceInstanceOperationProvision:
reason = provisioningInFlightReason
message = provisioningInFlightMessage
toUpdate.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired
toUpdate.Status.InProgressProperties = inProgressProperties
case v1beta1.ServiceInstanceOperationUpdate:
reason = instanceUpdatingInFlightReason
message = instanceUpdatingInFlightMessage
toUpdate.Status.InProgressProperties = inProgressProperties
case v1beta1.ServiceInstanceOperationDeprovision:
reason = deprovisioningInFlightReason
message = deprovisioningInFlightMessage
Expand Down Expand Up @@ -1289,7 +1288,7 @@ type requestHelper struct {

// prepareRequestHelper is a helper function that generates a struct with
// properties common to multiple request types.
func (c *controller) prepareRequestHelper(instance *v1beta1.ServiceInstance, serviceClass *v1beta1.ClusterServiceClass, servicePlan *v1beta1.ClusterServicePlan) (*requestHelper, error) {
func (c *controller) prepareRequestHelper(instance *v1beta1.ServiceInstance, servicePlan *v1beta1.ClusterServicePlan, setInProgressProperties bool) (*requestHelper, error) {
rh := &requestHelper{}

if utilfeature.DefaultFeatureGate.Enabled(scfeatures.OriginatingIdentity) {
Expand Down Expand Up @@ -1318,26 +1317,28 @@ func (c *controller) prepareRequestHelper(instance *v1beta1.ServiceInstance, ser
}
rh.ns = ns

parameters, parametersChecksum, rawParametersWithRedaction, err := prepareInProgressPropertyParameters(
c.kubeClient,
instance.Namespace,
instance.Spec.Parameters,
instance.Spec.ParametersFrom,
)
if err != nil {
return nil, &operationError{
reason: errorWithParameters,
message: err.Error(),
if setInProgressProperties {
parameters, parametersChecksum, rawParametersWithRedaction, err := prepareInProgressPropertyParameters(
c.kubeClient,
instance.Namespace,
instance.Spec.Parameters,
instance.Spec.ParametersFrom,
)
if err != nil {
return nil, &operationError{
reason: errorWithParameters,
message: err.Error(),
}
}
rh.parameters = parameters

rh.inProgressProperties = &v1beta1.ServiceInstancePropertiesState{
ClusterServicePlanExternalName: servicePlan.Spec.ExternalName,
ClusterServicePlanExternalID: servicePlan.Spec.ExternalID,
Parameters: rawParametersWithRedaction,
ParametersChecksum: parametersChecksum,
UserInfo: instance.Spec.UserInfo,
}
}
rh.parameters = parameters

rh.inProgressProperties = &v1beta1.ServiceInstancePropertiesState{
ClusterServicePlanExternalName: servicePlan.Spec.ExternalName,
ClusterServicePlanExternalID: servicePlan.Spec.ExternalID,
Parameters: rawParametersWithRedaction,
ParametersChecksum: parametersChecksum,
UserInfo: instance.Spec.UserInfo,
}

// osb client handles whether or not to really send this based
Expand All @@ -1353,7 +1354,7 @@ func (c *controller) prepareRequestHelper(instance *v1beta1.ServiceInstance, ser
// prepareProvisionRequest creates a provision request object to be passed to
// the broker client to provision the given instance.
func (c *controller) prepareProvisionRequest(instance *v1beta1.ServiceInstance, serviceClass *v1beta1.ClusterServiceClass, servicePlan *v1beta1.ClusterServicePlan) (*osb.ProvisionRequest, *v1beta1.ServiceInstancePropertiesState, error) {
rh, err := c.prepareRequestHelper(instance, serviceClass, servicePlan)
rh, err := c.prepareRequestHelper(instance, servicePlan, true)
if err != nil {
return nil, nil, err
}
Expand All @@ -1376,7 +1377,7 @@ func (c *controller) prepareProvisionRequest(instance *v1beta1.ServiceInstance,
// prepareUpdateInstanceRequest creates an update instance request object to be
// passed to the broker client to update the given instance.
func (c *controller) prepareUpdateInstanceRequest(instance *v1beta1.ServiceInstance, serviceClass *v1beta1.ClusterServiceClass, servicePlan *v1beta1.ClusterServicePlan) (*osb.UpdateInstanceRequest, *v1beta1.ServiceInstancePropertiesState, error) {
rh, err := c.prepareRequestHelper(instance, serviceClass, servicePlan)
rh, err := c.prepareRequestHelper(instance, servicePlan, true)
if err != nil {
return nil, nil, err
}
Expand Down Expand Up @@ -1410,39 +1411,42 @@ func (c *controller) prepareUpdateInstanceRequest(instance *v1beta1.ServiceInsta

// prepareDeprovisionRequest creates a deprovision request object to be passed
// to the broker client to deprovision the given instance.
func (c *controller) prepareDeprovisionRequest(instance *v1beta1.ServiceInstance, serviceClass *v1beta1.ClusterServiceClass, servicePlan *v1beta1.ClusterServicePlan) (*osb.DeprovisionRequest, error) {
rh, err := c.prepareRequestHelper(instance, serviceClass, servicePlan)
func (c *controller) prepareDeprovisionRequest(instance *v1beta1.ServiceInstance, serviceClass *v1beta1.ClusterServiceClass) (*osb.DeprovisionRequest, *v1beta1.ServiceInstancePropertiesState, error) {
rh, err := c.prepareRequestHelper(instance, nil, false)
if err != nil {
return nil, err
return nil, nil, err
}

var servicePlanExternalID string
if instance.Status.ExternalProperties != nil {
servicePlanExternalID = instance.Status.ExternalProperties.ClusterServicePlanExternalID
} else if servicePlan != nil {
servicePlanExternalID = servicePlan.Spec.ExternalID
// The plan reference in the spec might be updated since the latest
// provisioning/update request, thus we need to take values from the original
// provisioning request instead that we previously stored in status
if instance.Status.CurrentOperation != "" || instance.Status.OrphanMitigationInProgress {
if instance.Status.InProgressProperties == nil {
return nil, nil, stderrors.New("InProgressProperties must be set when there is an operation or orphan mitigation in progress")
}
rh.inProgressProperties = instance.Status.InProgressProperties
} else {
return nil, &operationError{
reason: errorUnknownServicePlanReason,
message: errorUnknownServicePlanMessage,
if instance.Status.ExternalProperties == nil {
return nil, nil, stderrors.New("ExternalProperties must be set before deprovisioning")
}
rh.inProgressProperties = instance.Status.ExternalProperties
}

request := &osb.DeprovisionRequest{
InstanceID: instance.Spec.ExternalID,
ServiceID: serviceClass.Spec.ExternalID,
PlanID: servicePlanExternalID,
PlanID: rh.inProgressProperties.ClusterServicePlanExternalID,
OriginatingIdentity: rh.originatingIdentity,
AcceptsIncomplete: true,
}

return request, nil
return request, rh.inProgressProperties, nil
}

// preparePollServiceInstanceRequest creates a request object to be passed to
// the broker client to query the given instance's last operation endpoint.
func (c *controller) prepareServiceInstanceLastOperationRequest(instance *v1beta1.ServiceInstance, serviceClass *v1beta1.ClusterServiceClass, servicePlan *v1beta1.ClusterServicePlan) (*osb.LastOperationRequest, error) {
rh, err := c.prepareRequestHelper(instance, serviceClass, servicePlan)
rh, err := c.prepareRequestHelper(instance, servicePlan, false)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1558,6 +1562,9 @@ func (c *controller) processProvisionFailure(instance *v1beta1.ServiceInstance,
err = fmt.Errorf(failedCond.Message)
} else {
clearServiceInstanceCurrentOperation(instance)
// Deprovisioning is not required for provisioning that has failed with an
// error that doesn't require orphan mitigation
instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusNotRequired
instance.Status.ReconciledGeneration = instance.Status.ObservedGeneration
}

Expand Down
Loading

0 comments on commit 5e1e90d

Please sign in to comment.