serve configuration: Fix remote execution check to allow unset value

In the scenario when 'just serve' acts as 'just execute', the
remote execution endpoint returned by the serve service should be
allowed to be empty.
In this case, from the server's perspective, there is nothing to be
checked, however a client might still want to ensure that its own
configured serve and execution endpoints match.

6 files changed, 61 insertions, 21 deletions

diff --git a/src/buildtool/serve_api/remote/TARGETS b/src/buildtool/serve_api/remote/TARGETS
index 8b64aee7..1de01320 100644
--- a/src/buildtool/serve_api/remote/TARGETS
+++ b/src/buildtool/serve_api/remote/TARGETS
@@ -71,6 +71,7 @@
   , "private-deps":
     [ ["@", "json", "", "json"]
     , ["src/buildtool/execution_api/remote", "config"]
+    , ["src/buildtool/serve_api/remote", "config"]
     ]
   }
 }
diff --git a/src/buildtool/serve_api/remote/configuration_client.cpp b/src/buildtool/serve_api/remote/configuration_client.cpp
index b4f74230..acb43886 100644
--- a/src/buildtool/serve_api/remote/configuration_client.cpp
+++ b/src/buildtool/serve_api/remote/configuration_client.cpp
@@ -14,8 +14,11 @@
 
 #include "src/buildtool/serve_api/remote/configuration_client.hpp"
 
+#include <optional>
+
 #include "nlohmann/json.hpp"
 #include "src/buildtool/execution_api/remote/config.hpp"
+#include "src/buildtool/serve_api/remote/config.hpp"
 
 auto ConfigurationClient::CheckServeRemoteExecution() -> bool {
     auto client_remote_address = RemoteExecutionConfig::RemoteAddress();
@@ -25,6 +28,13 @@ auto ConfigurationClient::CheckServeRemoteExecution() -> bool {
                      "have been set.");
         return false;
     }
+    auto client_serve_address = RemoteServeConfig::RemoteAddress();
+    if (!client_serve_address) {
+        logger_.Emit(
+            LogLevel::Error,
+            "Internal error: the serve endpoint should have been set.");
+        return false;
+    }
 
     grpc::ClientContext context;
     justbuild::just_serve::RemoteExecutionEndpointRequest request{};
@@ -36,16 +46,43 @@ auto ConfigurationClient::CheckServeRemoteExecution() -> bool {
         LogStatus(&logger_, LogLevel::Error, status);
         return false;
     }
-    auto serve_remote_endpoint = nlohmann::json::parse(response.address());
-    if (serve_remote_endpoint != client_remote_address->ToJson()) {
-        Logger::Log(LogLevel::Error,
-                    "Different execution endpoint detected.\nIn order to "
-                    "correctly use just serve, its remote execution endpoint "
-                    "must be the same used by the client.\nserve remote "
-                    "endpoint:  {}\nclient remote endpoint: {}",
-                    serve_remote_endpoint.dump(),
-                    client_remote_address->ToJson().dump());
-        return false;
+    auto client_msg = client_remote_address->ToJson().dump();
+    std::string serve_msg{};
+
+    if (response.address().empty()) {
+        // just serve acts as just execute, so from the server's perspective
+        // there is nothing to be checked and it's the client's job to ensure
+        // that its remote execution and serve endpoints match
+        //
+        // NOTE: This check might make sense to be removed altogether in the
+        // future, or updated to (at most) a warning.
+        if (client_remote_address->ToJson() == client_serve_address->ToJson()) {
+            return true;
+        }
+        serve_msg = client_serve_address->ToJson().dump();
+    }
+    else {
+        nlohmann::json serve_remote_endpoint{};
+        try {
+            serve_remote_endpoint = nlohmann::json::parse(response.address());
+        } catch (std::exception const& ex) {
+            logger_.Emit(
+                LogLevel::Error,
+                "Parsing configured address from response failed with:\n{}",
+                ex.what());
+        }
+        if (serve_remote_endpoint == client_remote_address->ToJson()) {
+            return true;
+        }
+        serve_msg = serve_remote_endpoint.dump();
     }
-    return true;
+    // log any mismatch found
+    logger_.Emit(
+        LogLevel::Error,
+        "Different execution endpoint detected.\nIn order to correctly use "
+        "just serve, its remote execution endpoint must be the same used by "
+        "the client.\nserve remote endpoint:  {}\nclient remote endpoint: {}",
+        serve_msg,
+        client_msg);
+    return false;
 }
