Separate code path for conservative tenant access control updates

4 files changed, 77 insertions, 27 deletions

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 b374a5bda82..c75e561036d 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
@@ -107,7 +107,7 @@ public class TenantController {
     public void create(TenantPermit permit) {
         try (Lock lock = lock(permit.tenant())) {
             requireNonExistent(permit.tenant());
-            curator.writeTenant(permits.createTenant(permit, asList(), Collections.emptyList()));
+            curator.writeTenant(permits.createTenant(permit, asList()));
         }
     }
 
@@ -136,13 +136,7 @@ public class TenantController {
     /** Updates the tenant contained in the given permit with new data. */
     public void update(TenantPermit permit) {
         try (Lock lock = lock(permit.tenant())) {
-            Tenant tenant = require(permit.tenant());
-            List<Tenant> otherTenants = new ArrayList<>(asList());
-            otherTenants.remove(tenant);
-
-            List<Application> applications = controller.applications().asList(permit.tenant());
-            permits.deleteTenant(permit, tenant, applications);
-            curator.writeTenant(permits.createTenant(permit, otherTenants, applications));
+            curator.writeTenant(permits.updateTenant(permit, asList(), controller.applications().asList(permit.tenant())));
         }
     }
 
@@ -155,7 +149,7 @@ public class TenantController {
                                                    + "': This tenant has active applications");
 
             curator.removeTenant(tenant.name());
-            permits.deleteTenant(permit, tenant, controller.applications().asList(permit.tenant()));
+            permits.deleteTenant(permit, tenant);
         }
     }
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/impl/AthenzFacade.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/impl/AthenzFacade.java
index 99ed7e3f9cb..887db2832ce 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/impl/AthenzFacade.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/impl/AthenzFacade.java
@@ -37,6 +37,7 @@ import java.util.stream.Collectors;
 
 /**
  * @author bjorncs
+ * @author jonmv
  */
 public class AthenzFacade implements AccessControlManager {
 
@@ -57,7 +58,7 @@ public class AthenzFacade implements AccessControlManager {
     }
 
     @Override
-    public Tenant createTenant(TenantPermit permit, List<Tenant> existing, List<Application> applications) {
+    public Tenant createTenant(TenantPermit permit, List<Tenant> existing) {
         AthenzTenantPermit athenzPermit = (AthenzTenantPermit) permit;
         AthenzDomain domain = athenzPermit.domain()
                                           .orElseThrow(() -> new IllegalArgumentException("Must provide Athenz domain."));
@@ -72,38 +73,79 @@ public class AthenzFacade implements AccessControlManager {
         verifyIsDomainAdmin(((AthenzPrincipal) athenzPermit.user()).getIdentity(), domain);
 
         Optional<Tenant> existingWithSameDomain = existing.stream()
-                .filter(existingTenant ->    existingTenant instanceof AthenzTenant
-                                          && domain.equals(((AthenzTenant) existingTenant).domain()))
-                .findAny();
+                                                          .filter(existingTenant ->    existingTenant.type() == Tenant.Type.athenz
+                                                                                       && domain.equals(((AthenzTenant) existingTenant).domain()))
+                                                          .findAny();
 
-        if (existingWithSameDomain.isPresent()) { // Throw if domain taken by someone else, or do nothing if taken by this tenant.
+        if (existingWithSameDomain.isPresent()) { // Throw if domain is already taken.
             if ( ! existingWithSameDomain.get().name().equals(permit.tenant()))
                 throw new IllegalArgumentException("Could not create tenant '" + athenzPermit.tenant().value() +
                                                    "': The Athens domain '" +
                                                    domain.getName() + "' is already connected to tenant '" +
                                                    existingWithSameDomain.get().name().value() + "'");
         }
-        else { // Create tenant, and optionally application, resources in Athenz if domain is not already taken.
+        else { // Create tenant resources in Athenz if domain is not already taken.
             log("createTenancy(tenantDomain=%s, service=%s)", athenzPermit.domain(), service);
             zmsClient.createTenancy(domain, service, athenzPermit.token());
+        }
+
+        return tenant;
+    }
+    @Override
+    public Tenant updateTenant(TenantPermit permit, List<Tenant> existing, List<Application> applications) {
+        AthenzTenantPermit athenzPermit = (AthenzTenantPermit) permit;
+        AthenzDomain domain = athenzPermit.domain()
+                                          .orElseThrow(() -> new IllegalArgumentException("Must provide Athenz domain."));
 
+        Tenant tenant = AthenzTenant.create(athenzPermit.tenant(),
+                                            athenzPermit.domain()
+                                                        .orElseThrow(() -> new IllegalArgumentException("Must provide Athenz domain.")),
+                                            athenzPermit.property()
+                                                        .orElseThrow(() -> new IllegalArgumentException("Must provide property.")),
+                                            athenzPermit.propertyId());
+
+        verifyIsDomainAdmin(((AthenzPrincipal) athenzPermit.user()).getIdentity(), domain);
+
+        AthenzTenant oldTenant = existing.stream()
+                                         .filter(existingTenant -> existingTenant.name().equals(permit.tenant()))
+                                         .findAny()
+                                         .map(AthenzTenant.class::cast)
+                                         .orElseThrow(() -> new IllegalArgumentException("Cannot update a non-existent tenant."));
+
+        Optional<Tenant> existingWithSameDomain = existing.stream()
+                                                          .filter(existingTenant ->    existingTenant.type() == Tenant.Type.athenz
+                                                                                    && domain.equals(((AthenzTenant) existingTenant).domain()))
+                                                          .findAny();
+
+        if (existingWithSameDomain.isPresent()) { // Throw if domain taken by someone else, or do nothing if taken by this tenant.
+            if ( ! existingWithSameDomain.get().equals(oldTenant))
+                throw new IllegalArgumentException("Could not create tenant '" + athenzPermit.tenant().value() +
+                                                   "': The Athens domain '" +
+                                                   domain.getName() + "' is already connected to tenant '" +
+                                                   existingWithSameDomain.get().name().value() + "'");
+
+            return tenant; // Short-circuit here if domain is still the same.
+        }
+        else { // Delete and recreate tenant, and optionally application, resources in Athenz otherwise.
+            log("createTenancy(tenantDomain=%s, service=%s)", athenzPermit.domain(), service);
+            zmsClient.createTenancy(domain, service, athenzPermit.token());
             for (Application application : applications)
                 createApplication(domain, application.id().application(), athenzPermit.token());
+
+            log("deleteTenancy(tenantDomain=%s, service=%s)", athenzPermit.domain(), service);
+            for (Application application : applications)
+                deleteApplication(oldTenant.domain(), application.id().application(), athenzPermit.token());
+            zmsClient.deleteTenancy(oldTenant.domain(), service, athenzPermit.token());
         }
 
         return tenant;
     }
 
     @Override
-    public void deleteTenant(TenantPermit permit, Tenant tenant, List<Application> applications) {
+    public void deleteTenant(TenantPermit permit, Tenant tenant) {
         AthenzTenantPermit athenzPermit = (AthenzTenantPermit) permit;
         AthenzDomain domain = ((AthenzTenant) tenant).domain();
 
-        for (Application application : applications)
-            deleteApplication(domain,
-                              application.id().application(),
-                              athenzPermit.token());
-
         log("deleteTenancy(tenantDomain=%s, service=%s)", athenzPermit.domain(), service);
         zmsClient.deleteTenancy(domain, service, athenzPermit.token());
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/permits/AccessControlManager.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/permits/AccessControlManager.java
index 0d68ab7512c..7a4544a13ed 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/permits/AccessControlManager.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/permits/AccessControlManager.java
@@ -23,19 +23,27 @@ public interface AccessControlManager {
      *
      * @param tenantPermit permit for the tenant to create
      * @param existing list of existing tenants, to check for conflicts
-     * @param applications list of applications this tenant already owns
      * @return the created tenant, for keeping
      */
-    Tenant createTenant(TenantPermit tenantPermit, List<Tenant> existing, List<Application> applications);
+    Tenant createTenant(TenantPermit tenantPermit, List<Tenant> existing);
+
+    /**
+     * Modifies up permissions for a tenant, based on the given permit, or throws.
+     *
+     * @param tenantPermit permit for the tenant to update
+     * @param existing list of existing tenants, to check for conflicts
+     * @param applications list of applications this tenant already owns
+     * @return the updated tenant, for keeping
+     */
+    Tenant updateTenant(TenantPermit tenantPermit, List<Tenant> existing, List<Application> applications);
 
     /**
      * Removes all permissions for tenant in the given permit, and for any applications it owns, or throws.
      *
      * @param tenantPermit permit for the tenant to delete
      * @param tenant the tenant to delete
-     * @param applications list of applications this tenant owns
      */
-    void deleteTenant(TenantPermit tenantPermit, Tenant tenant, List<Application> applications);
+    void deleteTenant(TenantPermit tenantPermit, Tenant tenant);
 
     /**
      * Sets up permissions for an application, based on the given permit, or throws.
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/permits/CloudAccessControlManager.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/permits/CloudAccessControlManager.java
index d72eb0087a0..4c1133d983c 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/permits/CloudAccessControlManager.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/permits/CloudAccessControlManager.java
@@ -6,6 +6,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.organization.Marketplac
 import com.yahoo.vespa.hosted.controller.tenant.CloudTenant;
 import com.yahoo.vespa.hosted.controller.tenant.Tenant;
 
+import javax.ws.rs.NotSupportedException;
 import java.security.Principal;
 import java.util.Collections;
 import java.util.List;
@@ -24,7 +25,7 @@ public class CloudAccessControlManager implements AccessControlManager {
     }
 
     @Override
-    public CloudTenant createTenant(TenantPermit permit, List<Tenant> existing, List<Application> applications) {
+    public CloudTenant createTenant(TenantPermit permit, List<Tenant> existing) {
         CloudTenantPermit cloudPermit = (CloudTenantPermit) permit;
 
         // Do things ...
@@ -33,7 +34,12 @@ public class CloudAccessControlManager implements AccessControlManager {
     }
 
     @Override
-    public void deleteTenant(TenantPermit permit, Tenant tenant, List<Application> applications) {
+    public Tenant updateTenant(TenantPermit tenantPermit, List<Tenant> existing, List<Application> applications) {
+        throw new NotSupportedException("Update is not supported here, as it would entail changing the tenant name.");
+    }
+
+    @Override
+    public void deleteTenant(TenantPermit permit, Tenant tenant) {
 
         // Probably delete customer subscription?