Merge pull request #29730 from vespa-engine/revert-29692-jonmv/keep-c…

…onfig-change-actions-in-dev Revert "Jonmv/keep config change actions in dev" MERGEOK
vespa-engine · Dec 21, 2023 · ef3db95 · ef3db95
2 parents d73a4b8 + 4b67fbc
commit ef3db95
Show file tree

Hide file tree

Showing 8 changed files with 63 additions and 128 deletions.
diff --git 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
@@ -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<Allow> overrides) {
         this(overrides, null);
     }
@@ -164,13 +163,10 @@ public boolean allows(ValidationId validationId, Instant now) {
      */
     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) {
@@ -179,11 +175,8 @@ private ValidationException(Map<ValidationId, Collection<String>> 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<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
@@ -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
@@ -217,6 +217,13 @@ private void validateXML(ApplicationPackage applicationPackage, boolean ignoreVa
     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
@@ -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<Validator> additionalValidators) { this.additionalValidators = additionalValidators; }
 
     /**
@@ -67,53 +62,52 @@ 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()) {
-            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<ConfigChangeAction> result = Collections.emptyList();
         if (deployState.getProperties().isFirstTimeDeployment()) {
-            validateFirstTimeDeployment(execution);
+            validateFirstTimeDeployment(model, deployState);
         } else {
             Optional<Model> 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<ConfigChangeAction> validateChanges(VespaModel currentModel, Execution execution) {
+    private static List<ConfigChangeAction> validateChanges(VespaModel currentModel, VespaModel nextModel,
+                                                            DeployState deployState) {
         ChangeValidator[] validators = new ChangeValidator[] {
                 new IndexingModeChangeValidator(),
                 new GlobalDocumentChangeValidator(),
@@ -133,15 +127,20 @@ private static List<ConfigChangeAction> validateChanges(VespaModel currentModel,
                 new RestartOnDeployForOnnxModelChangesValidator(),
         };
         List<ConfigChangeAction> 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<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());
         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<ConfigChangeAction> actions, VespaModel model) {
@@ -160,53 +159,4 @@ private static void deferConfigChangesForClustersToBeRestarted(List<ConfigChange
         }
     }
 
-
-    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/...yahoo/vespa/model/application/validation/change/search/IndexingScriptChangeValidator.java b/...yahoo/vespa/model/application/validation/change/search/IndexingScriptChangeValidator.java
@@ -40,7 +40,7 @@ public List<VespaConfigChangeAction> 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/...ahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java b/...ahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java
@@ -23,11 +23,9 @@
 public class CertificateRemovalChangeValidatorTest {
 
     private static final String validationOverrides =
-            """
-            <validation-overrides>
-                <allow until='2000-01-14' comment='test override'>certificate-removal</allow>
-            </validation-overrides>
-            """;
+            "<validation-overrides>\n" +
+            "    <allow until='2000-01-14' comment='test override'>certificate-removal</allow>\n" +
+            "</validation-overrides>\n";
 
     @Test
     void validate() {

diff --git a/.../com/yahoo/vespa/model/application/validation/change/IndexingModeChangeValidatorTest.java b/.../com/yahoo/vespa/model/application/validation/change/IndexingModeChangeValidatorTest.java
@@ -21,29 +21,20 @@
  */
 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 {
-            tester.deploy(oldModel, getServices("streaming"), Environment.prod, "<calidation-overrides />").getSecond();
+            List<ConfigChangeAction> changeActions =
+                    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: " +
-                    "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 <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());
         }
@@ -103,10 +94,8 @@ private static String getServices(String indexingMode) {
     }
 
     private static final String validationOverrides =
-            """
-            <validation-overrides>
-                <allow until='2000-01-14' comment='test override'>indexing-mode-change</allow>
-            </validation-overrides>
-            """;
+            "<validation-overrides>\n" +
+            "    <allow until='2000-01-14' comment='test override'>indexing-mode-change</allow>\n" +
+            "</validation-overrides>\n";
 
 }