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

Continue localstack test harness development #111

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions tensorstore/kvstore/s3/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,16 @@ tensorstore_cc_test(
name = "s3_key_value_store_test",
size = "small",
srcs = ["s3_key_value_store_test.cc"],
# args = ["--localstack_binary=$(location @pypa_localstack//:localstack)"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is the idea, but I don't know enough bazel to reference the localstack script install in the python environment's bin directory. Perhaps I need to declare py_binary that depends on @pypa_localstack?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To do that you'd need a workspace for localstack with a py_binary, similar to what we have for storagebench:

https://github.com/google/tensorstore/blob/master/third_party/com_google_storagetestbench/workspace.bzl

Then reference it like here, similar to what the gcs_grpc_testbench_test:

args = ["--testbench_binary=$(location @com_google_storagetestbench//:rest_server)"],

I'd call the directory com_github_localstack or perhaps cloud_localstack.

args = ["--localstack_binary=/home/simon/venv/tensorstore/bin/localstack"],
data = ["@pypa_localstack//:localstack"],
tags = [
"manual",
"notap", # TODO: Enable before linking by default.
#"manual",
# "notap", # TODO: Enable before linking by default.
"skip-cmake", # localstack is python, which will not work in bazel_to_cmake.
],
deps = [
":s3",
":s3_credential_provider",
":s3_request_builder",
"//tensorstore:context",
Expand Down
62 changes: 8 additions & 54 deletions tensorstore/kvstore/s3/s3_key_value_store_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,15 @@ using ::tensorstore::internal_http::HttpResponse;
using ::tensorstore::internal_kvstore_s3::S3Credentials;
using ::tensorstore::internal_kvstore_s3::S3RequestBuilder;

ABSL_FLAG(std::string, localstack_binary, "", "Path to the localstack");
ABSL_FLAG(std::string, localstack_binary, "", "Path to the localstack binary");
ABSL_FLAG(std::string, localstack_endpoint, "http://localhost:4566", "Localstack endpoint");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a command line flag in localstack to set the port, or does it require a config file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like its configurable via environment variables

I guess its possible to try PickUnusedPortOrDie as discussed here: #91 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, if it's unset (and the env var is unset), that would be nice.


namespace {

static constexpr char kAwsAccessKeyId[] = "LSIAQAAAAAAVNCBMPNSG";
static constexpr char kAwsSecretKeyId[] = "localstackdontcare";
static constexpr char kBucket[] = "testbucket";
static constexpr char kAwsRegion[] = "af-south-1";
static constexpr char kLocalStackEndpoint[] = "http://localhost:4566";
/// sha256 hash of an empty string
static constexpr char kEmptySha256[] =
"e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855";
Expand All @@ -78,7 +78,7 @@ class LocalStackProcess {

void SpawnProcess() {
if (running) return;
ABSL_LOG(INFO) << "Spawning localstack: " << endpoint_url();
ABSL_LOG(INFO) << "Spawning localstack: " << absl::GetFlag(FLAGS_localstack_endpoint);
{
SubprocessOptions options{absl::GetFlag(FLAGS_localstack_binary),
{"start", "-d"}};
Expand All @@ -93,7 +93,7 @@ class LocalStackProcess {

void StopProcess() {
if (!running) return;
ABSL_LOG(INFO) << "Shutting localstack down: " << endpoint_url();
ABSL_LOG(INFO) << "Shutting localstack down: " << absl::GetFlag(FLAGS_localstack_endpoint);
{
SubprocessOptions options{absl::GetFlag(FLAGS_localstack_binary),
{"stop"}};
Expand All @@ -113,7 +113,7 @@ class LocalStackProcess {
kAwsRegion)};

auto request =
S3RequestBuilder("PUT", endpoint_url())
S3RequestBuilder("PUT", absl::GetFlag(FLAGS_localstack_endpoint))
.BuildRequest(absl::StrFormat("%s.s3.amazonaws.com", kBucket),
S3Credentials{}, kAwsRegion, kEmptySha256,
absl::Now());
Expand All @@ -135,24 +135,15 @@ class LocalStackProcess {
}
}

std::string endpoint_url() { return kLocalStackEndpoint; }

bool running = false;
std::optional<Subprocess> child;
};

class LocalStackFixture : public ::testing::Test {
protected:
static LocalStackProcess process;
// Environment variables to save and restore during setup and teardown
static std::map<std::string, std::optional<std::string>> saved_vars;

static void SetUpTestSuite() {
for (auto &pair : saved_vars) {
pair.second = GetEnv(pair.first.c_str());
UnsetEnv(pair.first.c_str());
}

SetEnv("AWS_ACCESS_KEY_ID", kAwsAccessKeyId);
SetEnv("AWS_SECRET_KEY_ID", kAwsSecretKeyId);

Expand All @@ -164,22 +155,11 @@ class LocalStackFixture : public ::testing::Test {

static void TearDownTestSuite() {
process.StopProcess();

for (auto &pair : saved_vars) {
if (pair.second) {
SetEnv(pair.first.c_str(), pair.second.value().c_str());
}
}
}
};

LocalStackProcess LocalStackFixture::process;

std::map<std::string, std::optional<std::string>> LocalStackFixture::saved_vars{
{"AWS_ACCESS_KEY_ID", std::nullopt},
{"AWS_SECRET_ACCESS_KEY", std::nullopt},
};

Context DefaultTestContext() {
// Opens the s3 driver with small exponential backoff values.
return Context{Context::Spec::FromJson({{"s3_request_retries",
Expand All @@ -197,7 +177,7 @@ TEST_F(LocalStackFixture, Basic) {
{{"aws_region", kAwsRegion},
{"driver", "s3"},
{"bucket", kBucket},
{"endpoint", process.endpoint_url()},
{"endpoint", absl::GetFlag(FLAGS_localstack_endpoint)},
{"host", absl::StrFormat("%s.s3.%s.localstack.localhost.com",
kBucket, kAwsRegion)},
{"path", "tensorstore/test/"}},
Expand All @@ -210,39 +190,13 @@ TEST_F(LocalStackFixture, Basic) {
{{"aws_region", kAwsRegion},
{"driver", "s3"},
{"bucket", kBucket},
{"endpoint", process.endpoint_url()},
{"endpoint", absl::GetFlag(FLAGS_localstack_endpoint)},
{"host", absl::StrFormat("%s.s3.%s.localstack.localhost.com",
kBucket, kAwsRegion)},
{"path", "tensorstore/test/"},
{"profile", "default"},
{"requester_pays", false}})));
{"profile", "default"}})));

tensorstore::internal::TestKeyValueReadWriteOps(store);
}

TEST(S3KeyValueStoreTest, BadBucketNames) {
auto context = DefaultTestContext();
for (auto bucket : {"a", "_abc", "abc_", "ABC", "a..b", "a.-.b"}) {
EXPECT_FALSE(kvstore::Open({{"driver", "s3"},
{"bucket", bucket},
{"endpoint", "https://i.dont.exist"}},
context)
.result())
<< "bucket: " << bucket;
}
for (auto bucket :
{"abc", "abc.1-2-3.abc",
"a."
"0123456789123456789012345678912345678901234567891234567890"
"1234567891234567890123456789123456789012345678912345678901"
"23456789123456789.B"}) {
EXPECT_TRUE(kvstore::Open({{"driver", "s3"},
{"bucket", bucket},
{"endpoint", "https://i.dont.exist"}},
context)
.result())
<< "bucket: " << bucket;
}
}

} // namespace
18 changes: 17 additions & 1 deletion tensorstore/kvstore/s3/validate_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,17 @@ TEST(ValidateTest, ClassifyBucketName) {
EXPECT_EQ(ClassifyBucketName("sthree-configurator.bucket"),
BucketNameType::kInvalid);

// Standard bucket names are < 63 characters
// Standard bucket names are < 63 characters
Copy link
Collaborator

Choose a reason for hiding this comment

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

Split this test change into a separate pull request, please.

EXPECT_EQ(ClassifyBucketName("1234567890abcdefghij"
"1234567890abcdefghij"
"1234567890abcdefghij"
"123"),
BucketNameType::kStandard);

// Old us-east-1 style buckets allow uppercase and underscores.
EXPECT_EQ(ClassifyBucketName("ABC"), BucketNameType::kOldUSEast1);


EXPECT_EQ(ClassifyBucketName("1234567890abcdefghij"
"1234567890abcdefghij"
"1234567890abcdefghij"
Expand All @@ -62,38 +65,51 @@ TEST(ValidateTest, ClassifyBucketName) {
}

TEST(ValidateTest, IsValidBucketName) {
EXPECT_TRUE(IsValidBucketName("abc"));
EXPECT_TRUE(IsValidBucketName("foo"));
EXPECT_TRUE(IsValidBucketName("a.b"));
EXPECT_TRUE(IsValidBucketName("a-1"));
EXPECT_TRUE(IsValidBucketName("fo1.a-b"));
EXPECT_TRUE(IsValidBucketName("abc.1-2-3.abc"));

// No IP Address style names
EXPECT_FALSE(IsValidBucketName("192.168.5.4"));

// Invalid prefixes and suffixes
EXPECT_FALSE(IsValidBucketName("_abc"));
EXPECT_FALSE(IsValidBucketName("abc_"));
EXPECT_FALSE(IsValidBucketName("xn--bucket"));
EXPECT_FALSE(IsValidBucketName("bucket-s3alias"));
EXPECT_FALSE(IsValidBucketName("bucket--ol-s3"));
EXPECT_FALSE(IsValidBucketName("sthree-bucket"));
EXPECT_FALSE(IsValidBucketName("sthree-configurator.bucket"));

EXPECT_FALSE(IsValidBucketName("foo$bar"));
EXPECT_FALSE(IsValidBucketName("a")); // <3
EXPECT_FALSE(IsValidBucketName(".")); // <3
EXPECT_FALSE(IsValidBucketName("..")); // <3
EXPECT_FALSE(IsValidBucketName("aa")); // <3
EXPECT_FALSE(IsValidBucketName("-foo")); // not number or letter.
EXPECT_FALSE(IsValidBucketName("foo-")); // not number or letter.
EXPECT_FALSE(IsValidBucketName("a..b")); // consecutive dots
EXPECT_FALSE(IsValidBucketName("a."));
EXPECT_FALSE(IsValidBucketName("foo..bar"));
EXPECT_FALSE(IsValidBucketName("foo.-bar"));
EXPECT_FALSE(IsValidBucketName("a.-.b")); // Needs alphanumeric after .

// if a bucket has uppercase characters, then it is an
// old style US-East-1 bucket created before 2018.
EXPECT_TRUE(
IsValidBucketName("1234567890b123456789012345678901234567890"
"1234567890b123456789012345678901234567890"
"ABCD_EFGH"));
EXPECT_TRUE(IsValidBucketName("ABC"));
EXPECT_TRUE(IsValidBucketName(
"0123456789123456789012345678912345678901234567891234567890"
"1234567891234567890123456789123456789012345678912345678901"
"23456789123456789.B"));

EXPECT_TRUE(IsValidBucketName("ABC"));
EXPECT_TRUE(IsValidBucketName("ABCD_EFGH"));
EXPECT_TRUE(IsValidBucketName("abcd_efgh"));
}
Expand Down
2 changes: 1 addition & 1 deletion third_party/pypa/build_requirements_frozen.txt
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
# DO NOT EDIT: Generated from build_requirements.txt by generate_workspace.py
numpy==1.24.3
numpy==1.24.4
2 changes: 1 addition & 1 deletion third_party/pypa/cibuildwheel_requirements_frozen.txt
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
# DO NOT EDIT: Generated from cibuildwheel_requirements.txt by generate_workspace.py
cibuildwheel==2.12.3
cibuildwheel==2.15.0
8 changes: 4 additions & 4 deletions third_party/pypa/docs_requirements_frozen.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# DO NOT EDIT: Generated from docs_requirements.txt by generate_workspace.py
sphinx==6.0.1
sphinx-immaterial==0.11.3
jsonschema==4.17.3
pyyaml==6.0
markupsafe==2.1.2
sphinx-immaterial==0.11.7
jsonschema==4.19.0
pyyaml==6.0.1
markupsafe==2.1.3
2 changes: 1 addition & 1 deletion third_party/pypa/doctest_requirements_frozen.txt
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
# DO NOT EDIT: Generated from doctest_requirements.txt by generate_workspace.py
yapf==0.33.0
yapf==0.40.1
2 changes: 1 addition & 1 deletion third_party/pypa/examples_requirements_frozen.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
# DO NOT EDIT: Generated from examples_requirements.txt by generate_workspace.py
apache-beam==2.47.0
apache-beam==2.50.0
gin-config==0.5.0
9 changes: 8 additions & 1 deletion third_party/pypa/generate_workspace.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,14 @@ def _is_suitable_release(release):

def _find_suitable_version(name, j, spec):
releases = j["releases"]
versions = [(packaging.version.parse(v), v) for v in releases.keys()]
versions = []
for v in releases.keys():
try:
pv = packaging.version.parse(v)
except packaging.version.InvalidVersion:
continue # https://github.com/pypa/setuptools/issues/3772
else:
versions.append((pv, v))
versions.sort()
versions.reverse()
first_suitable = None
Expand Down
1 change: 1 addition & 0 deletions third_party/pypa/test_requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ crc32c
werkzeug
googleapis-common-protos
protobuf
localstack
17 changes: 9 additions & 8 deletions third_party/pypa/test_requirements_frozen.txt
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
# DO NOT EDIT: Generated from test_requirements.txt by generate_workspace.py
numpy==1.24.3
pytest==7.3.1
pytest-asyncio==0.21.0
numpy==1.24.4
pytest==7.4.1
pytest-asyncio==0.21.1
cloudpickle==2.2.1
grpcio==1.54.2
flask==2.3.2
grpcio==1.57.0
flask==2.3.3
requests-toolbelt==1.0.0
scalpl==0.4.2
crc32c==2.3.post0
werkzeug==2.3.4
googleapis-common-protos==1.59.0
protobuf==4.23.0
werkzeug==2.3.7
googleapis-common-protos==1.60.0
protobuf==4.24.2
localstack==2.2.0
2 changes: 1 addition & 1 deletion third_party/pypa/wheel_requirements_frozen.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
# DO NOT EDIT: Generated from wheel_requirements.txt by generate_workspace.py
wheel==0.40.0
wheel==0.41.2
setuptools_scm==7.1.0