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/graph_traverser/TARGETS | 4 +- .../graph_traverser/graph_traverser.test.hpp | 27 ++++--- .../graph_traverser/graph_traverser_local.test.cpp | 29 ++++--- .../graph_traverser_remote.test.cpp | 89 +++++++++------------- 4 files changed, 74 insertions(+), 75 deletions(-) (limited to 'test/buildtool/graph_traverser') diff --git a/test/buildtool/graph_traverser/TARGETS b/test/buildtool/graph_traverser/TARGETS index c67ded03..1d11b04a 100644 --- a/test/buildtool/graph_traverser/TARGETS +++ b/test/buildtool/graph_traverser/TARGETS @@ -15,7 +15,6 @@ , ["@", "src", "src/buildtool/logging", "logging"] , ["@", "src", "src/buildtool/progress_reporting", "progress"] , ["@", "src", "src/buildtool/execution_api/common", "api_bundle"] - , ["utils", "test_env"] ] , "stage": ["test", "buildtool", "graph_traverser"] } @@ -29,6 +28,7 @@ , ["@", "catch2", "", "catch2"] , ["", "catch-main"] , ["utils", "local_hermeticity"] + , ["utils", "test_auth_config"] ] , "stage": ["test", "buildtool", "graph_traverser"] } @@ -41,7 +41,7 @@ [ "graph_traverser_tests" , ["@", "catch2", "", "catch2"] , ["utils", "catch-main-remote-execution"] - , ["@", "src", "src/buildtool/auth", "auth"] + , ["utils", "test_auth_config"] ] , "stage": ["test", "buildtool", "graph_traverser"] } diff --git a/test/buildtool/graph_traverser/graph_traverser.test.hpp b/test/buildtool/graph_traverser/graph_traverser.test.hpp index 7d23dcd5..f1be7eec 100644 --- a/test/buildtool/graph_traverser/graph_traverser.test.hpp +++ b/test/buildtool/graph_traverser/graph_traverser.test.hpp @@ -26,6 +26,7 @@ #include #include "catch2/catch_test_macros.hpp" +#include "gsl/gsl" #include "nlohmann/json.hpp" #include "src/buildtool/auth/authentication.hpp" #include "src/buildtool/common/statistics.hpp" @@ -38,7 +39,6 @@ #include "src/buildtool/logging/logger.hpp" #include "src/buildtool/progress_reporting/progress.hpp" #include "src/utils/cpp/json.hpp" -#include "test/utils/test_env.hpp" // NOLINTNEXTLINE(google-build-namespaces) namespace { @@ -152,7 +152,7 @@ inline void SetLauncher() { } // namespace [[maybe_unused]] static void TestHelloWorldCopyMessage( - Auth::TLS const* auth, + gsl::not_null const& auth, bool is_hermetic = true) { TestProject p("hello_world_copy_message"); @@ -217,8 +217,9 @@ inline void SetLauncher() { } } -[[maybe_unused]] static void TestCopyLocalFile(Auth::TLS const* auth, - bool is_hermetic = true) { +[[maybe_unused]] static void TestCopyLocalFile( + gsl::not_null const& auth, + bool is_hermetic = true) { TestProject p("copy_local_file"); SetLauncher(); @@ -249,7 +250,7 @@ inline void SetLauncher() { } [[maybe_unused]] static void TestSequencePrinterBuildLibraryOnly( - Auth::TLS const* auth, + gsl::not_null const& auth, bool is_hermetic = true) { TestProject p("sequence_printer_build_library_only"); @@ -301,7 +302,7 @@ inline void SetLauncher() { } [[maybe_unused]] static void TestHelloWorldWithKnownSource( - Auth::TLS const* auth, + gsl::not_null const& auth, bool is_hermetic = true) { TestProject full_hello_world("hello_world_copy_message"); @@ -360,7 +361,7 @@ inline void SetLauncher() { } } -static void TestBlobsUploadedAndUsed(Auth::TLS const* auth, +static void TestBlobsUploadedAndUsed(gsl::not_null const& auth, bool is_hermetic = true) { TestProject p("use_uploaded_blobs"); auto const clargs = p.CmdLineArgs(); @@ -399,8 +400,9 @@ static void TestBlobsUploadedAndUsed(Auth::TLS const* auth, } } -static void TestEnvironmentVariablesSetAndUsed(Auth::TLS const* auth, - bool is_hermetic = true) { +static void TestEnvironmentVariablesSetAndUsed( + gsl::not_null const& auth, + bool is_hermetic = true) { TestProject p("use_env_variables"); auto const clargs = p.CmdLineArgs(); @@ -438,7 +440,8 @@ static void TestEnvironmentVariablesSetAndUsed(Auth::TLS const* auth, } } -static void TestTreesUsed(Auth::TLS const* auth, bool is_hermetic = true) { +static void TestTreesUsed(gsl::not_null const& auth, + bool is_hermetic = true) { TestProject p("use_trees"); auto const clargs = p.CmdLineArgs(); @@ -476,7 +479,7 @@ static void TestTreesUsed(Auth::TLS const* auth, bool is_hermetic = true) { } } -static void TestNestedTreesUsed(Auth::TLS const* auth, +static void TestNestedTreesUsed(gsl::not_null const& auth, bool is_hermetic = true) { TestProject p("use_nested_trees"); auto const clargs = p.CmdLineArgs(); @@ -515,7 +518,7 @@ static void TestNestedTreesUsed(Auth::TLS const* auth, } } -static void TestFlakyHelloWorldDetected(Auth::TLS const* auth, +static void TestFlakyHelloWorldDetected(gsl::not_null const& auth, bool /*is_hermetic*/ = true) { TestProject p("flaky_hello_world"); diff --git a/test/buildtool/graph_traverser/graph_traverser_local.test.cpp b/test/buildtool/graph_traverser/graph_traverser_local.test.cpp index 64d7189b..88990dc8 100644 --- a/test/buildtool/graph_traverser/graph_traverser_local.test.cpp +++ b/test/buildtool/graph_traverser/graph_traverser_local.test.cpp @@ -13,59 +13,70 @@ // limitations under the License. #include "catch2/catch_test_macros.hpp" +#include "src/buildtool/auth/authentication.hpp" #include "test/buildtool/graph_traverser/graph_traverser.test.hpp" #include "test/utils/hermeticity/local.hpp" +#include "test/utils/remote_execution/test_auth_config.hpp" TEST_CASE_METHOD(HermeticLocalTestFixture, "Local: Output created when entry point is local artifact", "[graph_traverser]") { - TestCopyLocalFile(/*auth=*/nullptr); + Auth auth{}; /*no TLS needed*/ + TestCopyLocalFile(&auth); } TEST_CASE_METHOD(HermeticLocalTestFixture, "Local: Output created and contents are correct", "[graph_traverser]") { - TestHelloWorldCopyMessage(/*auth=*/nullptr); + Auth auth{}; /*no TLS needed*/ + TestHelloWorldCopyMessage(&auth); } TEST_CASE_METHOD(HermeticLocalTestFixture, "Local: Actions are not re-run", "[graph_traverser]") { - TestSequencePrinterBuildLibraryOnly(/*auth=*/nullptr); + Auth auth{}; /*no TLS needed*/ + TestSequencePrinterBuildLibraryOnly(&auth); } TEST_CASE_METHOD(HermeticLocalTestFixture, "Local: KNOWN artifact", "[graph_traverser]") { - TestHelloWorldWithKnownSource(/*auth=*/nullptr); + Auth auth{}; /*no TLS needed*/ + TestHelloWorldWithKnownSource(&auth); } TEST_CASE_METHOD(HermeticLocalTestFixture, "Local: Blobs uploaded and correctly used", "[graph_traverser]") { - TestBlobsUploadedAndUsed(/*auth=*/nullptr); + Auth auth{}; /*no TLS needed*/ + TestBlobsUploadedAndUsed(&auth); } TEST_CASE_METHOD(HermeticLocalTestFixture, "Local: Environment variables are set and used", "[graph_traverser]") { - TestEnvironmentVariablesSetAndUsed(/*auth=*/nullptr); + Auth auth{}; /*no TLS needed*/ + TestEnvironmentVariablesSetAndUsed(&auth); } TEST_CASE_METHOD(HermeticLocalTestFixture, "Local: Trees correctly used", "[graph_traverser]") { - TestTreesUsed(/*auth=*/nullptr); + Auth auth{}; /*no TLS needed*/ + TestTreesUsed(&auth); } TEST_CASE_METHOD(HermeticLocalTestFixture, "Local: Nested trees correctly used", "[graph_traverser]") { - TestNestedTreesUsed(/*auth=*/nullptr); + Auth auth{}; /*no TLS needed*/ + TestNestedTreesUsed(&auth); } TEST_CASE_METHOD(HermeticLocalTestFixture, "Local: Detect flaky actions", "[graph_traverser]") { - TestFlakyHelloWorldDetected(/*auth=*/nullptr); + Auth auth{}; /*no TLS needed*/ + TestFlakyHelloWorldDetected(&auth); } diff --git a/test/buildtool/graph_traverser/graph_traverser_remote.test.cpp b/test/buildtool/graph_traverser/graph_traverser_remote.test.cpp index e33d5d27..e24a8d4e 100644 --- a/test/buildtool/graph_traverser/graph_traverser_remote.test.cpp +++ b/test/buildtool/graph_traverser/graph_traverser_remote.test.cpp @@ -12,88 +12,73 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include - #include "catch2/catch_test_macros.hpp" -#include "src/buildtool/auth/authentication.hpp" #include "test/buildtool/graph_traverser/graph_traverser.test.hpp" +#include "test/utils/remote_execution/test_auth_config.hpp" TEST_CASE("Remote: Output created and contents are correct", "[graph_traverser]") { - std::optional auth = {}; - if (Auth::Instance().GetAuthMethod() == AuthMethod::kTLS) { - auth = Auth::TLS::Instance(); - } - TestHelloWorldCopyMessage(auth ? &*auth : nullptr, - false /* not hermetic */); + auto auth_config = TestAuthConfig::ReadAuthConfigFromEnvironment(); + REQUIRE(auth_config); + + TestHelloWorldCopyMessage(&*auth_config, false /* not hermetic */); } TEST_CASE("Remote: Output created when entry point is local artifact", "[graph_traverser]") { - std::optional auth = {}; - if (Auth::Instance().GetAuthMethod() == AuthMethod::kTLS) { - auth = Auth::TLS::Instance(); - } - TestCopyLocalFile(auth ? &*auth : nullptr, false /* not hermetic */); + auto auth_config = TestAuthConfig::ReadAuthConfigFromEnvironment(); + REQUIRE(auth_config); + + TestCopyLocalFile(&*auth_config, false /* not hermetic */); } TEST_CASE("Remote: Actions are not re-run", "[graph_traverser]") { - std::optional auth = {}; - if (Auth::Instance().GetAuthMethod() == AuthMethod::kTLS) { - auth = Auth::TLS::Instance(); - } - TestSequencePrinterBuildLibraryOnly(auth ? &*auth : nullptr, + auto auth_config = TestAuthConfig::ReadAuthConfigFromEnvironment(); + REQUIRE(auth_config); + + TestSequencePrinterBuildLibraryOnly(&*auth_config, false /* not hermetic */); } TEST_CASE("Remote: KNOWN artifact", "[graph_traverser]") { - std::optional auth = {}; - if (Auth::Instance().GetAuthMethod() == AuthMethod::kTLS) { - auth = Auth::TLS::Instance(); - } - TestHelloWorldWithKnownSource(auth ? &*auth : nullptr, - false /* not hermetic */); + auto auth_config = TestAuthConfig::ReadAuthConfigFromEnvironment(); + REQUIRE(auth_config); + + TestHelloWorldWithKnownSource(&*auth_config, false /* not hermetic */); } TEST_CASE("Remote: Blobs uploaded and correctly used", "[graph_traverser]") { - std::optional auth = {}; - if (Auth::Instance().GetAuthMethod() == AuthMethod::kTLS) { - auth = Auth::TLS::Instance(); - } - TestBlobsUploadedAndUsed(auth ? &*auth : nullptr, false /* not hermetic */); + auto auth_config = TestAuthConfig::ReadAuthConfigFromEnvironment(); + REQUIRE(auth_config); + + TestBlobsUploadedAndUsed(&*auth_config, false /* not hermetic */); } TEST_CASE("Remote: Environment variables are set and used", "[graph_traverser]") { - std::optional auth = {}; - if (Auth::Instance().GetAuthMethod() == AuthMethod::kTLS) { - auth = Auth::TLS::Instance(); - } - TestEnvironmentVariablesSetAndUsed(auth ? &*auth : nullptr, - false /* not hermetic */); + auto auth_config = TestAuthConfig::ReadAuthConfigFromEnvironment(); + REQUIRE(auth_config); + + TestEnvironmentVariablesSetAndUsed(&*auth_config, false /* not hermetic */); } TEST_CASE("Remote: Trees correctly used", "[graph_traverser]") { - std::optional auth = {}; - if (Auth::Instance().GetAuthMethod() == AuthMethod::kTLS) { - auth = Auth::TLS::Instance(); - } - TestTreesUsed(auth ? &*auth : nullptr, false /* not hermetic */); + auto auth_config = TestAuthConfig::ReadAuthConfigFromEnvironment(); + REQUIRE(auth_config); + + TestTreesUsed(&*auth_config, false /* not hermetic */); } TEST_CASE("Remote: Nested trees correctly used", "[graph_traverser]") { - std::optional auth = {}; - if (Auth::Instance().GetAuthMethod() == AuthMethod::kTLS) { - auth = Auth::TLS::Instance(); - } - TestNestedTreesUsed(auth ? &*auth : nullptr, false /* not hermetic */); + auto auth_config = TestAuthConfig::ReadAuthConfigFromEnvironment(); + REQUIRE(auth_config); + + TestNestedTreesUsed(&*auth_config, false /* not hermetic */); } TEST_CASE("Remote: Detect flaky actions", "[graph_traverser]") { - std::optional auth = {}; - if (Auth::Instance().GetAuthMethod() == AuthMethod::kTLS) { - auth = Auth::TLS::Instance(); - } - TestFlakyHelloWorldDetected(auth ? &*auth : nullptr, - false /* not hermetic */); + auto auth_config = TestAuthConfig::ReadAuthConfigFromEnvironment(); + REQUIRE(auth_config); + + TestFlakyHelloWorldDetected(&*auth_config, false /* not hermetic */); } -- cgit v1.2.3