Merge pull request #28643 from vespa-engine/revert-28627-revert-28618-hmusum/validate-s3-urls-in-config-and-exclusive-cluster-nodes

Reapply "Validate use of s3 urls in config"

6 files changed, 189 insertions, 4 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/UrlConfigValidator.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/UrlConfigValidator.java
new file mode 100644
index 00000000000..d9dd3729bd3
--- /dev/null
+++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/UrlConfigValidator.java
@@ -0,0 +1,50 @@
+// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.model.application.validation;
+
+import com.yahoo.config.model.deploy.DeployState;
+import com.yahoo.vespa.model.VespaModel;
+import com.yahoo.vespa.model.container.ApplicationContainerCluster;
+
+/**
+ * Validates that config using s3:// urls is used in public system and with nodes that are exclusive.
+ *
+ * @author hmusum
+ */
+public class UrlConfigValidator extends Validator {
+
+    @Override
+    public void validate(VespaModel model, DeployState state) {
+        if (! state.isHostedTenantApplication(model.getAdmin().getApplicationType())) return;
+
+        model.getContainerClusters().forEach((__, cluster) -> {
+            var isExclusive = hasExclusiveNodes(model, cluster);
+            validateS3UlsInConfig(state, cluster, isExclusive);
+        });
+    }
+
+    private static boolean hasExclusiveNodes(VespaModel model, ApplicationContainerCluster cluster) {
+        return model.hostSystem().getHosts()
+                .stream()
+                .flatMap(hostResource -> hostResource.spec().membership().stream())
+                .filter(membership -> membership.cluster().id().equals(cluster.id()))
+                .anyMatch(membership -> membership.cluster().isExclusive());
+    }
+
+    private static void validateS3UlsInConfig(DeployState state, ApplicationContainerCluster cluster, boolean isExclusive) {
+        if (hasS3UrlInConfig(cluster)) {
+            // TODO: Would be even better if we could add which config/field the url is set for in the error message
+            String message = "Found s3:// urls in config for container cluster " + cluster.getName();
+            if ( ! state.zone().system().isPublic())
+                throw new IllegalArgumentException(message + ". This is only supported in public systems");
+            else if ( ! isExclusive)
+                throw new IllegalArgumentException(message + ". Nodes in the cluster need to be 'exclusive'," +
+                                                           " see https://cloud.vespa.ai/en/reference/services#nodes");
+        }
+    }
+
+    private static boolean hasS3UrlInConfig(ApplicationContainerCluster cluster) {
+        return cluster.userConfiguredUrls().all().stream()
+                .anyMatch(url -> url.startsWith("s3://"));
+    }
+
+}
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java
index b9ecf7c2d22..30aafe67be7 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java
@@ -87,6 +87,7 @@ public class Validation {
         new AccessControlFilterExcludeValidator().validate(model, deployState);
         new CloudUserFilterValidator().validate(model, deployState);
         new CloudHttpConnectorValidator().validate(model, deployState);
+        new UrlConfigValidator().validate(model, deployState);
         new JvmHeapSizeValidator().validate(model, deployState);
 
         additionalValidators.forEach(v -> v.validate(model, deployState));
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/ApplicationContainerCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/container/ApplicationContainerCluster.java
index da6e3387d6a..0945dfaf54a 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/ApplicationContainerCluster.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/ApplicationContainerCluster.java
@@ -40,9 +40,11 @@ import com.yahoo.vespa.model.container.component.SystemBindingPattern;
 import com.yahoo.vespa.model.container.configserver.ConfigserverCluster;
 import com.yahoo.vespa.model.filedistribution.UserConfiguredFiles;
 
+import java.net.URL;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Optional;
@@ -104,6 +106,8 @@ public final class ApplicationContainerCluster extends ContainerCluster<Applicat
 
     private List<ApplicationClusterEndpoint> endpoints = List.of();
 
+    private UserConfiguredUrls userConfiguredUrls = new UserConfiguredUrls();
+
     public ApplicationContainerCluster(TreeConfigProducer<?> parent, String configSubId, String clusterId, DeployState deployState) {
         super(parent, configSubId, clusterId, deployState, true, 10);
         this.tlsClientAuthority = deployState.tlsClientAuthority();
@@ -134,6 +138,8 @@ public final class ApplicationContainerCluster extends ContainerCluster<Applicat
         logger = deployState.getDeployLogger();
     }
 
+    public UserConfiguredUrls userConfiguredUrls() { return userConfiguredUrls; }
+
     @Override
     protected void doPrepare(DeployState deployState) {
         super.doPrepare(deployState);
@@ -154,7 +160,9 @@ public final class ApplicationContainerCluster extends ContainerCluster<Applicat
         if (containers.isEmpty()) return;
 
         // Files referenced from user configs to all components.
-        UserConfiguredFiles files = new UserConfiguredFiles(deployState.getFileRegistry(), deployState.getDeployLogger());
+        UserConfiguredFiles files = new UserConfiguredFiles(deployState.getFileRegistry(),
+                                                            deployState.getDeployLogger(),
+                                                            userConfiguredUrls);
         for (Component<?, ?> component : getAllComponents()) {
             files.register(component);
         }
@@ -382,4 +390,14 @@ public final class ApplicationContainerCluster extends ContainerCluster<Applicat
         }
     }
 
