diff --git a/CHANGES.txt b/CHANGES.txt index 75e7d2ace9..7a5d73a622 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -89,6 +89,7 @@ Merged from 3.0: * Calculate last compacted key on startup (CASSANDRA-6216) * Add schema to snapshot manifest, add USING TIMESTAMP clause to ALTER TABLE statements (CASSANDRA-7190) * 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: * Make Collections deserialization more robust (CASSANDRA-12618) * Fix exceptions when enabling gossip on nodes that haven't joined the ring (CASSANDRA-12253) diff --git a/NEWS.txt b/NEWS.txt index ad0f2be1ab..9ab7c26fc6 100644 --- a/NEWS.txt +++ b/NEWS.txt @@ -98,6 +98,9 @@ Upgrading - Application layer keep-alives were added to the streaming protocol to prevent idle incoming connections from timing out and failing the stream session (CASSANDRA-11839). This effectively deprecates the streaming_socket_timeout_in_ms property in favor of streaming_keep_alive_period_in_secs. See cassandra.yaml for more details about this property. + - Cassandra will no longer allow invalid keyspace replication options, such as invalid datacenter names for + NetworkTopologyStrategy. Existing keyspaces will continue to operate, but CREATE and ALTER will validate that + all datacenters specified exist in the cluster. 3.8 === diff --git a/src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java b/src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java index 038ac9fc5a..4ba1da82b9 100644 --- a/src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java +++ b/src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java @@ -320,7 +320,7 @@ public abstract class AbstractReplicationStrategy } } - private void validateExpectedOptions() throws ConfigurationException + protected void validateExpectedOptions() throws ConfigurationException { Collection expectedOptions = recognizedOptions(); if (expectedOptions == null) diff --git a/src/java/org/apache/cassandra/locator/NetworkTopologyStrategy.java b/src/java/org/apache/cassandra/locator/NetworkTopologyStrategy.java index 756b6893d7..442e6cf2bd 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 org.apache.cassandra.utils.Pair; @@ -214,6 +216,45 @@ public class NetworkTopologyStrategy extends AbstractReplicationStrategy return datacenters.keySet(); } + /* + * (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 buildValidDataCentersSet() + { + final Set 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 recognizedOptions() + { + // 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(); + } + public void validateOptions() throws ConfigurationException { for (Entry e : this.configOptions.entrySet()) diff --git a/test/unit/org/apache/cassandra/cql3/CQLTester.java b/test/unit/org/apache/cassandra/cql3/CQLTester.java index 3bb753fcc9..29d51a8c9a 100644 --- a/test/unit/org/apache/cassandra/cql3/CQLTester.java +++ b/test/unit/org/apache/cassandra/cql3/CQLTester.java @@ -57,6 +57,7 @@ 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.serializers.TypeSerializer; import org.apache.cassandra.service.ClientState; import org.apache.cassandra.service.QueryState; @@ -83,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; @@ -115,6 +118,14 @@ public abstract class CQLTester nativeAddr = InetAddress.getLoopbackAddress(); + // 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; } + }); + try { try (ServerSocket serverSocket = new ServerSocket(0)) 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 5cb76c9134..5af3ebb994 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java @@ -245,16 +245,6 @@ public class SecondaryIndexTest extends CQLTester tableName)); } - /** - * 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() */ 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 c9be6781b4..672b6eba3f 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); @@ -232,6 +232,49 @@ public class AlterTest extends CQLTester "max_threshold", "32"))); } + /** + * 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() 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 da0824fed2..4957b18f36 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; @@ -24,6 +25,7 @@ import java.util.UUID; 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.config.SchemaConstants; import org.apache.cassandra.cql3.CQLTester; @@ -31,6 +33,8 @@ 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; @@ -344,6 +348,33 @@ public class CreateTest extends CQLTester execute("DROP KEYSPACE testXYZ"); } + /** + * 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() @@ -494,6 +525,34 @@ public class CreateTest extends CQLTester assertRows(execute("SELECT * FROM %s WHERE b = ?", 4), row(2, 4)); } + @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 diff --git a/test/unit/org/apache/cassandra/dht/BootStrapperTest.java b/test/unit/org/apache/cassandra/dht/BootStrapperTest.java index 7662faa0f3..715f7dd38b 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; @@ -178,6 +179,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(); diff --git a/test/unit/org/apache/cassandra/service/MoveTest.java b/test/unit/org/apache/cassandra/service/MoveTest.java index 05757c0c49..e8d5ccde9a 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 { DatabaseDescriptor.daemonInitialization(); oldPartitioner = StorageService.instance.setPartitionerUnsafe(partitioner); @@ -106,7 +106,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() @@ -140,6 +140,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")