Are there any other concerns with the proposal or should we start a VOTE thread?
Eduard On Wed, Jul 10, 2024 at 5:20 PM Dmitri Bourlatchkov <dmitri.bourlatch...@dremio.com.invalid> wrote: > Re: remote signing, I agree that it does not look like a server capability >> that a client can / should discover. It is more like something that the >> server instructs / configures the client to do. > > > While a server can control this behavior and instruct the client to use > remote signing, technically nothing is preventing a client from configuring > s3.remote-signing-enabled=true. In such a case it seems more > appropriate to indicate that this capability isn't supported rather than a > generic 501, because not every server will support remote signing. > > > Good point regarding clients taking initiative and using request singing > without an explicit server-provided config. It moves the client operations > into a mode where the server has more control (over having longer term > client-side credentials), so it looks like a reasonable mode to support > from the security perspective. > > Let's keep that capability flag. > > Cheers, > Dmitri. > > On Wed, Jul 10, 2024 at 5:48 AM Eduard Tudenhöfner < > etudenhoef...@apache.org> wrote: > >> Hey everyone, >> >> I've added a few inline comments below. >> >> >> >>> Re: remote signing, I agree that it does not look like a server >>> capability that a client can / should discover. It is more like something >>> that the server instructs / configures the client to do. >> >> >> While a server can control this behavior and instruct the client to use >> remote signing, technically nothing is preventing a client from configuring >> s3.remote-signing-enabled=true. In such a case it seems more >> appropriate to indicate that this capability isn't supported rather than a >> generic 501, because not every server will support remote signing. >> >> The *vended-credentials* capability on the other hand is more >> informative in its nature and a server indeed configures a client. I think >> that was also one of the reasons I removed this capability but added it >> later back due to a comment from Jack. >> >> I'm ok either way in terms of removing / keeping *vended-credentials* as >> a capability but given that we'd want to include *actionable* capabilities >> at this point, I'd just remove it (nothing is preventing us from adding it >> later if necessary). >> >> >> In that case, why do we need all these other capabilities like tables, >>> remote-signing, etc. in the first place? >> >> >> Given that capabilities also carry versioning information, clients can >> make more informed decisions on which endpoints to call. One could argue >> that generally throwing a 501 on everything that isn't supported might be >> sufficient, but that doesn't necessarily help a client in knowing which >> versions of a capability are safe to call/use. >> >> Regarding the control of client-side fallback behavior: >> I think the default fallback behavior should be *tables* (with version >> 1) with a property in the REST catalog that allows configuring this to e.g. >> *rest-default-capabilities=tables,views,abc,xyz* (all of them defaulting >> to version 1). >> >> >> Eduard >> >> >> On Tue, Jul 9, 2024 at 7:00 PM Jack Ye <yezhao...@gmail.com> wrote: >> >>> Yes I agree that sounds like a valid use case. So the criteria so far is >>> that capabilities are used for: >>> - controlling client-side fallback behavior >>> - failing expensive operations early if we know it will eventually fail >>> due to missing capability >>> >>> Do we agree if this is the criteria we should use? What about the other >>> capabilities, namly tables, remote-signing, credential-vending? >>> >>> -Jack >>> >>> >>> On Tue, Jul 9, 2024 at 9:27 AM Ryan Blue <b...@databricks.com.invalid> >>> wrote: >>> >>>> > does it make a difference if I declare the capability or not? >>>> >>>> I think that it does in other cases. Multi-table commits, for example, >>>> are a building block for multi-statement transactions. If a service doesn't >>>> support multi-table commits then we ideally want clients to know that ahead >>>> of time so that they don't run a big transaction and then fail because the >>>> commit is not supported. >>>> >>>> On Tue, Jul 9, 2024 at 9:12 AM Dmitri Bourlatchkov >>>> <dmitri.bourlatch...@dremio.com.invalid> wrote: >>>> >>>>> Re: remote signing, I agree that it does not look like a server >>>>> capability that a client can / should discover. It is more like something >>>>> that the server instructs / configures the client to do. >>>>> >>>>> Cheers, >>>>> Dmitri. >>>>> >>>>> On Tue, Jul 9, 2024 at 12:05 PM Jack Ye <yezhao...@gmail.com> wrote: >>>>> >>>>>> I was reconciling the discussion yesterday, one point that was >>>>>> interesting to me was that we agreed the purpose of these capabilities is >>>>>> to "control client-side fallback behavior", or at least the client should >>>>>> behave differently based on these capabilities. However, this seems to be >>>>>> only needed so far for views, or more specifically, for loadView API only >>>>>> because it impacts the fallback behavior to resolve the identifier as a >>>>>> table or not. >>>>>> >>>>>> For all the other capabilities listed, and even the other endpoints >>>>>> in view, because a server can decide to implement it partially anyway and >>>>>> just document the behavior, does it make a difference if I declare the >>>>>> capability or not? The client will not stop the request, the server will >>>>>> just error out if it is not supported. Maybe the error is not in the >>>>>> expected code or message, but it is still an error. In that case, why do >>>>>> we >>>>>> need all these other capabilities like tables, remote-signing, etc. in >>>>>> the >>>>>> first place? >>>>>> >>>>>> Maybe it is too extreme of a thought, but could anyone help describe >>>>>> how the other capabilities could be used beyond potentially returning an >>>>>> error earlier? >>>>>> >>>>>> -Jack >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> On Tue, Jul 9, 2024 at 8:02 AM Dmitri Bourlatchkov >>>>>> <dmitri.bourlatch...@dremio.com.invalid> wrote: >>>>>> >>>>>>> Hi Eduard, >>>>>>> >>>>>>> > I've also added the 501 error to the response of the respective >>>>>>> endpoints but worth mentioning that *HEAD* / *GET *requests must >>>>>>> not return a 501 >>>>>>> <https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/501> (this >>>>>>> implies that the server impl would e.g. return a *404* in such a >>>>>>> case). >>>>>>> >>>>>>> My reading on the Mozilla page makes me think that it is phrased too >>>>>>> narrowly. Reading RFC 2616 [1] I believe that it does not preclude >>>>>>> responding with 501 to GET and HEAD requests. I think it means that GET >>>>>>> and >>>>>>> HEAD methods must be supported by "general purpose" servers. The Iceberg >>>>>>> REST server is not a general purpose server for resources. So, I think >>>>>>> it >>>>>>> should be fine to respond with 501 to unimplemented endpoints. >>>>>>> >>>>>>> Cheers, >>>>>>> Dmitri. >>>>>>> >>>>>>> [1] https://www.rfc-editor.org/rfc/rfc2616#section-5.1.1 >>>>>>> >>>>>>> On Tue, Jul 9, 2024 at 9:44 AM Eduard Tudenhöfner < >>>>>>> etudenhoef...@apache.org> wrote: >>>>>>> >>>>>>>> Hey everyone, >>>>>>>> >>>>>>>> I watched the catalog sync recording today and updated the PR >>>>>>>> <https://github.com/apache/iceberg/pull/9940> to remove >>>>>>>> fine-grained capabilities like *register-table / table-metrics*. >>>>>>>> >>>>>>>> The current capabilities (with versioning information) in the PR >>>>>>>> are: >>>>>>>> >>>>>>>> - tables >>>>>>>> - views >>>>>>>> - remote-signing >>>>>>>> - vended-credentials >>>>>>>> - multi-table-commit >>>>>>>> >>>>>>>> For servers that only *partially* implement endpoints under a >>>>>>>> capability the spec requires the server to throw a *501 Not >>>>>>>> Implemented*. I've also added the 501 error to the response of the >>>>>>>> respective endpoints but worth mentioning that *HEAD* / *GET *requests >>>>>>>> must not return a 501 >>>>>>>> <https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/501> (this >>>>>>>> implies that the server impl would e.g. return a *404* in such a >>>>>>>> case). >>>>>>>> >>>>>>>> >>>>>>>> Regards >>>>>>>> Eduard >>>>>>>> >>>>>>>> On Thu, Jul 4, 2024 at 3:59 PM Jean-Baptiste Onofré < >>>>>>>> j...@nanthrax.net> wrote: >>>>>>>> >>>>>>>>> Hi Eduard, >>>>>>>>> >>>>>>>>> It makes sense to return 501 for servers which don't implement all >>>>>>>>> endpoints. It means that the server will at least have to implement >>>>>>>>> empty endpoints if needed (that makes sense to me). >>>>>>>>> >>>>>>>>> I think we should focus on only "identified capabilities". I think >>>>>>>>> that I proposed before that the capabilities can be >>>>>>>>> overridden/provided by server implementation. Else, I'm afraid we >>>>>>>>> won't be flexible enough or always behind the implementation (if an >>>>>>>>> implementation wants to add "my-foo-cap"). >>>>>>>>> >>>>>>>>> Regards >>>>>>>>> JB >>>>>>>>> >>>>>>>>> On Thu, Jul 4, 2024 at 9:32 AM Eduard Tudenhöfner >>>>>>>>> <etudenhoef...@apache.org> wrote: >>>>>>>>> > >>>>>>>>> > I have clarified the wording in #9940 around the requirement on >>>>>>>>> having to implement all endpoints under a particular capability. >>>>>>>>> > >>>>>>>>> > For servers that only partially implement endpoints under a >>>>>>>>> capability the spec requires the server to throw a 501 Not >>>>>>>>> Implemented. >>>>>>>>> This was suggested by Jack and it seems reasonable to do that. >>>>>>>>> > >>>>>>>>> > Regarding the inclusion of table-spec / view-spec as a >>>>>>>>> capability: I think this might make sense for the next iteration of >>>>>>>>> the >>>>>>>>> REST spec but as I mentioned earlier I don't see any clear benefit >>>>>>>>> for the >>>>>>>>> current REST spec as the client wouldn't do anything with that >>>>>>>>> information. >>>>>>>>> > If there is a clear benefit of having this, then this can still >>>>>>>>> be added later to the current REST spec but I believe we should >>>>>>>>> rather have >>>>>>>>> a few well-defined and actionable capabilities rather than too many. >>>>>>>>> > >>>>>>>>> > Eduard >>>>>>>>> > >>>>>>>>> > On Wed, Jul 3, 2024 at 5:44 AM Renjie Liu < >>>>>>>>> liurenjie2...@gmail.com> wrote: >>>>>>>>> >>> >>>>>>>>> >>> Spec is an interesting topic we did not discuss. Robert, how >>>>>>>>> do you envision this to be used? >>>>>>>>> >>> In my mind, if a new table format v3 is launched, there are 2 >>>>>>>>> approaches we can go with, taking CreateTable as an example: >>>>>>>>> >>> (1) increment the related operation version, which means that >>>>>>>>> POST /v2/{prefix}/namespaces/{ns}/tables will be created and allow >>>>>>>>> creating >>>>>>>>> tables in the v3 version. >>>>>>>>> >>> (2) update the existing table metadata model to support both >>>>>>>>> v2 and v3 fields, and the server enforces the payload differently >>>>>>>>> based on >>>>>>>>> the TableMetadata.format-version field. If the server does not >>>>>>>>> support v3, >>>>>>>>> it can return unsupported at that time. >>>>>>>>> >>> Either way we go, the table-spec version does not need to be a >>>>>>>>> capability. (1) seems to be cleaner, but has some overhead in >>>>>>>>> provisioning >>>>>>>>> a new endpoint compared to (2). >>>>>>>>> >>> Do you see another way to do this leveraging the table-spec >>>>>>>>> version? >>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> >> 2 is cleaner but maybe inconsistent with current behavior, >>>>>>>>> since /v1/tables operation supports both v1 and v3. We should only go >>>>>>>>> to 2 >>>>>>>>> only when we have incompatible fields/break changes according to >>>>>>>>> discussion. >>>>>>>>> >> >>>>>>>>> >> Generally I agree with adding table-spec into capabilities. For >>>>>>>>> example, we can expose this to user in api so that user could choose a >>>>>>>>> supported table format version without throwing exception. >>>>>>>>> >> >>>>>>>>> >> On Wed, Jul 3, 2024 at 12:18 AM Jack Ye <yezhao...@gmail.com> >>>>>>>>> wrote: >>>>>>>>> >>> >>>>>>>>> >>> Spec is an interesting topic we did not discuss. Robert, how >>>>>>>>> do you envision this to be used? >>>>>>>>> >>> >>>>>>>>> >>> In my mind, if a new table format v3 is launched, there are 2 >>>>>>>>> approaches we can go with, taking CreateTable as an example: >>>>>>>>> >>> >>>>>>>>> >>> (1) increment the related operation version, which means that >>>>>>>>> POST /v2/{prefix}/namespaces/{ns}/tables will be created and allow >>>>>>>>> creating >>>>>>>>> tables in the v3 version. >>>>>>>>> >>> >>>>>>>>> >>> (2) update the existing table metadata model to support both >>>>>>>>> v2 and v3 fields, and the server enforces the payload differently >>>>>>>>> based on >>>>>>>>> the TableMetadata.format-version field. If the server does not >>>>>>>>> support v3, >>>>>>>>> it can return unsupported at that time. >>>>>>>>> >>> >>>>>>>>> >>> Either way we go, the table-spec version does not need to be a >>>>>>>>> capability. (1) seems to be cleaner, but has some overhead in >>>>>>>>> provisioning >>>>>>>>> a new endpoint compared to (2). >>>>>>>>> >>> >>>>>>>>> >>> Do you see another way to do this leveraging the table-spec >>>>>>>>> version? >>>>>>>>> >>> >>>>>>>>> >>> -Jack >>>>>>>>> >>> >>>>>>>>> >>> >>>>>>>>> >>> >>>>>>>>> >>> >>>>>>>>> >>> >>>>>>>>> >>> On Tue, Jul 2, 2024 at 6:03 AM Eduard Tudenhöfner >>>>>>>>> <eduard.tudenhoef...@databricks.com.invalid> wrote: >>>>>>>>> >>>> >>>>>>>>> >>>> >>>>>>>>> >>>> I couldn't make it to the catalog sync meeting yesterday but >>>>>>>>> I watched the recording today (thanks for providing that). >>>>>>>>> >>>> >>>>>>>>> >>>>> The missing piece is how (new, capabilities-aware) clients >>>>>>>>> handle the case when a service does _not_ return the capabilities >>>>>>>>> field >>>>>>>>> (absent). My proposal would be that a client should in this case >>>>>>>>> assume >>>>>>>>> that all _currently_ existing capabilities are supported. >>>>>>>>> >>>>> >>>>>>>>> >>>>> - tables: [1] >>>>>>>>> >>>>> - views: [1] >>>>>>>>> >>>>> - remote-signing: [1] >>>>>>>>> >>>>> - multi-table-commit: [1] >>>>>>>>> >>>>> - register-table: [1] >>>>>>>>> >>>>> - table-metrics: [1] >>>>>>>>> >>>>> - table-spec: [1,2] >>>>>>>>> >>>>> - view-spec: [1,2] >>>>>>>>> >>>>> >>>>>>>>> >>>>> >>>>>>>>> >>>> The one thing I would like to add here is that the current PR >>>>>>>>> uses the tables capability (as version 1) as the default when a server >>>>>>>>> doesn't return capabilities but it might be also ok to include views >>>>>>>>> (as >>>>>>>>> version 1) because the current client impl has some code to deal with >>>>>>>>> errors in case endpoints don't exist. >>>>>>>>> >>>> >>>>>>>>> >>>> Unless we agree that the currently existing functionality in >>>>>>>>> the REST spec is the default behavior to be assumed for older server, >>>>>>>>> I'm >>>>>>>>> not sure about including remote-signing / multi-table-commit / >>>>>>>>> register-table / table-metrics as it has been indicated in earlier >>>>>>>>> comments >>>>>>>>> on the PR/ML that not every REST server supports these. >>>>>>>>> >>>> >>>>>>>>> >>>> That being said, we should discuss whether we want the >>>>>>>>> default behavior (when an older server doesn't send back >>>>>>>>> capabilities) to be >>>>>>>>> >>>> a) tables (version 1) only >>>>>>>>> >>>> b) the currently existing functionality as defined in the >>>>>>>>> REST spec (as version 1) >>>>>>>>> >>>> >>>>>>>>> >>>> >>>>>>>>> >>>> On another note: Including table-spec / view-spec seems to be >>>>>>>>> more informative in its nature as I don't think a client would act >>>>>>>>> differently right now when seeing these. >>>>>>>>> >>>> >>>>>>>>> >>>> Thanks >>>>>>>>> >>>> Eduard >>>>>>>>> >>>> >>>>>>>>> >>>>>>>> >>>> >>>> -- >>>> Ryan Blue >>>> Databricks >>>> >>>
