Skip to content

feat(catalog): add WithOAuthTLSConfig for separate OAuth server TLS#895

Merged
zeroshade merged 1 commit into
apache:mainfrom
lovromazgon:lovro/oauth-tls-config
Apr 21, 2026
Merged

feat(catalog): add WithOAuthTLSConfig for separate OAuth server TLS#895
zeroshade merged 1 commit into
apache:mainfrom
lovromazgon:lovro/oauth-tls-config

Conversation

@lovromazgon
Copy link
Copy Markdown
Contributor

The Iceberg REST spec allows configuring the OAuth2 server separately from the catalog via oauth2-server-uri. When the OAuth2 server is a different host, it may require different TLS settings (different CA, client certificates, etc.).

Currently setupOAuthManager reuses the catalog's HTTP client for token requests, making it impossible to configure separate TLS trust without creating a custom AuthManager.

This adds WithOAuthTLSConfig(*tls.Config) which creates a dedicated HTTP client for OAuth token requests. When not set, behavior is unchanged.

The OAuth2 server can be a different host than the catalog, requiring
its own TLS configuration. Add WithOAuthTLSConfig to configure a
dedicated HTTP client for token requests instead of reusing the
catalog's client.
@lovromazgon lovromazgon requested a review from zeroshade as a code owner April 14, 2026 15:18
Comment thread catalog/rest/options.go
//
// If not set, the OAuth2 client reuses the catalog's HTTP client (and its TLS
// configuration).
func WithOAuthTLSConfig(config *tls.Config) Option {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively this could be generalized to WithOAuthTransport or even WithOAuthHTTPClient.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laskoviymishka what do you think from an API point of view, would we actually want to present a full WithOAuthTransport / WithOAuthHTTPClient? Or do you think this is sufficient? I'm curious about your thoughts here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is sufficient, plus this is flexible enough for most of the cases.

Copy link
Copy Markdown
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good to me, pending a response to my question above

@zeroshade zeroshade merged commit 931a2e0 into apache:main Apr 21, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants