From 79ed99f6bb27c99d9ddfb7159a4cce887c78ceee Mon Sep 17 00:00:00 2001 From: Talat Uyarer Date: Sun, 1 Jun 2025 22:18:48 -0700 Subject: [PATCH 1/6] Initial Version of GCPAuthManager --- build.gradle | 1 + .../org/apache/iceberg/gcp/GCPProperties.java | 3 + .../iceberg/gcp/auth/GoogleAuthManager.java | 160 ++++++++++++ .../iceberg/gcp/auth/GoogleAuthSession.java | 96 +++++++ .../gcp/auth/TestGoogleAuthManager.java | 234 ++++++++++++++++++ .../gcp/auth/TestGoogleAuthSession.java | 189 ++++++++++++++ 6 files changed, 683 insertions(+) create mode 100644 gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java create mode 100644 gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthSession.java create mode 100644 gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthManager.java create mode 100644 gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthSession.java diff --git a/build.gradle b/build.gradle index ed0809c2f66c..b6026af67f40 100644 --- a/build.gradle +++ b/build.gradle @@ -718,6 +718,7 @@ project(':iceberg-gcp') { testImplementation libs.esotericsoftware.kryo testImplementation libs.mockserver.netty testImplementation libs.mockserver.client.java + testImplementation libs.mockito.junit.jupiter } } diff --git a/gcp/src/main/java/org/apache/iceberg/gcp/GCPProperties.java b/gcp/src/main/java/org/apache/iceberg/gcp/GCPProperties.java index d91601125c74..417f4db38786 100644 --- a/gcp/src/main/java/org/apache/iceberg/gcp/GCPProperties.java +++ b/gcp/src/main/java/org/apache/iceberg/gcp/GCPProperties.java @@ -62,6 +62,9 @@ public class GCPProperties implements Serializable { */ public static final int GCS_DELETE_BATCH_SIZE_DEFAULT = 50; + public static final String GCP_CREDENTIALS_PATH_PROPERTY = "gcp.auth.credentials-path"; + public static final String GCP_SCOPES_PROPERTY = "gcp.auth.scopes"; + private final Map allProperties; private String projectId; diff --git a/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java new file mode 100644 index 000000000000..916e084af1cf --- /dev/null +++ b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java @@ -0,0 +1,160 @@ +/* + * 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.iceberg.gcp.auth; + +import com.google.auth.oauth2.GoogleCredentials; +import java.io.FileInputStream; +import java.io.IOException; +import java.io.UncheckedIOException; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import org.apache.iceberg.catalog.SessionCatalog; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.gcp.GCPProperties; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.apache.iceberg.rest.RESTClient; +import org.apache.iceberg.rest.auth.AuthManager; +import org.apache.iceberg.rest.auth.AuthSession; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * An authentication manager that uses Google Credentials (typically Application Default + * Credentials) to create {@link GoogleAuthSession} instances. + * + *

This manager can be configured with properties such as: + * + *

+ */ +public class GoogleAuthManager implements AuthManager { + private static final Logger LOG = LoggerFactory.getLogger(GoogleAuthManager.class); + public static final String DEFAULT_SCOPES = "https://www.googleapis.com/auth/cloud-platform"; + private final String name; + + private GoogleCredentials credentials; + private boolean initialized = false; + + public GoogleAuthManager(String managerName) { + this.name = managerName; + } + + public String getName() { + return name; + } + + private void initialize(Map properties) { + if (initialized) { + return; + } + + String credentialsPath = properties.get(GCPProperties.GCP_CREDENTIALS_PATH_PROPERTY); + String scopesString = + properties.getOrDefault(GCPProperties.GCP_SCOPES_PROPERTY, DEFAULT_SCOPES); + List scopes = ImmutableList.copyOf(scopesString.split(",")); + + try { + if (credentialsPath != null && !credentialsPath.isEmpty()) { + LOG.info("Using Google credentials from path: {}", credentialsPath); + try (FileInputStream credentialsStream = new FileInputStream(credentialsPath)) { + this.credentials = GoogleCredentials.fromStream(credentialsStream).createScoped(scopes); + } + } else { + LOG.info("Using Application Default Credentials with scopes: {}", scopesString); + this.credentials = GoogleCredentials.getApplicationDefault().createScoped(scopes); + } + } catch (IOException e) { + throw new UncheckedIOException("Failed to load Google credentials", e); + } + this.initialized = true; + } + + /** + * Initializes and returns a short-lived session, typically for fetching configuration. This + * implementation reuses the long-lived catalog session logic. + */ + @Override + public AuthSession initSession(RESTClient initClient, Map properties) { + return catalogSession(initClient, properties); + } + + /** + * Returns a long-lived session tied to the catalog's lifecycle. This session uses Google + * Application Default Credentials or a specified service account. + * + * @param sharedClient The long-lived RESTClient (not used by this implementation for credential + * fetching). + * @param properties Configuration properties for the auth manager. + * @return A {@link GoogleAuthSession}. + * @throws UncheckedIOException if credential loading fails. + */ + @Override + public AuthSession catalogSession(RESTClient sharedClient, Map properties) { + initialize(properties); + Preconditions.checkState( + credentials != null, "GoogleAuthManager not initialized or failed to load credentials"); + return new GoogleAuthSession(credentials); + } + + /** Returns a session for a specific context. Defaults to the catalog session. */ + @Override + public AuthSession contextualSession(SessionCatalog.SessionContext context, AuthSession parent) { + // For GCP, tokens are typically not context-specific in this manner. + // Re-using the parent (which should be a GoogleAuthSession) is appropriate. + // Or, if properties for a specific context were available, a new GoogleAuthSession could be + // derived. + if (parent instanceof GoogleAuthSession) { + return parent; + } + // Fallback to a new catalog-level session if the parent is not a GoogleAuthSession for some + // reason. + // This would require properties to be available or a default initialization. + LOG.warn( + "Parent session is not a GoogleAuthSession. Creating a new default catalog session. This might not be intended."); + return catalogSession( + null, Collections.emptyMap()); // Assuming default ADC without specific props + } + + /** Returns a session for a specific table or view. Defaults to the catalog session. */ + @Override + public AuthSession tableSession( + TableIdentifier table, Map properties, AuthSession parent) { + // Similar to contextualSession, GCP tokens are generally not table-specific. + if (parent instanceof GoogleAuthSession) { + return parent; + } + LOG.warn( + "Parent session for table {} is not a GoogleAuthSession. Creating a new default catalog session.", + table); + return catalogSession(null, properties); + } + + /** Closes the manager. This is a no-op for GoogleAuthManager. */ + @Override + public void close() { + // No-op. Credentials lifecycle is managed by the GoogleCredentials object itself or by the + // application. + } +} diff --git a/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthSession.java b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthSession.java new file mode 100644 index 000000000000..62c524aeb55b --- /dev/null +++ b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthSession.java @@ -0,0 +1,96 @@ +/* + * 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.iceberg.gcp.auth; + +import com.google.auth.oauth2.AccessToken; +import com.google.auth.oauth2.GoogleCredentials; +import java.io.IOException; +import java.io.UncheckedIOException; +import org.apache.iceberg.rest.HTTPHeaders; +import org.apache.iceberg.rest.HTTPRequest; +import org.apache.iceberg.rest.ImmutableHTTPRequest; +import org.apache.iceberg.rest.auth.AuthSession; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * An authentication session that uses Google Credentials (typically Application Default + * Credentials) to obtain an OAuth2 access token and add it to HTTP requests. + */ +public class GoogleAuthSession implements AuthSession { + private static final Logger LOG = LoggerFactory.getLogger(GoogleAuthSession.class); + private final GoogleCredentials credentials; + + /** + * Constructs a GoogleAuthSession with the provided GoogleCredentials. + * + * @param credentials The GoogleCredentials to use for authentication. + */ + public GoogleAuthSession(GoogleCredentials credentials) { + this.credentials = credentials; + } + + /** + * Authenticates the given HTTP request by adding an "Authorization: Bearer token" header. The + * access token is obtained from the GoogleCredentials. + * + * @param request The HTTPRequest to authenticate. + * @return A new HTTPRequest with the added Authorization header. + * @throws UncheckedIOException if an IOException occurs while refreshing the access token. + */ + @Override + public HTTPRequest authenticate(HTTPRequest request) { + try { + // Ensure the credentials have a valid token, refreshing if necessary. + // refreshIfExpired() returns true if the token was refreshed, false otherwise. + // In either case, getAccessToken() after this call should provide a usable token. + credentials.refreshIfExpired(); + AccessToken token = credentials.getAccessToken(); + + if (token != null && token.getTokenValue() != null) { + HTTPHeaders newHeaders = + request + .headers() + .putIfAbsent( + HTTPHeaders.of( + HTTPHeaders.HTTPHeader.of( + "Authorization", "Bearer " + token.getTokenValue()))); + return newHeaders.equals(request.headers()) + ? request + : ImmutableHTTPRequest.builder().from(request).headers(newHeaders).build(); + } else { + LOG.warn( + "Failed to obtain Google access token. Request will be sent without Authorization header."); + return request; + } + } catch (IOException e) { + LOG.error("IOException while trying to refresh Google access token", e); + throw new UncheckedIOException("Failed to refresh Google access token", e); + } + } + + /** + * Closes the session. This is a no-op for GoogleAuthSession as the lifecycle of GoogleCredentials + * is not managed by this session. + */ + @Override + public void close() { + // No-op + } +} diff --git a/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthManager.java b/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthManager.java new file mode 100644 index 000000000000..60503e776e41 --- /dev/null +++ b/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthManager.java @@ -0,0 +1,234 @@ +/* + * 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.iceberg.gcp.auth; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyList; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.google.auth.oauth2.GoogleCredentials; +import java.io.File; +import java.io.FileInputStream; +import java.io.IOException; +import java.io.UncheckedIOException; +import java.nio.file.Files; +import java.util.Collections; +import java.util.Map; +import org.apache.iceberg.catalog.SessionCatalog; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.gcp.GCPProperties; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.apache.iceberg.rest.RESTClient; +import org.apache.iceberg.rest.auth.AuthSession; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.io.TempDir; +import org.mockito.Mock; +import org.mockito.MockedStatic; +import org.mockito.Mockito; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +public class TestGoogleAuthManager { + + private static final String MANAGER_NAME = "testManager"; + @Mock private RESTClient mockRestClient; + @Mock private GoogleCredentials mockCredentialsInstance; + @Mock private GoogleCredentials mockCredsFromFile; + + private GoogleAuthManager authManager; + private MockedStatic mockedStaticCredentials; + + @TempDir File tempDir; + private File fakeCredentialFile; + + @BeforeEach + public void beforeEach() throws IOException { + authManager = new GoogleAuthManager(MANAGER_NAME); + mockedStaticCredentials = Mockito.mockStatic(GoogleCredentials.class); + fakeCredentialFile = new File(tempDir, "fake-creds.json"); + Files.write(fakeCredentialFile.toPath(), "{\"type\": \"service_account\"}".getBytes()); + } + + @AfterEach + public void afterEach() { + mockedStaticCredentials.close(); + } + + @Test + public void providesCorrectManagerName() { + assertThat(authManager.getName()).isEqualTo(MANAGER_NAME); + } + + @Test + public void buildsCatalogSessionFromCredentialsFile() throws IOException { + String customScopes = "scope1,scope2"; + Map properties = + ImmutableMap.of( + GCPProperties.GCP_CREDENTIALS_PATH_PROPERTY, + fakeCredentialFile.getAbsolutePath(), + GCPProperties.GCP_SCOPES_PROPERTY, + customScopes); + + mockedStaticCredentials + .when(() -> GoogleCredentials.fromStream(any(FileInputStream.class))) + .thenReturn(mockCredsFromFile); + when(mockCredsFromFile.createScoped(anyList())).thenReturn(mockCredentialsInstance); + + AuthSession session = authManager.catalogSession(mockRestClient, properties); + + assertThat(session).isInstanceOf(GoogleAuthSession.class); + verify(mockCredsFromFile).createScoped(ImmutableList.of("scope1", "scope2")); + } + + @Test + public void throwsUncheckedIOExceptionOnCredentialsFileError() throws IOException { + Map properties = + ImmutableMap.of( + GCPProperties.GCP_CREDENTIALS_PATH_PROPERTY, fakeCredentialFile.getAbsolutePath()); + + mockedStaticCredentials + .when(() -> GoogleCredentials.fromStream(any(FileInputStream.class))) + .thenThrow(new IOException("Simulated stream loading failure")); + + assertThatThrownBy(() -> authManager.catalogSession(mockRestClient, properties)) + .isInstanceOf(UncheckedIOException.class) + .hasMessageContaining("Failed to load Google credentials"); + } + + @Test + public void buildsCatalogSessionUsingADC() throws IOException { + Map properties = Collections.emptyMap(); + + mockedStaticCredentials + .when(GoogleCredentials::getApplicationDefault) + .thenReturn(mockCredsFromFile); + + when(mockCredsFromFile.createScoped(anyList())).thenReturn(mockCredentialsInstance); + + AuthSession session = authManager.catalogSession(mockRestClient, properties); + + assertThat(session).isInstanceOf(GoogleAuthSession.class); + mockedStaticCredentials.verify(GoogleCredentials::getApplicationDefault, times(1)); + } + + @Test + public void throwsUncheckedIOExceptionOnADCError() throws IOException { + Map properties = Collections.emptyMap(); + + mockedStaticCredentials + .when(GoogleCredentials::getApplicationDefault) + .thenThrow(new IOException("ADC unavailable")); + + assertThatThrownBy(() -> authManager.catalogSession(mockRestClient, properties)) + .isInstanceOf(UncheckedIOException.class) + .hasMessageContaining("Failed to load Google credentials"); + } + + @Test + public void initializationOccursOnlyOnce() throws IOException { + Map properties = Collections.emptyMap(); + mockedStaticCredentials + .when(GoogleCredentials::getApplicationDefault) + .thenReturn(mockCredsFromFile); + + when(mockCredsFromFile.createScoped(anyList())).thenReturn(mockCredentialsInstance); + + authManager.catalogSession(mockRestClient, properties); + authManager.catalogSession(mockRestClient, properties); + + mockedStaticCredentials.verify(GoogleCredentials::getApplicationDefault, times(1)); + } + + @Test + public void initSessionDelegatesToCatalogSession() { + GoogleAuthManager spyManager = spy(authManager); + AuthSession mockSession = mock(GoogleAuthSession.class); + Map props = Collections.emptyMap(); + + doReturn(mockSession).when(spyManager).catalogSession(mockRestClient, props); + + AuthSession resultSession = spyManager.initSession(mockRestClient, props); + assertThat(resultSession).isSameAs(mockSession); + verify(spyManager).catalogSession(mockRestClient, props); + } + + @Test + public void contextualSessionReusesGoogleAuthParent() { + AuthSession parentSession = mock(GoogleAuthSession.class); + AuthSession resultSession = + authManager.contextualSession(mock(SessionCatalog.SessionContext.class), parentSession); + assertThat(resultSession).isSameAs(parentSession); + } + + @Test + public void contextualSessionFallsBackToNewSessionWithNonGoogleParent() throws IOException { + AuthSession parentSession = mock(AuthSession.class); + mockedStaticCredentials + .when(GoogleCredentials::getApplicationDefault) + .thenReturn(mockCredsFromFile); + + when(mockCredsFromFile.createScoped(anyList())).thenReturn(mockCredentialsInstance); + + AuthSession resultSession = + authManager.contextualSession(mock(SessionCatalog.SessionContext.class), parentSession); + + assertThat(resultSession).isInstanceOf(GoogleAuthSession.class); + mockedStaticCredentials.verify(GoogleCredentials::getApplicationDefault, times(1)); + } + + @Test + public void tableSessionReusesGoogleAuthParent() { + AuthSession parentSession = mock(GoogleAuthSession.class); + AuthSession resultSession = + authManager.tableSession( + mock(TableIdentifier.class), Collections.emptyMap(), parentSession); + assertThat(resultSession).isSameAs(parentSession); + } + + @Test + public void tableSessionCreatesNewSessionWithNonGoogleParent() throws IOException { + AuthSession parentSession = mock(AuthSession.class); + Map properties = + ImmutableMap.of( + GCPProperties.GCP_CREDENTIALS_PATH_PROPERTY, fakeCredentialFile.getAbsolutePath()); + + mockedStaticCredentials + .when(() -> GoogleCredentials.fromStream(any(FileInputStream.class))) + .thenReturn(mockCredsFromFile); + when(mockCredsFromFile.createScoped(anyList())).thenReturn(mockCredentialsInstance); + + AuthSession resultSession = + authManager.tableSession(mock(TableIdentifier.class), properties, parentSession); + + assertThat(resultSession).isInstanceOf(GoogleAuthSession.class); + mockedStaticCredentials.verify( + () -> GoogleCredentials.fromStream(any(FileInputStream.class)), times(1)); + } +} diff --git a/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthSession.java b/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthSession.java new file mode 100644 index 000000000000..f7fad5388b9d --- /dev/null +++ b/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthSession.java @@ -0,0 +1,189 @@ +/* + * 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.iceberg.gcp.auth; + +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.google.auth.oauth2.AccessToken; +import com.google.auth.oauth2.GoogleCredentials; +import java.io.IOException; +import java.io.UncheckedIOException; +import java.net.URI; +import java.net.URISyntaxException; +import java.util.Set; +import org.apache.iceberg.rest.HTTPHeaders; +import org.apache.iceberg.rest.HTTPRequest; +import org.apache.iceberg.rest.ImmutableHTTPRequest; +import org.apache.iceberg.rest.auth.AuthSession; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +public class TestGoogleAuthSession { + + @Mock private GoogleCredentials credentials; + @Mock private AccessToken accessToken; + + private AuthSession session; + private static final String TEST_TOKEN_VALUE = "test-token-12345"; + private URI testBaseUri; + private static final String TEST_RELATIVE_PATH = "v1/some/resource"; + + @BeforeEach + public void beforeEach() throws URISyntaxException { + this.session = new GoogleAuthSession(credentials); + this.testBaseUri = new URI("http://localhost:8080"); + } + + @Test + public void addsAuthHeaderOnSuccessfulTokenFetch() throws IOException { + when(credentials.getAccessToken()).thenReturn(accessToken); + when(accessToken.getTokenValue()).thenReturn(TEST_TOKEN_VALUE); + + HTTPRequest originalRequest = + ImmutableHTTPRequest.builder() + .baseUri(testBaseUri) + .path(TEST_RELATIVE_PATH) + .method(HTTPRequest.HTTPMethod.GET) + .build(); + HTTPRequest authenticatedRequest = session.authenticate(originalRequest); + + verify(credentials).refreshIfExpired(); + verify(credentials).getAccessToken(); + + Assertions.assertNotSame( + originalRequest, authenticatedRequest, "A new request object should be returned"); + + HTTPHeaders headers = authenticatedRequest.headers(); + Assertions.assertEquals(1, headers.entries().size()); + Assertions.assertTrue(headers.contains("Authorization"), "Should contain Authorization header"); + + Set authHeaderEntries = headers.entries("Authorization"); + Assertions.assertEquals(1, authHeaderEntries.size(), "Should have one Authorization header"); + Assertions.assertEquals( + "Bearer " + TEST_TOKEN_VALUE, + authHeaderEntries.iterator().next().value(), + "Authorization header should be set correctly"); + } + + @Test + public void preservesExistingAuthHeader() throws IOException { + String existingAuthHeaderValue = "Bearer existing-bearer-token"; + HTTPHeaders initialHeaders = + HTTPHeaders.of(HTTPHeaders.HTTPHeader.of("Authorization", existingAuthHeaderValue)); + HTTPRequest originalRequest = + ImmutableHTTPRequest.builder() + .baseUri(testBaseUri) + .path(TEST_RELATIVE_PATH) + .method(HTTPRequest.HTTPMethod.GET) + .headers(initialHeaders) + .build(); + + when(credentials.getAccessToken()).thenReturn(accessToken); + when(accessToken.getTokenValue()).thenReturn(TEST_TOKEN_VALUE); + + HTTPRequest authenticatedRequest = session.authenticate(originalRequest); + + verify(credentials).refreshIfExpired(); + + Assertions.assertSame( + originalRequest, + authenticatedRequest, + "Original request object should be returned if header exists"); + + HTTPHeaders resultingHeaders = authenticatedRequest.headers(); + Assertions.assertEquals(1, resultingHeaders.entries().size()); + Assertions.assertEquals( + existingAuthHeaderValue, + resultingHeaders.entries("Authorization").iterator().next().value(), + "Existing Authorization header should be preserved"); + } + + @Test + public void propagatesIOExceptionAsUncheckedOnTokenRefreshFailure() throws IOException { + doThrow(new IOException("Failed to refresh token")).when(credentials).refreshIfExpired(); + + HTTPRequest originalRequest = + ImmutableHTTPRequest.builder() + .baseUri(testBaseUri) + .path(TEST_RELATIVE_PATH) + .method(HTTPRequest.HTTPMethod.GET) + .build(); + + UncheckedIOException thrown = + Assertions.assertThrows( + UncheckedIOException.class, + () -> session.authenticate(originalRequest), + "Should throw UncheckedIOException on refresh failure"); + + Assertions.assertEquals("Failed to refresh Google access token", thrown.getMessage()); + Assertions.assertInstanceOf(IOException.class, thrown.getCause()); + verify(credentials).refreshIfExpired(); + } + + @Test + public void returnsOriginalRequestWhenAccessTokenIsNull() throws IOException { + when(credentials.getAccessToken()).thenReturn(null); + + HTTPRequest originalRequest = + ImmutableHTTPRequest.builder() + .baseUri(testBaseUri) + .path(TEST_RELATIVE_PATH) + .method(HTTPRequest.HTTPMethod.GET) + .build(); + HTTPRequest authenticatedRequest = session.authenticate(originalRequest); + + Assertions.assertSame( + originalRequest, authenticatedRequest, "Request should be unchanged when token is null"); + Assertions.assertTrue( + authenticatedRequest.headers().entries().isEmpty(), "Headers should be empty"); + } + + @Test + public void returnsOriginalRequestWhenTokenValueIsNull() throws IOException { + when(credentials.getAccessToken()).thenReturn(accessToken); + when(accessToken.getTokenValue()).thenReturn(null); + + HTTPRequest originalRequest = + ImmutableHTTPRequest.builder() + .baseUri(testBaseUri) + .path(TEST_RELATIVE_PATH) + .method(HTTPRequest.HTTPMethod.GET) + .build(); + HTTPRequest authenticatedRequest = session.authenticate(originalRequest); + + Assertions.assertSame( + originalRequest, + authenticatedRequest, + "Request should be unchanged when token value is null"); + Assertions.assertTrue( + authenticatedRequest.headers().entries().isEmpty(), "Headers should be empty"); + } + + @Test + public void sessionCloseBehavesAsNoOp() { + Assertions.assertDoesNotThrow(session::close); + } +} From 6aab5f43f7e523066799eef13d56e13dcf35fe2f Mon Sep 17 00:00:00 2001 From: Talat Uyarer Date: Mon, 2 Jun 2025 09:20:03 -0700 Subject: [PATCH 2/6] Addressed @adutya's comment For GCP, tokens are typically not context-specific in this manner. --- .../iceberg/gcp/auth/GoogleAuthManager.java | 22 +------- .../gcp/auth/TestGoogleAuthManager.java | 55 ------------------- 2 files changed, 2 insertions(+), 75 deletions(-) diff --git a/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java index 916e084af1cf..9a655db1522e 100644 --- a/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java +++ b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java @@ -22,7 +22,6 @@ import java.io.FileInputStream; import java.io.IOException; import java.io.UncheckedIOException; -import java.util.Collections; import java.util.List; import java.util.Map; import org.apache.iceberg.catalog.SessionCatalog; @@ -123,18 +122,7 @@ public AuthSession catalogSession(RESTClient sharedClient, Map p public AuthSession contextualSession(SessionCatalog.SessionContext context, AuthSession parent) { // For GCP, tokens are typically not context-specific in this manner. // Re-using the parent (which should be a GoogleAuthSession) is appropriate. - // Or, if properties for a specific context were available, a new GoogleAuthSession could be - // derived. - if (parent instanceof GoogleAuthSession) { - return parent; - } - // Fallback to a new catalog-level session if the parent is not a GoogleAuthSession for some - // reason. - // This would require properties to be available or a default initialization. - LOG.warn( - "Parent session is not a GoogleAuthSession. Creating a new default catalog session. This might not be intended."); - return catalogSession( - null, Collections.emptyMap()); // Assuming default ADC without specific props + return parent; } /** Returns a session for a specific table or view. Defaults to the catalog session. */ @@ -142,13 +130,7 @@ public AuthSession contextualSession(SessionCatalog.SessionContext context, Auth public AuthSession tableSession( TableIdentifier table, Map properties, AuthSession parent) { // Similar to contextualSession, GCP tokens are generally not table-specific. - if (parent instanceof GoogleAuthSession) { - return parent; - } - LOG.warn( - "Parent session for table {} is not a GoogleAuthSession. Creating a new default catalog session.", - table); - return catalogSession(null, properties); + return parent; } /** Closes the manager. This is a no-op for GoogleAuthManager. */ diff --git a/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthManager.java b/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthManager.java index 60503e776e41..272693003d40 100644 --- a/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthManager.java +++ b/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthManager.java @@ -37,8 +37,6 @@ import java.nio.file.Files; import java.util.Collections; import java.util.Map; -import org.apache.iceberg.catalog.SessionCatalog; -import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.gcp.GCPProperties; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; @@ -178,57 +176,4 @@ public void initSessionDelegatesToCatalogSession() { assertThat(resultSession).isSameAs(mockSession); verify(spyManager).catalogSession(mockRestClient, props); } - - @Test - public void contextualSessionReusesGoogleAuthParent() { - AuthSession parentSession = mock(GoogleAuthSession.class); - AuthSession resultSession = - authManager.contextualSession(mock(SessionCatalog.SessionContext.class), parentSession); - assertThat(resultSession).isSameAs(parentSession); - } - - @Test - public void contextualSessionFallsBackToNewSessionWithNonGoogleParent() throws IOException { - AuthSession parentSession = mock(AuthSession.class); - mockedStaticCredentials - .when(GoogleCredentials::getApplicationDefault) - .thenReturn(mockCredsFromFile); - - when(mockCredsFromFile.createScoped(anyList())).thenReturn(mockCredentialsInstance); - - AuthSession resultSession = - authManager.contextualSession(mock(SessionCatalog.SessionContext.class), parentSession); - - assertThat(resultSession).isInstanceOf(GoogleAuthSession.class); - mockedStaticCredentials.verify(GoogleCredentials::getApplicationDefault, times(1)); - } - - @Test - public void tableSessionReusesGoogleAuthParent() { - AuthSession parentSession = mock(GoogleAuthSession.class); - AuthSession resultSession = - authManager.tableSession( - mock(TableIdentifier.class), Collections.emptyMap(), parentSession); - assertThat(resultSession).isSameAs(parentSession); - } - - @Test - public void tableSessionCreatesNewSessionWithNonGoogleParent() throws IOException { - AuthSession parentSession = mock(AuthSession.class); - Map properties = - ImmutableMap.of( - GCPProperties.GCP_CREDENTIALS_PATH_PROPERTY, fakeCredentialFile.getAbsolutePath()); - - mockedStaticCredentials - .when(() -> GoogleCredentials.fromStream(any(FileInputStream.class))) - .thenReturn(mockCredsFromFile); - when(mockCredsFromFile.createScoped(anyList())).thenReturn(mockCredentialsInstance); - - AuthSession resultSession = - authManager.tableSession(mock(TableIdentifier.class), properties, parentSession); - - assertThat(resultSession).isInstanceOf(GoogleAuthSession.class); - mockedStaticCredentials.verify( - () -> GoogleCredentials.fromStream(any(FileInputStream.class)), times(1)); - } } From 354df47fae7373683f6f82b1a32627d22c743a79 Mon Sep 17 00:00:00 2001 From: Talat Uyarer Date: Tue, 3 Jun 2025 08:22:58 -0700 Subject: [PATCH 3/6] Used AssertJ assertions --- .../iceberg/gcp/auth/GoogleAuthManager.java | 3 +- .../iceberg/gcp/auth/GoogleAuthSession.java | 2 + .../gcp/auth/TestGoogleAuthManager.java | 2 +- .../gcp/auth/TestGoogleAuthSession.java | 68 +++++++------------ 4 files changed, 31 insertions(+), 44 deletions(-) diff --git a/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java index 9a655db1522e..68242b1853ba 100644 --- a/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java +++ b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java @@ -60,7 +60,7 @@ public GoogleAuthManager(String managerName) { this.name = managerName; } - public String getName() { + public String name() { return name; } @@ -87,6 +87,7 @@ private void initialize(Map properties) { } catch (IOException e) { throw new UncheckedIOException("Failed to load Google credentials", e); } + this.initialized = true; } diff --git a/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthSession.java b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthSession.java index 62c524aeb55b..85da46349118 100644 --- a/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthSession.java +++ b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthSession.java @@ -22,6 +22,7 @@ import com.google.auth.oauth2.GoogleCredentials; import java.io.IOException; import java.io.UncheckedIOException; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; import org.apache.iceberg.rest.HTTPHeaders; import org.apache.iceberg.rest.HTTPRequest; import org.apache.iceberg.rest.ImmutableHTTPRequest; @@ -43,6 +44,7 @@ public class GoogleAuthSession implements AuthSession { * @param credentials The GoogleCredentials to use for authentication. */ public GoogleAuthSession(GoogleCredentials credentials) { + Preconditions.checkArgument(credentials != null, "credentials can not be null"); this.credentials = credentials; } diff --git a/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthManager.java b/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthManager.java index 272693003d40..35ff57b68332 100644 --- a/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthManager.java +++ b/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthManager.java @@ -81,7 +81,7 @@ public void afterEach() { @Test public void providesCorrectManagerName() { - assertThat(authManager.getName()).isEqualTo(MANAGER_NAME); + assertThat(authManager.name()).isEqualTo(MANAGER_NAME); } @Test diff --git a/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthSession.java b/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthSession.java index f7fad5388b9d..853d0cf0b417 100644 --- a/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthSession.java +++ b/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthSession.java @@ -18,6 +18,9 @@ */ package org.apache.iceberg.gcp.auth; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -28,12 +31,10 @@ import java.io.UncheckedIOException; import java.net.URI; import java.net.URISyntaxException; -import java.util.Set; import org.apache.iceberg.rest.HTTPHeaders; import org.apache.iceberg.rest.HTTPRequest; import org.apache.iceberg.rest.ImmutableHTTPRequest; import org.apache.iceberg.rest.auth.AuthSession; -import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -73,19 +74,14 @@ public void addsAuthHeaderOnSuccessfulTokenFetch() throws IOException { verify(credentials).refreshIfExpired(); verify(credentials).getAccessToken(); - Assertions.assertNotSame( - originalRequest, authenticatedRequest, "A new request object should be returned"); + assertThat(authenticatedRequest).isNotSameAs(originalRequest); HTTPHeaders headers = authenticatedRequest.headers(); - Assertions.assertEquals(1, headers.entries().size()); - Assertions.assertTrue(headers.contains("Authorization"), "Should contain Authorization header"); - - Set authHeaderEntries = headers.entries("Authorization"); - Assertions.assertEquals(1, authHeaderEntries.size(), "Should have one Authorization header"); - Assertions.assertEquals( - "Bearer " + TEST_TOKEN_VALUE, - authHeaderEntries.iterator().next().value(), - "Authorization header should be set correctly"); + assertThat(headers.contains("Authorization")).isTrue(); + assertThat(headers.entries("Authorization")) + .hasSize(1) + .extracting("value") + .containsExactly("Bearer " + TEST_TOKEN_VALUE); } @Test @@ -108,17 +104,12 @@ public void preservesExistingAuthHeader() throws IOException { verify(credentials).refreshIfExpired(); - Assertions.assertSame( - originalRequest, - authenticatedRequest, - "Original request object should be returned if header exists"); - - HTTPHeaders resultingHeaders = authenticatedRequest.headers(); - Assertions.assertEquals(1, resultingHeaders.entries().size()); - Assertions.assertEquals( - existingAuthHeaderValue, - resultingHeaders.entries("Authorization").iterator().next().value(), - "Existing Authorization header should be preserved"); + assertThat(authenticatedRequest).isSameAs(originalRequest); + assertThat(authenticatedRequest.headers().entries("Authorization")) + .hasSize(1) + .extracting("value") + .first() + .isEqualTo(existingAuthHeaderValue); } @Test @@ -132,14 +123,13 @@ public void propagatesIOExceptionAsUncheckedOnTokenRefreshFailure() throws IOExc .method(HTTPRequest.HTTPMethod.GET) .build(); - UncheckedIOException thrown = - Assertions.assertThrows( - UncheckedIOException.class, - () -> session.authenticate(originalRequest), - "Should throw UncheckedIOException on refresh failure"); + assertThatThrownBy(() -> session.authenticate(originalRequest)) + .isInstanceOf(UncheckedIOException.class) + .hasMessage("Failed to refresh Google access token") + .cause() + .isInstanceOf(IOException.class) + .hasMessage("Failed to refresh token"); - Assertions.assertEquals("Failed to refresh Google access token", thrown.getMessage()); - Assertions.assertInstanceOf(IOException.class, thrown.getCause()); verify(credentials).refreshIfExpired(); } @@ -155,10 +145,8 @@ public void returnsOriginalRequestWhenAccessTokenIsNull() throws IOException { .build(); HTTPRequest authenticatedRequest = session.authenticate(originalRequest); - Assertions.assertSame( - originalRequest, authenticatedRequest, "Request should be unchanged when token is null"); - Assertions.assertTrue( - authenticatedRequest.headers().entries().isEmpty(), "Headers should be empty"); + assertThat(authenticatedRequest).isSameAs(originalRequest); + assertThat(authenticatedRequest.headers().entries()).isEmpty(); } @Test @@ -174,16 +162,12 @@ public void returnsOriginalRequestWhenTokenValueIsNull() throws IOException { .build(); HTTPRequest authenticatedRequest = session.authenticate(originalRequest); - Assertions.assertSame( - originalRequest, - authenticatedRequest, - "Request should be unchanged when token value is null"); - Assertions.assertTrue( - authenticatedRequest.headers().entries().isEmpty(), "Headers should be empty"); + assertThat(authenticatedRequest).isSameAs(originalRequest); + assertThat(authenticatedRequest.headers().entries()).isEmpty(); } @Test public void sessionCloseBehavesAsNoOp() { - Assertions.assertDoesNotThrow(session::close); + assertThatCode(session::close).doesNotThrowAnyException(); } } From 88332d44779797e8d88e48b8f5d7246ba5774e43 Mon Sep 17 00:00:00 2001 From: Talat Uyarer Date: Wed, 4 Jun 2025 07:57:24 -0700 Subject: [PATCH 4/6] Adding a test where the scopes are just an empty string --- .../org/apache/iceberg/gcp/GCPProperties.java | 3 - .../iceberg/gcp/auth/GoogleAuthManager.java | 16 ++-- .../iceberg/gcp/auth/GoogleAuthSession.java | 2 +- .../gcp/auth/TestGoogleAuthManager.java | 88 +++++++++++-------- .../gcp/auth/TestGoogleAuthSession.java | 21 ++--- 5 files changed, 71 insertions(+), 59 deletions(-) diff --git a/gcp/src/main/java/org/apache/iceberg/gcp/GCPProperties.java b/gcp/src/main/java/org/apache/iceberg/gcp/GCPProperties.java index 417f4db38786..d91601125c74 100644 --- a/gcp/src/main/java/org/apache/iceberg/gcp/GCPProperties.java +++ b/gcp/src/main/java/org/apache/iceberg/gcp/GCPProperties.java @@ -62,9 +62,6 @@ public class GCPProperties implements Serializable { */ public static final int GCS_DELETE_BATCH_SIZE_DEFAULT = 50; - public static final String GCP_CREDENTIALS_PATH_PROPERTY = "gcp.auth.credentials-path"; - public static final String GCP_SCOPES_PROPERTY = "gcp.auth.scopes"; - private final Map allProperties; private String projectId; diff --git a/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java index 68242b1853ba..0cff5a2db704 100644 --- a/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java +++ b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java @@ -26,8 +26,9 @@ import java.util.Map; import org.apache.iceberg.catalog.SessionCatalog; import org.apache.iceberg.catalog.TableIdentifier; -import org.apache.iceberg.gcp.GCPProperties; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.base.Splitter; +import org.apache.iceberg.relocated.com.google.common.base.Strings; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; import org.apache.iceberg.rest.RESTClient; import org.apache.iceberg.rest.auth.AuthManager; @@ -51,6 +52,8 @@ public class GoogleAuthManager implements AuthManager { private static final Logger LOG = LoggerFactory.getLogger(GoogleAuthManager.class); public static final String DEFAULT_SCOPES = "https://www.googleapis.com/auth/cloud-platform"; + public static final String GCP_CREDENTIALS_PATH_PROPERTY = "gcp.auth.credentials-path"; + public static final String GCP_SCOPES_PROPERTY = "gcp.auth.scopes"; private final String name; private GoogleCredentials credentials; @@ -69,10 +72,13 @@ private void initialize(Map properties) { return; } - String credentialsPath = properties.get(GCPProperties.GCP_CREDENTIALS_PATH_PROPERTY); - String scopesString = - properties.getOrDefault(GCPProperties.GCP_SCOPES_PROPERTY, DEFAULT_SCOPES); - List scopes = ImmutableList.copyOf(scopesString.split(",")); + String credentialsPath = properties.get(GCP_CREDENTIALS_PATH_PROPERTY); + String scopesString = properties.getOrDefault(GCP_SCOPES_PROPERTY, DEFAULT_SCOPES); + List scopes = + Strings.isNullOrEmpty(scopesString) + ? ImmutableList.of() + : ImmutableList.copyOf( + Splitter.on(',').trimResults().omitEmptyStrings().splitToList(scopesString)); try { if (credentialsPath != null && !credentialsPath.isEmpty()) { diff --git a/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthSession.java b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthSession.java index 85da46349118..ce01eefc6b4f 100644 --- a/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthSession.java +++ b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthSession.java @@ -44,7 +44,7 @@ public class GoogleAuthSession implements AuthSession { * @param credentials The GoogleCredentials to use for authentication. */ public GoogleAuthSession(GoogleCredentials credentials) { - Preconditions.checkArgument(credentials != null, "credentials can not be null"); + Preconditions.checkArgument(credentials != null, "Invalid credentials: null"); this.credentials = credentials; } diff --git a/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthManager.java b/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthManager.java index 35ff57b68332..834cfbf92532 100644 --- a/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthManager.java +++ b/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthManager.java @@ -37,7 +37,6 @@ import java.nio.file.Files; import java.util.Collections; import java.util.Map; -import org.apache.iceberg.gcp.GCPProperties; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.apache.iceberg.rest.RESTClient; @@ -56,22 +55,22 @@ public class TestGoogleAuthManager { private static final String MANAGER_NAME = "testManager"; - @Mock private RESTClient mockRestClient; - @Mock private GoogleCredentials mockCredentialsInstance; - @Mock private GoogleCredentials mockCredsFromFile; + @Mock private RESTClient restClient; + @Mock private GoogleCredentials credentials; + @Mock private GoogleCredentials credentialsFromFile; private GoogleAuthManager authManager; private MockedStatic mockedStaticCredentials; @TempDir File tempDir; - private File fakeCredentialFile; + private File credentialFile; @BeforeEach public void beforeEach() throws IOException { authManager = new GoogleAuthManager(MANAGER_NAME); mockedStaticCredentials = Mockito.mockStatic(GoogleCredentials.class); - fakeCredentialFile = new File(tempDir, "fake-creds.json"); - Files.write(fakeCredentialFile.toPath(), "{\"type\": \"service_account\"}".getBytes()); + credentialFile = new File(tempDir, "fake-creds.json"); + Files.write(credentialFile.toPath(), "{\"type\": \"service_account\"}".getBytes()); } @AfterEach @@ -85,81 +84,96 @@ public void providesCorrectManagerName() { } @Test - public void buildsCatalogSessionFromCredentialsFile() throws IOException { + public void buildsCatalogSessionFromCredentialsFile() { String customScopes = "scope1,scope2"; Map properties = ImmutableMap.of( - GCPProperties.GCP_CREDENTIALS_PATH_PROPERTY, - fakeCredentialFile.getAbsolutePath(), - GCPProperties.GCP_SCOPES_PROPERTY, + GoogleAuthManager.GCP_CREDENTIALS_PATH_PROPERTY, + credentialFile.getAbsolutePath(), + GoogleAuthManager.GCP_SCOPES_PROPERTY, customScopes); mockedStaticCredentials .when(() -> GoogleCredentials.fromStream(any(FileInputStream.class))) - .thenReturn(mockCredsFromFile); - when(mockCredsFromFile.createScoped(anyList())).thenReturn(mockCredentialsInstance); + .thenReturn(credentialsFromFile); + when(credentialsFromFile.createScoped(anyList())).thenReturn(credentials); - AuthSession session = authManager.catalogSession(mockRestClient, properties); + AuthSession session = authManager.catalogSession(restClient, properties); assertThat(session).isInstanceOf(GoogleAuthSession.class); - verify(mockCredsFromFile).createScoped(ImmutableList.of("scope1", "scope2")); + verify(credentialsFromFile).createScoped(ImmutableList.of("scope1", "scope2")); } @Test - public void throwsUncheckedIOExceptionOnCredentialsFileError() throws IOException { + public void buildsCatalogSessionWithEmptyScopes() { Map properties = ImmutableMap.of( - GCPProperties.GCP_CREDENTIALS_PATH_PROPERTY, fakeCredentialFile.getAbsolutePath()); + GoogleAuthManager.GCP_CREDENTIALS_PATH_PROPERTY, + credentialFile.getAbsolutePath(), + GoogleAuthManager.GCP_SCOPES_PROPERTY, + ""); + + mockedStaticCredentials + .when(() -> GoogleCredentials.fromStream(any(FileInputStream.class))) + .thenReturn(credentialsFromFile); + when(credentialsFromFile.createScoped(anyList())).thenReturn(credentials); + + AuthSession session = authManager.catalogSession(restClient, properties); + + assertThat(session).isInstanceOf(GoogleAuthSession.class); + verify(credentialsFromFile).createScoped(ImmutableList.of()); + } + + @Test + public void throwsUncheckedIOExceptionOnCredentialsFileError() { + Map properties = + ImmutableMap.of( + GoogleAuthManager.GCP_CREDENTIALS_PATH_PROPERTY, credentialFile.getAbsolutePath()); mockedStaticCredentials .when(() -> GoogleCredentials.fromStream(any(FileInputStream.class))) .thenThrow(new IOException("Simulated stream loading failure")); - assertThatThrownBy(() -> authManager.catalogSession(mockRestClient, properties)) + assertThatThrownBy(() -> authManager.catalogSession(restClient, properties)) .isInstanceOf(UncheckedIOException.class) .hasMessageContaining("Failed to load Google credentials"); } @Test - public void buildsCatalogSessionUsingADC() throws IOException { - Map properties = Collections.emptyMap(); - + public void buildsCatalogSessionUsingADC() { mockedStaticCredentials .when(GoogleCredentials::getApplicationDefault) - .thenReturn(mockCredsFromFile); + .thenReturn(credentialsFromFile); - when(mockCredsFromFile.createScoped(anyList())).thenReturn(mockCredentialsInstance); + when(credentialsFromFile.createScoped(anyList())).thenReturn(credentials); - AuthSession session = authManager.catalogSession(mockRestClient, properties); + AuthSession session = authManager.catalogSession(restClient, Collections.emptyMap()); assertThat(session).isInstanceOf(GoogleAuthSession.class); mockedStaticCredentials.verify(GoogleCredentials::getApplicationDefault, times(1)); } @Test - public void throwsUncheckedIOExceptionOnADCError() throws IOException { - Map properties = Collections.emptyMap(); - + public void throwsUncheckedIOExceptionOnADCError() { mockedStaticCredentials .when(GoogleCredentials::getApplicationDefault) .thenThrow(new IOException("ADC unavailable")); - assertThatThrownBy(() -> authManager.catalogSession(mockRestClient, properties)) + assertThatThrownBy(() -> authManager.catalogSession(restClient, Collections.emptyMap())) .isInstanceOf(UncheckedIOException.class) .hasMessageContaining("Failed to load Google credentials"); } @Test - public void initializationOccursOnlyOnce() throws IOException { - Map properties = Collections.emptyMap(); + public void initializationOccursOnlyOnce() { mockedStaticCredentials .when(GoogleCredentials::getApplicationDefault) - .thenReturn(mockCredsFromFile); + .thenReturn(credentialsFromFile); - when(mockCredsFromFile.createScoped(anyList())).thenReturn(mockCredentialsInstance); + when(credentialsFromFile.createScoped(anyList())).thenReturn(credentials); - authManager.catalogSession(mockRestClient, properties); - authManager.catalogSession(mockRestClient, properties); + authManager.catalogSession(restClient, Collections.emptyMap()); + authManager.catalogSession(restClient, Collections.emptyMap()); mockedStaticCredentials.verify(GoogleCredentials::getApplicationDefault, times(1)); } @@ -170,10 +184,10 @@ public void initSessionDelegatesToCatalogSession() { AuthSession mockSession = mock(GoogleAuthSession.class); Map props = Collections.emptyMap(); - doReturn(mockSession).when(spyManager).catalogSession(mockRestClient, props); + doReturn(mockSession).when(spyManager).catalogSession(restClient, props); - AuthSession resultSession = spyManager.initSession(mockRestClient, props); + AuthSession resultSession = spyManager.initSession(restClient, props); assertThat(resultSession).isSameAs(mockSession); - verify(spyManager).catalogSession(mockRestClient, props); + verify(spyManager).catalogSession(restClient, props); } } diff --git a/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthSession.java b/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthSession.java index 853d0cf0b417..c860028792dd 100644 --- a/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthSession.java +++ b/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthSession.java @@ -44,13 +44,13 @@ @ExtendWith(MockitoExtension.class) public class TestGoogleAuthSession { + private static final String TEST_TOKEN_VALUE = "test-token-12345"; + private static final String TEST_RELATIVE_PATH = "v1/some/resource"; @Mock private GoogleCredentials credentials; @Mock private AccessToken accessToken; private AuthSession session; - private static final String TEST_TOKEN_VALUE = "test-token-12345"; private URI testBaseUri; - private static final String TEST_RELATIVE_PATH = "v1/some/resource"; @BeforeEach public void beforeEach() throws URISyntaxException { @@ -78,10 +78,8 @@ public void addsAuthHeaderOnSuccessfulTokenFetch() throws IOException { HTTPHeaders headers = authenticatedRequest.headers(); assertThat(headers.contains("Authorization")).isTrue(); - assertThat(headers.entries("Authorization")) - .hasSize(1) - .extracting("value") - .containsExactly("Bearer " + TEST_TOKEN_VALUE); + assertThat(authenticatedRequest.headers().entries()) + .containsExactly(HTTPHeaders.HTTPHeader.of("Authorization", "Bearer " + TEST_TOKEN_VALUE)); } @Test @@ -105,11 +103,8 @@ public void preservesExistingAuthHeader() throws IOException { verify(credentials).refreshIfExpired(); assertThat(authenticatedRequest).isSameAs(originalRequest); - assertThat(authenticatedRequest.headers().entries("Authorization")) - .hasSize(1) - .extracting("value") - .first() - .isEqualTo(existingAuthHeaderValue); + assertThat(authenticatedRequest.headers().entries()) + .containsExactly(HTTPHeaders.HTTPHeader.of("Authorization", existingAuthHeaderValue)); } @Test @@ -134,7 +129,7 @@ public void propagatesIOExceptionAsUncheckedOnTokenRefreshFailure() throws IOExc } @Test - public void returnsOriginalRequestWhenAccessTokenIsNull() throws IOException { + public void returnsOriginalRequestWhenAccessTokenIsNull() { when(credentials.getAccessToken()).thenReturn(null); HTTPRequest originalRequest = @@ -150,7 +145,7 @@ public void returnsOriginalRequestWhenAccessTokenIsNull() throws IOException { } @Test - public void returnsOriginalRequestWhenTokenValueIsNull() throws IOException { + public void returnsOriginalRequestWhenTokenValueIsNull() { when(credentials.getAccessToken()).thenReturn(accessToken); when(accessToken.getTokenValue()).thenReturn(null); From e901b3a9c9d1640dbb582ac950f99a64e825dab2 Mon Sep 17 00:00:00 2001 From: Talat Uyarer Date: Wed, 4 Jun 2025 08:39:59 -0700 Subject: [PATCH 5/6] Moved SPLITTER to private final static --- .../java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java index 0cff5a2db704..8e9ca4abf026 100644 --- a/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java +++ b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java @@ -51,6 +51,7 @@ */ public class GoogleAuthManager implements AuthManager { private static final Logger LOG = LoggerFactory.getLogger(GoogleAuthManager.class); + private static final Splitter SPLITTER = Splitter.on(',').trimResults().omitEmptyStrings(); public static final String DEFAULT_SCOPES = "https://www.googleapis.com/auth/cloud-platform"; public static final String GCP_CREDENTIALS_PATH_PROPERTY = "gcp.auth.credentials-path"; public static final String GCP_SCOPES_PROPERTY = "gcp.auth.scopes"; @@ -77,8 +78,7 @@ private void initialize(Map properties) { List scopes = Strings.isNullOrEmpty(scopesString) ? ImmutableList.of() - : ImmutableList.copyOf( - Splitter.on(',').trimResults().omitEmptyStrings().splitToList(scopesString)); + : ImmutableList.copyOf(SPLITTER.splitToList(scopesString)); try { if (credentialsPath != null && !credentialsPath.isEmpty()) { From 88bb1cf530c8ecddfce889748f16926eb94ce6f3 Mon Sep 17 00:00:00 2001 From: Talat Uyarer Date: Mon, 16 Jun 2025 16:25:17 -0700 Subject: [PATCH 6/6] Addresed @danielcweeks comments. --- .../iceberg/gcp/auth/GoogleAuthManager.java | 13 +++++-------- .../iceberg/gcp/auth/GoogleAuthSession.java | 12 ++++-------- .../iceberg/gcp/auth/TestGoogleAuthSession.java | 16 ++++++++-------- 3 files changed, 17 insertions(+), 24 deletions(-) diff --git a/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java index 8e9ca4abf026..a1d2b539ab16 100644 --- a/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java +++ b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthManager.java @@ -124,11 +124,12 @@ public AuthSession catalogSession(RESTClient sharedClient, Map p return new GoogleAuthSession(credentials); } - /** Returns a session for a specific context. Defaults to the catalog session. */ + /** + * Returns a session for a specific context. Defaults to the catalog session. For GCP, tokens are + * typically not context-specific in this manner. + */ @Override public AuthSession contextualSession(SessionCatalog.SessionContext context, AuthSession parent) { - // For GCP, tokens are typically not context-specific in this manner. - // Re-using the parent (which should be a GoogleAuthSession) is appropriate. return parent; } @@ -136,14 +137,10 @@ public AuthSession contextualSession(SessionCatalog.SessionContext context, Auth @Override public AuthSession tableSession( TableIdentifier table, Map properties, AuthSession parent) { - // Similar to contextualSession, GCP tokens are generally not table-specific. return parent; } /** Closes the manager. This is a no-op for GoogleAuthManager. */ @Override - public void close() { - // No-op. Credentials lifecycle is managed by the GoogleCredentials object itself or by the - // application. - } + public void close() {} } diff --git a/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthSession.java b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthSession.java index ce01eefc6b4f..c81053613da4 100644 --- a/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthSession.java +++ b/gcp/src/main/java/org/apache/iceberg/gcp/auth/GoogleAuthSession.java @@ -34,7 +34,7 @@ * An authentication session that uses Google Credentials (typically Application Default * Credentials) to obtain an OAuth2 access token and add it to HTTP requests. */ -public class GoogleAuthSession implements AuthSession { +class GoogleAuthSession implements AuthSession { private static final Logger LOG = LoggerFactory.getLogger(GoogleAuthSession.class); private final GoogleCredentials credentials; @@ -43,7 +43,7 @@ public class GoogleAuthSession implements AuthSession { * * @param credentials The GoogleCredentials to use for authentication. */ - public GoogleAuthSession(GoogleCredentials credentials) { + GoogleAuthSession(GoogleCredentials credentials) { Preconditions.checkArgument(credentials != null, "Invalid credentials: null"); this.credentials = credentials; } @@ -59,9 +59,6 @@ public GoogleAuthSession(GoogleCredentials credentials) { @Override public HTTPRequest authenticate(HTTPRequest request) { try { - // Ensure the credentials have a valid token, refreshing if necessary. - // refreshIfExpired() returns true if the token was refreshed, false otherwise. - // In either case, getAccessToken() after this call should provide a usable token. credentials.refreshIfExpired(); AccessToken token = credentials.getAccessToken(); @@ -77,9 +74,8 @@ public HTTPRequest authenticate(HTTPRequest request) { ? request : ImmutableHTTPRequest.builder().from(request).headers(newHeaders).build(); } else { - LOG.warn( - "Failed to obtain Google access token. Request will be sent without Authorization header."); - return request; + throw new IllegalStateException( + "Failed to obtain Google access token. Cannot authenticate request."); } } catch (IOException e) { LOG.error("IOException while trying to refresh Google access token", e); diff --git a/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthSession.java b/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthSession.java index c860028792dd..ff2163b421c7 100644 --- a/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthSession.java +++ b/gcp/src/test/java/org/apache/iceberg/gcp/auth/TestGoogleAuthSession.java @@ -129,7 +129,7 @@ public void propagatesIOExceptionAsUncheckedOnTokenRefreshFailure() throws IOExc } @Test - public void returnsOriginalRequestWhenAccessTokenIsNull() { + public void throwsExceptionWhenAccessTokenIsNull() { when(credentials.getAccessToken()).thenReturn(null); HTTPRequest originalRequest = @@ -138,14 +138,14 @@ public void returnsOriginalRequestWhenAccessTokenIsNull() { .path(TEST_RELATIVE_PATH) .method(HTTPRequest.HTTPMethod.GET) .build(); - HTTPRequest authenticatedRequest = session.authenticate(originalRequest); - assertThat(authenticatedRequest).isSameAs(originalRequest); - assertThat(authenticatedRequest.headers().entries()).isEmpty(); + assertThatThrownBy(() -> session.authenticate(originalRequest)) + .isInstanceOf(IllegalStateException.class) + .hasMessage("Failed to obtain Google access token. Cannot authenticate request."); } @Test - public void returnsOriginalRequestWhenTokenValueIsNull() { + public void throwsExceptionWhenTokenValueIsNull() { when(credentials.getAccessToken()).thenReturn(accessToken); when(accessToken.getTokenValue()).thenReturn(null); @@ -155,10 +155,10 @@ public void returnsOriginalRequestWhenTokenValueIsNull() { .path(TEST_RELATIVE_PATH) .method(HTTPRequest.HTTPMethod.GET) .build(); - HTTPRequest authenticatedRequest = session.authenticate(originalRequest); - assertThat(authenticatedRequest).isSameAs(originalRequest); - assertThat(authenticatedRequest.headers().entries()).isEmpty(); + assertThatThrownBy(() -> session.authenticate(originalRequest)) + .isInstanceOf(IllegalStateException.class) + .hasMessage("Failed to obtain Google access token. Cannot authenticate request."); } @Test