From c2f7ead468d5e65c57e7ecb49d7fbba4254c46b7 Mon Sep 17 00:00:00 2001 From: Paul Cristian Sarbu Date: Tue, 2 Jul 2024 16:47:13 +0200 Subject: Replace the Auth and Auth::TLS singletons Use a builder pattern for creation and validation, in a manner that allows also other authentication methods to be added in the future besides the current TLS/SSL. The main Auth instances are built early and then passed by not_null const pointers, to avoid passing temporaries, replacing the previous Auth::TLS instances passed by simple nullable const pointers. Where needed, these passed Auth instances are also stored, by const ref. Tests also build Auth instances as needed, either with the default 'no certification' or from the test environment arguments. --- test/buildtool/execution_engine/executor/TARGETS | 8 ++-- .../execution_engine/executor/executor.test.cpp | 28 +++++++---- .../executor/executor_api.test.hpp | 14 +++--- .../executor/executor_api_local.test.cpp | 43 +++++------------ .../executor/executor_api_remote_bazel.test.cpp | 55 +++++++++------------- 5 files changed, 63 insertions(+), 85 deletions(-) (limited to 'test/buildtool/execution_engine') diff --git a/test/buildtool/execution_engine/executor/TARGETS b/test/buildtool/execution_engine/executor/TARGETS index 0cca077d..df0f9df6 100644 --- a/test/buildtool/execution_engine/executor/TARGETS +++ b/test/buildtool/execution_engine/executor/TARGETS @@ -13,7 +13,6 @@ , ["@", "src", "src/buildtool/execution_engine/executor", "executor"] , ["@", "src", "src/buildtool/file_system", "file_system_manager"] , ["@", "src", "src/buildtool/progress_reporting", "progress"] - , ["utils", "test_env"] , ["@", "catch2", "", "catch2"] , ["@", "gsl", "", "gsl"] ] @@ -24,7 +23,8 @@ , "name": ["executor"] , "srcs": ["executor.test.cpp"] , "private-deps": - [ ["@", "src", "src/buildtool/common", "artifact_factory"] + [ ["@", "src", "src/buildtool/auth", "auth"] + , ["@", "src", "src/buildtool/common", "artifact_factory"] , ["@", "src", "src/buildtool/common", "common"] , ["@", "src", "src/buildtool/common", "config"] , ["@", "src", "src/buildtool/execution_api/common", "common"] @@ -44,7 +44,6 @@ , "data": ["test_data"] , "private-deps": [ "executor_api_tests" - , ["@", "src", "src/buildtool/auth", "auth"] , ["@", "src", "src/buildtool/common", "common"] , ["@", "src", "src/buildtool/common", "config"] , ["@", "src", "src/buildtool/execution_api/local", "local"] @@ -53,6 +52,7 @@ , ["@", "src", "src/buildtool/progress_reporting", "progress"] , ["utils", "catch-main-remote-execution"] , ["utils", "local_hermeticity"] + , ["utils", "test_auth_config"] , ["@", "catch2", "", "catch2"] ] , "stage": ["test", "buildtool", "execution_engine", "executor"] @@ -64,7 +64,6 @@ , "data": ["test_data"] , "private-deps": [ "executor_api_tests" - , ["@", "src", "src/buildtool/auth", "auth"] , ["@", "src", "src/buildtool/common", "common"] , ["@", "src", "src/buildtool/common", "config"] , ["@", "src", "src/buildtool/execution_api/remote", "bazel"] @@ -72,6 +71,7 @@ , ["@", "src", "src/buildtool/execution_engine/executor", "executor"] , ["@", "src", "src/buildtool/progress_reporting", "progress"] , ["utils", "catch-main-remote-execution"] + , ["utils", "test_auth_config"] , ["@", "catch2", "", "catch2"] ] , "stage": ["test", "buildtool", "execution_engine", "executor"] diff --git a/test/buildtool/execution_engine/executor/executor.test.cpp b/test/buildtool/execution_engine/executor/executor.test.cpp index a55c398d..dd9f59db 100644 --- a/test/buildtool/execution_engine/executor/executor.test.cpp +++ b/test/buildtool/execution_engine/executor/executor.test.cpp @@ -23,6 +23,7 @@ #include "catch2/catch_test_macros.hpp" #include "gsl/gsl" +#include "src/buildtool/auth/authentication.hpp" #include "src/buildtool/common/artifact_factory.hpp" #include "src/buildtool/common/repository_config.hpp" #include "src/buildtool/common/statistics.hpp" @@ -280,12 +281,13 @@ TEST_CASE("Executor: Process artifact", "[executor]") { auto api = TestApi::Ptr{new TestApi{config}}; Statistics stats{}; Progress progress{}; + Auth auth{}; Executor runner{&repo_config, api.get(), api.get(), /*properties=*/{}, /*dispatch_list=*/{}, - /*auth=*/nullptr, + &auth, &stats, &progress}; @@ -299,12 +301,13 @@ TEST_CASE("Executor: Process artifact", "[executor]") { auto api = TestApi::Ptr{new TestApi{config}}; Statistics stats{}; Progress progress{}; + Auth auth{}; Executor runner{&repo_config, api.get(), api.get(), /*properties=*/{}, /*dispatch_list=*/{}, - /*auth=*/nullptr, + &auth, &stats, &progress}; @@ -318,12 +321,13 @@ TEST_CASE("Executor: Process artifact", "[executor]") { auto api = TestApi::Ptr{new TestApi{config}}; Statistics stats{}; Progress progress{}; + Auth auth{}; Executor runner{&repo_config, api.get(), api.get(), /*properties=*/{}, /*dispatch_list=*/{}, - /*auth=*/nullptr, + &auth, &stats, &progress}; @@ -360,12 +364,13 @@ TEST_CASE("Executor: Process action", "[executor]") { auto api = TestApi::Ptr{new TestApi{config}}; Statistics stats{}; Progress progress{}; + Auth auth{}; Executor runner{&repo_config, api.get(), api.get(), /*properties=*/{}, /*dispatch_list=*/{}, - /*auth=*/nullptr, + &auth, &stats, &progress}; @@ -382,12 +387,13 @@ TEST_CASE("Executor: Process action", "[executor]") { auto api = TestApi::Ptr{new TestApi{config}}; Statistics stats{}; Progress progress{}; + Auth auth{}; Executor runner{&repo_config, api.get(), api.get(), /*properties=*/{}, /*dispatch_list=*/{}, - /*auth=*/nullptr, + &auth, &stats, &progress}; @@ -404,12 +410,13 @@ TEST_CASE("Executor: Process action", "[executor]") { auto api = TestApi::Ptr{new TestApi{config}}; Statistics stats{}; Progress progress{}; + Auth auth{}; Executor runner{&repo_config, api.get(), api.get(), /*properties=*/{}, /*dispatch_list=*/{}, - /*auth=*/nullptr, + &auth, &stats, &progress}; @@ -429,12 +436,13 @@ TEST_CASE("Executor: Process action", "[executor]") { auto api = TestApi::Ptr{new TestApi{config}}; Statistics stats{}; Progress progress{}; + Auth auth{}; Executor runner{&repo_config, api.get(), api.get(), /*properties=*/{}, /*dispatch_list=*/{}, - /*auth=*/nullptr, + &auth, &stats, &progress}; @@ -451,12 +459,13 @@ TEST_CASE("Executor: Process action", "[executor]") { auto api = TestApi::Ptr{new TestApi{config}}; Statistics stats{}; Progress progress{}; + Auth auth{}; Executor runner{&repo_config, api.get(), api.get(), /*properties=*/{}, /*dispatch_list=*/{}, - /*auth=*/nullptr, + &auth, &stats, &progress}; @@ -476,12 +485,13 @@ TEST_CASE("Executor: Process action", "[executor]") { auto api = TestApi::Ptr{new TestApi{config}}; Statistics stats{}; Progress progress{}; + Auth auth{}; Executor runner{&repo_config, api.get(), api.get(), /*properties=*/{}, /*dispatch_list=*/{}, - /*auth=*/nullptr, + &auth, &stats, &progress}; diff --git a/test/buildtool/execution_engine/executor/executor_api.test.hpp b/test/buildtool/execution_engine/executor/executor_api.test.hpp index db80e861..e499efae 100644 --- a/test/buildtool/execution_engine/executor/executor_api.test.hpp +++ b/test/buildtool/execution_engine/executor/executor_api.test.hpp @@ -24,7 +24,6 @@ #include "catch2/catch_test_macros.hpp" #include "gsl/gsl" -#include "src/buildtool/auth/authentication.hpp" #include "src/buildtool/common/artifact.hpp" #include "src/buildtool/common/artifact_description.hpp" #include "src/buildtool/common/artifact_factory.hpp" @@ -36,7 +35,6 @@ #include "src/buildtool/execution_engine/executor/executor.hpp" #include "src/buildtool/file_system/file_system_manager.hpp" #include "src/buildtool/progress_reporting/progress.hpp" -#include "test/utils/test_env.hpp" using ApiFactory = std::function; @@ -94,7 +92,7 @@ static inline void RunHelloWorldCompilation( gsl::not_null const& stats, gsl::not_null const& progress, ApiFactory const& factory, - Auth::TLS const* auth, + gsl::not_null const& auth, bool is_hermetic = true, int expected_queued = 0, int expected_cached = 0) { @@ -168,7 +166,7 @@ static inline void RunGreeterCompilation( gsl::not_null const& stats, gsl::not_null const& progress, ApiFactory const& factory, - Auth::TLS const* auth, + gsl::not_null const& auth, std::string const& greetcpp, bool is_hermetic = true, int expected_queued = 0, @@ -303,7 +301,7 @@ static inline void RunGreeterCompilation( gsl::not_null const& stats, gsl::not_null const& progress, ApiFactory const& factory, - Auth::TLS const* auth, + gsl::not_null const& auth, bool is_hermetic = true) { SetupConfig(repo_config); // expecting 1 action queued, 0 results from cache @@ -324,7 +322,7 @@ static inline void RunGreeterCompilation( gsl::not_null const& stats, gsl::not_null const& progress, ApiFactory const& factory, - Auth::TLS const* auth, + gsl::not_null const& auth, bool is_hermetic = true) { SetupConfig(repo_config); // expecting 3 action queued, 0 results from cache @@ -371,7 +369,7 @@ static inline void TestUploadAndDownloadTrees( gsl::not_null const& stats, gsl::not_null const& progress, ApiFactory const& factory, - Auth::TLS const* auth, + gsl::not_null const& auth, bool /*is_hermetic*/ = true, int /*expected_queued*/ = 0, int /*expected_cached*/ = 0) { @@ -525,7 +523,7 @@ static inline void TestRetrieveOutputDirectories( gsl::not_null const& stats, gsl::not_null const& progress, ApiFactory const& factory, - Auth::TLS const* auth, + gsl::not_null const& auth, bool /*is_hermetic*/ = true, int /*expected_queued*/ = 0, int /*expected_cached*/ = 0) { diff --git a/test/buildtool/execution_engine/executor/executor_api_local.test.cpp b/test/buildtool/execution_engine/executor/executor_api_local.test.cpp index b6ffe2d7..0572dc7b 100755 --- a/test/buildtool/execution_engine/executor/executor_api_local.test.cpp +++ b/test/buildtool/execution_engine/executor/executor_api_local.test.cpp @@ -13,10 +13,8 @@ // limitations under the License. #include -#include #include "catch2/catch_test_macros.hpp" -#include "src/buildtool/auth/authentication.hpp" #include "src/buildtool/common/repository_config.hpp" #include "src/buildtool/common/statistics.hpp" #include "src/buildtool/execution_api/local/local_api.hpp" @@ -25,6 +23,7 @@ #include "src/buildtool/progress_reporting/progress.hpp" #include "test/buildtool/execution_engine/executor/executor_api.test.hpp" #include "test/utils/hermeticity/local.hpp" +#include "test/utils/remote_execution/test_auth_config.hpp" TEST_CASE_METHOD(HermeticLocalTestFixture, "Executor: Upload blob", @@ -40,18 +39,14 @@ TEST_CASE_METHOD(HermeticLocalTestFixture, RepositoryConfig repo_config{}; Statistics stats{}; Progress progress{}; - - std::optional auth = {}; - if (Auth::Instance().GetAuthMethod() == AuthMethod::kTLS) { - auth = Auth::TLS::Instance(); - } - + auto auth_config = TestAuthConfig::ReadAuthConfigFromEnvironment(); + REQUIRE(auth_config); TestHelloWorldCompilation( &repo_config, &stats, &progress, [&] { return std::make_unique(&repo_config); }, - auth ? &*auth : nullptr); + &*auth_config); } TEST_CASE_METHOD(HermeticLocalTestFixture, @@ -60,18 +55,14 @@ TEST_CASE_METHOD(HermeticLocalTestFixture, RepositoryConfig repo_config{}; Statistics stats{}; Progress progress{}; - - std::optional auth = {}; - if (Auth::Instance().GetAuthMethod() == AuthMethod::kTLS) { - auth = Auth::TLS::Instance(); - } - + auto auth_config = TestAuthConfig::ReadAuthConfigFromEnvironment(); + REQUIRE(auth_config); TestGreeterCompilation( &repo_config, &stats, &progress, [&] { return std::make_unique(&repo_config); }, - auth ? &*auth : nullptr); + &*auth_config); } TEST_CASE_METHOD(HermeticLocalTestFixture, @@ -80,18 +71,14 @@ TEST_CASE_METHOD(HermeticLocalTestFixture, RepositoryConfig repo_config{}; Statistics stats{}; Progress progress{}; - - std::optional auth = {}; - if (Auth::Instance().GetAuthMethod() == AuthMethod::kTLS) { - auth = Auth::TLS::Instance(); - } - + auto auth_config = TestAuthConfig::ReadAuthConfigFromEnvironment(); + REQUIRE(auth_config); TestUploadAndDownloadTrees( &repo_config, &stats, &progress, [&] { return std::make_unique(&repo_config); }, - auth ? &*auth : nullptr); + &*auth_config); } TEST_CASE_METHOD(HermeticLocalTestFixture, @@ -100,16 +87,12 @@ TEST_CASE_METHOD(HermeticLocalTestFixture, RepositoryConfig repo_config{}; Statistics stats{}; Progress progress{}; - - std::optional auth = {}; - if (Auth::Instance().GetAuthMethod() == AuthMethod::kTLS) { - auth = Auth::TLS::Instance(); - } - + auto auth_config = TestAuthConfig::ReadAuthConfigFromEnvironment(); + REQUIRE(auth_config); TestRetrieveOutputDirectories( &repo_config, &stats, &progress, [&] { return std::make_unique(&repo_config); }, - auth ? &*auth : nullptr); + &*auth_config); } diff --git a/test/buildtool/execution_engine/executor/executor_api_remote_bazel.test.cpp b/test/buildtool/execution_engine/executor/executor_api_remote_bazel.test.cpp index 83554e64..d2f41bf1 100755 --- a/test/buildtool/execution_engine/executor/executor_api_remote_bazel.test.cpp +++ b/test/buildtool/execution_engine/executor/executor_api_remote_bazel.test.cpp @@ -15,7 +15,6 @@ #include #include "catch2/catch_test_macros.hpp" -#include "src/buildtool/auth/authentication.hpp" #include "src/buildtool/common/repository_config.hpp" #include "src/buildtool/common/statistics.hpp" #include "src/buildtool/execution_api/remote/bazel/bazel_api.hpp" @@ -23,22 +22,18 @@ #include "src/buildtool/execution_engine/executor/executor.hpp" #include "src/buildtool/progress_reporting/progress.hpp" #include "test/buildtool/execution_engine/executor/executor_api.test.hpp" +#include "test/utils/remote_execution/test_auth_config.hpp" TEST_CASE("Executor: Upload blob", "[executor]") { RepositoryConfig repo_config{}; ExecutionConfiguration config; auto const& info = RemoteExecutionConfig::RemoteAddress(); - std::optional auth = {}; - if (Auth::Instance().GetAuthMethod() == AuthMethod::kTLS) { - auth = Auth::TLS::Instance(); - } + auto auth_config = TestAuthConfig::ReadAuthConfigFromEnvironment(); + REQUIRE(auth_config); TestBlobUpload(&repo_config, [&] { - return BazelApi::Ptr{new BazelApi{"remote-execution", - info->host, - info->port, - auth ? &*auth : nullptr, - config}}; + return BazelApi::Ptr{new BazelApi{ + "remote-execution", info->host, info->port, &*auth_config, config}}; }); } @@ -51,10 +46,8 @@ TEST_CASE("Executor: Compile hello world", "[executor]") { auto const& info = RemoteExecutionConfig::RemoteAddress(); - std::optional auth = {}; - if (Auth::Instance().GetAuthMethod() == AuthMethod::kTLS) { - auth = Auth::TLS::Instance(); - } + auto auth_config = TestAuthConfig::ReadAuthConfigFromEnvironment(); + REQUIRE(auth_config); TestHelloWorldCompilation( &repo_config, @@ -64,10 +57,10 @@ TEST_CASE("Executor: Compile hello world", "[executor]") { return BazelApi::Ptr{new BazelApi{"remote-execution", info->host, info->port, - auth ? &*auth : nullptr, + &*auth_config, config}}; }, - auth ? &*auth : nullptr, + &*auth_config, false /* not hermetic */); } @@ -80,10 +73,8 @@ TEST_CASE("Executor: Compile greeter", "[executor]") { auto const& info = RemoteExecutionConfig::RemoteAddress(); - std::optional auth = {}; - if (Auth::Instance().GetAuthMethod() == AuthMethod::kTLS) { - auth = Auth::TLS::Instance(); - } + auto auth_config = TestAuthConfig::ReadAuthConfigFromEnvironment(); + REQUIRE(auth_config); TestGreeterCompilation( &repo_config, @@ -93,10 +84,10 @@ TEST_CASE("Executor: Compile greeter", "[executor]") { return BazelApi::Ptr{new BazelApi{"remote-execution", info->host, info->port, - auth ? &*auth : nullptr, + &*auth_config, config}}; }, - auth ? &*auth : nullptr, + &*auth_config, false /* not hermetic */); } @@ -109,10 +100,8 @@ TEST_CASE("Executor: Upload and download trees", "[executor]") { auto const& info = RemoteExecutionConfig::RemoteAddress(); - std::optional auth = {}; - if (Auth::Instance().GetAuthMethod() == AuthMethod::kTLS) { - auth = Auth::TLS::Instance(); - } + auto auth_config = TestAuthConfig::ReadAuthConfigFromEnvironment(); + REQUIRE(auth_config); TestUploadAndDownloadTrees( &repo_config, @@ -122,10 +111,10 @@ TEST_CASE("Executor: Upload and download trees", "[executor]") { return BazelApi::Ptr{new BazelApi{"remote-execution", info->host, info->port, - auth ? &*auth : nullptr, + &*auth_config, config}}; }, - auth ? &*auth : nullptr, + &*auth_config, false /* not hermetic */); } @@ -138,10 +127,8 @@ TEST_CASE("Executor: Retrieve output directories", "[executor]") { auto const& info = RemoteExecutionConfig::RemoteAddress(); - std::optional auth = {}; - if (Auth::Instance().GetAuthMethod() == AuthMethod::kTLS) { - auth = Auth::TLS::Instance(); - } + auto auth_config = TestAuthConfig::ReadAuthConfigFromEnvironment(); + REQUIRE(auth_config); TestRetrieveOutputDirectories( &repo_config, @@ -151,9 +138,9 @@ TEST_CASE("Executor: Retrieve output directories", "[executor]") { return BazelApi::Ptr{new BazelApi{"remote-execution", info->host, info->port, - auth ? &*auth : nullptr, + &*auth_config, config}}; }, - auth ? &*auth : nullptr, + &*auth_config, false /* not hermetic */); } -- cgit v1.2.3