Clarify expectations of a broker when parameters are updated

pmorie commented 6 years ago

The spec has a gap when it comes to which parameters are sent to the broker during an update. It's not clear whether all the parameters should be resent, or only the ones that have changed. We should clarify this so that broker authors have a correct expectation of what they will receive.

duglin commented 6 years ago

I would guess they should send them all, but then its weird to say that if none are present then the broker shouldn't change any of them. Makes ya wonder how you clear the values.

avade commented 6 years ago

I agree this needs needs clarification.

In Cloud Foundry the parameters are not stored and resent to the service broker for an update. The logic behind this is that the state of those parameters could have changed via a service dashboard or other mechanism. With the platform providing them it could result in the broker overriding any out of band changes, as it looked like user-supplied values.

cc @mattmcneeney

pmorie commented 6 years ago

@avade does that mean that parameter updates are never sent by CF?

duglin commented 6 years ago

ok I think I misread the spec. its says:

If this field is not present in the request message, then the service broker
MUST NOT change the parameters of the instance as a result of this request.

So I think its best to interpret it as:

no parameters property (regardless of whether there are sub-properties) means no change.
parameters is there but w/o any sub-properties == clear all old ones
parameters is there w/ sub-properties - only the ones specified are applied. Any old properties are cleared.

And yes we should clarify this in the spec.

mattmcneeney commented 6 years ago

The way I've always interpreted this (maybe in my head) is:

no parameters means don’t change anything
some parameters means only change those parameters
all parameters means change all parameters

For reasons @avade said, I don't think missing parameters should mean clear them. We currently don't provide a method for platforms to fetch the latest parameter values, so if the service broker provided any out-of-band mechanism for configuration, then this would leave platforms in a bad position.

duglin commented 6 years ago

There's also the problem of the platform not knowing if the broker has enough state to know what the old parameters are - so not sending anything (hoping the broker will keep the old values) might fail if the broker is stateless.

It seems to me that we should (if we can w/o breaking things) make it an all or nothing kind of thing. Sending just partial params is asking for confusion/ambiguity - and I do think we need a way to clear a property - even if we force a sub-property to be there with a "nil" value.

jmrodri commented 6 years ago

But the update is defined as a patch command:

PATCH /v2/service_instances/:instance_id

If we were expecting all parameters to be sent back then why is this not just a basic PUT? Patch was created to allow partial modification of the resource:

RFC 5789 reads:

This specification defines the new HTTP/1.1 [RFC2616] method, PATCH, which is used to apply partial modifications to a resource.

and PATCH was created because:

The PUT method is already defined to overwrite a resource with a complete new body, and cannot be reused to do partial changes.

So to me the fact that update is a PATCH, I, as the broker author, would assume it would only be sent the parameters that are to be updated.

duglin commented 6 years ago

I guess it depends on what level it operates on. "parameters" is a top-level property, so with the algorithm I described its absence or presence aligns with patch as you describe. But, what do we do about nested properties under "parameters? Do we use the same semantics for those too? How recursive do we go in the tree of json objects? I suspect we may have the freedom to decide what works best for us (or what is easiest to implement).

If we do apply the same semantics recursively to nested properties then we'd just need to tell people to use: "someField": null to clear it I guess.

duglin commented 6 years ago

Another interesting thing to think about is the case where the parameters for the two plans in questions are different - in general I think that's possible, and I believe the schema stuff we just added allows for that too. So, that would mean that within a single "parameters" property you could have fields from both the old (null'd out) and the new as siblings. Would look kind of weird - but also could make schema validation a bit more cumbersome since you'd need to separate them before any validation could occur.

nilebox commented 6 years ago

the problem of the platform not knowing if the broker has enough state to know what the old parameters are

We actually have the same problem for stateful brokers, I think. I wonder how do you define the subset of changed parameters in case that the last update has ended with failure? i.e.

parameters v1 -> CREATE, success
parameters v2 -> UPDATE, failure
parameters v3 -> ???

