adutra commented on PR #10621: URL: https://github.com/apache/iceberg/pull/10621#issuecomment-2205792107
@jackye1995 Thank you so much for initiating this. I have been working on something similar as well, so it's great to see that there is a solid community engagement around this! A few very high-level questions/comments: 1. How are you going to migrate `RESTSigV4Signer`? There is imho an impedance mismatch between the nothing of "sessions" in the REST catalog and the fact that authentication generally operates at request level. `AuthSession` is the key component here. Indeed it aims to provide a bridge between the two, by keeping track of the session context, but also providing auth headers to the underlying REST client. However I wonder how you will be able to migrate `RESTSigV4Signer` to the current `AuthManager` design. `RESTSigV4Signer` is currently implemented in a much lower lever, as an `HttpRequestInterceptor`, mainly because it needs to access the entire outgoing request: path, query parameters, headers, body etc. I therefore think that, to migrate `RESTSigV4Signer`, we'd need to enhance `AuthSession` with a method that gives access to the entire request, and allows the session to inject auth headers in it. We'd also need _pass it down to the HTTP layer_, instead of passing a `Map<String, String>` of auth headers only. 2. Couldn't we reuse `CredentialsProvider`? Really an open question here. I wonder if, in fact, the HTTP layer isn't better suited for this new `AuthManager`. The Apache HTTP client already has a lot of infrastructure to deal with authentication: not only it has `HttpRequestInterceptor` (the swiss knife of request customization) but also `CredentialsProvider` and its many implementations. Besides, `CredentialsProvider` is already leaking through the `HTTPClient` API, see `org.apache.iceberg.rest.HTTPClient.Builder#withProxyCredentialsProvider`. In fact I wonder: would this `AuthManager` work if we provide a proxy and a proxy credentials provider? My gut feeling right now is that it won't. We may want to keep this in mind as we work on the `AuthManager`. Anyways, thank you for this proposal! I will also leave a few comments inline. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
