Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update endpoint discovery resolver to handle explicit service URLs #3367

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

dscpinheiro
Copy link
Contributor

Fixes #3349

Motivation and Context

Timestream is one of two services (along with DynamoDB - see notes below) where the SDK attempts to perform dynamic endpoint discovery (important: this is not related to the endpoint resolution change introduced in version 3.7.100). A longer explanation is available in the developer guide, but in summary endpoint discovery works like this:

  1. If the requested operation requires endpoint discovery, the SDK calls the DescribeEndpoints API to fetch which endpoints customers should use (Timestream uses a cellular architecture so each customer is mapped to a specific cell)
  2. The SDK caches the returned endpoint and uses it for future invocations (refreshing it in the background)

One unusual thing about Timestream is that the DescribeEndpoints API can only be invoked via the regional endpoints (from usage notes). If customers create a VPC endpoint for Timestream and try to use its URL in the service client the operation fails (as the SDK attempts to perform endpoint discovery against an endpoint that doesn't support it).

var client = new AmazonTimestreamQueryClient(new AmazonTimestreamQueryConfig
{
    ServiceURL = "https://query-cell1.timestream.us-west-2.amazonaws.com"
});

// This will fail since the SDK attempts to call "DescribeEndpoints" behind the scenes.
await client.QueryAsync(...);

This PR updates the EndpointDiscoveryResolver not to attempt endpoint discovery if there's a service URL set in the current client config.

Testing

  • Dry-run: de91537f-92a8-4cc2-ba8a-2a93653ff0dd
  • Ran the console app mentioned in the original issue in a private EC2 instance (i.e. no internet access)

The outputs below are from the application setting the ServiceURL property; behaviour will not change when using RegionEndpoint instead.

Before:

Amazon Error: 4 : 
  AmazonClientException making request QueryRequest to https://query-cell1.timestream.us-west-2.amazonaws.com/. 
  Attempt 1., Amazon.Runtime.AmazonClientException: Failed to discover the endpoint for the request. 
  Requests will not succeed until an endpoint can be retrieved or an endpoint is manually specified.

Error querying Timestream: Failed to discover the endpoint for the request. 
Requests will not succeed until an endpoint can be retrieved or an endpoint is manually specified.

After (connection succeeded without attempting to call DescribeEndpoints):

Amazon Verbose: 0 : Received response (truncated to 1024 bytes): 
[
    {
        "ColumnInfo": [...],
        "QueryId": "",
        "QueryStatus": {},
        "Rows": [...]
    }
]

Notes

I couldn't find reports of the same issue happening with DynamoDB, my assumption is that endpoint discovery just isn't a common setup. DynamoDB also supports gateway endpoints, which in addition to being free of charge don't require setting a custom service URL (traffic is routed using prefix lists instead).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • My code follows the code style of this project
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed

License

  • I confirm that this pull request can be released under the Apache 2 license

@dscpinheiro dscpinheiro marked this pull request as ready for review July 2, 2024 13:27
// For example, in TimestreamQuery the "Query" API requires endpoint discovery, but if invoked through a VPC
// endpoint (i.e. "query-cell1.timestream.us-west-2.amazonaws.com") the "DescribeEndpoints" API performed by this resolver will fail.
// Original report: https://github.com/aws/aws-sdk-net/issues/3349
if (_isServiceUrlSet)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Using !string.IsNullOrEmpty(config.ServiceURL) here seems more straight forward instead of creating a property that isn't used anywhere else (yet).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, I ended up creating a new property since this resolver class wasn't saving the config object (and that seemed the simpler option).

@dscpinheiro dscpinheiro merged commit 2b13e41 into main-staging Jul 3, 2024
1 check passed
@dscpinheiro dscpinheiro deleted the dspin/gh-3349 branch July 3, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants