Skip to content

Commit

Permalink
Do not allow updates to an object if asynchronous operation is in pro…
Browse files Browse the repository at this point in the history 
…gress (openshift#853)

* Do not allow updates to an object if asynchronous operation is in progress

* add the note about Status being able to be modified

* Only check if async flag is on during the spec changes, allow it for now for all status changes and add more conditions tests

* do not allow instanceconditionready true while async op is ongoing

* fix args in wrong order
  • Loading branch information
@pmorie
Ville Aikas authored and pmorie committed May 26, 2017
1 parent 7295dad commit 262a94f
Show file tree
Hide file tree
Showing 3 changed files with 296 additions and 5 deletions.
46 changes: 43 additions & 3 deletions pkg/apis/servicecatalog/validation/instance.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ func internalValidateInstance(instance *sc.Instance, create bool) field.ErrorLis
validateInstanceName,
field.NewPath("metadata"))...)
allErrs = append(allErrs, validateInstanceSpec(&instance.Spec, field.NewPath("Spec"), create)...)
allErrs = append(allErrs, validateInstanceStatus(&instance.Status, field.NewPath("Status"), create)...)
return allErrs
}

Expand All @@ -62,15 +63,54 @@ func validateInstanceSpec(spec *sc.InstanceSpec, fldPath *field.Path, create boo
return allErrs
}

func validateInstanceStatus(spec *sc.InstanceStatus, fldPath *field.Path, create bool) field.ErrorList {
errors := field.ErrorList{}
// TODO(vaikas): Implement more comprehensive status validation.
// https://github.com/kubernetes-incubator/service-catalog/issues/882

// Do not allow the instance to be ready if an async operation is ongoing
// ongoing
if spec.AsyncOpInProgress {
for _, c := range spec.Conditions {
if c.Type == sc.InstanceConditionReady && c.Status == sc.ConditionTrue {
errors = append(errors, field.Forbidden(fldPath.Child("Conditions"), "Can not set InstanceConditionReady to true when an async operation is in progress"))
}
}
}

return errors
}

// internalValidateInstanceUpdateAllowed ensures there is not an asynchronous
// operation ongoing with the instance before allowing an update to go through.
func internalValidateInstanceUpdateAllowed(new *sc.Instance, old *sc.Instance) field.ErrorList {
errors := field.ErrorList{}
if old.Status.AsyncOpInProgress {
errors = append(errors, field.Forbidden(field.NewPath("Spec"), "Another operation for this service instance is in progress"))
}
return errors
}

// ValidateInstanceUpdate validates a change to the Instance's spec.
func ValidateInstanceUpdate(new *sc.Instance, old *sc.Instance) field.ErrorList {
return internalValidateInstance(new, false)
allErrs := field.ErrorList{}
allErrs = append(allErrs, internalValidateInstanceUpdateAllowed(new, old)...)
allErrs = append(allErrs, internalValidateInstance(new, false)...)
return allErrs
}

func internalValidateInstanceStatusUpdateAllowed(new *sc.Instance, old *sc.Instance) field.ErrorList {
errors := field.ErrorList{}
// TODO(vaikas): Are there any cases where we do not allow updates to
// Status during Async updates in progress?
return errors
}

// ValidateInstanceStatusUpdate checks that when changing from an older
// instance to a newer instance is okay.
// instance to a newer instance is okay. This only checks the instance.Status field.
func ValidateInstanceStatusUpdate(new *sc.Instance, old *sc.Instance) field.ErrorList {
allErrs := field.ErrorList{}
allErrs = append(allErrs, ValidateInstanceUpdate(new, old)...)
allErrs = append(allErrs, internalValidateInstanceStatusUpdateAllowed(new, old)...)
allErrs = append(allErrs, internalValidateInstance(new, false)...)
return allErrs
}
243 changes: 243 additions & 0 deletions pkg/apis/servicecatalog/validation/instance_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package validation

import (
"strings"
"testing"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -123,3 +124,245 @@ func TestValidateInstance(t *testing.T) {
}
}
}

