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

Remove support for Alternator DescribeEndpoints request #14410

Closed
nyh opened this issue Jun 27, 2023 · 3 comments
Closed

Remove support for Alternator DescribeEndpoints request #14410

nyh opened this issue Jun 27, 2023 · 3 comments
Assignees
Labels
area/alternator Alternator related Issues Backport candidate
Milestone

Comments

@nyh
Copy link
Contributor

nyh commented Jun 27, 2023

The short story is that although Alternator has some rudimentary support for the DescribeEndpoints request, I think it should be removed completely - or alternatively be made optional: A configuration option determines what value DescribeEndpoints should return, and if the configuration is unset, this request won't be supported.

It turns out that DynamoDB Local also doesn't support this request, so I think Alternator shouldn't either.

The long story is as follows:

We first saw this problem in the context of the new AWS DynamoDB Shell , which is written using the AWS C++ SDK, but the same problem may affect any application written with the AWS C++ SDK:

When you ask the C++ SDK to use a specific endpoint URL, e.g., http://localhost:8000, it turns out that the C++ SDK doesn't use this endpoint URL immediately, but rather on the first request it tries to run a DescribeEndpoints request on this URL, to possibly get a better endpoint. The relevant code in the C++ SDK source code looks like this (it's repeated many times in https://github.com/aws/aws-sdk-cpp/blob/main/generated/src/aws-cpp-sdk-dynamodb/source/DynamoDBClient.cpp):

      DescribeEndpointsRequest endpointRequest;
      auto endpointOutcome = DescribeEndpoints(endpointRequest);
      if (endpointOutcome.IsSuccess() && !endpointOutcome.GetResult().GetEndpoi
nts().empty())
      {
        const auto& item = endpointOutcome.GetResult().GetEndpoints()[0];
        ...
        endpoint = Aws::String(SchemeMapper::ToString(m_clientConfiguration.scheme)) + "://" + item.GetAddress();

The problem in this code is that no matter what the original endpoint URL was (in my case http://localhost:8000, the new endpoint built by this code will always use the default scheme ("https"), regardless of which scheme the original URL used. This will make this code try to use https://localhost:8000, which won't work because Alternator was listening on http, not https.

The "original sin" is that DescribeEndpoints is documented to return just the endpoint's "IP address", not a full URL. Although Alternator actually deviates from this and return an address and port - e.g., localhost:8000, it can't return the scheme of the URL, and in any case code such as the above just tacks on "https://" in front of the returned string, which is wrong.

There is no real way to fix this problem - it's a bug in Amazon's documentation (which doesn't say that DescribeEndpoints should return URLs) as well as the SDK implementations (this C++ SDK assumes the return value is not a URL).

The only way I can think of fixing this problem is to simply remove the DescribeEndpoints implementation. It turns out that DynamoDB Local doesn't support this request, so I would assume all official Amazon drivers will know how to handle this case. In the C++ SDK, if DescribeEndpoints doesn't work, the error is ignored and the request is sent to the original URL. In particular, if I comment out DescribeEndpoints handling in Alternator, the dynamodb-shell tool (written in the C++ SDK) starts to work.

Instead outright removing this request, we can also make it configurable - the configuration variable can list a specific string to return from DescribeEndpoints (or if missing, the operation won't be supported). Of course setting this will only make sense if the Scylla installation uses the default scheme (https).

I also reported this issue to the C++ SDK developers: aws/aws-sdk-cpp#2554

@nyh nyh added the area/alternator Alternator related Issues label Jun 27, 2023
@nyh nyh self-assigned this Jun 27, 2023
@nyh
Copy link
Contributor Author

nyh commented Jun 28, 2023

I think I want to do the configuration-option solution described above instead of disabling this feature completely. It will allow us to have some reasonable behavior by default (probably just the behavior we have today) plus be able to immediately fix a problem that a user might have with this feature - in the field without code changes.

nyh added a commit to nyh/scylla that referenced this issue Jun 28, 2023
The AWS C++ SDK has a bug (aws/aws-sdk-cpp#2554)
where even if a user specifies a specific enpoint URL, the SDK uses
DescribeEndpoints to try to "refresh" the endpoint. The problem is that
DescribeEndpoints can't return a scheme (http or https) and the SDK
arbitrarily picks https - making it unable to communicate with Alternator
over http. As an example, the new "dynamodb shell" (written in C++)
cannot communicate with Alternator running over http.

This patch adds a configuration option, "alternator_describe_endpoints",
which can be used to override what DescribeEndpoints does:

1. Empty string (the default) leaves the current behavior -
   DescribeEndpoints echos the request's "Host" header.

2. The string "disabled" disables the DescribeEndpoints (it will return
   an UnknownOperationException). This is how DynamoDB Local behaves,
   and the AWS C++ SDK and the Dynamodb Shell work well in this mode.

3. Any other string is a fixed string to be returned by DescribeEndpoints.
   It can be useful in setups that should return a known address.

Note that this patch does not, by default, change the current behaivor
of DescribeEndpoints. But it us the future to override its behavior
in a user experiences problems in the field - without code changes.

Fixes scylladb#14410.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
kbr-scylla pushed a commit that referenced this issue Jul 6, 2023
The AWS C++ SDK has a bug (aws/aws-sdk-cpp#2554)
where even if a user specifies a specific enpoint URL, the SDK uses
DescribeEndpoints to try to "refresh" the endpoint. The problem is that
DescribeEndpoints can't return a scheme (http or https) and the SDK
arbitrarily picks https - making it unable to communicate with Alternator
over http. As an example, the new "dynamodb shell" (written in C++)
cannot communicate with Alternator running over http.

This patch adds a configuration option, "alternator_describe_endpoints",
which can be used to override what DescribeEndpoints does:

1. Empty string (the default) leaves the current behavior -
   DescribeEndpoints echos the request's "Host" header.

2. The string "disabled" disables the DescribeEndpoints (it will return
   an UnknownOperationException). This is how DynamoDB Local behaves,
   and the AWS C++ SDK and the Dynamodb Shell work well in this mode.

3. Any other string is a fixed string to be returned by DescribeEndpoints.
   It can be useful in setups that should return a known address.

Note that this patch does not, by default, change the current behaivor
of DescribeEndpoints. But it us the future to override its behavior
in a user experiences problems in the field - without code changes.

Fixes #14410.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>

Closes #14432
@DoronArazii DoronArazii added this to the 5.4 milestone Aug 29, 2023
@nyh
Copy link
Contributor Author

nyh commented Aug 30, 2023

The bug in the AWS C++ SDK was apparently fixed, aws/aws-sdk-cpp#2554.
The fix they chose is to not call DescribeEndpoints at all when the endpoint was explicitly given.

So the fix we added (a configurable workaround) will not be necessary for new versions of the SDK, which suggests maybe it's not important to backport this fix - users can upgrade their SDK instead of Scylla.

@denesb
Copy link
Contributor

denesb commented Dec 15, 2023

@nyh does this need to be backported?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/alternator Alternator related Issues Backport candidate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants