Skip to content

Comments

Add scope for OAUTH2 authentication in Iceberg's REST catalog#23884

Merged
tdcmeehan merged 1 commit intoprestodb:masterfrom
denodo-research-labs:iceberg_rest_scope
Oct 24, 2024
Merged

Add scope for OAUTH2 authentication in Iceberg's REST catalog#23884
tdcmeehan merged 1 commit intoprestodb:masterfrom
denodo-research-labs:iceberg_rest_scope

Conversation

@denodo-research-labs
Copy link
Contributor

@denodo-research-labs denodo-research-labs commented Oct 24, 2024

Description

Presto issue #23833

Motivation and Context

When we configure the iceberg.rest.auth.oauth2.credential property to connect to a Polaris catalog we get the error The scope is invalid.
Since the Presto code does not set the 'scope' property, the Iceberg code is using 'catalog' as scope and the OAuth2 endpoint is considering it an invalid scope.

Impact

We cannot connect to the Polaris catalog using the iceberg.rest.auth.oauth2.credential property.

Test Plan

Modified tests in TestIcebergRestConfig

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Iceberg Connector Changes
* Add configuration property ``iceberg.rest.auth.oauth2.scope`` for OAUTH2 authentication in Iceberg's REST catalog :pr:`23884`

@ZacBlanco
Copy link
Contributor

@kiersten-stokes do you want to look at this?

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

Pull branch, local doc build, looks good. Thanks!

@tdcmeehan tdcmeehan self-assigned this Oct 24, 2024
Copy link
Contributor

@kiersten-stokes kiersten-stokes left a comment

Choose a reason for hiding this comment

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

Makes sense to me - thanks!

@tdcmeehan tdcmeehan merged commit 88a35db into prestodb:master Oct 24, 2024
@jaystarshot jaystarshot mentioned this pull request Nov 1, 2024
25 tasks
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.

5 participants