After the failure the instance is left in the unknown state, and without GET request (which OSB currently doesn't have) there is no way to reliably determine the "current" parameters snapshot and produce the "diff".

So the only reliable way is to always pass all parameters without making any assumptions about the broker's current state.

As to tweaking parameters directly using some UI (with bypassing the platform) - it sounds error prone, and I don't think there is a reliable way to mitigate issues caused by this. If we don't have a source of truth, what's the point of the platform then? And with stateless brokers it gets even worse.

duglin commented 6 years ago

While I like your conclusion that we should send all parameters :-) your failure case is something we should probably discuss and clarify (and is a little related to the discussion we had in today's OSBAPI call). I tend to think that a failure condition (like on your "update" case) should result in the resource returning to its pre-update state. Anything other than that leaves us in an unknown state and that's not good.

The HTTP spec doesn't completely help here either. For example, http 500 just says:

The server encountered an unexpected condition which prevented it from fulfilling the request.

This could be interpreted as it having no material impact on the resource, or it could be interpreted as not making any claim at all about it. But I'd like it if we mandated the former.

nilebox commented 6 years ago

@duglin fair enough, but unfortunately OSB doesn't explicitly require such behavior (which probably leaves it up to the interpretation by brokers and the platform).

should result in the resource returning to its pre-update state

Offtopic: does it mean that Service Catalog should backup parameters snapshots and support "rollback" operation to recover to the latest successfully applied parameters, to remain as a source of truth? We can probably discuss this feature at some point.

jmontleon commented 6 years ago

It may be outside of the scope of your conversation around parameters today, but we discovered an issue with ASB and origin-web-catalog. We were trying to control which plans any given plan could be updated to with an updates_to: parameter on each plan.

The thinking was we wanted to have a way to allow adding something imortant, but at the same time going in reverse and removing it, so for example, allow adding persistent storage for a DB pod, but prevent being switched to a plan that removes the persistent storage, and potentially causes data loss.

The question is, is this reasonable and something that support can and should be added for, or do we need to handle any plan being updated to any other plan?

duglin commented 6 years ago

That's an interesting question. On the surface it seems like its possible that not all plans could be morphed into any other plan, so being able to warn people in advance which update() op will fail/work would be a good thing.

It does seem like there are two reason why the migration might not be allowed: 1 - because, as you said, it might not be good for the user if some resources are removed. However, if that's what the user asked for, I say let them shoot themselves in the foot :-) 2 - because its not technically possible. I wonder how often this would be the case though. Seems like the worst case scenario is that the entire service is reset back to some initial state under the new plan - again, if the user asked for it...

I'd be interested to hear from other broker authors to know if they have a need to advertise this info in a well-defined way.

duglin commented 6 years ago

Offtopic: does it mean that Service Catalog should backup parameters snapshots and support "rollback" operation to recover to the latest successfully applied parameters, to remain as a source of truth? We can probably discuss this feature at some point.

I think we do (sort of) keep a backup via the ExternalProperties field - it should be the value of the resource as known by the broker (or that we think is known by the broker). Although, it doesn't keep secret info in there.

@pmorie you've mentioned that we need to be able to recreate the *Status resources - if ExternalProperties is in Status and we don't have a GET operation on the broker, are we in an inconsistent state because we can't reconstruct ExternalProperties ?

jmrodri commented 6 years ago

@pmorie @duglin what is considered ALL? Say in the catalog response I return for the create that my service accepts parameters a, b, c, d, e, f and the update accepts d, e, f.

I would expect during an update call that I get d, e, f, whether you send me all 3 or just f because that is the one that happened to change. What I do not expect is getting a, b, c, d, e, f during an update. Our broker can also handle getting just f which was the one that changed.

nilebox commented 6 years ago

and the update accepts d, e, f.

@jmrodri where do you declare this? Can you declare this in JSON schema in the catalog? Probably not. Thus, if you don't support updates of a, b and c, you (broker) can validate that their values haven't changed, and otherwise reject update and return an error with corresponding description.

nilebox commented 6 years ago

Interesting, there is actually support for separate schemas for instance create and update. Does it mean that they can be completely different? Say, create supports a, b and c, while update supports only d, e and f. Also, parameters block is an arbitrary JSON, not necessarily a flat key-value list. I wonder how would you support create and update parameters having completely different structures...

I would say that this is a confusing feature, and I would prefer if the broker supported the same schema for create and update, and rejected updates if some parameters can't change (just as not every plan upgrade is supported).

duglin commented 6 years ago

@nilebox you are correct that as of right now the list of params for create() and update() can be totally different. And to make it even more interesting, the list of params for update() can be very different based on the plan.

So, imagine you create an instance with plan "free" with params: p1, p2, p3 Then you update the plan to "paid" and it takes: p3, p4, p5 but you only specify p4 and p5. You could interpret this as "p3 gets cleared" or you could interpret it as "p3 keeps its old value". My concern with keeping p3's value is:

people need to remember that p3 was previously specified and (somehow) explicitly clear it if they don't want that old value.
if we say "unspecified old values retain their value" then it seems like p1 and p2 should retain their values too. And is that same thing as us including p1 and p2 on the update? Or is specifying p1 and p2 on the update an error? And what if we revert back to "free" later on - should p1 and p2 retain their old values?

It seems to me that the cleanest approach is to treat update() like a create() in the sense that while you're not starting from scratch w.r.t. any data that might be kept in the service instance (like your DB), you are starting over w.r.t. the configuration of the service instance. Allowing for old data to linger could really lead to confusion and require a lot of tracking by the user or the platform to ensure the user knows exactly what they are getting. It also requires the broker to be stateful and keep the old values around so when it sets up the new plan it can use the old/unspecified values in that operation.

I'd prefer to error on the side of requiring people to be explicit in what they're asking for and then later on do some optimizations if we can find a safe way to do so.

duglin commented 6 years ago

@mattmcneeney @avade are there a lot of brokers that have different params for create() vs update()? I can easily see cases where create() may have more because once they're set they can't be updated, but I'm wondering if we should simplify the model and say that the set for create() and update() are the same, even though trying to change some on the update() could result in an error - but specifying them with the old value is fine.

btw - having update-only params (meaning not allowed on create) doesn't really make sense to me since the broker could just treat it as a create() followed by an update() and then process all params just fine.

mattmcneeney commented 6 years ago

We've definitely heard of some brokers that only want some parameters at create time, for example enabling 'HA mode' that can't be enabled at a later date.

Typically I've heard that the update parameters are a subset of create ones, but there may be cases where that is not true.

jmrodri commented 6 years ago

@duglin @mattmcneeney the ansible broker use case falls into that category where the update parameters are a subset of the create, at least so far.

nilebox commented 6 years ago

@jmrodri is ansible broker stateful? i.e. is it able to compare the passed parameter value with the current (previous) value? If so, it can just validate that certain parameters can't change. For the stateless brokers (which already can't fully validate input and implement all provisioning semantics properly IMO) it could be fine to just ignore certain parameters that cannot be updated.

