jerryshao commented on code in PR #13370:
URL: https://github.com/apache/gravitino/pull/13370#discussion_r4060174360
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -103,6 +174,9 @@ static RuntimeException toSchemaException(GlueException e,
String context) {
* @return a Gravitino or standard Java runtime exception
*/
static RuntimeException toTableException(GlueException e, String context) {
+ if (isAuthenticationFailure(e)) {
+ return toAuthenticationException(e, context);
+ }
Review Comment:
[Nit] Partition operations do not reach this branch, so they keep emitting
raw AWS auth errors.
`GlueTableOperations` never calls `GlueExceptionConverter`; every one of its
Glue calls ends in `ExceptionMessages.wrap(...)` instead
(GlueTableOperations.java:102, 124, 144, 184, 213). An
`UnrecognizedClientException` raised while listing or adding partitions
therefore still surfaces as a generic wrapped AWS message with no mention of
`aws-access-key-id` / `aws-secret-access-key`, even though the PR description
says the actionable diagnostics are applied to "schema and table operations".
Routing those catch blocks through `toTableException` would close the gap;
if partitions are intentionally out of scope for this PR, worth saying so in
the description so it is not mistaken for full coverage.
Verified by: grepped the whole `catalog-glue` main source set for
`GlueExceptionConverter.` - the only call sites are in `GlueCatalogOperations`
and `GlueClientProvider`; read GlueTableOperations.java:95-215.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -39,6 +42,20 @@ final class GlueExceptionConverter {
private static final String NO_CREDENTIALS_MARKER =
"Unable to load credentials from any of the providers";
+ private static final Set<String> AUTHENTICATION_ERROR_CODES =
+ Set.of(
+ "AuthFailure",
+ "ExpiredToken",
+ "ExpiredTokenException",
+ "IncompleteSignature",
+ "InvalidAccessKeyId",
+ "InvalidClientTokenId",
+ "InvalidSignatureException",
+ "RequestExpired",
+ "SignatureDoesNotMatch",
+ "TokenRefreshRequired",
Review Comment:
[Nit] Four of these codes point the operator at properties that cannot be
the cause.
`ExpiredToken`, `ExpiredTokenException`, `TokenRefreshRequired` and
`RequestExpired` all land in the message "Verify the 'aws-access-key-id' and
'aws-secret-access-key' catalog properties" (lines 127-133 and 195-206). But
the Glue connector has no session-token property at all - `GlueConstants`
defines only `aws-region`, `aws-glue-catalog-id`, `aws-access-key-id`,
`aws-secret-access-key` and `aws-glue-endpoint`
(catalogs/catalog-common/.../GlueConstants.java:29-47) - so an expired token
can only come from the default chain (assumed role, web identity, instance
profile), and `RequestExpired` is usually clock skew on the Gravitino host, not
a credential problem at all.
The trailing "or the configured default AWS credential source" softens this,
but the operator's actual next step (refresh the role credentials, or check
host clock skew) is never stated. Consider splitting the expiry/skew codes into
their own message, or dropping `RequestExpired` from the set so it falls
through to the generic branch.
Verified by: read GlueExceptionConverter.java:45-57, 116-141, 195-206 and
catalogs/catalog-common/src/main/java/org/apache/gravitino/catalog/glue/GlueConstants.java:25-47.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -72,6 +89,57 @@ static RuntimeException
toCredentialException(SdkClientException e, String conte
e);
}
+ /**
+ * Whether AWS Glue rejected credentials that were successfully resolved by
the configured
+ * provider. Static credential providers can return any nonblank access-key
pair locally, so only
+ * an AWS service response can establish whether that pair is authentic.
+ *
+ * @param e the service exception raised by AWS Glue
+ * @return true if AWS classified the failure as an authentication error
+ */
+ static boolean isAuthenticationFailure(GlueException e) {
+ AwsErrorDetails details = e.awsErrorDetails();
+ return details != null
+ && StringUtils.isNotBlank(details.errorCode())
+ && AUTHENTICATION_ERROR_CODES.contains(details.errorCode());
+ }
+
+ /**
+ * Converts an AWS Glue SDK failure raised by a connection probe into a
connection error. Known
+ * credential failures name the connector properties that an operator can
correct; authorization
+ * and transport failures retain the AWS or SDK detail without claiming the
credentials are
+ * invalid.
+ *
+ * @param e the SDK failure raised by the connection probe
+ * @return an actionable connection failure
+ */
+ static ConnectionFailedException toConnectionException(SdkException e) {
+ if (e instanceof SdkClientException &&
isCredentialFailure((SdkClientException) e)) {
+ return new ConnectionFailedException(
+ e,
+ "Failed to authenticate with AWS Glue. No usable AWS credentials
were found. Set both "
+ + "'%s' and '%s' catalog properties, or ensure the default AWS
credential chain can "
+ + "resolve credentials.",
+ GlueConstants.AWS_ACCESS_KEY_ID,
+ GlueConstants.AWS_SECRET_ACCESS_KEY);
+ }
+ if (e instanceof GlueException && isAuthenticationFailure((GlueException)
e)) {
+ return new ConnectionFailedException(
+ e,
+ "AWS Glue rejected the configured credentials. Verify the '%s' and
'%s' catalog "
+ + "properties, or the configured default AWS credential source.
AWS error: %s",
+ GlueConstants.AWS_ACCESS_KEY_ID,
+ GlueConstants.AWS_SECRET_ACCESS_KEY,
+ awsErrorDetail((GlueException) e));
+ }
+
+ String detail =
+ e instanceof GlueException
+ ? awsErrorDetail((GlueException) e)
+ : StringUtils.defaultIfBlank(e.getMessage(),
e.getClass().getSimpleName());
+ return new ConnectionFailedException(e, "Failed to connect to AWS Glue:
%s", detail);
Review Comment:
[Nit] This fallback loses the "while resolving credentials" context that the
removed code carried.
When `validateCredentials` (GlueClientProvider.java:110-116) catches a
non-marker `SdkClientException` - an IMDS timeout, a proxy reset - it now
reaches this line and the operator reads "Failed to connect to AWS Glue:
connection refused". The code this PR removes said "Failed to resolve AWS
credentials for the Glue catalog: ...", which told them the failure happened
during credential resolution rather than during a Glue API call. Those are very
different things to go debug, and the new wording points at the wrong one.
`testValidateCredentialsWithNonCredentialFailureDoesNotClaimNoCredentials`
(TestGlueClientProvider.java:86-101) only asserts that the message does not
claim "No usable AWS credentials", so it passes either way. Passing a context
string into `toConnectionException`, or giving `validateCredentials` its own
message for the non-marker case, would keep both properties.
Verified by: compared the removed lines in this PR's diff of
GlueClientProvider.java with the current GlueExceptionConverter.java:136-140;
read TestGlueClientProvider.java:86-101.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueClientProvider.java:
##########
@@ -97,31 +99,19 @@ public static GlueClient buildClient(Map<String, String>
config) {
}
/**
- * Eagerly resolves {@code credentialsProvider} to confirm a usable
credential source exists,
- * instead of leaving resolution to the first real Glue API call. Without
this check, a catalog
- * created with no static credentials and no usable default-chain source
(env vars, instance
- * profile, etc.) is stored successfully and then fails on every operation
with a raw AWS SDK
- * error that never mentions this connector's own credential properties.
+ * Eagerly resolves {@code credentialsProvider} when Glue operations are
initialized, instead of
+ * leaving resolution to the first real Glue API call. This makes an
explicit connection test or
+ * the first operation fail with an actionable connection error when no
credential source is
+ * available. It does not authenticate static credentials; only an AWS API
request can do that.
*
- * @throws IllegalArgumentException if no credentials can be resolved
+ * @throws ConnectionFailedException if no credentials can be resolved
*/
@VisibleForTesting
static void validateCredentials(AwsCredentialsProvider credentialsProvider) {
try {
credentialsProvider.resolveCredentials();
} catch (SdkClientException e) {
- if (!GlueExceptionConverter.isCredentialFailure(e)) {
- throw new IllegalArgumentException(
- "Failed to resolve AWS credentials for the Glue catalog: " +
e.getMessage(), e);
- }
- throw new IllegalArgumentException(
- String.format(
- "No usable AWS credentials found for the Glue catalog. Set both
'%s' and '%s' "
- + "catalog properties for static authentication, or ensure
the default AWS "
- + "credential chain (environment variables, instance
profile, web identity "
- + "token, etc.) can resolve credentials.",
- GlueConstants.AWS_ACCESS_KEY_ID,
GlueConstants.AWS_SECRET_ACCESS_KEY),
- e);
+ throw GlueExceptionConverter.toConnectionException(e);
Review Comment:
[Important] This converts "no AWS credentials anywhere" from a caller error
into a downstream-dependency error, on every catalog operation and not only on
the connection test.
`validateCredentials` is called from `buildClient`
(GlueClientProvider.java:89), which runs inside
`GlueCatalogOperations.initialize` (GlueCatalogOperations.java:141).
`GlueCatalog` does not override `shouldValidateOnCreate()` (default `false`,
BaseCatalog.java:226) and `GlueCatalog.catalogPropertiesMetadata()` returns a
static instance, so this never runs at catalog creation - it runs lazily on the
*first real operation*, through `BaseCatalog.ops()` (BaseCatalog.java:197-219)
and `CatalogWrapper.doWithSchemaOps`/`doWithCatalogOps`
(CatalogManager.java:297, 399), which use the `throws Exception` overload of
`withClassLoader` and therefore propagate the exception unchanged.
Before this PR that exception was `IllegalArgumentException`, which
`ExceptionHandlers` maps to `illegalArguments` (HTTP 400). After this PR it is
`ConnectionFailedException`, mapped to `Utils.connectionFailed` ->
`CONNECTION_FAILED_CODE = 1007` (ErrorConstants.java:46), which the base
handler documents as "502 Bad Gateway ... a downstream-dependency failure, not
an internal Gravitino error" (ExceptionHandlers.java:1163-1168; catalog path at
ExceptionHandlers.java:424-425). So a catalog created with no static
credentials on a host where the default chain resolves nothing - a
configuration mistake the operator can fix - is now reported as an AWS outage.
That misroutes operators, and it changes the exception Java-client callers
catch.
For the `testConnection` path the new type is an improvement and I would
keep it (handleTestConnectionException, ExceptionHandlers.java:418-421,
distinguishes both). The narrower fix is to keep `IllegalArgumentException`
when `GlueExceptionConverter.isCredentialFailure(e)` is true here in
`validateCredentials` (chain exhausted = misconfiguration) and let only the
transport/IMDS case become `ConnectionFailedException`; or, if the
reclassification is deliberate, say so in the PR's "user-facing change" section
- it currently states only that catalog creation is unchanged, which is true,
while the first-operation error code change goes unmentioned - and pin it with
a test.
Verified by: read GlueClientProvider.java:63-116,
GlueCatalogOperations.java:139-141, BaseCatalog.java:197-226,
CatalogManager.java:290-410 and 1817-1845, ExceptionHandlers.java:210-225,
418-432, 1160-1170, ErrorResponse.java:145-162, ErrorConstants.java:43-49; the
removed `IllegalArgumentException` is visible in this PR's own diff.
--
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]
