Support for role based access of HadoopCatalog table listing #1941 by cccs-jc · Pull Request #1979 · apache/iceberg

cccs-jc · 2020-12-23T15:32:58Z

Modified the implementation of the HadoopCatalog to avoid using the deprecated isDirectory method.

Instead used the getFileStatus() method to get both the fact that a path exists and is a directory in a single call.

Created two methods to determine isTableDir() and isDirectory() which catch any I/O exception which are thrown when a user/client does not have access to certain folders.

http://hadoop.apache.org/docs/stable/api/org/apache/hadoop/fs/FileSystem.html#isDirectory-org.apache.hadoop.fs.Path-

[skip ci]

rdblue · 2020-12-28T19:51:13Z

+    try {
+      return fs.listStatus(metadataPath, TABLE_FILTER).length >= 1;
+    } catch (IOException e) {
+      return false;


Same here. Returning false is not correct.

We use the iceberg HadoopCatalog over Azure ABFS, Users use spark to list the tables in the catalog. However not every user has access to all tables on disk (we use file system access control list). When a user cannot access a folder the listStatus function will throw a subclass of IOException.

In my ticket I suggested using a flag to configure the HadoopCatalog to throw errors when unable to list these directories or return false if it is expected that some users will not be able to list all folders/tables.

#1941

The listStatus throws an FileNotFoundException when the path provided does not exists. I will handle that as a "normal" condition, any other IOException I will either catch or not based on a configuration flag. This should handle scenarios where not all user's can access all tables/namespaces

What do you think about that proposal?

I think it makes sense to catch FileNotFoundException and return false. What is the exception class that you get when a user can't list path? I would rather use that instead of suppressing any IOException if there is a flag set.

It is also unclear how that suppression is set.

I've added support for a flag. However I have not yet retrieved the value from the configuration properties.

The exception thrown is this one:

https://hadoop.apache.org/docs/r3.2.0/api/org/apache/hadoop/fs/azurebfs/contracts/exceptions/AbfsRestOperationException.html

IOException
AzureBlobFileSystemException
AbfsRestOperationException

Unfortunately the exception thrown is an ABFS specific class.

Should the boolean be configurable? How are you setting it?

Hey Ryan, sorry for the late reply. Ya I'm thinking it could support a flag passed in as a configuration via the org.apache.hadoop.conf.Configuration

I'm just not sure how to pass values into that configuration object. For example there is already a FILE_IO_IMPL supported. How would I add an additional configuration like FILE_IO_IMPL?

Getting the config from catalog properties looks good, but I'd move the property constant as I noted above.

this is really brittle design. It will only work for one specific store and contains the assumption that the store always returns the string "AuthorizationPermissionMismatch" on an error. If the abfs store ever changed that string everything will break.

That abfs exception also includes a status code, 403, which is what should really be used as the signal.

Other stores return AccessDeniedException. Has anyone considered filing a hadoop bug asking for translation of rest exceptions with 403s into those? I am reasonably confident nobody is going reject PRs there.

rdblue · 2020-12-28T19:53:38Z

Thanks for working on this, @cccs-jc. I like that it avoids extra calls to the NameNode.

massdosage · 2021-01-04T16:32:19Z

  private final String warehouseLocation;
  private final FileSystem fs;
  private final FileIO fileIO;
+  private boolean suppressACLExcpetion = false;


Suggested change

private boolean suppressACLExcpetion = false;

private boolean suppressACLException = false;

massdosage · 2021-01-04T16:35:03Z

+    } catch (FileNotFoundException e) {
+      return false;
+    } catch (IOException e) {
+      if (suppressACLExcpetion) {


These exceptions could be thrown be errors that don't have anything to do with ACL so can we try come up with a more generic name for this flag? suppressIOException maybe? I guess it depends on where they will be configured and how.

fair enough.

I'm going to add this configuration like it is done for the FILE_IO_IMPL for example.

rdblue · 2021-01-05T18:24:04Z

I'm still concerned about adding this without a better check. Adding a flag to turn all IOException instances into a false response seems really brittle. Can we inspect the exception message? Is there a cause that is available? Maybe we can at least check the package of the exception instance?

cccs-jc · 2021-01-05T20:08:09Z

sure I could check if it's an azure blob storage exception but that would make it specific to azure..

This is the outline of the listTables method. In the end if a location can't be read it will return NoSuchNamespaceException. If for any reason you cannot list the dir/metadata folder it won't return the table.

So the fact that IOException are not return will not break anything. Worst case users don't see the namespace/tables and hunt down why that is. Maybe logging the error would be helpful while still returning false. My goal here is to not interrupt the execution of listTables if certain user's can't access certain tables. So having a log entry might be useful for the admins/operators.

tables listTables(location)
  if !isDirectory(location) (tries to list "location", if any exception return false)
    throw NoSuchNamespaceException

    for dir in location
      if dir.isFolder()
        if isTableDir(dir) (tries to list "dir/metadata", if any exception return false)
          add to list of tables to return

rdblue · 2021-01-05T22:17:56Z

Logging the error is definitely a good idea.

It sounds like the main problem is that there isn't a specific error to catch. I think it would be fine if any directory that the user doesn't have access to doesn't show up in the list results. But we do need to ensure that ignoring directories isn't applied to other situations where it isn't reasonable. So if we can inspect the exception from Azure and make this work I think that's the way to go. We can support other errors later as we find them.

cccs-jc · 2021-01-06T18:43:23Z

sounds good. I will narrow it to the Azure package and also narrow it to when it is listing a table or namespace only.

cccs-jc · 2021-01-06T19:15:19Z

I've checked everywhere the following methods are used

isTableDir
isDirectory
isNamespace

In all cases I can see it makes sense to not throw the exception since these are tests. For example

  createNamespace()
    if isNamespace
      throw AlreadyExistsException
	  
    if it does not exists it will try to create it.
    if a user for some reason can't list the dir it will return false and
    proceed to try to create it, at that point it will fail with a IOException
    showing why (could be restricted access or any other reason)
    
  
  listNamespaces()
    if not isNamespace
      throws NoSuchNamespaceException
  
    then proceeds to list the namespaces under that location, so like listTables it might be a partial list
    
  dropNamespace()
    if can't access it will consider it does not exists and return false (not dropped)
  
  
  loadNamespaceMetadata()
    again if can't access namespace a NoSuchNamespaceException is returned

Nonetheless I will add checks for determining if the exception is an azure blob exception.

Conflicts: core/src/main/java/org/apache/iceberg/CatalogProperties.java core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java

rdblue · 2021-01-07T01:06:13Z

  public static final String HIVE_CLIENT_POOL_SIZE = "clients";
  public static final int HIVE_CLIENT_POOL_SIZE_DEFAULT = 2;

+  public static final String HADOOP_SUPPRESS_IOEXCEPTION = "suppress-io-exception";


Since this is specific to Hadoop, let's move it into HadoopCatalog. I don't think the other catalogs would need this because they do not list using FS operations. A private constant in HadoopCatalog should be fine and we can always expose it later if we need to.

rdblue · 2021-01-07T01:06:57Z


    String fileIOImpl = properties.get(CatalogProperties.FILE_IO_IMPL);
+    this.suppressIOException = Boolean.parseBoolean(properties.get(CatalogProperties.HADOOP_SUPPRESS_IOEXCEPTION));
+


Can you move this to after fileIO is set? The line above and that line are related so we shouldn't separate them.

rdblue · 2021-01-07T01:07:48Z


+  private boolean shouldSuppressIOException(IOException ioException) {
+    String name = ioException.getClass().getName();
+    return suppressIOException && name.startsWith("org.apache.hadoop.fs.azurebfs");


I would really prefer if this were more specific and looked at the exception message as well to ensure the error is a permissions or ACL problem.

I have fixed the code review suggestions.

I'm detecting the azure blob permission exception using the message (that's all I have available). In that message there is an AzureServiceErrorCode of value "AuthorizationPermissionMismatch" which I use to detect a permission issue.

I agree this is much better error handling since it will let other errors be reported.

I'm thinking the flag should thus be "suppress permission errors". What do you think?

That sounds good to me!

rdblue · 2021-01-07T01:08:05Z

+      return false;
+    } catch (IOException e) {
+      if (shouldSuppressIOException(e)) {
+        LOG.warn("Unalbe to metadata directory {}: {}", metadataPath, e);


Typo: Unalbe -> Unable

rdblue · 2021-01-07T01:08:16Z

+      return false;
+    } catch (IOException e) {
+      if (shouldSuppressIOException(e)) {
+        LOG.warn("Unalbe to list directory {}: {}", path, e);


rdblue · 2021-01-08T01:55:42Z

  }

  public static final String ENGINE_HIVE_ENABLED = "iceberg.engine.hive.enabled";
+


Nit: this file doesn't need to be touched. Could you remove this whitespace change? It may cause git conflicts.

rdblue · 2021-01-08T01:57:08Z

@cccs-jc, thanks for updating this! Other than the file that doesn't need to be modified and renaming the config property, I think this is ready to go. Thanks for getting this done!

cccs-jc · 2021-01-08T17:49:22Z

Hey Ryan,

Made the change. Should be good to go now. Please have a final spot check.
Cheers

rdblue · 2021-01-12T23:28:02Z

Thanks, @cccs-jc! The tests are passing after a restart, so I'll merge this.

steveloughran · 2021-01-20T13:35:34Z

FWIW, HADOOP-15710 first reported this issue in 2018. Someone needs to fix this. Will escalate

…1941 (apache#1979)

cccs-jc and others added 3 commits December 21, 2020 09:59

Set up CI with Azure Pipelines

97f0e46

[skip ci]

avoid using deprecated hadoop filesystem exist and isDirectory

0b3a92b

removed unused filter

efa8c87

github-actions Bot added the core label Dec 23, 2020

removed pipeline file

4c67221

rdblue reviewed Dec 28, 2020

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java

rdblue reviewed Dec 28, 2020

View reviewed changes

added flag for suppressing ACL exceptions

2219cd9

massdosage reviewed Jan 4, 2021

View reviewed changes

made suppresion configurable

ac5e25e

cccs-jc added 2 commits January 6, 2021 14:18

check if azure blob exception

c865b35

Merge remote-tracking branch 'upstream/master'

a6ee95a

Conflicts: core/src/main/java/org/apache/iceberg/CatalogProperties.java core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java

rdblue reviewed Jan 7, 2021

View reviewed changes

cccs-jc added 2 commits January 7, 2021 10:21

code review fixes

561a202

fix indentation

6b03929

rdblue reviewed Jan 8, 2021

View reviewed changes

cccs-jc added 2 commits January 8, 2021 12:44

code review fixes

66cca57

code review fixes

e6dbe2e

rdblue merged commit 01fca3d into apache:master Jan 12, 2021

XuQianJin-Stars pushed a commit to XuQianJin-Stars/iceberg that referenced this pull request Mar 22, 2021

Core: Fix HadoopCatalog listing with Azure permission failure apache#…

22878c7

…1941 (apache#1979)

	private boolean suppressACLExcpetion = false;
	private boolean suppressACLException = false;


		String fileIOImpl = properties.get(CatalogProperties.FILE_IO_IMPL);
		this.suppressIOException = Boolean.parseBoolean(properties.get(CatalogProperties.HADOOP_SUPPRESS_IOEXCEPTION));

		}

		public static final String ENGINE_HIVE_ENABLED = "iceberg.engine.hive.enabled";

Conversation

cccs-jc commented Dec 23, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cccs-jc Dec 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue commented Dec 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue commented Jan 5, 2021

Uh oh!

cccs-jc commented Jan 5, 2021

Uh oh!

rdblue commented Jan 5, 2021

Uh oh!

cccs-jc commented Jan 6, 2021

Uh oh!

cccs-jc commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue commented Jan 8, 2021

Uh oh!

cccs-jc commented Jan 8, 2021

Uh oh!

rdblue commented Jan 12, 2021

Uh oh!

steveloughran commented Jan 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cccs-jc Dec 29, 2020 •

edited

Loading

cccs-jc commented Jan 6, 2021 •

edited

Loading