Standardize AWS credential names #922

HonahX · 2024-07-13T00:35:26Z

There has been many discussions and concerns over the current behavior of loading AWS credential for glue/dynamo catalog: #892, #515, #570

This PR tries to standardize the property names of AWS credential configs by introducing:

New client-specific configs: glue.access-key-id, dynamodb.access-key-id,
Unified AWS configurations: client.access-key-id
Deprecate old names such as aws_access_key_id.

For example, in the use case of GlueCatalog + S3, the order of config that each client will look for is

Glue

glue.access-key-id
client.access-key-id
aws_access_key_id (deprecated)

s3

s3.access-key-id
client.access-key-id

Note: the choice of names for Unified AWS Configurations

I chose client. for unified configurations because in java:
https://github.com/apache/iceberg/blob/c68abfc9fd3956077b43aba20441f089bb8b93d6/aws/src/main/java/org/apache/iceberg/aws/AwsClientProperties.java#L67
client.region is used for configurations that affect all the clients.

cc @kevinjqliu @jayceslesar @Fokko @syun64

HonahX · 2024-07-13T00:56:17Z

pyiceberg/io/__init__.py

@@ -46,6 +48,10 @@

 logger = logging.getLogger(__name__)

+AWS_REGION = "client.region"


I chose client. for unified configurations because in java:
https://github.com/apache/iceberg/blob/c68abfc9fd3956077b43aba20441f089bb8b93d6/aws/src/main/java/org/apache/iceberg/aws/AwsClientProperties.java#L67
client.region is used for configurations that affect all the clients.

Nice, that's a good find 👍

would be good to highlight this in the PR description!

jayceslesar · 2024-07-13T02:04:54Z

went to generate the mkdocs and spawned #923 but I think the approach looks good. The only way to make less repeatable would be to have some factory to generate the constant names with a given prefix but that seems like overkill until there are more AWS services involved (if that happens).

I think keeping the s3 prefix also makes sense in the case a user is backing blob storage with MinIO

sungwy

Hi @HonahX - this looks great, thank you for working on this standardization effort.

It took a minute for me to get up to speed with the frustrations the users have been experiencing with the choices of AWS configurations due to my lack of experience using glue or dynamodb. Given that there are so many issues that are currently open regarding this family of issues, I think this will be a very welcome change for that user base.

I left a few nits, but the approach looks safe and clean enough for us to push for it to be merged into 0.7.0 in conjunction with some tests.

pyiceberg/catalog/__init__.py

pyiceberg/io/__init__.py

kevinjqliu

Thanks for working on this! I think this will solve a lot of issues currently around AWS credentials.
I like the current approach with the client-specific config and fallback to the general config.

We should add a few test cases around the issues that have been reported.

I wonder if we can treat the old property names (i.e. aws_access_key_id) as an alias for the fallback config (client.access-key-id) versus fully deprecating it in 0.8.0

HonahX · 2024-07-14T09:36:30Z

I wonder if we can treat the old property names (i.e. aws_access_key_id) as an alias for the fallback config (client.access-key-id) versus fully deprecating it in 0.8.0

@kevinjqliu Thanks for raising this! I chose to deprecate aws_access_key_id because I would like to take this chance to formalize the property names in pyiceberg (iceberg), to use <service name>. prefix and hyphens to connect words. Another reason is that I think it is better to ultimately having one property name for a specific functionality to avoid confusion.

Please let me know if these reasons for the deprecation make sense. We could also consider postponing the deprecation to a later release if more discussion is needed or extending the deprecation notice period to 0.9.0 or later to give users more time to adjust.

HonahX · 2024-07-14T09:38:01Z

mkdocs/docs/configuration.md

-For example, `PYICEBERG_CATALOG__DEFAULT__S3__ACCESS_KEY_ID`, sets `s3.access-key-id` on the `default` catalog.
-
-# Tables
+## Tables


Adding the extra # so that the Table of contents sidebar can be rendered correctly:

HonahX · 2024-07-14T09:43:06Z

pyiceberg/catalog/dynamodb.py

-            aws_session_token=properties.get("aws_session_token"),
+            profile_name=PropertyUtil.get_first_property_value(properties, DYNAMODB_PROFILE_NAME, DEPRECATED_PROFILE_NAME),
+            region_name=PropertyUtil.get_first_property_value(properties, DYNAMODB_REGION, AWS_REGION, DEPRECATED_REGION),
+            botocore_session=properties.get(DEPRECATED_BOTOCORE_SESSION),


