diff options
author | Martin Polden <mpolden@mpolden.no> | 2021-11-05 10:35:33 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-11-05 10:35:33 +0100 |
commit | 9c4689e1094bba36a28299f04fbbed1fea5b88ee (patch) | |
tree | 44376dd931badd1dfd66b1575b945bbb170829ba | |
parent | 076647d7d136824a5e32ebfbd08a9473098cbe14 (diff) | |
parent | d9e4c7e1543ed96a2ddd29ec8c693366d9b85dfb (diff) |
Merge pull request #19880 from vespa-engine/mpolden/application-level-endpoint-syntax
Finalize application-level endpoint syntax
3 files changed, 59 insertions, 52 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"); diff --git a/config-model/src/main/resources/schema/deployment.rnc b/config-model/src/main/resources/schema/deployment.rnc index ecd43d1ad28..f24750bde8b 100644 --- a/config-model/src/main/resources/schema/deployment.rnc +++ b/config-model/src/main/resources/schema/deployment.rnc @@ -113,15 +113,20 @@ Delay = element delay { } EndpointRegion = element region { - attribute instance { xsd:string }? & - attribute weight { xsd:long }? & + text +} + +EndpointInstance = element instance { + attribute weight { xsd:long } & text } Endpoint = element endpoint { attribute id { xsd:string }? & attribute container-id { xsd:string } & - EndpointRegion* + attribute region { xsd:string }? & + EndpointRegion* & + EndpointInstance* } Endpoints = element endpoints { |