From 1770e53c93134b268f0fac6239bc84b8f15688c4 Mon Sep 17 00:00:00 2001 From: jonmv Date: Fri, 27 Jan 2023 18:02:53 +0100 Subject: Revert "Merge pull request #25773 from vespa-engine/jonmv/private-endpoints-2" This reverts commit 414aaf3e1e478deadf199488887dc0d9da0881ab, reversing changes made to a3ae8f5b0ec3a7f2f3c9205289470dbb89e477ff. --- .../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, 25 insertions(+), 75 deletions(-) (limited to 'node-repository') 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 e74e3068674..c19aebcda6e 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(), - spec.settings().isPrivateEndpoint() ? Optional.of(PrivateServiceId.of("service")) : Optional.empty(), + spec.settings().orElse(ZoneEndpoint.defaultEndpoint), + spec.settings().map(__ -> PrivateServiceId.of("service")), 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 6be8bd71553..e0ef6739542 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 ZoneEndpoint settings; + private final Optional 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 = Objects.requireNonNull(settings); + this.settings = Optional.ofNullable(settings); this.cloudAccount = Objects.requireNonNull(cloudAccount); } @@ -49,7 +49,7 @@ public class LoadBalancerSpec { } /** Static user-configured settings for this load balancer. */ - public ZoneEndpoint settings() { + public Optional 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 fa672c0fd1e..0722bb8e1dc 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,15 +29,13 @@ 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(), + spec.settings().orElse(ZoneEndpoint.defaultEndpoint), 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 fc03ddcc3b0..92fdb1d2e52 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. Its state will remain unchanged. + * nodes. It's 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, cluster.zoneEndpoint(), requestedNodes.cloudAccount()); + prepare(loadBalancerId, nodes, requestedNodes.cloudAccount()); } } @@ -189,10 +189,10 @@ public class LoadBalancerProvisioner { return loadBalancerId; } - private void prepare(LoadBalancerId id, NodeList nodes, ZoneEndpoint zoneEndpoint, CloudAccount cloudAccount) { + private void prepare(LoadBalancerId id, NodeList nodes, CloudAccount cloudAccount) { Instant now = nodeRepository.clock().instant(); Optional loadBalancer = db.readLoadBalancer(id); - Optional instance = provisionInstance(id, nodes, loadBalancer, zoneEndpoint, cloudAccount, true); + Optional instance = provisionInstance(id, nodes, loadBalancer, null, cloudAccount); LoadBalancer newLoadBalancer; LoadBalancer.State fromState = null; if (loadBalancer.isEmpty()) { @@ -206,19 +206,9 @@ 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, newLoadBalancer, cloudAccount, zoneEndpoint); - } - - private static boolean hasCorrectVisibility(LoadBalancer newLoadBalancer, ZoneEndpoint zoneEndpoint) { - return newLoadBalancer.instance().isEmpty() - || newLoadBalancer.instance().get().settings().isPublicEndpoint() == zoneEndpoint.isPublicEndpoint(); + requireInstance(id, instance, cloudAccount); } private void activate(ApplicationTransaction transaction, ClusterSpec.Id cluster, ZoneEndpoint settings, NodeList nodes) { @@ -228,19 +218,18 @@ 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(), false); + Optional instance = provisionInstance(id, nodes, loadBalancer, settings, loadBalancer.get().instance().get().cloudAccount()); 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, newLoadBalancer, loadBalancer.get().instance().get().cloudAccount(), settings); + requireInstance(id, instance, loadBalancer.get().instance().get().cloudAccount()); } /** Provision or reconfigure a load balancer instance, if necessary */ private Optional provisionInstance(LoadBalancerId id, NodeList nodes, Optional currentLoadBalancer, ZoneEndpoint zoneEndpoint, - CloudAccount cloudAccount, - boolean preparing) { + CloudAccount cloudAccount) { boolean shouldDeactivateRouting = deactivateRouting.with(FetchVector.Dimension.APPLICATION_ID, id.application().serializedForm()) .value(); @@ -250,15 +239,15 @@ public class LoadBalancerProvisioner { } else { reals = realsOf(nodes); } - if (isUpToDate(currentLoadBalancer, reals, zoneEndpoint, preparing)) + if (isUpToDate(currentLoadBalancer, reals, zoneEndpoint)) 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 = ! preparing ? zoneEndpoint - : currentLoadBalancer.flatMap(LoadBalancer::instance) - .map(LoadBalancerInstance::settings) - .orElse(zoneEndpoint); + ZoneEndpoint settings = zoneEndpoint != null ? zoneEndpoint + : currentLoadBalancer.flatMap(LoadBalancer::instance) + .map(LoadBalancerInstance::settings) + .orElse(null); 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()) @@ -318,12 +307,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, boolean preparing) { + /** Returns whether load balancer has given reals, and settings if specified*/ + private static boolean isUpToDate(Optional loadBalancer, Set reals, ZoneEndpoint zoneEndpoint) { if (loadBalancer.isEmpty()) return false; if (loadBalancer.get().instance().isEmpty()) return false; return loadBalancer.get().instance().get().reals().equals(reals) - && (preparing || loadBalancer.get().instance().get().settings().equals(zoneEndpoint)); + && (zoneEndpoint == null || loadBalancer.get().instance().get().settings().equals(zoneEndpoint)); } /** Returns whether to allow given load balancer to have no reals */ @@ -342,17 +331,14 @@ public class LoadBalancerProvisioner { return reachable; } - private static void requireInstance(LoadBalancerId id, LoadBalancer loadBalancer, CloudAccount cloudAccount, ZoneEndpoint zoneEndpoint) { - if (loadBalancer.instance().isEmpty()) { + private static void requireInstance(LoadBalancerId id, Optional instance, CloudAccount cloudAccount) { + if (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 (!inAccount(cloudAccount, loadBalancer)) { + if (!instance.get().cloudAccount().equals(cloudAccount)) { 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 0aa2a001bfa..9ba8c4d2d75 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,7 +43,6 @@ 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; @@ -334,39 +333,6 @@ 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