Fix BackupS3BlobCorrectness.toml to work with MockS3Server

fdbclient/S3BlobStore.actor.cpp

@@ -199,6 +199,11 @@ std::string S3BlobStoreEndpoint::BlobKnobs::getURLParameters() const {
 }
 
 std::string guessRegionFromDomain(std::string domain) {
+	// Special case for localhost/127.0.0.1 to prevent basic_string exception
+	if (domain == "127.0.0.1" || domain == "localhost") {
+		return "us-east-1";
+	}
+
 	static const std::vector<const char*> knownServices = { "s3.", "cos.", "oss-", "obs." };
 	boost::algorithm::to_lower(domain);
 
@@ -843,6 +848,10 @@ ACTOR Future<S3BlobStoreEndpoint::ReusableConnection> connect_impl(Reference<S3B
 	} else {
 		wait(store(conn, INetworkConnections::net()->connect(host, service, isTLS)));
 	}
+
+	// Ensure connection is valid before handshake
+	ASSERT(conn.isValid());
+
 	wait(conn->connectHandshake());
 
 	TraceEvent("S3BlobStoreEndpointNewConnectionSuccess")
@@ -2441,4 +2450,4 @@ TEST_CASE("/backup/s3/guess_region") {
 		ASSERT_EQ(e.code(), error_code_backup_invalid_url);
 	}
 	return Void();
-}
+}

fdbserver/MockS3Server.actor.cpp

@@ -207,6 +207,14 @@ class MockS3ServerImpl {
 			}
 		}
 
+		// MockS3Server handles S3 HTTP requests where bucket is always the first path component
+		// For bucket operations: HEAD /bucket_name
+		// For object operations: HEAD /bucket_name/object_path
+		if (bucket.empty()) {
+			TraceEvent(SevError, "MockS3MissingBucketInPath").detail("Resource", resource).detail("QueryString", query);
+			throw backup_invalid_url();
+		}
+
 		TraceEvent("MockS3ParsedPath")
 		    .detail("OriginalResource", resource)
 		    .detail("Bucket", bucket)
@@ -956,3 +964,166 @@ ACTOR Future<Void> startMockS3Server(NetworkAddress listenAddress) {
 
 	return Void();
 }
