Cassandra 16958 #1205
Cassandra 16958 #1205
Conversation
jmckenzie-dev
force-pushed
the
cassandra-16958
branch
2 times, most recently
from
afe050f to
50c0199
Compare
Patch by Blake Eggleston; reviewed by Sam Tunnicliffe, Jason Brown, and xxxx for CASSANDRA-16958 Co-authored by: Blake Eggleston <bdeggleston@gmail.com> Co-authored by: Josh McKenzie <jmckenzie@apache.org>
jmckenzie-dev
force-pushed
the
cassandra-16958
branch
from
9665c77 to
de1aeff
Compare
| private static final Logger logger = LoggerFactory.getLogger(AuthCacheService.class); | ||
| public static final AuthCacheService instance = new AuthCacheService(); | ||
|
|
||
| private final Set<AuthCache<?, ?>> caches = new HashSet<>(); |
There was a problem hiding this comment.
A few questions around this caches state and how it's protected...
The first thing I noticed is that warmCaches() isn't synchronized, so we might be left open to a ConcurrentModificationException. Then I started looking at the actual sources of possible concurrent access to caches. It seems like the primary startup pathway isn't really a concern, but the nodetool commands for resuming bootstrap and join might be, since they hit doAuthSetup(). It that's the case, the warmCaches() issue above stands, but also only Roles.init() seems to protect us against double-adding its cache. (For AuthenticatedUser, that would be permissionsCache and networkPermissionsCache.) Then again, registering a few more things to caches on those nodetool commands might no matter, given warmCaches() is only called from CassandraDaemon#setup(). Finally, Roles.init() and PasswordAuthenticator#setup() aren't synchronized, so their cache fields aren't protected.
There was a problem hiding this comment.
...and as I finished writing that, I see that doAuthSetup() is guarded by authSetupCalled and should only happen once :)
So unless we're going to hit warmCaches() in CassandraDaemon#setup() somehow after a nodetool command, I'm not sure if there's a thread-safety issue at all. If that is possible, just throwing Collections.synchronizedSet() on AuthCacheService#caches would probably be sufficient (without synchronizing any methods here).
There was a problem hiding this comment.
I think we should protect against dupe adding from all 4 sources as well as synchronize on the init and reg call. I'll whip that up and get another commit for that; forgot to come back to this.
There was a problem hiding this comment.
Pushed a commit that hopefully addresses this. Running CI now.
There was a problem hiding this comment.
AuthCacheService is at least thread-safe now, regardless of whether or not that's strictly necessary. The only thing that's still a bit odd to me is that the nodetool commands (unless I'm misreading something) can register caches that just sort of sit in caches forever, being neither removed nor warmed.
There was a problem hiding this comment.
That... does not seem like something we want. Let me take a look.
There was a problem hiding this comment.
Ok. Taking a page out of the guard around doAuthSetup to codify the intended usage of the method.
| */ | ||
| public volatile int validation_preview_purge_head_start_in_sec = 60 * 60; | ||
|
|
||
| public boolean auth_cache_warming_enabled = true; |
There was a problem hiding this comment.
nit: Did the original Jira description mention making this opt-in?
There was a problem hiding this comment.
Hm. Nope, should be optional. Changed to false and added to cassandra.yaml
test/unit/org/apache/cassandra/tools/nodetool/InvalidateJmxPermissionsCacheTest.java
Outdated
Show resolved
Hide resolved
test/unit/org/apache/cassandra/tools/nodetool/InvalidateNetworkPermissionsCacheTest.java
Outdated
Show resolved
Hide resolved
test/unit/org/apache/cassandra/tools/nodetool/InvalidatePermissionsCacheTest.java
Outdated
Show resolved
Hide resolved
| roleManager.createRole(AuthenticatedUser.SYSTEM_USER, ROLE_A, AuthTestUtils.getLoginRoleOptions()); | ||
| roleManager.createRole(AuthenticatedUser.SYSTEM_USER, ROLE_B, AuthTestUtils.getLoginRoleOptions()); | ||
| roleManager.createRole(AuthenticatedUser.SYSTEM_USER, ROLE_A, AuthTestUtils.getLoginRoleOptions()); | ||
| roleManager.createRole(AuthenticatedUser.SYSTEM_USER, ROLE_B, AuthTestUtils.getLoginRoleOptions()); |
There was a problem hiding this comment.
Do we need to add these roles twice? The test still seems to pass if I remove the duplicates...
There was a problem hiding this comment.
Not at all. Removed.
test/unit/org/apache/cassandra/tools/nodetool/InvalidateRolesCacheTest.java
Outdated
Show resolved
Hide resolved
test/unit/org/apache/cassandra/auth/CassandraAuthorizerTest.java
Outdated
Show resolved
Hide resolved
test/unit/org/apache/cassandra/auth/CassandraAuthorizerTruncatingTests.java
Outdated
Show resolved
Hide resolved
test/unit/org/apache/cassandra/auth/CassandraRoleManagerTest.java
Outdated
Show resolved
Hide resolved
| CassandraRoleManager crm = new CassandraRoleManager(); | ||
|
|
||
| assertEquals(CassandraRoleManager.hasExistingRoles(), true); | ||
| assertEquals(crm.isClusterReady(), false); |
There was a problem hiding this comment.
nit: could use assertTrue() and assertFalse() respectively (although perhaps that's less important than throwing an explicit failure message in there)
There was a problem hiding this comment.
Fleshed out a bit and swapped to the less tautological options.
| public class PasswordAuthenticatorTest extends CQLTester | ||
| { | ||
|
|
||
| private static PasswordAuthenticator authenticator = new PasswordAuthenticator(); |
There was a problem hiding this comment.
Interesting. We had a couple PasswordAuthenticator creations in the tests that shadowed the class static variable (ew), some vestigial throws Exception, a whitespace... and this. Tidied that file up.
test/unit/org/apache/cassandra/auth/PasswordAuthenticatorTest.java
Outdated
Show resolved
Hide resolved
test/unit/org/apache/cassandra/auth/PasswordAuthenticatorTest.java
Outdated
Show resolved
Hide resolved
| private final Set<Option> alterableOptions; | ||
|
|
||
| // Will be set to true when all nodes in the cluster are on a version which supports roles (i.e. 2.2+) | ||
| private volatile boolean isClusterReady = false; |
There was a problem hiding this comment.
Can we just remove isClusterReady altogether? We won't allow upgrading directly from 2.1 -> 4.1 right?
4 participants
No description provided.