Copilot commented on code in PR #10052: URL: https://github.com/apache/gravitino/pull/10052#discussion_r2988906308
########## trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/GravitinoAuthProvider.java: ########## @@ -0,0 +1,224 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.trino.connector.catalog; + +import java.io.File; +import java.util.HashMap; +import java.util.Locale; +import java.util.Map; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.client.DefaultOAuth2TokenProvider; +import org.apache.gravitino.client.GravitinoAdminClient; +import org.apache.gravitino.client.GravitinoClientConfiguration; +import org.apache.gravitino.client.KerberosTokenProvider; +import org.apache.gravitino.trino.connector.GravitinoConfig; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Builds a {@link GravitinoAdminClient} with the appropriate authentication provider based on the + * Gravitino config. + */ +class GravitinoAuthProvider { + + private static final Logger LOG = LoggerFactory.getLogger(GravitinoAuthProvider.class); + + /** Authentication type configuration key. */ + static final String AUTH_TYPE_KEY = + GravitinoClientConfiguration.GRAVITINO_CLIENT_CONFIG_PREFIX + "authType"; + + /** Simple authentication user configuration key. */ + static final String SIMPLE_AUTH_USER_KEY = + GravitinoClientConfiguration.GRAVITINO_CLIENT_CONFIG_PREFIX + "simpleAuthUser"; + + /** OAuth server URI configuration key. */ + static final String OAUTH_SERVER_URI_KEY = + GravitinoClientConfiguration.GRAVITINO_CLIENT_CONFIG_PREFIX + "oauth.serverUri"; + + /** OAuth credential configuration key. */ + static final String OAUTH_CREDENTIAL_KEY = + GravitinoClientConfiguration.GRAVITINO_CLIENT_CONFIG_PREFIX + "oauth.credential"; + + /** OAuth path configuration key. */ + static final String OAUTH_PATH_KEY = + GravitinoClientConfiguration.GRAVITINO_CLIENT_CONFIG_PREFIX + "oauth.path"; + + /** OAuth scope configuration key. */ + static final String OAUTH_SCOPE_KEY = + GravitinoClientConfiguration.GRAVITINO_CLIENT_CONFIG_PREFIX + "oauth.scope"; + + /** Kerberos principal configuration key. */ + static final String KERBEROS_PRINCIPAL_KEY = + GravitinoClientConfiguration.GRAVITINO_CLIENT_CONFIG_PREFIX + "kerberos.principal"; + + /** Kerberos keytab file path configuration key. */ + static final String KERBEROS_KEYTAB_FILE_PATH_KEY = + GravitinoClientConfiguration.GRAVITINO_CLIENT_CONFIG_PREFIX + "kerberos.keytabFilePath"; + + /** Authentication types supported by the Trino connector. */ + enum AuthType { + SIMPLE, + OAUTH, + KERBEROS, + NONE + } + + private GravitinoAuthProvider() {} + + /** + * Builds a GravitinoAdminClient from the given config, applying authentication settings found in + * the client config. + * + * @param config the Gravitino configuration containing server URI and client properties + * @return a configured GravitinoAdminClient + */ + public static GravitinoAdminClient buildClient(GravitinoConfig config) { + Map<String, String> clientConfig = new HashMap<>(config.getClientConfig()); + String uri = config.getURI(); + String authTypeStr = clientConfig.get(AUTH_TYPE_KEY); + + GravitinoAdminClient.AdminClientBuilder builder = GravitinoAdminClient.builder(uri); + + if (StringUtils.isNotBlank(authTypeStr)) { + AuthType authType; + try { + authType = AuthType.valueOf(authTypeStr.toUpperCase(Locale.ROOT)); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException( + String.format( + "Invalid authentication type: %s. Valid values are: simple, oauth, kerberos, none", + authTypeStr), + e); + } + + switch (authType) { + case SIMPLE: + buildSimpleAuth(builder, clientConfig); + break; + case OAUTH: + builder.withOAuth(buildOAuthProvider(clientConfig)); + break; + case KERBEROS: + builder.withKerberosAuth(buildKerberosProvider(clientConfig)); + break; + case NONE: + default: + break; + } + } + + // Remove auth-specific keys before passing to withClientConfig + clientConfig.remove(AUTH_TYPE_KEY); + clientConfig.remove(SIMPLE_AUTH_USER_KEY); + clientConfig.remove(OAUTH_SERVER_URI_KEY); + clientConfig.remove(OAUTH_CREDENTIAL_KEY); + clientConfig.remove(OAUTH_PATH_KEY); + clientConfig.remove(OAUTH_SCOPE_KEY); + clientConfig.remove(KERBEROS_PRINCIPAL_KEY); + clientConfig.remove(KERBEROS_KEYTAB_FILE_PATH_KEY); + + builder.withClientConfig(clientConfig); + return builder.build(); + } + + private static void buildSimpleAuth( + GravitinoAdminClient.AdminClientBuilder builder, Map<String, String> clientConfig) { + String simpleUser = clientConfig.get(SIMPLE_AUTH_USER_KEY); + if (StringUtils.isNotBlank(simpleUser)) { + builder.withSimpleAuth(simpleUser); + } else { + builder.withSimpleAuth(); + } + } + + private static DefaultOAuth2TokenProvider buildOAuthProvider(Map<String, String> config) { + String serverUri = config.get(OAUTH_SERVER_URI_KEY); + String credential = config.get(OAUTH_CREDENTIAL_KEY); + String path = config.get(OAUTH_PATH_KEY); + String scope = config.get(OAUTH_SCOPE_KEY); + + if (StringUtils.isBlank(serverUri)) { + throw new IllegalArgumentException( + String.format("OAuth server URI is required. Please set %s", OAUTH_SERVER_URI_KEY)); + } + if (StringUtils.isBlank(credential)) { + throw new IllegalArgumentException( + String.format("OAuth credential is required. Please set %s", OAUTH_CREDENTIAL_KEY)); + } + if (StringUtils.isBlank(path)) { + throw new IllegalArgumentException( + String.format("OAuth path is required. Please set %s", OAUTH_PATH_KEY)); + } + if (StringUtils.isBlank(scope)) { + throw new IllegalArgumentException( + String.format("OAuth scope is required. Please set %s", OAUTH_SCOPE_KEY)); + } + + // Remove leading slash from path if present + String normalizedPath = path.startsWith("/") ? path.substring(1) : path; + + LOG.info("Initializing OAuth2 token provider with server URI: {}", serverUri); + try { + return DefaultOAuth2TokenProvider.builder() + .withUri(serverUri) + .withCredential(credential) + .withPath(normalizedPath) + .withScope(scope) + .build(); + } catch (Exception e) { + throw new IllegalStateException( + String.format( + "Failed to initialize OAuth2 token provider for server URI '%s'. " + + "Check that the server is reachable and credentials are correct.", + serverUri), + e); + } Review Comment: `buildOAuthProvider()` catches `Exception` broadly and wraps it. Per project guidelines, avoid generic `catch (Exception)`; it can mask programmer errors (e.g., NPE) and makes it harder to handle expected failures cleanly. Prefer catching the specific exception types you expect from the OAuth provider build / token fetch and letting unexpected runtime exceptions propagate. ```suggestion return DefaultOAuth2TokenProvider.builder() .withUri(serverUri) .withCredential(credential) .withPath(normalizedPath) .withScope(scope) .build(); ``` ########## trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/catalog/TestGravitinoAuthProvider.java: ########## @@ -0,0 +1,154 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.trino.connector.catalog; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import com.google.common.collect.ImmutableMap; +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import org.apache.gravitino.trino.connector.GravitinoConfig; +import org.junit.jupiter.api.Assumptions; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +public class TestGravitinoAuthProvider { + + @Test + public void testBuildClientNoAuth() { + assertDoesNotThrow(() -> GravitinoAuthProvider.buildClient(buildConfig(ImmutableMap.of()))); + } + + @Test + public void testBuildClientNoneAuth() { + assertDoesNotThrow( + () -> + GravitinoAuthProvider.buildClient( + buildConfig(ImmutableMap.of(GravitinoAuthProvider.AUTH_TYPE_KEY, "none")))); + } + + @Test + public void testBuildClientSimpleAuthWithUser() { + assertDoesNotThrow( + () -> + GravitinoAuthProvider.buildClient( + buildConfig( + ImmutableMap.of( + GravitinoAuthProvider.AUTH_TYPE_KEY, "simple", + GravitinoAuthProvider.SIMPLE_AUTH_USER_KEY, "alice")))); + } + + @Test + public void testBuildClientSimpleAuthNoUser() { + assertDoesNotThrow( + () -> + GravitinoAuthProvider.buildClient( + buildConfig(ImmutableMap.of(GravitinoAuthProvider.AUTH_TYPE_KEY, "simple")))); + } + + @Test + public void testBuildClientInvalidAuthType() { + assertThrows( + IllegalArgumentException.class, + () -> + GravitinoAuthProvider.buildClient( + buildConfig(ImmutableMap.of(GravitinoAuthProvider.AUTH_TYPE_KEY, "invalid_type")))); + } + + @Test + public void testBuildClientOAuthMissingServerUri() { + assertThrows( + IllegalArgumentException.class, + () -> + GravitinoAuthProvider.buildClient( + buildConfig( + ImmutableMap.of( + GravitinoAuthProvider.AUTH_TYPE_KEY, "oauth", + GravitinoAuthProvider.OAUTH_CREDENTIAL_KEY, "cred", + GravitinoAuthProvider.OAUTH_PATH_KEY, "oauth2/token", + GravitinoAuthProvider.OAUTH_SCOPE_KEY, "scope")))); + } + + @Test + public void testBuildClientOAuthMissingCredential() { + assertThrows( + IllegalArgumentException.class, + () -> + GravitinoAuthProvider.buildClient( + buildConfig( + ImmutableMap.of( + GravitinoAuthProvider.AUTH_TYPE_KEY, "oauth", + GravitinoAuthProvider.OAUTH_SERVER_URI_KEY, "http://auth.example.com";, + GravitinoAuthProvider.OAUTH_PATH_KEY, "oauth2/token", + GravitinoAuthProvider.OAUTH_SCOPE_KEY, "scope")))); + } + + @Test + public void testBuildClientKerberosMissingPrincipal() { + assertThrows( + IllegalArgumentException.class, + () -> + GravitinoAuthProvider.buildClient( + buildConfig(ImmutableMap.of(GravitinoAuthProvider.AUTH_TYPE_KEY, "kerberos")))); + } + + @Test + public void testBuildClientKerberosKeytabNotFound(@TempDir java.nio.file.Path tempDir) { + assertThrows( + IllegalArgumentException.class, + () -> + GravitinoAuthProvider.buildClient( + buildConfig( + ImmutableMap.of( + GravitinoAuthProvider.AUTH_TYPE_KEY, "kerberos", + GravitinoAuthProvider.KERBEROS_PRINCIPAL_KEY, "[email protected]", + GravitinoAuthProvider.KERBEROS_KEYTAB_FILE_PATH_KEY, + tempDir.resolve("missing.keytab").toString())))); + } + + @Test + public void testBuildClientKerberosWithKeytab(@TempDir java.nio.file.Path tempDir) + throws IOException { + // KerberosTokenProvider.build() calls Subject.getSubject(AccessController.getContext()) + // which throws UnsupportedOperationException on JVM 17+. + Assumptions.assumeTrue(Runtime.version().feature() < 17, "Kerberos test skipped on JVM 17+"); + File keytabFile = tempDir.resolve("user.keytab").toFile(); Review Comment: The test is skipped on JVM 17+ due to an assumption that `Subject.getSubject(AccessController.getContext())` throws `UnsupportedOperationException` starting in Java 17. That behavior actually changes in later JDKs (it does *not* throw on 17–22; it becomes unsupported starting in newer releases when the SecurityManager is disabled). This skip condition should be updated (or removed) so Kerberos coverage isn’t unnecessarily lost on the project’s current JDKs. ########## docs/trino-connector/authentication.md: ########## @@ -0,0 +1,136 @@ +--- +title: "Apache Gravitino Trino connector Authentication" +slug: /trino-connector/authentication +keyword: gravitino connector trino authentication +license: "This software is licensed under the Apache License version 2." +--- + +## Authentication + +The Gravitino Trino connector supports authenticating to the Gravitino server using the same authentication mechanisms as the Gravitino Java client: Simple, OAuth, and Kerberos. Authentication is configured through the Trino connector properties file using the `gravitino.client.*` prefix. + +If `gravitino.client.authType` is not set, the connector operates in no-authentication mode and connects to the Gravitino server without any credentials. + +### Simple Authentication + +Simple authentication uses a username to authenticate with the Gravitino server. + +**Configuration in `etc/catalog/gravitino.properties`:** + +```properties +connector.name=gravitino +gravitino.metalake=metalake +gravitino.uri=http://localhost:8090 + +# Simple authentication with username +gravitino.client.authType=simple +gravitino.client.simpleAuthUser=admin +``` + +**Configuration properties:** + +| Property | Description | Default value | Required | Since version | +|-----------------------------------|----------------------------------------------------------------------|------------------|----------------------------------------|---------------| +| `gravitino.client.authType` | Authentication type: `simple`, `oauth`, or `kerberos` | (none) | No | 1.3.0 | +| `gravitino.client.simpleAuthUser` | Username for simple authentication | (none) | No (uses system user if not specified) | 1.3.0 | + +### OAuth Authentication + +OAuth authentication uses OAuth 2.0 tokens to authenticate with the Gravitino server. + +**Configuration in `etc/catalog/gravitino.properties`:** + +```properties +connector.name=gravitino +gravitino.metalake=metalake +gravitino.uri=http://localhost:8090 + +# OAuth authentication +gravitino.client.authType=oauth +gravitino.client.oauth.serverUri=http://oauth-server:8080 +gravitino.client.oauth.credential=client_id:client_secret +gravitino.client.oauth.path=oauth2/token +gravitino.client.oauth.scope=gravitino +``` + +**Configuration properties:** + +| Property | Description | Default value | Required | Since version | +|-------------------------------------|-------------------------------------------------------|---------------|----------------------------|---------------| +| `gravitino.client.authType` | Authentication type: `simple`, `oauth`, or `kerberos` | (none) | Yes | 1.3.0 | +| `gravitino.client.oauth.serverUri` | OAuth server URI | (none) | Yes if authType is `oauth` | 1.3.0 | +| `gravitino.client.oauth.credential` | OAuth credentials in format `client_id:client_secret` | (none) | Yes if authType is `oauth` | 1.3.0 | +| `gravitino.client.oauth.path` | OAuth token endpoint path | (none) | Yes if authType is `oauth` | 1.3.0 | +| `gravitino.client.oauth.scope` | OAuth scope | (none) | Yes if authType is `oauth` | 1.3.0 | + +### Kerberos Authentication + +Kerberos authentication uses Kerberos tickets to authenticate with the Gravitino server. + +**Configuration in `etc/catalog/gravitino.properties`:** + +```properties +connector.name=gravitino +gravitino.metalake=metalake +gravitino.uri=http://localhost:8090 + +# Kerberos authentication with keytab +gravitino.client.authType=kerberos +gravitino.client.kerberos.principal=user@REALM +gravitino.client.kerberos.keytabFilePath=/path/to/user.keytab +``` + +**Configuration properties:** + +| Property | Description | Default value | Required | Since version | +|--------------------------------------------|---------------------|---------------|-----------------------------------------|---------------| +| `gravitino.client.authType` | Authentication type: `simple`, `oauth`, or `kerberos` | (none) | Yes | 1.3.0 | +| `gravitino.client.kerberos.principal` | Kerberos principal | (none) | Yes if authType is `kerberos` | 1.3.0 | +| `gravitino.client.kerberos.keytabFilePath` | Path to keytab file | (none) | No (uses ticket cache if not specified) | 1.3.0 | Review Comment: Similarly in the Kerberos section table, `gravitino.client.authType` is marked as required, which can be read as “always required” even though the connector supports no-auth mode. Consider updating the “Required” column wording to reflect that it’s required only when Kerberos auth is enabled. ```suggestion | Property | Description | Default value | Required | Since version | |--------------------------------------------|---------------------|---------------|------------------------------------------------------------|---------------| | `gravitino.client.authType` | Authentication type: `simple`, `oauth`, or `kerberos` | (none) | Yes, when authentication is enabled; omit for no-auth mode | 1.3.0 | | `gravitino.client.kerberos.principal` | Kerberos principal | (none) | Yes if authType is `kerberos` | 1.3.0 | | `gravitino.client.kerberos.keytabFilePath` | Path to keytab file | (none) | No (uses ticket cache if not specified) | 1.3.0 | ``` ########## trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/GravitinoAuthProvider.java: ########## @@ -0,0 +1,224 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.trino.connector.catalog; + +import java.io.File; +import java.util.HashMap; +import java.util.Locale; +import java.util.Map; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.client.DefaultOAuth2TokenProvider; +import org.apache.gravitino.client.GravitinoAdminClient; +import org.apache.gravitino.client.GravitinoClientConfiguration; +import org.apache.gravitino.client.KerberosTokenProvider; +import org.apache.gravitino.trino.connector.GravitinoConfig; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Builds a {@link GravitinoAdminClient} with the appropriate authentication provider based on the + * Gravitino config. + */ +class GravitinoAuthProvider { + + private static final Logger LOG = LoggerFactory.getLogger(GravitinoAuthProvider.class); + + /** Authentication type configuration key. */ + static final String AUTH_TYPE_KEY = + GravitinoClientConfiguration.GRAVITINO_CLIENT_CONFIG_PREFIX + "authType"; + + /** Simple authentication user configuration key. */ + static final String SIMPLE_AUTH_USER_KEY = + GravitinoClientConfiguration.GRAVITINO_CLIENT_CONFIG_PREFIX + "simpleAuthUser"; + + /** OAuth server URI configuration key. */ + static final String OAUTH_SERVER_URI_KEY = + GravitinoClientConfiguration.GRAVITINO_CLIENT_CONFIG_PREFIX + "oauth.serverUri"; + + /** OAuth credential configuration key. */ + static final String OAUTH_CREDENTIAL_KEY = + GravitinoClientConfiguration.GRAVITINO_CLIENT_CONFIG_PREFIX + "oauth.credential"; + + /** OAuth path configuration key. */ + static final String OAUTH_PATH_KEY = + GravitinoClientConfiguration.GRAVITINO_CLIENT_CONFIG_PREFIX + "oauth.path"; + + /** OAuth scope configuration key. */ + static final String OAUTH_SCOPE_KEY = + GravitinoClientConfiguration.GRAVITINO_CLIENT_CONFIG_PREFIX + "oauth.scope"; + + /** Kerberos principal configuration key. */ + static final String KERBEROS_PRINCIPAL_KEY = + GravitinoClientConfiguration.GRAVITINO_CLIENT_CONFIG_PREFIX + "kerberos.principal"; + + /** Kerberos keytab file path configuration key. */ + static final String KERBEROS_KEYTAB_FILE_PATH_KEY = + GravitinoClientConfiguration.GRAVITINO_CLIENT_CONFIG_PREFIX + "kerberos.keytabFilePath"; + + /** Authentication types supported by the Trino connector. */ + enum AuthType { + SIMPLE, + OAUTH, + KERBEROS, + NONE + } + + private GravitinoAuthProvider() {} + + /** + * Builds a GravitinoAdminClient from the given config, applying authentication settings found in + * the client config. + * + * @param config the Gravitino configuration containing server URI and client properties + * @return a configured GravitinoAdminClient + */ + public static GravitinoAdminClient buildClient(GravitinoConfig config) { + Map<String, String> clientConfig = new HashMap<>(config.getClientConfig()); + String uri = config.getURI(); + String authTypeStr = clientConfig.get(AUTH_TYPE_KEY); + + GravitinoAdminClient.AdminClientBuilder builder = GravitinoAdminClient.builder(uri); + + if (StringUtils.isNotBlank(authTypeStr)) { + AuthType authType; + try { + authType = AuthType.valueOf(authTypeStr.toUpperCase(Locale.ROOT)); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException( + String.format( + "Invalid authentication type: %s. Valid values are: simple, oauth, kerberos, none", + authTypeStr), + e); + } + + switch (authType) { + case SIMPLE: + buildSimpleAuth(builder, clientConfig); + break; + case OAUTH: + builder.withOAuth(buildOAuthProvider(clientConfig)); + break; + case KERBEROS: + builder.withKerberosAuth(buildKerberosProvider(clientConfig)); + break; + case NONE: + default: + break; + } + } + + // Remove auth-specific keys before passing to withClientConfig + clientConfig.remove(AUTH_TYPE_KEY); + clientConfig.remove(SIMPLE_AUTH_USER_KEY); + clientConfig.remove(OAUTH_SERVER_URI_KEY); + clientConfig.remove(OAUTH_CREDENTIAL_KEY); + clientConfig.remove(OAUTH_PATH_KEY); + clientConfig.remove(OAUTH_SCOPE_KEY); + clientConfig.remove(KERBEROS_PRINCIPAL_KEY); + clientConfig.remove(KERBEROS_KEYTAB_FILE_PATH_KEY); + + builder.withClientConfig(clientConfig); + return builder.build(); + } + + private static void buildSimpleAuth( + GravitinoAdminClient.AdminClientBuilder builder, Map<String, String> clientConfig) { + String simpleUser = clientConfig.get(SIMPLE_AUTH_USER_KEY); + if (StringUtils.isNotBlank(simpleUser)) { + builder.withSimpleAuth(simpleUser); + } else { + builder.withSimpleAuth(); + } + } + + private static DefaultOAuth2TokenProvider buildOAuthProvider(Map<String, String> config) { + String serverUri = config.get(OAUTH_SERVER_URI_KEY); + String credential = config.get(OAUTH_CREDENTIAL_KEY); + String path = config.get(OAUTH_PATH_KEY); + String scope = config.get(OAUTH_SCOPE_KEY); + + if (StringUtils.isBlank(serverUri)) { + throw new IllegalArgumentException( + String.format("OAuth server URI is required. Please set %s", OAUTH_SERVER_URI_KEY)); + } + if (StringUtils.isBlank(credential)) { + throw new IllegalArgumentException( + String.format("OAuth credential is required. Please set %s", OAUTH_CREDENTIAL_KEY)); + } + if (StringUtils.isBlank(path)) { + throw new IllegalArgumentException( + String.format("OAuth path is required. Please set %s", OAUTH_PATH_KEY)); + } + if (StringUtils.isBlank(scope)) { + throw new IllegalArgumentException( + String.format("OAuth scope is required. Please set %s", OAUTH_SCOPE_KEY)); + } + + // Remove leading slash from path if present + String normalizedPath = path.startsWith("/") ? path.substring(1) : path; + + LOG.info("Initializing OAuth2 token provider with server URI: {}", serverUri); + try { + return DefaultOAuth2TokenProvider.builder() + .withUri(serverUri) + .withCredential(credential) + .withPath(normalizedPath) + .withScope(scope) + .build(); + } catch (Exception e) { + throw new IllegalStateException( + String.format( + "Failed to initialize OAuth2 token provider for server URI '%s'. " + + "Check that the server is reachable and credentials are correct.", + serverUri), + e); + } + } + + private static KerberosTokenProvider buildKerberosProvider(Map<String, String> config) { + String principal = config.get(KERBEROS_PRINCIPAL_KEY); + String keytabFilePath = config.get(KERBEROS_KEYTAB_FILE_PATH_KEY); + + if (StringUtils.isBlank(principal)) { + throw new IllegalArgumentException( + String.format("Kerberos principal is required. Please set %s", KERBEROS_PRINCIPAL_KEY)); + } + + KerberosTokenProvider.Builder kerberosBuilder = + KerberosTokenProvider.builder().withClientPrincipal(principal); + + if (StringUtils.isNotBlank(keytabFilePath)) { + File keytabFile = new File(keytabFilePath); + if (!keytabFile.exists()) { + throw new IllegalArgumentException( + String.format("Keytab file not found: %s", keytabFilePath)); + } Review Comment: `buildKerberosProvider()` only checks `exists()` for the keytab path. If the path points to a directory (or an unreadable file), it will pass this check and fail later in a less actionable way. Consider validating `keytabFile.isFile()` and `keytabFile.canRead()` here (and using a message that points to the config key) before calling `withKeyTabFile(...)`. ```suggestion String.format( "Keytab file configured via %s does not exist: %s", KERBEROS_KEYTAB_FILE_PATH_KEY, keytabFilePath)); } if (!keytabFile.isFile()) { throw new IllegalArgumentException( String.format( "Keytab path configured via %s is not a file: %s", KERBEROS_KEYTAB_FILE_PATH_KEY, keytabFilePath)); } if (!keytabFile.canRead()) { throw new IllegalArgumentException( String.format( "Keytab file configured via %s is not readable: %s", KERBEROS_KEYTAB_FILE_PATH_KEY, keytabFilePath)); } ``` ########## trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorManager.java: ########## @@ -116,10 +116,16 @@ public void config(GravitinoConfig config, GravitinoAdminClient client) { Preconditions.checkArgument(config != null, "config is not null"); this.config = config; if (client == null) { - this.gravitinoClient = - GravitinoAdminClient.builder(config.getURI()) - .withClientConfig(config.getClientConfig()) - .build(); + String authType = config.getClientConfig().getOrDefault("gravitino.client.authType", "none"); + LOG.info("Building Gravitino client with authType: {}", authType); + try { + this.gravitinoClient = GravitinoAuthProvider.buildClient(config); + } catch (Exception e) { + throw new TrinoException( + GravitinoErrorCode.GRAVITINO_MISSING_CONFIG, + "Failed to build Gravitino client with authType '" + authType + "': " + e.getMessage(), + e); Review Comment: `config()` wraps any failure to build the Gravitino client as `GRAVITINO_MISSING_CONFIG`, but `GravitinoAuthProvider.buildClient()` can also fail for runtime reasons (e.g., OAuth token endpoint unreachable) that aren’t “missing config”. Consider mapping invalid properties to `GRAVITINO_ILLEGAL_ARGUMENT` and runtime failures to `GRAVITINO_RUNTIME_ERROR` (or a more specific code) so operators get accurate error classification. ```suggestion } catch (IllegalArgumentException e) { throw new TrinoException( GravitinoErrorCode.GRAVITINO_ILLEGAL_ARGUMENT, "Invalid Gravitino client configuration for authType '" + authType + "': " + e.getMessage(), e); } catch (RuntimeException e) { throw new TrinoException( GravitinoErrorCode.GRAVITINO_RUNTIME_ERROR, "Runtime failure while building Gravitino client with authType '" + authType + "': " + e.getMessage(), e); ``` ########## docs/trino-connector/authentication.md: ########## @@ -0,0 +1,136 @@ +--- +title: "Apache Gravitino Trino connector Authentication" +slug: /trino-connector/authentication +keyword: gravitino connector trino authentication +license: "This software is licensed under the Apache License version 2." +--- + +## Authentication + +The Gravitino Trino connector supports authenticating to the Gravitino server using the same authentication mechanisms as the Gravitino Java client: Simple, OAuth, and Kerberos. Authentication is configured through the Trino connector properties file using the `gravitino.client.*` prefix. + +If `gravitino.client.authType` is not set, the connector operates in no-authentication mode and connects to the Gravitino server without any credentials. + +### Simple Authentication + +Simple authentication uses a username to authenticate with the Gravitino server. + +**Configuration in `etc/catalog/gravitino.properties`:** + +```properties +connector.name=gravitino +gravitino.metalake=metalake +gravitino.uri=http://localhost:8090 + +# Simple authentication with username +gravitino.client.authType=simple +gravitino.client.simpleAuthUser=admin +``` + +**Configuration properties:** + +| Property | Description | Default value | Required | Since version | +|-----------------------------------|----------------------------------------------------------------------|------------------|----------------------------------------|---------------| +| `gravitino.client.authType` | Authentication type: `simple`, `oauth`, or `kerberos` | (none) | No | 1.3.0 | +| `gravitino.client.simpleAuthUser` | Username for simple authentication | (none) | No (uses system user if not specified) | 1.3.0 | + +### OAuth Authentication + +OAuth authentication uses OAuth 2.0 tokens to authenticate with the Gravitino server. + +**Configuration in `etc/catalog/gravitino.properties`:** + +```properties +connector.name=gravitino +gravitino.metalake=metalake +gravitino.uri=http://localhost:8090 + +# OAuth authentication +gravitino.client.authType=oauth +gravitino.client.oauth.serverUri=http://oauth-server:8080 +gravitino.client.oauth.credential=client_id:client_secret +gravitino.client.oauth.path=oauth2/token +gravitino.client.oauth.scope=gravitino +``` + +**Configuration properties:** + +| Property | Description | Default value | Required | Since version | +|-------------------------------------|-------------------------------------------------------|---------------|----------------------------|---------------| +| `gravitino.client.authType` | Authentication type: `simple`, `oauth`, or `kerberos` | (none) | Yes | 1.3.0 | +| `gravitino.client.oauth.serverUri` | OAuth server URI | (none) | Yes if authType is `oauth` | 1.3.0 | +| `gravitino.client.oauth.credential` | OAuth credentials in format `client_id:client_secret` | (none) | Yes if authType is `oauth` | 1.3.0 | +| `gravitino.client.oauth.path` | OAuth token endpoint path | (none) | Yes if authType is `oauth` | 1.3.0 | +| `gravitino.client.oauth.scope` | OAuth scope | (none) | Yes if authType is `oauth` | 1.3.0 | Review Comment: In the OAuth section table, `gravitino.client.authType` is marked as `Required = Yes`, but earlier in this doc you state that if `authType` is not set the connector runs in no-auth mode. For consistency/clarity, consider expressing this as “Yes (to enable OAuth)” or “Yes if using OAuth”, rather than implying it’s required for the connector overall. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
