[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Vadim Spector updated SENTRY-1508:
----------------------------------
Description:
MetastorePlugin.java constructor initializes successfully regardless of whether
initThread.run() was successful. E.g. if cacheInitializer.createInitialUpdate()
cal inside the constructor fails (in one case in the field it was due to an
invalid table uri in HMS database, whose parsing produced
ArrayIndexOutOfBoundsException), initComplete is still assigned to true and
MetastorePlugin gets constructed. While initError is assigned to non-null value
in case of failure, it is effectively ignored as will be described later.
MetastorePlugin implements several callbacks to be called on the Hive side, to
update Sentry state. It is critical functionality for enforcing access control.
Its callbacks (addPath(), removeAllPaths(), etc) all end up calling
notifySentryAndApplyLocal() which starts with the following code:
if (initComplete) {
processUpdate()
}...
which means that processUpdate() is called even when MetastorePlugin
initialization has failed (i.e. initError == true). There is an option of
asynchronous initialization, when MetastorePlugin is constructed and returned
to the caller before initialization is complete, but the code makes no
distinction between the two cases, as will be shown below.
processUpdate() called regardless of whether MetastorePlugin a) has been
initialized (sync mode, success), or b) still being initialized (async mode),
or c) failed to initialized (either mode). It calls (unconditionally)
applyLocal() and notifySentry(). aplyLocal() calls authzPaths.updatePartial()
which immediately fails with NullPointerException if authzPaths == null which
happens if initThread within the constructor fails.
SENTRY-1270 avoids NullPointerException by execuitng this before dereferencing
authzPaths inside applyLocal():
if(authzPaths == null) {
LOGGER.error(initializationFailureMsg);
return;
}
However, this leads to hard-to-diagnose behavior, because a) local updates fail
forever, and b) housekeeping thread running SyncTask to synchronize with
Sentry-side state fails as well, printing endlessly misleading "#### Metastore
Plugin cache has not finished initialization." message suggesting its temporary
condition as opposed to a permanent failure. Failure to sync up with Sentry
state can lead to very subtle, often intermittent, problems with acls.
MetastorePlugin and its caller never get fully aware of the permanent nature of
init failure.
Suggestion:
a) define 3 states in MetastorePlugin: initializing, initialized, init_failed.
2) communicate those states to the caller clearly:
a) for state == initializing, throw some kind of InitInProgressException.
This only applies for asynchronous initialization configuration. It is unknown
whether it is actually deployed anywhere, but the code exists and there is no
reason why not to support it. Async init makes sense as an optimization, as
long as initialization-in-progress condition is clearly communicated.
b) for state == init_failed throw InitializationFailedException of some
sort. For usability, it should contain the cause - the original exception
originating either from the constructor or from the initThread.run() started
from the constructor (in sync or async modes alike).
Although in the sync mode initialization failure could be detected easily by
constructor failure, we may want to preserve the current design, in which
constructor always succeeds, so the client code does not need to be aware of
whether init mode is configured as sync vs async.
Additional cleanup:
getSentry() is always dereferenced, while it can actually return null
(exception is swallowed and error is logged). Suggested fix: getClient() may
still print error message, but it should also -re-throw an original exception -
it is much easier to debug than dealing with inevitable NullPointerException up
the caller stack.
General rule - error messages are to help with diagnostic, not to replace
throwing exception, which is the only way to make sure error conditions are
properly propagated to the caller.
Each code fork deserves log message: e.g. addPath() retrurns immediately if
PathsUpdate.parsePath() returns null - w/o printing any log.
was:
MetastorePlugin.java constructor initializes successfully regardless of whether
initThread.run() was successful. E.g. if cacheInitializer.createInitialUpdate()
cal inside the constructor fails (in one case in the field it was due to an
invalid table uri in HMS database, whose parsing produced
ArrayOutOfBoundsException), initComplete is still assigned to true and
MetastorePlugin gets constructed. While initError is assigned to non-null value
in case of failure, it is effectively ignored as will be described later.
MetastorePlugin implements several callbacks to be called on the Hive side, to
update Sentry state. It is critical functionality for enforcing access control.
Its callbacks (addPath(), removeAllPaths(), etc) all end up calling
notifySentryAndApplyLocal() which starts with the following code:
if (initComplete) {
processUpdate()
}...
which means that processUpdate() is called even when MetastorePlugin
initialization has failed (i.e. initError == true). There is an option of
asynchronous initialization, when MetastorePlugin is constructed and returned
to the caller before initialization is complete, but the code makes no
distinction between the two cases, as will be shown below.
processUpdate() called regardless of whether MetastorePlugin a) has been
initialized (sync mode, success), or b) still being initialized (async mode),
or c) failed to initialized (either mode). It calls (unconditionally)
applyLocal() and notifySentry(). aplyLocal() calls authzPaths.updatePartial()
which immediately fails with NullPointerException if authzPaths == null which
happens if initThread within the constructor fails.
SENTRY-1270 avoids NullPointerException by execuitng this before dereferencing
authzPaths inside applyLocal():
if(authzPaths == null) {
LOGGER.error(initializationFailureMsg);
return;
}
However, this leads to hard-to-diagnose behavior, because a) local updates fail
forever, and b) housekeeping thread running SyncTask to synchronize with
Sentry-side state fails as well, printing endlessly misleading "#### Metastore
Plugin cache has not finished initialization." message suggesting its temporary
condition as opposed to a permanent failure. Failure to sync up with Sentry
state can lead to very subtle, often intermittent, problems with acls.
MetastorePlugin and its caller never get fully aware of the permanent nature of
init failure.
Suggestion:
a) define 3 states in MetastorePlugin: initializing, initialized, init_failed.
2) communicate those states to the caller clearly:
a) for state == initializing, throw some kind of InitInProgressException.
This only applies for asynchronous initialization configuration. It is unknown
whether it is actually deployed anywhere, but the code exists and there is no
reason why not to support it. Async init makes sense as an optimization, as
long as initialization-in-progress condition is clearly communicated.
b) for state == init_failed throw InitializationFailedException of some
sort. For usability, it should contain the cause - the original exception
originating either from the constructor or from the initThread.run() started
from the constructor (in sync or async modes alike).
Although in the sync mode initialization failure could be detected easily by
constructor failure, we may want to preserve the current design, in which
constructor always succeeds, so the client code does not need to be aware of
whether init mode is configured as sync vs async.
Additional cleanup:
getSentry() is always dereferenced, while it can actually return null
(exception is swallowed and error is logged). Suggested fix: getClient() may
still print error message, but it should also -re-throw an original exception -
it is much easier to debug than dealing with inevitable NullPointerException up
the caller stack.
General rule - error messages are to help with diagnostic, not to replace
throwing exception, which is the only way to make sure error conditions are
properly propagated to the caller.
Each code fork deserves log message: e.g. addPath() retrurns immediately if
PathsUpdate.parsePath() returns null - w/o printing any log.
> MetastorePlugin.java does not handle properly initialization failure
> --------------------------------------------------------------------
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
> Reporter: Vadim Spector
> Priority: Critical
>
> MetastorePlugin.java constructor initializes successfully regardless of
> whether initThread.run() was successful. E.g. if
> cacheInitializer.createInitialUpdate() cal inside the constructor fails (in
> one case in the field it was due to an invalid table uri in HMS database,
> whose parsing produced ArrayIndexOutOfBoundsException), initComplete is still
> assigned to true and MetastorePlugin gets constructed. While initError is
> assigned to non-null value in case of failure, it is effectively ignored as
> will be described later.
> MetastorePlugin implements several callbacks to be called on the Hive side,
> to update Sentry state. It is critical functionality for enforcing access
> control. Its callbacks (addPath(), removeAllPaths(), etc) all end up calling
> notifySentryAndApplyLocal() which starts with the following code:
> if (initComplete) {
> processUpdate()
> }...
> which means that processUpdate() is called even when MetastorePlugin
> initialization has failed (i.e. initError == true). There is an option of
> asynchronous initialization, when MetastorePlugin is constructed and returned
> to the caller before initialization is complete, but the code makes no
> distinction between the two cases, as will be shown below.
> processUpdate() called regardless of whether MetastorePlugin a) has been
> initialized (sync mode, success), or b) still being initialized (async mode),
> or c) failed to initialized (either mode). It calls (unconditionally)
> applyLocal() and notifySentry(). aplyLocal() calls authzPaths.updatePartial()
> which immediately fails with NullPointerException if authzPaths == null which
> happens if initThread within the constructor fails.
> SENTRY-1270 avoids NullPointerException by execuitng this before
> dereferencing authzPaths inside applyLocal():
> if(authzPaths == null) {
> LOGGER.error(initializationFailureMsg);
> return;
> }
> However, this leads to hard-to-diagnose behavior, because a) local updates
> fail forever, and b) housekeeping thread running SyncTask to synchronize with
> Sentry-side state fails as well, printing endlessly misleading "####
> Metastore Plugin cache has not finished initialization." message suggesting
> its temporary condition as opposed to a permanent failure. Failure to sync up
> with Sentry state can lead to very subtle, often intermittent, problems with
> acls. MetastorePlugin and its caller never get fully aware of the permanent
> nature of init failure.
> Suggestion:
> a) define 3 states in MetastorePlugin: initializing, initialized, init_failed.
> 2) communicate those states to the caller clearly:
> a) for state == initializing, throw some kind of InitInProgressException.
> This only applies for asynchronous initialization configuration. It is
> unknown whether it is actually deployed anywhere, but the code exists and
> there is no reason why not to support it. Async init makes sense as an
> optimization, as long as initialization-in-progress condition is clearly
> communicated.
> b) for state == init_failed throw InitializationFailedException of some
> sort. For usability, it should contain the cause - the original exception
> originating either from the constructor or from the initThread.run() started
> from the constructor (in sync or async modes alike).
> Although in the sync mode initialization failure could be detected easily by
> constructor failure, we may want to preserve the current design, in which
> constructor always succeeds, so the client code does not need to be aware of
> whether init mode is configured as sync vs async.
> Additional cleanup:
> getSentry() is always dereferenced, while it can actually return null
> (exception is swallowed and error is logged). Suggested fix: getClient() may
> still print error message, but it should also -re-throw an original exception
> - it is much easier to debug than dealing with inevitable
> NullPointerException up the caller stack.
> General rule - error messages are to help with diagnostic, not to replace
> throwing exception, which is the only way to make sure error conditions are
> properly propagated to the caller.
> Each code fork deserves log message: e.g. addPath() retrurns immediately if
> PathsUpdate.parsePath() returns null - w/o printing any log.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
