Uploaded image for project: 'Accumulo'
  1. Accumulo
  2. ACCUMULO-1018

Client does not give informative message when user can not read table

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.4.0
    • Fix Version/s: 1.6.0
    • Component/s: client
    • Labels:
      None

      Description

      Saw this in 1.4, not sure if its an issue in later versions.

      Assume a user has an application that is reading from many tables and does not have permission to read from one table. In this case the exception does not tell them which table they can not read from. If not familiar with the application, it can take a while to track this issue down on a system with many tables.

      1. ACCUMULO-1018.patch
        6 kB
        Kevin Faro
      2. ACCUMULO-1018-2.patch
        11 kB
        Kevin Faro
      3. ACCUMULO-1018-3.patch
        12 kB
        Kevin Faro

        Activity

        Hide
        vines John Vines added a comment -

        With the changes made to the BatchWriter in 1.5 to provide SecurityErrorCodes, it shouldn't be that bad to extend it to also include KeyExtents or Tables they occured in. Hell, I think that's entirely possible client side since I know the impl has KeyExtent->ErrorCode pairings available to it.

        Show
        vines John Vines added a comment - With the changes made to the BatchWriter in 1.5 to provide SecurityErrorCodes, it shouldn't be that bad to extend it to also include KeyExtents or Tables they occured in. Hell, I think that's entirely possible client side since I know the impl has KeyExtent->ErrorCode pairings available to it.
        Hide
        kturner Keith Turner added a comment -

        I also think the client code will have all of the information needed, its just a matter of passing it along. Maybe some client code does, I have not really looked into it. Just saw a case in 1.4 where the scanner was not passing this info along. Need to look at scanner, batch scanner, batch writer, and table operations.

        Show
        kturner Keith Turner added a comment - I also think the client code will have all of the information needed, its just a matter of passing it along. Maybe some client code does, I have not really looked into it. Just saw a case in 1.4 where the scanner was not passing this info along. Need to look at scanner, batch scanner, batch writer, and table operations.
        Hide
        ctubbsii Christopher Tubbs added a comment -

        Under what circumstances can a client not know which table it was trying to read when it got a permissions error? It seems to me that the client code should always have the table name when an exception occurs as a result of not having sufficient permissions. I imagine the observed behavior is the result of an application passing through an AccumuloSecurityException, instead of handling it properly. If that is the case, then this is a problem with the application using Accumulo, not Accumulo having insufficiently informative messages.

        Show
        ctubbsii Christopher Tubbs added a comment - Under what circumstances can a client not know which table it was trying to read when it got a permissions error? It seems to me that the client code should always have the table name when an exception occurs as a result of not having sufficient permissions. I imagine the observed behavior is the result of an application passing through an AccumuloSecurityException, instead of handling it properly. If that is the case, then this is a problem with the application using Accumulo, not Accumulo having insufficiently informative messages.
        Hide
        vines John Vines added a comment -

        I don't have the code in front of me, but I believe there is a case for this with any of the multi-table readers/writers we have.

        Show
        vines John Vines added a comment - I don't have the code in front of me, but I believe there is a case for this with any of the multi-table readers/writers we have.
        Hide
        ctubbsii Christopher Tubbs added a comment -

        I think you're right. I just did a quick scan through the code, and it looks like the MultiTableBatchWriter can have this problem when it flushes, and the AccumuloOutputFormat probably also has this problem, since it uses the MultiTableBatchWriter.

        Show
        ctubbsii Christopher Tubbs added a comment - I think you're right. I just did a quick scan through the code, and it looks like the MultiTableBatchWriter can have this problem when it flushes, and the AccumuloOutputFormat probably also has this problem, since it uses the MultiTableBatchWriter.
        Hide
        kturner Keith Turner added a comment -

        Under what circumstances can a client not know which table it was trying to read when it got a permissions error?

        One situation is that Alice writes and application that reads from 20 tables using the scanner. Bob deploys Alice's application on an Accumulo instance with 100's of tables. When the application says it can not read something, Bob has to spend a lot of time figuring out what it can not read. If the scanner exception gave the table name, it would make things a lot easier for Bob. Sure Alice's application could have given a better error message, but it would be easy for the scanner to do this so I do not see why it should not.

        Show
        kturner Keith Turner added a comment - Under what circumstances can a client not know which table it was trying to read when it got a permissions error? One situation is that Alice writes and application that reads from 20 tables using the scanner. Bob deploys Alice's application on an Accumulo instance with 100's of tables. When the application says it can not read something, Bob has to spend a lot of time figuring out what it can not read. If the scanner exception gave the table name, it would make things a lot easier for Bob. Sure Alice's application could have given a better error message, but it would be easy for the scanner to do this so I do not see why it should not.
        Hide
        kevin.faro Kevin Faro added a comment -

        I started taking a look at this. It wasn't too bad to add support for scanning. I added an optional tableName field to AccumuloSecurityException that the ThriftScanner sets the tableName before it re-throws the excpetion. TableOperations and BatchScanner was similar.

        However, the (MultiTable)BatchWriter throws a MutationsRejectedException that has the KeyExtent->ErrorCode pairings (as John pointed out) but I do not see an effective way map KeyExtent.tableId to a tableName (that the client would probably prefer) since the MRE doesn't have access to the instance. Any ideas?

        Show
        kevin.faro Kevin Faro added a comment - I started taking a look at this. It wasn't too bad to add support for scanning. I added an optional tableName field to AccumuloSecurityException that the ThriftScanner sets the tableName before it re-throws the excpetion. TableOperations and BatchScanner was similar. However, the (MultiTable)BatchWriter throws a MutationsRejectedException that has the KeyExtent->ErrorCode pairings (as John pointed out) but I do not see an effective way map KeyExtent.tableId to a tableName (that the client would probably prefer) since the MRE doesn't have access to the instance. Any ideas?
        Hide
        kturner Keith Turner added a comment -

        However, the (MultiTable)BatchWriter throws a MutationsRejectedException that has the KeyExtent->ErrorCode pairings (as John pointed out) but I do not see an effective way map KeyExtent.tableId to a tableName (that the client would probably prefer) since the MRE doesn't have access to the instance. Any ideas?

        I looked around the code to see where else KeyExtent was used in the public API. o.a.a.c.client.admin.ActiveScan and ActiveCompaction expose KeyExtent to the user. Howerver these classes also provide getTable() methods which return the table name. The implementation of these methods use code that is not part of the public API.

        The javadoc for MRE.getAuthorizationFailures() could point users to TableOperations.tableIdMap(). However, this map is not readily usable, because its keyed on table name. So I guess basically we need an easy way for users to map table ids to table names thats in the public API. If this existed, then the javadoc for MRE could point to it. Maybe add something to TableOperations that returns a map keyed on table id? I looked for a built in java util in Collections that would invert a map and did not see anything. If there was a simple way to invert the map, then we would not need to add anything to the public API, just add some javadoc pointing out this inversion util.

        Another possibility is adding a convenience method "String getTableName(Instance i)" to KeyExtent. This addresses the issue at hand, but I think a more general solution in TableOperations would be better.

        Show
        kturner Keith Turner added a comment - However, the (MultiTable)BatchWriter throws a MutationsRejectedException that has the KeyExtent->ErrorCode pairings (as John pointed out) but I do not see an effective way map KeyExtent.tableId to a tableName (that the client would probably prefer) since the MRE doesn't have access to the instance. Any ideas? I looked around the code to see where else KeyExtent was used in the public API. o.a.a.c.client.admin.ActiveScan and ActiveCompaction expose KeyExtent to the user. Howerver these classes also provide getTable() methods which return the table name. The implementation of these methods use code that is not part of the public API. The javadoc for MRE.getAuthorizationFailures() could point users to TableOperations.tableIdMap(). However, this map is not readily usable, because its keyed on table name. So I guess basically we need an easy way for users to map table ids to table names thats in the public API. If this existed, then the javadoc for MRE could point to it. Maybe add something to TableOperations that returns a map keyed on table id? I looked for a built in java util in Collections that would invert a map and did not see anything. If there was a simple way to invert the map, then we would not need to add anything to the public API, just add some javadoc pointing out this inversion util. Another possibility is adding a convenience method "String getTableName(Instance i)" to KeyExtent. This addresses the issue at hand, but I think a more general solution in TableOperations would be better.
        Hide
        kevin.faro Kevin Faro added a comment -

        Here is what I have so far. There are things to clean up to make the code look a little nicer ... but I still haven't been able to crack the nut of getting tableNames from KeyExtent ...

        Show
        kevin.faro Kevin Faro added a comment - Here is what I have so far. There are things to clean up to make the code look a little nicer ... but I still haven't been able to crack the nut of getting tableNames from KeyExtent ...
        Hide
        vines John Vines added a comment -

        tableID is still better than nothing though. Looks good so far

        Show
        vines John Vines added a comment - tableID is still better than nothing though. Looks good so far
        Hide
        ctubbsii Christopher Tubbs added a comment -

        Try org.apache.accumulo.core.client.impl.Tables to get the name for an ID.

        Show
        ctubbsii Christopher Tubbs added a comment - Try org.apache.accumulo.core.client.impl.Tables to get the name for an ID.
        Hide
        kevin.faro Kevin Faro added a comment -

        I used o.a.a.c.c.i.Tables to get the name for an ID in the TabletServerBatchReaderIterator and the TriftScanner ... but I didn't know where to get a reference to instance in the KeyExtent to make that translation.

        Show
        kevin.faro Kevin Faro added a comment - I used o.a.a.c.c.i.Tables to get the name for an ID in the TabletServerBatchReaderIterator and the TriftScanner ... but I didn't know where to get a reference to instance in the KeyExtent to make that translation.
        Hide
        kturner Keith Turner added a comment -

        MRE is only constructed by the TSBW (TabletServerBatchWriter). The TSBW could pass its Instance to the MRE constructor. Then MRE could use that Instance when calling methods on o.a.a.c.c.i.Tables.

        If you are interested, the following changes would be nice :

        • Make the format() function you added to MRE reduce Map<KeyExtent,Set<SecurityErrorCode>> to Map<TableNameIdString, Set<SecurityErrorCode>> and print that out. That would be much easier for a human to read. If someone is interested in the details they can call getAuthorizationFailuresMap().
        • I am not sure all of the messages you added will have both table id and table name. Something like "tablename(tableid)" for all the messages would be nice.
        • I believe the scanState.tableName in thrift scanner is misnamed, I think that is actually a tableId.
        Show
        kturner Keith Turner added a comment - MRE is only constructed by the TSBW (TabletServerBatchWriter). The TSBW could pass its Instance to the MRE constructor. Then MRE could use that Instance when calling methods on o.a.a.c.c.i.Tables. If you are interested, the following changes would be nice : Make the format() function you added to MRE reduce Map<KeyExtent,Set<SecurityErrorCode>> to Map<TableNameIdString, Set<SecurityErrorCode>> and print that out. That would be much easier for a human to read. If someone is interested in the details they can call getAuthorizationFailuresMap(). I am not sure all of the messages you added will have both table id and table name. Something like "tablename(tableid)" for all the messages would be nice. I believe the scanState.tableName in thrift scanner is misnamed, I think that is actually a tableId.
        Hide
        kevin.faro Kevin Faro added a comment -
        • I changed the format of the tableName in the error Messages to be <tableName>(ID:<tableId>).
        • I added instance to the MRE constructor
        • I created ACCUMULO-1144 to capture that the tableName in the ThriftScanner$ScanState is really tableId
        Show
        kevin.faro Kevin Faro added a comment - I changed the format of the tableName in the error Messages to be <tableName>(ID:<tableId>). I added instance to the MRE constructor I created ACCUMULO-1144 to capture that the tableName in the ThriftScanner$ScanState is really tableId
        Hide
        kevin.faro Kevin Faro added a comment -

        ACCUMULO-1018-2.patch

        Thanks Keith! I incorporated your suggestions.

        Show
        kevin.faro Kevin Faro added a comment - ACCUMULO-1018 -2.patch Thanks Keith! I incorporated your suggestions.
        Hide
        kturner Keith Turner added a comment - - edited

        Patch #2 looks good, I have only one issue with it. The format() function should create new set for a table if one does not exist. Then it should add all security error codes to the tables set for each key extent.

        I created a little test program to experiment with the patch. I like the output.

        Attemping batch scan : 
        	org.apache.accumulo.core.client.AccumuloSecurityException: Error PERMISSION_DENIED for user user2 on table table1(ID:3) - User does not have permission to perform this action
        Attemping scan : 
        	org.apache.accumulo.core.client.AccumuloSecurityException: Error PERMISSION_DENIED for user user2 on table table1(ID:3) - User does not have permission to perform this action
        Attemping batch write : 
        	# constraint violations : 0  security codes: {table1(ID:3)=[PERMISSION_DENIED]}  # server errors 0 # exceptions 0
        Attemping to delete table : 
        	Error PERMISSION_DENIED for user user2 on table table1(ID:3) - User does not have permission to perform this action
        
        
        Show
        kturner Keith Turner added a comment - - edited Patch #2 looks good, I have only one issue with it. The format() function should create new set for a table if one does not exist. Then it should add all security error codes to the tables set for each key extent. I created a little test program to experiment with the patch. I like the output. Attemping batch scan : org.apache.accumulo.core.client.AccumuloSecurityException: Error PERMISSION_DENIED for user user2 on table table1(ID:3) - User does not have permission to perform this action Attemping scan : org.apache.accumulo.core.client.AccumuloSecurityException: Error PERMISSION_DENIED for user user2 on table table1(ID:3) - User does not have permission to perform this action Attemping batch write : # constraint violations : 0 security codes: {table1(ID:3)=[PERMISSION_DENIED]} # server errors 0 # exceptions 0 Attemping to delete table : Error PERMISSION_DENIED for user user2 on table table1(ID:3) - User does not have permission to perform this action
        Hide
        kevin.faro Kevin Faro added a comment -

        I am not sure I follow. Do you mean that if you use a BatchWriter to write to multiple tables (a & b) and only fail on b, the format method would produce the following?

        {a(ID:1)=[], b(ID:2)=[PERMISSION_DENIED]}

        or do you mean something else?

        Show
        kevin.faro Kevin Faro added a comment - I am not sure I follow. Do you mean that if you use a BatchWriter to write to multiple tables (a & b) and only fail on b, the format method would produce the following? {a(ID:1)=[], b(ID:2)=[PERMISSION_DENIED]} or do you mean something else?
        Hide
        kturner Keith Turner added a comment -

        I was thinking of something else. Was thinking of the case where there are two extents E1 and E2, both from table T1. The extents see different security errors. So the input would be something like the following.

          {E1={SE1}, E2={SE2}}
        

        I think the code will output the following

          {T1={SE2}}
        

        It should output the following

          {T1={SE1, SE2}}
        
        Show
        kturner Keith Turner added a comment - I was thinking of something else. Was thinking of the case where there are two extents E1 and E2, both from table T1. The extents see different security errors. So the input would be something like the following. {E1={SE1}, E2={SE2}} I think the code will output the following {T1={SE2}} It should output the following {T1={SE1, SE2}}
        Hide
        kevin.faro Kevin Faro added a comment -

        Gotcha ... makes total sense. Thanks!

        I will put out another patch with those changes.

        Show
        kevin.faro Kevin Faro added a comment - Gotcha ... makes total sense. Thanks! I will put out another patch with those changes.
        Hide
        kevin.faro Kevin Faro added a comment -

        Thanks for the feedback Keith! I incorporated your suggestions in ACCUMULO-1018-3.patch.

        Let me know if you have any other suggestions.

        Show
        kevin.faro Kevin Faro added a comment - Thanks for the feedback Keith! I incorporated your suggestions in ACCUMULO-1018 -3.patch. Let me know if you have any other suggestions.
        Hide
        hudson Hudson added a comment -

        Integrated in Accumulo-Trunk #768 (See https://builds.apache.org/job/Accumulo-Trunk/768/)
        ACCUMULO-1018 applied patch from Kevin Faro that adds table info to security exception messages (Revision 1454126)

        Result = FAILURE
        kturner :
        Files :

        • /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/AccumuloSecurityException.java
        • /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/MutationsRejectedException.java
        • /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/admin/TableOperationsImpl.java
        • /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/impl/Tables.java
        • /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderIterator.java
        • /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java
        • /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/impl/ThriftScanner.java
        Show
        hudson Hudson added a comment - Integrated in Accumulo-Trunk #768 (See https://builds.apache.org/job/Accumulo-Trunk/768/ ) ACCUMULO-1018 applied patch from Kevin Faro that adds table info to security exception messages (Revision 1454126) Result = FAILURE kturner : Files : /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/AccumuloSecurityException.java /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/MutationsRejectedException.java /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/admin/TableOperationsImpl.java /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/impl/Tables.java /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderIterator.java /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/impl/ThriftScanner.java
        Hide
        hudson Hudson added a comment -

        Integrated in Accumulo-Trunk-Hadoop-2.0 #127 (See https://builds.apache.org/job/Accumulo-Trunk-Hadoop-2.0/127/)
        ACCUMULO-1018 applied patch from Kevin Faro that adds table info to security exception messages (Revision 1454126)

        Result = SUCCESS
        kturner :
        Files :

        • /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/AccumuloSecurityException.java
        • /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/MutationsRejectedException.java
        • /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/admin/TableOperationsImpl.java
        • /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/impl/Tables.java
        • /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderIterator.java
        • /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java
        • /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/impl/ThriftScanner.java
        Show
        hudson Hudson added a comment - Integrated in Accumulo-Trunk-Hadoop-2.0 #127 (See https://builds.apache.org/job/Accumulo-Trunk-Hadoop-2.0/127/ ) ACCUMULO-1018 applied patch from Kevin Faro that adds table info to security exception messages (Revision 1454126) Result = SUCCESS kturner : Files : /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/AccumuloSecurityException.java /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/MutationsRejectedException.java /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/admin/TableOperationsImpl.java /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/impl/Tables.java /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderIterator.java /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java /accumulo/trunk/core/src/main/java/org/apache/accumulo/core/client/impl/ThriftScanner.java
        Hide
        kturner Keith Turner added a comment -

        patch 3 looks good, I applied it thanks.

        Show
        kturner Keith Turner added a comment - patch 3 looks good, I applied it thanks.

          People

          • Assignee:
            kevin.faro Kevin Faro
            Reporter:
            kturner Keith Turner
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development