Merge pull request #19880 from vespa-engine/mpolden/application-level-endpoint-syntax

Finalize application-level endpoint syntax

2 files changed, 51 insertions, 49 deletions

diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java b/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java
index 17a18cbb8c2..4ca9ee1dc2f 100644
--- a/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java
+++ b/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java
@@ -60,7 +60,6 @@ public class DeploymentSpecXmlReader {
     private static final String endpointTag = "endpoint";
     private static final String notificationsTag = "notifications";
 
-
     private static final String idAttribute = "id";
     private static final String athenzServiceAttribute = "athenz-service";
     private static final String athenzDomainAttribute = "athenz-domain";
@@ -276,32 +275,35 @@ public class DeploymentSpecXmlReader {
             String containerId = requireStringAttribute("container-id", endpointElement);
             String msgPrefix = (level == Endpoint.Level.application ? "Application-level" : "Instance-level") +
                                " endpoint '" + endpointId + "': ";
+            String invalidChild = level == Endpoint.Level.application ? "region" : "instance";
+            if (!XML.getChildren(endpointElement, invalidChild).isEmpty()) illegal(msgPrefix + "invalid element '" + invalidChild + "'");
 
             List<Endpoint.Target> targets = new ArrayList<>();
-            for (var regionElement : XML.getChildren(endpointElement, "region")) {
-                String region = regionElement.getTextContent();
-                if (region == null || region.isBlank()) illegal(msgPrefix + "empty 'region' element");
-                Optional<String> instanceFromAttribute = stringAttribute("instance", regionElement);
-                Optional<String> weightFromAttribute = stringAttribute("weight", regionElement);
-                if (level == Endpoint.Level.application) {
-                    if (instanceFromAttribute.isEmpty()) illegal(msgPrefix + "element 'region' must have 'instance' attribute");
-                    if (weightFromAttribute.isEmpty()) illegal(msgPrefix + "element 'region' must have 'weight' attribute");
-                } else {
-                    if (instanceFromAttribute.isPresent()) illegal(msgPrefix + "element 'region' cannot have 'instance' attribute");
-                    if (weightFromAttribute.isPresent()) illegal(msgPrefix + "element 'region' cannot have 'weight' attribute");
-                    instanceFromAttribute = instance;
-                }
-                int weight = 1;
-                if (weightFromAttribute.isPresent()) {
+            if (level == Endpoint.Level.application) {
+                String region = requireStringAttribute("region", endpointElement);
+                for (var instanceElement : XML.getChildren(endpointElement, "instance")) {
+                    String instanceName = instanceElement.getTextContent();
+                    String weightFromAttribute = requireStringAttribute("weight", instanceElement);
+                    if (instanceName == null || instanceName.isBlank()) illegal(msgPrefix + "empty 'instance' element");
+                    int weight;
                     try {
-                        weight = Integer.parseInt(weightFromAttribute.get());
+                        weight = Integer.parseInt(weightFromAttribute);
                     } catch (NumberFormatException e) {
-                        throw new IllegalArgumentException(msgPrefix + "invalid weight value '" + weightFromAttribute.get() + "'");
+                        throw new IllegalArgumentException(msgPrefix + "invalid weight value '" + weightFromAttribute + "'");
                     }
+                    targets.add(new Endpoint.Target(RegionName.from(region),
+                                                    InstanceName.from(instanceName),
+                                                    weight));
+                }
+            } else {
+                if (stringAttribute("region", endpointElement).isPresent()) illegal(msgPrefix + "invalid 'region' attribute");
+                for (var regionElement : XML.getChildren(endpointElement, "region")) {
+                    String region = regionElement.getTextContent();
+                    if (region == null || region.isBlank()) illegal(msgPrefix + "empty 'region' element");
+                    targets.add(new Endpoint.Target(RegionName.from(region),
+                                                    InstanceName.from(instance.get()),
+                                                    1));
                 }
-                targets.add(new Endpoint.Target(RegionName.from(region),
-                                                InstanceName.from(instanceFromAttribute.get()),
-                                                weight));
             }
             if (targets.isEmpty() && level == Endpoint.Level.instance) {
                 // No explicit targets given for instance level endpoint. Include all declared regions by default
diff --git a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java
index 72c873a6eab..8f9c9bccfcd 100644
--- a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java
+++ b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java
@@ -1181,22 +1181,22 @@ public class DeploymentSpecTest {
     }
 
     @Test
-    public void instanceEndpointDisallowsApplicationLevelAttributes() {
+    public void instanceEndpointDisallowsRegionAttributeOrInstanceTag() {
         String xmlForm = "<deployment>\n" +
-                         "  <prod>\n" +
-                         "    <region active=\"true\">us-east</region>\n" +
-                         "    <region active=\"true\">us-west</region>\n" +
-                         "  </prod>\n" +
-                         "  <endpoints>\n" +
-                         "    <endpoint id=\"foo\" container-id=\"bar\">\n" +
-                         "      <region %s>us-east</region>\n" +
-                         "    </endpoint>\n" +
-                         "  </endpoints>\n" +
+                         "  <instance id='default'>\n" +
+                         "    <prod>\n" +
+                         "      <region active=\"true\">us-east</region>\n" +
+                         "      <region active=\"true\">us-west</region>\n" +
+                         "    </prod>\n" +
+                         "    <endpoints>\n" +
+                         "      <endpoint id=\"foo\" container-id=\"bar\" %s>\n" +
+                         "        %s\n" +
+                         "      </endpoint>\n" +
+                         "    </endpoints>\n" +
+                         "  </instance>\n" +
                          "</deployment>";
-        assertInvalid(String.format(xmlForm, "instance='foo'"),
-                      "Instance-level endpoint 'foo': element 'region' cannot have 'instance' attribute");
-        assertInvalid(String.format(xmlForm, "weight='1'"),
-                      "Instance-level endpoint 'foo': element 'region' cannot have 'weight' attribute");
+        assertInvalid(String.format(xmlForm, "region='us-east'", "<region>us-east</region>"), "Instance-level endpoint 'foo': invalid 'region' attribute");
+        assertInvalid(String.format(xmlForm, "", "<instance>us-east</instance>"), "Instance-level endpoint 'foo': invalid element 'instance'");
     }
 
     @Test
@@ -1215,18 +1215,17 @@ public class DeploymentSpecTest {
                          "    </prod>\n" +
                          "  </instance>\n" +
                          "  <endpoints>\n" +
-                         "    <endpoint id=\"foo\" container-id=\"qrs\">\n" +
-                         "      <region %s>%s</region>\n" +
+                         "    <endpoint id=\"foo\" container-id=\"qrs\" %s>\n" +
+                         "      <instance %s>%s</instance>\n%s" +
                          "    </endpoint>\n" +
                          "  </endpoints>\n" +
                          "</deployment>\n";
-        assertInvalid(String.format(xmlForm, "", "us-west-1"), "Application-level endpoint 'foo': element 'region' must have 'instance' attribute");
-        assertInvalid(String.format(xmlForm, "instance='beta'", "us-west-1"), "Application-level endpoint 'foo': element 'region' must have 'weight' attribute");
-        assertInvalid(String.format(xmlForm, "instance='foo' weight='1'", "us-west-1"), "Application-level endpoint 'foo': targets undeclared instance 'foo'");
-        assertInvalid(String.format(xmlForm, "instance='beta' weight='foo'", "us-west-1"), "Application-level endpoint 'foo': invalid weight value 'foo'");
-        assertInvalid(String.format(xmlForm, "instance='beta' weight='1'", "eu-north-1"), "Application-level endpoint 'foo': targets undeclared region 'eu-north-1' in instance 'beta'");
-        assertInvalid(String.format(xmlForm, "instance='main' weight='1'", "us-west-1</region><region instance ='beta' weight='1'>us-east-3"),
-                      "Instance 'beta' declares a region different from instance 'main': 'us-east-3'");
+        assertInvalid(String.format(xmlForm, "", "", "", ""), "Missing required attribute 'region' in 'endpoint'");
+        assertInvalid(String.format(xmlForm, "region='us-west-1'", "", "main", ""), "Missing required attribute 'weight' in 'instance");
+        assertInvalid(String.format(xmlForm, "region='us-west-1'", "weight='1'", "", ""), "Application-level endpoint 'foo': empty 'instance' element");
+        assertInvalid(String.format(xmlForm, "region='invalid'", "weight='1'", "main", ""), "Application-level endpoint 'foo': targets undeclared region 'invalid' in instance 'main'");
+        assertInvalid(String.format(xmlForm, "region='us-west-1'", "weight='foo'", "main", ""), "Application-level endpoint 'foo': invalid weight value 'foo'");
+        assertInvalid(String.format(xmlForm, "region='us-west-1'", "weight='1'", "main", "<region>us-east-3</region>"), "Application-level endpoint 'foo': invalid element 'region'");
     }
 
     @Test
@@ -1248,12 +1247,12 @@ public class DeploymentSpecTest {
                                                      "    </endpoints>\n" +
                                                      "  </instance>\n" +
                                                      "  <endpoints>\n" +
-                                                     "    <endpoint id=\"foo\" container-id=\"movies\">\n" +
-                                                     "      <region instance=\"beta\" weight=\"2\">us-west-1</region>\n" +
-                                                     "      <region instance=\"main\" weight=\"8\">us-west-1</region>\n" +
+                                                     "    <endpoint id=\"foo\" container-id=\"movies\" region='us-west-1'>\n" +
+                                                     "      <instance weight=\"2\">beta</instance>\n" +
+                                                     "      <instance weight=\"8\">main</instance>\n" +
                                                      "    </endpoint>\n" +
-                                                     "    <endpoint id=\"bar\" container-id=\"music\">\n" +
-                                                     "      <region instance=\"main\" weight=\"10\">us-east-3</region>\n" +
+                                                     "    <endpoint id=\"bar\" container-id=\"music\" region='us-east-3'>\n" +
+                                                     "      <instance weight=\"10\">main</instance>\n" +
                                                      "    </endpoint>\n" +
                                                      "  </endpoints>\n" +
                                                      "</deployment>\n");
@@ -1270,6 +1269,7 @@ public class DeploymentSpecTest {
     }
 
     private static void assertInvalid(String deploymentSpec, String errorMessagePart) {
+        if (errorMessagePart.isEmpty()) throw new IllegalArgumentException("Message part must be non-empty");
         try {
             DeploymentSpec.fromXml(deploymentSpec);
             fail("Expected exception");