Delay deletion of DNS challenge TXT records until deactivation

4 files changed, 28 insertions, 14 deletions

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 a25aa9797ba..dc9c4650191 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
@@ -7,6 +7,7 @@ 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.ClusterSpec.Id;
 import com.yahoo.config.provision.HostName;
 import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.TenantName;
@@ -602,7 +603,7 @@ public class CuratorDb {
 
     public List<DnsChallenge> readDnsChallenges(DeploymentId id) {
         return curator.getChildren(dnsChallengePath(id)).stream()
-                      .map(cluster -> readDnsChallenge(new ClusterId(id, ClusterSpec.Id.from(cluster))))
+                      .map(cluster -> readDnsChallenge(new ClusterId(id, Id.from(cluster))))
                       .toList();
     }
 
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 de25161c461..5919889cdbd 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
@@ -416,7 +416,7 @@ public class RoutingPolicies {
 
     private void setPrivateDns(Endpoint endpoint, LoadBalancer loadBalancer, DeploymentId deploymentId) {
         if (loadBalancer.service().isEmpty()) return;
-        // TODO(mpolden): Why is this done? Consider creating private DNS for all auth methods
+        // TODO(mpolden): Model one service for each endpoint (type), to allow private endpoints with tokens.
         boolean skipBasedOnAuthMethod = switch (endpoint.authMethod()) {
             case token -> true;
             case mtls -> false;
@@ -436,10 +436,21 @@ public class RoutingPolicies {
                   });
     }
 
+    /** Deletes all DNS challenges, and corresponding TXT records, for the given deployment. */
+    public void removeDnsChallenges(DeploymentId deploymentId) {
+        try (Mutex lock = db.lockNameServiceQueue()) {
+            for (DnsChallenge challenge : db.readDnsChallenges(deploymentId)) {
+                controller.nameServiceForwarder().removeRecords(Record.Type.TXT, challenge.name(), Priority.normal, ownerOf(deploymentId));
+                db.deleteDnsChallenge(challenge.clusterId());
+            }
+        }
+    }
+
     /** 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));
+            challenges.removeIf(challenge -> challenge.state() == ChallengeState.done);
             Set<RecordName> pendingRequests = controller.curator().readNameServiceQueue().requests().stream()
                                                         .map(NameServiceRequest::name)
                                                         .collect(Collectors.toSet());
@@ -450,14 +461,8 @@ public class RoutingPolicies {
                         challenge = challenge.withState(ChallengeState.ready);
                     }
                     ChallengeState state = controller.serviceRegistry().vpcEndpointService().process(challenge);
-                    if (state == ChallengeState.done) {
-                        removeDnsChallenge(challenge);
-                        return true;
-                    }
-                    else {
-                        db.writeDnsChallenge(challenge.withState(state));
-                        return false;
-                    }
+                    db.writeDnsChallenge(challenge.withState(state));
+                    return state == ChallengeState.done;
                 });
                 return challenges.isEmpty();
             }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/context/DeploymentRoutingContext.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/context/DeploymentRoutingContext.java
index df0226176a2..99f60735f6e 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/context/DeploymentRoutingContext.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/context/DeploymentRoutingContext.java
@@ -57,6 +57,7 @@ public abstract class DeploymentRoutingContext implements RoutingContext {
     /** Deactivate routing configuration for the deployment in this context, using given deployment spec */
     public final void deactivate(DeploymentSpec deploymentSpec) {
         routing.policies().refresh(deployment, deploymentSpec, EndpointList.EMPTY);
+        routing.policies().removeDnsChallenges(deployment);
     }
 
     /** Routing method of this context */
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 630de5137bb..1b2fa956763 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
@@ -598,21 +598,28 @@ public class RoutingPoliciesTest {
 
         app.deploy();
 
-        // TXT records are cleaned up as we go—the last challenge is the last to go here, and we must flush it ourselves.
+        // TXT records are cleaned up when deployments are deactivated.
+        // The last challenge is the last to go here, and we must flush it ourselves.
         assertEquals(List.of("a.t.aws-us-east-33a.vespa.oath.cloud",
                              "challenge--a.t.aws-us-east-33a.vespa.oath.cloud"),
                      tester.recordNames());
         app.flushDnsUpdates();
         assertEquals(Set.of(new Record(Type.CNAME,
                                        RecordName.from("a.t.aws-us-east-33a.vespa.oath.cloud"),
-                                       RecordData.from("lb-0--t.a.default--prod.aws-us-east-33a."))),
+                                       RecordData.from("lb-0--t.a.default--prod.aws-us-east-33a.")),
+                            new Record(Type.TXT,
+                                       RecordName.from("challenge--a.t.aws-us-east-33a.vespa.oath.cloud"),
+                                       RecordData.from("system"))),
                      tester.controllerTester().nameService().records());
 
+        tester.controllerTester().controller().applications().deactivate(app.instanceId(), zone3);
+        app.flushDnsUpdates();
+        assertEquals(Set.of(),
+                     tester.controllerTester().nameService().records());
 
+        // Deployment fails because challenge is not answered (immediately).
         tester.tester.controllerTester().serviceRegistry().vpcEndpointService().outcomes
                 .put(RecordName.from("challenge--a.t.aws-us-east-33a.vespa.oath.cloud"), ChallengeState.running);
-
-        // Deployment fails because challenge is not answered (immediately).
         assertEquals("Status of run 2 of production-aws-us-east-33a for t.a ==> expected: <succeeded> but was: <unfinished>",
                      assertThrows(AssertionError.class,
                                   () -> app.submit(appPackage).deploy())