diff --git a/src/buildtool/serve_api/remote/configuration_client.hpp b/src/buildtool/serve_api/remote/configuration_client.hpp
index 13584832..ee59bcba 100644
--- a/src/buildtool/serve_api/remote/configuration_client.hpp
+++ b/src/buildtool/serve_api/remote/configuration_client.hpp
@@ -31,9 +31,7 @@
 class ConfigurationClient {
   public:
     ConfigurationClient(std::string const& server, Port port) noexcept
-        :
-
-          stub_{justbuild::just_serve::Configuration::NewStub(
+        : stub_{justbuild::just_serve::Configuration::NewStub(
               CreateChannelWithCredentials(server, port))} {}
 
     auto CheckServeRemoteExecution() -> bool;
diff --git a/src/buildtool/serve_api/serve_service/configuration.cpp b/src/buildtool/serve_api/serve_service/configuration.cpp
index 4a103ea4..a4d0ba88 100644
--- a/src/buildtool/serve_api/serve_service/configuration.cpp
+++ b/src/buildtool/serve_api/serve_service/configuration.cpp
@@ -24,11 +24,6 @@ auto ConfigurationService::RemoteExecutionEndpoint(
     ::justbuild::just_serve::RemoteExecutionEndpointResponse* response)
     -> ::grpc::Status {
     auto address = RemoteExecutionConfig::RemoteAddress();
-    if (!address) {
-        return grpc::Status{grpc::StatusCode::INTERNAL,
-                            "could not obtain the address of the associated "
-                            "remote execution endpoint"};
-    }
-    response->set_address(address->ToJson().dump());
+    response->set_address(address ? address->ToJson().dump() : std::string());
     return ::grpc::Status::OK;
 }
diff --git a/src/buildtool/serve_api/serve_service/configuration.hpp b/src/buildtool/serve_api/serve_service/configuration.hpp
index 9bd3c892..c466576f 100644
--- a/src/buildtool/serve_api/serve_service/configuration.hpp
+++ b/src/buildtool/serve_api/serve_service/configuration.hpp
@@ -20,6 +20,11 @@
 class ConfigurationService final
     : public justbuild::just_serve::Configuration::Service {
   public:
+    // Returns the address of the associated remote endpoint, if set,
+    // or an empty string signaling that the serve endpoint acts also
+    // as a remote execution endpoint.
+    //
+    // There are no method-specific errors.
     auto RemoteExecutionEndpoint(
         ::grpc::ServerContext* context,
         const ::justbuild::just_serve::RemoteExecutionEndpointRequest* request,
diff --git a/src/buildtool/serve_api/serve_service/just_serve.proto b/src/buildtool/serve_api/serve_service/just_serve.proto
index 3d004c13..3472f646 100644
--- a/src/buildtool/serve_api/serve_service/just_serve.proto
+++ b/src/buildtool/serve_api/serve_service/just_serve.proto
@@ -283,6 +283,10 @@ message RemoteExecutionEndpointResponse {
 
 // This service can be used by the client to double check the server configuration
 service Configuration {
-  // Returns the address of the associated remote endpoint
+  // Returns the address of the associated remote endpoint, if set,
+  // or an empty string signaling that the serve endpoint acts also
+  // as a remote execution endpoint.
+  //
+  // There are no method-specific errors.
   rpc RemoteExecutionEndpoint(RemoteExecutionEndpointRequest) returns (RemoteExecutionEndpointResponse) {}
 }