Improve the configurability of setting ingress rules (#939)

priam/src/main/java/com/netflix/priam/aws/UpdateSecuritySettings.java

@@ -16,6 +16,7 @@
  */
 package com.netflix.priam.aws;
 
+import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
 import com.google.inject.Inject;
@@ -24,7 +25,6 @@
 import com.netflix.priam.identity.IMembership;
 import com.netflix.priam.identity.IPriamInstanceFactory;
 import com.netflix.priam.identity.InstanceIdentity;
-import com.netflix.priam.identity.config.InstanceInfo;
 import com.netflix.priam.scheduler.SimpleTimer;
 import com.netflix.priam.scheduler.Task;
 import com.netflix.priam.scheduler.TaskTimer;
@@ -53,20 +53,15 @@ public class UpdateSecuritySettings extends Task {
     private static final Random ran = new Random();
     private final IMembership membership;
     private final IPriamInstanceFactory factory;
-    private final InstanceInfo instanceInfo;
 
     @Inject
     // Note: do not parameterized the generic type variable to an implementation as it confuses
     // Guice in the binding.
     public UpdateSecuritySettings(
-            IConfiguration config,
-            IMembership membership,
-            IPriamInstanceFactory factory,
-            InstanceInfo instanceInfo) {
+            IConfiguration config, IMembership membership, IPriamInstanceFactory factory) {
         super(config);
         this.membership = membership;
         this.factory = factory;
-        this.instanceInfo = instanceInfo;
     }
 
     /**
@@ -82,21 +77,25 @@ public void execute() {
                         .stream()
                         .map(i -> i.getHostIP() + "/32")
                         .collect(Collectors.toSet());
-        // Make sure a hole is opened for my instance.
-        // This accommodates the eventually consistent CassandraInstanceFactory.
-        // Remove once IPs are all private as there won't be any chance of a discrepancy anymore.
-        String myIp =
-                config.usePrivateIP() ? instanceInfo.getPrivateIP() : instanceInfo.getHostIP();
-        desiredAcl.add(myIp + "/32");
-        Set<String> aclToAdd = Sets.difference(desiredAcl, currentAcl);
-        if (!aclToAdd.isEmpty()) {
-            membership.addACL(aclToAdd, port, port);
-            firstTimeUpdated = true;
+        if (!config.skipDeletingOthersIngressRules()) {
+            Set<String> aclToRemove = Sets.difference(currentAcl, desiredAcl);
+            logger.info("ingress rules to delete: {}", Joiner.on(",").join(aclToRemove));
+            if (!aclToRemove.isEmpty()) {
+                membership.removeACL(aclToRemove, port, port);
+                firstTimeUpdated = true;
+            }
         }
-        Set<String> aclToRemove = Sets.difference(currentAcl, desiredAcl);
-        if (!aclToRemove.isEmpty()) {
-            membership.removeACL(aclToRemove, port, port);
-            firstTimeUpdated = true;
+        if (!config.skipUpdatingOthersIngressRules()) {
+            Set<String> aclToAdd = Sets.difference(desiredAcl, currentAcl);
+            logger.info("ingress rules to update: {}", Joiner.on(",").join(aclToAdd));
+            if (!aclToAdd.isEmpty()) {
+                int resultingSize = aclToAdd.size() + currentAcl.size();
+                if (resultingSize > config.getACLSizeWarnThreshold()) {
+                    logger.warn("We are trying to make too many ingress rules! {}", resultingSize);
+                }
+                membership.addACL(aclToAdd, port, port);
+                firstTimeUpdated = true;
+            }
         }
     }
 

priam/src/main/java/com/netflix/priam/config/IConfiguration.java

@@ -1099,6 +1099,26 @@ default boolean usePrivateIP() {
         return getSnitch().equals("org.apache.cassandra.locator.GossipingPropertyFileSnitch");
     }
 
+    /**
+     * @return true if Priam should skip deleting ingress rules for IPs not found in the token
+     *     database.
+     */
+    default boolean skipDeletingOthersIngressRules() {
+        return false;
+    }
+
+    /**
+     * @return true if Priam should skip updating ingress rules for ips found in the token database.
+     */
+    default boolean skipUpdatingOthersIngressRules() {
+        return false;
+    }
+
+    /** @return get the threshold at which point we might risk not getting our ingress rule set. */
+    default int getACLSizeWarnThreshold() {
+        return 500;
+    }
+
     /**
      * Escape hatch for getting any arbitrary property by key This is useful so we don't have to
      * keep adding methods to this interface for every single configuration option ever. Also

priam/src/test/java/com/netflix/priam/aws/TestUpdateSecuritySettings.java

@@ -9,7 +9,6 @@
 import com.netflix.priam.config.IConfiguration;
 import com.netflix.priam.identity.IMembership;
 import com.netflix.priam.identity.IPriamInstanceFactory;
-import com.netflix.priam.identity.config.InstanceInfo;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -21,7 +20,6 @@ public class TestUpdateSecuritySettings {
     private IMembership membership;
     private IPriamInstanceFactory factory;
     private FakeConfiguration config;
-    private InstanceInfo instanceInfo;
 
     @Before
     public void setUp() {
@@ -30,7 +28,6 @@ public void setUp() {
         membership = injector.getInstance(IMembership.class);
         updateSecuritySettings = injector.getInstance(UpdateSecuritySettings.class);
         config = (FakeConfiguration) injector.getInstance(IConfiguration.class);
-        instanceInfo = injector.getInstance(InstanceInfo.class);
     }
 
     @Test
@@ -68,41 +65,21 @@ public void delete_factoryEmpty() { // edge-case, not expected
     }
 
     @Test
-    public void addMyPrivateIP() {
-        config.usePrivateIP(true);
-        String ingressRule = instanceInfo.getPrivateIP() + "/32";
-        Truth.assertThat(membership.listACL(PORT, PORT)).doesNotContain(ingressRule);
-        updateSecuritySettings.execute();
-        Truth.assertThat(membership.listACL(PORT, PORT)).contains(ingressRule);
-    }
-
-    @Test
-    public void addMyPublicIP() {
-        config.usePrivateIP(false);
-        String ingressRule = instanceInfo.getHostIP() + "/32";
-        Truth.assertThat(membership.listACL(PORT, PORT)).doesNotContain(ingressRule);
-        updateSecuritySettings.execute();
-        Truth.assertThat(membership.listACL(PORT, PORT)).contains(ingressRule);
-    }
-
-    @Test
-    public void keepMyPrivateIP() {
-        config.usePrivateIP(true);
-        String ingressRule = instanceInfo.getPrivateIP() + "/32";
-        membership.addACL(ImmutableSet.of(ingressRule), PORT, PORT);
-        Truth.assertThat(membership.listACL(PORT, PORT)).contains(ingressRule);
+    public void dontDeleteOthersIngress() {
+        config.setSkipDeletingOthersIngressRules(true);
+        membership.addACL(ImmutableSet.of("1.1.1.1/32"), PORT, PORT);
+        Truth.assertThat(membership.listACL(PORT, PORT)).contains("1.1.1.1/32");
         updateSecuritySettings.execute();
-        Truth.assertThat(membership.listACL(PORT, PORT)).contains(ingressRule);
+        Truth.assertThat(membership.listACL(PORT, PORT)).contains("1.1.1.1/32");
     }
 
     @Test
-    public void keepMyPublicIP() {
-        config.usePrivateIP(false);
-        String ingressRule = instanceInfo.getHostIP() + "/32";
-        membership.addACL(ImmutableSet.of(ingressRule), PORT, PORT);
-        Truth.assertThat(membership.listACL(PORT, PORT)).contains(ingressRule);
+    public void dontUpdateOthersIngress() {
+        addToFactory(1, "1.1.1.1");
+        config.setSkipUpdatingOthersIngressRules(true);
+        Truth.assertThat(membership.listACL(PORT, PORT)).isEmpty();
         updateSecuritySettings.execute();
-        Truth.assertThat(membership.listACL(PORT, PORT)).contains(ingressRule);
+        Truth.assertThat(membership.listACL(PORT, PORT)).isEmpty();
     }
 
     private void addToFactory(int id, String ip) {

priam/src/test/java/com/netflix/priam/config/FakeConfiguration.java

@@ -34,6 +34,8 @@ public class FakeConfiguration implements IConfiguration {
     private boolean mayCreateNewToken;
     private ImmutableList<String> racs;
     private boolean usePrivateIp;
+    private boolean skipDeletingOthersIngressRules;
+    private boolean skipUpdatingOthersIngressRules;
 
     public final Map<String, String> fakeProperties = new HashMap<>();
 
@@ -227,4 +229,22 @@ public boolean usePrivateIP() {
     public void usePrivateIP(boolean usePrivateIp) {
         this.usePrivateIp = usePrivateIp;
     }
+
+    @Override
+    public boolean skipDeletingOthersIngressRules() {
+        return this.skipDeletingOthersIngressRules;
+    }
+
+    public void setSkipDeletingOthersIngressRules(boolean skipDeletingOthersIngressRules) {
+        this.skipDeletingOthersIngressRules = skipDeletingOthersIngressRules;
+    }
+
+    @Override
+    public boolean skipUpdatingOthersIngressRules() {
+        return this.skipUpdatingOthersIngressRules;
+    }
+
+    public void setSkipUpdatingOthersIngressRules(boolean skipUpdatingOthersIngressRules) {
+        this.skipUpdatingOthersIngressRules = skipUpdatingOthersIngressRules;
+    }
 }