From 47f98b872c6896ad2af37e30db440321c2adb492 Mon Sep 17 00:00:00 2001 From: arithmetic1728 <58957152+arithmetic1728@users.noreply.github.com> Date: Fri, 28 Jan 2022 17:39:42 -0800 Subject: [PATCH] fix: revert "feat: add api key support (#1436)" (#1617) --- .../com/google/api/gax/rpc/ClientContext.java | 58 +++------------ .../com/google/api/gax/rpc/StubSettings.java | 20 ----- .../google/api/gax/rpc/ClientContextTest.java | 74 +------------------ 3 files changed, 11 insertions(+), 141 deletions(-) diff --git a/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java b/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java index 2afd5e2a8..ff18e77d3 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java +++ b/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java @@ -35,14 +35,12 @@ import com.google.api.gax.core.BackgroundResource; import com.google.api.gax.core.ExecutorAsBackgroundResource; import com.google.api.gax.core.ExecutorProvider; -import com.google.api.gax.rpc.internal.EnvironmentProvider; import com.google.api.gax.rpc.internal.QuotaProjectIdHidingCredentials; import com.google.api.gax.rpc.mtls.MtlsProvider; import com.google.api.gax.tracing.ApiTracerFactory; import com.google.api.gax.tracing.BaseApiTracerFactory; import com.google.auth.Credentials; import com.google.auto.value.AutoValue; -import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Sets; @@ -67,7 +65,6 @@ @AutoValue public abstract class ClientContext { private static final String QUOTA_PROJECT_ID_HEADER_KEY = "x-goog-user-project"; - private static final String API_KEY_HEADER_KEY = "x-goog-api-key"; /** * The objects that need to be closed in order to clean up the resources created in the process of @@ -162,32 +159,6 @@ static String getEndpoint( return endpoint; } - /** - * Retrieves the API key value and add it to the headers if API key exists. It first tries to - * retrieve the value from the stub settings. If not found, it then tries the load the - * GOOGLE_API_KEY environment variable. An IOException will be thrown if both GOOGLE_API_KEY and - * GOOGLE_APPLICATION_CREDENTIALS environment variables are set. - */ - @VisibleForTesting - static void addApiKeyToHeaders( - StubSettings settings, EnvironmentProvider environmentProvider, Map headers) - throws IOException { - if (settings.getApiKey() != null) { - headers.put(API_KEY_HEADER_KEY, settings.getApiKey()); - return; - } - - String apiKey = environmentProvider.getenv("GOOGLE_API_KEY"); - String applicationCredentials = environmentProvider.getenv("GOOGLE_APPLICATION_CREDENTIALS"); - if (apiKey != null && applicationCredentials != null) { - throw new IOException( - "Environment variables GOOGLE_API_KEY and GOOGLE_APPLICATION_CREDENTIALS are mutually exclusive"); - } - if (apiKey != null) { - headers.put(API_KEY_HEADER_KEY, apiKey); - } - } - /** * Instantiates the executor, credentials, and transport context based on the given client * settings. @@ -198,21 +169,14 @@ public static ClientContext create(StubSettings settings) throws IOException { ExecutorProvider backgroundExecutorProvider = settings.getBackgroundExecutorProvider(); final ScheduledExecutorService backgroundExecutor = backgroundExecutorProvider.getExecutor(); - Credentials credentials = null; - Map headers = getHeadersFromSettingsAndEnvironment(settings, System::getenv); - - boolean hasApiKey = headers.containsKey(API_KEY_HEADER_KEY); - if (!hasApiKey) { - credentials = settings.getCredentialsProvider().getCredentials(); + Credentials credentials = settings.getCredentialsProvider().getCredentials(); - if (settings.getQuotaProjectId() != null) { - // If the quotaProjectId is set, wrap original credentials with correct quotaProjectId as - // QuotaProjectIdHidingCredentials. - // Ensure that a custom set quota project id takes priority over one detected by - // credentials. - // Avoid the backend receiving possibly conflict values of quotaProjectId - credentials = new QuotaProjectIdHidingCredentials(credentials); - } + if (settings.getQuotaProjectId() != null) { + // If the quotaProjectId is set, wrap original credentials with correct quotaProjectId as + // QuotaProjectIdHidingCredentials. + // Ensure that a custom set quota project id takes priority over one detected by credentials. + // Avoid the backend receiving possibly conflict values of quotaProjectId + credentials = new QuotaProjectIdHidingCredentials(credentials); } TransportChannelProvider transportChannelProvider = settings.getTransportChannelProvider(); @@ -222,11 +186,11 @@ public static ClientContext create(StubSettings settings) throws IOException { if (transportChannelProvider.needsExecutor() && settings.getExecutorProvider() != null) { transportChannelProvider = transportChannelProvider.withExecutor(backgroundExecutor); } - + Map headers = getHeadersFromSettings(settings); if (transportChannelProvider.needsHeaders()) { transportChannelProvider = transportChannelProvider.withHeaders(headers); } - if (!hasApiKey && transportChannelProvider.needsCredentials()) { + if (transportChannelProvider.needsCredentials() && credentials != null) { transportChannelProvider = transportChannelProvider.withCredentials(credentials); } String endpoint = @@ -296,8 +260,7 @@ public static ClientContext create(StubSettings settings) throws IOException { * Getting a header map from HeaderProvider and InternalHeaderProvider from settings with Quota * Project Id. */ - private static Map getHeadersFromSettingsAndEnvironment( - StubSettings settings, EnvironmentProvider environmentProvider) throws IOException { + private static Map getHeadersFromSettings(StubSettings settings) { // Resolve conflicts when merging headers from multiple sources Map userHeaders = settings.getHeaderProvider().getHeaders(); Map internalHeaders = settings.getInternalHeaderProvider().getHeaders(); @@ -323,7 +286,6 @@ private static Map getHeadersFromSettingsAndEnvironment( effectiveHeaders.putAll(internalHeaders); effectiveHeaders.putAll(userHeaders); effectiveHeaders.putAll(conflictResolution); - addApiKeyToHeaders(settings, environmentProvider, effectiveHeaders); return ImmutableMap.copyOf(effectiveHeaders); } diff --git a/gax/src/main/java/com/google/api/gax/rpc/StubSettings.java b/gax/src/main/java/com/google/api/gax/rpc/StubSettings.java index 2fc25f566..04f1d59c9 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/StubSettings.java +++ b/gax/src/main/java/com/google/api/gax/rpc/StubSettings.java @@ -73,7 +73,6 @@ public abstract class StubSettings> { private final String endpoint; private final String mtlsEndpoint; private final String quotaProjectId; - private final String apiKey; @Nullable private final WatchdogProvider streamWatchdogProvider; @Nonnull private final Duration streamWatchdogCheckInterval; @Nonnull private final ApiTracerFactory tracerFactory; @@ -100,7 +99,6 @@ protected StubSettings(Builder builder) { this.mtlsEndpoint = builder.mtlsEndpoint; this.switchToMtlsEndpointAllowed = builder.switchToMtlsEndpointAllowed; this.quotaProjectId = builder.quotaProjectId; - this.apiKey = builder.apiKey; this.streamWatchdogProvider = builder.streamWatchdogProvider; this.streamWatchdogCheckInterval = builder.streamWatchdogCheckInterval; this.tracerFactory = builder.tracerFactory; @@ -156,10 +154,6 @@ public final String getQuotaProjectId() { return quotaProjectId; } - public final String getApiKey() { - return apiKey; - } - @BetaApi("The surface for streaming is not stable yet and may change in the future.") @Nullable public final WatchdogProvider getStreamWatchdogProvider() { @@ -195,7 +189,6 @@ public String toString() { .add("mtlsEndpoint", mtlsEndpoint) .add("switchToMtlsEndpointAllowed", switchToMtlsEndpointAllowed) .add("quotaProjectId", quotaProjectId) - .add("apiKey", apiKey) .add("streamWatchdogProvider", streamWatchdogProvider) .add("streamWatchdogCheckInterval", streamWatchdogCheckInterval) .add("tracerFactory", tracerFactory) @@ -216,7 +209,6 @@ public abstract static class Builder< private String endpoint; private String mtlsEndpoint; private String quotaProjectId; - private String apiKey; @Nullable private WatchdogProvider streamWatchdogProvider; @Nonnull private Duration streamWatchdogCheckInterval; @Nonnull private ApiTracerFactory tracerFactory; @@ -242,7 +234,6 @@ protected Builder(StubSettings settings) { this.mtlsEndpoint = settings.mtlsEndpoint; this.switchToMtlsEndpointAllowed = settings.switchToMtlsEndpointAllowed; this.quotaProjectId = settings.quotaProjectId; - this.apiKey = settings.apiKey; this.streamWatchdogProvider = settings.streamWatchdogProvider; this.streamWatchdogCheckInterval = settings.streamWatchdogCheckInterval; this.tracerFactory = settings.tracerFactory; @@ -267,7 +258,6 @@ private static String getQuotaProjectIdFromClientContext(ClientContext clientCon } protected Builder(ClientContext clientContext) { - this.apiKey = null; if (clientContext == null) { this.backgroundExecutorProvider = InstantiatingExecutorProvider.newBuilder().build(); this.transportChannelProvider = null; @@ -442,11 +432,6 @@ public B setQuotaProjectId(String quotaProjectId) { return self(); } - public B setApiKey(String apiKey) { - this.apiKey = apiKey; - return self(); - } - /** * Sets how often the {@link Watchdog} will check ongoing streaming RPCs. Defaults to 10 secs. * Use {@link Duration#ZERO} to disable. @@ -528,10 +513,6 @@ public String getQuotaProjectId() { return quotaProjectId; } - public String getApiKey() { - return apiKey; - } - @BetaApi("The surface for streaming is not stable yet and may change in the future.") @Nonnull public Duration getStreamWatchdogCheckInterval() { @@ -568,7 +549,6 @@ public String toString() { .add("mtlsEndpoint", mtlsEndpoint) .add("switchToMtlsEndpointAllowed", switchToMtlsEndpointAllowed) .add("quotaProjectId", quotaProjectId) - .add("apiKey", apiKey) .add("streamWatchdogProvider", streamWatchdogProvider) .add("streamWatchdogCheckInterval", streamWatchdogCheckInterval) .add("tracerFactory", tracerFactory) diff --git a/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java b/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java index b90e1e422..7fe418dd6 100644 --- a/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java +++ b/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java @@ -32,7 +32,6 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -42,7 +41,6 @@ import com.google.api.gax.core.ExecutorProvider; import com.google.api.gax.core.FixedCredentialsProvider; import com.google.api.gax.core.FixedExecutorProvider; -import com.google.api.gax.rpc.internal.EnvironmentProvider; import com.google.api.gax.rpc.mtls.MtlsProvider; import com.google.api.gax.rpc.mtls.MtlsProvider.MtlsEndpointUsagePolicy; import com.google.api.gax.rpc.testing.FakeChannel; @@ -56,7 +54,6 @@ import com.google.common.truth.Truth; import java.io.IOException; import java.util.Collections; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.concurrent.Executor; @@ -179,7 +176,7 @@ public TransportChannelProvider withPoolSize(int size) { @Override public TransportChannel getTransportChannel() throws IOException { - if (needsCredentials() && !headers.containsKey("x-goog-api-key")) { + if (needsCredentials()) { throw new IllegalStateException("Needs Credentials"); } transport.setExecutor(executor); @@ -772,73 +769,4 @@ public void testExecutorSettings() throws Exception { transportChannel = (FakeTransportChannel) context.getTransportChannel(); assertThat(transportChannel.getExecutor()).isSameInstanceAs(executorProvider.getExecutor()); } - - @Test - public void testAddApiKeyToHeadersFromStubSettings() throws IOException { - StubSettings settings = new FakeStubSettings.Builder().setApiKey("stub-setting-key").build(); - EnvironmentProvider environmentProvider = - name -> name.equals("GOOGLE_API_KEY") ? "env-key" : null; - Map headers = new HashMap<>(); - ClientContext.addApiKeyToHeaders(settings, environmentProvider, headers); - assertThat(headers).containsEntry("x-goog-api-key", "stub-setting-key"); - } - - @Test - public void testAddApiKeyToHeadersFromEnvironmentProvider() throws IOException { - StubSettings settings = new FakeStubSettings.Builder().build(); - EnvironmentProvider environmentProvider = - name -> name.equals("GOOGLE_API_KEY") ? "env-key" : null; - Map headers = new HashMap<>(); - ClientContext.addApiKeyToHeaders(settings, environmentProvider, headers); - assertThat(headers).containsEntry("x-goog-api-key", "env-key"); - } - - @Test - public void testAddApiKeyToHeadersNoApiKey() throws IOException { - StubSettings settings = new FakeStubSettings.Builder().build(); - EnvironmentProvider environmentProvider = name -> null; - Map headers = new HashMap<>(); - ClientContext.addApiKeyToHeaders(settings, environmentProvider, headers); - assertThat(headers).doesNotContainKey("x-goog-api-key"); - } - - @Test - public void testAddApiKeyToHeadersThrows() throws IOException { - StubSettings settings = new FakeStubSettings.Builder().build(); - EnvironmentProvider environmentProvider = - name -> name.equals("GOOGLE_API_KEY") ? "env-key" : "/path/to/adc/json"; - Map headers = new HashMap<>(); - Exception ex = - assertThrows( - IOException.class, - () -> ClientContext.addApiKeyToHeaders(settings, environmentProvider, headers)); - assertThat(ex) - .hasMessageThat() - .contains( - "Environment variables GOOGLE_API_KEY and GOOGLE_APPLICATION_CREDENTIALS are mutually exclusive"); - } - - @Test - public void testApiKey() throws IOException { - FakeStubSettings.Builder builder = new FakeStubSettings.Builder(); - - FakeTransportChannel transportChannel = FakeTransportChannel.create(new FakeChannel()); - FakeTransportProvider transportProvider = - new FakeTransportProvider(transportChannel, null, true, null, null); - builder.setTransportChannelProvider(transportProvider); - - HeaderProvider headerProvider = Mockito.mock(HeaderProvider.class); - Mockito.when(headerProvider.getHeaders()).thenReturn(ImmutableMap.of()); - builder.setHeaderProvider(headerProvider); - - // Set API key. - builder.setApiKey("key"); - - ClientContext context = ClientContext.create(builder.build()); - - // Check API key is in the transport channel's header. - List resources = context.getBackgroundResources(); - FakeTransportChannel fakeTransportChannel = (FakeTransportChannel) resources.get(0); - assertThat(fakeTransportChannel.getHeaders()).containsEntry("x-goog-api-key", "key"); - } }