I think it may be more reasonable to stop exposing the botocore_session configuration:

Unlike other configs which takes string, the botocore_session takes a botocore.Session instance, which is not aligned with the convention of catalog properties to be a Dict[str, str]

I think we should hide this level of details from users.

HonahX · 2024-07-14T09:47:18Z

pyiceberg/table/__init__.py

+            if property_value := properties.get(property_name):
+                return property_value
+        return None
+


This may be in a follow-up PR: I am considering whether it is worth moving the PropertyUtil class to a module in utils. Currently, PropertyUtil is used in the table, catalog, and io modules. Given its wide usage and the fact that it is a standalone utility class, I believe it can be extracted from the table module.

Agreed. We can probably reorganize many of our functions to avoid circular dependencies 🙂

Fokko · 2024-07-14T11:24:06Z

pyiceberg/catalog/glue.py

@@ -117,6 +124,12 @@
 ICEBERG_FIELD_OPTIONAL = "iceberg.field.optional"
 ICEBERG_FIELD_CURRENT = "iceberg.field.current"

+GLUE_PROFILE_NAME = "glue.profile-name"
+GLUE_REGION = "glue.region"
+GLUE_ACCESS_KEY_ID = "glue.access-key-id"


How common is it to have a separate access-key-id for glue and the table itself? The S3FileIO would not pick up the glue.access-key-id:

iceberg-python/pyiceberg/io/pyarrow.py

Line 349 in 3f44dfe

"access_key": self.properties.get(S3_ACCESS_KEY_ID),

This way you would need to set both glue.access-key-id (which is used for authenticating against Glue), and then you need to set s3.access-key-id to ensure that it can access the metadata on the bucket. Maybe good to highlight this on the docs, WDYT?

I'm not an AWS expert, but my gut feeling is that normally people rely on AWS_ACCESS_KEY_ID to be picked up for both Glue and S3, but I prefer to be able to put this in the ~/.pyiceberg.yaml as well.

Thanks for the suggestions! I've updated the doc to explicitly indicating that glue.* properties are for Glue Catalog only. I've also added the s3.* properties to the example to make this clear.

but my gut feeling is that normally people rely on AWS_ACCESS_KEY_ID to be picked up for both Glue and S3, but I prefer to be able to put this in the ~/.pyiceberg.yaml as well.

The client.* properties will cover this case. When user set client.* properties, both catalog and FileIO can pick up the credentials. However, due to the limitation of pyarrow's S3FileSystem as noted in #570, we cannot yet support a unified profile-name property.

I added a separate section Unified AWS Credentials for the client.* properties. Please let me know WDYT.

Here are some preview of the updated page:

That makes sense, I'm good with that now. At some point we should have a bigger conversation across languages to unify this.

kevinjqliu

Added a few comments, overall LGTM!
Also thanks for making the docs better!!

kevinjqliu · 2024-07-17T19:46:18Z

pyiceberg/io/__init__.py

@@ -46,6 +48,10 @@

 logger = logging.getLogger(__name__)

+AWS_REGION = "client.region"


would be good to highlight this in the PR description!

kevinjqliu · 2024-07-17T19:49:15Z

tests/catalog/test_dynamodb.py

+        "client.secret-access-key": "client.secret-access-key",
+        "client.region": "client.region",
+        "client.session-token": "client.session-token",
+        "aws_access_key_id": "aws_access_key_id",


nit: can make these reuseable. i noticed this is used frequently

"aws_access_key_id": "aws_access_key_id", "aws_secret_access_key": "aws_secret_access_key", "aws_session_token": "aws_session_token", "region_name": "region_name", "profile_name": "profile_name",

Perhaps

deprecated_properties = { "aws_access_key_id": "aws_access_key_id", "aws_secret_access_key": "aws_secret_access_key", "aws_session_token": "aws_session_token", "region_name": "region_name", "profile_name": "profile_name", } session_properties = { ..., **deprecated_properties }

Thanks for the suggestion! I've extracted deprecated properties and unified configurations (client.*) to conftest.py`

sungwy

LGTM. Thank you for working on this in time for the release!! 👍

Fokko

LGTM, Thanks @HonahX for fixing this 🙌

HonahX added 4 commits July 12, 2024 00:55

first try

1310b0e

glue + s3

