Copilot commented on code in PR #10726: URL: https://github.com/apache/gravitino/pull/10726#discussion_r3055986066
########## catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueClientProvider.java: ########## @@ -0,0 +1,123 @@ +/* + * 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.catalog.glue; + +import static org.apache.gravitino.catalog.glue.GlueConstants.AWS_ACCESS_KEY_ID; +import static org.apache.gravitino.catalog.glue.GlueConstants.AWS_GLUE_ENDPOINT; +import static org.apache.gravitino.catalog.glue.GlueConstants.AWS_REGION; +import static org.apache.gravitino.catalog.glue.GlueConstants.AWS_SECRET_ACCESS_KEY; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.HashMap; +import java.util.Map; +import org.junit.jupiter.api.Test; +import software.amazon.awssdk.services.glue.GlueClient; + +class TestGlueClientProvider { + + @Test + void testBuildClientWithStaticCredentials() { + Map<String, String> config = new HashMap<>(); + config.put(AWS_REGION, "us-east-1"); + config.put(AWS_ACCESS_KEY_ID, "AKIAIOSFODNN7EXAMPLE"); + config.put(AWS_SECRET_ACCESS_KEY, "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"); Review Comment: These look like real AWS credential patterns and may trigger secret scanners in CI/security tooling even though they’re documented examples. Consider replacing them with obviously fake/test-only values (e.g., 'test-access-key'/'test-secret-key') since the tests only validate client construction. ```suggestion config.put(AWS_ACCESS_KEY_ID, "test-access-key"); config.put(AWS_SECRET_ACCESS_KEY, "test-secret-key"); ``` ########## catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueClientProvider.java: ########## @@ -0,0 +1,104 @@ +/* + * 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.catalog.glue; + +import com.google.common.base.Preconditions; +import com.google.common.base.Strings; +import java.net.URI; +import java.util.Map; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.glue.GlueClient; +import software.amazon.awssdk.services.glue.GlueClientBuilder; + +/** + * Factory for creating AWS {@link GlueClient} instances from Gravitino catalog configuration. + * + * <p>Authentication priority: + * + * <ol> + * <li>Static credentials ({@code aws-access-key-id} + {@code aws-secret-access-key}) + * <li>Default credential chain (environment variables, instance profile, container credentials) + * </ol> + * + * <p>An optional endpoint override ({@code aws-glue-endpoint}) enables connectivity to VPC + * endpoints and LocalStack for integration testing. + */ +public final class GlueClientProvider { + + private GlueClientProvider() {} + + /** + * Builds a {@link GlueClient} from the given catalog configuration map. + * + * @param config Catalog configuration properties. + * @return A configured and ready-to-use {@link GlueClient}. + * @throws IllegalArgumentException if {@code aws-region} is missing or blank, if only one of the + * credential keys is provided, or if {@code aws-glue-endpoint} is not a valid URI. + */ + public static GlueClient buildClient(Map<String, String> config) { + String region = config.get(GlueConstants.AWS_REGION); + Preconditions.checkArgument( + !Strings.isNullOrEmpty(region) && !region.isBlank(), Review Comment: The same 'non-null/non-empty/non-blank' check is repeated for region/keys/endpoint. Consider extracting a small helper (e.g., `isNotBlank(String)`) to reduce duplication and make the intent consistent across all fields. ########## catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueClientProvider.java: ########## @@ -0,0 +1,104 @@ +/* + * 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.catalog.glue; + +import com.google.common.base.Preconditions; +import com.google.common.base.Strings; +import java.net.URI; +import java.util.Map; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.glue.GlueClient; +import software.amazon.awssdk.services.glue.GlueClientBuilder; + +/** + * Factory for creating AWS {@link GlueClient} instances from Gravitino catalog configuration. + * + * <p>Authentication priority: + * + * <ol> + * <li>Static credentials ({@code aws-access-key-id} + {@code aws-secret-access-key}) + * <li>Default credential chain (environment variables, instance profile, container credentials) + * </ol> + * + * <p>An optional endpoint override ({@code aws-glue-endpoint}) enables connectivity to VPC + * endpoints and LocalStack for integration testing. + */ +public final class GlueClientProvider { + + private GlueClientProvider() {} + + /** + * Builds a {@link GlueClient} from the given catalog configuration map. + * + * @param config Catalog configuration properties. + * @return A configured and ready-to-use {@link GlueClient}. + * @throws IllegalArgumentException if {@code aws-region} is missing or blank, if only one of the + * credential keys is provided, or if {@code aws-glue-endpoint} is not a valid URI. + */ + public static GlueClient buildClient(Map<String, String> config) { + String region = config.get(GlueConstants.AWS_REGION); + Preconditions.checkArgument( + !Strings.isNullOrEmpty(region) && !region.isBlank(), + "Property '%s' is required to create a Glue client", + GlueConstants.AWS_REGION); + + GlueClientBuilder builder = GlueClient.builder().region(Region.of(region)); + + // Static credentials take priority over the default credential chain. + // Both keys must be provided together — a partial pair is always a misconfiguration. + String accessKey = config.get(GlueConstants.AWS_ACCESS_KEY_ID); + String secretKey = config.get(GlueConstants.AWS_SECRET_ACCESS_KEY); + boolean hasAccessKey = !Strings.isNullOrEmpty(accessKey) && !accessKey.isBlank(); + boolean hasSecretKey = !Strings.isNullOrEmpty(secretKey) && !secretKey.isBlank(); + Preconditions.checkArgument( + hasAccessKey == hasSecretKey, + "Incomplete static credentials: '%s' requires '%s'. " + + "Either provide both keys for static authentication, " + + "or omit both to use the default credential chain.", + hasAccessKey ? GlueConstants.AWS_ACCESS_KEY_ID : GlueConstants.AWS_SECRET_ACCESS_KEY, + hasAccessKey ? GlueConstants.AWS_SECRET_ACCESS_KEY : GlueConstants.AWS_ACCESS_KEY_ID); + + if (hasAccessKey) { + builder.credentialsProvider( + StaticCredentialsProvider.create(AwsBasicCredentials.create(accessKey, secretKey))); + } else { + builder.credentialsProvider(DefaultCredentialsProvider.create()); + } + + // Optional custom endpoint override for VPC endpoints or LocalStack testing. + String endpoint = config.get(GlueConstants.AWS_GLUE_ENDPOINT); + if (!Strings.isNullOrEmpty(endpoint) && !endpoint.isBlank()) { Review Comment: The same 'non-null/non-empty/non-blank' check is repeated for region/keys/endpoint. Consider extracting a small helper (e.g., `isNotBlank(String)`) to reduce duplication and make the intent consistent across all fields. ########## catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueTablePropertiesMetadata.java: ########## @@ -26,12 +30,36 @@ /** * Properties metadata for Glue tables. * - * <p>TODO PR-02: support passthrough of Glue Table.parameters() keys such as {@code table_type} and - * {@code metadata_location} for Iceberg, Delta, and other formats. + * <p>Defines well-known Glue {@code Table.parameters()} keys that Gravitino exposes. All entries + * are optional and mutable, reflecting that Glue stores them as free-form key-value pairs. Unknown + * parameters from {@code Table.parameters()} are passed through transparently by the catalog + * operations layer and are not validated here. + * + * <p>Note: storage location ({@code StorageDescriptor.location}) varies by table format and is + * handled per-format in the Table CRUD layer, not declared here. */ public class GlueTablePropertiesMetadata extends BasePropertiesMetadata { - private static final Map<String, PropertyEntry<?>> PROPERTIES_METADATA = ImmutableMap.of(); + private static final Map<String, PropertyEntry<?>> PROPERTIES_METADATA = + ImmutableMap.<String, PropertyEntry<?>>builder() + .put( + TABLE_FORMAT_TYPE, + stringOptionalPropertyEntry( + TABLE_FORMAT_TYPE, + "Glue table format type stored in Table.parameters(). Common values:" + + " ICEBERG, HIVE, DELTA, PARQUET.", Review Comment: This list of common values is inconsistent with `GlueConstants.TABLE_FORMAT_TYPE` Javadoc (which also mentions `VIRTUAL_VIEW`). Consider aligning the documented value set and casing across both places to avoid conflicting guidance. ```suggestion + " ICEBERG, HIVE, DELTA, PARQUET, VIRTUAL_VIEW.", ``` ########## catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueClientProvider.java: ########## @@ -0,0 +1,104 @@ +/* + * 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.catalog.glue; + +import com.google.common.base.Preconditions; +import com.google.common.base.Strings; +import java.net.URI; +import java.util.Map; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.glue.GlueClient; +import software.amazon.awssdk.services.glue.GlueClientBuilder; + +/** + * Factory for creating AWS {@link GlueClient} instances from Gravitino catalog configuration. + * + * <p>Authentication priority: + * + * <ol> + * <li>Static credentials ({@code aws-access-key-id} + {@code aws-secret-access-key}) + * <li>Default credential chain (environment variables, instance profile, container credentials) + * </ol> + * + * <p>An optional endpoint override ({@code aws-glue-endpoint}) enables connectivity to VPC + * endpoints and LocalStack for integration testing. + */ +public final class GlueClientProvider { + + private GlueClientProvider() {} + + /** + * Builds a {@link GlueClient} from the given catalog configuration map. + * + * @param config Catalog configuration properties. + * @return A configured and ready-to-use {@link GlueClient}. + * @throws IllegalArgumentException if {@code aws-region} is missing or blank, if only one of the + * credential keys is provided, or if {@code aws-glue-endpoint} is not a valid URI. + */ + public static GlueClient buildClient(Map<String, String> config) { + String region = config.get(GlueConstants.AWS_REGION); + Preconditions.checkArgument( + !Strings.isNullOrEmpty(region) && !region.isBlank(), + "Property '%s' is required to create a Glue client", + GlueConstants.AWS_REGION); + + GlueClientBuilder builder = GlueClient.builder().region(Region.of(region)); + + // Static credentials take priority over the default credential chain. + // Both keys must be provided together — a partial pair is always a misconfiguration. + String accessKey = config.get(GlueConstants.AWS_ACCESS_KEY_ID); + String secretKey = config.get(GlueConstants.AWS_SECRET_ACCESS_KEY); + boolean hasAccessKey = !Strings.isNullOrEmpty(accessKey) && !accessKey.isBlank(); + boolean hasSecretKey = !Strings.isNullOrEmpty(secretKey) && !secretKey.isBlank(); Review Comment: The same 'non-null/non-empty/non-blank' check is repeated for region/keys/endpoint. Consider extracting a small helper (e.g., `isNotBlank(String)`) to reduce duplication and make the intent consistent across all fields. ########## catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueClientProvider.java: ########## @@ -0,0 +1,104 @@ +/* + * 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.catalog.glue; + +import com.google.common.base.Preconditions; +import com.google.common.base.Strings; +import java.net.URI; +import java.util.Map; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.glue.GlueClient; +import software.amazon.awssdk.services.glue.GlueClientBuilder; + +/** + * Factory for creating AWS {@link GlueClient} instances from Gravitino catalog configuration. + * + * <p>Authentication priority: + * + * <ol> + * <li>Static credentials ({@code aws-access-key-id} + {@code aws-secret-access-key}) + * <li>Default credential chain (environment variables, instance profile, container credentials) + * </ol> + * + * <p>An optional endpoint override ({@code aws-glue-endpoint}) enables connectivity to VPC + * endpoints and LocalStack for integration testing. + */ +public final class GlueClientProvider { + + private GlueClientProvider() {} + + /** + * Builds a {@link GlueClient} from the given catalog configuration map. + * + * @param config Catalog configuration properties. + * @return A configured and ready-to-use {@link GlueClient}. + * @throws IllegalArgumentException if {@code aws-region} is missing or blank, if only one of the + * credential keys is provided, or if {@code aws-glue-endpoint} is not a valid URI. + */ + public static GlueClient buildClient(Map<String, String> config) { + String region = config.get(GlueConstants.AWS_REGION); + Preconditions.checkArgument( + !Strings.isNullOrEmpty(region) && !region.isBlank(), + "Property '%s' is required to create a Glue client", + GlueConstants.AWS_REGION); + + GlueClientBuilder builder = GlueClient.builder().region(Region.of(region)); + + // Static credentials take priority over the default credential chain. + // Both keys must be provided together — a partial pair is always a misconfiguration. + String accessKey = config.get(GlueConstants.AWS_ACCESS_KEY_ID); + String secretKey = config.get(GlueConstants.AWS_SECRET_ACCESS_KEY); + boolean hasAccessKey = !Strings.isNullOrEmpty(accessKey) && !accessKey.isBlank(); + boolean hasSecretKey = !Strings.isNullOrEmpty(secretKey) && !secretKey.isBlank(); + Preconditions.checkArgument( + hasAccessKey == hasSecretKey, + "Incomplete static credentials: '%s' requires '%s'. " + + "Either provide both keys for static authentication, " + + "or omit both to use the default credential chain.", + hasAccessKey ? GlueConstants.AWS_ACCESS_KEY_ID : GlueConstants.AWS_SECRET_ACCESS_KEY, + hasAccessKey ? GlueConstants.AWS_SECRET_ACCESS_KEY : GlueConstants.AWS_ACCESS_KEY_ID); Review Comment: The error message can be confusing because the '%s requires %s' wording is symmetric but the placeholder order flips depending on which key is present, which may read oddly (e.g., secret 'requires' access). Prefer an explicit message that always names both keys and clearly states which one is missing (or simply: \"Both 'aws-access-key-id' and 'aws-secret-access-key' must be set together\"). ```suggestion "Both '%s' and '%s' must be set together. " + "Either provide both keys for static authentication, " + "or omit both to use the default credential chain.", GlueConstants.AWS_ACCESS_KEY_ID, GlueConstants.AWS_SECRET_ACCESS_KEY); ``` -- 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]
