You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "aratno (via GitHub)" <gi...@apache.org> on 2024/11/22 21:43:25 UTC
[PR] CASSANDRA-19385: ALTER ROLE WITH LOGIN=FALSE and REVOKE ROLE do not disconnect existing users [cassandra]
aratno opened a new pull request, #3706:
URL: https://github.com/apache/cassandra/pull/3706
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19385: ALTER ROLE WITH LOGIN=FALSE and REVOKE ROLE do not disconnect existing users [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #3706:
URL: https://github.com/apache/cassandra/pull/3706#discussion_r1854759039
##########
src/java/org/apache/cassandra/auth/CassandraRoleManager.java:
##########
@@ -301,6 +306,7 @@ public void dropRole(AuthenticatedUser performer, RoleResource role) throws Requ
consistencyForRoleWrite(role.getRoleName()));
removeAllMembers(role.getRoleName());
removeAllIdentitiesOfRole(role.getRoleName());
+ disconnect(role);
Review Comment:
I think I'll remove ALTER-coordinator disconnects here, seems better to have all disconnects happen on the jitter schedule (particularly for smaller clusters)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19385: ALTER ROLE WITH LOGIN=FALSE and REVOKE ROLE do not disconnect existing users [cassandra]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #3706:
URL: https://github.com/apache/cassandra/pull/3706#discussion_r1883965722
##########
src/java/org/apache/cassandra/auth/CassandraRoleManager.java:
##########
@@ -732,4 +737,32 @@ public Supplier<Map<RoleResource, Set<Role>>> bulkLoader()
return entries;
};
}
+
+ protected void scheduleDisconnectInvalidRoleTask()
+ {
+ // Delay between executions, with jitter
+ int period = (int) DatabaseDescriptor.getInvalidClientDisconnectPeriod(TimeUnit.MILLISECONDS);
+ int jitter = (int) DatabaseDescriptor.getInvalidClientDisconnectJitter(TimeUnit.MILLISECONDS);
Review Comment:
use long for period + jitter
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -5349,4 +5349,26 @@ public static boolean isPasswordValidatorReconfigurationEnabled()
{
return conf.password_validator_reconfiguration_enabled;
}
+
+ public static long getInvalidClientDisconnectPeriod(TimeUnit unit)
+ {
+ return conf.invalid_client_disconnect_period.to(unit);
+ }
+
+ public static void setInvalidClientDisconnectPeriod(TimeUnit unit, long duration)
+ {
+ logger.info("invalid_client_disconnect_period set to {} {}", duration, unit);
+ conf.invalid_client_disconnect_period = new DurationSpec.LongMillisecondsBound(duration, unit);
+ }
+
+ public static long getInvalidClientDisconnectJitter(TimeUnit unit)
+ {
+ return conf.invalid_client_disconnect_jitter.to(unit);
+ }
+
+ public static void setInvalidClientDisconnectJitter(TimeUnit unit, long duration)
+ {
+ logger.info("invalid_client_disconnect_jitter set to {} {}", duration, unit);
Review Comment:
same here.
##########
src/java/org/apache/cassandra/auth/CassandraRoleManager.java:
##########
@@ -732,4 +737,32 @@ public Supplier<Map<RoleResource, Set<Role>>> bulkLoader()
return entries;
};
}
+
+ protected void scheduleDisconnectInvalidRoleTask()
+ {
+ // Delay between executions, with jitter
+ int period = (int) DatabaseDescriptor.getInvalidClientDisconnectPeriod(TimeUnit.MILLISECONDS);
+ int jitter = (int) DatabaseDescriptor.getInvalidClientDisconnectJitter(TimeUnit.MILLISECONDS);
+ if (period < 0)
+ {
+ logger.info("Skipping invalid role disconnection");
+ return;
+ }
+ IntSupplier delayMillis = () -> period + ThreadLocalRandom.current().nextInt(0, jitter);
Review Comment:
This should be a LongSupplier. In general, delay should be supplied as long, for consistency we should stick to long here as well.
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -5349,4 +5349,26 @@ public static boolean isPasswordValidatorReconfigurationEnabled()
{
return conf.password_validator_reconfiguration_enabled;
}
+
+ public static long getInvalidClientDisconnectPeriod(TimeUnit unit)
+ {
+ return conf.invalid_client_disconnect_period.to(unit);
+ }
+
+ public static void setInvalidClientDisconnectPeriod(TimeUnit unit, long duration)
Review Comment:
are we expecting to update these values from somewhere? I don't see any call sites for this method
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -5349,4 +5349,26 @@ public static boolean isPasswordValidatorReconfigurationEnabled()
{
return conf.password_validator_reconfiguration_enabled;
}
+
+ public static long getInvalidClientDisconnectPeriod(TimeUnit unit)
+ {
+ return conf.invalid_client_disconnect_period.to(unit);
+ }
+
+ public static void setInvalidClientDisconnectPeriod(TimeUnit unit, long duration)
+ {
+ logger.info("invalid_client_disconnect_period set to {} {}", duration, unit);
Review Comment:
I find it useful to know what was the previous value as well during debugging, so if we could include that, it'd be great. i.e. `invalid_client_disconnect_period set from <previous> to <new>`
##########
src/java/org/apache/cassandra/auth/CassandraRoleManager.java:
##########
@@ -732,4 +737,32 @@ public Supplier<Map<RoleResource, Set<Role>>> bulkLoader()
return entries;
};
}
+
+ protected void scheduleDisconnectInvalidRoleTask()
+ {
+ // Delay between executions, with jitter
+ int period = (int) DatabaseDescriptor.getInvalidClientDisconnectPeriod(TimeUnit.MILLISECONDS);
Review Comment:
I see methods to set these properties (period+jitter), however if you statically retrieve the value once, setting them anywhere else will have no effect. So if the desire is to be able to have some mechanism to update these values while the process is running (i.e. JMX), we should probably retrieve these values dynamically as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19385: ALTER ROLE WITH LOGIN=FALSE and REVOKE ROLE do not disconnect existing users [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #3706:
URL: https://github.com/apache/cassandra/pull/3706#discussion_r1901092498
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -4969,6 +4969,13 @@ public void clearConnectionHistory()
daemon.clearConnectionHistory();
logger.info("Cleared connection history");
}
+
+ public void disconnectInvalidRoles()
+ {
+ logger.info("Disconnecting invalid roles");
Review Comment:
I was going to keep this one, it should log very infrequently. Most folks don't enable debug logging unless they see something go wrong, so an indication that the job is running would be helpful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19385: ALTER ROLE WITH LOGIN=FALSE and REVOKE ROLE do not disconnect existing users [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic closed pull request #3706: CASSANDRA-19385: ALTER ROLE WITH LOGIN=FALSE and REVOKE ROLE do not disconnect existing users
URL: https://github.com/apache/cassandra/pull/3706
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19385: ALTER ROLE WITH LOGIN=FALSE and REVOKE ROLE do not disconnect existing users [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #3706:
URL: https://github.com/apache/cassandra/pull/3706#discussion_r1901094870
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -5349,4 +5349,28 @@ public static boolean isPasswordValidatorReconfigurationEnabled()
{
return conf.password_validator_reconfiguration_enabled;
}
+
+ public static long getInvalidClientDisconnectPeriod(TimeUnit unit)
+ {
+ return conf.invalid_client_disconnect_period.to(unit);
+ }
+
+ public static void setInvalidClientDisconnectPeriod(TimeUnit unit, long duration)
+ {
+ long prevDuration = conf.invalid_client_disconnect_period.to(unit);
+ logger.info("invalid_client_disconnect_period set from {} to {}", prevDuration, duration == 0 ? "disabled" : String.format("%s %s", duration, unit));
Review Comment:
This would render as "disabled (MILLISECONDS)" though. This is going to log super infrequently, so clarity feels more important than performance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19385: ALTER ROLE WITH LOGIN=FALSE and REVOKE ROLE do not disconnect existing users [cassandra]
Posted by "bbotella (via GitHub)" <gi...@apache.org>.
bbotella commented on PR #3706:
URL: https://github.com/apache/cassandra/pull/3706#issuecomment-2568229881
+1 (nb)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19385: ALTER ROLE WITH LOGIN=FALSE and REVOKE ROLE do not disconnect existing users [cassandra]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #3706:
URL: https://github.com/apache/cassandra/pull/3706#discussion_r1894316030
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -4969,6 +4969,13 @@ public void clearConnectionHistory()
daemon.clearConnectionHistory();
logger.info("Cleared connection history");
}
+
+ public void disconnectInvalidRoles()
+ {
+ logger.info("Disconnecting invalid roles");
Review Comment:
maybe move this to debug?
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -5349,4 +5349,28 @@ public static boolean isPasswordValidatorReconfigurationEnabled()
{
return conf.password_validator_reconfiguration_enabled;
}
+
+ public static long getInvalidClientDisconnectPeriod(TimeUnit unit)
+ {
+ return conf.invalid_client_disconnect_period.to(unit);
+ }
+
+ public static void setInvalidClientDisconnectPeriod(TimeUnit unit, long duration)
+ {
+ long prevDuration = conf.invalid_client_disconnect_period.to(unit);
+ logger.info("invalid_client_disconnect_period set from {} to {}", prevDuration, duration == 0 ? "disabled" : String.format("%s %s", duration, unit));
Review Comment:
NIT to avoid the string concat, although we don't save much here
```suggestion
logger.info("invalid_client_disconnect_period set from {} to {} ({})", prevDuration, duration == 0 ? "disabled" : duration, unit);
```
The unit shows all uppercase but that's fine, I don't want to do a lowercase here.
##########
src/java/org/apache/cassandra/auth/CassandraRoleManager.java:
##########
@@ -732,4 +747,76 @@ public Supplier<Map<RoleResource, Set<Role>>> bulkLoader()
return entries;
};
}
+
+ protected void disconnectInvalidRoles()
+ {
+ // This should always run with jitter, otherwise there's a risk that all nodes disconnect clients at the same time
+ StorageService.instance.disconnectInvalidRoles();
+ }
+
+ protected void invalidRoleDisconnectTask(LongSupplier delayMillis, ScheduledExecutorService executor)
+ {
+ try
+ {
+ disconnectInvalidRoles();
+ }
+ catch (Exception e)
+ {
+ logger.warn("Failed to disconnect invalid roles", e);
+ }
+
+ long nextDelayMillis = delayMillis.getAsLong();
+ logger.info("Scheduling next invalid role disconnection in {} millis", nextDelayMillis);
+ this.invalidRoleDisconnectTask = executor.schedule(() -> invalidRoleDisconnectTask(delayMillis, executor), nextDelayMillis, TimeUnit.MILLISECONDS);
+ }
+
+ protected void scheduleDisconnectInvalidRoleTask()
+ {
+ // Cancel any pending execution if it exists, since we may have changed period / jitter parameters
+ if (this.invalidRoleDisconnectTask != null)
+ {
+ logger.debug("Canceling previous invalidRoleDisconnectTask");
+ this.invalidRoleDisconnectTask.cancel(true);
+ }
+
+ long period = getInvalidClientDisconnectPeriodMillis();
+ long jitter = getInvalidClientDisconnectJitterMillis();
+ if (period <= 0)
+ {
+ logger.info("Invalid role disconnection is disabled");
+ return;
+ }
+ LongSupplier delayMillis = () -> period + ThreadLocalRandom.current().nextLong(0, jitter);
+ long firstDelayMillis = delayMillis.getAsLong();
+ ScheduledExecutorPlus executor = ScheduledExecutors.optionalTasks;
+
+ logger.info("Scheduling first invalid role disconnection in {} millis", firstDelayMillis);
Review Comment:
should we move to debug level?
##########
src/java/org/apache/cassandra/auth/CassandraRoleManagerMBean.java:
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.auth;
+
+public interface CassandraRoleManagerMBean
Review Comment:
can we add javadocs on public interfaces and its methods?
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -5349,4 +5349,28 @@ public static boolean isPasswordValidatorReconfigurationEnabled()
{
return conf.password_validator_reconfiguration_enabled;
}
+
+ public static long getInvalidClientDisconnectPeriod(TimeUnit unit)
+ {
+ return conf.invalid_client_disconnect_period.to(unit);
+ }
+
+ public static void setInvalidClientDisconnectPeriod(TimeUnit unit, long duration)
+ {
+ long prevDuration = conf.invalid_client_disconnect_period.to(unit);
+ logger.info("invalid_client_disconnect_period set from {} to {}", prevDuration, duration == 0 ? "disabled" : String.format("%s %s", duration, unit));
+ conf.invalid_client_disconnect_period = new DurationSpec.LongMillisecondsBound(duration, unit);
+ }
+
+ public static long getInvalidClientDisconnectJitter(TimeUnit unit)
+ {
+ return conf.invalid_client_disconnect_jitter.to(unit);
+ }
+
+ public static void setInvalidClientDisconnectJitter(TimeUnit unit, long duration)
+ {
+ long prevDuration = conf.invalid_client_disconnect_jitter.to(unit);
+ logger.info("invalid_client_disconnect_jitter set from {} to {} {}", prevDuration, duration, unit);
Review Comment:
NIT
```suggestion
logger.info("invalid_client_disconnect_jitter set from {} to {} ({})", prevDuration, duration, unit);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19385: ALTER ROLE WITH LOGIN=FALSE and REVOKE ROLE do not disconnect existing users [cassandra]
Posted by "bbotella (via GitHub)" <gi...@apache.org>.
bbotella commented on code in PR #3706:
URL: https://github.com/apache/cassandra/pull/3706#discussion_r1901151341
##########
src/java/org/apache/cassandra/auth/CassandraRoleManager.java:
##########
@@ -732,4 +747,76 @@ public Supplier<Map<RoleResource, Set<Role>>> bulkLoader()
return entries;
};
}
+
+ protected void disconnectInvalidRoles()
+ {
+ // This should always run with jitter, otherwise there's a risk that all nodes disconnect clients at the same time
+ StorageService.instance.disconnectInvalidRoles();
+ }
+
+ protected void invalidRoleDisconnectTask(LongSupplier delayMillis, ScheduledExecutorService executor)
+ {
+ try
+ {
+ disconnectInvalidRoles();
+ }
+ catch (Exception e)
+ {
+ logger.warn("Failed to disconnect invalid roles", e);
+ }
+
+ long nextDelayMillis = delayMillis.getAsLong();
+ logger.info("Scheduling next invalid role disconnection in {} millis", nextDelayMillis);
+ this.invalidRoleDisconnectTask = executor.schedule(() -> invalidRoleDisconnectTask(delayMillis, executor), nextDelayMillis, TimeUnit.MILLISECONDS);
+ }
+
+ protected void scheduleDisconnectInvalidRoleTask()
+ {
+ // Cancel any pending execution if it exists, since we may have changed period / jitter parameters
+ if (this.invalidRoleDisconnectTask != null)
+ {
+ logger.debug("Canceling previous invalidRoleDisconnectTask");
+ this.invalidRoleDisconnectTask.cancel(true);
+ }
+
+ long period = getInvalidClientDisconnectPeriodMillis();
+ long jitter = getInvalidClientDisconnectJitterMillis();
Review Comment:
Nit: Maybe get Jitter after the `<=` check for period?
##########
test/unit/org/apache/cassandra/auth/CassandraRoleManagerTest.java:
##########
@@ -160,4 +168,38 @@ private void assertRoleSet(Set<Role> actual, RoleResource...expected)
for (RoleResource expectedRole : expected)
assertTrue(actual.stream().anyMatch(role -> role.resource.equals(expectedRole)));
}
+
+ @Test
+ public void example() throws InterruptedException
Review Comment:
Maybe find a more descriptive name? Something like `numDisconnectAttempsTest`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org