Skip to content

Commit

Permalink
Revert "Pass correct plan ID in deprovision request (for both deletin…
Browse files Browse the repository at this point in the history 
…g and orphan mitigation) (openshift#1803)" (openshift#1843)

This reverts commit 4b5d159.
  • Loading branch information
@n3wscott
n3wscott authored and Doug Davis committed Mar 16, 2018
1 parent 94b5795 commit 82fc6e4
Show file tree
Hide file tree
Showing 6 changed files with 153 additions and 128 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, sc.ServiceInstanceOperationDeprovision:
case sc.ServiceInstanceOperationProvision, sc.ServiceInstanceOperationUpdate:
if status.InProgressProperties == nil {
allErrs = append(allErrs, field.Required(fldPath.Child("inProgressProperties"), `inProgressProperties is required when currentOperation is "Provision", "Update" or "Deprovision"`))
allErrs = append(allErrs, field.Required(fldPath.Child("inProgressProperties"), `inProgressProperties is required when currentOperation is "Provision" or "Update"`))
}
default:
if status.InProgressProperties != nil {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("inProgressProperties"), `inProgressProperties must not be present when currentOperation is not "Provision", "Update" or "Deprovision"`))
allErrs = append(allErrs, field.Forbidden(fldPath.Child("inProgressProperties"), `inProgressProperties must not be present when currentOperation is neither "Provision" nor "Update"`))
}
}

Expand Down
11 changes: 10 additions & 1 deletion pkg/apis/servicecatalog/validation/instance_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ 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 @@ -222,6 +221,7 @@ 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,6 +319,15 @@ 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: 13 additions & 24 deletions pkg/controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -264,9 +264,16 @@ 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) {
serviceClass, brokerName, brokerClient, err := c.getClusterServiceClassAndClusterServiceBroker(instance)
pcb := pretty.NewContextBuilder(pretty.ServiceInstance, instance.Namespace, instance.Name)
serviceClass, err := c.serviceClassLister.Get(instance.Spec.ClusterServiceClassRef.Name)
if err != nil {
return nil, nil, "", nil, err
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,
),
}
}

var servicePlan *v1beta1.ClusterServicePlan
Expand All @@ -283,28 +290,10 @@ 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, &operationError{
return nil, nil, "", nil, &operationError{
reason: errorNonexistentClusterServiceBrokerReason,
message: fmt.Sprintf(
"The instance references a non-existent broker %q",
Expand All @@ -316,7 +305,7 @@ func (c *controller) getClusterServiceClassAndClusterServiceBroker(instance *v1b

authConfig, err := getAuthCredentialsFromClusterServiceBroker(c.kubeClient, broker)
if err != nil {
return nil, "", nil, &operationError{
return nil, nil, "", nil, &operationError{
reason: errorAuthCredentialsReason,
message: fmt.Sprintf(
"Error getting broker auth credentials for broker %q: %s",
Expand All @@ -329,10 +318,10 @@ func (c *controller) getClusterServiceClassAndClusterServiceBroker(instance *v1b
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, err
return nil, nil, "", nil, err
}

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

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

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

Expand Down Expand Up @@ -70,6 +69,8 @@ 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 @@ -541,26 +542,25 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns
return c.handleServiceInstanceReconciliationError(instance, err)
}

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

request, inProgressProperties, err := c.prepareDeprovisionRequest(instance, serviceClass)
request, err := c.prepareDeprovisionRequest(instance, serviceClass, servicePlan)
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, inProgressProperties)
instance, err = c.recordStartOfServiceInstanceOperation(instance, v1beta1.ServiceInstanceOperationDeprovision, nil)
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,17 +1176,18 @@ 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 @@ -1288,7 +1289,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, servicePlan *v1beta1.ClusterServicePlan, setInProgressProperties bool) (*requestHelper, error) {
func (c *controller) prepareRequestHelper(instance *v1beta1.ServiceInstance, serviceClass *v1beta1.ClusterServiceClass, servicePlan *v1beta1.ClusterServicePlan) (*requestHelper, error) {
rh := &requestHelper{}

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

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,
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,
}

// osb client handles whether or not to really send this based
// on the version of the client.
Expand All @@ -1354,7 +1353,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, servicePlan, true)
rh, err := c.prepareRequestHelper(instance, serviceClass, servicePlan)
if err != nil {
return nil, nil, err
}
Expand All @@ -1377,7 +1376,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, servicePlan, true)
rh, err := c.prepareRequestHelper(instance, serviceClass, servicePlan)
if err != nil {
return nil, nil, err
}
Expand Down Expand Up @@ -1411,42 +1410,39 @@ 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) (*osb.DeprovisionRequest, *v1beta1.ServiceInstancePropertiesState, error) {
rh, err := c.prepareRequestHelper(instance, nil, false)
func (c *controller) prepareDeprovisionRequest(instance *v1beta1.ServiceInstance, serviceClass *v1beta1.ClusterServiceClass, servicePlan *v1beta1.ClusterServicePlan) (*osb.DeprovisionRequest, error) {
rh, err := c.prepareRequestHelper(instance, serviceClass, servicePlan)
if err != nil {
return nil, nil, err
return nil, err
}

// 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
var servicePlanExternalID string
if instance.Status.ExternalProperties != nil {
servicePlanExternalID = instance.Status.ExternalProperties.ClusterServicePlanExternalID
} else if servicePlan != nil {
servicePlanExternalID = servicePlan.Spec.ExternalID
} else {
if instance.Status.ExternalProperties == nil {
return nil, nil, stderrors.New("ExternalProperties must be set before deprovisioning")
return nil, &operationError{
reason: errorUnknownServicePlanReason,
message: errorUnknownServicePlanMessage,
}
rh.inProgressProperties = instance.Status.ExternalProperties
}

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

return request, rh.inProgressProperties, nil
return request, 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, servicePlan, false)
rh, err := c.prepareRequestHelper(instance, serviceClass, servicePlan)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1562,9 +1558,6 @@ 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 82fc6e4

Please sign in to comment.