Azure / open-service-broker-azure

The Open Service Broker API Server for Azure Services
https://osba.sh
MIT License
248 stars 101 forks source link

organization_guid and space_guid SHOULD be required #88

Open jeremyrickard opened 6 years ago

jeremyrickard commented 6 years ago

Currently, we do not fail provision requests that are missing _organizationguid and/or _spaceguid. The osb-checker asserts that these should fail, as they are indicated as required fields by the specification document.

OSB spec Provisioning section states:

Request field Type Description
context object Platform specific contextual information under which the service instance is to be provisioned. Although most brokers will not use this field, it could be helpful in determining data placement or applying custom business rules. context will replace organization_guid and space_guid in future versions of the specification - in the interim both SHOULD be used to ensure interoperability with old and new implementations.
organization_guid* string Deprecated in favor of context. The platform GUID for the organization under which the service instance is to be provisioned. Although most brokers will not use this field, it might be helpful for executing operations on a user's behalf. MUST be a non-empty string.
space_guid* string Deprecated in favor of context. The identifier for the project space within the platform organization. Although most brokers will not use this field, it might be helpful for executing operations on a user's behalf. MUST be a non-empty string.

While deprecated, the spec explicitly states that both should be used in the interm, even if the broker chooses not to use them.

krancour commented 6 years ago

@jeremyrickard my interpretation of the spec here is that clients are obligated to include these fields in a request, but I think the language is clear that most brokers won't utilize them. Ours is such a broker. I see no language that indicates it's obligatory for a broker that doesn't use those fields to reject requests that don't include them. Would you agree?

Maybe the real action item here is a PR against the checker?

jeremyrickard commented 6 years ago

I wanted to revisit this as I'm reviewing the spec again while working on some refactor for the osb checker.

In addition to the bit above, the spec also says:

400 Bad Request | MUST be returned if the request is malformed or missing mandatory data.

The organization_guid and space_guid are marked as mandatory (as is the X-Broker-API-Version header). I don't believe we validate any of those right now, which my reading would say makes us non-compliant. I believe, at least for the space/org guid, that will eventually be replaced by the context (appears to be done already for update) so I guess we could make a determination about spec before our 1.0 release? It appears that service catalog sends the namespace id for both.

krancour commented 6 years ago

Ya... we've completely ignored those things up until now. I'm starting to develop some vague sense that they might be useful after all. As you said, K8s sends the namespace for both of these. I can imagine that knowing what "namespace" an instance belongs to could be important for us in eventually developing some notion of who owns a given instance. We could start to do things like enforce no bindings from one space to instances in another. In the case of k8s, and probably CF as well, we can count on the platform enforcing those sort of limitations to a certain extent, but as OSB gains traction, it's possible that there will be a wider variety of clients in our future and they might not all be quite as sophisticated.

All of the above is still a half-baked thought, but I could totally see the sense in requiring these values and recording them (i.e. adding them to the storage schema) for potential use down the road.

krancour commented 6 years ago

We don't need to implement this in the v0.10.0-alpha timeframe, but in that timeframe, let's at least develop a proposal for how we're going to approach this.