Skip to content

Commit

Permalink
Pass correct plan ID in deprovision request (for both deleting and or…
Browse files Browse the repository at this point in the history 
…phan mitigation) (openshift#1803)

* In deprovisioning pass the plan id that was used during the last provision/update operation

* Leave a TODO

* Fix nil pointer dereference panic

* Rewrite handling deletion for provisioning that is in progress

* Fix review comments

* Fix tests

* Fix deprovisioning when no orphan mitigation is required

* Tests cleanup
  • Loading branch information
@nilebox @kibbles-n-bytes
nilebox authored and kibbles-n-bytes committed Mar 15, 2018
1 parent cc02f0e commit 4b5d159
Show file tree
Hide file tree
Showing 6 changed files with 128 additions and 153 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 4b5d159

Please sign in to comment.