[jira] [Updated] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure

["Vadim Spector (JIRA)" <ji...@apache.org>]

     [ 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)