diff options
author | Jon Bratseth <bratseth@gmail.com> | 2023-12-18 14:12:22 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2023-12-18 14:12:22 +0100 |
commit | 6adad88bd950d1af73a676752639d8c9cd4a74c1 (patch) | |
tree | 820c9124be4c50191bf28d4aff6e817488448feb | |
parent | ee6bc94776a8998ce78353bbf685c8c121b3b41b (diff) | |
parent | 3de9d979247c190819070c86a6fcbb8ff6e2243a (diff) |
Merge pull request #29692 from vespa-engine/jonmv/keep-config-change-actions-in-dev
Jonmv/keep config change actions in dev
8 files changed, 128 insertions, 63 deletions
diff --git a/config-model-api/abi-spec.json b/config-model-api/abi-spec.json index ae2753d47bc..a8aaf0f57ef 100644 --- a/config-model-api/abi-spec.json +++ b/config-model-api/abi-spec.json @@ -771,7 +771,9 @@ "attributes" : [ "public" ], - "methods" : [ ], + "methods" : [ + "public java.util.Map messagesById()" + ], "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 1edddb63e52..7b52d825473 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,6 +15,7 @@ import java.time.format.DateTimeFormatter; 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; @@ -36,7 +37,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<Allow> overrides) { this(overrides, null); } @@ -163,10 +164,13 @@ public class ValidationOverrides { */ public static class ValidationException extends IllegalArgumentException { + private final Map<ValidationId, Collection<String>> 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<ValidationId, Collection<String>> messagesById) { @@ -175,8 +179,11 @@ public class ValidationOverrides { 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<ValidationId, Collection<String>> 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 33dfee58d1a..f19341098f4 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 class DeployState implements ConfigDefinitionStore { /** 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 903f1c06024..65572d07cc2 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,13 +217,6 @@ public class VespaModelFactory implements ModelFactory { private List<ConfigChangeAction> 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 56277345515..b9995d290cc 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,6 +3,7 @@ package com.yahoo.vespa.model.application.validation; 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; @@ -25,15 +26,19 @@ import com.yahoo.vespa.model.application.validation.change.RestartOnDeployForOnn 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; @@ -51,7 +56,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<Validator> additionalValidators) { this.additionalValidators = additionalValidators; } /** @@ -62,52 +67,53 @@ public class Validation { * @throws ValidationOverrides.ValidationException if the change fails validation */ public List<ConfigChangeAction> validate(VespaModel model, ValidationParameters validationParameters, DeployState deployState) { + Execution execution = new Execution(model, deployState); if (validationParameters.checkRouting()) { - new RoutingValidator().validate(model, deployState); - new RoutingSelectorValidator().validate(model, deployState); + execution.run(new RoutingValidator()); + execution.run(new RoutingSelectorValidator()); } - 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)); + 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); List<ConfigChangeAction> result = Collections.emptyList(); if (deployState.getProperties().isFirstTimeDeployment()) { - validateFirstTimeDeployment(model, deployState); + validateFirstTimeDeployment(execution); } else { Optional<Model> currentActiveModel = deployState.getPreviousModel(); if (currentActiveModel.isPresent() && (currentActiveModel.get() instanceof VespaModel)) { - result = validateChanges((VespaModel) currentActiveModel.get(), model, deployState); + result = validateChanges((VespaModel) currentActiveModel.get(), execution); deferConfigChangesForClustersToBeRestarted(result, model); } } + execution.throwIfFailed(); return result; } - private static List<ConfigChangeAction> validateChanges(VespaModel currentModel, VespaModel nextModel, - DeployState deployState) { + private static List<ConfigChangeAction> validateChanges(VespaModel currentModel, Execution execution) { ChangeValidator[] validators = new ChangeValidator[] { new IndexingModeChangeValidator(), new GlobalDocumentChangeValidator(), @@ -127,20 +133,15 @@ public class Validation { new RestartOnDeployForOnnxModelChangesValidator(), }; List<ConfigChangeAction> actions = Arrays.stream(validators) - .flatMap(v -> v.validate(currentModel, nextModel, deployState).stream()) + .flatMap(v -> v.validate(currentModel, execution.model, execution.deployState).stream()) .toList(); - Map<ValidationId, Collection<String>> 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()); + execution.runChanges(actions); return actions; } - private static void validateFirstTimeDeployment(VespaModel model, DeployState deployState) { - new RedundancyValidator().validate(model, deployState); + private static void validateFirstTimeDeployment(Execution execution) { + execution.run(new RedundancyValidator()); } private static void deferConfigChangesForClustersToBeRestarted(List<ConfigChangeAction> actions, VespaModel model) { @@ -159,4 +160,53 @@ public class Validation { } } + + private static class Execution { + + private final Map<ValidationId, List<String>> 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<ConfigChangeAction> 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 fb07f65b5f4..e2dd3aca0b9 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 class IndexingScriptChangeValidator { String fieldName = nextField.getName(); ImmutableSDField currentField = currentSchema.getConcreteField(fieldName); if (currentField != null) { - validateScripts(currentField, nextField).ifPresent(r -> result.add(r)); + validateScripts(currentField, nextField).ifPresent(result::add); } 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 6b7df8871aa..bc36b800bfb 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,9 +23,11 @@ import static org.junit.jupiter.api.Assertions.assertThrows; public class CertificateRemovalChangeValidatorTest { private static final String validationOverrides = - "<validation-overrides>\n" + - " <allow until='2000-01-14' comment='test override'>certificate-removal</allow>\n" + - "</validation-overrides>\n"; + """ + <validation-overrides> + <allow until='2000-01-14' comment='test override'>certificate-removal</allow> + </validation-overrides> + """; @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 3fd3180b37e..9e0eab9aba7 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 @@ -22,19 +22,28 @@ import static org.junit.jupiter.api.Assertions.fail; public class IndexingModeChangeValidatorTest { @Test + void testChangingIndexModeFromIndexedToStreamingWhenDisallowedButInDev() { + ValidationTester tester = new ValidationTester(); + + VespaModel oldModel = + tester.deploy(null, getServices("index"), Environment.dev, "<validation-overrides />").getFirst(); + List<ConfigChangeAction> actions = tester.deploy(oldModel, getServices("streaming"), Environment.dev, "<calidation-overrides />").getSecond(); + assertReindexingChange("Document type 'music' in cluster 'default-content' changed indexing mode from 'indexed' to 'streaming'", actions); + } + + @Test void testChangingIndexModeFromIndexedToStreamingWhenDisallowed() { ValidationTester tester = new ValidationTester(); VespaModel oldModel = tester.deploy(null, getServices("index"), Environment.prod, "<validation-overrides />").getFirst(); try { - List<ConfigChangeAction> changeActions = - tester.deploy(oldModel, getServices("streaming"), Environment.prod, "<calidation-overrides />").getSecond(); + tester.deploy(oldModel, getServices("streaming"), Environment.prod, "<calidation-overrides />").getSecond(); fail("Should throw on disallowed config change action"); } catch (ValidationException e) { - assertEquals("indexing-mode-change:\n" + - "\tDocument type 'music' in cluster 'default-content' changed indexing mode from 'indexed' to 'streaming'\n" + + assertEquals("indexing-mode-change: " + + "Document type 'music' in cluster 'default-content' changed indexing mode from 'indexed' to 'streaming'. " + "To allow this add <allow until='yyyy-mm-dd'>indexing-mode-change</allow> to validation-overrides.xml, see https://docs.vespa.ai/en/reference/validation-overrides.html", e.getMessage()); } @@ -94,8 +103,10 @@ public class IndexingModeChangeValidatorTest { } private static final String validationOverrides = - "<validation-overrides>\n" + - " <allow until='2000-01-14' comment='test override'>indexing-mode-change</allow>\n" + - "</validation-overrides>\n"; + """ + <validation-overrides> + <allow until='2000-01-14' comment='test override'>indexing-mode-change</allow> + </validation-overrides> + """; } |