Retry real deployment until it has correct cert

3 files changed, 68 insertions, 16 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java
index a3beb131b75..47478d14c33 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java
@@ -88,7 +88,9 @@ import static com.yahoo.vespa.hosted.controller.deployment.RunStatus.error;
 import static com.yahoo.vespa.hosted.controller.deployment.RunStatus.installationFailed;
 import static com.yahoo.vespa.hosted.controller.deployment.RunStatus.outOfCapacity;
 import static com.yahoo.vespa.hosted.controller.deployment.RunStatus.running;
+import static com.yahoo.vespa.hosted.controller.deployment.RunStatus.success;
 import static com.yahoo.vespa.hosted.controller.deployment.RunStatus.testFailure;
+import static com.yahoo.vespa.hosted.controller.deployment.Step.Status.succeeded;
 import static com.yahoo.vespa.hosted.controller.deployment.Step.copyVespaLogs;
 import static com.yahoo.vespa.hosted.controller.deployment.Step.deactivateReal;
 import static com.yahoo.vespa.hosted.controller.deployment.Step.deactivateTester;
@@ -189,12 +191,20 @@ public class InternalStepRunner implements StepRunner {
     }
 
     private Optional<RunStatus> deployReal(RunId id, boolean setTheStage, DualLogger logger) {
+        Optional<X509Certificate> testerCertificate = controller.jobController().run(id).get().testerCertificate();
         return deploy(() -> controller.applications().deploy(id.job(), setTheStage),
                       controller.jobController().run(id).get()
                                 .stepInfo(setTheStage ? deployInitialReal : deployReal).get()
                                 .startTime().get(),
-                      logger,
-                      false);
+                      logger)
+                .filter(result -> {
+                    // If no tester cert, or deployment failed, propagate original result.
+                    if (testerCertificate.isEmpty() || result != running)
+                        return true;
+                    // If tester cert, ensure real is deployed with the tester cert whose key was successfully deployed.
+                    return    controller.jobController().run(id).get().stepStatus(deployTester).get() == succeeded
+                           && testerCertificate.equals(controller.jobController().run(id).get().testerCertificate());
+                });
     }
 
     private Optional<RunStatus> deployTester(RunId id, DualLogger logger) {
@@ -207,12 +217,10 @@ public class InternalStepRunner implements StepRunner {
                       controller.jobController().run(id).get()
                                 .stepInfo(deployTester).get()
                                 .startTime().get(),
-                      logger,
-                      true);
+                      logger);
     }
 
