fix: Fix Bearer authentication with Nessie catalog #26512
+254
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
denodo-research-labs
requested review from
a team,
ZacBlanco and
hantangwangd
as code owners
Contributor
Reviewer's GuideThis PR enables Bearer authentication for the Nessie catalog by propagating the authentication type in the catalog factory, extending the testing infrastructure with a Keycloak test container and necessary dependencies, and adding an integration test that verifies the end-to-end Bearer auth flow. Sequence diagram for Bearer authentication flow in Nessie catalog testssequenceDiagram
actor Tester
participant "TestIcebergSystemTablesNessieWithBearerAuth"
participant "KeycloakContainer"
participant "NessieContainer"
participant "IcebergNessieCatalogFactory"
Tester->>"TestIcebergSystemTablesNessieWithBearerAuth": Start test
"TestIcebergSystemTablesNessieWithBearerAuth"->>"KeycloakContainer": Request Bearer token
"KeycloakContainer"->>"KeycloakContainer": getAccessToken()
"KeycloakContainer"-->>"TestIcebergSystemTablesNessieWithBearerAuth": Return Bearer token
"TestIcebergSystemTablesNessieWithBearerAuth"->>"NessieContainer": Connect using Bearer token
"NessieContainer"->>"IcebergNessieCatalogFactory": Pass authentication type
"IcebergNessieCatalogFactory"->>"NessieContainer": Propagate nessie.authentication.type property
"NessieContainer"-->>"TestIcebergSystemTablesNessieWithBearerAuth": Connection established
Tester-->>"TestIcebergSystemTablesNessieWithBearerAuth": Test result
Entity relationship diagram for new Keycloak and Nessie container environment variableserDiagram
KEYCLOAK_CONTAINER {
KC_BOOTSTRAP_ADMIN_USERNAME string
KC_BOOTSTRAP_ADMIN_PASSWORD string
KC_HOSTNAME string
}
NESSIE_CONTAINER {
QUARKUS_HTTP_PORT int
NESSIE_VERSION_STORE_TYPE string
}
KEYCLOAK_CONTAINER ||--o| NESSIE_CONTAINER : "used for authentication"
Class diagram for new KeycloakContainer and related changesclassDiagram
class KeycloakContainer {
+String DEFAULT_IMAGE
+String DEFAULT_HOST_NAME
+String DEFAULT_USER_NAME
+String DEFAULT_PASSWORD
+String MASTER_REALM
+String ADMIN_CLI_CLIENT
+int PORT
+String SERVER_URL
+KeycloakContainer_Builder builder()
+String getUrl()
+String getAccessToken()
+void start()
}
class KeycloakContainer_Builder {
+KeycloakContainer build()
}
KeycloakContainer <|-- KeycloakContainer_Builder
KeycloakContainer --|> BaseTestContainer
KeycloakContainer_Builder --|> BaseTestContainer_Builder
class NessieContainer {
+ImmutableMap DEFAULT_ENV_VARS
}
NessieContainer --|> BaseTestContainer
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `presto-testing-docker/src/main/java/com/facebook/presto/testing/containers/KeycloakContainer.java:102` </location>
<code_context>
+ RealmRepresentation masterRep = master.toRepresentation();
+ // change access token lifespan from 1 minute (default) to 1 hour
+ // to keep the token alive in case testcase takes more than a minute to finish execution.
+ masterRep.setAccessTokenLifespan(3600);
+ master.update(masterRep);
+ return keycloak.tokenManager().grantToken().getToken();
</code_context>
<issue_to_address>
**issue (bug_risk):** Changing the access token lifespan on every token request may have unintended side effects.
Setting the access token lifespan within getAccessToken() can cause race conditions in concurrent tests. It's better to configure this during container setup or initialization.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| RealmRepresentation masterRep = master.toRepresentation(); | ||
| // change access token lifespan from 1 minute (default) to 1 hour | ||
| // to keep the token alive in case testcase takes more than a minute to finish execution. | ||
| masterRep.setAccessTokenLifespan(3600); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Changing the access token lifespan on every token request may have unintended side effects.
Setting the access token lifespan within getAccessToken() can cause race conditions in concurrent tests. It's better to configure this during container setup or initialization.
denodo-research-labs
force-pushed
the
bearer_authentication_nessie_catalog
branch
3 times, most recently
from
45f4a0b to
f5c6fe6
Compare
denodo-research-labs
force-pushed
the
bearer_authentication_nessie_catalog
branch
from
f5c6fe6 to
22c24b9
Compare
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `presto-testing-docker/src/main/java/com/facebook/presto/testing/containers/KeycloakContainer.java:116-119` </location>
<code_context>
+ this.image = DEFAULT_IMAGE;
+ this.hostName = DEFAULT_HOST_NAME;
+ this.exposePorts = ImmutableSet.of(PORT);
+ this.envVars = ImmutableMap.of(
+ "KC_BOOTSTRAP_ADMIN_USERNAME", DEFAULT_USER_NAME,
+ "KC_BOOTSTRAP_ADMIN_PASSWORD", DEFAULT_PASSWORD,
+ "KC_HOSTNAME", SERVER_URL);
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Setting KC_HOSTNAME to SERVER_URL may not match Keycloak's expected value.
KC_HOSTNAME should be set to just the hostname, not the full SERVER_URL. Use DEFAULT_HOST_NAME instead to avoid issues with Keycloak startup.
</issue_to_address>
### Comment 2
<location> `presto-iceberg/src/test/java/com/facebook/presto/iceberg/nessie/TestIcebergSystemTablesNessieWithBearerAuth.java:99` </location>
<code_context>
+ @Override
+ protected QueryRunner createQueryRunner()
</code_context>
<issue_to_address>
**suggestion (testing):** Suggestion to add negative test cases for invalid or expired Bearer tokens.
Please add tests for invalid and expired tokens to confirm authentication fails as expected.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+116
to
+119
| this.envVars = ImmutableMap.of( | ||
| "KC_BOOTSTRAP_ADMIN_USERNAME", DEFAULT_USER_NAME, | ||
| "KC_BOOTSTRAP_ADMIN_PASSWORD", DEFAULT_PASSWORD, | ||
| "KC_HOSTNAME", SERVER_URL); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Setting KC_HOSTNAME to SERVER_URL may not match Keycloak's expected value.
KC_HOSTNAME should be set to just the hostname, not the full SERVER_URL. Use DEFAULT_HOST_NAME instead to avoid issues with Keycloak startup.
|
|
||
| queryRunner.createCatalog(ICEBERG_CATALOG, "iceberg", icebergProperties); | ||
|
|
||
| return queryRunner; |
Contributor
There was a problem hiding this comment.
suggestion (testing): Suggestion to add negative test cases for invalid or expired Bearer tokens.
Please add tests for invalid and expired tokens to confirm authentication fails as expected.
denodo-research-labs
force-pushed
the
bearer_authentication_nessie_catalog
branch
from
22c24b9 to
da3406b
Compare
denodo-research-labs
force-pushed
the
bearer_authentication_nessie_catalog
branch
from
da3406b to
b45e02a
Compare
Contributor
Author
|
The test failure seems to be unrelated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Bearer authentication for Nessie catalog is not working
Motivation and Context
Fixes the problem with Bearer authentication for Nessie catalog
Impact
Is not possible to connect to Nessie catalog using Bearer authentication
Test Plan
A new TestIcebergSystemTablesNessieWithBearerAuth class has been added. It extends TestIcebergSystemTablesNessie and re-runs those tests using Bearer authentication. The other Nessie tests are not duplicated, as this class sufficiently validates the authentication mechanism.
Test adapted from trinodb/trino#17725
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.