From a6bdb943097a3e0f8d665201b3cc16b1f16b0601 Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Mon, 23 Jan 2023 16:00:38 -0800 Subject: [PATCH 1/3] googleapis: Allow user set c2p bootstrap config Instead of always overriding the bootstrap with a custom c2p config, now we allow user defined ones to also be used. This only applies when running in GCP with federation. --- .../GoogleCloudToProdNameResolver.java | 15 ++++++++++++--- .../GoogleCloudToProdNameResolverTest.java | 19 +++++++++++++++++-- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/googleapis/src/main/java/io/grpc/googleapis/GoogleCloudToProdNameResolver.java b/googleapis/src/main/java/io/grpc/googleapis/GoogleCloudToProdNameResolver.java index df592802fa8..fc92e9dce20 100644 --- a/googleapis/src/main/java/io/grpc/googleapis/GoogleCloudToProdNameResolver.java +++ b/googleapis/src/main/java/io/grpc/googleapis/GoogleCloudToProdNameResolver.java @@ -162,7 +162,14 @@ public void run() { try { zone = queryZoneMetadata(METADATA_URL_ZONE); supportIpv6 = queryIpv6SupportMetadata(METADATA_URL_SUPPORT_IPV6); - rawBootstrap = generateBootstrap(zone, supportIpv6); + + // User provided bootstrap configs are only supported with federation. If federation is + // not enabled or there is no user provided config, we set a custom bootstrap override. + // Otherwise, we don't set the override, which will allow a user provided bootstrap config + // to take effect. + if (!enableFederation || !xdsBootstrapProvided) { + rawBootstrap = generateBootstrap(zone, supportIpv6); + } } catch (IOException e) { listener.onError(Status.INTERNAL.withDescription("Unable to get metadata").withCause(e)); } finally { @@ -170,8 +177,10 @@ public void run() { syncContext.execute(new Runnable() { @Override public void run() { - if (!shutdown && finalRawBootstrap != null) { - bootstrapSetter.setBootstrap(finalRawBootstrap); + if (!shutdown) { + if (finalRawBootstrap != null) { + bootstrapSetter.setBootstrap(finalRawBootstrap); + } delegate.start(listener); succeeded = true; } diff --git a/googleapis/src/test/java/io/grpc/googleapis/GoogleCloudToProdNameResolverTest.java b/googleapis/src/test/java/io/grpc/googleapis/GoogleCloudToProdNameResolverTest.java index a7c4ab059b6..52174c19a3b 100644 --- a/googleapis/src/test/java/io/grpc/googleapis/GoogleCloudToProdNameResolverTest.java +++ b/googleapis/src/test/java/io/grpc/googleapis/GoogleCloudToProdNameResolverTest.java @@ -195,9 +195,9 @@ public void onGcpAndNoProvidedBootstrapDelegateToXds() { @SuppressWarnings("unchecked") @Test - public void onGcpAndProvidedBootstrapAndFederationEnabledDelegateToXds() { + public void onGcpAndNoProvidedBootstrapAndFederationEnabledDelegateToXds() { GoogleCloudToProdNameResolver.isOnGcp = true; - GoogleCloudToProdNameResolver.xdsBootstrapProvided = true; + GoogleCloudToProdNameResolver.xdsBootstrapProvided = false; GoogleCloudToProdNameResolver.enableFederation = true; createResolver(); resolver.start(mockListener); @@ -223,6 +223,21 @@ public void onGcpAndProvidedBootstrapAndFederationEnabledDelegateToXds() { ImmutableMap.of("xds_servers", ImmutableList.of(server))); } + @SuppressWarnings("unchecked") + @Test + public void onGcpAndProvidedBootstrapAndFederationEnabledDontDelegateToXds() { + GoogleCloudToProdNameResolver.isOnGcp = true; + GoogleCloudToProdNameResolver.xdsBootstrapProvided = true; + GoogleCloudToProdNameResolver.enableFederation = true; + createResolver(); + resolver.start(mockListener); + fakeExecutor.runDueTasks(); + assertThat(delegatedResolver.keySet()).containsExactly("xds"); + verify(Iterables.getOnlyElement(delegatedResolver.values())).start(mockListener); + // Bootstrapper should not have been set, since there was no user provided config. + assertThat(fakeBootstrapSetter.bootstrapRef.get()).isNull(); + } + @Test public void failToQueryMetadata() { GoogleCloudToProdNameResolver.isOnGcp = true; From 0edaca3871c752472defee74fcca335e4753a4d7 Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Mon, 23 Jan 2023 17:41:41 -0800 Subject: [PATCH 2/3] Include zone and ipv6 in the condition Also some refactoring for code clarity. --- .../GoogleCloudToProdNameResolver.java | 53 +++++++++---------- 1 file changed, 25 insertions(+), 28 deletions(-) diff --git a/googleapis/src/main/java/io/grpc/googleapis/GoogleCloudToProdNameResolver.java b/googleapis/src/main/java/io/grpc/googleapis/GoogleCloudToProdNameResolver.java index fc92e9dce20..24eea542bf9 100644 --- a/googleapis/src/main/java/io/grpc/googleapis/GoogleCloudToProdNameResolver.java +++ b/googleapis/src/main/java/io/grpc/googleapis/GoogleCloudToProdNameResolver.java @@ -156,38 +156,35 @@ private void resolve() { class Resolve implements Runnable { @Override public void run() { - String zone; - boolean supportIpv6; ImmutableMap rawBootstrap = null; - try { - zone = queryZoneMetadata(METADATA_URL_ZONE); - supportIpv6 = queryIpv6SupportMetadata(METADATA_URL_SUPPORT_IPV6); - - // User provided bootstrap configs are only supported with federation. If federation is - // not enabled or there is no user provided config, we set a custom bootstrap override. - // Otherwise, we don't set the override, which will allow a user provided bootstrap config - // to take effect. - if (!enableFederation || !xdsBootstrapProvided) { - rawBootstrap = generateBootstrap(zone, supportIpv6); + // User provided bootstrap configs are only supported with federation. If federation is + // not enabled or there is no user provided config, we set a custom bootstrap override. + // Otherwise, we don't set the override, which will allow a user provided bootstrap config + // to take effect. + if (!enableFederation || !xdsBootstrapProvided) { + try { + rawBootstrap = generateBootstrap(queryZoneMetadata(METADATA_URL_ZONE), + queryIpv6SupportMetadata(METADATA_URL_SUPPORT_IPV6)); + } catch (IOException e) { + listener.onError( + Status.INTERNAL.withDescription("Unable to get metadata").withCause(e)); } - } catch (IOException e) { - listener.onError(Status.INTERNAL.withDescription("Unable to get metadata").withCause(e)); - } finally { - final ImmutableMap finalRawBootstrap = rawBootstrap; - syncContext.execute(new Runnable() { - @Override - public void run() { - if (!shutdown) { - if (finalRawBootstrap != null) { - bootstrapSetter.setBootstrap(finalRawBootstrap); - } - delegate.start(listener); - succeeded = true; + } + + final ImmutableMap finalRawBootstrap = rawBootstrap; + syncContext.execute(new Runnable() { + @Override + public void run() { + if (!shutdown) { + if (finalRawBootstrap != null) { + bootstrapSetter.setBootstrap(finalRawBootstrap); } - resolving = false; + delegate.start(listener); + succeeded = true; } - }); - } + resolving = false; + } + }); } } From 3eb59db9eaf3d3fa786ae54154b98344005e92a0 Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Tue, 24 Jan 2023 09:43:53 -0800 Subject: [PATCH 3/3] Put delegate starting back into a finally block --- .../GoogleCloudToProdNameResolver.java | 46 +++++++++---------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/googleapis/src/main/java/io/grpc/googleapis/GoogleCloudToProdNameResolver.java b/googleapis/src/main/java/io/grpc/googleapis/GoogleCloudToProdNameResolver.java index 24eea542bf9..1db4825ccb8 100644 --- a/googleapis/src/main/java/io/grpc/googleapis/GoogleCloudToProdNameResolver.java +++ b/googleapis/src/main/java/io/grpc/googleapis/GoogleCloudToProdNameResolver.java @@ -157,34 +157,34 @@ class Resolve implements Runnable { @Override public void run() { ImmutableMap rawBootstrap = null; - // User provided bootstrap configs are only supported with federation. If federation is - // not enabled or there is no user provided config, we set a custom bootstrap override. - // Otherwise, we don't set the override, which will allow a user provided bootstrap config - // to take effect. - if (!enableFederation || !xdsBootstrapProvided) { - try { + try { + // User provided bootstrap configs are only supported with federation. If federation is + // not enabled or there is no user provided config, we set a custom bootstrap override. + // Otherwise, we don't set the override, which will allow a user provided bootstrap config + // to take effect. + if (!enableFederation || !xdsBootstrapProvided) { rawBootstrap = generateBootstrap(queryZoneMetadata(METADATA_URL_ZONE), queryIpv6SupportMetadata(METADATA_URL_SUPPORT_IPV6)); - } catch (IOException e) { - listener.onError( - Status.INTERNAL.withDescription("Unable to get metadata").withCause(e)); } - } - - final ImmutableMap finalRawBootstrap = rawBootstrap; - syncContext.execute(new Runnable() { - @Override - public void run() { - if (!shutdown) { - if (finalRawBootstrap != null) { - bootstrapSetter.setBootstrap(finalRawBootstrap); + } catch (IOException e) { + listener.onError( + Status.INTERNAL.withDescription("Unable to get metadata").withCause(e)); + } finally { + final ImmutableMap finalRawBootstrap = rawBootstrap; + syncContext.execute(new Runnable() { + @Override + public void run() { + if (!shutdown) { + if (finalRawBootstrap != null) { + bootstrapSetter.setBootstrap(finalRawBootstrap); + } + delegate.start(listener); + succeeded = true; } - delegate.start(listener); - succeeded = true; + resolving = false; } - resolving = false; - } - }); + }); + } } }