-    private Optional<RunStatus> deploy(Supplier<ActivateResult> deployment, Instant startTime, DualLogger logger,
-                                       boolean failOnAnyError) {
+    private Optional<RunStatus> deploy(Supplier<ActivateResult> deployment, Instant startTime, DualLogger logger) {
         try {
             PrepareResponse prepareResponse = deployment.get().prepareResponse();
             if (prepareResponse.log != null)
@@ -231,7 +239,7 @@ public class InternalStepRunner implements StepRunner {
         }
         catch (ConfigServerException e) {
             // Retry certain failures for up to one hour.
-            Optional<RunStatus> result = failOnAnyError || startTime.isBefore(controller.clock().instant().minus(Duration.ofHours(1)))
+            Optional<RunStatus> result = startTime.isBefore(controller.clock().instant().minus(Duration.ofHours(1)))
                                          ? Optional.of(deploymentFailed) : Optional.empty();
             switch (e.code()) {
                 case CERTIFICATE_NOT_READY:
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java
index 2819d9970b7..d8bddac4187 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java
@@ -33,7 +33,6 @@ import org.junit.Test;
 
 import java.io.IOException;
 import java.io.UncheckedIOException;
-import java.net.URI;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
@@ -54,11 +53,11 @@ import static com.yahoo.vespa.hosted.controller.deployment.DeploymentContext.app
 import static com.yahoo.vespa.hosted.controller.deployment.DeploymentTester.instanceId;
 import static com.yahoo.vespa.hosted.controller.deployment.RunStatus.deploymentFailed;
 import static com.yahoo.vespa.hosted.controller.deployment.RunStatus.installationFailed;
-import static com.yahoo.vespa.hosted.controller.deployment.RunStatus.running;
 import static com.yahoo.vespa.hosted.controller.deployment.Step.Status.failed;
 import static com.yahoo.vespa.hosted.controller.deployment.Step.Status.succeeded;
 import static com.yahoo.vespa.hosted.controller.deployment.Step.Status.unfinished;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertTrue;
 
 /**
@@ -422,6 +421,43 @@ public class InternalStepRunnerTest {
     }
 
     @Test
+    public void realDeploymentRequiresForTesterCert() {
+        tester.controllerTester().zoneRegistry().setSystemName(SystemName.Public);
+        var zones = List.of(ZoneApiMock.fromId("test.aws-us-east-1c"),
+                            ZoneApiMock.fromId("staging.aws-us-east-1c"),
+                            ZoneApiMock.fromId("prod.aws-us-east-1c"));
+        tester.controllerTester().zoneRegistry()
+              .setZones(zones)
+              .setRoutingMethod(zones, RoutingMethod.exclusive);
+        tester.configServer().bootstrap(tester.controllerTester().zoneRegistry().zones().all().ids(), SystemApplication.values());
+        ZoneId testZone = JobType.systemTest.zone(tester.controller().system());
+        RunId id = app.newRun(JobType.systemTest);
+        tester.configServer().throwOnPrepare(instanceId -> {
+            if (instanceId.instance().isTester())
+                throw new ConfigServerException(ConfigServerException.ErrorCode.PARENT_HOST_NOT_READY, "provisioning", "deploy tester");
+        });
+        tester.runner().run();
+        assertEquals(unfinished, tester.jobs().run(id).get().stepStatuses().get(Step.deployTester));
+        assertEquals(unfinished, tester.jobs().run(id).get().stepStatuses().get(Step.deployReal));
+
+        List<X509Certificate> oldTrusted = new ArrayList<>(DeploymentContext.publicApplicationPackage().trustedCertificates());
+        X509Certificate oldCert = tester.jobs().run(id).get().testerCertificate().get();
+        oldTrusted.add(oldCert);
+        assertEquals(oldTrusted, tester.configServer().application(app.instanceId(), id.type().zone(system())).get().applicationPackage().trustedCertificates());
+
+        tester.configServer().throwOnNextPrepare(null);
+        tester.runner().run();
+        assertEquals(succeeded, tester.jobs().run(id).get().stepStatuses().get(Step.deployTester));
+        assertEquals(succeeded, tester.jobs().run(id).get().stepStatuses().get(Step.deployReal));
+
+        List<X509Certificate> newTrusted = new ArrayList<>(DeploymentContext.publicApplicationPackage().trustedCertificates());
+        X509Certificate newCert = tester.jobs().run(id).get().testerCertificate().get();
+        newTrusted.add(newCert);
+        assertEquals(newTrusted, tester.configServer().application(app.instanceId(), id.type().zone(system())).get().applicationPackage().trustedCertificates());
+        assertNotEquals(oldCert, newCert);
+    }
+
+    @Test
     public void certificateTimeoutAbortsJob() {
         tester.controllerTester().zoneRegistry().setSystemName(SystemName.Public);
         var zones = List.of(ZoneApiMock.fromId("test.aws-us-east-1c"),
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java
index fbe591a6433..407ab2432a5 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java
@@ -63,6 +63,10 @@ import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
 import java.util.UUID;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.BooleanSupplier;
+import java.util.function.Consumer;
+import java.util.function.Function;
 import java.util.function.Supplier;
 import java.util.logging.Level;
 import java.util.stream.Collectors;
@@ -94,7 +98,7 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
     private List<ProtonMetrics> protonMetrics;
 
     private Version lastPrepareVersion = null;
-    private RuntimeException prepareException = null;
+    private Consumer<ApplicationId> prepareException = null;
     private ConfigChangeActions configChangeActions = null;
     private Supplier<String> log = () -> "INFO - All good";
 
@@ -206,9 +210,14 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
         return Optional.ofNullable(lastPrepareVersion);
     }
 
+    /** Sets a function that may throw, determined by app id. */
+    public void throwOnPrepare(Consumer<ApplicationId> prepareThrower) {
+        this.prepareException = prepareThrower;
+    }
+
     /** The exception to throw on the next prepare run, or null to continue normally */
     public void throwOnNextPrepare(RuntimeException prepareException) {
-        this.prepareException = prepareException;
+        this.prepareException = prepareException == null ? null : id -> { this.prepareException = null; throw prepareException; };
     }
 
     /** Set version for an application in a given zone */
@@ -367,11 +376,10 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
     @Override
     public PreparedApplication deploy(DeploymentData deployment) {
         lastPrepareVersion = deployment.platform();
-        if (prepareException != null) {
-            RuntimeException prepareException = this.prepareException;
-            this.prepareException = null;
-            throw prepareException;
-        }
+        if (prepareException != null)
+            prepareException.accept(ApplicationId.from(deployment.instance().tenant(),
+                                                       deployment.instance().application(),
+                                                       deployment.instance().instance()));
         DeploymentId id = new DeploymentId(deployment.instance(), deployment.zone());
 
         applications.put(id, new Application(id.applicationId(), lastPrepareVersion, new ApplicationPackage(deployment.applicationPackage())));