Validate zone endpoint changes in controller, on submission

5 files changed, 150 insertions, 12 deletions

diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java
index 9d584efcd6b..b36c1409459 100644
--- a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java
+++ b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java
@@ -269,7 +269,16 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps {
     }
 
     /** Returns the zone endpoint data for this instance. */
-    Map<ClusterSpec.Id, Map<ZoneId, ZoneEndpoint>> zoneEndpoints() { return zoneEndpoints; }
+    Map<ClusterSpec.Id, Map<ZoneId, ZoneEndpoint>> zoneEndpoints() {
+        return zoneEndpoints;
+    }
+
+    /** The zone endpoints in the given zone, possibly default values. */
+    public Map<ClusterSpec.Id, ZoneEndpoint> zoneEndpoints(ZoneId zone) {
+        return zoneEndpoints.keySet().stream()
+                            .collect(Collectors.toMap(cluster -> cluster,
+                                                      cluster -> zoneEndpoint(zone, cluster).orElse(ZoneEndpoint.defaultEndpoint)));
+    }
 
     @Override
     public boolean equals(Object o) {
diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationId.java b/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationId.java
index 571cc3c7d5c..b4be99ad20b 100644
--- a/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationId.java
+++ b/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationId.java
@@ -24,6 +24,7 @@ public enum ValidationId {
     skipOldConfigModels("skip-old-config-models"), // Internal use
     accessControl("access-control"), // Internal use, used in zones where there should be no access-control
     globalEndpointChange("global-endpoint-change"), // Changing global endpoints
+    zoneEndpointChange("zone-endpoint-change"), // Changing zone (possibly private) endpoint settings
     redundancyIncrease("redundancy-increase"), // Increasing redundancy - may easily cause feed blocked
     redundancyOne("redundancy-one"), // redundancy=1 requires a validation override on first deployment
     pagedSettingRemoval("paged-setting-removal"), // May cause content nodes to run out of memory
diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/ZoneEndpoint.java b/config-provisioning/src/main/java/com/yahoo/config/provision/ZoneEndpoint.java
index a14747a0226..10e22f8df06 100644
--- a/config-provisioning/src/main/java/com/yahoo/config/provision/ZoneEndpoint.java
+++ b/config-provisioning/src/main/java/com/yahoo/config/provision/ZoneEndpoint.java
@@ -120,10 +120,11 @@ public class ZoneEndpoint {
 
         @Override
         public String toString() {
-            return "AllowedUrn{" +
-                   "type=" + type +
-                   ", urn='" + urn + '\'' +
-                   '}';
+            return "'" + urn + "' through '" +
+                   switch (type) {
+                       case awsPrivateLink -> "aws-private-link";
+                       case gcpServiceConnect -> "gcp-service-connect";
+                   } + "'";
         }
 
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageValidator.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageValidator.java
index bce9d44f8c6..842d99abe71 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageValidator.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageValidator.java
@@ -4,26 +4,28 @@ package com.yahoo.vespa.hosted.controller.application.pkg;
 import com.yahoo.component.Version;
 import com.yahoo.config.application.api.DeploymentInstanceSpec;
 import com.yahoo.config.application.api.DeploymentSpec;
+import com.yahoo.config.application.api.DeploymentSpec.DeclaredZone;
 import com.yahoo.config.application.api.Endpoint;
 import com.yahoo.config.application.api.Endpoint.Level;
 import com.yahoo.config.application.api.ValidationId;
 import com.yahoo.config.application.api.ValidationOverrides;
 import com.yahoo.config.provision.CloudAccount;
 import com.yahoo.config.provision.CloudName;
+import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.RegionName;
+import com.yahoo.config.provision.ZoneEndpoint;
+import com.yahoo.config.provision.ZoneEndpoint.AllowedUrn;
 import com.yahoo.config.provision.zone.ZoneApi;
 import com.yahoo.config.provision.zone.ZoneId;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.Controller;
-import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId;
 import com.yahoo.vespa.hosted.controller.application.EndpointId;
 import com.yahoo.vespa.hosted.controller.versions.VespaVersion;
 
 import java.time.Instant;
 import java.util.ArrayList;
-import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -150,10 +152,10 @@ public class ApplicationPackageValidator {
 
     /** Verify endpoint configuration of given application package */
     private void validateEndpointChange(Application application, ApplicationPackage applicationPackage, Instant instant) {
-        applicationPackage.deploymentSpec().instances().forEach(instance -> validateEndpointChange(application,
-                                                                                                   instance.name(),
-                                                                                                   applicationPackage,
-                                                                                                   instant));
+        for (DeploymentInstanceSpec instance : applicationPackage.deploymentSpec().instances()) {
+            validateGlobalEndpointChanges(application, instance.name(), applicationPackage, instant);
+            validateZoneEndpointChanges(application, instance.name(), applicationPackage, instant);
+        }
     }
 
     /** Verify that compactable endpoint parts (instance name and endpoint ID) do not clash */
@@ -176,7 +178,7 @@ public class ApplicationPackageValidator {
     }
 
     /** Verify changes to endpoint configuration by comparing given application package to the existing one, if any */
-    private void validateEndpointChange(Application application, InstanceName instanceName, ApplicationPackage applicationPackage, Instant instant) {
+    private void validateGlobalEndpointChanges(Application application, InstanceName instanceName, ApplicationPackage applicationPackage, Instant instant) {
         var validationId = ValidationId.globalEndpointChange;
         if (applicationPackage.validationOverrides().allows(validationId, instant)) return;
 
@@ -200,6 +202,43 @@ public class ApplicationPackageValidator {
                                            ". " + ValidationOverrides.toAllowMessage(validationId));
     }
 
+    /** Verify changes to endpoint configuration by comparing given application package to the existing one, if any */
+    private void validateZoneEndpointChanges(Application application, InstanceName instance, ApplicationPackage applicationPackage, Instant now) {
+        ValidationId validationId = ValidationId.zoneEndpointChange;
+        if (applicationPackage.validationOverrides().allows(validationId, now)) return;;
+
+        String prefix = validationId + ": application '" + application.id() +
+                        (instance.isDefault() ? "" : "." + instance.value()) + "' ";
+        DeploymentInstanceSpec spec = applicationPackage.deploymentSpec().requireInstance(instance);
+        for (DeclaredZone zone : spec.zones()) {
+            if (zone.environment() == Environment.prod) {
+                Map<ClusterSpec.Id, ZoneEndpoint> newEndpoints = spec.zoneEndpoints(ZoneId.from(zone.environment(), zone.region().get()));
+                application.deploymentSpec().instance(instance)                                     // If old spec has this instance ...
+                           .filter(oldSpec -> oldSpec.concerns(zone.environment(), zone.region()))  // ... and deploys to this zone ...
+                           .map(oldSpec -> oldSpec.zoneEndpoints(ZoneId.from(zone.environment(), zone.region().get())))
+                           .ifPresent(oldEndpoints -> {                                             // ... then we compare the endpoints present in both.
+                               oldEndpoints.forEach((cluster, oldEndpoint) -> {
+                                   ZoneEndpoint newEndpoint = newEndpoints.getOrDefault(cluster, ZoneEndpoint.defaultEndpoint);
+                                   if ( ! newEndpoint.allowedUrns().containsAll(oldEndpoint.allowedUrns()))
+                                       throw new IllegalArgumentException(prefix + "allows access to cluster '" + cluster.value() +
+                                                                          "' in '" + zone.region().get().value() + "' to " +
+                                                                          oldEndpoint.allowedUrns().stream().map(AllowedUrn::toString).collect(joining(", ", "[", "]")) +
+                                                                          ", but does not include all these in the new deployment spec. " +
+                                                                          "Deploying with the new settings will allow access to " +
+                                                                          (newEndpoint.allowedUrns().isEmpty() ? "no one" : newEndpoint.allowedUrns().stream().map(AllowedUrn::toString).collect(joining(", ", "[", "]"))));
+                               });
+                               newEndpoints.forEach((cluster, newEndpoint) -> {
+                                    ZoneEndpoint oldEndpoint = oldEndpoints.getOrDefault(cluster, ZoneEndpoint.defaultEndpoint);
+                                    if (oldEndpoint.isPublicEndpoint() && ! newEndpoint.isPublicEndpoint())
+                                        throw new IllegalArgumentException(prefix + "has a public endpoint for cluster '" + cluster.value() +
+                                                                           "' in '" + zone.region().get().value() + "', but the new deployment spec " +
+                                                                           "disables this");
+                               });
+                           });
+            }
+        }
+    }
+
     /** Returns whether newEndpoints contains all destinations in endpoints */
     private static boolean containsAllDestinationsOf(List<Endpoint> endpoints, List<Endpoint> newEndpoints) {
         var containsAllRegions = true;
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 95b81dffaed..db96c265363 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
@@ -1118,6 +1118,94 @@ public class ControllerTest {
     }
 
     @Test
+    void testZoneEndpointChanges() {
+        DeploymentContext app = tester.newDeploymentContext();
+        // Set up app with default settings.
+        app.submit(ApplicationPackageBuilder.fromDeploymentXml("""
+                                                               <deployment>
+                                                                 <prod>
+                                                                   <region>us-east-3</region>
+                                                                 </prod>
+                                                               </deployment>"""));
+
+        assertEquals("zone-endpoint-change: application 'tenant.application' has a public endpoint for cluster 'foo' in 'us-east-3', but the new deployment spec disables this",
+                     assertThrows(IllegalArgumentException.class,
+                                  () -> app.submit(ApplicationPackageBuilder.fromDeploymentXml("""
+                                                                                               <deployment>
+                                                                                                 <prod>
+                                                                                                   <region>us-east-3</region>
+                                                                                                 </prod>
+                                                                                                 <endpoints>
+                                                                                                   <endpoint type='zone' container-id='foo' enabled='false' />
+                                                                                                 </endpoints>
+                                                                                               </deployment>""")))
+                             .getMessage());
+
+        // Disabling endpoints is OK with override.
+        app.submit(ApplicationPackageBuilder.fromDeploymentXml("""
+                                                               <deployment>
+                                                                 <prod>
+                                                                   <region>us-east-3</region>
+                                                                 </prod>
+                                                                 <endpoints>
+                                                                   <endpoint type='zone' container-id='foo' enabled='false' />
+                                                                 </endpoints>
+                                                               </deployment>""",
+                                                               ValidationId.zoneEndpointChange));
+
+        // Enabling endpoints again is OK, as is adding a private endpoint with some URN.
+        app.submit(ApplicationPackageBuilder.fromDeploymentXml("""
+                                                               <deployment>
+                                                                 <prod>
+                                                                   <region>us-east-3</region>
+                                                                 </prod>
+                                                                 <endpoints>
+                                                                   <endpoint type='private' container-id='foo'>
+                                                                     <allow with='aws-private-link' arn='yarn' />
+                                                                   </endpoint>
+                                                                 </endpoints>
+                                                               </deployment>""",
+                                                               ValidationId.zoneEndpointChange));
+
+        // Changing URNs is guarded.
+        assertEquals("zone-endpoint-change: application 'tenant.application' allows access to cluster 'foo' in 'us-east-3' to " +
+                     "['yarn' through 'aws-private-link'], but does not include all these in the new deployment spec. " +
+                     "Deploying with the new settings will allow access to ['yarn' through 'gcp-service-connect']",
+                     assertThrows(IllegalArgumentException.class,
+                                  () -> app.submit(ApplicationPackageBuilder.fromDeploymentXml("""
+                                                                                               <deployment>
+                                                                                                 <prod>
+                                                                                                   <region>us-east-3</region>
+                                                                                                 </prod>
+                                                                                                 <endpoints>
+                                                                                                   <endpoint type='private' container-id='foo'>
+                                                                                                     <allow with='gcp-service-connect' project='yarn' />
+                                                                                                   </endpoint>
+                                                                                                 </endpoints>
+                                                                                               </deployment>""")))
+                             .getMessage());
+
+        // Changing cluster, effectively removing old URNs, is also guarded.
+        assertEquals("zone-endpoint-change: application 'tenant.application' allows access to cluster 'foo' in 'us-east-3' to " +
+                     "['yarn' through 'aws-private-link'], but does not include all these in the new deployment spec. " +
+                     "Deploying with the new settings will allow access to no one",
+                     assertThrows(IllegalArgumentException.class,
+                                  () -> app.submit(ApplicationPackageBuilder.fromDeploymentXml("""
+                                                                                               <deployment>
+                                                                                                 <prod>
+                                                                                                   <region>us-east-3</region>
+                                                                                                 </prod>
+                                                                                                 <endpoints>
+                                                                                                   <endpoint type='private' container-id='bar'>
+                                                                                                     <allow with='aws-private-link' arn='yarn' />
+                                                                                                   </endpoint>
+                                                                                                 </endpoints>
+                                                                                               </deployment>""")))
+                             .getMessage());
+    }
+
+
+        @Test
     void testReadableApplications() {
         var db = new MockCuratorDb(tester.controller().system());
         var tester = new DeploymentTester(new ControllerTester(db));