Provision Internal LBs from AWS for non-public endpoints

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<Real> reals;
-    private final Optional<ZoneEndpoint> settings;
+    private final ZoneEndpoint settings;
     private final CloudAccount cloudAccount;
 
     public LoadBalancerSpec(ApplicationId application, ClusterSpec.Id cluster, Set<Real> 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<ZoneEndpoint> 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.
      * <p>
      * 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.
      * <p>
      * If no load balancer exists, a new one will be provisioned in {@link LoadBalancer.State#reserved}.
      * <p>
@@ -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> loadBalancer = db.readLoadBalancer(id);
-        Optional<LoadBalancerInstance> instance = provisionInstance(id, nodes, loadBalancer, null, cloudAccount);
+        Optional<LoadBalancerInstance> 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<LoadBalancerInstance> instance = provisionInstance(id, nodes, loadBalancer, settings, loadBalancer.get().instance().get().cloudAccount());
+        Optional<LoadBalancerInstance> 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<LoadBalancerInstance> provisionInstance(LoadBalancerId id, NodeList nodes,
                                                              Optional<LoadBalancer> 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> loadBalancer, Set<Real> reals, ZoneEndpoint zoneEndpoint) {
+    /** Returns whether load balancer has given reals, and settings if specified */
+    private static boolean isUpToDate(Optional<LoadBalancer> loadBalancer, Set<Real> 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<LoadBalancerInstance> 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() {