diff --git a/src/applications/conduit/call/ConduitCall.php b/src/applications/conduit/call/ConduitCall.php --- a/src/applications/conduit/call/ConduitCall.php +++ b/src/applications/conduit/call/ConduitCall.php @@ -43,6 +43,10 @@ $this->request = new ConduitAPIRequest($params); } + public function getAPIRequest() { + return $this->request; + } + public function setUser(PhabricatorUser $user) { $this->user = $user; return $this; diff --git a/src/applications/conduit/controller/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/PhabricatorConduitAPIController.php --- a/src/applications/conduit/controller/PhabricatorConduitAPIController.php +++ b/src/applications/conduit/controller/PhabricatorConduitAPIController.php @@ -34,11 +34,14 @@ $result = null; - // TODO: Straighten out the auth pathway here. We shouldn't be creating - // a ConduitAPIRequest at this level, but some of the auth code expects - // it. Landing a halfway version of this to unblock T945. + // TODO: The relationship between ConduitAPIRequest and ConduitCall is a + // little odd here and could probably be improved. Specifically, the + // APIRequest is a sub-object of the Call, which does not parallel the + // role of AphrontRequest (which is an indepenent object). + // In particular, the setUser() and getUser() existing independently on + // the Call and APIRequest is very awkward. - $api_request = new ConduitAPIRequest($params); + $api_request = $call->getAPIRequest(); $allow_unguarded_writes = false; $auth_error = null; @@ -284,6 +287,9 @@ } $user = PhabricatorUser::getOmnipotentUser(); + + // Flag this as an intracluster request. + $api_request->setIsClusterRequest(true); } return $this->validateAuthenticatedUser( @@ -380,6 +386,9 @@ 'cluster address range. Requests signed with cluster API '. 'tokens must originate from within the cluster.'),); } + + // Flag this as an intracluster request. + $api_request->setIsClusterRequest(true); } $user = $token->getObject(); diff --git a/src/applications/conduit/protocol/ConduitAPIRequest.php b/src/applications/conduit/protocol/ConduitAPIRequest.php --- a/src/applications/conduit/protocol/ConduitAPIRequest.php +++ b/src/applications/conduit/protocol/ConduitAPIRequest.php @@ -4,6 +4,7 @@ protected $params; private $user; + private $isClusterRequest = false; public function __construct(array $params) { $this->params = $params; @@ -42,4 +43,13 @@ return $this->user; } + public function setIsClusterRequest($is_cluster_request) { + $this->isClusterRequest = $is_cluster_request; + return $this; + } + + public function getIsClusterRequest() { + return $this->isClusterRequest; + } + } diff --git a/src/applications/diffusion/conduit/DiffusionQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionQueryConduitAPIMethod.php --- a/src/applications/diffusion/conduit/DiffusionQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionQueryConduitAPIMethod.php @@ -105,9 +105,36 @@ 'initFromConduit' => false, )); - $this->setDiffusionRequest($drequest); - - return $this->getResult($request); + // Figure out whether we're going to handle this request on this device, + // or proxy it to another node in the cluster. + + // If this is a cluster request and we need to proxy, we'll explode here + // to prevent infinite recursion. + + $is_cluster_request = $request->getIsClusterRequest(); + + $repository = $drequest->getRepository(); + $client = $repository->newConduitClient( + $request->getUser(), + $is_cluster_request); + if ($client) { + // We're proxying, so just make an intracluster call. + return $client->callMethodSynchronous( + $this->getAPIMethodName(), + $request->getAllParameters()); + } else { + + // We pass this flag on to prevent proxying of any other Conduit calls + // which we need to make in order to respond to this one. Although we + // could safely proxy them, we take a big performance hit in the common + // case, and doing more proxying wouldn't exercise any additional code so + // we wouldn't gain a testability/predictability benefit. + $drequest->setIsClusterRequest($is_cluster_request); + + $this->setDiffusionRequest($drequest); + + return $this->getResult($request); + } } protected function getResult(ConduitAPIRequest $request) { diff --git a/src/applications/diffusion/query/DiffusionQuery.php b/src/applications/diffusion/query/DiffusionQuery.php --- a/src/applications/diffusion/query/DiffusionQuery.php +++ b/src/applications/diffusion/query/DiffusionQuery.php @@ -72,7 +72,9 @@ $params = $params + $core_params; - $client = $repository->newConduitClient($user); + $client = $repository->newConduitClient( + $user, + $drequest->getIsClusterRequest()); if (!$client) { return id(new ConduitCall($method, $params)) ->setUser($user) diff --git a/src/applications/diffusion/request/DiffusionRequest.php b/src/applications/diffusion/request/DiffusionRequest.php --- a/src/applications/diffusion/request/DiffusionRequest.php +++ b/src/applications/diffusion/request/DiffusionRequest.php @@ -24,6 +24,7 @@ protected $repositoryCommitData; protected $arcanistProjects; + private $isClusterRequest = false; private $initFromConduit = true; private $user; private $branchObject = false; @@ -765,5 +766,13 @@ } } + public function setIsClusterRequest($is_cluster_request) { + $this->isClusterRequest = $is_cluster_request; + return $this; + } + + public function getIsClusterRequest() { + return $this->isClusterRequest; + } } diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -1520,13 +1520,39 @@ * Build a new Conduit client in order to make a service call to this * repository. * - * If the repository is hosted locally, this method returns `null`. The + * If the repository is hosted locally, this method may return `null`. The * caller should use `ConduitCall` or other local logic to complete the * request. * + * By default, we will return a @{class:ConduitClient} for any repository with + * a service, even if that service is on the current device. + * + * We do this because this configuration does not make very much sense in a + * production context, but is very common in a test/development context + * (where the developer's machine is both the web host and the repository + * service). By proxying in development, we get more consistent behavior + * between development and production, and don't have a major untested + * codepath. + * + * The `$never_proxy` parameter can be used to prevent this local proxying. + * If the flag is passed: + * + * - The method will return `null` (implying a local service call) + * if the repository service is hosted on the current device. + * - The method will throw if it would need to return a client. + * + * This is used to prevent loops in Conduit: the first request will proxy, + * even in development, but the second request will be identified as a + * cluster request and forced not to proxy. + * + * @param PhabricatorUser Viewing user. + * @param bool `true` to throw if a client would be returned. * @return ConduitClient|null Client, or `null` for local repositories. */ - public function newConduitClient(PhabricatorUser $viewer) { + public function newConduitClient( + PhabricatorUser $viewer, + $never_proxy = false) { + $service_phid = $this->getAlmanacServicePHID(); if (!$service_phid) { // No service, so this is a local repository. @@ -1561,10 +1587,32 @@ 'interfaces.')); } + $local_device = AlmanacKeys::getDeviceID(); + + if ($never_proxy && !$local_device) { + throw new Exception( + pht( + 'Unable to handle proxied service request. This device is not '. + 'registered, so it can not identify local services. Register '. + 'this device before sending requests here.')); + } + $uris = array(); foreach ($bindings as $binding) { $iface = $binding->getInterface(); + // If we're never proxying this and it's locally satisfiable, return + // `null` to tell the caller to handle it locally. If we're allowed to + // proxy, we skip this check and may proxy the request to ourselves. + // (That proxied request will end up here with proxying forbidden, + // return `null`, and then the request will actually run.) + + if ($local_device && $never_proxy) { + if ($iface->getDevice()->getName() == $local_device) { + return null; + } + } + $protocol = $binding->getAlmanacPropertyValue('protocol'); if ($protocol === 'http') { $uris[] = 'http://'.$iface->renderDisplayAddress().'/'; @@ -1579,6 +1627,13 @@ } } + if ($never_proxy) { + throw new Exception( + pht( + 'Refusing to proxy a repository request from a cluster host. '. + 'Cluster hosts must correctly route their intracluster requests.')); + } + shuffle($uris); $uri = head($uris);