func TestValidateInstanceUpdate(t *testing.T) {
cases := []struct {
name string
old *servicecatalog.Instance
new *servicecatalog.Instance
valid bool
err string // Error string to match against if error expected
}{
{
name: "no update with async op in progress",
old: &servicecatalog.Instance{
ObjectMeta: metav1.ObjectMeta{
Name: "test-instance",
Namespace: "test-ns",
},
Spec: servicecatalog.InstanceSpec{
ServiceClassName: "test-serviceclass",
PlanName: "Test-Plan",
},
Status: servicecatalog.InstanceStatus{
AsyncOpInProgress: true,
},
},
new: &servicecatalog.Instance{
ObjectMeta: metav1.ObjectMeta{
Name: "test-instance",
Namespace: "test-ns",
},
Spec: servicecatalog.InstanceSpec{
ServiceClassName: "test-serviceclass",
PlanName: "Test-Plan-2",
},
Status: servicecatalog.InstanceStatus{
AsyncOpInProgress: true,
},
},
valid: false,
err: "Another operation for this service instance is in progress",
},
{
name: "allow update with no async op in progress",
old: &servicecatalog.Instance{
ObjectMeta: metav1.ObjectMeta{
Name: "test-instance",
Namespace: "test-ns",
},
Spec: servicecatalog.InstanceSpec{
ServiceClassName: "test-serviceclass",
PlanName: "Test-Plan",
},
Status: servicecatalog.InstanceStatus{
AsyncOpInProgress: false,
},
},
new: &servicecatalog.Instance{
ObjectMeta: metav1.ObjectMeta{
Name: "test-instance",
Namespace: "test-ns",
},
Spec: servicecatalog.InstanceSpec{
ServiceClassName: "test-serviceclass",
// TODO(vaikas): This does not actually update
// spec yet, but once it does, validate it changes.
PlanName: "Test-Plan-2",
},
Status: servicecatalog.InstanceStatus{
AsyncOpInProgress: false,
},
},
valid: true,
err: "",
},
}

for _, tc := range cases {
errs := ValidateInstanceUpdate(tc.new, tc.old)
if len(errs) != 0 && tc.valid {
t.Errorf("%v: unexpected error: %v", tc.name, errs)
continue
} else if len(errs) == 0 && !tc.valid {
t.Errorf("%v: unexpected success", tc.name)
}
if !tc.valid {
for _, err := range errs {
if !strings.Contains(err.Detail, tc.err) {
t.Errorf("Error %q did not contain expected message %q", err.Detail, tc.err)
}
}
}
}
}

