cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sam Tunnicliffe (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CASSANDRA-10552) Pluggable IResources
Date Tue, 20 Oct 2015 11:43:27 GMT

    [ https://issues.apache.org/jira/browse/CASSANDRA-10552?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14965002#comment-14965002
] 

Sam Tunnicliffe commented on CASSANDRA-10552:
---------------------------------------------

I can see that making {{IResource}} a more accessible extension point might have benefits
for integration. What this patch alone won't do is provide a means to manage permissions on
custom resources via CQL. As it stands, the grammar only permits the existing implementations
in {{GRANT}} and {{REVOKE}} statements. That could be extended to support something like {{GRANT
ALL ON CUSTOM RESOURCE foos/foo1 TO <role>}}, otherwise permissions will only be able
to be granted/revoked programatically.

CQL syntax aside, there's also a bug in the patch. Because {{IResource}} implementations are
lazily registered, attempting to create one from its representation in the system table will
fail if the class hasn't already been loaded. A simple repro is to login as the default super
user & create a keyspace, then restart C*, login again & run {{LIST ALL PERMISSIONS}}.
The permissions representing data resources are ok as the class is referenced in the static
initializer of {{ClientState}}, but {{FunctionResource}} hasn't been registered yet and so
an {{IllegalArgumentException}} is thrown. For this to work, we'll need some means of discovering
all of the available IResource implementations at startup and ensuring they're registered.
While we're doing that, it could be worth adding a {{StartupCheck}} to ensure that if auth
is enabled, there's a registered factory for every resource listed in {{system_auth.role_permissions}},
in case a jar containing a custom implementation is missing etc. The check could also verify
that there are no conflicts on resource prefixes.

I'd also like to formalize the structure of resource names a little, to make the registration
& factory stuff a little bit cleaner. Extracting the {{/}} separator out into a constant
on {{IResource}} would let us split out the root element from a resource name & look up
the factory for a given name directly, rather than iterating & checking {{startsWith}}.
It would also enforce a bit more structure & consistency on custom implementations. It
would also be good to add something to the class level javadoc for {{IResource}} which explains
about registration & details the requirement for a static initializer etc I notice that
that doc is already outdated as it asserts that {{DataResource}} is the only implementation,
that's my bad so I'll update that on commit.

Lastly, it would be nice to get some test coverage for this. Aside from the discovery bug,
most regressions should be covered by the existing dtests, but I think it'll be possible to
add a unit test which exercises custom resource impls. This would be valuable not least because
it would require providing custom resource impl, which may highlight assumptions we've previously
made, particularly in the case of CQL syntax etc.

> Pluggable IResources
> --------------------
>
>                 Key: CASSANDRA-10552
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-10552
>             Project: Cassandra
>          Issue Type: Improvement
>          Components: Core
>            Reporter: Anthony Cozzie
>            Assignee: Anthony Cozzie
>             Fix For: 3.0.0
>
>         Attachments: cassandra-3.0.0-10552.txt
>
>
> It is impossible to add new IResources because of the static method Resources.fromName(),
which creates IResources from the text values in the authentication tables.  This patch replaces
the static list of checks with a hash table that can be extended.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message