Implement application lock pattern for tenants

4 files changed, 127 insertions, 62 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedTenant.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedTenant.java
new file mode 100644
index 00000000000..c8a3b52b9fc
--- /dev/null
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedTenant.java
@@ -0,0 +1,62 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.controller;
+
+import com.yahoo.config.provision.TenantName;
+import com.yahoo.vespa.athenz.api.AthenzDomain;
+import com.yahoo.vespa.curator.Lock;
+import com.yahoo.vespa.hosted.controller.api.identifiers.Property;
+import com.yahoo.vespa.hosted.controller.api.identifiers.PropertyId;
+import com.yahoo.vespa.hosted.controller.tenant.AthenzTenant;
+
+import java.util.Objects;
+import java.util.Optional;
+
+/**
+ * A tenant that has been locked for modification. Provides methods for modifying a tenant's fields.
+ *
+ * @author mpolden
+ */
+public class LockedTenant {
+
+    private final Lock lock;
+    private final TenantName name;
+    private final AthenzDomain domain;
+    private final Property property;
+    private final Optional<PropertyId> propertyId;
+
+    LockedTenant(AthenzTenant tenant, Lock lock) {
+        this(lock, tenant.name(), tenant.domain(), tenant.property(), tenant.propertyId());
+    }
+
+    private LockedTenant(Lock lock, TenantName name, AthenzDomain domain, Property property,
+                         Optional<PropertyId> propertyId) {
+        this.lock = Objects.requireNonNull(lock, "lock must be non-null");
+        this.name = Objects.requireNonNull(name, "name must be non-null");
+        this.domain = Objects.requireNonNull(domain, "domain must be non-null");
+        this.property = Objects.requireNonNull(property, "property must be non-null");
+        this.propertyId = Objects.requireNonNull(propertyId, "propertId must be non-null");
+    }
+
+    /** Returns a read-only copy of this */
+    public AthenzTenant get() {
+        return new AthenzTenant(name, domain, property, propertyId);
+    }
+
+    public LockedTenant with(AthenzDomain domain) {
+        return new LockedTenant(lock, name, domain, property, propertyId);
+    }
+
+    public LockedTenant with(Property property) {
+        return new LockedTenant(lock, name, domain, property, propertyId);
+    }
+
+    public LockedTenant with(PropertyId propertyId) {
+        return new LockedTenant(lock, name, domain, property, Optional.of(propertyId));
+    }
+
+    @Override
+    public String toString() {
+        return "tenant '" + name + "'";
+    }
+
+}
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/TenantController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/TenantController.java
index 228ca01e764..16e160d0939 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/TenantController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/TenantController.java
@@ -15,12 +15,12 @@ import com.yahoo.vespa.hosted.controller.tenant.AthenzTenant;
 import com.yahoo.vespa.hosted.controller.tenant.Tenant;
 import com.yahoo.vespa.hosted.controller.tenant.UserTenant;
 
-import java.time.Duration;
 import java.util.Comparator;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
+import java.util.function.Consumer;
 import java.util.logging.Logger;
 import java.util.stream.Collectors;
 
@@ -79,12 +79,33 @@ public class TenantController {
         }
     }
 
+    /**
+     * Lock a tenant for modification and apply action. Only valid for Athenz tenants as it's the only type that
+     * accepts modification.
+     */
+    public void lockIfPresent(TenantName name, Consumer<LockedTenant> action) {
+        try (Lock lock = lock(name)) {
+            athenzTenant(name).map(tenant -> new LockedTenant(tenant, lock)).ifPresent(action);
+        }
+    }
+
+    /** Lock a tenant for modification and apply action. Throws if the tenant does not exist */
+    public void lockOrThrow(TenantName name, Consumer<LockedTenant> action) {
+        try (Lock lock = lock(name)) {
+            action.accept(new LockedTenant(requireAthenzTenant(name), lock));
+        }
+    }
+
+    /** Replace and store any previous version of given tenant */
+    public void store(LockedTenant tenant) {
+        curator.writeTenant(tenant.get());
+    }
+
     /** Create an user tenant with given username */
     public void create(UserTenant tenant) {
         try (Lock lock = lock(tenant.name())) {
             requireNonExistent(tenant.name());
             curator.writeTenant(tenant);
-            log.info("Created " + tenant);
         }
     }
 
