Repository navigation
[#11443] feat(auth): Add Basic auth support for Spark, Flink, and Trino connectors - #11457
Conversation
…Trino Wire HTTP Basic authentication through Spark, Flink, and Trino connectors so clients can use the built-in IDP, and document the new configuration properties for each connector. Fix: apache#11443 Co-authored-by: Cursor <cursoragent@cursor.com>
…or-basic-auth Resolve documentation conflicts from the docs style sweep while keeping Basic authentication content for Spark, Flink, and Trino connectors. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…p-basic test dep Replace isTransitive=false with module excludes so bcprov and other idp-basic transitives are retained while api, common, core, and server-common stay on the explicit connector test classpath. Co-authored-by: Cursor <cursoragent@cursor.com>
…le deps Clarify that explicitly setting gravitino.client.auth.type only supports oauth2 and basic in the Flink connector doc, and add spacing between the idp-basic and hive2 test dependencies in spark-common build.gradle.kts. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Code Coverage Report
Files
|
| String gravitinoUri, Map<String, String> config) { | ||
| String username = config.get(GravitinoCatalogStoreFactoryOptions.BASIC_USERNAME); | ||
| String password = config.get(GravitinoCatalogStoreFactoryOptions.BASIC_PASSWORD); | ||
| if (Strings.isNullOrEmpty(username)) { |
| String username = config.get(GravitinoCatalogStoreFactoryOptions.BASIC_USERNAME); | ||
| String password = config.get(GravitinoCatalogStoreFactoryOptions.BASIC_PASSWORD); | ||
| if (Strings.isNullOrEmpty(username)) { | ||
| throw new IllegalArgumentException( |
| GravitinoAdminClient.AdminClientBuilder builder, Map<String, String> config) { | ||
| String username = config.get(BASIC_USERNAME_KEY); | ||
| String password = config.get(BASIC_PASSWORD_KEY); | ||
| if (StringUtils.isBlank(username)) { |
Co-authored-by: Cursor <cursoragent@cursor.com>
| * connector-side config parsing is covered by {@code TestGravitinoAuthProvider#testBuildBasicAuth}. | ||
| */ | ||
| public class TrinoBasicAuthIT extends BaseIT { | ||
|
|
There was a problem hiding this comment.
This test has nothing to do with Trino, right?
There was a problem hiding this comment.
Thank you very much for your suggestion.I have restructured this method and it will conduct the basic authentication test.
| try (GravitinoAdminClient basicClient = | ||
| GravitinoAdminClient.builder(gravitinoUri).withBasicAuth(ADMIN, ADMIN_PASSWORD).build()) { | ||
| GravitinoMetalake loadedMetalake = basicClient.loadMetalake(METALAKE); | ||
| Assertions.assertEquals(METALAKE, loadedMetalake.name()); |
There was a problem hiding this comment.
The integration test you added cannot verify whether Spark, Trino, and Flink can connect to the Gravitino server. It is more like a basic authentication test between the Gravitino client and the Gravitino server.
There was a problem hiding this comment.
Thank you very much for your suggestion.I have restructured this method and it will conduct the basic authentication test.
There was a problem hiding this comment.
What I mean is that simply verifying that SHOW CATALOGS does not throw an exception cannot validate your change. If this is the way you are testing it, it would be better to add an integration test to verify the basic authentication between the Gravitino client and the Gravitino server.
If this integration test is in place, you can add unit tests to ensure the connector configuration is processed correctly. and remove the integration tests for those connectors. Manual verification is also acceptable.
There was a problem hiding this comment.
Thank you very much for your suggestion. I have followed the method you mentioned and deleted the unnecessary IT, and also added the IT for the basic client. Additionally, I have already set up Trino/Spark/Flink clusters locally and successfully implemented basic authentication(Tested the create/select/insert table operations).
Resolve conflict in spark-authentication-with-gravitino.md by keeping basic auth in the authType supported values table. Co-authored-by: Cursor <cursoragent@cursor.com>
|
|
||
| @Test | ||
| void testTrinoConnectorConnectsWithBasicAuth() { | ||
| Assertions.assertDoesNotThrow(() -> trinoQueryRunner.runQuery("SHOW CATALOGS")); |
There was a problem hiding this comment.
Your test only relies on SHOW CATALOGS, which is insufficient to validate your changes.
There was a problem hiding this comment.
Thank you very much for your suggestion. I have followed the method you mentioned and deleted the unnecessary IT, and also added the IT for the basic client. Additionally, I have already set up Trino/Spark/Flink clusters locally and successfully implemented basic authentication(Tested the create/select/insert table operations).
…nt IT. Move end-to-end Basic authentication coverage to BasicAuthOperationsIT in client-java, and add connector UTs that verify Basic auth configuration parsing and validation without duplicating Spark/Flink/Trino integration tests. Co-authored-by: Cursor <cursoragent@cursor.com>
Initialize the Basic auth client in @BeforeAll, add @afterall teardown, and rename the test method to testAuthenticationApi for consistency. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Please update description of this PR |
Got Description has been updated. |
…rt for Spark, Flink, and Trino connectors (#11457) (#11531) **Cherry-pick Information:** - Original commit: d7fe5f5 - Target branch: `branch-1.3` - Status: ✅ Clean cherry-pick (no conflicts) --------- Co-authored-by: MaSai <30949009+lasdf1234@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Qi Yu <yuqi@datastrato.com> Co-authored-by: yuqi <yuqi4733@apache.org>
What changes were proposed in this pull request?
Add HTTP Basic authentication support to the Spark, Flink, and Trino connectors so they can
authenticate against the Gravitino built-in IDP:
GravitinoSparkConfigproperties andbasicbranch inGravitinoDriverPlugin.createGravitinoClient().basicauth inGravitinoCatalogManagerandGravitinoCatalogStoreFactoryOptions.basicauth inGravitinoAuthProvider.configuration examples and supported auth types.
Why are the changes needed?
The built-in IDP requires HTTP Basic Authentication, but the Spark/Flink/Trino connectors only
supported
simple,oauth2, andkerberos. Users could not connect through these connectorswith built-in IDP credentials even though the Java client already supports
withBasicAuth().Fix: #11443
Does this PR introduce any user-facing change?
Yes. New optional connector configuration for Basic auth:
spark.sql.gravitino.authType=basic,spark.sql.gravitino.basic.username,spark.sql.gravitino.basic.passwordtable.catalog-store.gravitino.gravitino.client.auth.type=basic,table.catalog-store.gravitino.gravitino.client.basic.username,table.catalog-store.gravitino.gravitino.client.basic.passwordgravitino.client.authType=basic,gravitino.client.basic.username,gravitino.client.basic.passwordWhen Flink
table.catalog-store.gravitino.gravitino.client.auth.typeis explicitly set, onlyoauth2andbasicare supported.How was this patch tested?
./gradlew :spark-connector:spark-common:test --tests "org.apache.gravitino.spark.connector.plugin.TestGravitinoSparkBasicAuth" -PskipITs./gradlew :flink-connector:flink-common:test --tests "org.apache.gravitino.flink.connector.catalog.TestGravitinoCatalogManagerBasicAuth" -PskipITs./gradlew :trino-connector:trino-connector:test --tests "org.apache.gravitino.trino.connector.security.TestGravitinoAuthProvider" -PskipITs./gradlew :clients:client-java:test --tests "org.apache.gravitino.client.integration.test.authorization.BasicAuthOperationsIT" -PskipITs -PjdbcBackend=h2