Copilot commented on code in PR #11735: URL: https://github.com/apache/gravitino/pull/11735#discussion_r3433713290
########## bundles/aws/src/main/java/org/apache/gravitino/s3/credential/webidentity/WebIdentityTokenSources.java: ########## @@ -0,0 +1,94 @@ +/* + * 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.s3.credential.webidentity; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.ServiceLoader; +import org.apache.commons.lang3.StringUtils; + +/** Factory for resolving and initializing a {@link WebIdentityTokenSource} from properties. */ +public final class WebIdentityTokenSources { + + /** + * Resolve the {@link WebIdentityTokenSource} configured by {@code s3-web-identity-token-source} + * (defaults to {@code file}) and initialize it with the given properties. + * + * <p>Sources are discovered through {@link ServiceLoader}, so additional implementations can be + * registered by listing them under {@code + * META-INF/services/org.apache.gravitino.s3.credential.webidentity.WebIdentityTokenSource}. + */ + public static WebIdentityTokenSource create(Map<String, String> properties) { + return create(properties, ServiceLoader.load(WebIdentityTokenSource.class)); + } + + /** + * Package-private overload that takes the candidate sources directly, enabling tests to exercise + * the resolution logic (in particular duplicate-name detection) without touching the {@link + * ServiceLoader} registry. + */ + static WebIdentityTokenSource create( + Map<String, String> properties, Iterable<WebIdentityTokenSource> candidates) { + String type = properties.getOrDefault(WebIdentityTokenSourceConfig.SOURCE, ""); + if (StringUtils.isBlank(type)) { + type = WebIdentityTokenSourceConfig.DEFAULT_SOURCE; + } + + List<String> available = new ArrayList<>(); + List<WebIdentityTokenSource> matched = new ArrayList<>(); + for (WebIdentityTokenSource candidate : candidates) { + String candidateName = candidate.name(); + if (StringUtils.isBlank(candidateName)) { + throw new IllegalStateException( + "WebIdentity token source " + + candidate.getClass().getName() + + " must return a non-blank name()."); + } + available.add(candidateName); + if (type.equalsIgnoreCase(candidateName)) { + matched.add(candidate); + } + } Review Comment: `WebIdentityTokenSource#name()` is documented as needing to be unique across all implementations, but the factory only detects duplicates among sources that match the selected type. This can leave duplicate names on the classpath undetected until a different type is selected, and makes the SPI contract unenforced. ########## bundles/aws/src/main/java/org/apache/gravitino/s3/credential/AwsIrsaCredentialGenerator.java: ########## @@ -77,27 +73,7 @@ public void initialize(Map<String, String> properties) { @Override public AwsIrsaCredential generate(CredentialContext context) { if (!(context instanceof PathBasedCredentialContext)) { - // Fallback to original behavior for non-path-based contexts - AwsCredentials creds = baseCredentialsProvider.resolveCredentials(); - if (creds instanceof AwsSessionCredentials) { - AwsSessionCredentials sessionCreds = (AwsSessionCredentials) creds; - if (!sessionCreds.expirationTime().isPresent()) { - throw new IllegalStateException( - "AWS IRSA session credentials must include an expiration time for vended credential" - + " refresh"); - } - long expiration = sessionCreds.expirationTime().get().toEpochMilli(); - return new AwsIrsaCredential( - sessionCreds.accessKeyId(), - sessionCreds.secretAccessKey(), - sessionCreds.sessionToken(), - expiration); - } else { - throw new IllegalStateException( - "AWS IRSA credentials must be of type AwsSessionCredentials. " - + "Check your EKS/IRSA configuration. Got: " - + creds.getClass().getName()); - } + return toAwsIrsaCredential(createCredentials(context.getUserName(), null)); } Review Comment: In basic (non-path-based) mode, `generate()` now calls STS `AssumeRoleWithWebIdentity` on every request (`createCredentials(..., null)`), whereas the previous `WebIdentityTokenFileCredentialsProvider` path could cache/refresh session credentials. This can add per-request latency and increase STS API call volume/throttling risk for catalog-credential requests. ########## bundles/aws/src/main/java/org/apache/gravitino/s3/credential/AwsIrsaCredentialGenerator.java: ########## @@ -107,36 +83,36 @@ public AwsIrsaCredential generate(CredentialContext context) { pathBasedCredentialContext.getReadPaths(), pathBasedCredentialContext.getWritePaths(), pathBasedCredentialContext.getUserName()); - return new AwsIrsaCredential( - s3Token.accessKeyId(), - s3Token.secretAccessKey(), - s3Token.sessionToken(), - s3Token.expiration().toEpochMilli()); + return toAwsIrsaCredential(s3Token); } private Credentials createCredentialsWithSessionPolicy( Set<String> readLocations, Set<String> writeLocations, String userName) { validateInputParameters(readLocations, writeLocations, userName); IamPolicy sessionPolicy = createSessionPolicy(readLocations, writeLocations, region); - String webIdentityTokenFile = getValidatedWebIdentityTokenFile(); - String effectiveRoleArn = getValidatedRoleArn(roleArn); + return createCredentials(userName, sessionPolicy); + } + private Credentials createCredentials(String userName, @Nullable IamPolicy sessionPolicy) { + String effectiveRoleArn = getValidatedRoleArn(roleArn); try { - String tokenContent = - new String(Files.readAllBytes(Paths.get(webIdentityTokenFile)), StandardCharsets.UTF_8); - if (StringUtils.isBlank(tokenContent)) { - throw new IllegalStateException( - "Web identity token file is empty: " + webIdentityTokenFile); - } - - return assumeRoleWithSessionPolicy(effectiveRoleArn, userName, tokenContent, sessionPolicy); + String tokenContent = tokenSource.getToken(); + return assumeRoleWithWebIdentity(effectiveRoleArn, userName, tokenContent, sessionPolicy); } catch (Exception e) { throw new RuntimeException( - "Failed to create credentials with session policy for user: " + userName, e); + "Failed to create WebIdentity credentials for user: " + userName, e); } Review Comment: `createCredentials` catches all `Exception` and rethrows `RuntimeException`, which discards the original exception type even though `CredentialGenerator.generate()` already allows throwing exceptions. Letting the original exception propagate (or throwing a more specific unchecked exception) improves debuggability and avoids masking `IllegalArgumentException`/`IllegalStateException` from validation. -- 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]
