From 4b67fbc8d76a9e4137bd080ea0ee0ee5ebd8e6b1 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Thu, 21 Dec 2023 10:15:22 +0100 Subject: [PATCH] Revert "Jonmv/keep config change actions in dev" --- config-model-api/abi-spec.json | 4 +- .../application/api/ValidationOverrides.java | 9 +- .../config/model/deploy/DeployState.java | 2 +- .../yahoo/vespa/model/VespaModelFactory.java | 7 + .../application/validation/Validation.java | 134 ++++++------------ .../search/IndexingScriptChangeValidator.java | 2 +- ...CertificateRemovalChangeValidatorTest.java | 8 +- .../IndexingModeChangeValidatorTest.java | 25 +--- 8 files changed, 63 insertions(+), 128 deletions(-) diff --git a/config-model-api/abi-spec.json b/config-model-api/abi-spec.json index 10c5662678eb..c3e20e534ff9 100644 --- a/config-model-api/abi-spec.json +++ b/config-model-api/abi-spec.json @@ -771,9 +771,7 @@ "attributes" : [ "public" ], - "methods" : [ - "public java.util.Map messagesById()" - ], + "methods" : [ ], "fields" : [ ] }, "com.yahoo.config.application.api.ValidationOverrides" : { diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationOverrides.java b/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationOverrides.java index 7b52d8254735..1edddb63e52f 100644 --- a/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationOverrides.java +++ b/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationOverrides.java @@ -15,7 +15,6 @@ import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; -import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Optional; @@ -37,7 +36,7 @@ public class ValidationOverrides { private final String xmlForm; - /** Creates a validation overrides which does not have an XML form */ + /** Creates a validation overrides which does not have an xml form */ public ValidationOverrides(List overrides) { this(overrides, null); } @@ -164,13 +163,10 @@ public boolean allows(ValidationId validationId, Instant now) { */ public static class ValidationException extends IllegalArgumentException { - private final Map> messagesById = new LinkedHashMap<>(); - static final long serialVersionUID = 789984668; private ValidationException(ValidationId validationId, String message) { super(validationId + ": " + message + ". " + toAllowMessage(validationId)); - messagesById.put(validationId, List.of(message)); } private ValidationException(Map> messagesById) { @@ -179,11 +175,8 @@ private ValidationException(Map> messagesById) String.join("\n\t", messages.getValue()) + "\n" + toAllowMessage(messages.getKey())) .collect(Collectors.joining("\n"))); - messagesById.forEach((id, messages) -> this.messagesById.put(id, List.copyOf(messages))); } - public Map> messagesById() { return Map.copyOf(messagesById); } - } } diff --git a/config-model/src/main/java/com/yahoo/config/model/deploy/DeployState.java b/config-model/src/main/java/com/yahoo/config/model/deploy/DeployState.java index f19341098f40..33dfee58d1a4 100644 --- a/config-model/src/main/java/com/yahoo/config/model/deploy/DeployState.java +++ b/config-model/src/main/java/com/yahoo/config/model/deploy/DeployState.java @@ -172,7 +172,7 @@ public static HostProvisioner getDefaultModelHostProvisioner(ApplicationPackage /** Get the global rank profile registry for this application. */ public final RankProfileRegistry rankProfileRegistry() { return rankProfileRegistry; } - /** Returns the validation overrides of this. This is never null. */ + /** Returns the validation overrides of this. This is never null */ public ValidationOverrides validationOverrides() { return validationOverrides; } @Override diff --git a/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java b/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java index 65572d07cc2e..903f1c06024b 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java @@ -217,6 +217,13 @@ private void validateXML(ApplicationPackage applicationPackage, boolean ignoreVa private List validateModel(VespaModel model, DeployState deployState, ValidationParameters validationParameters) { try { return new Validation(additionalValidators).validate(model, validationParameters, deployState); + } catch (ValidationOverrides.ValidationException e) { + if (deployState.isHosted() && zone.environment().isManuallyDeployed()) + deployState.getDeployLogger().logApplicationPackage(Level.WARNING, + "Auto-overriding validation which would be disallowed in production: " + + Exceptions.toMessageString(e)); + else + rethrowUnlessIgnoreErrors(e, validationParameters.ignoreValidationErrors()); } catch (IllegalArgumentException | TransientException | QuotaExceededException e) { rethrowUnlessIgnoreErrors(e, validationParameters.ignoreValidationErrors()); } catch (Exception e) { 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 b9995d290cc1..562773455159 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 @@ -3,7 +3,6 @@ import com.yahoo.config.application.api.ValidationId; import com.yahoo.config.application.api.ValidationOverrides; -import com.yahoo.config.application.api.ValidationOverrides.ValidationException; import com.yahoo.config.model.api.ConfigChangeAction; import com.yahoo.config.model.api.Model; import com.yahoo.config.model.api.ValidationParameters; @@ -26,19 +25,15 @@ import com.yahoo.vespa.model.application.validation.change.StartupCommandChangeValidator; import com.yahoo.vespa.model.application.validation.change.StreamingSearchClusterChangeValidator; import com.yahoo.vespa.model.application.validation.first.RedundancyValidator; -import com.yahoo.yolean.Exceptions; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; -import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.logging.Level; import java.util.stream.Collectors; import static java.util.stream.Collectors.groupingBy; @@ -56,7 +51,7 @@ public class Validation { public Validation() { this(List.of()); } - /** Create instance taking additional validators (e.g., for cloud applications) */ + /** Create instance taking additional validators (e.g for cloud applications) */ public Validation(List additionalValidators) { this.additionalValidators = additionalValidators; } /** @@ -67,53 +62,52 @@ public class Validation { * @throws ValidationOverrides.ValidationException if the change fails validation */ public List validate(VespaModel model, ValidationParameters validationParameters, DeployState deployState) { - Execution execution = new Execution(model, deployState); if (validationParameters.checkRouting()) { - execution.run(new RoutingValidator()); - execution.run(new RoutingSelectorValidator()); + new RoutingValidator().validate(model, deployState); + new RoutingSelectorValidator().validate(model, deployState); } - execution.run(new SchemasDirValidator()); - execution.run(new BundleValidator()); - execution.run(new PublicApiBundleValidator()); - execution.run(new SearchDataTypeValidator()); - execution.run(new ComplexFieldsWithStructFieldAttributesValidator()); - execution.run(new ComplexFieldsWithStructFieldIndexesValidator()); - execution.run(new StreamingValidator()); - execution.run(new RankSetupValidator(validationParameters.ignoreValidationErrors())); - execution.run(new NoPrefixForIndexes()); - execution.run(new ContainerInCloudValidator()); - execution.run(new DeploymentSpecValidator()); - execution.run(new ValidationOverridesValidator()); - execution.run(new ConstantValidator()); - execution.run(new SecretStoreValidator()); - execution.run(new EndpointCertificateSecretsValidator()); - execution.run(new AccessControlFilterValidator()); - execution.run(new QuotaValidator()); - execution.run(new UriBindingsValidator()); - execution.run(new CloudDataPlaneFilterValidator()); - execution.run(new AccessControlFilterExcludeValidator()); - execution.run(new CloudUserFilterValidator()); - execution.run(new CloudHttpConnectorValidator()); - execution.run(new UrlConfigValidator()); - execution.run(new JvmHeapSizeValidator()); - - additionalValidators.forEach(execution::run); + new SchemasDirValidator().validate(model, deployState); + new BundleValidator().validate(model, deployState); + new PublicApiBundleValidator().validate(model, deployState); + new SearchDataTypeValidator().validate(model, deployState); + new ComplexFieldsWithStructFieldAttributesValidator().validate(model, deployState); + new ComplexFieldsWithStructFieldIndexesValidator().validate(model, deployState); + new StreamingValidator().validate(model, deployState); + new RankSetupValidator(validationParameters.ignoreValidationErrors()).validate(model, deployState); + new NoPrefixForIndexes().validate(model, deployState); + new ContainerInCloudValidator().validate(model, deployState); + new DeploymentSpecValidator().validate(model, deployState); + new ValidationOverridesValidator().validate(model, deployState); + new ConstantValidator().validate(model, deployState); + new SecretStoreValidator().validate(model, deployState); + new EndpointCertificateSecretsValidator().validate(model, deployState); + new AccessControlFilterValidator().validate(model, deployState); + new QuotaValidator().validate(model, deployState); + new UriBindingsValidator().validate(model, deployState); + new CloudDataPlaneFilterValidator().validate(model, deployState); + 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)); List result = Collections.emptyList(); if (deployState.getProperties().isFirstTimeDeployment()) { - validateFirstTimeDeployment(execution); + validateFirstTimeDeployment(model, deployState); } else { Optional currentActiveModel = deployState.getPreviousModel(); if (currentActiveModel.isPresent() && (currentActiveModel.get() instanceof VespaModel)) { - result = validateChanges((VespaModel) currentActiveModel.get(), execution); + result = validateChanges((VespaModel) currentActiveModel.get(), model, deployState); deferConfigChangesForClustersToBeRestarted(result, model); } } - execution.throwIfFailed(); return result; } - private static List validateChanges(VespaModel currentModel, Execution execution) { + private static List validateChanges(VespaModel currentModel, VespaModel nextModel, + DeployState deployState) { ChangeValidator[] validators = new ChangeValidator[] { new IndexingModeChangeValidator(), new GlobalDocumentChangeValidator(), @@ -133,15 +127,20 @@ private static List validateChanges(VespaModel currentModel, new RestartOnDeployForOnnxModelChangesValidator(), }; List actions = Arrays.stream(validators) - .flatMap(v -> v.validate(currentModel, execution.model, execution.deployState).stream()) + .flatMap(v -> v.validate(currentModel, nextModel, deployState).stream()) .toList(); - execution.runChanges(actions); + Map> disallowableActions = actions.stream() + .filter(action -> action.validationId().isPresent()) + .collect(groupingBy(action -> action.validationId().orElseThrow(), + mapping(ConfigChangeAction::getMessage, + toCollection(LinkedHashSet::new)))); + deployState.validationOverrides().invalid(disallowableActions, deployState.now()); return actions; } - private static void validateFirstTimeDeployment(Execution execution) { - execution.run(new RedundancyValidator()); + private static void validateFirstTimeDeployment(VespaModel model, DeployState deployState) { + new RedundancyValidator().validate(model, deployState); } private static void deferConfigChangesForClustersToBeRestarted(List actions, VespaModel model) { @@ -160,53 +159,4 @@ private static void deferConfigChangesForClustersToBeRestarted(List> failures = new LinkedHashMap<>(); - private final VespaModel model; - private final DeployState deployState; - - private Execution(VespaModel model, DeployState deployState) { - this.model = model; - this.deployState = deployState; - } - - private void run(Validator validator) { - try { - validator.validate(model, deployState); - } - catch (ValidationException e) { - e.messagesById().forEach((id, messages) -> failures.computeIfAbsent(id, __ -> new ArrayList<>()).addAll(messages)); - } - } - - private void runChanges(List actions) { - for (ConfigChangeAction action : actions) { - if (action.validationId().isPresent()) run(new Validator() { // Changes without a validation ID are always allowed. - @Override public void validate(VespaModel model, DeployState deployState) { - deployState.validationOverrides().invalid(action.validationId().get(), action.getMessage(), deployState.now()); - } - }); - } - } - - private void throwIfFailed() { - try { - if (failures.size() == 1 && failures.values().iterator().next().size() == 1) // Retain single-form exception message when possible. - deployState.validationOverrides().invalid(failures.keySet().iterator().next(), failures.values().iterator().next().get(0), deployState.now()); - else - deployState.validationOverrides().invalid(failures, deployState.now()); - } - catch (ValidationException e) { - if (deployState.isHosted() && deployState.zone().environment().isManuallyDeployed()) - deployState.getDeployLogger().logApplicationPackage(Level.WARNING, - "Auto-overriding validation which would be disallowed in production: " + - Exceptions.toMessageString(e)); - else throw e; - } - } - - } - } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/IndexingScriptChangeValidator.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/IndexingScriptChangeValidator.java index e2dd3aca0b95..fb07f65b5f45 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/IndexingScriptChangeValidator.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/IndexingScriptChangeValidator.java @@ -40,7 +40,7 @@ public List validate() { String fieldName = nextField.getName(); ImmutableSDField currentField = currentSchema.getConcreteField(fieldName); if (currentField != null) { - validateScripts(currentField, nextField).ifPresent(result::add); + validateScripts(currentField, nextField).ifPresent(r -> result.add(r)); } else if (nextField.isExtraField()) { result.add(VespaReindexAction.of(id, diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java index bc36b800bfb5..6b7df8871aa8 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java @@ -23,11 +23,9 @@ public class CertificateRemovalChangeValidatorTest { private static final String validationOverrides = - """ - - certificate-removal - - """; + "\n" + + " certificate-removal\n" + + "\n"; @Test void validate() { diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/IndexingModeChangeValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/IndexingModeChangeValidatorTest.java index 9e0eab9aba70..3fd3180b37e3 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/IndexingModeChangeValidatorTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/IndexingModeChangeValidatorTest.java @@ -21,16 +21,6 @@ */ public class IndexingModeChangeValidatorTest { - @Test - void testChangingIndexModeFromIndexedToStreamingWhenDisallowedButInDev() { - ValidationTester tester = new ValidationTester(); - - VespaModel oldModel = - tester.deploy(null, getServices("index"), Environment.dev, "").getFirst(); - List actions = tester.deploy(oldModel, getServices("streaming"), Environment.dev, "").getSecond(); - assertReindexingChange("Document type 'music' in cluster 'default-content' changed indexing mode from 'indexed' to 'streaming'", actions); - } - @Test void testChangingIndexModeFromIndexedToStreamingWhenDisallowed() { ValidationTester tester = new ValidationTester(); @@ -38,12 +28,13 @@ void testChangingIndexModeFromIndexedToStreamingWhenDisallowed() { VespaModel oldModel = tester.deploy(null, getServices("index"), Environment.prod, "").getFirst(); try { - tester.deploy(oldModel, getServices("streaming"), Environment.prod, "").getSecond(); + List changeActions = + tester.deploy(oldModel, getServices("streaming"), Environment.prod, "").getSecond(); fail("Should throw on disallowed config change action"); } catch (ValidationException e) { - assertEquals("indexing-mode-change: " + - "Document type 'music' in cluster 'default-content' changed indexing mode from 'indexed' to 'streaming'. " + + assertEquals("indexing-mode-change:\n" + + "\tDocument type 'music' in cluster 'default-content' changed indexing mode from 'indexed' to 'streaming'\n" + "To allow this add indexing-mode-change to validation-overrides.xml, see https://docs.vespa.ai/en/reference/validation-overrides.html", e.getMessage()); } @@ -103,10 +94,8 @@ private static String getServices(String indexingMode) { } private static final String validationOverrides = - """ - - indexing-mode-change - - """; + "\n" + + " indexing-mode-change\n" + + "\n"; }