+
+// Unit Tests for MockS3Server
+TEST_CASE("/fdbserver/MockS3Server/parseS3Request/ValidBucketParameter") {
+	MockS3ServerImpl server;
+	std::string resource = "/test?bucket=mybucket&region=us-east-1";
+	std::string bucket, object;
+	std::map<std::string, std::string> queryParams;
+
+	server.parseS3Request(resource, bucket, object, queryParams);
+
+	ASSERT(bucket == "mybucket");
+	ASSERT(object == "");
+	ASSERT(queryParams["bucket"] == "mybucket");
+	ASSERT(queryParams["region"] == "us-east-1");
+
+	return Void();
+}
+
+TEST_CASE("/fdbserver/MockS3Server/parseS3Request/MissingBucketParameter") {
+	MockS3ServerImpl server;
+	std::string resource = "/test?region=us-east-1";
+	std::string bucket, object;
+	std::map<std::string, std::string> queryParams;
+
+	try {
+		server.parseS3Request(resource, bucket, object, queryParams);
+		ASSERT(false); // Should not reach here
+	} catch (Error& e) {
+		ASSERT(e.code() == error_code_backup_invalid_url);
+	}
+
+	return Void();
+}
+
+TEST_CASE("/fdbserver/MockS3Server/parseS3Request/EmptyQueryString") {
+	MockS3ServerImpl server;
+	std::string resource = "/test";
+	std::string bucket, object;
+	std::map<std::string, std::string> queryParams;
+
+	try {
+		server.parseS3Request(resource, bucket, object, queryParams);
+		ASSERT(false); // Should not reach here
+	} catch (Error& e) {
+		ASSERT(e.code() == error_code_backup_invalid_url);
+	}
+
+	return Void();
+}
+
+TEST_CASE("/fdbserver/MockS3Server/parseS3Request/BucketParameterOverride") {
+	MockS3ServerImpl server;
+	std::string resource = "/pathbucket/testobject?bucket=querybucket&region=us-east-1";
+	std::string bucket, object;
+	std::map<std::string, std::string> queryParams;
+
+	server.parseS3Request(resource, bucket, object, queryParams);
+
+	ASSERT(bucket == "querybucket"); // Should use query parameter, not path
+	ASSERT(object == "testobject");
+	ASSERT(queryParams["bucket"] == "querybucket");
+	ASSERT(queryParams["region"] == "us-east-1");
+
+	return Void();
+}
+
+TEST_CASE("/fdbserver/MockS3Server/parseS3Request/ComplexPath") {
+	MockS3ServerImpl server;
+	std::string resource = "/pathbucket/folder/subfolder/file.txt?bucket=querybucket&region=us-east-1";
+	std::string bucket, object;
+	std::map<std::string, std::string> queryParams;
+
+	server.parseS3Request(resource, bucket, object, queryParams);
+
+	ASSERT(bucket == "querybucket"); // Should use query parameter
+	ASSERT(object == "folder/subfolder/file.txt");
+	ASSERT(queryParams["bucket"] == "querybucket");
+	ASSERT(queryParams["region"] == "us-east-1");
+
+	return Void();
+}
+
+TEST_CASE("/fdbserver/MockS3Server/parseS3Request/URLEncodedParameters") {
+	MockS3ServerImpl server;
+	std::string resource = "/test?bucket=my%20bucket&region=us-east-1&param=value%3Dtest";
+	std::string bucket, object;
+	std::map<std::string, std::string> queryParams;
+
+	server.parseS3Request(resource, bucket, object, queryParams);
+
+	ASSERT(bucket == "my bucket"); // Should be URL decoded
+	ASSERT(queryParams["bucket"] == "my bucket");
+	ASSERT(queryParams["region"] == "us-east-1");
+	ASSERT(queryParams["param"] == "value=test"); // Should be URL decoded
+
+	return Void();
+}
+
+TEST_CASE("/fdbserver/MockS3Server/parseS3Request/EmptyPath") {
+	MockS3ServerImpl server;
+	std::string resource = "/?bucket=mybucket&region=us-east-1";
+	std::string bucket, object;
+	std::map<std::string, std::string> queryParams;
+
+	server.parseS3Request(resource, bucket, object, queryParams);
+
+	ASSERT(bucket == "mybucket");
+	ASSERT(object == "");
+	ASSERT(queryParams["bucket"] == "mybucket");
+	ASSERT(queryParams["region"] == "us-east-1");
+
+	return Void();
+}
+
+TEST_CASE("/fdbserver/MockS3Server/parseS3Request/OnlyBucketInPath") {
+	MockS3ServerImpl server;
+	std::string resource = "/pathbucket?bucket=querybucket&region=us-east-1";
+	std::string bucket, object;
+	std::map<std::string, std::string> queryParams;
+
+	server.parseS3Request(resource, bucket, object, queryParams);
+
+	ASSERT(bucket == "querybucket"); // Should use query parameter
+	ASSERT(object == "");
+	ASSERT(queryParams["bucket"] == "querybucket");
+	ASSERT(queryParams["region"] == "us-east-1");
+
+	return Void();
+}
+
+TEST_CASE("/fdbserver/MockS3Server/parseS3Request/MultipleParameters") {
+	MockS3ServerImpl server;
+	std::string resource = "/test?bucket=mybucket&region=us-east-1&version=1&encoding=utf8";
+	std::string bucket, object;
+	std::map<std::string, std::string> queryParams;
+
+	server.parseS3Request(resource, bucket, object, queryParams);
+
+	ASSERT(bucket == "mybucket");
+	ASSERT(queryParams["bucket"] == "mybucket");
+	ASSERT(queryParams["region"] == "us-east-1");
+	ASSERT(queryParams["version"] == "1");
+	ASSERT(queryParams["encoding"] == "utf8");
+	ASSERT(queryParams.size() == 4);
+
+	return Void();
+}
+
+TEST_CASE("/fdbserver/MockS3Server/parseS3Request/ParametersWithoutValues") {
+	MockS3ServerImpl server;
+	std::string resource = "/test?bucket=mybucket&flag&region=us-east-1";
+	std::string bucket, object;
+	std::map<std::string, std::string> queryParams;
+
+	server.parseS3Request(resource, bucket, object, queryParams);
+
+	ASSERT(bucket == "mybucket");
+	ASSERT(queryParams["bucket"] == "mybucket");
+	ASSERT(queryParams["flag"] == ""); // Parameter without value should be empty string
+	ASSERT(queryParams["region"] == "us-east-1");
+
+	return Void();
+}

fdbserver/workloads/BackupToBlob.actor.cpp

@@ -25,6 +25,7 @@
 #include "fdbserver/Knobs.h"
 #include "fdbserver/workloads/BlobStoreWorkload.h"
 #include "fdbserver/workloads/workloads.actor.h"
