Warn on flag data for undefined flags

5 files changed, 129 insertions, 9 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/FlagsClient.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/FlagsClient.java
index 2993b780dfe..161d3734aae 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/FlagsClient.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/FlagsClient.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.hosted.controller.restapi.systemflags;
 
 import ai.vespa.util.http.retry.DelayedConnectionLevelRetryHandler;
+import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.yahoo.vespa.athenz.api.AthenzIdentity;
 import com.yahoo.vespa.athenz.identity.ServiceIdentityProvider;
@@ -35,6 +36,7 @@ import java.io.UncheckedIOException;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.time.Duration;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Set;
 
@@ -65,6 +67,17 @@ class FlagsClient {
         });
     }
 
+    List<FlagId> listDefinedFlags(FlagsTarget target) {
+        HttpGet request = new HttpGet(createUri(target, "/defined", List.of()));
+        return executeRequest(request, response -> {
+            verifySuccess(response, null);
+            JsonNode json = mapper.readTree(response.getEntity().getContent());
+            List<FlagId> flagIds = new ArrayList<>();
+            json.fieldNames().forEachRemaining(fieldName -> flagIds.add(new FlagId(fieldName)));
+            return flagIds;
+        });
+    }
+
     void putFlagData(FlagsTarget target, FlagData flagData) throws FlagsException, UncheckedIOException {
         HttpPut request = new HttpPut(createUri(target, "/data/" + flagData.id().toString(), List.of()));
         request.setEntity(jsonContent(flagData.serializeToJson()));
@@ -82,7 +95,6 @@ class FlagsClient {
         });
     }
 
