Merge pull request #13835 from vespa-engine/mpolden/legacy-syntax

Support legacy global-service-id syntax combined with feature flag

3 files changed, 76 insertions, 59 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/NameServiceForwarder.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/NameServiceForwarder.java
index b8692233b2d..4f36e713ba4 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/NameServiceForwarder.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/NameServiceForwarder.java
@@ -69,7 +69,7 @@ public class NameServiceForwarder {
         forward(new RemoveRecords(type, data), priority);
     }
 
-    private void forward(NameServiceRequest request, NameServiceQueue.Priority priority) {
+    protected void forward(NameServiceRequest request, NameServiceQueue.Priority priority) {
         try (Lock lock = db.lockNameServiceQueue()) {
             NameServiceQueue queue = db.readNameServiceQueue();
             var queued = queue.requests().size();
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 e5a99c2e69d..d6f0ff4d85f 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
@@ -24,6 +24,7 @@ import com.yahoo.vespa.hosted.controller.application.EndpointId;
 import com.yahoo.vespa.hosted.controller.application.SystemApplication;
 import com.yahoo.vespa.hosted.controller.dns.NameServiceForwarder;
 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 java.util.ArrayList;
@@ -166,7 +167,7 @@ public class RoutingPolicies {
 
     // TODO(mpolden): Remove and inline call to updateGlobalDnsOf when feature flag disappears
     private void updateGlobalDnsOf(ApplicationId application, Collection<RoutingPolicy> routingPolicies, Set<ZoneId> inactiveZones, @SuppressWarnings("unused") Lock lock) {
-        if (weightedDnsPerRegion.with(FetchVector.Dimension.APPLICATION_ID, application.serializedForm()).value()) {
+        if (useWeightedDnsPerRegion(application)) {
             updateGlobalDnsOf(routingPolicies, inactiveZones, lock);
         } else {
             legacyUpdateGlobalDnsOf(routingPolicies, inactiveZones, lock);
@@ -245,7 +246,7 @@ public class RoutingPolicies {
             var policyId = new RoutingPolicyId(loadBalancer.application(), loadBalancer.cluster(), allocation.deployment.zoneId());
             var existingPolicy = policies.get(policyId);
             var newPolicy = new RoutingPolicy(policyId, loadBalancer.hostname(), loadBalancer.dnsZone(),
-                                              allocation.endpointIdsOf(loadBalancer),
+                                              allocation.endpointIdsOf(loadBalancer, useWeightedDnsPerRegion(loadBalancer.application())),
                                               new Status(isActive(loadBalancer), GlobalRouting.DEFAULT_STATUS));
             // Preserve global routing status for existing policy
             if (existingPolicy != null) {
@@ -262,15 +263,15 @@ public class RoutingPolicies {
         var name = RecordName.from(policy.endpointIn(controller.system(), RoutingMethod.exclusive, controller.zoneRegistry())
                                          .dnsName());
         var data = RecordData.fqdn(policy.canonicalName().value());
-        NameUpdater nameUpdater = nameUpdaterIn(policy.id().zone());
+        NameServiceForwarder forwarder = nameServiceForwarderIn(policy.id().zone());
         if (policy.id().owner().equals(SystemApplication.configServer.id())) {
             // TODO(mpolden): Remove this after transition is complete. Before automatic provisioning of config server
             //                load balancers, the DNS records for the config server LB were of type A. It's not possible
             //                to change the type of an existing record, we therefore remove the A record before creating
             //                a CNAME.
-            nameUpdater.removeRecords(Record.Type.A, name);
+            forwarder.removeRecords(Record.Type.A, name, Priority.normal);
         }
-        nameUpdater.createCname(name, data);
+        forwarder.createCname(name, data, Priority.normal);
     }
 
     /** Remove policies and zone DNS records unreferenced by given load balancers */
@@ -284,7 +285,9 @@ public class RoutingPolicies {
                 !policy.id().zone().equals(allocation.deployment.zoneId())) continue;
 
             var dnsName = policy.endpointIn(controller.system(), RoutingMethod.exclusive, controller.zoneRegistry()).dnsName();
-            nameUpdaterIn(allocation.deployment.zoneId()).removeRecords(Record.Type.CNAME, RecordName.from(dnsName));
+            nameServiceForwarderIn(allocation.deployment.zoneId()).removeRecords(Record.Type.CNAME,
+                                                                                 RecordName.from(dnsName),
+                                                                                 Priority.normal);
             newPolicies.remove(policy.id());
         }
         db.writeRoutingPolicies(allocation.deployment.applicationId(), newPolicies);
@@ -300,16 +303,17 @@ public class RoutingPolicies {
             var endpoints = controller.routing().endpointsOf(id.application())
                                       .not().requiresRotation()
                                       .named(id.endpointId());
-            var nameUpdater = nameUpdaterIn(allocation.deployment.zoneId());
-            endpoints.forEach(endpoint -> nameUpdater.removeRecords(Record.Type.ALIAS, RecordName.from(endpoint.dnsName())));
+            var forwarder = nameServiceForwarderIn(allocation.deployment.zoneId());
+            endpoints.forEach(endpoint -> forwarder.removeRecords(Record.Type.ALIAS, RecordName.from(endpoint.dnsName()),
+                                                                  Priority.normal));
         }
     }
 
     /** Compute routing IDs from given load balancers */
-    private static Set<RoutingId> routingIdsFrom(LoadBalancerAllocation allocation) {
+    private Set<RoutingId> routingIdsFrom(LoadBalancerAllocation allocation) {
         Set<RoutingId> routingIds = new LinkedHashSet<>();
         for (var loadBalancer : allocation.loadBalancers) {
-            for (var endpointId : allocation.endpointIdsOf(loadBalancer)) {
+            for (var endpointId : allocation.endpointIdsOf(loadBalancer, useWeightedDnsPerRegion(loadBalancer.application()))) {
                 routingIds.add(new RoutingId(loadBalancer.application(), endpointId));
             }
         }
@@ -349,6 +353,10 @@ public class RoutingPolicies {
         return false;
     }
 
+    private boolean useWeightedDnsPerRegion(ApplicationId application) {
+        return weightedDnsPerRegion.with(FetchVector.Dimension.APPLICATION_ID, application.serializedForm()).value();
+    }
+
     /** Represents records for a region-wide endpoint */
     private static class RegionEndpoint {
 
@@ -410,7 +418,7 @@ public class RoutingPolicies {
         }
 
         /** Compute all endpoint IDs for given load balancer */
-        private Set<EndpointId> endpointIdsOf(LoadBalancer loadBalancer) {
+        private Set<EndpointId> endpointIdsOf(LoadBalancer loadBalancer, boolean useWeightedDnsPerRegion) {
             if (!deployment.zoneId().environment().isProduction()) { // Only production deployments have configurable endpoints
                 return Set.of();
             }
@@ -418,12 +426,16 @@ public class RoutingPolicies {
             if (instanceSpec.isEmpty()) {
                 return Set.of();
             }
+            if (useWeightedDnsPerRegion && instanceSpec.get().globalServiceId().filter(id -> id.equals(loadBalancer.cluster().value())).isPresent()) {
+                // Legacy assignment always has the default endpoint Id
+                return Set.of(EndpointId.defaultId());
+            }
             return instanceSpec.get().endpoints().stream()
                                .filter(endpoint -> endpoint.containerId().equals(loadBalancer.cluster().value()))
                                .filter(endpoint -> endpoint.regions().contains(deployment.zoneId().region()))
                                .map(com.yahoo.config.application.api.Endpoint::endpointId)
                                .map(EndpointId::of)
-                               .collect(Collectors.toSet());
+                               .collect(Collectors.toUnmodifiableSet());
         }
 
     }
@@ -440,52 +452,24 @@ public class RoutingPolicies {
     }
 
     /** Returns the name updater to use for given zone */
-    private NameUpdater nameUpdaterIn(ZoneId zone) {
+    private NameServiceForwarder nameServiceForwarderIn(ZoneId zone) {
         if (controller.zoneRegistry().routingMethods(zone).contains(RoutingMethod.exclusive)) {
-            return new NameUpdater(controller.nameServiceForwarder());
+            return controller.nameServiceForwarder();
         }
-        return new DiscardingNameUpdater();
+        return new NameServiceDiscarder(controller.curator());
     }
 
-    /** A name updater that passes name service operations to the next handler */
-    private static class NameUpdater {
-
-        private final NameServiceForwarder forwarder;
-
-        public NameUpdater(NameServiceForwarder forwarder) {
-            this.forwarder = forwarder;
-        }
+    /** A {@link NameServiceForwarder} that does nothing. Used in zones where no explicit DNS updates are needed */
+    private static class NameServiceDiscarder extends NameServiceForwarder {
 
-        public void removeRecords(Record.Type type, RecordName name) {
-            forwarder.removeRecords(type, name, Priority.normal);
+        public NameServiceDiscarder(CuratorDb db) {
+            super(db);
         }
 
-        public void createAlias(RecordName name, Set<AliasTarget> targets) {
-            forwarder.createAlias(name, targets, Priority.normal);
-        }
-
-        public void createCname(RecordName name, RecordData data) {
-            forwarder.createCname(name, data, Priority.normal);
-        }
-
-    }
-
-    /** A name updater that does nothing */
-    private static class DiscardingNameUpdater extends NameUpdater {
-
-        private DiscardingNameUpdater() {
-            super(null);
-        }
-
-        @Override
-        public void removeRecords(Record.Type type, RecordName name) {}
-
-        @Override
-        public void createAlias(RecordName name, Set<AliasTarget> targets) {}
-
         @Override
-        public void createCname(RecordName name, RecordData data) {}
-
+        protected void forward(NameServiceRequest request, Priority priority) {
+            // Ignored
+        }
     }
 
 }
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 3ee4c6961a4..7064c6e1362 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
@@ -176,8 +176,8 @@ public class RoutingPoliciesTest {
 
         // Weight of inactive zone is set to zero
         tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, ImmutableMap.of(zone1, 1L,
-                                                                                  zone3, 1L,
-                                                                                  zone4, 0L));
+                                                                                           zone3, 1L,
+                                                                                           zone4, 0L));
 
         // Other zone in shared region is set out. Entire record group for the region is removed as all zones in the
         // region are out (weight sum = 0)
@@ -198,6 +198,27 @@ public class RoutingPoliciesTest {
     }
 
     @Test
+    public void global_routing_policies_legacy_global_service_id() {
+        var tester = new RoutingPoliciesTester();
+        var context = tester.newDeploymentContext("tenant1", "app1", "default");
+        int clustersPerZone = 2;
+        int numberOfDeployments = 2;
+        var applicationPackage = applicationPackageBuilder()
+                .region(zone1.region())
+                .region(zone2.region())
+                .globalServiceId("c0")
+                .build();
+        tester.provisionLoadBalancers(clustersPerZone, context.instanceId(), zone1, zone2);
+
+        // Creates alias records
+        context.submit(applicationPackage).deferLoadBalancerProvisioningIn(Environment.prod).deploy();
+        tester.assertTargets(context.instanceId(), EndpointId.defaultId(), 0, zone1, zone2);
+        assertEquals("Routing policy count is equal to cluster count",
+                     numberOfDeployments * clustersPerZone,
+                     tester.policiesOf(context.instance().id()).size());
+    }
+
+    @Test
     public void zone_routing_policies() {
         zone_routing_policies(false);
         zone_routing_policies(true);
@@ -294,8 +315,19 @@ public class RoutingPoliciesTest {
     }
 
     @Test
+    public void zone_routing_policies_without_dns_update() {
+        var tester = new RoutingPoliciesTester(new DeploymentTester(), false);
+        var context = tester.newDeploymentContext("tenant1", "app1", "default");
+        tester.provisionLoadBalancers(1, context.instanceId(), true, zone1, zone2);
+        context.submit(applicationPackage).deferLoadBalancerProvisioningIn(Environment.prod).deploy();
+        assertEquals(0, tester.controllerTester().controller().curator().readNameServiceQueue().requests().size());
+        assertEquals(Set.of(), tester.recordNames());
+        assertEquals(2, tester.policiesOf(context.instanceId()).size());
+    }
+
+    @Test
     public void global_routing_policies_in_rotationless_system() {
-        var tester = new RoutingPoliciesTester(new DeploymentTester(new ControllerTester(new RotationsConfig.Builder().build())));
+        var tester = new RoutingPoliciesTester(new DeploymentTester(new ControllerTester(new RotationsConfig.Builder().build())), true);
         var context = tester.newDeploymentContext("tenant1", "app1", "default");
         tester.provisionLoadBalancers(1, context.instanceId(), zone1, zone2);
 
@@ -692,7 +724,7 @@ public class RoutingPoliciesTest {
         }
 
         public RoutingPoliciesTester(SystemName system) {
-            this(new DeploymentTester(new ControllerTester(new ServiceRegistryMock(system))));
+            this(new DeploymentTester(new ControllerTester(new ServiceRegistryMock(system))), true);
         }
 
         public RoutingPolicies routingPolicies() {
@@ -707,14 +739,15 @@ public class RoutingPoliciesTest {
             return tester.controllerTester();
         }
 
-        public RoutingPoliciesTester(DeploymentTester tester) {
+        public RoutingPoliciesTester(DeploymentTester tester, boolean exclusiveRouting) {
             this.tester = tester;
             List<ZoneApi> zones = new ArrayList<>(tester.controllerTester().zoneRegistry().zones().all().zones());
             zones.add(ZoneApiMock.from(zone3));
             zones.add(ZoneApiMock.from(zone4));
-            tester.controllerTester().zoneRegistry()
-                  .setZones(zones)
-                  .exclusiveRoutingIn(zones);
+            tester.controllerTester().zoneRegistry().setZones(zones);
+            if (exclusiveRouting) {
+                  tester.controllerTester().zoneRegistry().exclusiveRoutingIn(zones);
+            }
             tester.controllerTester().configServer().bootstrap(tester.controllerTester().zoneRegistry().zones().all().ids(),
                                                                SystemApplication.all());
             ((InMemoryFlagSource) tester.controllerTester().controller().flagSource()).withBooleanFlag(Flags.WEIGHTED_DNS_PER_REGION.id(), true);