@@ -103,7 +124,6 @@ public class TenantController {
             }
             athenzClientFactory.createZmsClientWithAuthorizedServiceToken(token).createTenant(domain);
             curator.writeTenant(tenant);
-            log.info("Created " + tenant);
         }
     }
 
@@ -129,14 +149,29 @@ public class TenantController {
         return curator.readAthenzTenant(name);
     }
 
-    /** Update Athenz tenant */
-    public void updateTenant(AthenzTenant updatedTenant, NToken token) {
-        try (Lock lock = lock(updatedTenant.name())) {
-            requireExists(updatedTenant.name());
-            updateAthenzDomain(updatedTenant, token);
-            curator.writeTenant(updatedTenant);
-            log.info("Updated " + updatedTenant);
-        }
+    /** Returns Athenz tenant with name or throws if no such tenant exists */
+    public AthenzTenant requireAthenzTenant(TenantName name) {
+        return athenzTenant(name).orElseThrow(() -> new IllegalArgumentException("Tenant '" + name + "' not found"));
+    }
+
+    /** Update Athenz domain for tenant. Returns the updated tenant which must be explicitly stored */
+    public LockedTenant withDomain(LockedTenant tenant, AthenzDomain newDomain, NToken token) {
+        AthenzDomain existingDomain = tenant.get().domain();
+        if (existingDomain.equals(newDomain)) return tenant;
+        Optional<Tenant> existingTenantWithNewDomain = tenantIn(newDomain);
+        if (existingTenantWithNewDomain.isPresent())
+            throw new IllegalArgumentException("Could not set domain of " + tenant + " to '" + newDomain +
+                                               "':" + existingTenantWithNewDomain.get() + " already has this domain");
+
+        ZmsClient zmsClient = athenzClientFactory.createZmsClientWithAuthorizedServiceToken(token);
+        zmsClient.createTenant(newDomain);
+        List<Application> applications = controller.applications().asList(tenant.get().name());
+        applications.forEach(a -> zmsClient.addApplication(newDomain, new com.yahoo.vespa.hosted.controller.api.identifiers.ApplicationId(a.id().application().value())));
+        applications.forEach(a -> zmsClient.deleteApplication(existingDomain, new com.yahoo.vespa.hosted.controller.api.identifiers.ApplicationId(a.id().application().value())));
+        zmsClient.deleteTenant(existingDomain);
+        log.info("Set Athenz domain for '" + tenant + "' from '" + existingDomain + "' to '" + newDomain + "'");
+
+        return tenant.with(newDomain);
     }
 
     /** Delete an user tenant */
@@ -160,28 +195,6 @@ public class TenantController {
                                                + "': This tenant has active applications");
         }
         curator.removeTenant(name);
-        log.info("Deleted " + name);
-    }
-
-    private void updateAthenzDomain(AthenzTenant updatedTenant, NToken token) {
-        Optional<AthenzTenant> existingTenant = athenzTenant(updatedTenant.name());
-        if ( ! existingTenant.isPresent()) return;
-
-        AthenzDomain existingDomain = existingTenant.get().domain();
-        AthenzDomain newDomain = updatedTenant.domain();
-        if (existingDomain.equals(newDomain)) return;
-        Optional<Tenant> existingTenantWithNewDomain = tenantIn(newDomain);
-        if (existingTenantWithNewDomain.isPresent())
-            throw new IllegalArgumentException("Could not set domain of " + updatedTenant + " to '" + newDomain +
-                                               "':" + existingTenantWithNewDomain.get() + " already has this domain");
-
-        ZmsClient zmsClient = athenzClientFactory.createZmsClientWithAuthorizedServiceToken(token);
-        zmsClient.createTenant(newDomain);
-        List<Application> applications = controller.applications().asList(existingTenant.get().name());
-        applications.forEach(a -> zmsClient.addApplication(newDomain, new com.yahoo.vespa.hosted.controller.api.identifiers.ApplicationId(a.id().application().value())));
-        applications.forEach(a -> zmsClient.deleteApplication(existingDomain, new com.yahoo.vespa.hosted.controller.api.identifiers.ApplicationId(a.id().application().value())));
-        zmsClient.deleteTenant(existingDomain);
-        log.info("Updated Athens domain for " + updatedTenant + " from " + existingDomain + " to " + newDomain);
     }
 
     private void requireNonExistent(TenantName name) {
@@ -193,12 +206,6 @@ public class TenantController {
         }
     }
 
