Cleanup and proper application creation on deploy

6 files changed, 19 insertions, 24 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
index 407f5b65e81..b4b27037499 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
@@ -236,11 +236,8 @@ public class ApplicationController {
      * @throws IllegalArgumentException if the application already exists
      */
     public Application createApplication(ApplicationId id, Optional<NToken> token) {
-        // TODO: TLS: PR names change to prXXX.
-        if ( ! (   id.instance().value().equals("default")
-                || id.instance().value().matches("^default-pr\\d+$") // TODO: Remove when these no longer deploy.
-                || id.instance().value().matches("^\\d+$"))) // TODO: Support instances properly
-            throw new UnsupportedOperationException("Only the instance names 'default' and names starting with 'default-pr' or which are just the PR number are supported at the moment");
+        if ( ! (id.instance().value().equals("default") || id.instance().value().matches("\\d+"))) // TODO: Support instances properly
+            throw new UnsupportedOperationException("Only the instance names 'default' and names which are just the PR number are supported at the moment");
         try (Lock lock = lock(id)) {
             com.yahoo.vespa.hosted.controller.api.identifiers.ApplicationId.validate(id.application().value());
 
@@ -251,16 +248,16 @@ public class ApplicationController {
                 throw new IllegalArgumentException("Could not create '" + id + "': Application already exists");
             if (get(dashToUnderscore(id)).isPresent()) // VESPA-1945
                 throw new IllegalArgumentException("Could not create '" + id + "': Application " + dashToUnderscore(id) + " already exists");
-            if (tenant.get().isAthensTenant() && ! token.isPresent())
-                throw new IllegalArgumentException("Could not create '" + id + "': No NToken provided");
             if (tenant.get().isAthensTenant()) {
+                if ( ! token.isPresent())
+                throw new IllegalArgumentException("Could not create '" + id + "': No NToken provided");
+
                 ZmsClient zmsClient = zmsClientFactory.createZmsClientWithAuthorizedServiceToken(token.get());
                 try {
                     zmsClient.deleteApplication(tenant.get().getAthensDomain().get(),
                                                 new com.yahoo.vespa.hosted.controller.api.identifiers.ApplicationId(id.application().value()));
                 }
-                catch (ZmsException ignored) {
-                }
+                catch (ZmsException ignored) { }
                 zmsClient.addApplication(tenant.get().getAthensDomain().get(),
                                          new com.yahoo.vespa.hosted.controller.api.identifiers.ApplicationId(id.application().value()));
             }
@@ -275,15 +272,12 @@ public class ApplicationController {
     // TODO: Get rid of the options arg
     public ActivateResult deployApplication(ApplicationId applicationId, ZoneId zone,
                                             Optional<ApplicationPackage> applicationPackageFromDeployer,
-                                            DeployOptions options) {
+                                            DeployOptions options,
+                                            Optional<NToken> token) {
         try (Lock lock = lock(applicationId)) {
-            // TODO: Move application creation outside, to the deploy call in the handler.
             LockedApplication application = get(applicationId)
                     .map(app -> new LockedApplication(app, lock))
-                    .orElseGet(() -> {
-                        com.yahoo.vespa.hosted.controller.api.identifiers.ApplicationId.validate(applicationId.application().value());
-                        return new LockedApplication(new Application(applicationId), lock);
-                    });
+                    .orElseGet(() -> new LockedApplication(createApplication(applicationId, token), lock));
 
             // Determine Vespa version to use
             Version version;
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 77e0fb1ef05..443c9964cdd 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
@@ -783,7 +783,7 @@ public class ApplicationApiHandler extends LoggingRequestHandler {
                                                                   .map(ApplicationPackage::new);
         DeployAuthorizer deployAuthorizer = new DeployAuthorizer(controller.zoneRegistry(), athenzClientFactory);
         Tenant tenant = controller.tenants().tenant(new TenantId(tenantName)).orElseThrow(() -> new NotExistsException(new TenantId(tenantName)));
-        Principal principal = authorizer.getPrincipal(request);
+        AthenzPrincipal principal = authorizer.getPrincipal(request);
         deployAuthorizer.throwIfUnauthorizedForDeploy(principal, Environment.from(environment), tenant, applicationId, applicationPackage);
 
         // TODO: get rid of the json object
@@ -794,7 +794,8 @@ public class ApplicationApiHandler extends LoggingRequestHandler {
         ActivateResult result = controller.applications().deployApplication(applicationId,
                                                                             zone,
                                                                             applicationPackage,
-                                                                            deployOptionsJsonClass);
+                                                                            deployOptionsJsonClass,
+                                                                            principal.getNToken());
         return new SlimeJsonResponse(toSlime(result));
     }
 
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
index 5b71d12a68b..4100995566e 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
@@ -361,7 +361,7 @@ public class ControllerTest {
 
         // pull-request deployment - uses different instance id
         ApplicationId app1pr = tester.createAndDeploy("tenant1",  "domain1",
-                                                      "application1", "default-pr1",
+                                                      "application1", "1",
                                                       Environment.staging, app1ProjectId, null).id();
 
         assertTrue(applications.get(app1).isPresent());
@@ -838,7 +838,8 @@ public class ControllerTest {
         // Same options as used in our integration tests
         DeployOptions options = new DeployOptions(Optional.empty(), Optional.empty(), false,
                                                   false);
-        tester.controller().applications().deployApplication(app.id(), zone, Optional.of(applicationPackage), options);
+        tester.controller().applications().deployApplication(app.id(), zone, Optional.of(applicationPackage), options,
+                                                             Optional.of(TestIdentities.userNToken));
 
         assertTrue("Application deployed and activated",
                    tester.controllerTester().configServer().activated().getOrDefault(app.id(), false));
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java
index 3b574ac606b..cd7ea85ea76 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java
@@ -224,7 +224,8 @@ public final class ControllerTester {
         controller().applications().deployApplication(application.id(),
                                                       zone,
                                                       applicationPackage,
-                                                      new DeployOptions(Optional.of(new ScrewdriverBuildJob(app1ScrewdriverId, app1RevisionId)), Optional.empty(), false, deployCurrentVersion));
+                                                      new DeployOptions(Optional.of(new ScrewdriverBuildJob(app1ScrewdriverId, app1RevisionId)), Optional.empty(), false, deployCurrentVersion),
+                                                      Optional.of(TestIdentities.userNToken));
     }
 
     public ApplicationId applicationId(String tenant, String application, String instance) {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java
index 5b806d580e2..4a92d671210 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java
@@ -82,7 +82,8 @@ public class ContainerControllerTester {
         controller().applications().deployApplication(application.id(),
                                                     zone,
                                                     Optional.of(applicationPackage),
-                                                    new DeployOptions(Optional.of(new ScrewdriverBuildJob(app1ScrewdriverId, app1RevisionId)), Optional.empty(), false, false));
+                                                    new DeployOptions(Optional.of(new ScrewdriverBuildJob(app1ScrewdriverId, app1RevisionId)), Optional.empty(), false, false),
+                                                      Optional.of(TestIdentities.userNToken));
         return application;
     }
 
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java
index 7745cae1cb9..ee6b61f5582 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java
@@ -140,9 +140,6 @@ public class VersionStatusTest {
         Application ignored0 = tester.createApplication("ignored0", "tenant1", 1000, 1000L);
 
         // Pull request builds
-         tester.controllerTester().createApplication(new TenantId("tenant1"),
-                                                                           "ignored1",
-                                                                           "default-pr42", 1000);
         tester.controllerTester().createApplication(new TenantId("tenant1"),
                                                     "ignored1",
                                                     "43", 1000);