953a883

add dynamo

f7384e9

change from aws. to client.

0abae8f

HonahX mentioned this pull request Jul 13, 2024

[Bug] Load the proper AWS credential for glue/dynamodb catalog #892

Closed

HonahX commented Jul 13, 2024

View reviewed changes

sungwy reviewed Jul 13, 2024

View reviewed changes

pyiceberg/catalog/__init__.py Outdated Show resolved Hide resolved

pyiceberg/io/__init__.py Outdated Show resolved Hide resolved

kevinjqliu reviewed Jul 13, 2024

View reviewed changes

HonahX added 7 commits July 13, 2024 16:42

add missing label

94337c2

change to PropertyUtil.get_first_property_value

996c874

add unit tests for glue and dynamodb

ce5b604

Merge branch 'main' into honah_aws_configs

a67f549

add unit tests for fileio

7a85275

add doc

846fc08

revise doc

c91aaeb

HonahX commented Jul 14, 2024

View reviewed changes

HonahX marked this pull request as ready for review July 14, 2024 09:48

HonahX linked an issue Jul 14, 2024 that may be closed by this pull request

[Bug] Load the proper AWS credential for glue/dynamodb catalog #892

Closed

Fokko reviewed Jul 14, 2024

View reviewed changes

Fokko added this to the PyIceberg 0.7.0 release milestone Jul 14, 2024

resolve review comments

964132f

kevinjqliu approved these changes Jul 17, 2024

View reviewed changes

sungwy approved these changes Jul 18, 2024

View reviewed changes

HonahX added 2 commits July 17, 2024 22:51

Merge branch 'main' into honah_aws_configs

fd2af56

adopt kevinjqliu's suggestions

dd65c6d

Fokko approved these changes Jul 19, 2024

View reviewed changes

Fokko merged commit d0bfb4a into apache:main Jul 19, 2024
7 checks passed

kevinjqliu mentioned this pull request Jul 19, 2024

PyArrow S3FileSystem doesn't honor the AWS profile config #570

Closed

kevinjqliu mentioned this pull request Jul 19, 2024

Unable to load an iceberg table from aws glue catalog #515

Closed

HonahX mentioned this pull request Jul 20, 2024

Support loading custom catalog impl #947

Merged

BTheunissen mentioned this pull request Aug 27, 2024

Do not deprecate Botocore Session in upcoming release (0.8) #1104

Closed

nastra mentioned this pull request Sep 24, 2024

OpenAPI: Standardize credentials in loadTable/loadView responses apache/iceberg#10722

Merged

wudihero2 mentioned this pull request Sep 25, 2024

Why not use the profile name when initialising the S3FileSystem class? #1207

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize AWS credential names #922

Standardize AWS credential names #922

HonahX commented Jul 13, 2024 •

edited

Loading

HonahX Jul 13, 2024

Fokko Jul 13, 2024

kevinjqliu Jul 17, 2024

jayceslesar commented Jul 13, 2024 •

edited

Loading

sungwy left a comment

kevinjqliu left a comment

HonahX commented Jul 14, 2024

HonahX Jul 14, 2024

HonahX Jul 14, 2024

HonahX Jul 14, 2024

sungwy Jul 18, 2024

Fokko Jul 14, 2024

HonahX Jul 15, 2024

Fokko Jul 19, 2024

kevinjqliu left a comment

kevinjqliu Jul 17, 2024

kevinjqliu Jul 17, 2024

HonahX Jul 18, 2024

sungwy left a comment •

edited

Loading

Fokko left a comment

		@@ -46,6 +48,10 @@

		logger = logging.getLogger(__name__)

		AWS_REGION = "client.region"

Standardize AWS credential names #922

Standardize AWS credential names #922

Conversation

HonahX commented Jul 13, 2024 • edited Loading

Glue

s3

Note: the choice of names for Unified AWS Configurations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayceslesar commented Jul 13, 2024 • edited Loading

sungwy left a comment

Choose a reason for hiding this comment

kevinjqliu left a comment

Choose a reason for hiding this comment

HonahX commented Jul 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinjqliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sungwy left a comment • edited Loading

Choose a reason for hiding this comment

Fokko left a comment

Choose a reason for hiding this comment

HonahX commented Jul 13, 2024 •

edited

Loading

jayceslesar commented Jul 13, 2024 •

edited

Loading

sungwy left a comment •

edited

Loading