(from AES) Scrap AMBASSADOR_FAST_VALIDATION entirely. Let CI run one …

…test with AMBASSADOR_LEGACY_MODE and one with AMBASSADOR_FAST_RECONFIGURE

.circleci/config.yml

@@ -115,8 +115,8 @@ commands:
           fi
           export DEV_KUBE_NO_PVC=yes
           export KAT_REQ_LIMIT=900
-          if << parameters.fast-reconfigure >>; then
-            export AMBASSADOR_FAST_VALIDATION=yes # empty/nonempty
+          if ! << parameters.fast-reconfigure >>; then
+            export AMBASSADOR_LEGACY_MODE=true # ParseBool
           fi
           export AMBASSADOR_FAST_RECONFIGURE=<< parameters.fast-reconfigure >> # ParseBool
           export TEST_XML_DIR=/tmp/test-logs/xml/

.circleci/config.yml.d/amb_jobs.yml

@@ -125,8 +125,8 @@ commands:
             fi
             export DEV_KUBE_NO_PVC=yes
             export KAT_REQ_LIMIT=900
-            if << parameters.fast-reconfigure >>; then
-              export AMBASSADOR_FAST_VALIDATION=yes # empty/nonempty
+            if ! << parameters.fast-reconfigure >>; then
+              export AMBASSADOR_LEGACY_MODE=true # ParseBool
             fi
             export AMBASSADOR_FAST_RECONFIGURE=<< parameters.fast-reconfigure >> # ParseBool
             export TEST_XML_DIR=/tmp/test-logs/xml/

builder/builder.mk

@@ -367,7 +367,6 @@ pytest-only: sync preflight-cluster | docker/$(NAME).docker.push.remote docker/k
 		-e DOCKER_BUILD_USERNAME \
 		-e DOCKER_BUILD_PASSWORD \
 		-e AMBASSADOR_LEGACY_MODE \
-		-e AMBASSADOR_FAST_VALIDATION \
 		-e AMBASSADOR_FAST_RECONFIGURE \
 		-it $(shell $(BUILDER)) /buildroot/builder.sh pytest-internal ; test_exit=$$? ; \
 		[ -n "$(TEST_XML_DIR)" ] && docker cp $(shell $(BUILDER)):/tmp/test-data/pytest.xml $(TEST_XML_DIR) ; exit $$test_exit

cmd/watt/aggregator/aggregator.go

