diff options
author | Harald Musum <musum@yahooinc.com> | 2023-09-25 14:40:55 +0200 |
---|---|---|
committer | Harald Musum <musum@yahooinc.com> | 2023-09-25 14:40:55 +0200 |
commit | 7c37396177a488ebed5cef7694aa996783e257ff (patch) | |
tree | 39bf4a2f64437350be00b31ac176b2610227382f /config-model | |
parent | 8b80c2bb7ba62960dfeebed27d4cc08f9475234d (diff) | |
parent | 7facdd6177063f772c497000b9c12e4653a2db83 (diff) |
Merge branch 'master' into hmusum/add-feature-flag-3
Diffstat (limited to 'config-model')
7 files changed, 285 insertions, 30 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 a4ed187206d..8d9ef84ef71 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 @@ -11,11 +11,13 @@ import com.yahoo.config.application.api.ComponentInfo; import com.yahoo.config.application.api.DeployLogger; import com.yahoo.config.model.api.ApplicationClusterEndpoint; import com.yahoo.config.model.api.ApplicationClusterInfo; +import com.yahoo.config.model.api.ContainerEndpoint; import com.yahoo.config.model.api.Model; import com.yahoo.config.model.api.OnnxModelCost; import com.yahoo.config.model.deploy.DeployState; import com.yahoo.config.model.producer.TreeConfigProducer; import com.yahoo.config.provision.AllocatedHosts; +import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.HostSpec; import com.yahoo.container.bundle.BundleInstantiationSpecification; import com.yahoo.container.di.config.ApplicationBundlesConfig; @@ -43,6 +45,7 @@ import com.yahoo.vespa.model.filedistribution.UserConfiguredFiles; 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 +107,8 @@ public final class ApplicationContainerCluster extends ContainerCluster<Applicat private List<ApplicationClusterEndpoint> endpoints = List.of(); + private final 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 +139,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); @@ -156,7 +163,8 @@ public final class ApplicationContainerCluster extends ContainerCluster<Applicat // Files referenced from user configs to all components. UserConfiguredFiles files = new UserConfiguredFiles(deployState.getFileRegistry(), deployState.getDeployLogger(), - deployState.featureFlags()); + deployState.featureFlags(), + userConfiguredUrls); for (Component<?, ?> component : getAllComponents()) { files.register(component); } @@ -218,23 +226,49 @@ public final class ApplicationContainerCluster extends ContainerCluster<Applicat private void createEndpoints(DeployState deployState) { if (!deployState.isHosted()) return; if (deployState.getProperties().applicationId().instance().isTester()) return; - // Add endpoints provided by the controller - List<String> hosts = getContainers().stream().map(AbstractService::getHostName).sorted().toList(); List<ApplicationClusterEndpoint> endpoints = new ArrayList<>(); - deployState.getEndpoints().stream() - .filter(ce -> ce.clusterId().equals(getName())) - .forEach(ce -> ce.names().forEach( - name -> endpoints.add(ApplicationClusterEndpoint.builder() - .scope(ce.scope()) - .weight(ce.weight().orElse(1)) - .routingMethod(ce.routingMethod()) - .dnsName(ApplicationClusterEndpoint.DnsName.from(name)) - .hosts(hosts) - .clusterId(getName()) - .authMethod(ce.authMethod()) - .build()) - )); - this.endpoints = Collections.unmodifiableList(endpoints); + + List<String> hosts = getContainers().stream() + .map(AbstractService::getHostName) + .sorted() + .toList(); + + Set<ContainerEndpoint> endpointsFromController = deployState.getEndpoints(); + // Add zone-scoped endpoints if not provided by the controller + // TODO(mpolden): Remove this when controller always includes zone-scope endpoints, and config models < 8.230 are gone + if (endpointsFromController.stream().noneMatch(endpoint -> endpoint.scope() == ApplicationClusterEndpoint.Scope.zone)) { + for (String suffix : deployState.getProperties().zoneDnsSuffixes()) { + ApplicationClusterEndpoint.DnsName l4Name = ApplicationClusterEndpoint.DnsName.sharedL4NameFrom( + deployState.zone().system(), + ClusterSpec.Id.from(getName()), + deployState.getProperties().applicationId(), + suffix); + endpoints.add(ApplicationClusterEndpoint.builder() + .zoneScope() + .sharedL4Routing() + .dnsName(l4Name) + .hosts(hosts) + .clusterId(getName()) + .authMethod(ApplicationClusterEndpoint.AuthMethod.mtls) + .build()); + } + } + + // Include all endpoints provided by controller + endpointsFromController.stream() + .filter(ce -> ce.clusterId().equals(getName())) + .forEach(ce -> ce.names().forEach( + name -> endpoints.add(ApplicationClusterEndpoint.builder() + .scope(ce.scope()) + .weight(ce.weight().orElse(1)) // Default to weight=1 if not set + .routingMethod(ce.routingMethod()) + .dnsName(ApplicationClusterEndpoint.DnsName.from(name)) + .hosts(hosts) + .clusterId(getName()) + .authMethod(ce.authMethod()) + .build()) + )); + this.endpoints = List.copyOf(endpoints); } @Override @@ -384,4 +418,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 60afb491be2..0095dec8079 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 @@ -12,6 +12,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; @@ -22,6 +23,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. * @@ -31,11 +34,15 @@ public class UserConfiguredFiles implements Serializable { private final FileRegistry fileRegistry; private final DeployLogger logger; + private final UserConfiguredUrls userConfiguredUrls; private final String unknownConfigDefinition; - public UserConfiguredFiles(FileRegistry fileRegistry, DeployLogger logger, ModelContext.FeatureFlags featureFlags) { + public UserConfiguredFiles(FileRegistry fileRegistry, DeployLogger logger, + ModelContext.FeatureFlags featureFlags, + UserConfiguredUrls userConfiguredUrls) { this.fileRegistry = fileRegistry; this.logger = logger; + this.userConfiguredUrls = userConfiguredUrls; this.unknownConfigDefinition = featureFlags.unknownConfigDefinition(); } @@ -139,7 +146,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/container/ContainerClusterTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java index 3bdb60a0a8d..894fc55c014 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java @@ -42,14 +42,14 @@ import java.util.Objects; import java.util.OptionalInt; import java.util.Set; +import static com.yahoo.config.model.api.ApplicationClusterEndpoint.RoutingMethod.exclusive; +import static com.yahoo.config.model.api.ApplicationClusterEndpoint.RoutingMethod.shared; import static com.yahoo.config.model.api.ApplicationClusterEndpoint.RoutingMethod.sharedLayer4; import static com.yahoo.config.model.api.ApplicationClusterEndpoint.Scope.application; import static com.yahoo.config.model.api.ApplicationClusterEndpoint.Scope.global; -import static com.yahoo.config.model.api.ApplicationClusterEndpoint.Scope.zone; +import static com.yahoo.config.provision.SystemName.cd; import static com.yahoo.config.provision.SystemName.main; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.*; /** * @author Simon Thoresen Hult @@ -365,23 +365,62 @@ public class ContainerClusterTest { @Test void generatesCorrectRoutingInfo() { + // main system: assertNames(main, ApplicationId.from("t1", "a1", "i1"), - Set.of(new ContainerEndpoint("search-cluster", zone, List.of("search-cluster.i1.a1.t1.endpoint.suffix"), OptionalInt.empty(), sharedLayer4)), + Set.of(), List.of("search-cluster.i1.a1.t1.endpoint.suffix")); assertNames(main, ApplicationId.from("t1", "a1", "default"), - Set.of(new ContainerEndpoint("not-in-this-cluster", global, List.of("foo", "bar")), - new ContainerEndpoint("search-cluster", zone, List.of("search-cluster.a1.t1.endpoint.suffix"), OptionalInt.empty(), sharedLayer4)), + Set.of(), + List.of("search-cluster.a1.t1.endpoint.suffix")); + + assertNames(main, + ApplicationId.from("t1", "default", "default"), + Set.of(), + List.of("search-cluster.default.t1.endpoint.suffix")); + + assertNames(main, + ApplicationId.from("t1", "a1", "default"), + Set.of(new ContainerEndpoint("not-in-this-cluster", global, List.of("foo", "bar"))), List.of("search-cluster.a1.t1.endpoint.suffix")); assertNames(main, ApplicationId.from("t1", "a1", "default"), - Set.of(new ContainerEndpoint("search-cluster", global, List.of("rotation-1.x.y.z", "rotation-2.x.y.z"), OptionalInt.empty(), sharedLayer4), - new ContainerEndpoint("search-cluster", application, List.of("app-rotation.x.y.z"), OptionalInt.of(3), sharedLayer4), - new ContainerEndpoint("search-cluster", zone, List.of("search-cluster.a1.t1.endpoint.suffix"), OptionalInt.empty(), sharedLayer4)), + Set.of(new ContainerEndpoint("search-cluster", global, List.of("rotation-1.x.y.z", "rotation-2.x.y.z"), OptionalInt.empty(), sharedLayer4), + new ContainerEndpoint("search-cluster", application, List.of("app-rotation.x.y.z"), OptionalInt.of(3), sharedLayer4)), List.of("search-cluster.a1.t1.endpoint.suffix", "rotation-1.x.y.z", "rotation-2.x.y.z", "app-rotation.x.y.z")); + + // cd system: + assertNames(cd, + ApplicationId.from("t1", "a1", "i1"), + Set.of(), + List.of("search-cluster.cd.i1.a1.t1.endpoint.suffix")); + + assertNames(cd, + ApplicationId.from("t1", "a1", "default"), + Set.of(), + List.of("search-cluster.cd.a1.t1.endpoint.suffix")); + + assertNames(cd, + ApplicationId.from("t1", "default", "default"), + Set.of(), + List.of("search-cluster.cd.default.t1.endpoint.suffix")); + + assertNames(cd, + ApplicationId.from("t1", "a1", "default"), + Set.of(new ContainerEndpoint("not-in-this-cluster", global, List.of("foo", "bar"))), + List.of("search-cluster.cd.a1.t1.endpoint.suffix")); + + assertNames(cd, + ApplicationId.from("t1", "a1", "default"), + Set.of(new ContainerEndpoint("search-cluster", global, List.of("rotation-1.x.y.z", "rotation-2.x.y.z"), OptionalInt.empty(), sharedLayer4), + new ContainerEndpoint("search-cluster", global, List.of("a.b.x.y.z", "rotation-2.x.y.z"), OptionalInt.empty(), shared), + new ContainerEndpoint("search-cluster", application, List.of("app-rotation.x.y.z"), OptionalInt.of(3), sharedLayer4), + new ContainerEndpoint("not-supported", global, List.of("not.supported"), OptionalInt.empty(), exclusive)), + List.of("search-cluster.cd.a1.t1.endpoint.suffix", "rotation-1.x.y.z", "rotation-2.x.y.z", "app-rotation.x.y.z")); + } private void assertNames(SystemName systemName, ApplicationId appId, Set<ContainerEndpoint> globalEndpoints, List<String> expectedSharedL4Names) { 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 7dde969b8b3..653bdbccf15 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 @@ -14,6 +14,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; @@ -69,7 +70,10 @@ public class UserConfiguredFilesTest { } private UserConfiguredFiles userConfiguredFiles() { - return new UserConfiguredFiles(fileRegistry, new BaseDeployLogger(), new TestProperties()); + return new UserConfiguredFiles(fileRegistry, + new BaseDeployLogger(), + new TestProperties(), + new ApplicationContainerCluster.UserConfiguredUrls()); } @BeforeEach |