OpenAPI: Clarify signer property passthrough from server to client#15850
OpenAPI: Clarify signer property passthrough from server to client#15850adutra wants to merge 1 commit intoapache:mainfrom
Conversation
The `RemoteSignRequest` body already supports a `properties` field, but the spec does not describe how such properties could be leveraged. Catalog servers, on the other hand, may want signer clients to include certain properties in sign requests, e.g. pre-computed RBAC data. But again, the spec does not describe how such properties should be communicated to clients. This PR closes both gaps and formalizes a new `signer.properties.*` prefix for the `config` field of the `LoadTableResult` object. It also defines the expectations for signer clients when such properties are present.
| If remote signing for a specific storage provider is enabled, clients must respect the following configurations when creating a remote signer client: | ||
| - `signer.endpoint`: the remote signer endpoint. Required. Can either be a relative path (to be resolved against `signer.uri`) or an absolute URI. | ||
| - `signer.uri`: the base URI to resolve `signer.endpoint` against. Optional. Only meaningful if `signer.endpoint` is a relative path. Defaults to the catalog's base URI if not set. | ||
| - `signer.properties.*`: additional properties to be passed through to the signer endpoint in remote sign |
There was a problem hiding this comment.
| - `signer.properties.*`: additional properties to be passed through to the signer endpoint in remote sign | |
| - `signer.properties.*`: additional properties that SHOULD be passed back to the client when calling the signer endpoint in remote sign |
| - `signer.endpoint`: the remote signer endpoint. Required. Can either be a relative path (to be resolved against `signer.uri`) or an absolute URI. | ||
| - `signer.uri`: the base URI to resolve `signer.endpoint` against. Optional. Only meaningful if `signer.endpoint` is a relative path. Defaults to the catalog's base URI if not set. | ||
| - `signer.properties.*`: additional properties to be passed through to the signer endpoint in remote sign | ||
| requests. Optional. If such properties are present, signer clients MUST pass them through to the signer |
There was a problem hiding this comment.
why MUST and not SHOULD ?, worst case we re-compute ? i understand though MUST be ideal, but we need to be cognizant of older client versions, who don't recognize this at all
There was a problem hiding this comment.
I think MUST is more appropriate: if a new client recognizes these new properties, it would be incorrect for it to deliberately omit them.
Indeed, older clients don't know about these properties, but that's imo a server concern: the server could decide to either degrade gracefully and re-compute the missing properties, or reject the request.
There was a problem hiding this comment.
it would be incorrect for it to deliberately omit them
why ? they can choose not to just forward its maybe since its not easy to passback or simply they care about the freshness of the GRANTs duration (when we stamp them and send back we kind of signup of stale grants, but this is true for vended creds too)
I believe server should be prepared if they don't send back these values to compute these always ? because we need this for backward compatibility.
wdyt considering above
There was a problem hiding this comment.
I think we have 2 parallel discussion topics here:
- Is a newer client allowed to omit the properties communicated by the server? Imho it's not. You mentioned "its not easy to passback", but in fact, it's a trivial thing. As for the "freshness of the GRANTs duration", that's not a client concern and the client most likely won't be able to know if the data is stale or not. It's up to the server to decide if the data is fresh or stale.
- What servers should do with older clients? Again I think this topic does not belong in here, but I see 2 options: degrade or reject. I think that servers should decide and pick the best option for them, knowing that the "degrade" option may incur in significant performance hits. (A 3rd option would be to detect the client is old and include the data in the URL for those old clients.)
There was a problem hiding this comment.
1/ The way i see this is that its an optimization and not a requirement, so spec should not mandate sending them back. Also it might be easy for current iceberg java implementation, but what about other engines which don't use iceberg java i would not make my assumptions.
I kind of disagree freshness of GRANTs is catalog concern, isn't the credential (which are based on GRANTs) freshness a client concern ? is there anything wrong if client comes back to catalog before the creds vended before are expired ?
2/ I don't think we can reject those clients, we should ok with degrading from catalog POV if this fields are not sent, the way to say this is that this is entirely optimization not a requirement, if we go by this i think 1 would make sense. for option to detect old client, first this is very tricky to detect, except iceberg java no one send client version, second, i don't think putting stuff to url is a good idea, it kind of defeats the standard body construct i know there are sections in complete url where can stuff but i don't recommend doing that.
There was a problem hiding this comment.
- I think these properties are being given excessive meaning; they should remain entirely opaque from the client's standpoint. Defining them as mandatory would significantly improve the clarity of the specification. Relying on a "SHOULD" is problematic, as it essentially informs clients that while sending these properties is recommended, omitting them is also acceptable, which strikes me as a poor specification.
- Again, this topic is outside of scope for this PR imho. I suggest we resume this conversation when implementing this feature in Apache Polaris.
2 participants
The
RemoteSignRequestbody already supports apropertiesfield, but the spec does not describe how such properties could be leveraged.Catalog servers, on the other hand, may want signer clients to include certain properties in sign requests, e.g. pre-computed RBAC data. But again, the spec does not describe how such properties should be communicated to clients.
This PR closes both gaps and formalizes a new
signer.properties.*prefix for theconfigfield of theLoadTableResultobject. It also defines the expectations for signer clients when such properties are present.