Skip to content

Commit c2c0e28

Browse files
millemsmillems
committed
Updated endpoint discovery behavior for operations that require endpoint discovery.
Specifically, when endpoint discovery is required: 1. If endpoint discovery is disabled, throw an exception. Endpoint discovery is required. Disabling it should not be allowed. 2. If an endpoint override is specified, throw an exception. Endpoint discovery being required implies the endpoint MUST be discovered. 3. Introduce an "allowEndpointOverrideForEndpointDiscoveryRequiredOperations" customization flag that can be used by services that need to disable (2) before we can reach a more long-term state of consistency across the SDKs. This flag will treat the "endpointOverride" as the *discovered* endpoint. Motivation: 1. The behavior across the SDKs is inconsistent, so this brings the Java SDK more into line with the other SDKs. 2. This removes the ambiguity as to whether the "endpoint override" is the *discovery* endpoint or the *discovered* endpoint by just disallowing its use altogether. In the future we can add a "discoveryEndpointOverride" to codify "endpointOverride" as being the *discovered* endpoint.
1 parent c5ece8a commit c2c0e28

File tree

15 files changed

+684
-13
lines changed

15 files changed

+684
-13
lines changed

‎codegen/src/main/java/software/amazon/awssdk/codegen/model/config/customization/CustomizationConfig.java

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,13 @@ public class CustomizationConfig {
165165
*/
166166
private boolean enableEndpointDiscoveryMethodRequired = false;
167167

168+
/**
169+
* Allow a customer to set an endpoint override AND bypass endpoint discovery on their client even when endpoint discovery
170+
* enabled is true and endpoint discovery is required for an operation. This customization should almost never be "true"
171+
* because it creates a confusing customer experience.
172+
*/
173+
private boolean allowEndpointOverrideForEndpointDiscoveryRequiredOperations = false;
174+
168175
private CustomizationConfig() {
169176
}
170177

@@ -420,4 +427,14 @@ public boolean isEnableEndpointDiscoveryMethodRequired() {
420427
public void setEnableEndpointDiscoveryMethodRequired(boolean enableEndpointDiscoveryMethodRequired) {
421428
this.enableEndpointDiscoveryMethodRequired = enableEndpointDiscoveryMethodRequired;
422429
}
430+
431+
public boolean allowEndpointOverrideForEndpointDiscoveryRequiredOperations() {
432+
return allowEndpointOverrideForEndpointDiscoveryRequiredOperations;
433+
}
434+
435+
public void setAllowEndpointOverrideForEndpointDiscoveryRequiredOperations(
436+
boolean allowEndpointOverrideForEndpointDiscoveryRequiredOperations) {
437+
this.allowEndpointOverrideForEndpointDiscoveryRequiredOperations =
438+
allowEndpointOverrideForEndpointDiscoveryRequiredOperations;
439+
}
423440
}

‎codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/AsyncClientClass.java

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,17 @@ private MethodSpec constructor(Builder classBuilder) {
166166
EndpointDiscoveryRefreshCache.class,
167167
poetExtensions.getClientClass(model.getNamingStrategy().getServiceName() +
168168
"AsyncEndpointDiscoveryCacheLoader"));
169+
170+
if (model.getCustomizationConfig().allowEndpointOverrideForEndpointDiscoveryRequiredOperations()) {
171+
builder.beginControlFlow("if (clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == "
172+
+ "Boolean.TRUE)");
173+
builder.addStatement("log.warn($S)",
174+
"Endpoint discovery is enabled for this client, and an endpoint override was also "
175+
+ "specified. This will disable endpoint discovery for methods that require it, instead "
176+
+ "using the specified endpoint override. This may or may not be what you intended.");
177+
builder.endControlFlow();
178+
}
179+
169180
builder.endControlFlow();
170181
}
171182

