Add cloud flag dimension

8 files changed, 167 insertions, 20 deletions

diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneApiMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneApiMock.java
index 6fd44e09d8d..21fe1f66bc5 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneApiMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneApiMock.java
@@ -36,8 +36,8 @@ public class ZoneApiMock implements ZoneApi {
         }
     }
 
-    public static ZoneApiMock fromId(String id) {
-        return from(ZoneId.from(id));
+    public static ZoneApiMock fromId(String zoneId) {
+        return from(ZoneId.from(zoneId));
     }
 
     public static ZoneApiMock from(Environment environment, RegionName region) {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java
index e6a9014df94..63d479d4c6c 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java
@@ -161,7 +161,7 @@ public class ZoneRegistryMock extends AbstractComponent implements ZoneRegistry
 
     @Override
     public ZoneApi systemZone() {
-        return ZoneApiMock.fromId("prod.controller");
+        return ZoneApiMock.newBuilder().withSystem(system).withVirtualId(ZoneId.ofVirtualControllerZone()).build();
     }
 
     @Override
@@ -180,7 +180,7 @@ public class ZoneRegistryMock extends AbstractComponent implements ZoneRegistry
     private ZoneApiMock systemAsZone() {
         return ZoneApiMock.newBuilder()
                           .with(ZoneId.from("prod.us-east-1"))
-                          .withVirtualId(ZoneId.from("prod.controller"))
+                          .withVirtualId(ZoneId.ofVirtualControllerZone())
                           .build();
     }
 
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 36679e0dd91..d0d362abcfc 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
@@ -20,9 +20,12 @@ import static org.assertj.core.api.Assertions.assertThat;
  * @author bjorncs
  */
 public class SystemFlagsDeployResultTest {
+    private final ZoneApiMock prodUsWest1Zone = ZoneApiMock.fromId("prod.us-west-1");
+    private final ZoneRegistryMock registry = new ZoneRegistryMock(SystemName.cd).setZones(prodUsWest1Zone);
+
     @Test
     void changes_and_errors_are_present_in_wire_format() {
-        FlagsTarget controllerTarget = FlagsTarget.forController(SystemName.cd);
+        FlagsTarget controllerTarget = FlagsTarget.forController(registry.systemZone());
         FlagId flagOne = new FlagId("flagone");
         FlagId flagTwo = new FlagId("flagtwo");
         SystemFlagsDeployResult result = new SystemFlagsDeployResult(
@@ -41,10 +44,8 @@ public class SystemFlagsDeployResultTest {
 
     @Test
     void identical_errors_and_changes_from_multiple_targets_are_merged() {
-        ZoneApiMock prodUsWest1Zone = ZoneApiMock.fromId("prod.us-west-1");
-        ZoneRegistryMock registry = new ZoneRegistryMock(SystemName.cd).setZones(prodUsWest1Zone);
-        FlagsTarget prodUsWest1Target = FlagsTarget.forConfigServer(registry, prodUsWest1Zone.getId());
-        FlagsTarget controllerTarget = FlagsTarget.forController(SystemName.cd);
+        FlagsTarget prodUsWest1Target = FlagsTarget.forConfigServer(registry, prodUsWest1Zone);
+        FlagsTarget controllerTarget = FlagsTarget.forController(registry.systemZone());
 
         FlagId flagOne = new FlagId("flagone");
         FlagId flagTwo = new FlagId("flagtwo");
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 50354639f6f..14e37853191 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
@@ -2,7 +2,9 @@
 package com.yahoo.vespa.hosted.controller.restapi.systemflags;
 
 import com.yahoo.config.provision.SystemName;
+import com.yahoo.vespa.flags.FetchVector;
 import com.yahoo.vespa.flags.FlagId;
+import com.yahoo.vespa.flags.Flags;
 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;
@@ -15,11 +17,16 @@ import java.io.UncheckedIOException;
 import java.nio.file.Files;
 import java.nio.file.Paths;
 import java.util.List;
+import java.util.Optional;
 import java.util.Set;
 
 import static com.yahoo.vespa.hosted.controller.restapi.systemflags.SystemFlagsDeployResult.FlagDataChange;
 import static com.yahoo.vespa.hosted.controller.restapi.systemflags.SystemFlagsDeployResult.OperationError;
+import static com.yahoo.yolean.Exceptions.uncheck;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
@@ -38,12 +45,12 @@ public class SystemFlagsDeployerTest {
     private final ZoneApiMock prodUsEast3Zone = ZoneApiMock.fromId("prod.us-east-3");
     private final ZoneRegistryMock registry = new ZoneRegistryMock(SYSTEM).setZones(prodUsWest1Zone, prodUsEast3Zone);
 
-    private final FlagsTarget controllerTarget = FlagsTarget.forController(SYSTEM);
-    private final FlagsTarget prodUsWest1Target = FlagsTarget.forConfigServer(registry, prodUsWest1Zone.getId());
-    private final FlagsTarget prodUsEast3Target = FlagsTarget.forConfigServer(registry, prodUsEast3Zone.getId());
+    private final FlagsTarget controllerTarget = FlagsTarget.forController(registry.systemZone());
+    private final FlagsTarget prodUsWest1Target = FlagsTarget.forConfigServer(registry, prodUsWest1Zone);
+    private final FlagsTarget prodUsEast3Target = FlagsTarget.forConfigServer(registry, prodUsEast3Zone);
 
     @Test
-    void deploys_flag_data_to_targets() throws IOException {
+    void deploys_flag_data_to_targets() {
         FlagsClient flagsClient = mock(FlagsClient.class);
         when(flagsClient.listFlagData(controllerTarget)).thenReturn(List.of());
         when(flagsClient.listFlagData(prodUsWest1Target)).thenReturn(List.of(flagData("existing-prod.us-west-1.json")));
@@ -74,7 +81,81 @@ public class SystemFlagsDeployerTest {
     }
 
     @Test
-    void dryrun_should_not_change_flags() throws IOException {
+    void deploys_partial_flag_data_to_targets() {
+        // default.json contains one rule with 2 conditions, one of which has a condition on the aws cloud.
+        // This condition IS resolved for a config server target, but NOT for a controller target, because FLAG_ID
+        // has the CLOUD dimension set.
+        deployFlags(Optional.empty(), "partial/default.json", Optional.of("partial/put-controller.json"), true, PutType.CREATE, FetchVector.Dimension.CLOUD);
+        deployFlags(Optional.empty(), "partial/default.json", Optional.empty(), false, PutType.NONE, FetchVector.Dimension.CLOUD);
+        deployFlags(Optional.of("partial/initial.json"), "partial/default.json", Optional.of("partial/put-controller-2.json"), true, PutType.UPDATE, FetchVector.Dimension.CLOUD);
+        deployFlags(Optional.of("partial/initial.json"), "partial/default.json", Optional.empty(), false, PutType.DELETE, FetchVector.Dimension.CLOUD);
+
+        // When the CLOUD dimension is NOT set on the dimension, the controller target will also resolve that dimension, and
+        // the result should be identical to the config server target.  Let's also verify the config server target is unchanged.
+        deployFlags(Optional.empty(), "partial/default.json", Optional.empty(), true, PutType.NONE);
+        deployFlags(Optional.empty(), "partial/default.json", Optional.empty(), false, PutType.NONE);
+        deployFlags(Optional.of("partial/initial.json"), "partial/default.json", Optional.empty(), true, PutType.DELETE);
+        deployFlags(Optional.of("partial/initial.json"), "partial/default.json", Optional.empty(), false, PutType.DELETE);
+    }
+
+    private enum PutType {
+        CREATE,
+        UPDATE,
+        DELETE,
+        NONE
+    }
+
+    /**
+     * @param existingFlagDataPath path to flag data the target already has
+     * @param defaultFlagDataPath  path to default json file
+     * @param putFlagDataPath      path to flag data pushed to target, or empty if nothing should be pushed
+     * @param controller           whether to target the controller, or config server
+     */
+    private void deployFlags(Optional<String> existingFlagDataPath,
+                             String defaultFlagDataPath,
+                             Optional<String> putFlagDataPath,
+                             boolean controller,
+                             PutType putType,
+                             FetchVector.Dimension... flagDimensions) {
+        List<FlagData> existingFlagData = existingFlagDataPath.map(SystemFlagsDeployerTest::flagData).map(List::of).orElse(List.of());
+        FlagData defaultFlagData = flagData(defaultFlagDataPath);
+        FlagsTarget target = controller ? controllerTarget : prodUsWest1Target;
+        Optional<FlagData> putFlagData = putFlagDataPath.map(SystemFlagsDeployerTest::flagData);
+
+        try (var replacer = Flags.clearFlagsForTesting()) {
+            Flags.defineStringFlag(FLAG_ID.toString(), "default", List.of("hakonhall"), "2023-07-27", "2123-07-27", "", "", flagDimensions);
+
+            FlagsClient flagsClient = mock(FlagsClient.class);
+            when(flagsClient.listFlagData(target)).thenReturn(existingFlagData);
+
+            SystemFlagsDataArchive archive = new SystemFlagsDataArchive.Builder()
+                    .addFile("default.json", defaultFlagData)
+                    .build();
+
+            SystemFlagsDeployer deployer = new SystemFlagsDeployer(flagsClient, SYSTEM, Set.of(target));
+
+            List<FlagDataChange> changes = deployer.deployFlags(archive, false).flagChanges();
+
+            putFlagData.ifPresentOrElse(flagData -> {
+                verify(flagsClient).putFlagData(target, flagData);
+                switch (putType) {
+                    case CREATE -> assertThat(changes).containsOnly(FlagDataChange.created(FLAG_ID, target, flagData));
+                    case UPDATE -> assertThat(changes).containsOnly(FlagDataChange.updated(FLAG_ID, target, flagData, existingFlagData.get(0)));
+                    case DELETE, NONE -> throw new IllegalStateException("Flag data put to the target, but change type is " + putType);
+                }
+            }, () -> {
+                verify(flagsClient, never()).putFlagData(eq(target), any());
+                switch (putType) {
+                    case DELETE -> assertThat(changes).containsOnly(FlagDataChange.deleted(FLAG_ID, target));
+                    case NONE -> assertEquals(changes, List.of());
+                    default -> throw new IllegalStateException("No flag data is expected to be put to the target but change type is " + putType);
+                }
+            });
+        }
+    }
+
+    @Test
+    void dryrun_should_not_change_flags() {
         FlagsClient flagsClient = mock(FlagsClient.class);
         when(flagsClient.listFlagData(controllerTarget)).thenReturn(List.of());
         when(flagsClient.listDefinedFlags(controllerTarget)).thenReturn(List.of(new FlagId("my-flag")));
@@ -97,7 +178,7 @@ public class SystemFlagsDeployerTest {
     }
 
     @Test
-    void creates_error_entries_in_result_if_flag_data_operations_fail() throws IOException {
+    void creates_error_entries_in_result_if_flag_data_operations_fail() {
         FlagsClient flagsClient = mock(FlagsClient.class);
         UncheckedIOException exception = new UncheckedIOException(new IOException("I/O error message"));
         when(flagsClient.listFlagData(prodUsWest1Target)).thenThrow(exception);
@@ -120,7 +201,7 @@ public class SystemFlagsDeployerTest {
     }
 
     @Test
-    void creates_error_entry_for_invalid_flag_archive() throws IOException {
+    void creates_error_entry_for_invalid_flag_archive() {
         FlagsClient flagsClient = mock(FlagsClient.class);
         FlagData defaultData = flagData("flags/my-flag/main.json");
         SystemFlagsDataArchive archive = new SystemFlagsDataArchive.Builder()
@@ -135,7 +216,7 @@ public class SystemFlagsDeployerTest {
     }
 
     @Test
-    void creates_error_entry_for_flag_data_of_undefined_flag() throws IOException {
+    void creates_error_entry_for_flag_data_of_undefined_flag() {
         FlagData prodUsEast3Data = flagData("flags/my-flag/main.prod.us-east-3.json");
         FlagsClient flagsClient = mock(FlagsClient.class);
         when(flagsClient.listFlagData(prodUsEast3Target))
@@ -154,7 +235,7 @@ public class SystemFlagsDeployerTest {
     }
 
     @Test
-    void creates_warning_entry_for_existing_flag_data_for_undefined_flag() throws IOException {
+    void creates_warning_entry_for_existing_flag_data_for_undefined_flag() {
         FlagData prodUsEast3Data = flagData("flags/my-flag/main.prod.us-east-3.json");
         FlagsClient flagsClient = mock(FlagsClient.class);
         when(flagsClient.listFlagData(prodUsEast3Target))
@@ -170,8 +251,8 @@ public class SystemFlagsDeployerTest {
                 .containsOnly(OperationError.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)));
+    private static FlagData flagData(String filename) {
+        return FlagData.deserializeUtf8Json(uncheck(() -> Files.readAllBytes(Paths.get("src/test/resources/system-flags/" + filename))));
     }
 
 }
diff --git a/controller-server/src/test/resources/system-flags/partial/default.json b/controller-server/src/test/resources/system-flags/partial/default.json
new file mode 100644
index 00000000000..881d4170c3b
--- /dev/null
+++ b/controller-server/src/test/resources/system-flags/partial/default.json
@@ -0,0 +1,20 @@
+{
+  "id" : "my-flag",
+  "rules" : [
+    {
+      "conditions": [
+        {
+          "type": "whitelist",
+          "dimension": "system",
+          "values": [ "main" ]
+        },
+        {
+          "type": "whitelist",
+          "dimension": "cloud",
+          "values": [ "aws" ]
+        }
+      ],
+      "value" : "foo-value"
+    }
+  ]
+}
\ No newline at end of file
diff --git a/controller-server/src/test/resources/system-flags/partial/initial.json b/controller-server/src/test/resources/system-flags/partial/initial.json
new file mode 100644
index 00000000000..a16ea583005
--- /dev/null
+++ b/controller-server/src/test/resources/system-flags/partial/initial.json
@@ -0,0 +1,15 @@
+{
+  "id" : "my-flag",
+  "rules" : [
+    {
+      "conditions": [
+        {
+          "type": "whitelist",
+          "dimension": "application",
+          "values": [ "a:b:c" ]
+        }
+      ],
+      "value" : "bar-value"
+    }
+  ]
+}
\ No newline at end of file
diff --git a/controller-server/src/test/resources/system-flags/partial/put-controller-2.json b/controller-server/src/test/resources/system-flags/partial/put-controller-2.json
new file mode 100644
index 00000000000..47aa0af47ce
--- /dev/null
+++ b/controller-server/src/test/resources/system-flags/partial/put-controller-2.json
@@ -0,0 +1,15 @@
+{
+  "id" : "my-flag",
+  "rules" : [
+    {
+      "conditions": [
+        {
+          "type": "whitelist",
+          "dimension": "cloud",
+          "values": [ "aws" ]
+        }
+      ],
+      "value" : "foo-value"
+    }
+  ]
+}
\ No newline at end of file
diff --git a/controller-server/src/test/resources/system-flags/partial/put-controller.json b/controller-server/src/test/resources/system-flags/partial/put-controller.json
new file mode 100644
index 00000000000..47aa0af47ce
--- /dev/null
+++ b/controller-server/src/test/resources/system-flags/partial/put-controller.json
@@ -0,0 +1,15 @@
+{
+  "id" : "my-flag",
+  "rules" : [
+    {
+      "conditions": [
+        {
+          "type": "whitelist",
+          "dimension": "cloud",
+          "values": [ "aws" ]
+        }
+      ],
+      "value" : "foo-value"
+    }
+  ]
+}
\ No newline at end of file