diff options
author | Jon Marius Venstad <jonmv@users.noreply.github.com> | 2020-12-07 10:16:19 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-12-07 10:16:19 +0100 |
commit | 60d60a7c1cdf2c9672acdbeeeff72a941140f72b (patch) | |
tree | 0baf810e87f9caed8485b13c851cb6d4e8dbd47e /node-repository | |
parent | c183491dd0d3ad22477f4c1f884bec07156956bf (diff) | |
parent | 3fb466294261a4de4a531dba94527ad5d9ea6135 (diff) |
Merge pull request #15682 from vespa-engine/mpolden/detect-clashing-names
Reject endpoints with clashing non-compactable IDs
Diffstat (limited to 'node-repository')
2 files changed, 70 insertions, 3 deletions
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java index 206aa077027..0b5a04ca42c 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java @@ -2,10 +2,12 @@ package com.yahoo.vespa.hosted.provision.provisioning; import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.ApplicationName; import com.yahoo.config.provision.ApplicationTransaction; import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.NodeType; +import com.yahoo.config.provision.TenantName; import com.yahoo.config.provision.exception.LoadBalancerServiceException; import com.yahoo.transaction.NestedTransaction; import com.yahoo.vespa.flags.BooleanFlag; @@ -82,7 +84,8 @@ public class LoadBalancerProvisioner { try (var lock = db.lock(application)) { ClusterSpec.Id clusterId = effectiveId(cluster); List<Node> nodes = nodesOf(clusterId, application); - provision(application, clusterId, nodes, false); + LoadBalancerId loadBalancerId = requireNonClashing(new LoadBalancerId(application, clusterId)); + provision(loadBalancerId, nodes, false); } } @@ -149,9 +152,30 @@ public class LoadBalancerProvisioner { return canForwardTo; } + /** Find all load balancer IDs owned by given tenant and application */ + private List<LoadBalancerId> findLoadBalancers(TenantName tenant, ApplicationName application) { + return db.readLoadBalancerIds().stream() + .filter(id -> id.application().tenant().equals(tenant) && + id.application().application().equals(application)) + .collect(Collectors.toUnmodifiableList()); + } + + /** Require that load balancer IDs do not clash. This prevents name clashing when compacting endpoint DNS names */ + private LoadBalancerId requireNonClashing(LoadBalancerId loadBalancerId) { + List<LoadBalancerId> loadBalancerIds = findLoadBalancers(loadBalancerId.application().tenant(), + loadBalancerId.application().application()); + List<String> nonCompactableIds = withoutCompactableIds(loadBalancerId); + for (var id : loadBalancerIds) { + if (id.equals(loadBalancerId)) continue; + if (nonCompactableIds.equals(withoutCompactableIds(id))) { + throw new IllegalArgumentException(loadBalancerId + " clashes with " + id); + } + } + return loadBalancerId; + } + /** Idempotently provision a load balancer for given application and cluster */ - private void provision(ApplicationId application, ClusterSpec.Id clusterId, List<Node> nodes, boolean activate) { - var id = new LoadBalancerId(application, clusterId); + private void provision(LoadBalancerId id, List<Node> nodes, boolean activate) { var now = nodeRepository.clock().instant(); var loadBalancer = db.readLoadBalancer(id); if (loadBalancer.isEmpty() && activate) return; // Nothing to activate as this load balancer was never prepared @@ -171,6 +195,10 @@ public class LoadBalancerProvisioner { db.writeLoadBalancer(newLoadBalancer); } + private void provision(ApplicationId application, ClusterSpec.Id clusterId, List<Node> nodes, boolean activate) { + provision(new LoadBalancerId(application, clusterId), nodes, activate); + } + private LoadBalancerInstance provisionInstance(LoadBalancerId id, List<Node> nodes, boolean force) { var reals = new LinkedHashSet<Real>(); for (var node : nodes) { @@ -206,6 +234,18 @@ public class LoadBalancerProvisioner { return nodes.stream().collect(Collectors.groupingBy(node -> effectiveId(node.allocation().get().membership().cluster()))); } + /** Returns a list of the non-compactable IDs of given load balancer */ + private static List<String> withoutCompactableIds(LoadBalancerId id) { + List<String> ids = new ArrayList<>(2); + if (!"default".equals(id.cluster().value())) { + ids.add(id.cluster().value()); + } + if (!id.application().instance().isDefault()) { + ids.add(id.application().instance().value()); + } + return ids; + } + /** Find IP addresses reachable by the load balancer service */ private Set<String> reachableIpAddresses(Node node) { Set<String> reachable = new LinkedHashSet<>(node.ipConfig().primary()); @@ -225,4 +265,5 @@ public class LoadBalancerProvisioner { return cluster.combinedId().orElse(cluster.id()); } + } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java index c51ef7250e2..7a636a030ec 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java @@ -33,6 +33,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; /** * @author mpolden @@ -236,6 +237,31 @@ public class LoadBalancerProvisionerTest { assertEquals(cluster, lbs.get().get(0).id().cluster()); } + @Test + public void reject_load_balancers_with_clashing_names() { + ApplicationId instance1 = ApplicationId.from("t1", "a1", "default"); + ApplicationId instance2 = ApplicationId.from("t1", "a1", "dev"); + ApplicationId instance3 = ApplicationId.from("t1", "a1", "qrs"); + ClusterSpec.Id devCluster = ClusterSpec.Id.from("dev"); + ClusterSpec.Id defaultCluster = ClusterSpec.Id.from("default"); + + // instance1 is deployed + tester.activate(instance1, prepare(instance1, clusterRequest(ClusterSpec.Type.container, devCluster))); + + // instance2 clashes because cluster name matches instance1 + try { + prepare(instance2, clusterRequest(ClusterSpec.Type.container, defaultCluster)); + fail("Expected exception"); + } catch (IllegalArgumentException ignored) { + } + + // instance2 changes cluster name and does not clash + tester.activate(instance2, prepare(instance2, clusterRequest(ClusterSpec.Type.container, ClusterSpec.Id.from("qrs")))); + + // instance3 clashes because instance name matches instance2 cluster + tester.activate(instance3, prepare(instance3, clusterRequest(ClusterSpec.Type.container, defaultCluster))); + } + private void dirtyNodesOf(ApplicationId application) { tester.nodeRepository().setDirty(tester.nodeRepository().getNodes(application), Agent.system, this.getClass().getSimpleName()); } |