Druid basic authentication class composition config #7789
Conversation
| @@ -168,6 +211,33 @@ public static BasicAuthorizerCacheNotifier createAuthorizerCacheNotifier(final I | |||
| ); | |||
| } | |||
|
|
|||
| /** | |||
| * Returns the instance provided either by a config property or coordinator-run class or default class. | |||
There was a problem hiding this comment.
I think the semantics of this method are too complex. Maybe you can extract a method in fewer places, but they are simpler, e. g. they accept non-null parameters.
There was a problem hiding this comment.
Config based properties might be null so anyway there will be null-checks
| if (defaultClass != null) { | ||
| return injector.getInstance(defaultClass); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Not sure what does returning null mean for such method. Is it a failure? Then why not IllegalArgumentException? In any case, it would be better to avoid returning null in a simpler method.
There was a problem hiding this comment.
Null is for backward compatibility (for cases when a specific component is not needed)
There was a problem hiding this comment.
Literally, null is needed only as a return value to satisfy the signature of @Provides methods which won't be called
There was a problem hiding this comment.
Why not throw an AssertException then (and log assertionError())?
There was a problem hiding this comment.
Added the exception
|
Hmmm, integration tests failed so I believe sometimes null is expected |
|
@leventov How about using no-op implementation instead of null? There will be no null and an AssertException. The issue with the AssertException happened because the instances are registered in Lifecycle even if they are null. |
| import java.util.Collections; | ||
| import java.util.Map; | ||
|
|
||
| public class NoopBasicAuthenticatorMetadataStorageUpdater implements BasicAuthenticatorMetadataStorageUpdater |
There was a problem hiding this comment.
Please add a short doc about
- whether anybody is supposed to use this class (including via node configuration) or it's solely for use in
BasicSecurityDruidModule - What does specifically "noop" mean here - reject, allow, etc.
There was a problem hiding this comment.
- these classes should be used if it is supposed that a node does not support metadata update
- "noop" means no operation, no exception, empty values if something should be returned
There was a problem hiding this comment.
Sorry, but this doesn't make sense to me. These comments just repeat the code below. I don't actually understand what does "node supporting/not supporting authenticator metadata storage update" mean. Too many noun words in a row, and without connection to the real world.
I think it would be more understandable if was explained by example, e. g. "Druid's Xxx (broker/coordinator/router/historical/whatever) nodes use this because when requests come from them from nodes Yyy they are already authorized, so these nodes don't need to send delete/create users"... - something like this, a small story. Could you please rewrite the text like this?
The problem stems from BasicAuthenticatorMetadataStorageUpdater itself which class-level Javadoc comment just repeats the code without giving understanding. Could you please upgrade its Javadoc? (Then Javadocs for Noop classes can be shorter and refer to BasicAuthenticatorMetadataStorageUpdater instead.)
There was a problem hiding this comment.
NoopBasicAuthenticatorMetadataStorageUpdater was developed only to remove nulls from BasicSecurityDruidModule with the assumption that its methods won't be called. I don't think the class would be useful except for this case.
It is better to remove the comment The class is needed if a node should not support authenticator metadata storage update because it is not correct in general. But again I don't know how to use this class except how it is used now.
There was a problem hiding this comment.
Consider this optional, but I still don't understand what do *AuthenticatorMetadataStorageUpdater classes do, reading through their Javadocs. To me, their Javadocs are currently not reaching their goal of explaining things.
The config is an option to specify classes of user/role managers, caches and notifiers.
If a config field is specified then the corresponding class is instantiated regardless of what type of Druid component runs it.