Add endpoint for REST version and capabilities. #5429

shaunrd0 · 2025-01-21T16:59:16Z

This adds the REST capabilities endpoint to RestClientRemote and serialization for the RestCapabilities response. The endpoint is currently not in use internally and there is no way to make the request using public APIs.

The RestCapabilities response currently provides the TileDB core version deployed to REST, and the minimum TileDB core client version supported by the REST server. The response may be updated in the future to add more information as needed - discussion in shortcut and design document

TYPE: FEATURE
DESC: Add endpoint for REST version and capabilities.

ypatia

Thank you for starting this! I left some first comments and let's discuss the next steps.

tiledb/sm/rest/rest_client_remote.cc

tiledb/sm/rest/curl.cc

ypatia · 2025-01-22T10:00:26Z

tiledb/sm/rest/curl.cc

@@ -369,6 +369,8 @@ Status Curl::set_headers(struct curl_slist** headers) const {
    std::string basic_auth = username + std::string(":") + password;
    curl_easy_setopt(curl, CURLOPT_HTTPAUTH, CURLAUTH_BASIC);
    curl_easy_setopt(curl, CURLOPT_USERPWD, basic_auth.c_str());
+  } else {
+    curl_easy_setopt(curl, CURLOPT_HTTPAUTH, CURLAUTH_NONE);


hmm. Why allow that? This doesn't look good or necessary. Why allow version checking to be unauthenticated?

The endpoint doesn't require authentication server side, all other endpoints currently supported by core do. If we make this request we could assume we plan to make requests to authenticated routes so the error is likely inevitable, on first pass it just seemed strange to require a token for an unauthenticated route.

I removed it and we now require the authentication to be configured, I don't think there is really a case where we would just make this request and not send any others. I'll bring this up on my REST PR as well to see if we want to require authentication on the REST side as well.

I'd go for requiring authentication on the REST side as well, but yes checking with REST experts might be a better idea. The only downside I can see is performance, but having a general rule that all client-server communication needs to be authenticated sounds more important to me.

ypatia · 2025-01-22T10:01:22Z

tiledb/sm/rest/curl.cc

@@ -956,6 +960,7 @@ Status Curl::get_data(

  CURLcode ret;
  headerData.uri = &res_ns_uri;
+  headerData.should_cache_redirect = res_ns_uri != "no-cache";


Is that because the version request will always be the first one to get to the server? It's worth at least a comment.

Sort of yeah, AFAICT this is the first endpoint supported by core that doesn't require an asset URI. We use the asset URI in Curl::write_header_callback to parse and cache redirect URIs. This was quite hacky though, it was really just for POC.

There was already a similar boolean flag in Curl::init to disable this so I updated to use that instead. Without an asset URI we need to disable the caching done on the URI or we fail the curl request after returning 0 here: https://github.com/tiledb-inc/tiledb/blob/286da187ee9a3bc9d66538ce2ed433cc2c8b759a/tiledb/sm/rest/curl.cc#L167

The logging looks like this when you set TILEDB_CONFIG_LOGGING_LEVEL=5, we end up in a retry loop after returning 0 in the callback linked above (Failure writing output to destination, passed 17 returned 0)

[2025-01-22 15:47:08.313] [Process: 2121225] [error] [1737578828284111920-Global] Rest components as array_ns and array_uri cannot be empty [2025-01-22 15:47:08.313] [Process: 2121225] [trace] [1737578828261932792-Context: 2] [curl : 2] [curl : 1] OP=CORE-TO-REST,SECONDS=0.001,RETRY=0,CODE=200,URL=127.0.0.1:8181/v4/version [2025-01-22 15:47:08.313] [Process: 2121225] [debug] [1737578828284111920-Global] Request to 127.0.0.1:8181/v4/version failed with Curl error message "Failure writing output to destination, passed 17 returned 0", will sleep 500ms, retry count 0

tiledb/sm/serialization/rest_version.cc

test/src/unit-ctx.cc

ypatia · 2025-01-22T10:32:23Z

Also I guess the UT can't go in as part of this PR, but will have to be moved to the next one that will come after the REST server has moved the libtiledb (minor) release that includes this PR. Hopefully for the last time 😅

test/src/unit-ctx.cc

tiledb/sm/rest/rest_capabilities.cc

tiledb/sm/rest/rest_capabilities.h

tiledb/sm/rest/rest_client.h

tiledb/sm/rest/rest_client_remote.cc

tiledb/sm/serialization/rest_version.h

tiledb/sm/rest/rest_client.h

ypatia

A few more comments but overall it looks good!

tiledb/sm/serialization/tiledb-rest.capnp

tiledb/sm/rest/rest_client.h

ypatia · 2025-01-23T09:56:46Z

tiledb/sm/rest/rest_capabilities.h

+class RestClientRemote;
+
+class RestCapabilities {
+  friend RestClientRemote;


Since we have accessors for all private arguments, how about making this a struct as well and simplify things?

Well, I guess what I meant is that once we make this a struct , we no longer need friend or getters for the members. If you think it's important that the member variables are not set outside of the struct or its friends you can revert this to a class to preserve those semantics.

Ah gotcha, this should work just fine since RestClientRemote returns only a const reference of the struct. I kept the RestClientRemote::rest_tiledb_version() and similar accessors in RestClientRemote for convenience, but I removed accessors from the struct itself since they are not needed.

tiledb/sm/rest/rest_capabilities.h

tiledb/sm/rest/rest_client_remote.cc

ypatia · 2025-01-23T13:35:05Z

tiledb/sm/rest/curl.cc

@@ -369,6 +369,8 @@ Status Curl::set_headers(struct curl_slist** headers) const {
    std::string basic_auth = username + std::string(":") + password;
    curl_easy_setopt(curl, CURLOPT_HTTPAUTH, CURLAUTH_BASIC);
    curl_easy_setopt(curl, CURLOPT_USERPWD, basic_auth.c_str());
+  } else {
+    curl_easy_setopt(curl, CURLOPT_HTTPAUTH, CURLAUTH_NONE);


I'd go for requiring authentication on the REST side as well, but yes checking with REST experts might be a better idea. The only downside I can see is performance, but having a general rule that all client-server communication needs to be authenticated sounds more important to me.

tiledb/sm/rest/curl.cc

tiledb/sm/serialization/rest_capabilities.cc

Co-authored-by: Theodore Tsirpanis <[email protected]>

+ Can be added back once REST used in CI supports the endpoint.

shaunrd0 · 2025-01-28T21:12:28Z

This is ready for review, a few notes for reviewers -

The endpoint does not and will not exist in legacy REST, so a REST CI test here will fail until REST CI is updated to pull from the latest REST repository SC-62628
The request won't be made until we internally make a call to RestClientRemote::get_capabilities_from_rest. At this time there is no usage of this method, so IIUC this can either go into 2.27.1 or 2.28.0 based on preference / need for a 2.27.1 patch release (cc @ihnorton) There is no way to manually make this request using public APIs.

ypatia

Some last comments and I'll approve. Let's also add a link to the design doc in the PR description.

tiledb/sm/rest/rest_capabilities.h

ypatia · 2025-01-29T09:40:15Z

tiledb/sm/rest/rest_capabilities.h

+class RestClientRemote;
+
+class RestCapabilities {
+  friend RestClientRemote;


Well, I guess what I meant is that once we make this a struct , we no longer need friend or getters for the members. If you think it's important that the member variables are not set outside of the struct or its friends you can revert this to a class to preserve those semantics.

ypatia reviewed Jan 22, 2025

View reviewed changes

teo-tsirpanis reviewed Jan 22, 2025

View reviewed changes

test/src/unit-ctx.cc Outdated Show resolved Hide resolved

test/src/unit-ctx.cc Outdated Show resolved Hide resolved

shaunrd0 force-pushed the smr/sc-61403/rest-version-endpoint branch from ebcebce to feef2fb Compare January 22, 2025 21:43

teo-tsirpanis reviewed Jan 22, 2025

View reviewed changes

ypatia reviewed Jan 23, 2025

View reviewed changes

shaunrd0 force-pushed the smr/sc-61403/rest-version-endpoint branch 4 times, most recently from f05f1d0 to fc3332c Compare January 27, 2025 21:25

teo-tsirpanis reviewed Jan 28, 2025

View reviewed changes

tiledb/sm/serialization/rest_capabilities.cc Outdated Show resolved Hide resolved

shaunrd0 and others added 17 commits January 28, 2025 14:51

REST version endpoint WIP.

5699b42

Add untracked files.

03bbd4e

Add REST CI test.

3f890c2

Submit unauthenticated request.

d8a54c4

Don't attempt to cache with no asset URI.

63df0c9

Update models and tests.

4b60f9d

Untracked files.

c004931

Changes from review.

f11c152

Update endpoint name.

46896c2

Changes from review.

d7eecd4

Regenerate capnp.

0b57e9b

Update testing.

1f0816b

Update tiledb/sm/serialization/rest_capabilities.cc

708a5b2

Co-authored-by: Theodore Tsirpanis <[email protected]>

Fix CI builds.

65c5b89

Fix UT build with serialization disabled.

5d18c61

Format.

cae1f5b

Temporarily remove test.

790757f

+ Can be added back once REST used in CI supports the endpoint.

shaunrd0 force-pushed the smr/sc-61403/rest-version-endpoint branch from 7099ce3 to fc8044a Compare January 28, 2025 20:04

Clean up after renames.

8c67284

shaunrd0 force-pushed the smr/sc-61403/rest-version-endpoint branch from fc8044a to 8c67284 Compare January 28, 2025 20:57

shaunrd0 marked this pull request as ready for review January 28, 2025 21:11

shaunrd0 requested review from teo-tsirpanis and ypatia January 28, 2025 21:12

ypatia reviewed Jan 29, 2025

View reviewed changes

Remove accessors from struct.

b28995e

ypatia approved these changes Jan 30, 2025

View reviewed changes

shaunrd0 merged commit ca7bd07 into main Jan 30, 2025
60 checks passed

shaunrd0 deleted the smr/sc-61403/rest-version-endpoint branch January 30, 2025 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add endpoint for REST version and capabilities. #5429

Add endpoint for REST version and capabilities. #5429

shaunrd0 commented Jan 21, 2025 •

edited

Loading

ypatia left a comment •

edited

Loading

ypatia Jan 22, 2025

shaunrd0 Jan 22, 2025

ypatia Jan 23, 2025

ypatia Jan 22, 2025

shaunrd0 Jan 22, 2025 •

edited

Loading

ypatia commented Jan 22, 2025

ypatia left a comment

ypatia Jan 23, 2025

shaunrd0 Jan 24, 2025

ypatia Jan 29, 2025

shaunrd0 Jan 29, 2025

ypatia Jan 23, 2025

shaunrd0 commented Jan 28, 2025

ypatia left a comment

ypatia Jan 29, 2025

Add endpoint for REST version and capabilities. #5429

Add endpoint for REST version and capabilities. #5429

Conversation

shaunrd0 commented Jan 21, 2025 • edited Loading

ypatia left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaunrd0 Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

ypatia commented Jan 22, 2025

ypatia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaunrd0 commented Jan 28, 2025

ypatia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaunrd0 commented Jan 21, 2025 •

edited

Loading

ypatia left a comment •

edited

Loading

shaunrd0 Jan 22, 2025 •

edited

Loading