duglin commented 6 years ago

In talking with our Bluemix guys they have no expectations around what parameters to send upon an update. The assumption is that, per CF, only the parameters specified on the "cf update-service" cmd are sent and how the broker deals with them is up to it - its a blackbox.

In order not to break backwards compatibility we probably can't do much other than, perhaps, to say something like:

The set of parameters sent to the Service Broker are not defined by this specification. In other words, whether a Platform sends just a subset of the valid update parameters (e.g. just the changed values) is an implementation detail. However, typically the request to update a Service Instance will be end-user initiated and at that time a set of parameters might be specified; it is often just those parameters that are included in the request to update the Service Instance.

For kube's svc-cat, I suspect (with no hard evidence) that we could break people if we were to send everything, especially if we then made the assumption that unspecified properties would be cleared. So, we may have to bite the bullet and add an "updateParameters" type of property to the ServiceInstanceSpec and ask the user to specify which exact parameters to include on the udpate() op. That would at least make it consistent with CF. Like @spadgett suggested in: https://github.com/kubernetes-incubator/service-catalog/issues/1488

duglin commented 6 years ago

PR to try to deal with this: https://github.com/openservicebrokerapi/servicebroker/pull/365

While its not ideal, I don't think we can do more than document what CF does today on this.

mattmcneeney commented 6 years ago

@duglin Do we believe #365 does enough for us to close this issue for now?

duglin commented 6 years ago

@mattmcneeney I think so, but we should discuss it on today's call.

openservicebrokerapi / servicebroker

Clarify expectations of a broker when parameters are updated #350