@@ -220,8 +231,37 @@ protected MethodSpec.Builder operationBody(MethodSpec.Builder builder, Operation
220231
builder.addCode(eventToByteBufferPublisher(opModel));
221232

222233
if (opModel.getEndpointDiscovery() != null) {
234+
builder.addStatement("boolean endpointDiscoveryEnabled = "
235+
+ "clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED)");
236+
builder.addStatement("boolean endpointOverridden = "
237+
+ "clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == Boolean.TRUE");
238+
239+
if (opModel.getEndpointDiscovery().isRequired()) {
240+
if (!model.getCustomizationConfig().allowEndpointOverrideForEndpointDiscoveryRequiredOperations()) {
241+
builder.beginControlFlow("if (endpointOverridden)");
242+
builder.addStatement("throw new $T($S)", IllegalStateException.class,
243+
"This operation requires endpoint discovery, but an endpoint override was specified "
244+
+ "when the client was created. This is not supported.");
245+
builder.endControlFlow();
246+
247+
builder.beginControlFlow("if (!endpointDiscoveryEnabled)");
248+
builder.addStatement("throw new $T($S)", IllegalStateException.class,
249+
"This operation requires endpoint discovery, but endpoint discovery was disabled on the "
250+
+ "client.");
251+
builder.endControlFlow();
252+
} else {
253+
builder.beginControlFlow("if (endpointOverridden)");
254+
builder.addStatement("endpointDiscoveryEnabled = false");
255+
builder.nextControlFlow("else if (!endpointDiscoveryEnabled)");
256+
builder.addStatement("throw new $T($S)", IllegalStateException.class,
257+
"This operation requires endpoint discovery to be enabled, or for you to specify an "
258+
+ "endpoint override when the client is created.");
259+
builder.endControlFlow();
260+
}
261+
}
262+
223263
builder.addStatement("$T cachedEndpoint = null", URI.class);
224-
builder.beginControlFlow("if (clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED))");
264+
builder.beginControlFlow("if (endpointDiscoveryEnabled)");
225265
builder.addStatement("\n\nString key = clientConfiguration.option($T.CREDENTIALS_PROVIDER).resolveCredentials()" +
226266
".accessKeyId()", AwsClientOption.class);
227267
builder.addStatement("EndpointDiscoveryRequest endpointDiscoveryRequest = $T.builder().required($L)" +

‎codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/SyncClientClass.java

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@
6060
import software.amazon.awssdk.metrics.MetricCollector;
6161
import software.amazon.awssdk.metrics.MetricPublisher;
6262
import software.amazon.awssdk.metrics.NoOpMetricCollector;
63+
import software.amazon.awssdk.utils.Logger;
6364

6465
//TODO Make SyncClientClass extend SyncClientInterface (similar to what we do in AsyncClientClass)
6566
public class SyncClientClass implements ClassSpec {
@@ -86,6 +87,7 @@ public TypeSpec poetSpec() {
8687
.addSuperinterface(interfaceClass)
8788
.addJavadoc("Internal implementation of {@link $1T}.\n\n@see $1T#builder()",
8889
interfaceClass)
90+
.addField(logger())
8991
.addField(SyncClientHandler.class, "clientHandler", PRIVATE, FINAL)
9092
.addField(protocolSpec.protocolFactory(model))
9193
.addField(SdkClientConfiguration.class, "clientConfiguration", PRIVATE, FINAL)
@@ -119,6 +121,12 @@ public TypeSpec poetSpec() {
119121
return classBuilder.build();
120122
}
121123

124+
private FieldSpec logger() {
125+
return FieldSpec.builder(Logger.class, "log", PRIVATE, STATIC, FINAL)
126+
.initializer("$T.loggerFor($T.class)", Logger.class, className)
127+
.build();
128+
}
129+
122130
private MethodSpec nameMethod() {
123131
return MethodSpec.methodBuilder("serviceName")
124132
.addAnnotation(Override.class)
@@ -154,6 +162,17 @@ private MethodSpec constructor() {
154162
EndpointDiscoveryRefreshCache.class,
155163
poetExtensions.getClientClass(model.getNamingStrategy().getServiceName() +
156164
"EndpointDiscoveryCacheLoader"));
165+
166+
if (model.getCustomizationConfig().allowEndpointOverrideForEndpointDiscoveryRequiredOperations()) {
167+
builder.beginControlFlow("if (clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == "
168+
+ "Boolean.TRUE)");
169+
builder.addStatement("log.warn(() -> $S)",
170+
"Endpoint discovery is enabled for this client, and an endpoint override was also "
171+
+ "specified. This will disable endpoint discovery for methods that require it, instead "
172+
+ "using the specified endpoint override. This may or may not be what you intended.");
173+
builder.endControlFlow();
174+
}
175+
157176
builder.endControlFlow();
158177
}
159178

@@ -181,8 +200,37 @@ private List<MethodSpec> operationMethodSpecs(OperationModel opModel) {
181200
protocolSpec.errorResponseHandler(opModel).ifPresent(method::addCode);
182201

183202
if (opModel.getEndpointDiscovery() != null) {
203+
method.addStatement("boolean endpointDiscoveryEnabled = "
204+
+ "clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED)");
205+
method.addStatement("boolean endpointOverridden = "
206+
+ "clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == Boolean.TRUE");
207+
208+
if (opModel.getEndpointDiscovery().isRequired()) {
209+
if (!model.getCustomizationConfig().allowEndpointOverrideForEndpointDiscoveryRequiredOperations()) {
210+
method.beginControlFlow("if (endpointOverridden)");
211+
method.addStatement("throw new $T($S)", IllegalStateException.class,
212+
"This operation requires endpoint discovery, but an endpoint override was specified "
213+
+ "when the client was created. This is not supported.");
214+
method.endControlFlow();
215+
216+
method.beginControlFlow("if (!endpointDiscoveryEnabled)");
217+
method.addStatement("throw new $T($S)", IllegalStateException.class,
218+
"This operation requires endpoint discovery, but endpoint discovery was disabled on the "
219+
+ "client.");
220+
method.endControlFlow();
221+
} else {
222+
method.beginControlFlow("if (endpointOverridden)");
223+
method.addStatement("endpointDiscoveryEnabled = false");
224+
method.nextControlFlow("else if (!endpointDiscoveryEnabled)");
225+
method.addStatement("throw new $T($S)", IllegalStateException.class,
226+
"This operation requires endpoint discovery to be enabled, or for you to specify an "
227+
+ "endpoint override when the client is created.");
228+
method.endControlFlow();
229+
}
230+
}
231+
184232
method.addStatement("$T cachedEndpoint = null", URI.class);
185-
method.beginControlFlow("if (clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED))");
233+
method.beginControlFlow("if (endpointDiscoveryEnabled)");
186234
method.addStatement("\n\nString key = clientConfiguration.option($T.CREDENTIALS_PROVIDER)." +
187235
"resolveCredentials().accessKeyId()", AwsClientOption.class);
188236
method.addStatement("EndpointDiscoveryRequest endpointDiscoveryRequest = $T.builder().required($L)" +

‎codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/test-endpoint-discovery-async.java

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -167,8 +167,18 @@ public CompletableFuture<TestDiscoveryIdentifiersRequiredResponse> testDiscovery
167167

168168
HttpResponseHandler<AwsServiceException> errorResponseHandler = createErrorResponseHandler(protocolFactory,
169169
operationMetadata);
170+
boolean endpointDiscoveryEnabled = clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED);
171+
boolean endpointOverridden = clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == Boolean.TRUE;
172+
if (endpointOverridden) {
173+
throw new IllegalStateException(
174+
"This operation requires endpoint discovery, but an endpoint override was specified when the client was created. This is not supported.");
175+
}
176+
if (!endpointDiscoveryEnabled) {
177+
throw new IllegalStateException(
178+
"This operation requires endpoint discovery, but endpoint discovery was disabled on the client.");
179+
}
170180
URI cachedEndpoint = null;
171-
if (clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED)) {
181+
if (endpointDiscoveryEnabled) {
172182

173183
String key = clientConfiguration.option(AwsClientOption.CREDENTIALS_PROVIDER).resolveCredentials().accessKeyId();
174184
EndpointDiscoveryRequest endpointDiscoveryRequest = EndpointDiscoveryRequest.builder().required(true)
@@ -231,8 +241,10 @@ public CompletableFuture<TestDiscoveryOptionalResponse> testDiscoveryOptional(
231241

232242
HttpResponseHandler<AwsServiceException> errorResponseHandler = createErrorResponseHandler(protocolFactory,
233243
operationMetadata);
244+
boolean endpointDiscoveryEnabled = clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED);
245+
boolean endpointOverridden = clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == Boolean.TRUE;
234246
URI cachedEndpoint = null;
235-
if (clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED)) {
247+
if (endpointDiscoveryEnabled) {
236248

237249
String key = clientConfiguration.option(AwsClientOption.CREDENTIALS_PROVIDER).resolveCredentials().accessKeyId();
238250
EndpointDiscoveryRequest endpointDiscoveryRequest = EndpointDiscoveryRequest.builder().required(false)
@@ -295,8 +307,18 @@ public CompletableFuture<TestDiscoveryRequiredResponse> testDiscoveryRequired(
295307

296308
HttpResponseHandler<AwsServiceException> errorResponseHandler = createErrorResponseHandler(protocolFactory,
297309
operationMetadata);
310+
boolean endpointDiscoveryEnabled = clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED);
311+
boolean endpointOverridden = clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == Boolean.TRUE;
312+
if (endpointOverridden) {
313+
throw new IllegalStateException(
314+
"This operation requires endpoint discovery, but an endpoint override was specified when the client was created. This is not supported.");
315+
}
316+
if (!endpointDiscoveryEnabled) {
317+
throw new IllegalStateException(
318+
"This operation requires endpoint discovery, but endpoint discovery was disabled on the client.");
319+
}
298320
URI cachedEndpoint = null;
299-
if (clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED)) {
321+
if (endpointDiscoveryEnabled) {
300322

301323
String key = clientConfiguration.option(AwsClientOption.CREDENTIALS_PROVIDER).resolveCredentials().accessKeyId();
302324
EndpointDiscoveryRequest endpointDiscoveryRequest = EndpointDiscoveryRequest.builder().required(true)

‎codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/test-endpoint-discovery-sync.java

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import software.amazon.awssdk.services.endpointdiscoverytest.transform.TestDiscoveryIdentifiersRequiredRequestMarshaller;
3939
import software.amazon.awssdk.services.endpointdiscoverytest.transform.TestDiscoveryOptionalRequestMarshaller;
4040
import software.amazon.awssdk.services.endpointdiscoverytest.transform.TestDiscoveryRequiredRequestMarshaller;
41+
import software.amazon.awssdk.utils.Logger;
4142

4243
/**
4344
* Internal implementation of {@link EndpointDiscoveryTestClient}.
@@ -47,6 +48,8 @@
4748
@Generated("software.amazon.awssdk:codegen")
4849
@SdkInternalApi
4950
final class DefaultEndpointDiscoveryTestClient implements EndpointDiscoveryTestClient {
51+
private static final Logger log = Logger.loggerFor(DefaultEndpointDiscoveryTestClient.class);
52+
5053
private final SyncClientHandler clientHandler;
5154

5255
private final AwsJsonProtocolFactory protocolFactory;
@@ -139,8 +142,18 @@ public TestDiscoveryIdentifiersRequiredResponse testDiscoveryIdentifiersRequired
139142

140143
HttpResponseHandler<AwsServiceException> errorResponseHandler = createErrorResponseHandler(protocolFactory,
141144
operationMetadata);
145+
boolean endpointDiscoveryEnabled = clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED);
146+
boolean endpointOverridden = clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == Boolean.TRUE;
147+
if (endpointOverridden) {
148+
throw new IllegalStateException(
149+
"This operation requires endpoint discovery, but an endpoint override was specified when the client was created. This is not supported.");
150+
}
151+
if (!endpointDiscoveryEnabled) {
152+
throw new IllegalStateException(
153+
"This operation requires endpoint discovery, but endpoint discovery was disabled on the client.");
154+
}
142155
URI cachedEndpoint = null;
143-
if (clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED)) {
156+
if (endpointDiscoveryEnabled) {
144157

145158
String key = clientConfiguration.option(AwsClientOption.CREDENTIALS_PROVIDER).resolveCredentials().accessKeyId();
146159
EndpointDiscoveryRequest endpointDiscoveryRequest = EndpointDiscoveryRequest.builder().required(true)
@@ -191,8 +204,10 @@ public TestDiscoveryOptionalResponse testDiscoveryOptional(TestDiscoveryOptional
191204

192205
HttpResponseHandler<AwsServiceException> errorResponseHandler = createErrorResponseHandler(protocolFactory,
193206
operationMetadata);
207+
boolean endpointDiscoveryEnabled = clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED);
208+
boolean endpointOverridden = clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == Boolean.TRUE;
194209
URI cachedEndpoint = null;
195-
if (clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED)) {
210+
if (endpointDiscoveryEnabled) {
196211

197212
String key = clientConfiguration.option(AwsClientOption.CREDENTIALS_PROVIDER).resolveCredentials().accessKeyId();
198213
EndpointDiscoveryRequest endpointDiscoveryRequest = EndpointDiscoveryRequest.builder().required(false)
@@ -242,8 +257,18 @@ public TestDiscoveryRequiredResponse testDiscoveryRequired(TestDiscoveryRequired
242257

243258
HttpResponseHandler<AwsServiceException> errorResponseHandler = createErrorResponseHandler(protocolFactory,
244259
operationMetadata);
260+
boolean endpointDiscoveryEnabled = clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED);
261+
boolean endpointOverridden = clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == Boolean.TRUE;
262+
if (endpointOverridden) {
263+
throw new IllegalStateException(
264+
"This operation requires endpoint discovery, but an endpoint override was specified when the client was created. This is not supported.");
265+
}
266+
if (!endpointDiscoveryEnabled) {
267+
throw new IllegalStateException(
268+
"This operation requires endpoint discovery, but endpoint discovery was disabled on the client.");
269+
}
245270
URI cachedEndpoint = null;
246-
if (clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED)) {
271+
if (endpointDiscoveryEnabled) {
247272

248273
String key = clientConfiguration.option(AwsClientOption.CREDENTIALS_PROVIDER).resolveCredentials().accessKeyId();
249274
EndpointDiscoveryRequest endpointDiscoveryRequest = EndpointDiscoveryRequest.builder().required(true)

0 commit comments

Comments
 (0)