Make Drydock more broadly aware of policies

Summary: Ref T2015. Moves a bunch of raw object loads into modern policy-aware queries. Also straightens out the Log and Lease policies a little bit: there are legitimate states where these objects are not attached to a resource (particularly, while a lease is being acquired). Handle these more gracefully. Test Plan: Lint / browsed stuff. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T2015 Differential Revision: https://secure.phabricator.com/D7836
2013-12-27 13:15:19 -08:00
parent 1650874004
commit 9b0fa5747b
17 changed files with 192 additions and 86 deletions
--- a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php
+++ b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php
@@ -122,6 +122,7 @@ abstract class DrydockBlueprintImplementation {
      $resource->beginReadLocking();
        $resource->reload();

+        // TODO: Policy stuff.
        $other_leases = id(new DrydockLease())->loadAllWhere(
          'status IN (%Ld) AND resourceID = %d',
          array(
@@ -388,13 +389,13 @@ abstract class DrydockBlueprintImplementation {
  }

  protected function newResourceTemplate($name) {
-    $resource = new DrydockResource();
-    $resource->setBlueprintPHID($this->getInstance()->getPHID());
-    $resource->setBlueprintClass($this->getBlueprintClass());
-    $resource->setType($this->getType());
-    $resource->setStatus(DrydockResourceStatus::STATUS_PENDING);
-    $resource->setName($name);
-    $resource->save();
+    $resource = id(new DrydockResource())
+      ->setBlueprintPHID($this->getInstance()->getPHID())
+      ->setBlueprintClass($this->getBlueprintClass())
+      ->setType($this->getType())
+      ->setStatus(DrydockResourceStatus::STATUS_PENDING)
+      ->setName($name)
+      ->save();

    $this->activeResource = $resource;

--- a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php
+++ b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php
@@ -52,6 +52,7 @@ final class DrydockWorkingCopyBlueprintImplementation
        throw new Exception("Unsupported VCS!");
    }

+    // TODO: Policy stuff here too.
    $host_lease = id(new DrydockLease())
      ->setResourceType('host')
      ->waitUntilActive();
--- a/src/applications/drydock/controller/DrydockBlueprintCreateController.php
+++ b/src/applications/drydock/controller/DrydockBlueprintCreateController.php
@@ -16,12 +16,12 @@ final class DrydockBlueprintCreateController
        return $this->createDialog($implementations);
      }

-      $blueprint = new DrydockBlueprint();
-      $blueprint->setClassName($class);
-      $blueprint->setDetails(array());
-      $blueprint->setViewPolicy(PhabricatorPolicies::POLICY_ADMIN);
-      $blueprint->setEditPolicy(PhabricatorPolicies::POLICY_ADMIN);
-      $blueprint->save();
+      $blueprint = id(new DrydockBlueprint())
+        ->setClassName($class)
+        ->setDetails(array())
+        ->setViewPolicy(PhabricatorPolicies::POLICY_ADMIN)
+        ->setEditPolicy(PhabricatorPolicies::POLICY_ADMIN)
+        ->save();

      $edit_uri = $this->getApplicationURI(
        "blueprint/edit/".$blueprint->getID()."/");
--- a/src/applications/drydock/controller/DrydockBlueprintViewController.php
+++ b/src/applications/drydock/controller/DrydockBlueprintViewController.php
@@ -10,9 +10,12 @@ final class DrydockBlueprintViewController extends DrydockBlueprintController {

  public function processRequest() {
    $request = $this->getRequest();
-    $user = $request->getUser();
+    $viewer = $request->getUser();

-    $blueprint = id(new DrydockBlueprint())->load($this->id);
+    $blueprint = id(new DrydockBlueprintQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($this->id))
+      ->executeOne();
    if (!$blueprint) {
      return new Aphront404Response();
    }
@@ -30,7 +33,7 @@ final class DrydockBlueprintViewController extends DrydockBlueprintController {

    $resources = id(new DrydockResourceQuery())
      ->withBlueprintPHIDs(array($blueprint->getPHID()))
-      ->setViewer($user)
+      ->setViewer($viewer)
      ->execute();

    $resource_list = $this->buildResourceListView($resources);
--- a/src/applications/drydock/controller/DrydockLeaseViewController.php
+++ b/src/applications/drydock/controller/DrydockLeaseViewController.php
@@ -10,9 +10,12 @@ final class DrydockLeaseViewController extends DrydockLeaseController {

  public function processRequest() {
    $request = $this->getRequest();
-    $user = $request->getUser();
+    $viewer = $request->getUser();

-    $lease = id(new DrydockLease())->load($this->id);
+    $lease = id(new DrydockLeaseQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($this->id))
+      ->executeOne();
    if (!$lease) {
      return new Aphront404Response();
    }
@@ -32,7 +35,7 @@ final class DrydockLeaseViewController extends DrydockLeaseController {
    $pager->setOffset($request->getInt('offset'));

    $logs = id(new DrydockLogQuery())
-      ->setViewer($user)
+      ->setViewer($viewer)
      ->withLeaseIDs(array($lease->getID()))
      ->executeWithOffsetPager($pager);

--- a/src/applications/drydock/controller/DrydockResourceCloseController.php
+++ b/src/applications/drydock/controller/DrydockResourceCloseController.php
@@ -10,9 +10,12 @@ final class DrydockResourceCloseController extends DrydockResourceController {

  public function processRequest() {
    $request = $this->getRequest();
-    $user = $request->getUser();
+    $viewer = $request->getUser();

-    $resource = id(new DrydockResource())->load($this->id);
+    $resource = id(new DrydockResourceQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($this->id))
+      ->executeOne();
    if (!$resource) {
      return new Aphront404Response();
    }
@@ -22,7 +25,7 @@ final class DrydockResourceCloseController extends DrydockResourceController {

    if ($resource->getStatus() != DrydockResourceStatus::STATUS_OPEN) {
      $dialog = id(new AphrontDialogView())
-        ->setUser($user)
+        ->setUser($viewer)
        ->setTitle(pht('Resource Not Open'))
        ->appendChild(phutil_tag('p', array(), pht(
          'You can only close "open" resources.')))
@@ -31,22 +34,22 @@ final class DrydockResourceCloseController extends DrydockResourceController {
      return id(new AphrontDialogResponse())->setDialog($dialog);
    }

-    if (!$request->isDialogFormPost()) {
-      $dialog = id(new AphrontDialogView())
-        ->setUser($user)
-        ->setTitle(pht('Really close resource?'))
-        ->appendChild(phutil_tag('p', array(), pht(
-          'Closing a resource releases all leases and destroys the '.
-          'resource. It can not be undone. Continue?')))
-        ->addSubmitButton(pht('Close Resource'))
-        ->addCancelButton($resource_uri);
-
-      return id(new AphrontDialogResponse())->setDialog($dialog);
+    if ($request->isFormPost()) {
+      $resource->closeResource();
+      return id(new AphrontReloadResponse())->setURI($resource_uri);
    }

-    $resource->closeResource();
+    $dialog = id(new AphrontDialogView())
+      ->setUser($viewer)
+      ->setTitle(pht('Really close resource?'))
+      ->appendChild(
+        pht(
+          'Closing a resource releases all leases and destroys the '.
+          'resource. It can not be undone. Continue?'))
+      ->addSubmitButton(pht('Close Resource'))
+      ->addCancelButton($resource_uri);

-    return id(new AphrontReloadResponse())->setURI($resource_uri);
+    return id(new AphrontDialogResponse())->setDialog($dialog);
  }

 }
--- a/src/applications/drydock/controller/DrydockResourceViewController.php
+++ b/src/applications/drydock/controller/DrydockResourceViewController.php
@@ -10,9 +10,12 @@ final class DrydockResourceViewController extends DrydockResourceController {

  public function processRequest() {
    $request = $this->getRequest();
-    $user = $request->getUser();
+    $viewer = $request->getUser();

-    $resource = id(new DrydockResource())->load($this->id);
+    $resource = id(new DrydockResourceQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($this->id))
+      ->executeOne();
    if (!$resource) {
      return new Aphront404Response();
    }
@@ -29,7 +32,7 @@ final class DrydockResourceViewController extends DrydockResourceController {
    $resource_uri = $this->getApplicationURI($resource_uri);

    $leases = id(new DrydockLeaseQuery())
-      ->setViewer($user)
+      ->setViewer($viewer)
      ->withResourceIDs(array($resource->getID()))
      ->execute();

@@ -41,7 +44,7 @@ final class DrydockResourceViewController extends DrydockResourceController {
    $pager->setOffset($request->getInt('offset'));

    $logs = id(new DrydockLogQuery())
-      ->setViewer($user)
+      ->setViewer($viewer)
      ->withResourceIDs(array($resource->getID()))
      ->executeWithOffsetPager($pager);

--- a/src/applications/drydock/management/DrydockManagementCloseWorkflow.php
+++ b/src/applications/drydock/management/DrydockManagementCloseWorkflow.php
@@ -25,8 +25,15 @@ final class DrydockManagementCloseWorkflow
        "Specify one or more resource IDs to close.");
    }

+    $viewer = PhabricatorUser::getOmnipotentUser();
+
+    $resources = id(new DrydockResourceQuery())
+      ->setViewer($viewer)
+      ->withIDs($ids)
+      ->execute();
+
    foreach ($ids as $id) {
-      $resource = id(new DrydockResource())->load($id);
+      $resource = idx($resources, $id);
      if (!$resource) {
        $console->writeErr("Resource %d does not exist!\n", $id);
      } else if ($resource->getStatus() != DrydockResourceStatus::STATUS_OPEN) {
--- a/src/applications/drydock/management/DrydockManagementCreateResourceWorkflow.php
+++ b/src/applications/drydock/management/DrydockManagementCreateResourceWorkflow.php
@@ -49,17 +49,22 @@ final class DrydockManagementCreateResourceWorkflow
      $attributes = $options->parse($attributes);
    }

-    $blueprint = id(new DrydockBlueprint())->load((int)$blueprint_id);
+    $viewer = PhabricatorUser::getOmnipotentUser();
+
+    $blueprint = id(new DrydockBlueprintQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($blueprint_id))
+      ->executeOne();
    if (!$blueprint) {
      throw new PhutilArgumentUsageException(
        "Specified blueprint does not exist.");
    }

-    $resource = new DrydockResource();
-    $resource->setBlueprintPHID($blueprint->getPHID());
-    $resource->setType($blueprint->getImplementation()->getType());
-    $resource->setName($resource_name);
-    $resource->setStatus(DrydockResourceStatus::STATUS_OPEN);
+    $resource = id(new DrydockResource())
+      ->setBlueprintPHID($blueprint->getPHID())
+      ->setType($blueprint->getImplementation()->getType())
+      ->setName($resource_name)
+      ->setStatus(DrydockResourceStatus::STATUS_OPEN);
    if ($attributes) {
      $resource->setAttributes($attributes);
    }
--- a/src/applications/drydock/query/DrydockLeaseQuery.php
+++ b/src/applications/drydock/query/DrydockLeaseQuery.php
@@ -44,17 +44,25 @@ final class DrydockLeaseQuery
  }

  public function willFilterPage(array $leases) {
-    $resources = id(new DrydockResourceQuery())
-      ->setParentQuery($this)
-      ->setViewer($this->getViewer())
-      ->withIDs(mpull($leases, 'getResourceID'))
-      ->execute();
+    $resource_ids = array_filter(mpull($leases, 'getResourceID'));
+    if ($resource_ids) {
+      $resources = id(new DrydockResourceQuery())
+        ->setParentQuery($this)
+        ->setViewer($this->getViewer())
+        ->withIDs($resource_ids)
+        ->execute();
+    } else {
+      $resources = array();
+    }

    foreach ($leases as $key => $lease) {
-      $resource = idx($resources, $lease->getResourceID());
-      if (!$resource) {
-        unset($leases[$key]);
-        continue;
+      $resource = null;
+      if ($lease->getResourceID()) {
+        $resource = idx($resources, $lease->getResourceID());
+        if (!$resource) {
+          unset($leases[$key]);
+          continue;
+        }
      }
      $lease->attachResource($resource);
    }
--- a/src/applications/drydock/query/DrydockLogQuery.php
+++ b/src/applications/drydock/query/DrydockLogQuery.php
@@ -31,18 +31,60 @@ final class DrydockLogQuery extends PhabricatorCursorPagedPolicyAwareQuery {
  }

  public function willFilterPage(array $logs) {
-    $resource_ids = mpull($logs, 'getResourceID');
-    $resources = id(new DrydockResourceQuery())
-      ->setParentQuery($this)
-      ->setViewer($this->getViewer())
-      ->withIDs($resource_ids)
-      ->execute();
+    $resource_ids = array_filter(mpull($logs, 'getResourceID'));
+    if ($resource_ids) {
+      $resources = id(new DrydockResourceQuery())
+        ->setParentQuery($this)
+        ->setViewer($this->getViewer())
+        ->withIDs($resource_ids)
+        ->execute();
+    } else {
+      $resources = array();
+    }

    foreach ($logs as $key => $log) {
-      $resource = idx($resources, $log->getResourceID());
+      $resource = null;
+      if ($log->getResourceID()) {
+        $resource = idx($resources, $log->getResourceID());
+        if (!$resource) {
+          unset($logs[$key]);
+          continue;
+        }
+      }
      $log->attachResource($resource);
    }

+    $lease_ids = array_filter(mpull($logs, 'getLeaseID'));
+    if ($lease_ids) {
+      $leases = id(new DrydockLeaseQuery())
+        ->setParentQuery($this)
+        ->setViewer($this->getViewer())
+        ->withIDs($lease_ids)
+        ->execute();
+    } else {
+      $leases = array();
+    }
+
+    foreach ($logs as $key => $log) {
+      $lease = null;
+      if ($log->getLeaseID()) {
+        $lease = idx($leases, $log->getLeaseID());
+        if (!$lease) {
+          unset($logs[$key]);
+          continue;
+        }
+      }
+      $log->attachLease($lease);
+    }
+
+    // These logs are meaningless and their policies aren't computable. They
+    // shouldn't exist, but throw them away if they do.
+    foreach ($logs as $key => $log) {
+      if (!$log->getResource() && !$log->getLease()) {
+        unset($logs[$key]);
+      }
+    }
+
    return $logs;
  }

--- a/src/applications/drydock/storage/DrydockLease.php
+++ b/src/applications/drydock/storage/DrydockLease.php
@@ -67,7 +67,7 @@ final class DrydockLease extends DrydockDAO
    return $this->assertAttached($this->resource);
  }

-  public function attachResource(DrydockResource $resource) {
+  public function attachResource(DrydockResource $resource = null) {
    $this->resource = $resource;
    return $this;
  }
@@ -194,11 +194,17 @@ final class DrydockLease extends DrydockDAO
  }

  public function getPolicy($capability) {
-    return $this->getResource()->getPolicy($capability);
+    if ($this->getResource()) {
+      return $this->getResource()->getPolicy($capability);
+    }
+    return PhabricatorPolicies::getMostOpenPolicy();
  }

  public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
-    return $this->getResource()->hasAutomaticCapability($capability, $viewer);
+    if ($this->getResource()) {
+      return $this->getResource()->hasAutomaticCapability($capability, $viewer);
+    }
+    return false;
  }

  public function describeAutomaticCapability($capability) {
--- a/src/applications/drydock/storage/DrydockLog.php
+++ b/src/applications/drydock/storage/DrydockLog.php
@@ -9,6 +9,7 @@ final class DrydockLog extends DrydockDAO
  protected $message;

  private $resource = self::ATTACHABLE;
+  private $lease = self::ATTACHABLE;

  public function getConfiguration() {
    return array(
@@ -25,6 +26,15 @@ final class DrydockLog extends DrydockDAO
    return $this->assertAttached($this->resource);
  }

+  public function attachLease(DrydockLease $lease = null) {
+    $this->lease = $lease;
+    return $this;
+  }
+
+  public function getLease() {
+    return $this->assertAttached($this->lease);
+  }
+

 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */

@@ -36,17 +46,17 @@ final class DrydockLog extends DrydockDAO
  }

  public function getPolicy($capability) {
-    if (!$this->getResource()) {
-      return PhabricatorPolicies::getMostOpenPolicy();
+    if ($this->getResource()) {
+      return $this->getResource()->getPolicy($capability);
    }
-    return $this->getResource()->getPolicy($capability);
+    return $this->getLease()->getPolicy($capability);
  }

  public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
-    if (!$this->getResource()) {
-      return false;
+    if ($this->getResource()) {
+      return $this->getResource()->hasAutomaticCapability($capability, $viewer);
    }
-    return $this->getResource()->hasAutomaticCapability($capability, $viewer);
+    return $this->getLease()->hasAutomaticCapability($capability, $viewer);
  }

  public function describeAutomaticCapability($capability) {
--- a/src/applications/drydock/storage/DrydockResource.php
+++ b/src/applications/drydock/storage/DrydockResource.php
@@ -52,6 +52,7 @@ final class DrydockResource extends DrydockDAO
  }

  public function getBlueprint() {
+    // TODO: Policy stuff.
    if (empty($this->blueprint)) {
      $blueprint = id(new DrydockBlueprint())
        ->loadOneWhere('phid = %s', $this->blueprintPHID);
@@ -62,13 +63,16 @@ final class DrydockResource extends DrydockDAO

  public function closeResource() {
    $this->openTransaction();
-      $leases = id(new DrydockLease())->loadAllWhere(
-        'resourceID = %d AND status IN (%Ld)',
-        $this->getID(),
-        array(
-          DrydockLeaseStatus::STATUS_PENDING,
-          DrydockLeaseStatus::STATUS_ACTIVE,
-        ));
+      $statuses = array(
+        DrydockLeaseStatus::STATUS_PENDING,
+        DrydockLeaseStatus::STATUS_ACTIVE,
+      );
+
+      $leases = id(new DrydockLeaseQuery())
+        ->setViewer(PhabricatorUser::getOmnipotentUser())
+        ->withResourceIDs(array($this->getID()))
+        ->withStatuses($statuses)
+        ->execute();

      foreach ($leases as $lease) {
        switch ($lease->getStatus()) {
--- a/src/applications/drydock/worker/DrydockAllocatorWorker.php
+++ b/src/applications/drydock/worker/DrydockAllocatorWorker.php
@@ -13,10 +13,13 @@ final class DrydockAllocatorWorker extends PhabricatorWorker {

  private function loadLease() {
    if (empty($this->lease)) {
-      $lease = id(new DrydockLease())->load($this->getTaskData());
+      $lease = id(new DrydockLeaseQuery())
+        ->setViewer(PhabricatorUser::getOmnipotentUser())
+        ->withIDs(array($this->getTaskData()))
+        ->executeOne();
      if (!$lease) {
        throw new PhabricatorWorkerPermanentFailureException(
-          "No such lease!");
+          pht("No such lease %d!", $this->getTaskData()));
      }
      $this->lease = $lease;
    }
@@ -53,7 +56,10 @@ final class DrydockAllocatorWorker extends PhabricatorWorker {
  }

  private function loadAllBlueprints() {
-    $instances = id(new DrydockBlueprint())->loadAll();
+    $viewer = PhabricatorUser::getOmnipotentUser();
+    $instances = id(new DrydockBlueprintQuery())
+      ->setViewer($viewer)
+      ->execute();
    $blueprints = array();
    foreach ($instances as $instance) {
      $blueprints[$instance->getPHID()] = $instance;
@@ -66,6 +72,7 @@ final class DrydockAllocatorWorker extends PhabricatorWorker {

    $blueprints = $this->loadAllBlueprints();

+    // TODO: Policy stuff.
    $pool = id(new DrydockResource())->loadAllWhere(
      'type = %s AND status = %s',
      $lease->getResourceType(),
--- a/src/applications/harbormaster/step/LeaseHostBuildStepImplementation.php
+++ b/src/applications/harbormaster/step/LeaseHostBuildStepImplementation.php
@@ -28,11 +28,13 @@ final class LeaseHostBuildStepImplementation
    $settings = $this->getSettings();

    // Create the lease.
-    $lease = new DrydockLease();
-    $lease->setResourceType('host');
-    $lease->setAttributes(
-      array('platform' => $settings['platform']));
-    $lease->queueForActivation();
+    $lease = id(new DrydockLease())
+      ->setResourceType('host')
+      ->setAttributes(
+        array(
+          'platform' => $settings['platform'],
+        ))
+      ->queueForActivation();

    // Wait until the lease is fulfilled.
    // TODO: This will throw an exception if the lease can't be fulfilled;
--- a/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php
+++ b/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php
@@ -82,6 +82,7 @@ final class HarbormasterBuildArtifact extends HarbormasterDAO
    $data = $this->getArtifactData();

    // FIXME: Is there a better way of doing this?
+    // TODO: Policy stuff, etc.
    $lease = id(new DrydockLease())->load(
      $data['drydock-lease']);
    if ($lease === null) {