+#include "fdbserver/MockS3Server.h"
 #include "flow/actorcompiler.h" // This must be the last #include.
 
 struct BackupToBlobWorkload : TestWorkload {
@@ -49,7 +50,27 @@ struct BackupToBlobWorkload : TestWorkload {
 		backupURL = backupURLString;
 	}
 
-	Future<Void> setup(Database const& cx) override { return Void(); }
+	ACTOR static Future<Void> _setup(Database cx, BackupToBlobWorkload* self) {
+		if (self->clientId == 0) {
+			// Check if we're using a local mock server URL pattern
+			bool useMockS3 = self->backupURL.toString().find("127.0.0.1") != std::string::npos ||
+			                 self->backupURL.toString().find("localhost") != std::string::npos;
+
+			if (useMockS3 && g_network->isSimulated()) {
+				TraceEvent("BackupToBlob").detail("Phase", "Registering MockS3Server").detail("URL", self->backupURL);
+
+				// Register MockS3Server with IP address - simulation environment doesn't support hostname resolution.
+				wait(g_simulator->registerSimHTTPServer("127.0.0.1", "8080", makeReference<MockS3RequestHandler>()));
+
+				TraceEvent("BackupToBlob")
+				    .detail("Phase", "MockS3Server Registered")
+				    .detail("Address", "127.0.0.1:8080");
+			}
+		}
+		return Void();
+	}
+
+	Future<Void> setup(Database const& cx) override { return _setup(cx, this); }
 
 	ACTOR static Future<Void> _start(Database cx, BackupToBlobWorkload* self) {
 		state FileBackupAgent backupAgent;

fdbserver/workloads/S3ClientWorkload.actor.cpp

@@ -48,7 +48,6 @@ struct S3ClientWorkload : TestWorkload {
 	std::string s3Url;
 	std::string credentials;
 	std::string simfdbDir;
-
 	S3ClientWorkload(WorkloadContext const& wcx) : TestWorkload(wcx), enabled(true), pass(true) {
 		s3Url = getOption(options, "s3Url"_sr, ""_sr).toString();
 		if (s3Url.empty()) {
@@ -84,7 +83,7 @@ struct S3ClientWorkload : TestWorkload {
 		// Write the credentials file content -- hardcoded and nonsense for now. It just needs to be present.
 		writeFile(
 		    credentials,
-		    "{\"accounts\":{\"@host\":{\"api_key\":\"seaweedfs\",\"secret\":\"tot4llys3cure\",\"token\":\"TOKEN\"}}}");
+		    "{\"accounts\":{\"@host\":{\"api_key\":\"mocks3\",\"secret\":\"mocksecret\",\"token\":\"mocktoken\"}}}");
 
 		// Set the credentials file path into the global network configuration
 		auto* blobCredFiles = (std::vector<std::string>*)g_network->global(INetwork::enBlobCredentialFiles);
@@ -115,7 +114,8 @@ struct S3ClientWorkload : TestWorkload {
 
 			// Get the current path
 			std::string currentPath = baseUrl.substr(hostEnd, queryStart - hostEnd);
-			if (!currentPath.empty() && currentPath.back() != '/') {
+			// Ensure there's always a path separator
+			if (currentPath.empty() || currentPath.back() != '/') {
 				currentPath += '/';
 			}
 
@@ -136,6 +136,7 @@ struct S3ClientWorkload : TestWorkload {
 			// Only run one time workload in the simulation
 			return Void();
 		}
+
 		if (g_network->isSimulated()) {
 			// Network partition between CC and DD can cause DD no longer existing,
 			// which results in the bulk loading task cannot complete

tests/CMakeLists.txt

@@ -149,7 +149,7 @@ if(WITH_PYTHON)
   add_fdb_test(TEST_FILES fast/AuthzSecurityWithBlobGranules.toml)
   add_fdb_test(TEST_FILES fast/AutomaticIdempotency.toml)
   add_fdb_test(TEST_FILES fast/BackupAzureBlobCorrectness.toml IGNORE)
-  add_fdb_test(TEST_FILES fast/BackupS3BlobCorrectness.toml IGNORE)
+  add_fdb_test(TEST_FILES fast/BackupS3BlobCorrectness.toml)
   add_fdb_test(TEST_FILES fast/BackupCorrectness.toml)
   add_fdb_test(TEST_FILES fast/BackupCorrectnessWithEKPKeyFetchFailures.toml)
   add_fdb_test(TEST_FILES fast/BackupCorrectnessWithTenantDeletion.toml)
@@ -500,6 +500,7 @@ if(WITH_PYTHON)
   add_fdb_test(TEST_FILES slow/ApiCorrectnessSwitchover.toml)
   add_fdb_test(TEST_FILES slow/ApiCorrectnessWithConsistencyCheck.toml)
   add_fdb_test(TEST_FILES slow/BackupAndRestore.toml)
+
   add_fdb_test(TEST_FILES slow/BackupCorrectnessPartitioned.toml)
   add_fdb_test(TEST_FILES slow/BackupNewAndOldRestore.toml)
   add_fdb_test(TEST_FILES slow/BackupOldAndNewRestore.toml)