-
     private static CloseableHttpClient createClient(ServiceIdentityProvider identityProvider, Set<FlagsTarget> targets) {
         DelayedConnectionLevelRetryHandler retryHandler = DelayedConnectionLevelRetryHandler.Builder
                 .withExponentialBackoff(Duration.ofSeconds(1), Duration.ofSeconds(20), 5)
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResult.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResult.java
index c55d9d9a5e9..3ba57095350 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResult.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResult.java
@@ -8,6 +8,7 @@ import com.yahoo.vespa.hosted.controller.api.systemflags.v1.FlagsTarget;
 import com.yahoo.vespa.hosted.controller.api.systemflags.v1.wire.WireSystemFlagsDeployResult;
 import com.yahoo.vespa.hosted.controller.api.systemflags.v1.wire.WireSystemFlagsDeployResult.WireFlagDataChange;
 import com.yahoo.vespa.hosted.controller.api.systemflags.v1.wire.WireSystemFlagsDeployResult.WireOperationFailure;
+import com.yahoo.vespa.hosted.controller.api.systemflags.v1.wire.WireSystemFlagsDeployResult.WireWarning;
 
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -27,14 +28,16 @@ class SystemFlagsDeployResult {
 
     private final List<FlagDataChange> flagChanges;
     private final List<OperationError> errors;
+    private final List<Warning> warnings;
 
-    SystemFlagsDeployResult(List<FlagDataChange> flagChanges, List<OperationError> errors) {
+    SystemFlagsDeployResult(List<FlagDataChange> flagChanges, List<OperationError> errors, List<Warning> warnings) {
         this.flagChanges = flagChanges;
         this.errors = errors;
+        this.warnings = warnings;
     }
 
     SystemFlagsDeployResult(List<OperationError> errors) {
-        this(List.of(), errors);
+        this(List.of(), errors, List.of());
     }
 
     List<FlagDataChange> flagChanges() {
@@ -45,10 +48,13 @@ class SystemFlagsDeployResult {
         return errors;
     }
 
+    List<Warning> warnings() { return warnings; }
+
     static SystemFlagsDeployResult merge(List<SystemFlagsDeployResult> results) {
         List<FlagDataChange> mergedChanges = mergeChanges(results);
         List<OperationError> mergedErrors = mergeErrors(results);
-        return new SystemFlagsDeployResult(mergedChanges, mergedErrors);
+        List<Warning> mergedWarnings = mergeWarnings(results);
+        return new SystemFlagsDeployResult(mergedChanges, mergedErrors, mergedWarnings);
     }
 
     private static List<OperationError> mergeErrors(List<SystemFlagsDeployResult> results) {
@@ -81,6 +87,20 @@ class SystemFlagsDeployResult {
         return mergedChanges;
     }
 
+    private static List<Warning> mergeWarnings(List<SystemFlagsDeployResult> results) {
+        Map<WarningWithoutTarget, Set<FlagsTarget>> targetsForWarning = new HashMap<>();
+        for (SystemFlagsDeployResult result : results) {
+            for (Warning warning : result.warnings()) {
+                var warningWithoutTarget = new WarningWithoutTarget(warning);
+                targetsForWarning.computeIfAbsent(warningWithoutTarget, k -> new HashSet<>())
+                        .addAll(warning.targets());
+            }
+        }
+        List<Warning> mergedWarnings = new ArrayList<>();
+        targetsForWarning.forEach(
+                (warning, targets) -> mergedWarnings.add(warning.toWarning(targets)));
+        return mergedWarnings;
+    }
 
     WireSystemFlagsDeployResult toWire() {
         var wireResult = new WireSystemFlagsDeployResult();
@@ -104,6 +124,14 @@ class SystemFlagsDeployResult {
             wireError.data = error.flagData().map(FlagData::toWire).orElse(null);
             wireResult.errors.add(wireError);
         }
+        wireResult.warnings = new ArrayList<>();
+        for (Warning warning : warnings) {
+            var wireWarning = new WireWarning();
+            wireWarning.message = warning.message();
+            wireWarning.flagId = warning.flagId().toString();
+            wireWarning.targets = warning.targets().stream().map(FlagsTarget::asString).collect(toList());
+            wireResult.warnings.add(wireWarning);
+        }
         return wireResult;
     }
 
@@ -264,6 +292,38 @@ class SystemFlagsDeployResult {
         String asString() { return stringValue; }
     }
 
+    static class Warning {
+        final String message;
+        final Set<FlagsTarget> targets;
+        final FlagId flagId;
+
+        private Warning(String message, Set<FlagsTarget> targets, FlagId flagId) {
+            this.message = message;
+            this.targets = targets;
+            this.flagId = flagId;
+        }
+
+        static Warning dataForUndefinedFlag(FlagsTarget target, FlagId flagId) {
+            return new Warning("Flag data present for undefined flag", Set.of(target), flagId);
+        }
+
+        String message() { return message; }
+        Set<FlagsTarget> targets() { return targets; }
+        FlagId flagId() { return flagId; }
+
+        @Override
+        public boolean equals(Object o) {
+            if (this == o) return true;
+            if (o == null || getClass() != o.getClass()) return false;
+            Warning warning = (Warning) o;
+            return Objects.equals(message, warning.message) &&
+                    Objects.equals(targets, warning.targets) &&
+                    Objects.equals(flagId, warning.flagId);
+        }
+
+        @Override public int hashCode() { return Objects.hash(message, targets, flagId); }
+    }
+
     private static class FlagDataChangeWithoutTarget {
         final FlagId flagId;
         final OperationType operationType;
@@ -333,4 +393,16 @@ class SystemFlagsDeployResult {
 
         @Override public int hashCode() { return Objects.hash(message, operation, flagId, flagData); }
     }
+
+    private static class WarningWithoutTarget {
+        final String message;
+        final FlagId flagId;
+
+        WarningWithoutTarget(Warning warning) {
+            this.message = warning.message();
+            this.flagId = warning.flagId();
+        }
+
+        Warning toWarning(Set<FlagsTarget> targets) { return new Warning(message, targets, flagId); }
+    }
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployer.java
index c75a6d7b413..820384ce810 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployer.java
@@ -11,6 +11,7 @@ import com.yahoo.vespa.flags.json.FlagData;
 import com.yahoo.vespa.hosted.controller.api.systemflags.v1.FlagsTarget;
 import com.yahoo.vespa.hosted.controller.api.systemflags.v1.SystemFlagsDataArchive;
 import com.yahoo.vespa.hosted.controller.restapi.systemflags.SystemFlagsDeployResult.OperationError;
+import com.yahoo.vespa.hosted.controller.restapi.systemflags.SystemFlagsDeployResult.Warning;
 
 import java.util.ArrayList;
 import java.util.Collection;
@@ -79,8 +80,10 @@ class SystemFlagsDeployer  {
     private SystemFlagsDeployResult deployFlags(FlagsTarget target, Set<FlagData> flagData, boolean dryRun) {
         Map<FlagId, FlagData> wantedFlagData = lookupTable(flagData);
         Map<FlagId, FlagData> currentFlagData;
+        List<FlagId> definedFlags;
         try {
             currentFlagData = lookupTable(client.listFlagData(target));
+            definedFlags = client.listDefinedFlags(target);
         } catch (Exception e) {
             log.log(LogLevel.WARNING, String.format("Failed to list flag data for target '%s': %s", target, e.getMessage()), e);
             return new SystemFlagsDeployResult(List.of(OperationError.listFailed(e.getMessage(), target)));
@@ -88,12 +91,13 @@ class SystemFlagsDeployer  {
 
         List<OperationError> errors = new ArrayList<>();
         List<FlagDataChange> results = new ArrayList<>();
+        List<Warning> warnings = new ArrayList<>();
 
         createNewFlagData(target, dryRun, wantedFlagData, currentFlagData, results, errors);
         updateExistingFlagData(target, dryRun, wantedFlagData, currentFlagData, results, errors);
         removeOldFlagData(target, dryRun, wantedFlagData, currentFlagData, results, errors);
-
-        return new SystemFlagsDeployResult(results, errors);
+        warnOnFlagDataForUndefinedFlags(target, wantedFlagData, currentFlagData, definedFlags, warnings);
+        return new SystemFlagsDeployResult(results, errors, warnings);
     }
 
     private void createNewFlagData(FlagsTarget target,
@@ -171,6 +175,18 @@ class SystemFlagsDeployer  {
         });
     }
 
+    private static void warnOnFlagDataForUndefinedFlags(FlagsTarget target,
+                                                        Map<FlagId, FlagData> wantedFlagData,
+                                                        Map<FlagId, FlagData> currentFlagData,
+                                                        List<FlagId> definedFlags,
+                                                        List<Warning> warnings) {
+        for (FlagId flagId : currentFlagData.keySet()) {
+            if (wantedFlagData.containsKey(flagId) && !definedFlags.contains(flagId)) {
+                warnings.add(Warning.dataForUndefinedFlag(target, flagId));
+            }
+        }
+    }
+
     private static void dryRunFlagDataValidation(FlagData data) {
         Flags.getFlag(data.id())
                 .ifPresent(definition -> data.validate(definition.getUnboundFlag().serializer()));
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResultTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResultTest.java
index 104bb91a8cb..b2a835b6b55 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResultTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResultTest.java
@@ -27,7 +27,8 @@ public class SystemFlagsDeployResultTest {
                 List.of(
                         FlagDataChange.deleted(flagOne, controllerTarget)),
                 List.of(
-                        OperationError.deleteFailed("delete failed", controllerTarget, flagTwo)));
+                        OperationError.deleteFailed("delete failed", controllerTarget, flagTwo)),
+                List.of());
         WireSystemFlagsDeployResult wire = result.toWire();
 
         assertThat(wire.changes).hasSize(1);
@@ -49,11 +50,13 @@ public class SystemFlagsDeployResultTest {
         SystemFlagsDeployResult resultController =
                 new SystemFlagsDeployResult(
                         List.of(FlagDataChange.deleted(flagOne, controllerTarget)),
-                        List.of(OperationError.deleteFailed("message", controllerTarget, flagTwo)));
+                        List.of(OperationError.deleteFailed("message", controllerTarget, flagTwo)),
+                        List.of());
         SystemFlagsDeployResult resultProdUsWest1 =
                 new SystemFlagsDeployResult(
                         List.of(FlagDataChange.deleted(flagOne, prodUsWest1Target)),
-                        List.of(OperationError.deleteFailed("message", prodUsWest1Target, flagTwo)));
+                        List.of(OperationError.deleteFailed("message", prodUsWest1Target, flagTwo)),
+                        List.of());
 
         var results = List.of(resultController, resultProdUsWest1);
         SystemFlagsDeployResult mergedResult = merge(results);
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployerTest.java
index 4c41b54585f..475ac12f2fd 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployerTest.java
@@ -132,6 +132,23 @@ public class SystemFlagsDeployerTest {
                 .containsOnly(OperationError.archiveValidationFailed("Unknown flag file: flags/my-flag/main.prod.unknown-region.json"));
     }
 
+    @Test
+    public void creates_warning_entry_for_existing_flag_data_for_undefined_flag() throws IOException {
+        FlagData prodUsEast3Data = flagData("flags/my-flag/main.prod.us-east-3.json");
+        FlagsClient flagsClient = mock(FlagsClient.class);
+        when(flagsClient.listFlagData(prodUsEast3Target))
+                .thenReturn(List.of(prodUsEast3Data));
+        when(flagsClient.listDefinedFlags(prodUsEast3Target))
+                .thenReturn(List.of());
+        SystemFlagsDataArchive archive = new SystemFlagsDataArchive.Builder()
+                .addFile("main.prod.us-east-3.json", prodUsEast3Data)
+                .build();
+        SystemFlagsDeployer deployer = new SystemFlagsDeployer(flagsClient, SYSTEM, Set.of(prodUsEast3Target));
+        SystemFlagsDeployResult result = deployer.deployFlags(archive, true);
+        assertThat(result.warnings())
+                .containsOnly(SystemFlagsDeployResult.Warning.dataForUndefinedFlag(prodUsEast3Target, new FlagId("my-flag")));
+    }
+
     private static FlagData flagData(String filename) throws IOException {
         return FlagData.deserializeUtf8Json(Files.readAllBytes(Paths.get("src/test/resources/system-flags/" + filename)));
     }