adutra commented on PR #12406: URL: https://github.com/apache/iceberg/pull/12406#issuecomment-2742711559
> I have a few concerns here and it may overlap a little with what @adutra was getting at. This appears to tunnel a separate config/auth to the http client as opposed to extending and using the AuthManager. Since this is primarily setting host/port and auth, why wouldn't we configure this via basic auth manager or a proxy auth manager? I'm just concerned we're creating two alternate paths to configure auth. @danielcweeks I am fine having the proxy authentication being done through the http client own machinery rather than an auth manager, for a few reasons: * An AuthManager is basically a layered authentication system (catalog/context/table) but I can hardly think of a valid use case where one layer would want to override the proxy authentication settings of another layer. It looks a bit overkill to leverage an AuthManager here. * AuthManager was designed for authenticating against the target host, not the proxy host: to leverage an AuthManager for proxy auth, we'd need to introduce a new method e.g. `proxyAuthenticate`, or a new constructor parameter `boolean isProxyAuth` – because the request headers to inject are not the same. Probably not worth the hassle. * Proxy authentication is imo an infrastructure concern and should stay transparent to the application logic, whereas the AuthManager integrates more tightly with application logic. I expressed however a different concern: we are moving towards a world where the REST client needs to talk to TWO servers instead of one: the catalog server and the authorization server. We should therefore make it possible for the client to use different proxy settings for each server. @akhilputhiry proposed a solution for that using "named" proxy configs: https://github.com/apache/iceberg/pull/12406#discussion_r1984518742 The proposal is interesting but I don't think it has been implemented in this PR, so we'd need to address that as a follow-up task. To summarize my POV: I'm +1 on this PR, provided that we introduce multi-host proxy settings later on. -- 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
