Merge pull request #4876 from vespa-engine/jvenstad/dont-revalidate-application-name

Jvenstad/dont revalidate application name

5 files changed, 64 insertions, 52 deletions

diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/identifiers/Identifier.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/identifiers/Identifier.java
index bdcde7360fc..2fd75e25498 100644
--- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/identifiers/Identifier.java
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/identifiers/Identifier.java
@@ -13,10 +13,10 @@ import java.util.regex.Pattern;
 public abstract class Identifier {
 
     protected static final String strictPatternExplanation =
-            "New tenant or application names must start with a letter, may contain no more than 30 " +
+            "New tenant or application names must start with a letter, may contain no more than 20 " +
             "characters, and may only contain lowercase letters, digits or dashes, but no double-dashes.";
     // TODO: Use this also for instances, if they ever get proper support.
-    protected static final Pattern strictPattern = Pattern.compile("^(?=.{1,30}$)[a-z](-?[a-z0-9]+)*$");
+    protected static final Pattern strictPattern = Pattern.compile("^(?=.{1,20}$)[a-z](-?[a-z0-9]+)*$");
     private static final Pattern serializedIdentifierPattern = Pattern.compile("[a-zA-Z0-9_-]+");
     private static final Pattern serializedPattern = Pattern.compile("[a-zA-Z0-9_.-]+");
 
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..50d95c232c4 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,13 +236,12 @@ 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().isDefault() || 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());
+            // Validate only application names which do not already exist.
+            if (asList(id.tenant()).stream().noneMatch(application -> application.id().application().equals(id.application())))
+                com.yahoo.vespa.hosted.controller.api.identifiers.ApplicationId.validate(id.application().value());
 
             Optional<Tenant> tenant = controller.tenants().tenant(new TenantId(id.tenant().value()));
             if ( ! tenant.isPresent())
@@ -251,16 +250,11 @@ 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 (id.instance().isDefault() && tenant.get().isAthensTenant()) { // Only create the athens application for "default" instances.
+                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) {
-                }
                 zmsClient.addApplication(tenant.get().getAthensDomain().get(),
                                          new com.yahoo.vespa.hosted.controller.api.identifiers.ApplicationId(id.application().value()));
             }
@@ -277,13 +271,9 @@ public class ApplicationController {
                                             Optional<ApplicationPackage> applicationPackageFromDeployer,
                                             DeployOptions options) {
         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, Optional.empty()), lock));
 
             // Determine Vespa version to use
             Version version;
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..46f045df215 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
@@ -16,9 +16,11 @@ import com.yahoo.vespa.config.SlimeUtils;
 import com.yahoo.vespa.hosted.controller.api.Tenant;
 import com.yahoo.vespa.hosted.controller.api.application.v4.model.DeployOptions;
 import com.yahoo.vespa.hosted.controller.api.application.v4.model.EndpointStatus;
+import com.yahoo.vespa.hosted.controller.api.application.v4.model.ScrewdriverBuildJob;
 import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId;
 import com.yahoo.vespa.hosted.controller.api.identifiers.Property;
 import com.yahoo.vespa.hosted.controller.api.identifiers.PropertyId;
+import com.yahoo.vespa.hosted.controller.api.identifiers.ScrewdriverId;
 import com.yahoo.vespa.hosted.controller.api.identifiers.TenantId;
 import com.yahoo.vespa.hosted.controller.api.identifiers.UserGroup;
 import com.yahoo.vespa.hosted.controller.api.integration.BuildService.BuildJob;
@@ -38,6 +40,7 @@ import com.yahoo.vespa.hosted.controller.athenz.mock.AthenzDbMock;
 import com.yahoo.vespa.hosted.controller.deployment.ApplicationPackageBuilder;
 import com.yahoo.vespa.hosted.controller.deployment.BuildSystem;
 import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester;
+import com.yahoo.vespa.hosted.controller.persistence.ApplicationSerializer;
 import com.yahoo.vespa.hosted.controller.rotation.RotationId;
 import com.yahoo.vespa.hosted.controller.rotation.RotationLock;
 import com.yahoo.vespa.hosted.controller.versions.DeploymentStatistics;
@@ -54,6 +57,7 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.NoSuchElementException;
 import java.util.Optional;
 import java.util.function.Supplier;
 