+    public static class UserConfiguredUrls {
+
+        private final Set<String> urls = new HashSet<>();
+
+        public void add(String url) { urls.add(url); }
+
+        public Set<String> all() { return urls; }
+
+    }
+
 }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/filedistribution/UserConfiguredFiles.java b/config-model/src/main/java/com/yahoo/vespa/model/filedistribution/UserConfiguredFiles.java
index 8bed5e64bf5..03541ecadf3 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/filedistribution/UserConfiguredFiles.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/filedistribution/UserConfiguredFiles.java
@@ -11,6 +11,7 @@ import com.yahoo.path.Path;
 import com.yahoo.vespa.config.ConfigDefinition;
 import com.yahoo.vespa.config.ConfigDefinitionKey;
 import com.yahoo.vespa.config.ConfigPayloadBuilder;
+
 import com.yahoo.yolean.Exceptions;
 
 import java.io.File;
@@ -21,6 +22,8 @@ import java.util.Map;
 import java.util.Optional;
 import java.util.logging.Level;
 
+import static com.yahoo.vespa.model.container.ApplicationContainerCluster.UserConfiguredUrls;
+
 /**
  * Utility methods for registering file distribution of files/paths/urls/models defined by the user.
  *
@@ -30,10 +33,12 @@ public class UserConfiguredFiles implements Serializable {
 
     private final FileRegistry fileRegistry;
     private final DeployLogger logger;
+    private final UserConfiguredUrls userConfiguredUrls;
 
-    public UserConfiguredFiles(FileRegistry fileRegistry, DeployLogger logger) {
+    public UserConfiguredFiles(FileRegistry fileRegistry, DeployLogger logger, UserConfiguredUrls userConfiguredUrls) {
         this.fileRegistry = fileRegistry;
         this.logger = logger;
+        this.userConfiguredUrls = userConfiguredUrls;
     }
 
     /**
@@ -133,7 +138,10 @@ public class UserConfiguredFiles implements Serializable {
         Path path;
         if (isModelType) {
             var modelReference = ModelReference.valueOf(builder.getValue());
-            if (modelReference.path().isEmpty()) return;
+            if (modelReference.path().isEmpty()) {
+                modelReference.url().ifPresent(url -> userConfiguredUrls.add(url.value()));
+                return;
+            }
             path = Path.fromString(modelReference.path().get().value());
         }
         else {
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/UrlConfigValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/UrlConfigValidatorTest.java
new file mode 100644
index 00000000000..cef4d8c27dd
--- /dev/null
+++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/UrlConfigValidatorTest.java
@@ -0,0 +1,107 @@
+package com.yahoo.vespa.model.application.validation;
+
+import com.yahoo.config.application.api.ApplicationPackage;
+import com.yahoo.config.model.NullConfigModelRegistry;
+import com.yahoo.config.model.api.ConfigDefinitionRepo;
+import com.yahoo.config.model.application.provider.MockFileRegistry;
+import com.yahoo.config.model.deploy.DeployState;
+import com.yahoo.config.model.deploy.TestProperties;
+import com.yahoo.config.model.test.MockApplicationPackage;
+import com.yahoo.config.provision.RegionName;
+import com.yahoo.config.provision.SystemName;
+import com.yahoo.config.provision.Zone;
+import com.yahoo.embedding.BertBaseEmbedderConfig;
+import com.yahoo.vespa.config.ConfigDefinitionKey;
+import com.yahoo.vespa.config.buildergen.ConfigDefinition;
+import com.yahoo.vespa.model.VespaModel;
+import org.junit.jupiter.api.Test;
+import org.xml.sax.SAXException;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
+import static com.yahoo.config.provision.Environment.prod;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+public class UrlConfigValidatorTest {
+
+    @Test
+    void failsWhenContainerNodesNotExclusive() throws IOException, SAXException {
+        runValidatorOnApp(true, SystemName.Public);  // Exclusive nodes in public => success
+
+        assertEquals("Found s3:// urls in config for container cluster default. This is only supported in public systems",
+                     assertThrows(IllegalArgumentException.class,
+                                  () -> runValidatorOnApp(false, SystemName.main))
+                             .getMessage());
+
+        assertEquals("Found s3:// urls in config for container cluster default. This is only supported in public systems",
+                     assertThrows(IllegalArgumentException.class,
+                                  () -> runValidatorOnApp(true, SystemName.main))
+                             .getMessage());
+
+        assertEquals("Found s3:// urls in config for container cluster default. Nodes in the cluster need to be 'exclusive',"
+                     + " see https://cloud.vespa.ai/en/reference/services#nodes",
+                     assertThrows(IllegalArgumentException.class,
+                                  () -> runValidatorOnApp(false, SystemName.Public))
+                             .getMessage());
+    }
+
+    private static String containerXml(boolean isExclusive) {
+        return """
+                           <container version='1.0' id='default'>
+                               <component id='transformer' class='ai.vespa.embedding.BertBaseEmbedder' bundle='model-integration'>
+                                   <config name='embedding.bert-base-embedder'>
+                                     <transformerModel url='s3://models/minilm-l6-v2/sentence_all_MiniLM_L6_v2.onnx' path='foo'/>
+                                     <tokenizerVocab url='s3://models/bert-base-uncased.txt'/>
+                                   </config>
+                               </component>
+                               <search/>
+                               <document-api/>
+                               <nodes count='2' exclusive='%s' />
+                           </container>
+                """.formatted(Boolean.toString(isExclusive));
+    }
+
+    private static void runValidatorOnApp(boolean isExclusive, SystemName systemName) throws IOException, SAXException {
+        String container = containerXml(isExclusive);
+        String servicesXml = """
+                        <services version='1.0'>
+                          %s
+                        </services>
+                """.formatted(container);
+        ApplicationPackage app = new MockApplicationPackage.Builder()
+                .withServices(servicesXml)
+                .build();
+        DeployState deployState = createDeployState(app, systemName);
+        VespaModel model = new VespaModel(new NullConfigModelRegistry(), deployState);
+        new UrlConfigValidator().validate(model, deployState);
+    }
+
+    private static DeployState createDeployState(ApplicationPackage app, SystemName systemName) {
+        boolean isHosted = true;
+        var builder = new DeployState.Builder()
+                .applicationPackage(app)
+                .zone(new Zone(systemName, prod, RegionName.from("us-east-3")))
+                .properties(new TestProperties().setHostedVespa(isHosted))
+                .fileRegistry(new MockFileRegistry());
+
+        Map<ConfigDefinitionKey, ConfigDefinition> defs = new HashMap<>();
+        defs.put(new ConfigDefinitionKey(BertBaseEmbedderConfig.CONFIG_DEF_NAME, BertBaseEmbedderConfig.CONFIG_DEF_NAMESPACE),
+                 new ConfigDefinition(BertBaseEmbedderConfig.CONFIG_DEF_NAME, BertBaseEmbedderConfig.CONFIG_DEF_SCHEMA));
+        builder.configDefinitionRepo(new ConfigDefinitionRepo() {
+            @Override
+            public Map<ConfigDefinitionKey, com.yahoo.vespa.config.buildergen.ConfigDefinition> getConfigDefinitions() {
+                return defs;
+            }
+
+            @Override
+            public com.yahoo.vespa.config.buildergen.ConfigDefinition get(ConfigDefinitionKey key) {
+                return defs.get(key);
+            }
+        });
+        return builder.build();
+    }
+
+}
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/filedistribution/UserConfiguredFilesTest.java b/config-model/src/test/java/com/yahoo/vespa/model/filedistribution/UserConfiguredFilesTest.java
index bb5ba840c2c..e2a25642575 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/filedistribution/UserConfiguredFilesTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/filedistribution/UserConfiguredFilesTest.java
@@ -13,6 +13,7 @@ import com.yahoo.vespa.config.ConfigDefinition;
 import com.yahoo.vespa.config.ConfigDefinitionKey;
 import com.yahoo.vespa.config.ConfigPayloadBuilder;
 import com.yahoo.vespa.model.SimpleConfigProducer;
+import com.yahoo.vespa.model.container.ApplicationContainerCluster;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
@@ -68,7 +69,7 @@ public class UserConfiguredFilesTest {
     }
 
     private UserConfiguredFiles userConfiguredFiles() {
-        return new UserConfiguredFiles(fileRegistry, new BaseDeployLogger());
+        return new UserConfiguredFiles(fileRegistry, new BaseDeployLogger(), new ApplicationContainerCluster.UserConfiguredUrls());
     }
 
     @BeforeEach