Reject invalid DC names as option while creating or altering NetworkTopologyStrategy
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/f2c5ad74 Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/f2c5ad74 Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/f2c5ad74 Branch: refs/heads/trunk Commit: f2c5ad743933498e60e7eef55e8daaa6ce338a03 Parents: 21d8a7d Author: Nachiket Patil <nachiket_pa...@apple.com> Authored: Fri Aug 5 16:05:34 2016 -0700 Committer: Jeff Jirsa <jeff.ji...@crowdstrike.com> Committed: Tue Sep 27 18:16:26 2016 -0700 ---------------------------------------------------------------------- CHANGES.txt | 1 + NEWS.txt | 11 ++++ .../locator/AbstractReplicationStrategy.java | 2 +- .../locator/NetworkTopologyStrategy.java | 39 ++++++++++++- .../org/apache/cassandra/cql3/CQLTester.java | 11 ++++ .../validation/entities/SecondaryIndexTest.java | 10 ---- .../cql3/validation/operations/AlterTest.java | 47 +++++++++++++++- .../cql3/validation/operations/CreateTest.java | 59 ++++++++++++++++++++ .../apache/cassandra/dht/BootStrapperTest.java | 10 +++- .../org/apache/cassandra/service/MoveTest.java | 9 ++- 10 files changed, 181 insertions(+), 18 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/f2c5ad74/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index 4280abd..6edc491 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -6,6 +6,7 @@ * Extend ColumnIdentifier.internedInstances key to include the type that generated the byte buffer (CASSANDRA-12516) * Backport CASSANDRA-10756 (race condition in NativeTransportService shutdown) (CASSANDRA-12472) * If CF has no clustering columns, any row cache is full partition cache (CASSANDRA-12499) + * Reject invalid replication settings when creating or altering a keyspace (CASSANDRA-12681) Merged from 2.2: * Fix exceptions when enabling gossip on nodes that haven't joined the ring (CASSANDRA-12253) * Fix authentication problem when invoking clqsh copy from a SOURCE command (CASSANDRA-12642) http://git-wip-us.apache.org/repos/asf/cassandra/blob/f2c5ad74/NEWS.txt ---------------------------------------------------------------------- diff --git a/NEWS.txt b/NEWS.txt index 0bd3920..b97a420 100644 --- a/NEWS.txt +++ b/NEWS.txt @@ -13,6 +13,17 @@ restore snapshots created with the previous major version using the 'sstableloader' tool. You can upgrade the file format of your snapshots using the provided 'sstableupgrade' tool. +3.0.10 +===== + +Upgrading +--------- + - To protect against accidental data loss, cassandra no longer allows + users to set arbitrary datacenter names for NetworkTopologyStrategy. + Cassandra will allow users to continue using existing keyspaces + with invalid datacenter names, but will validat DC names on CREATE and + ALTER + 3.0.9 ===== http://git-wip-us.apache.org/repos/asf/cassandra/blob/f2c5ad74/src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java b/src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java index c90c6a1..d72c0c2 100644 --- a/src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java +++ b/src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java @@ -319,7 +319,7 @@ public abstract class AbstractReplicationStrategy } } - private void validateExpectedOptions() throws ConfigurationException + protected void validateExpectedOptions() throws ConfigurationException { Collection expectedOptions = recognizedOptions(); if (expectedOptions == null) http://git-wip-us.apache.org/repos/asf/cassandra/blob/f2c5ad74/src/java/org/apache/cassandra/locator/NetworkTopologyStrategy.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/locator/NetworkTopologyStrategy.java b/src/java/org/apache/cassandra/locator/NetworkTopologyStrategy.java index 7c8d95e..78f5b06 100644 --- a/src/java/org/apache/cassandra/locator/NetworkTopologyStrategy.java +++ b/src/java/org/apache/cassandra/locator/NetworkTopologyStrategy.java @@ -24,9 +24,11 @@ import java.util.Map.Entry; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.exceptions.ConfigurationException; import org.apache.cassandra.dht.Token; import org.apache.cassandra.locator.TokenMetadata.Topology; +import org.apache.cassandra.service.StorageService; import org.apache.cassandra.utils.FBUtilities; import com.google.common.collect.Multimap; @@ -193,10 +195,43 @@ public class NetworkTopologyStrategy extends AbstractReplicationStrategy } } + /* + * (non-javadoc) Method to generate list of valid data center names to be used to validate the replication parameters during CREATE / ALTER keyspace operations. + * All peers of current node are fetched from {@link TokenMetadata} and then a set is build by fetching DC name of each peer. + * @return a set of valid DC names + */ + private static Set<String> buildValidDataCentersSet() + { + final Set<String> validDataCenters = new HashSet<>(); + final IEndpointSnitch snitch = DatabaseDescriptor.getEndpointSnitch(); + + // Add data center of localhost. + validDataCenters.add(snitch.getDatacenter(FBUtilities.getBroadcastAddress())); + // Fetch and add DCs of all peers. + for (final InetAddress peer : StorageService.instance.getTokenMetadata().getAllEndpoints()) + { + validDataCenters.add(snitch.getDatacenter(peer)); + } + + return validDataCenters; + } + public Collection<String> recognizedOptions() { - // We explicitely allow all options - return null; + // only valid options are valid DC names. + return buildValidDataCentersSet(); + } + + protected void validateExpectedOptions() throws ConfigurationException + { + // Do not accept query with no data centers specified. + if (this.configOptions.isEmpty()) + { + throw new ConfigurationException("Configuration for at least one datacenter must be present"); + } + + // Validate the data center names + super.validateExpectedOptions(); } @Override http://git-wip-us.apache.org/repos/asf/cassandra/blob/f2c5ad74/test/unit/org/apache/cassandra/cql3/CQLTester.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/cql3/CQLTester.java b/test/unit/org/apache/cassandra/cql3/CQLTester.java index 7f5eb02..69a0b79 100644 --- a/test/unit/org/apache/cassandra/cql3/CQLTester.java +++ b/test/unit/org/apache/cassandra/cql3/CQLTester.java @@ -56,6 +56,8 @@ import org.apache.cassandra.dht.Murmur3Partitioner; import org.apache.cassandra.exceptions.ConfigurationException; import org.apache.cassandra.exceptions.SyntaxException; import org.apache.cassandra.io.util.FileUtils; +import org.apache.cassandra.locator.AbstractEndpointSnitch; +import org.apache.cassandra.locator.IEndpointSnitch; import org.apache.cassandra.serializers.TypeSerializer; import org.apache.cassandra.service.ClientState; import org.apache.cassandra.service.QueryState; @@ -82,6 +84,8 @@ public abstract class CQLTester protected static final long ROW_CACHE_SIZE_IN_MB = Integer.valueOf(System.getProperty("cassandra.test.row_cache_size_in_mb", "0")); private static final AtomicInteger seqNumber = new AtomicInteger(); protected static final ByteBuffer TOO_BIG = ByteBuffer.allocate(FBUtilities.MAX_UNSIGNED_SHORT + 1024); + public static final String DATA_CENTER = "datacenter1"; + public static final String RACK1 = "rack1"; private static org.apache.cassandra.transport.Server server; protected static final int nativePort; @@ -127,6 +131,13 @@ public abstract class CQLTester { throw new RuntimeException(e); } + // Register an EndpointSnitch which returns fixed values for test. + DatabaseDescriptor.setEndpointSnitch(new AbstractEndpointSnitch() + { + @Override public String getRack(InetAddress endpoint) { return RACK1; } + @Override public String getDatacenter(InetAddress endpoint) { return DATA_CENTER; } + @Override public int compareEndpoints(InetAddress target, InetAddress a1, InetAddress a2) { return 0; } + }); } public static ResultMessage lastSchemaChangeResult; http://git-wip-us.apache.org/repos/asf/cassandra/blob/f2c5ad74/test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java b/test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java index 0cf13bd..2b31481 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java @@ -239,16 +239,6 @@ public class SecondaryIndexTest extends CQLTester } /** - * Check one can use arbitrary name for datacenter when creating keyspace (#4278), - * migrated from cql_tests.py:TestCQL.keyspace_creation_options_test() - */ - @Test - public void testDataCenterName() throws Throwable - { - execute("CREATE KEYSPACE Foo WITH replication = { 'class' : 'NetworkTopologyStrategy', 'us-east' : 1, 'us-west' : 1 };"); - } - - /** * Migrated from cql_tests.py:TestCQL.indexes_composite_test() */ @Test http://git-wip-us.apache.org/repos/asf/cassandra/blob/f2c5ad74/test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java b/test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java index bcd6587..48108cd 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java @@ -212,13 +212,13 @@ public class AlterTest extends CQLTester row(ks1, true), row(ks2, false)); - schemaChange("ALTER KEYSPACE " + ks1 + " WITH replication = { 'class' : 'NetworkTopologyStrategy', 'dc1' : 1 } AND durable_writes=False"); + schemaChange("ALTER KEYSPACE " + ks1 + " WITH replication = { 'class' : 'NetworkTopologyStrategy', '" + DATA_CENTER + "' : 1 } AND durable_writes=False"); schemaChange("ALTER KEYSPACE " + ks2 + " WITH durable_writes=true"); assertRowsIgnoringOrderAndExtra(execute("SELECT keyspace_name, durable_writes, replication FROM system_schema.keyspaces"), row(KEYSPACE, true, map("class", "org.apache.cassandra.locator.SimpleStrategy", "replication_factor", "1")), row(KEYSPACE_PER_TEST, true, map("class", "org.apache.cassandra.locator.SimpleStrategy", "replication_factor", "1")), - row(ks1, false, map("class", "org.apache.cassandra.locator.NetworkTopologyStrategy", "dc1", "1")), + row(ks1, false, map("class", "org.apache.cassandra.locator.NetworkTopologyStrategy", DATA_CENTER , "1")), row(ks2, true, map("class", "org.apache.cassandra.locator.SimpleStrategy", "replication_factor", "1"))); execute("USE " + ks1); @@ -233,6 +233,49 @@ public class AlterTest extends CQLTester } /** + * Test {@link ConfigurationException} thrown on alter keyspace to no DC option in replication configuration. + */ + @Test + public void testAlterKeyspaceWithNoOptionThrowsConfigurationException() throws Throwable + { + // Create keyspaces + execute("CREATE KEYSPACE testABC WITH replication={ 'class' : 'NetworkTopologyStrategy', '" + DATA_CENTER + "' : 3 }"); + execute("CREATE KEYSPACE testXYZ WITH replication={ 'class' : 'SimpleStrategy', 'replication_factor' : 3 }"); + + // Try to alter the created keyspace without any option + assertInvalidThrow(ConfigurationException.class, "ALTER KEYSPACE testABC WITH replication={ 'class' : 'NetworkTopologyStrategy' }"); + assertInvalidThrow(ConfigurationException.class, "ALTER KEYSPACE testXYZ WITH replication={ 'class' : 'SimpleStrategy' }"); + + // Make sure that the alter works as expected + execute("ALTER KEYSPACE testABC WITH replication={ 'class' : 'NetworkTopologyStrategy', '" + DATA_CENTER + "' : 2 }"); + execute("ALTER KEYSPACE testXYZ WITH replication={ 'class' : 'SimpleStrategy', 'replication_factor' : 2 }"); + + // clean up + execute("DROP KEYSPACE IF EXISTS testABC"); + execute("DROP KEYSPACE IF EXISTS testXYZ"); + } + + /** + * Test {@link ConfigurationException} thrown when altering a keyspace to invalid DC option in replication configuration. + */ + @Test + public void testAlterKeyspaceWithNTSOnlyAcceptsConfiguredDataCenterNames() throws Throwable + { + // Create a keyspace with expected DC name. + execute("CREATE KEYSPACE testABC WITH replication = {'class' : 'NetworkTopologyStrategy', '" + DATA_CENTER + "' : 2 }"); + + // try modifying the keyspace + assertInvalidThrow(ConfigurationException.class, "ALTER KEYSPACE testABC WITH replication = { 'class' : 'NetworkTopologyStrategy', 'INVALID_DC' : 2 }"); + execute("ALTER KEYSPACE testABC WITH replication = {'class' : 'NetworkTopologyStrategy', '" + DATA_CENTER + "' : 3 }"); + + // Mix valid and invalid, should throw an exception + assertInvalidThrow(ConfigurationException.class, "ALTER KEYSPACE testABC WITH replication={ 'class' : 'NetworkTopologyStrategy', '" + DATA_CENTER + "' : 2 , 'INVALID_DC': 1}"); + + // clean-up + execute("DROP KEYSPACE IF EXISTS testABC"); + } + + /** * Test for bug of 5232, * migrated from cql_tests.py:TestCQL.alter_bug_test() */ http://git-wip-us.apache.org/repos/asf/cassandra/blob/f2c5ad74/test/unit/org/apache/cassandra/cql3/validation/operations/CreateTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/cql3/validation/operations/CreateTest.java b/test/unit/org/apache/cassandra/cql3/validation/operations/CreateTest.java index 33a41d8..0781169 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/operations/CreateTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/operations/CreateTest.java @@ -17,6 +17,7 @@ */ package org.apache.cassandra.cql3.validation.operations; +import java.net.InetAddress; import java.util.Collection; import java.util.Collections; import java.util.UUID; @@ -25,12 +26,15 @@ import org.junit.Test; import org.apache.cassandra.config.CFMetaData; +import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.config.Schema; import org.apache.cassandra.cql3.CQLTester; import org.apache.cassandra.db.Mutation; import org.apache.cassandra.db.partitions.Partition; import org.apache.cassandra.exceptions.ConfigurationException; import org.apache.cassandra.exceptions.SyntaxException; +import org.apache.cassandra.locator.AbstractEndpointSnitch; +import org.apache.cassandra.locator.IEndpointSnitch; import org.apache.cassandra.schema.SchemaKeyspace; import org.apache.cassandra.triggers.ITrigger; import org.apache.cassandra.utils.ByteBufferUtil; @@ -345,6 +349,33 @@ public class CreateTest extends CQLTester } /** + * Test {@link ConfigurationException} is thrown on create keyspace with invalid DC option in replication configuration . + */ + @Test + public void testCreateKeyspaceWithNTSOnlyAcceptsConfiguredDataCenterNames() throws Throwable + { + assertInvalidThrow(ConfigurationException.class, "CREATE KEYSPACE testABC WITH replication = { 'class' : 'NetworkTopologyStrategy', 'INVALID_DC' : 2 }"); + execute("CREATE KEYSPACE testABC WITH replication = {'class' : 'NetworkTopologyStrategy', '" + DATA_CENTER + "' : 2 }"); + + // Mix valid and invalid, should throw an exception + assertInvalidThrow(ConfigurationException.class, "CREATE KEYSPACE testXYZ WITH replication={ 'class' : 'NetworkTopologyStrategy', '" + DATA_CENTER + "' : 2 , 'INVALID_DC': 1}"); + + // clean-up + execute("DROP KEYSPACE IF EXISTS testABC"); + execute("DROP KEYSPACE IF EXISTS testXYZ"); + } + + /** + * Test {@link ConfigurationException} is thrown on create keyspace without any options. + */ + @Test + public void testConfigurationExceptionThrownWhenCreateKeyspaceWithNoOptions() throws Throwable + { + assertInvalidThrow(ConfigurationException.class, "CREATE KEYSPACE testXYZ with replication = { 'class': 'NetworkTopologyStrategy' }"); + assertInvalidThrow(ConfigurationException.class, "CREATE KEYSPACE testXYZ WITH replication = { 'class' : 'SimpleStrategy' }"); + } + + /** * Test create and drop table * migrated from cql_tests.py:TestCQL.table_test() */ @@ -495,6 +526,34 @@ public class CreateTest extends CQLTester } @Test + // tests CASSANDRA-4278 + public void testHyphenDatacenters() throws Throwable + { + IEndpointSnitch snitch = DatabaseDescriptor.getEndpointSnitch(); + + // Register an EndpointSnitch which returns fixed values for test. + DatabaseDescriptor.setEndpointSnitch(new AbstractEndpointSnitch() + { + @Override + public String getRack(InetAddress endpoint) { return RACK1; } + + @Override + public String getDatacenter(InetAddress endpoint) { return "us-east-1"; } + + @Override + public int compareEndpoints(InetAddress target, InetAddress a1, InetAddress a2) { return 0; } + }); + + execute("CREATE KEYSPACE Foo WITH replication = { 'class' : 'NetworkTopologyStrategy', 'us-east-1' : 1 };"); + + // Restore the previous EndpointSnitch + DatabaseDescriptor.setEndpointSnitch(snitch); + + // Clean up + execute("DROP KEYSPACE IF EXISTS Foo"); + } + + @Test // tests CASSANDRA-9565 public void testDoubleWith() throws Throwable { http://git-wip-us.apache.org/repos/asf/cassandra/blob/f2c5ad74/test/unit/org/apache/cassandra/dht/BootStrapperTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/dht/BootStrapperTest.java b/test/unit/org/apache/cassandra/dht/BootStrapperTest.java index 8974791..cd34aea 100644 --- a/test/unit/org/apache/cassandra/dht/BootStrapperTest.java +++ b/test/unit/org/apache/cassandra/dht/BootStrapperTest.java @@ -27,6 +27,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.UUID; import com.google.common.collect.Lists; @@ -59,7 +60,7 @@ public class BootStrapperTest static IPartitioner oldPartitioner; @BeforeClass - public static void setup() throws ConfigurationException + public static void setup() throws Exception { oldPartitioner = StorageService.instance.setPartitionerUnsafe(Murmur3Partitioner.instance); SchemaLoader.startGossiper(); @@ -177,6 +178,13 @@ public class BootStrapperTest int vn = 16; String ks = "BootStrapperTestNTSKeyspace" + rackCount + replicas; String dc = "1"; + + // Register peers with expected DC for NetworkTopologyStrategy. + TokenMetadata metadata = StorageService.instance.getTokenMetadata(); + metadata.clearUnsafe(); + metadata.updateHostId(UUID.randomUUID(), InetAddress.getByName("127.1.0.99")); + metadata.updateHostId(UUID.randomUUID(), InetAddress.getByName("127.15.0.99")); + SchemaLoader.createKeyspace(ks, KeyspaceParams.nts(dc, replicas, "15", 15), SchemaLoader.standardCFMD(ks, "Standard1")); TokenMetadata tm = new TokenMetadata(); tm.clearUnsafe(); http://git-wip-us.apache.org/repos/asf/cassandra/blob/f2c5ad74/test/unit/org/apache/cassandra/service/MoveTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/service/MoveTest.java b/test/unit/org/apache/cassandra/service/MoveTest.java index 53365aa..6c07a47 100644 --- a/test/unit/org/apache/cassandra/service/MoveTest.java +++ b/test/unit/org/apache/cassandra/service/MoveTest.java @@ -82,7 +82,7 @@ public class MoveTest * So instead of extending SchemaLoader, we call it's method below. */ @BeforeClass - public static void setup() throws ConfigurationException + public static void setup() throws Exception { oldPartitioner = StorageService.instance.setPartitionerUnsafe(partitioner); SchemaLoader.loadSchema(); @@ -105,7 +105,7 @@ public class MoveTest StorageService.instance.getTokenMetadata().clearUnsafe(); } - private static void addNetworkTopologyKeyspace(String keyspaceName, Integer... replicas) throws ConfigurationException + private static void addNetworkTopologyKeyspace(String keyspaceName, Integer... replicas) throws Exception { DatabaseDescriptor.setEndpointSnitch(new AbstractNetworkTopologySnitch() @@ -139,6 +139,11 @@ public class MoveTest } }); + final TokenMetadata tmd = StorageService.instance.getTokenMetadata(); + tmd.clearUnsafe(); + tmd.updateHostId(UUID.randomUUID(), InetAddress.getByName("127.0.0.1")); + tmd.updateHostId(UUID.randomUUID(), InetAddress.getByName("127.0.0.2")); + KeyspaceMetadata keyspace = KeyspaceMetadata.create(keyspaceName, KeyspaceParams.nts(configOptions(replicas)), Tables.of(CFMetaData.Builder.create(keyspaceName, "CF1")
