Run DNS challenges asynchronously, and check from job runner

7 files changed, 111 insertions, 46 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 14f2b38f24a..9721396cd54 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
@@ -358,8 +358,12 @@ public class InternalStepRunner implements StepRunner {
             controller.jobController().locked(id, lockedRun -> lockedRun.withSummary(null));
             Availability availability = endpointsAvailable(id.application(), id.type().zone(), logger);
             if (availability.status() == Status.available) {
+                if (controller.routing().policies().processDnsChallenges(new DeploymentId(id.application(), id.type().zone()))) {
                     logger.log("Installation succeeded!");
                     return Optional.of(running);
+                }
+                logger.log("Waiting for DNS challenges for private endpoints to be processed");
+                return Optional.empty();
             }
             logger.log(availability.message());
             if (availability.status() == Status.endpointsUnavailable && timedOut(id, deployment.get(), timeouts.endpoint())) {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java
index be58370285a..f4980073d6c 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java
@@ -6,6 +6,7 @@ import com.yahoo.component.Version;
 import com.yahoo.component.annotation.Inject;
 import com.yahoo.concurrent.UncheckedTimeoutException;
 import com.yahoo.config.provision.ApplicationId;
+import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.HostName;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.config.provision.zone.ZoneId;
@@ -15,12 +16,14 @@ import com.yahoo.slime.SlimeUtils;
 import com.yahoo.transaction.Mutex;
 import com.yahoo.vespa.curator.Curator;
 import com.yahoo.vespa.hosted.controller.Application;
+import com.yahoo.vespa.hosted.controller.api.identifiers.ClusterId;
 import com.yahoo.vespa.hosted.controller.api.identifiers.ControllerVersion;
 import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId;
 import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveBucket;
 import com.yahoo.vespa.hosted.controller.api.integration.certificates.EndpointCertificateMetadata;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.RunId;
+import com.yahoo.vespa.hosted.controller.api.integration.dns.VpcEndpointService.DnsChallenge;
 import com.yahoo.vespa.hosted.controller.api.integration.vcmr.VespaChangeRequest;
 import com.yahoo.vespa.hosted.controller.application.TenantAndApplicationId;
 import com.yahoo.vespa.hosted.controller.auditlog.AuditLog;
@@ -90,6 +93,7 @@ public class CuratorDb {
     private static final Path jobRoot = root.append("jobs");
     private static final Path controllerRoot = root.append("controllers");
     private static final Path routingPoliciesRoot = root.append("routingPolicies");
+    private static final Path dnsChallengesRoot = root.append("dnsChallenges");
     private static final Path zoneRoutingPoliciesRoot = root.append("zoneRoutingPolicies");
     private static final Path endpointCertificateRoot = root.append("applicationCertificates");
     private static final Path archiveBucketsRoot = root.append("archiveBuckets");
@@ -114,6 +118,7 @@ public class CuratorDb {
     private final RunSerializer runSerializer = new RunSerializer();
     private final RetriggerEntrySerializer retriggerEntrySerializer = new RetriggerEntrySerializer();
     private final NotificationsSerializer notificationsSerializer = new NotificationsSerializer();
+    private final DnsChallengeSerializer dnsChallengeSerializer = new DnsChallengeSerializer();
 
     private final Curator curator;
     private final Duration tryLockTimeout;
@@ -559,6 +564,35 @@ public class CuratorDb {
                                                      .orElseGet(() -> new ZoneRoutingPolicy(zone, RoutingStatus.DEFAULT));
     }
 
+    public void writeDnsChallenge(DnsChallenge challenge) {
+        curator.set(dnsChallengePath(challenge.clusterId()), dnsChallengeSerializer.toJson(challenge));
+    }
+
+    public void deleteDnsChallenge(ClusterId id) {
+        curator.delete(dnsChallengePath(id));
+    }
+
+    public List<DnsChallenge> readDnsChallenges(DeploymentId id) {
+        return curator.getChildren(dnsChallengePath(id)).stream()
+                      .map(cluster -> readDnsChallenge(new ClusterId(id, ClusterSpec.Id.from(cluster))))
+                      .toList();
+    }
+
+    private DnsChallenge readDnsChallenge(ClusterId clusterId) {
+        return curator.getData(dnsChallengePath(clusterId))
+                      .map(bytes -> dnsChallengeSerializer.fromJson(bytes, clusterId))
+                      .orElseThrow(() -> new IllegalArgumentException("no DNS challenge for " + clusterId));
+    }
+
+    private static Path dnsChallengePath(DeploymentId id) {
+        return dnsChallengesRoot.append(id.applicationId().serializedForm())
+                                .append(id.zoneId().value());
+    }
+
+    private static Path dnsChallengePath(ClusterId id) {
+        return dnsChallengePath(id.deploymentId()).append(id.clusterId().value());
+    }
+
     // -------------- Application endpoint certificates ----------------------------
 
     public void writeEndpointCertificateMetadata(ApplicationId applicationId, EndpointCertificateMetadata endpointCertificateMetadata) {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java
index 1c4916b9bed..61c71e964f6 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java
@@ -5,6 +5,7 @@ import ai.vespa.http.DomainName;
 import com.yahoo.concurrent.UncheckedTimeoutException;
 import com.yahoo.config.application.api.DeploymentSpec;
 import com.yahoo.config.provision.ApplicationId;
+import com.yahoo.config.provision.CloudAccount;
 import com.yahoo.config.provision.zone.RoutingMethod;
 import com.yahoo.config.provision.zone.ZoneId;
 import com.yahoo.transaction.Mutex;
@@ -20,6 +21,8 @@ import com.yahoo.vespa.hosted.controller.api.integration.dns.Record;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.Record.Type;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordData;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordName;
+import com.yahoo.vespa.hosted.controller.api.integration.dns.VpcEndpointService.DnsChallenge;
+import com.yahoo.vespa.hosted.controller.api.integration.dns.VpcEndpointService.State;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.WeightedAliasTarget;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.WeightedDirectTarget;
 import com.yahoo.vespa.hosted.controller.application.Endpoint;
@@ -27,12 +30,14 @@ import com.yahoo.vespa.hosted.controller.application.EndpointId;
 import com.yahoo.vespa.hosted.controller.application.EndpointList;
 import com.yahoo.vespa.hosted.controller.application.TenantAndApplicationId;
 import com.yahoo.vespa.hosted.controller.dns.NameServiceForwarder;
+import com.yahoo.vespa.hosted.controller.dns.NameServiceQueue;
 import com.yahoo.vespa.hosted.controller.dns.NameServiceQueue.Priority;
 import com.yahoo.vespa.hosted.controller.dns.NameServiceRequest;
 import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
 import com.yahoo.yolean.UncheckedInterruptedException;
 
 import java.time.Instant;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
@@ -394,28 +399,51 @@ public class RoutingPolicies {
                                  new ClusterId(deploymentId, endpoint.cluster()),
                                  loadBalancer.cloudAccount())
                   .ifPresent(challenge -> {
-                      try {
+                      try (Mutex lock = db.lockNameServiceQueue()) {
                           nameServiceForwarderIn(deploymentId.zoneId()).createTxt(challenge.name(), List.of(challenge.data()), Priority.high, ownerOf(deploymentId));
-                          Instant doom = controller.clock().instant().plusSeconds(30);
-                          while (controller.clock().instant().isBefore(doom)) {
-                              try (Mutex lock = controller.curator().lockNameServiceQueue()) {
-                                  if (controller.curator().readNameServiceQueue().requests().stream()
-                                                .noneMatch(request -> request.name().equals(challenge.name()))) {
-                                      try { challenge.trigger().run(); }
-                                      finally { nameServiceForwarderIn(deploymentId.zoneId()).removeRecords(Type.TXT, challenge.name(), Priority.normal, ownerOf(deploymentId)); }
-                                      return;
-                                  }
-                              }
-                              Thread.sleep(100);
-                          }
-                          throw new UncheckedTimeoutException("timed out waiting for DNS challenge to be processed");
-                      }
-                      catch (InterruptedException e) {
-                          throw new UncheckedInterruptedException("interrupted waiting for DNS challenge to be processed", e, true);
+                          db.writeDnsChallenge(challenge);
                       }
                   });
     }
 
+    /** Returns true iff. the given deployment has no incomplete DNS challenges, or throws (and cleans up) on errors. */
+    public boolean processDnsChallenges(DeploymentId deploymentId) {
+        try (Mutex lock = db.lockNameServiceQueue()) {
+            List<DnsChallenge> challenges = new ArrayList<>(db.readDnsChallenges(deploymentId));
+            Set<RecordName> pendingRequests = controller.curator().readNameServiceQueue().requests().stream()
+                                                        .map(NameServiceRequest::name)
+                                                        .collect(Collectors.toSet());
+            try {
+                challenges.removeIf(challenge -> {
+                    if (challenge.state() == State.pending) {
+                        if (pendingRequests.contains(challenge.name())) return false;
+                        challenge = challenge.withState(State.ready);
+                    }
+                    State state = controller.serviceRegistry().vpcEndpointService().process(challenge);
+                    if (state == State.done) {
+                        removeDnsChallenge(challenge);
+                        return true;
+                    }
+                    else {
+                        db.writeDnsChallenge(challenge.withState(state));
+                        return false;
+                    }
+                });
+                return challenges.isEmpty();
+            }
+            catch (RuntimeException e) {
+                challenges.forEach(this::removeDnsChallenge);
+                throw e;
+            }
+        }
+    }
+
+    private void removeDnsChallenge(DnsChallenge challenge) {
+        nameServiceForwarderIn(challenge.clusterId().deploymentId().zoneId())
+                .removeRecords(Type.TXT, challenge.name(), Priority.normal, ownerOf(challenge.clusterId().deploymentId()));
+        db.deleteDnsChallenge(challenge.clusterId());
+    }
+
     /**
      * Remove policies and zone DNS records unreferenced by given load balancers
      *
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java
index 95c22480c0f..14835a822e6 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java
@@ -251,7 +251,7 @@ public class DeploymentContext {
         flushDnsUpdates(Integer.MAX_VALUE);
         assertEquals(List.of(),
                      tester.controller().curator().readNameServiceQueue().requests(),
-                   "All name service requests dispatched");
+                     "All name service requests dispatched");
         return this;
     }
 
@@ -590,6 +590,9 @@ public class DeploymentContext {
             tester.cloud().set(Status.SUCCESS);
             runner.advance(currentRun(job));
             assertEquals(succeeded, jobs.run(id).stepStatuses().get(Step.endStagingSetup));
+
+            if ( ! deferDnsUpdates)
+                flushDnsUpdates();
         }
     }
 
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 6e6ecef4e66..b8954ff6e73 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
@@ -22,6 +22,7 @@ import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.ZoneEndpoint.AllowedUrn;
 import com.yahoo.config.provision.ZoneEndpoint.AccessType;
 import com.yahoo.config.provision.zone.ZoneId;
+import com.yahoo.rdl.UUID;
 import com.yahoo.vespa.flags.json.FlagData;
 import com.yahoo.vespa.hosted.controller.api.application.v4.model.ClusterMetrics;
 import com.yahoo.vespa.hosted.controller.api.application.v4.model.DeploymentData;
@@ -69,8 +70,8 @@ import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Random;
 import java.util.Set;
-import java.util.UUID;
 import java.util.function.Consumer;
 import java.util.function.Supplier;
 import java.util.logging.Level;
@@ -385,8 +386,7 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
 
     /** Add any of given loadBalancers that do not already exist to the load balancers in zone */
     public void putLoadBalancers(ZoneId zone, List<LoadBalancer> loadBalancers) {
-        this.loadBalancers.putIfAbsent(zone, new LinkedHashSet<>());
-        this.loadBalancers.get(zone).addAll(loadBalancers);
+        this.loadBalancers.computeIfAbsent(zone, __ -> new LinkedHashSet<>()).addAll(loadBalancers);
     }
 
     public void removeLoadBalancers(ApplicationId application, ZoneId zone) {
@@ -420,7 +420,7 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
         deployment.cloudAccount().ifPresent(account -> this.cloudAccounts.put(id, account));
 
         if (!deferLoadBalancerProvisioning.contains(id.zoneId().environment())) {
-            putLoadBalancers(id.zoneId(), List.of(new LoadBalancer(UUID.randomUUID().toString(),
+            putLoadBalancers(id.zoneId(), List.of(new LoadBalancer(id.dottedString() + "." + cluster,
                                                                    id.applicationId(),
                                                                    cluster,
                                                                    Optional.of(HostName.of("lb-0--" + id.applicationId().toFullString() + "--" + id.zoneId().toString())),
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java
index 0ba8866c990..be257daa211 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java
@@ -67,7 +67,7 @@ public class ServiceRegistryMock extends AbstractComponent implements ServiceReg
     private final ZoneRegistryMock zoneRegistryMock;
     private final ConfigServerMock configServerMock;
     private final MemoryNameService memoryNameService = new MemoryNameService();
-    private final MockVpcEndpointService vpcEndpointService = new MockVpcEndpointService();
+    private final MockVpcEndpointService vpcEndpointService = new MockVpcEndpointService(clock, memoryNameService);
     private final MockMailer mockMailer = new MockMailer();
     private final EndpointCertificateMock endpointCertificateMock = new EndpointCertificateMock(clock);
     private final EndpointCertificateValidatorMock endpointCertificateValidatorMock = new EndpointCertificateValidatorMock();
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java
index 2932860efaa..94cffb94184 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java
@@ -26,6 +26,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.dns.Record.Type;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordData;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordName;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.VpcEndpointService.DnsChallenge;
+import com.yahoo.vespa.hosted.controller.api.integration.dns.VpcEndpointService.State;
 import com.yahoo.vespa.hosted.controller.application.Endpoint;
 import com.yahoo.vespa.hosted.controller.application.EndpointId;
 import com.yahoo.vespa.hosted.controller.application.EndpointList;
@@ -57,6 +58,7 @@ import java.util.stream.Collectors;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 /**
@@ -515,39 +517,33 @@ public class RoutingPoliciesTest {
     void private_dns_for_vpc_endpoint() {
         // Challenge answered for endpoint
         RoutingPoliciesTester tester = new RoutingPoliciesTester();
-        Map<RecordName, RecordData> challenges = new ConcurrentHashMap<>();
-        tester.tester.controllerTester().serviceRegistry().vpcEndpointService().delegate = (name, cluster, account) -> {
-                RecordName recordName = RecordName.from("challenge--" + name.value());
-                if (challenges.containsKey(recordName)) return Optional.empty();
-                RecordData recordData = RecordData.from(account.map(CloudAccount::value).orElse("system"));
-                return Optional.of(new DnsChallenge(recordName, recordData, () -> challenges.put(recordName, recordData)));
-            };
+        tester.tester.controllerTester().serviceRegistry().vpcEndpointService().enabled.set(true);
 
         DeploymentContext app = tester.newDeploymentContext("t", "a", "default");
         ApplicationPackage appPackage = applicationPackageBuilder().region(zone3.region()).build();
         app.submit(appPackage);
 
-        AtomicBoolean done = new AtomicBoolean();
-        new Thread(() -> {
-            while ( ! done.get()) {
-                app.flushDnsUpdates(Integer.MAX_VALUE);
-                try { Thread.sleep(10); } catch (InterruptedException e) { break; }
-            }
-        }).start();
         app.deploy();
-        done.set(true);
-
-        assertEquals(Map.of( RecordName.from("challenge--a.t.aws-us-east-1a.vespa.oath.cloud"),
-                             RecordData.from("system"),
-                             RecordName.from("challenge--a.t.us-east-1.test.vespa.oath.cloud"),
-                             RecordData.from("system"),
-                             RecordName.from("challenge--a.t.us-east-3.staging.vespa.oath.cloud"),
-                             RecordData.from("system")),
-                     challenges);
+
+        // TXT records are cleaned up as we go—the last challenge is the last to go here, and we must flush it ourselves.
+        assertEquals(Set.of("a.t.aws-us-east-1a.vespa.oath.cloud",
+                            "challenge--a.t.aws-us-east-1a.vespa.oath.cloud"),
+                     tester.recordNames());
+        app.flushDnsUpdates();
         assertEquals(Set.of(new Record(Type.CNAME,
                                        RecordName.from("a.t.aws-us-east-1a.vespa.oath.cloud"),
                                        RecordData.from("lb-0--t.a.default--prod.aws-us-east-1a."))),
                      tester.controllerTester().nameService().records());
+
+
+        tester.tester.controllerTester().serviceRegistry().vpcEndpointService().outcomes
+                .put(RecordName.from("challenge--a.t.aws-us-east-1a.vespa.oath.cloud"), State.running);
+
+        // Deployment fails because challenge is not answered (immediately).
+        assertEquals("Status of run 2 of production-aws-us-east-1a for t.a ==> expected: <succeeded> but was: <unfinished>",
+                     assertThrows(AssertionError.class,
+                                  () -> app.submit(appPackage).deploy())
+                             .getMessage());
     }
 
     @Test