diff options
author | Martin Polden <mpolden@mpolden.no> | 2019-06-06 14:45:50 +0200 |
---|---|---|
committer | Martin Polden <mpolden@mpolden.no> | 2019-06-06 14:48:31 +0200 |
commit | 066dbed6da6e52e71112f478b44338ba87ab1944 (patch) | |
tree | 661c1672a373cd744324e8fe3f0a0d9ca6aa67ad | |
parent | df757401715013727123f58f3b9e1aac6dae7f87 (diff) |
Improve validation of endpoint IDs
Same validation rules that are in place for `<rotations/>' in `services.xml`,
which we are removing.
3 files changed, 72 insertions, 3 deletions
diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/Endpoint.java b/config-model-api/src/main/java/com/yahoo/config/application/api/Endpoint.java index 73328027540..7d31e07dac5 100644 --- a/config-model-api/src/main/java/com/yahoo/config/application/api/Endpoint.java +++ b/config-model-api/src/main/java/com/yahoo/config/application/api/Endpoint.java @@ -5,6 +5,7 @@ import com.yahoo.config.provision.RegionName; import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.regex.Pattern; import java.util.stream.Collectors; /** @@ -13,19 +14,37 @@ import java.util.stream.Collectors; * should point to. * * If the endpointId is not set, it will default to the same as the containerId. + * + * @author ogronnesby */ public class Endpoint { + + /* + * Endpoint IDs must be: + * - lowercase + * - alphanumeric + * - begin with a character + * - contain zero consecutive dashes + * - have a length between 1 and 12 + */ + private static final Pattern endpointPattern = Pattern.compile("^[a-z](?:-?[a-z0-9]+)*$"); + private static final int endpointMaxLength = 12; + private final Optional<String> endpointId; private final String containerId; private final Set<RegionName> regions; public Endpoint(Optional<String> endpointId, String containerId, Set<String> regions) { - this.endpointId = endpointId; - this.containerId = containerId; + this.endpointId = Objects.requireNonNull(endpointId, "endpointId must be non-null"); + this.containerId = Objects.requireNonNull(containerId, "containerId must be non-null"); this.regions = Set.copyOf( Objects.requireNonNull( regions.stream().map(RegionName::from).collect(Collectors.toList()), "Missing 'regions' parameter")); + + if (endpointId().length() > endpointMaxLength || !endpointPattern.matcher(endpointId()).matches()) { + throw new IllegalArgumentException("Invalid endpoint ID: '" + endpointId() + "'"); + } } public String endpointId() { @@ -54,4 +73,5 @@ public class Endpoint { public int hashCode() { return Objects.hash(endpointId, containerId, regions); } + } 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 58795f6ea9e..650f68591b6 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 @@ -183,7 +183,11 @@ public class DeploymentSpecXmlReader { regions.add(region); } - endpoints.add(new Endpoint(rotationId, containerId.get(), regions)); + var endpoint = new Endpoint(rotationId, containerId.get(), regions); + if (endpoints.contains(endpoint)) { + throw new IllegalArgumentException("Duplicate 'endpoint' in 'endpoints' tag"); + } + endpoints.add(endpoint); } return endpoints; 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 3049d511bf1..c161b04087f 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 @@ -493,4 +493,49 @@ public class DeploymentSpecTest { assertEquals(Set.of(RegionName.from("us-east")), spec.endpoints().get(0).regions()); } + @Test + public void invalidEndpoints() { + assertInvalid("<endpoint id='FOO' container-id='qrs'/>"); // Uppercase + assertInvalid("<endpoint id='123' container-id='qrs'/>"); // Starting with non-character + assertInvalid("<endpoint id='foo!' container-id='qrs'/>"); // Non-alphanumeric + assertInvalid("<endpoint id='foo.bar' container-id='qrs'/>"); + assertInvalid("<endpoint id='foo--bar' container-id='qrs'/>"); // Multiple consecutive dashes + assertInvalid("<endpoint id='foo-' container-id='qrs'/>"); // Trailing dash + assertInvalid("<endpoint id='foooooooooooo' container-id='qrs'/>"); // Too long + assertInvalid("<endpoint id='foo' container-id='qrs'/><endpoint id='foo' container-id='qrs'/>"); // Duplicate + } + + @Test + public void validEndpoints() { + assertEquals(List.of("qrs"), endpointIds("<endpoint container-id='qrs'/>")); + assertEquals(List.of("qrs"), endpointIds("<endpoint id='' container-id='qrs'/>")); + assertEquals(List.of("f"), endpointIds("<endpoint id='f' container-id='qrs'/>")); + assertEquals(List.of("foo"), endpointIds("<endpoint id='foo' container-id='qrs'/>")); + assertEquals(List.of("foo-bar"), endpointIds("<endpoint id='foo-bar' container-id='qrs'/>")); + assertEquals(List.of("foo", "bar"), endpointIds("<endpoint id='foo' container-id='qrs'/><endpoint id='bar' container-id='qrs'/>")); + assertEquals(List.of("fooooooooooo"), endpointIds("<endpoint id='fooooooooooo' container-id='qrs'/>")); + } + + private static void assertInvalid(String endpointTag) { + try { + endpointIds(endpointTag); + fail("Expected exception for input '" + endpointTag + "'"); + } catch (IllegalArgumentException ignored) {} + } + + private static List<String> endpointIds(String endpointTag) { + var xml = "<deployment>" + + " <prod>" + + " <region active=\"true\">us-east</region>" + + " </prod>" + + " <endpoints>" + + endpointTag + + " </endpoints>" + + "</deployment>"; + + return DeploymentSpec.fromXml(xml).endpoints().stream() + .map(Endpoint::endpointId) + .collect(Collectors.toList()); + } + } |