@@ -2,7 +2,6 @@ package aggregator
 
 import (
 	"encoding/json"
-	"os"
 	"os/exec"
 	"strings"
 	"sync"
@@ -239,7 +238,10 @@ func (a *Aggregator) generateSnapshot(p *supervisor.Process) (string, error) {
 	return string(jsonBytes), nil
 }
 
-var fastValidation = len(os.Getenv("AMBASSADOR_FAST_VALIDATION")) > 0
+// watt only runs in legacy mode now, and legacy mode is defined
+// to not do fast validation.
+// var fastValidation = len(os.Getenv("AMBASSADOR_FAST_VALIDATION")) > 0
+var fastValidation = false
 
 func (a *Aggregator) validate(p *supervisor.Process, resources []k8s.Resource) {
 	if !fastValidation {

python/ambassador/config/config.py

@@ -28,7 +28,7 @@
 from pkg_resources import Requirement, resource_filename
 from google.protobuf import json_format
 
-from ..utils import RichStatus, dump_json
+from ..utils import RichStatus, dump_json, parse_bool
 
 from ..resource import Resource
 from .acresource import ACResource
@@ -61,7 +61,7 @@ class Config:
     single_namespace: ClassVar[bool] = bool(os.environ.get('AMBASSADOR_SINGLE_NAMESPACE'))
     certs_single_namespace: ClassVar[bool] = bool(os.environ.get('AMBASSADOR_CERTS_SINGLE_NAMESPACE', os.environ.get('AMBASSADOR_SINGLE_NAMESPACE')))
     enable_endpoints: ClassVar[bool] = not bool(os.environ.get('AMBASSADOR_DISABLE_ENDPOINTS'))
-    fast_validation: ClassVar[bool] = bool(os.environ.get('AMBASSADOR_FAST_VALIDATION'))
+    legacy_mode: ClassVar[bool] = parse_bool(os.environ.get('AMBASSADOR_LEGACY_MODE'))
 
     StorageByKind: ClassVar[Dict[str, str]] = {
         'authservice': "auth_configs",
@@ -114,8 +114,8 @@ class Config:
     fatal_errors: int
     object_errors: int
 
-    # fast_validation_disagreements tracks places where watt says a resource
-    # is invalid, but Python says it's OK.
+    # fast_validation_disagreements tracks places where entrypoint.go says a
+    # resource is invalid, but Python says it's OK.
     fast_validation_disagreements: Dict[str, List[str]]
 
     def __init__(self, schema_dir_path: Optional[str]=None) -> None:
@@ -290,7 +290,7 @@ def load_all(self, resources: Iterable[ACResource]) -> None:
         the set of ACResources to be sorted in some way that makes sense.
         """
 
-        self.logger.debug(f"Loading config; fast validation is {'enabled' if Config.fast_validation else 'disabled'}")
+        self.logger.debug(f"Loading config; legacy mode is {'enabled' if Config.legacy_mode else 'disabled'}")
 
         rcount = 0
 
@@ -481,28 +481,31 @@ def validate_object(self, resource: ACResource) -> RichStatus:
 
         need_validation = True
 
-        # Next up: is the AMBASSADOR_FAST_VALIDATION flag set?
-
-        if Config.fast_validation:
-            # Yes, so we _don't_ need to do validation here _UNLESS THIS IS A MODULE_.
-            # Why is that? Well, at present Golang doesn't validate Modules _at all_
+        # If we're not running in legacy mode, though...
+        if not Config.legacy_mode:
+            # ...then entrypoint.go will have already done validation, and we'll 
+            # trust that its validation is good _UNLESS THIS IS A MODULE_. Why?
+            # Well, at present entrypoint.go can't actually validate Modules _at all_ 
             # (because Module.spec.config is just a dict of anything, pretty much),
             # and that means it can't check for Modules with missing configs, and 
-            # _that_ crashes stuff.
+            # Modules with missing configs will crash Ambassador.
 
             if resource.kind.lower() != "module":
                 need_validation = False
 
-        # Finally, does the object specifically demand validation? (This is presently used
-        # for objects coming from annotations, since watt can't currently validate those.)
+        # Finally, does the resource specifically demand validation? (This is currently
+        # just for resources from annotations, which entrypoint.go doesn't yet validate.)
         if resource.get('_force_validation', False):
             # Yup, so we'll honor that.
             need_validation = True
             del(resource['_force_validation'])
 
-        # Did watt find errors here? (This can only happen if AMBASSADOR_FAST_VALIDATION
-        # is enabled -- in a later version we'll short-circuit earlier, but for now we're
-        # going to re-validate as a check on watt.)
+        # Did entrypoint.go flag errors here? (This can only happen if we're not in 
+        # legacy mode -- in a later version we'll short-circuit earlier, but for now 
+        # we're going to re-validate as a sanity check.)
+        #
+        # (It's still called watt_errors because our other docs talk about "watt 
+        # snapshots", and I'm OK with retaining that name for the format.)
 
         watt_errors = None
 
@@ -525,8 +528,8 @@ def validate_object(self, resource: ACResource) -> RichStatus:
 
         # ...then, let's see whether reality matches our assumption.
         if need_validation:
-            # Aha, we need to do validation -- either fast validation isn't on, or it
-            # was requested, or watt reported errors. So if we can do validation, do it.
+            # Aha, we need to do validation -- either we're in legacy mode, or 
+            # entrypoint.go reported errors. So if we can do validation, do it.
 
             # Do we have a validator that can work on this object?
             validator = self.get_validator(apiVersion, resource.kind)
@@ -538,19 +541,20 @@ def validate_object(self, resource: ACResource) -> RichStatus:
                 rc = RichStatus.OK(msg=f"no validator for {apiVersion} {resource.kind} {name} so calling it good")
 
             if watt_errors:
-                # watt reported errors. Did we find errors or not?
+                # entrypoint.go reported errors. Did we find errors or not?
 
                 if rc:
                     # We did not. Post this into fast_validation_disagreements
                     fvd = self.fast_validation_disagreements.setdefault(resource.rkey, [])
                     fvd.append(watt_errors)
                     self.logger.debug(f"validation disagreement: good {resource.kind} {name} has watt errors {watt_errors}")
 
-                    # Note that we override watt here by returning the successful
-                    # result from our validator. That's intentional in 1.6.0.
+                    # Note that we override entrypoint.go here by returning the successful
+                    # result from our validator. That's intentional for now.
                 else:
-                    # We don't like it either. Stick with the watt errors (since we know,
-                    # a priori, that fast validation is enabled).
+                    # We don't like it either. Stick with the entrypoint.go errors (since we
+                    # know, a priori, that fast validation is enabled, so the incoming error
+                    # makes more sense to report).
                     rc = RichStatus.fromError(watt_errors)
 
         # One way or the other, we're done here. Finally.

python/ambassador/ir/ir.py

@@ -1138,7 +1138,9 @@ def features(self) -> Dict[str, Any]:
         od['listener_count'] = len(self.listeners)
         od['host_count'] = len(self.hosts)
 
-        od['fast_validation'] = Config.fast_validation
+        od['legacy_mode'] = Config.legacy_mode
+        # Preserve the 'fast_validation' feature for the moment to make analytics easier
+        od['fast_validation'] = not Config.legacy_mode
         od['fast_validation_disagreements'] = len(self.aconf.fast_validation_disagreements.keys())
 
         # Fast reconfiguration information is supplied in check_scout in diagd.py.

python/tests/abstract_tests.py

@@ -113,12 +113,6 @@ def manifests(self) -> str:
       value: "true"
 """
 
-        if os.environ.get('AMBASSADOR_FAST_VALIDATION', 'false').lower() == 'true':
-            self.manifest_envs += """
-    - name: AMBASSADOR_FAST_VALIDATION
-      value: "true"
-"""
-
         if os.environ.get('AMBASSADOR_FAST_RECONFIGURE', 'false').lower() == 'true':
             self.manifest_envs += """
     - name: AMBASSADOR_FAST_RECONFIGURE