Revert "Merge pull request #25773 from vespa-engine/jonmv/private-endpoints-2"

This reverts commit 414aaf3e1e478deadf199488887dc0d9da0881ab, reversing
changes made to a3ae8f5b0ec3a7f2f3c9205289470dbb89e477ff.

5 files changed, 25 insertions, 75 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 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<Real> reals;
-    private final ZoneEndpoint settings;
+    private final Optional<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 = 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<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 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.
      * <p>
      * 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.
      * <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, 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> loadBalancer = db.readLoadBalancer(id);
-        Optional<LoadBalancerInstance> instance = provisionInstance(id, nodes, loadBalancer, zoneEndpoint, cloudAccount, true);
+        Optional<LoadBalancerInstance> 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<LoadBalancerInstance> instance = provisionInstance(id, nodes, loadBalancer, settings, loadBalancer.get().instance().get().cloudAccount(), false);
+        Optional<LoadBalancerInstance> 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<LoadBalancerInstance> provisionInstance(LoadBalancerId id, NodeList nodes,
                                                              Optional<LoadBalancer> 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> loadBalancer, Set<Real> reals, ZoneEndpoint zoneEndpoint, boolean preparing) {
+    /** Returns whether load balancer has given reals, and settings if specified*/
+    private static boolean isUpToDate(Optional<LoadBalancer> loadBalancer, Set<Real> 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<LoadBalancerInstance> 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() {