-    private void requireExists(TenantName name) {
-        if (!tenant(name).isPresent()) {
-            throw new IllegalArgumentException("Tenant '" + name + "' does not exist");
-        }
-    }
-
     /**
      * Returns a lock which provides exclusive rights to changing this tenant.
      * Any operation which stores a tenant need to first acquire this lock, then read, modify
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
index 07286fda90b..4acede561b9 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
@@ -642,19 +642,27 @@ public class ApplicationApiHandler extends LoggingRequestHandler {
     }
 
     private HttpResponse updateTenant(String tenantName, HttpRequest request) {
-        Optional<AthenzTenant> existingTenant = controller.tenants().athenzTenant(TenantName.from(tenantName));
-        if ( ! existingTenant.isPresent()) return ErrorResponse.notFoundError("Tenant '" + tenantName + "' does not exist");
+        Optional<AthenzTenant> tenant = controller.tenants().athenzTenant(TenantName.from(tenantName));
+        if ( ! tenant.isPresent()) return ErrorResponse.notFoundError("Tenant '" + tenantName + "' does not exist");
 
         Inspector requestData = toSlime(request.getData()).get();
-        AthenzTenant updatedTenant = existingTenant.get()
-                                                   .with(new AthenzDomain(mandatory("athensDomain", requestData).asString()))
-                                                   .with(new Property(mandatory("property", requestData).asString()));
-        Optional<PropertyId> propertyId = optional("propertyId", requestData).map(PropertyId::new);
-        if (propertyId.isPresent()) {
-            updatedTenant = updatedTenant.with(propertyId.get());
-        }
-        controller.tenants().updateTenant(updatedTenant, requireNToken(request, "Could not update " + tenantName));
-        return tenant(updatedTenant, request, true);
+        NToken token = requireNToken(request, "Could not update " + tenantName);
+
+        controller.tenants().lockOrThrow(tenant.get().name(), lockedTenant -> {
+            lockedTenant = lockedTenant.with(new Property(mandatory("property", requestData).asString()));
+            lockedTenant = controller.tenants().withDomain(
+                    lockedTenant,
+                    new AthenzDomain(mandatory("athensDomain", requestData).asString()),
+                    token
+            );
+            Optional<PropertyId> propertyId = optional("propertyId", requestData).map(PropertyId::new);
+            if (propertyId.isPresent()) {
+                lockedTenant = lockedTenant.with(propertyId.get());
+            }
+            controller.tenants().store(lockedTenant);
+        });
+
+        return tenant(controller.tenants().requireAthenzTenant(tenant.get().name()), request, true);
     }
 
     private HttpResponse createTenant(String tenantName, HttpRequest request) {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/tenant/AthenzTenant.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/tenant/AthenzTenant.java
index 0ba0eea2dab..52d205e4eeb 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/tenant/AthenzTenant.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/tenant/AthenzTenant.java
@@ -55,18 +55,6 @@ public class AthenzTenant extends Tenant {
         return "athenz tenant '" + name() + "'";
     }
 
-    public AthenzTenant with(AthenzDomain domain) {
-        return new AthenzTenant(name(), domain, property(), propertyId());
-    }
-
-    public AthenzTenant with(Property property) {
-        return new AthenzTenant(name(), domain, property, propertyId());
-    }
-
-    public AthenzTenant with(PropertyId propertyId) {
-        return new AthenzTenant(name(), domain, property, Optional.of(propertyId));
-    }
-
     /** Create a new Athenz tenant */
     public static AthenzTenant create(TenantName name, AthenzDomain domain, Property property,
                                       Optional<PropertyId> propertyId) {