> On March 12, 2018, 11:04 p.m., Sergio Pena wrote: > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java > > Lines 354-359 (patched) > > <https://reviews.apache.org/r/65866/diff/2/?file=1968479#file1968479line360> > > > > Why was it moved? Didn't make sense to have input and output privileges > > logged before checking for authorization?
I moved it so it will be easier to isoloate if hasAccess() was caused by input or output privileges. Before they were both logged first and then we checked hasAccess for input and output. Rather we should just log input privileges -> check hasAccess for input -> log output privileges -> check hasAccess for output > On March 12, 2018, 11:04 p.m., Sergio Pena wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java > > Lines 285-287 (patched) > > <https://reviews.apache.org/r/65866/diff/2/?file=1968480#file1968480line285> > > > > is the LOG.isDebugEnabled() necessary? The LOG.debug() arguments are > > not doing any intensitve to have this validation, aren't they? > > > > And, what would 'this' print in the log? > > > > Should'nt this log being at the beginning of the method? this will print all HiveAuthzBindingHook class attribute values. There is a toString() method I created at the bottom. It is logged at the end because the class attributes like currDB, currTab are all set before this > On March 12, 2018, 11:04 p.m., Sergio Pena wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java > > Lines 304-306 (patched) > > <https://reviews.apache.org/r/65866/diff/2/?file=1968480#file1968480line304> > > > > Same question about the LOG.isDebugEnabled(). Doesn't hurt to have isDebugEnabled. I wasn't sure of the lenght of the toString output of either stmtOperation, stmtAuthObject, or subject > On March 12, 2018, 11:04 p.m., Sergio Pena wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java > > Line 343 (original) > > <https://reviews.apache.org/r/65866/diff/2/?file=1968481#file1968481line343> > > > > Why was it removed? isn't it needed anymore? It was removed because with new logging changes it is logged by the caller of this method. So having it twice would seem redundant > On March 12, 2018, 11:04 p.m., Sergio Pena wrote: > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java > > Line 39 (original), 42 (patched) > > <https://reviews.apache.org/r/65866/diff/2/?file=1968483#file1968483line42> > > > > Why are these unchecked exceptions part of the constructor declaration? > > > > I think that only checked exceptions are useful to have here. Unchecked > > exceptions are errors that are thrown that are not expected to happen, > > aren't they? Like a NullPointerException. Currently all classes that implement Privilege class like CommonPrivilege, or in older versions classes like DBWildcardPrivilege throw IllegalArgumentException, and AssertionError in their constructors but the constructor itself doesn't throw any exceptions. Also the current doHasAccess method doesn't have a try catch block. So if we were to hit these exceptions Sentry wouldn't catch them, we won't log any exceptions. > On March 12, 2018, 11:04 p.m., Sergio Pena wrote: > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java > > Lines 48 (patched) > > <https://reviews.apache.org/r/65866/diff/2/?file=1968483#file1968483line48> > > > > Do we need this validation? privilegeStr is a simple string, so I don't > > think it causes harm if we leave the LOG.debug() call without checking if > > debug is enabled. How expensive is a isDebugEnabled operation? > On March 12, 2018, 11:04 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java > > Lines 112-115 (original), 111 (patched) > > <https://reviews.apache.org/r/65866/diff/2/?file=1968486#file1968486line112> > > > > Why is this code removed? Isn't this patch only improving log messages? Because the Set<String>hierarchy is not used anywhere. We can remove it because it is just being set in the for loop for no reason > On March 12, 2018, 11:04 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java > > Lines 139-151 (patched) > > <https://reviews.apache.org/r/65866/diff/2/?file=1968486#file1968486line143> > > > > How useful are these error messages for a user? What argument is > > illegal? Is there a way to handle these errors differently than just > > rethrowing the erros and logging the messages? Look at the current CommonPrivilege constructor. We throw IllegalArgumentException and AssertionError ==================================================================================================================== public CommonPrivilege(String privilegeStr) { privilegeStr = Strings.nullToEmpty(privilegeStr).trim(); if (privilegeStr.isEmpty()) { throw new IllegalArgumentException("Privilege string cannot be null or empty."); } List<KeyValue> parts = Lists.newArrayList(); for (String authorizable : SentryConstants.AUTHORIZABLE_SPLITTER.trimResults().split( privilegeStr)) { if (authorizable.isEmpty()) { throw new IllegalArgumentException("Privilege '" + privilegeStr + "' has an empty section"); } parts.add(new KeyValue(authorizable)); } if (parts.isEmpty()) { throw new AssertionError("Should never occur: " + privilegeStr); } this.parts = ImmutableList.copyOf(parts); } ==================================================================================================================== Error messages are very useful. We know why they were being thrown. Right now if these errors occur, all the user gets to see is he/she doesn't have necessary privileges. Which is misleading. I was re-throwing them so they also get logged on the client side like Impala or Hive - Arjun ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65866/#review199044 ----------------------------------------------------------- On March 1, 2018, 6:19 p.m., Arjun Mishra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65866/ > ----------------------------------------------------------- > > (Updated March 1, 2018, 6:19 p.m.) > > > Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar > kalvagadda, Liam Sargent, Na Li, Steve Moist, Sergio Pena, Vadim Spector, and > Xinran Tinney. > > > Repository: sentry > > > Description > ------- > > There are a bunch of improvements that should be made to > ResourceAuthorizationProvider. For example, exceptions thrown by > privilegeFactory.createPrivilege are not gracefully handled. Makes debugging > hard. > > We also need to add a lot more logging that needs to be added to related > classes > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java > 7565a34b5 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java > 09bd9b566 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java > 447deaf58 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java > 4e944e5f2 > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java > ab5560994 > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeFactory.java > 2f8296bfa > > sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPrivilegeFactory.java > d338f0edd > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java > a9b98f319 > > > Diff: https://reviews.apache.org/r/65866/diff/2/ > > > Testing > ------- > > mvn -f sentry-binding/sentry-binding-hive/pom.xml test > mvn -f sentry-core/sentry-core-common/pom.xml test > mvn -f sentry-policy/sentry-policy-common/pom.xml test > mvn -f sentry-policy/sentry-policy-engine/pom.xml test > mvn -f sentry-provider/sentry-provider-common/pom.xml test > > > Thanks, > > Arjun Mishra > >