@@ -361,7 +365,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());
@@ -571,7 +575,7 @@ public class ControllerTest {
         // Setup tester and app def
         ControllerTester tester = new ControllerTester();
         ZoneId zone = ZoneId.from(Environment.defaultEnvironment(), RegionName.defaultName());
-        ApplicationId appId = tester.applicationId("tenant", "app1", "default");
+        ApplicationId appId = ApplicationId.from("tenant", "app1", "default");
         DeploymentId deployId = new DeploymentId(appId, zone);
 
         // Check initial rotation status
@@ -857,34 +861,34 @@ public class ControllerTest {
         // Component notifies completion
         tester.notifyJobCompletion(component, app, Optional.empty(), sourceRevision, initialBuildNumber);
         ApplicationVersion change = sourceRevision.map(sr -> ApplicationVersion.from(sr, initialBuildNumber))
-                                                  .orElse(ApplicationVersion.unknown);
+                .orElse(ApplicationVersion.unknown);
         assertEquals(change.id(), tester.controller().applications()
-                                        .require(application)
-                                        .change().application().get().id());
+                .require(application)
+                .change().application().get().id());
 
         // Deploy in test
         tester.deployAndNotify(app, applicationPackage, true, systemTest);
         tester.deployAndNotify(app, applicationPackage, true, stagingTest);
         //assertEquals(4, applications.require(application).deploymentJobs().jobStatus().size());
         assertStatus(JobStatus.initial(stagingTest)
-                              .withTriggering(vespaVersion, expectedVersion, false, "",
-                                              tester.clock().instant().minus(Duration.ofMillis(1)))
-                              .withCompletion(42, Optional.empty(), tester.clock().instant(),
-                                              tester.controller()), application, tester.controller());
+                             .withTriggering(vespaVersion, expectedVersion, false, "",
+                                             tester.clock().instant().minus(Duration.ofMillis(1)))
+                             .withCompletion(42, Optional.empty(), tester.clock().instant(),
+                                             tester.controller()), application, tester.controller());
 
         // Deploy in production
         tester.deployAndNotify(app, applicationPackage, true, productionCorpUsEast1);
         assertStatus(JobStatus.initial(productionCorpUsEast1)
-                              .withTriggering(vespaVersion, expectedVersion, false, "",
-                                              tester.clock().instant().minus(Duration.ofMillis(1)))
-                              .withCompletion(42, Optional.empty(), tester.clock().instant(),
-                                              tester.controller()), application, tester.controller());
+                             .withTriggering(vespaVersion, expectedVersion, false, "",
+                                             tester.clock().instant().minus(Duration.ofMillis(1)))
+                             .withCompletion(42, Optional.empty(), tester.clock().instant(),
+                                             tester.controller()), application, tester.controller());
         tester.deployAndNotify(app, applicationPackage, true, true, productionUsEast3);
         assertStatus(JobStatus.initial(productionUsEast3)
-                              .withTriggering(vespaVersion, expectedVersion, false, "",
-                                              tester.clock().instant().minus(Duration.ofMillis(1)))
-                              .withCompletion(42, Optional.empty(), tester.clock().instant(),
-                                              tester.controller()), application, tester.controller());
+                             .withTriggering(vespaVersion, expectedVersion, false, "",
+                                             tester.clock().instant().minus(Duration.ofMillis(1)))
+                             .withCompletion(42, Optional.empty(), tester.clock().instant(),
+                                             tester.controller()), application, tester.controller());
 
         // Verify deployed version
         app = tester.controller().applications().require(app.id());
@@ -893,4 +897,34 @@ public class ControllerTest {
         }
     }
 
+    @Test
+    public void testDeploymentOfNewInstanceWithIllegalApplicationName() {
+        ControllerTester tester = new ControllerTester();
+        String application = "this_application_name_is_far_too_long_and_has_underscores";
+        ZoneId zone = ZoneId.from("test", "us-east-1");
+        DeployOptions options = new DeployOptions(Optional.of(new ScrewdriverBuildJob(new ScrewdriverId("123"),
+                                                                                      null)),
+                                                  Optional.empty(),
+                                                  false,
+                                                  false);
+
+        tester.createTenant("tenant", "domain", null);
+
+        // Deploy an application which doesn't yet exist, and which has an illegal application name.
+        try {
+            tester.controller().applications().deployApplication(ApplicationId.from("tenant", application, "123"), zone, Optional.empty(), options);
+            fail("Illegal application name should cause validation exception.");
+        }
+        catch (IllegalArgumentException e) {
+            assertTrue(e.getMessage().contains("Invalid id"));
+        }
+
+        // Sneak an illegal application in the back door.
+        tester.createApplication(new ApplicationSerializer().toSlime(new Application(ApplicationId.from("tenant", application, "default"))));
+
+        // Deploy a PR instance for the application, with no NToken.
+        tester.controller().applications().deployApplication(ApplicationId.from("tenant", application, "456"), zone, Optional.empty(), options);
+        assertTrue(tester.controller().applications().get(ApplicationId.from("tenant", application, "456")).isPresent());
+    }
+
 }
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..ccc1798358d 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
@@ -2,11 +2,8 @@
 package com.yahoo.vespa.hosted.controller;
 
 import com.yahoo.config.provision.ApplicationId;
-import com.yahoo.config.provision.ApplicationName;
 import com.yahoo.config.provision.Environment;
-import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.RegionName;
-import com.yahoo.config.provision.TenantName;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.ArtifactRepository;
 import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId;
 import com.yahoo.slime.Slime;
@@ -199,7 +196,7 @@ public final class ControllerTester {
     }
 
     public Application createApplication(TenantId tenant, String applicationName, String instanceName, long projectId) {
-        ApplicationId applicationId = applicationId(tenant.id(), applicationName, instanceName);
+        ApplicationId applicationId = ApplicationId.from(tenant.id(), applicationName, instanceName);
         controller().applications().createApplication(applicationId, Optional.of(TestIdentities.userNToken));
         controller().applications().lockOrThrow(applicationId, lockedApplication ->
                 controller().applications().store(lockedApplication.withProjectId(projectId)));
@@ -227,12 +224,6 @@ public final class ControllerTester {
                                                       new DeployOptions(Optional.of(new ScrewdriverBuildJob(app1ScrewdriverId, app1RevisionId)), Optional.empty(), false, deployCurrentVersion));
     }
 
-    public ApplicationId applicationId(String tenant, String application, String instance) {
-        return ApplicationId.from(TenantName.from(tenant),
-                                  ApplicationName.from(application),
-                                  InstanceName.from(instance));
-    }
-
     // Used by ApplicationSerializerTest to avoid breaking encapsulation. Should not be used by anything else
     public static LockedApplication writable(Application application) {
         return new LockedApplication(application, new Lock("/test", new MockCurator()));
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);