should allow service catalog client to update a binding's spec field if the reconciler didn't send a binding request to a broker

[carlory]

Bug Report

What happened:

service catalog client couldn't update the spec field of a binding because of the RESTUpdateStrategy limitation.

func (bindingRESTStrategy) PrepareForUpdate(ctx context.Context, new, old runtime.Object) {
	// 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.
	newServiceBinding.Spec = oldServiceBinding.Spec
}

After I try to remove this check, I am also stuck in trouble when the OriginatingIdentityLocking feature and the ServicePlanDefaults feature are both set.

// internalValidateServiceBindingUpdateAllowed ensures there is not a
// pending update on-going with the spec of the binding before allowing an update
// to the spec to go through.
func internalValidateServiceBindingUpdateAllowed(new *sc.ServiceBinding, old *sc.ServiceBinding) field.ErrorList {
	errors := field.ErrorList{}

	// If the OriginatingIdentityLocking feature is set then don't allow spec updates
	// if processing of the current generation hasn't finished yet
	if utilfeature.DefaultFeatureGate.Enabled(scfeatures.OriginatingIdentityLocking) {
		if old.Generation != new.Generation && old.Status.ReconciledGeneration != old.Generation {
			errors = append(errors, field.Forbidden(field.NewPath("spec"), "another change to the spec is in progress"))
		}
	}

	return errors
}

the ReconciledGeneration field isn't equal to the Generation field if the reconciler updated a binding before it send a binding request to a given broker. so the ReconciledGeneration won't be updated and the update action is also failed.

it is blocking the ServicePlanDefaults feature. related pr is #2435

What you expected to happen:

should allow service catalog client to update a binding's spec field if the reconciler didn't send a binding request to a broker.

Related issues and PRs:

#1872

#2348

#2349

[carlory]

@nilebox @carolynvs @duglin Could you give me some advice about it ?

[nilebox]

@carlory As far as I know, OSB doesn't allow updates to bindings, so I think the current behavior is valid - if you need to generate fresh credentials, you should create a new binding, switch your application to a new secret, and remove the old one.

See https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#request-6 for details.

should allow service catalog client to update a binding's spec field if the reconciler didn't send a binding request to a broker.

This seems like a rare use case, not sure if we want to support it. Why do you need it?

[carlory]

@nilebox Thanks for you comment.

@carlory As far as I know, OSB doesn't allow updates to bindings, so I think the current behavior is valid

Yes, that's right !

But if we want to solve those issue #2348 #2349, we should support this changes.

Please see the proposal for additional context and details: https://github.com/carolynvs/service-catalog/blob/default-service-plan-proposal/docs/proposals/default-service-plans.md#binding-with-defaults.

This is my PR #2435 which requires this changes.

Could you take a look at my PR?

In my PR, this function applyDefaultBindingParameters https://github.com/kubernetes-incubator/service-catalog/pull/2435/files#diff-ac77d94525ae05a353ce905a06988cc9R1167 should be called before the reconciler send a binding request to a broker because OSB doesn't allow updates to a exist binding.

[nilebox]

@carlory I think defaults could be injected by an admission controller (i.e. by API server, before the binding has been persisted) rather that binding controller. I would prefer this approach over checking that binding request has been sent (this is hard to get right, and I don't think ReconciledGeneration is the right field to check against). I need to double check in the code if we have status that guarantees that binding request wasn't sent.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten