From 6318428ce2bb74f452eaba35df760483f8b7989d Mon Sep 17 00:00:00 2001 From: jonmv Date: Fri, 27 Jan 2023 15:50:24 +0100 Subject: Provision Internal LBs from AWS for non-public endpoints --- .../provision/lb/LoadBalancerServiceMock.java | 4 +- .../hosted/provision/lb/LoadBalancerSpec.java | 6 +-- .../provision/lb/SharedLoadBalancerService.java | 4 +- .../provisioning/LoadBalancerProvisioner.java | 52 ++++++++++++++-------- .../provisioning/LoadBalancerProvisionerTest.java | 34 ++++++++++++++ 5 files changed, 75 insertions(+), 25 deletions(-) diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java index c19aebcda6e..e74e3068674 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java @@ -62,8 +62,8 @@ public class LoadBalancerServiceMock implements LoadBalancerService { Collections.singleton(4443), ImmutableSet.of("10.2.3.0/24", "10.4.5.0/24"), spec.reals(), - spec.settings().orElse(ZoneEndpoint.defaultEndpoint), - spec.settings().map(__ -> PrivateServiceId.of("service")), + spec.settings(), + spec.settings().isPrivateEndpoint() ? Optional.of(PrivateServiceId.of("service")) : Optional.empty(), spec.cloudAccount()); instances.put(id, instance); return instance; diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerSpec.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerSpec.java index e0ef6739542..6be8bd71553 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerSpec.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerSpec.java @@ -21,7 +21,7 @@ public class LoadBalancerSpec { private final ApplicationId application; private final ClusterSpec.Id cluster; private final Set reals; - private final Optional settings; + private final ZoneEndpoint settings; private final CloudAccount cloudAccount; public LoadBalancerSpec(ApplicationId application, ClusterSpec.Id cluster, Set reals, @@ -29,7 +29,7 @@ public class LoadBalancerSpec { this.application = Objects.requireNonNull(application); this.cluster = Objects.requireNonNull(cluster); this.reals = ImmutableSortedSet.copyOf(Objects.requireNonNull(reals)); - this.settings = Optional.ofNullable(settings); + this.settings = Objects.requireNonNull(settings); this.cloudAccount = Objects.requireNonNull(cloudAccount); } @@ -49,7 +49,7 @@ public class LoadBalancerSpec { } /** Static user-configured settings for this load balancer. */ - public Optional settings() { + public ZoneEndpoint settings() { return settings; } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerService.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerService.java index 0722bb8e1dc..fa672c0fd1e 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerService.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerService.java @@ -29,13 +29,15 @@ public class SharedLoadBalancerService implements LoadBalancerService { @Override public LoadBalancerInstance create(LoadBalancerSpec spec, boolean force) { + if ( ! spec.settings().isPublicEndpoint()) + throw new IllegalArgumentException("non-public endpoints is not supported with " + getClass()); return new LoadBalancerInstance(Optional.of(DomainName.of(vipHostname)), Optional.empty(), Optional.empty(), Set.of(4443), Set.of(), spec.reals(), - spec.settings().orElse(ZoneEndpoint.defaultEndpoint), + spec.settings(), Optional.empty(), spec.cloudAccount()); } 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 92fdb1d2e52..fc03ddcc3b0 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 @@ -81,7 +81,7 @@ public class LoadBalancerProvisioner { * Prepare a load balancer for given application and cluster. *

* If a load balancer for the cluster already exists, it will be reconfigured based on the currently allocated - * nodes. It's state will remain unchanged. + * nodes. Its state will remain unchanged. *

* If no load balancer exists, a new one will be provisioned in {@link LoadBalancer.State#reserved}. *

@@ -93,7 +93,7 @@ public class LoadBalancerProvisioner { ClusterSpec.Id clusterId = effectiveId(cluster); LoadBalancerId loadBalancerId = requireNonClashing(new LoadBalancerId(application, clusterId)); NodeList nodes = nodesOf(clusterId, application); - prepare(loadBalancerId, nodes, requestedNodes.cloudAccount()); + prepare(loadBalancerId, nodes, cluster.zoneEndpoint(), requestedNodes.cloudAccount()); } } @@ -189,10 +189,10 @@ public class LoadBalancerProvisioner { return loadBalancerId; } - private void prepare(LoadBalancerId id, NodeList nodes, CloudAccount cloudAccount) { + private void prepare(LoadBalancerId id, NodeList nodes, ZoneEndpoint zoneEndpoint, CloudAccount cloudAccount) { Instant now = nodeRepository.clock().instant(); Optional loadBalancer = db.readLoadBalancer(id); - Optional instance = provisionInstance(id, nodes, loadBalancer, null, cloudAccount); + Optional instance = provisionInstance(id, nodes, loadBalancer, zoneEndpoint, cloudAccount, true); LoadBalancer newLoadBalancer; LoadBalancer.State fromState = null; if (loadBalancer.isEmpty()) { @@ -206,9 +206,19 @@ public class LoadBalancerProvisioner { // provision a new one in the wanted account newLoadBalancer = newLoadBalancer.with(LoadBalancer.State.removable, now); } + if (!hasCorrectVisibility(newLoadBalancer, zoneEndpoint)) { + // We have a load balancer, but with the wrong visibility. Load balancer must be removed before we can + // provision a new one with the wanted visibility + newLoadBalancer = newLoadBalancer.with(LoadBalancer.State.removable, now); + } // Always store the load balancer. LoadBalancerExpirer will remove unwanted ones db.writeLoadBalancer(newLoadBalancer, fromState); - requireInstance(id, instance, cloudAccount); + requireInstance(id, newLoadBalancer, cloudAccount, zoneEndpoint); + } + + private static boolean hasCorrectVisibility(LoadBalancer newLoadBalancer, ZoneEndpoint zoneEndpoint) { + return newLoadBalancer.instance().isEmpty() + || newLoadBalancer.instance().get().settings().isPublicEndpoint() == zoneEndpoint.isPublicEndpoint(); } private void activate(ApplicationTransaction transaction, ClusterSpec.Id cluster, ZoneEndpoint settings, NodeList nodes) { @@ -218,18 +228,19 @@ public class LoadBalancerProvisioner { if (loadBalancer.isEmpty()) throw new IllegalArgumentException("Could not activate load balancer that was never prepared: " + id); if (loadBalancer.get().instance().isEmpty()) throw new IllegalArgumentException("Activating " + id + ", but prepare never provisioned a load balancer instance"); - Optional instance = provisionInstance(id, nodes, loadBalancer, settings, loadBalancer.get().instance().get().cloudAccount()); + Optional instance = provisionInstance(id, nodes, loadBalancer, settings, loadBalancer.get().instance().get().cloudAccount(), false); LoadBalancer.State state = instance.isPresent() ? LoadBalancer.State.active : loadBalancer.get().state(); LoadBalancer newLoadBalancer = loadBalancer.get().with(instance).with(state, now); db.writeLoadBalancers(List.of(newLoadBalancer), loadBalancer.get().state(), transaction.nested()); - requireInstance(id, instance, loadBalancer.get().instance().get().cloudAccount()); + requireInstance(id, newLoadBalancer, loadBalancer.get().instance().get().cloudAccount(), settings); } /** Provision or reconfigure a load balancer instance, if necessary */ private Optional provisionInstance(LoadBalancerId id, NodeList nodes, Optional currentLoadBalancer, ZoneEndpoint zoneEndpoint, - CloudAccount cloudAccount) { + CloudAccount cloudAccount, + boolean preparing) { boolean shouldDeactivateRouting = deactivateRouting.with(FetchVector.Dimension.APPLICATION_ID, id.application().serializedForm()) .value(); @@ -239,15 +250,15 @@ public class LoadBalancerProvisioner { } else { reals = realsOf(nodes); } - if (isUpToDate(currentLoadBalancer, reals, zoneEndpoint)) + if (isUpToDate(currentLoadBalancer, reals, zoneEndpoint, preparing)) return currentLoadBalancer.get().instance(); log.log(Level.INFO, () -> "Provisioning instance for " + id + ", targeting: " + reals); try { // Override settings at activation, otherwise keep existing ones. - ZoneEndpoint settings = zoneEndpoint != null ? zoneEndpoint - : currentLoadBalancer.flatMap(LoadBalancer::instance) - .map(LoadBalancerInstance::settings) - .orElse(null); + ZoneEndpoint settings = ! preparing ? zoneEndpoint + : currentLoadBalancer.flatMap(LoadBalancer::instance) + .map(LoadBalancerInstance::settings) + .orElse(zoneEndpoint); LoadBalancerInstance created = service.create(new LoadBalancerSpec(id.application(), id.cluster(), reals, settings, cloudAccount), shouldDeactivateRouting || allowEmptyReals(currentLoadBalancer)); if (created.serviceId().isEmpty() && currentLoadBalancer.flatMap(LoadBalancer::instance).flatMap(LoadBalancerInstance::serviceId).isPresent()) @@ -307,12 +318,12 @@ public class LoadBalancerProvisioner { return loadBalancer.instance().isEmpty() || loadBalancer.instance().get().cloudAccount().equals(cloudAccount); } - /** Returns whether load balancer has given reals, and settings if specified*/ - private static boolean isUpToDate(Optional loadBalancer, Set reals, ZoneEndpoint zoneEndpoint) { + /** Returns whether load balancer has given reals, and settings if specified */ + private static boolean isUpToDate(Optional loadBalancer, Set reals, ZoneEndpoint zoneEndpoint, boolean preparing) { if (loadBalancer.isEmpty()) return false; if (loadBalancer.get().instance().isEmpty()) return false; return loadBalancer.get().instance().get().reals().equals(reals) - && (zoneEndpoint == null || loadBalancer.get().instance().get().settings().equals(zoneEndpoint)); + && (preparing || loadBalancer.get().instance().get().settings().equals(zoneEndpoint)); } /** Returns whether to allow given load balancer to have no reals */ @@ -331,14 +342,17 @@ public class LoadBalancerProvisioner { return reachable; } - private static void requireInstance(LoadBalancerId id, Optional instance, CloudAccount cloudAccount) { - if (instance.isEmpty()) { + private static void requireInstance(LoadBalancerId id, LoadBalancer loadBalancer, CloudAccount cloudAccount, ZoneEndpoint zoneEndpoint) { + if (loadBalancer.instance().isEmpty()) { // Signal that load balancer is not ready yet throw new LoadBalancerServiceException("Could not (re)configure " + id + ". The operation will be retried on next deployment"); } - if (!instance.get().cloudAccount().equals(cloudAccount)) { + if (!inAccount(cloudAccount, loadBalancer)) { throw new LoadBalancerServiceException("Could not (re)configure " + id + " due to change in cloud account. The operation will be retried on next deployment"); } + if (!hasCorrectVisibility(loadBalancer, zoneEndpoint)) { + throw new LoadBalancerServiceException("Could not (re)configure " + id + " due to change in load balancer visibility. The operation will be retried on next deployment"); + } } private static ClusterSpec.Id effectiveId(ClusterSpec cluster) { 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 9ba8c4d2d75..0aa2a001bfa 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 @@ -43,6 +43,7 @@ import java.util.stream.Collectors; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -333,6 +334,39 @@ public class LoadBalancerProvisionerTest { assertEquals(settings, loadBalancers.first().get().instance().get().settings()); } + @Test + public void load_balancer_with_changing_visibility() { + ClusterResources resources = new ClusterResources(3, 1, nodeResources); + Capacity capacity = Capacity.from(resources, resources, IntRange.empty(), false, true, Optional.of(CloudAccount.empty)); + tester.activate(app1, prepare(app1, capacity, clusterRequest(ClusterSpec.Type.container, ClusterSpec.Id.from("c1")))); + LoadBalancerList loadBalancers = tester.nodeRepository().loadBalancers().list(); + assertEquals(1, loadBalancers.size()); + assertEquals(ZoneEndpoint.defaultEndpoint, loadBalancers.first().get().instance().get().settings()); + + // Next deployment has only a private endpoint + ZoneEndpoint settings = new ZoneEndpoint(false, true, List.of(new AllowedUrn(AccessType.awsPrivateLink, "alice"), new AllowedUrn(AccessType.gcpServiceConnect, "bob"))); + assertEquals("Could not (re)configure load balancer tenant1:application1:default:c1 due to change in load balancer visibility. The operation will be retried on next deployment", + assertThrows(LoadBalancerServiceException.class, + () -> prepare(app1, capacity, clusterRequest(ClusterSpec.Type.container, ClusterSpec.Id.from("c1"), Optional.empty(), settings))) + .getMessage()); + + // Existing LB is removed + loadBalancers = tester.nodeRepository().loadBalancers().list(); + assertEquals(1, loadBalancers.size()); + assertSame(LoadBalancer.State.removable, loadBalancers.first().get().state()); + new LoadBalancerExpirer(tester.nodeRepository(), + Duration.ofDays(1), + tester.loadBalancerService(), + new TestMetric()) + .run(); + assertEquals(0, tester.nodeRepository().loadBalancers().list().in(LoadBalancer.State.removable).size()); + + // Next deployment provisions a new LB + tester.activate(app1, prepare(app1, capacity, clusterRequest(ClusterSpec.Type.container, ClusterSpec.Id.from("c1"), Optional.empty(), settings))); + loadBalancers = tester.nodeRepository().loadBalancers().list(); + assertEquals(1, loadBalancers.size()); + assertEquals(settings, loadBalancers.first().get().instance().get().settings()); + } @Test public void load_balancer_with_custom_cloud_account() { -- cgit v1.2.3