Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F14008113
D11475.id27605.diff
No One
Temporary
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Size
9 KB
Referenced Files
None
Subscribers
None
D11475.id27605.diff
View Options
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;
@@ -755,5 +756,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);
File Metadata
Details
Attached
Mime Type
text/plain
Expires
Wed, Oct 30, 3:54 PM (2 w, 6 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6754586
Default Alt Text
D11475.id27605.diff (9 KB)
Attached To
Mode
D11475: Proxy Diffusion Conduit API calls
Attached
Detach File
Event Timeline
Log In to Comment