diff --git a/priam/src/main/java/com/netflix/priam/aws/UpdateSecuritySettings.java b/priam/src/main/java/com/netflix/priam/aws/UpdateSecuritySettings.java index 29f9dc653..10a3c39df 100644 --- a/priam/src/main/java/com/netflix/priam/aws/UpdateSecuritySettings.java +++ b/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 aclToAdd = Sets.difference(desiredAcl, currentAcl); - if (!aclToAdd.isEmpty()) { - membership.addACL(aclToAdd, port, port); - firstTimeUpdated = true; + if (!config.skipDeletingOthersIngressRules()) { + Set 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 aclToRemove = Sets.difference(currentAcl, desiredAcl); - if (!aclToRemove.isEmpty()) { - membership.removeACL(aclToRemove, port, port); - firstTimeUpdated = true; + if (!config.skipUpdatingOthersIngressRules()) { + Set 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; + } } } diff --git a/priam/src/main/java/com/netflix/priam/config/IConfiguration.java b/priam/src/main/java/com/netflix/priam/config/IConfiguration.java index 0d2e334ec..55bde1b73 100644 --- a/priam/src/main/java/com/netflix/priam/config/IConfiguration.java +++ b/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 diff --git a/priam/src/test/java/com/netflix/priam/aws/TestUpdateSecuritySettings.java b/priam/src/test/java/com/netflix/priam/aws/TestUpdateSecuritySettings.java index 8f53e9c3c..5b61b9f37 100644 --- a/priam/src/test/java/com/netflix/priam/aws/TestUpdateSecuritySettings.java +++ b/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) { diff --git a/priam/src/test/java/com/netflix/priam/config/FakeConfiguration.java b/priam/src/test/java/com/netflix/priam/config/FakeConfiguration.java index 099bc2e53..ddd8a44e4 100644 --- a/priam/src/test/java/com/netflix/priam/config/FakeConfiguration.java +++ b/priam/src/test/java/com/netflix/priam/config/FakeConfiguration.java @@ -34,6 +34,8 @@ public class FakeConfiguration implements IConfiguration { private boolean mayCreateNewToken; private ImmutableList racs; private boolean usePrivateIp; + private boolean skipDeletingOthersIngressRules; + private boolean skipUpdatingOthersIngressRules; public final Map 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; + } }