func TestValidateInstanceStatusUpdate(t *testing.T) {
cases := []struct {
name string
old *servicecatalog.InstanceStatus
new *servicecatalog.InstanceStatus
valid bool
err string // Error string to match against if error expected
}{
{
name: "Start async op",
old: &servicecatalog.InstanceStatus{
AsyncOpInProgress: false,
},
new: &servicecatalog.InstanceStatus{
AsyncOpInProgress: true,
},
valid: true,
err: "",
},
{
name: "Complete async op",
old: &servicecatalog.InstanceStatus{
AsyncOpInProgress: true,
},
new: &servicecatalog.InstanceStatus{
AsyncOpInProgress: false,
},
valid: true,
err: "",
},
{
name: "InstanceConditionReady can not be true if async is ongoing",
old: &servicecatalog.InstanceStatus{
AsyncOpInProgress: true,
Conditions: []servicecatalog.InstanceCondition{{
Type: servicecatalog.InstanceConditionReady,
Status: servicecatalog.ConditionFalse,
}},
},
new: &servicecatalog.InstanceStatus{
AsyncOpInProgress: true,
Conditions: []servicecatalog.InstanceCondition{{
Type: servicecatalog.InstanceConditionReady,
Status: servicecatalog.ConditionTrue,
}},
},
valid: false,
err: "async operation is in progress",
},
{
name: "InstanceConditionReady can be true if async is completed",
old: &servicecatalog.InstanceStatus{
AsyncOpInProgress: true,
Conditions: []servicecatalog.InstanceCondition{{
Type: servicecatalog.InstanceConditionReady,
Status: servicecatalog.ConditionFalse,
}},
},
new: &servicecatalog.InstanceStatus{
AsyncOpInProgress: false,
Conditions: []servicecatalog.InstanceCondition{{
Type: servicecatalog.InstanceConditionReady,
Status: servicecatalog.ConditionTrue,
}},
},
valid: true,
err: "",
},
{
name: "Update instance condition ready status during async",
old: &servicecatalog.InstanceStatus{
AsyncOpInProgress: true,
Conditions: []servicecatalog.InstanceCondition{{Status: servicecatalog.ConditionFalse}},
},
new: &servicecatalog.InstanceStatus{
AsyncOpInProgress: true,
Conditions: []servicecatalog.InstanceCondition{{Status: servicecatalog.ConditionTrue}},
},
valid: true,
err: "",
},
{
name: "Update instance condition ready status during async false",
old: &servicecatalog.InstanceStatus{
AsyncOpInProgress: false,
Conditions: []servicecatalog.InstanceCondition{{Status: servicecatalog.ConditionFalse}},
},
new: &servicecatalog.InstanceStatus{
AsyncOpInProgress: false,
Conditions: []servicecatalog.InstanceCondition{{Status: servicecatalog.ConditionTrue}},
},
valid: true,
err: "",
},
{
name: "Update instance condition to ready status and finish async op",
old: &servicecatalog.InstanceStatus{
AsyncOpInProgress: true,
Conditions: []servicecatalog.InstanceCondition{{Status: servicecatalog.ConditionFalse}},
},
new: &servicecatalog.InstanceStatus{
AsyncOpInProgress: false,
Conditions: []servicecatalog.InstanceCondition{{Status: servicecatalog.ConditionTrue}},
},
valid: true,
err: "",
},
}

for _, tc := range cases {
old := &servicecatalog.Instance{
ObjectMeta: metav1.ObjectMeta{
Name: "test-instance",
Namespace: "test-ns",
},
Spec: servicecatalog.InstanceSpec{
ServiceClassName: "test-serviceclass",
PlanName: "Test-Plan",
},
Status: *tc.old,
}
new := &servicecatalog.Instance{
ObjectMeta: metav1.ObjectMeta{
Name: "test-instance",
Namespace: "test-ns",
},
Spec: servicecatalog.InstanceSpec{
ServiceClassName: "test-serviceclass",
PlanName: "Test-Plan",
},
Status: *tc.new,
}

errs := ValidateInstanceStatusUpdate(new, old)
if len(errs) != 0 && tc.valid {
t.Errorf("%v: unexpected error: %v", tc.name, errs)
continue
} else if len(errs) == 0 && !tc.valid {
t.Errorf("%v: unexpected success", tc.name)
}
if !tc.valid {
for _, err := range errs {
if !strings.Contains(err.Detail, tc.err) {
t.Errorf("Error %q did not contain expected message %q", err.Detail, tc.err)
}
}
}
}
}
12 changes: 10 additions & 2 deletions pkg/registry/servicecatalog/instance/strategy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,14 @@ func NewScopeStrategy() rest.NamespaceScopedStrategy {

// implements interfaces RESTCreateStrategy, RESTUpdateStrategy, RESTDeleteStrategy,
// NamespaceScopedStrategy
// The implementation disallows any modifications to the instance.Status fields.
type instanceRESTStrategy struct {
runtime.ObjectTyper // inherit ObjectKinds method
names.NameGenerator // GenerateName method for CreateStrategy
}

// implements interface RESTUpdateStrategy
// implements interface RESTUpdateStrategy. This implementation validates updates to
// instance.Status updates only and disallows any modifications to the instance.Spec.
type instanceStatusRESTStrategy struct {
instanceRESTStrategy
}
Expand Down Expand Up @@ -122,7 +124,13 @@ func (instanceRESTStrategy) PrepareForUpdate(ctx genericapirequest.Context, new,
glog.Fatal("received a non-instance object to update from")
}

// 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
newInstance.Spec = oldInstance.Spec

// Do not allow any updates to the Status field while updating the Spec
newInstance.Status = oldInstance.Status
}

Expand All @@ -148,7 +156,7 @@ func (instanceStatusRESTStrategy) PrepareForUpdate(ctx genericapirequest.Context
if !ok {
glog.Fatal("received a non-instance object to update from")
}
// status changes are not allowed to update spec
// Status changes are not allowed to update spec
newInstance.Spec = oldInstance.Spec

foundReadyConditionTrue := false
Expand Down

0 comments on commit 262a94f

Please sign in to comment.