[#5623] feat(python): supports credential API in python client #5777

FANNG1 · 2024-12-06T01:06:10Z

What changes were proposed in this pull request?

supports credential API in python client

        catalog = gravitino_client.load_catalog(catalog_name)
        catalog.as_fileset_catalog().support_credentials().get_credentials()
        
        fileset = catalog.as_fileset_catalog().load_fileset(
            NameIdentifier.of("schema", "fileset")
        )
        credentials = fileset.support_credentials().get_credentials()

Why are the changes needed?

Fix: #5623

Does this PR introduce any user-facing change?

no

How was this patch tested?

add UT
setup a Gravitino server which returns credential, test with python client

FANNG1 · 2024-12-09T13:39:48Z

It's ready to review now, @jerryshao @yuqi1129 please help to review, thanks

clients/client-python/gravitino/api/credential/credential.py

clients/client-python/gravitino/api/credential/oss_token_credential.py

clients/client-python/gravitino/client/metadata_object_credential_operations.py

clients/client-python/gravitino/dto/responses/credential_response.py

FANNG1 · 2024-12-11T02:17:25Z

@jerryshao @xloya , all comments are addressed, please help to review again

clients/client-python/gravitino/api/credential/gcs_token_credential.py

clients/client-python/gravitino/api/credential/oss_token_credential.py

yuqi1129 · 2024-12-12T01:27:13Z

clients/client-python/gravitino/api/credential/s3_token_credential.py

+        )
+        Precondition.check_argument(
+            self.expire_time_in_ms,
+            "The expiration time of S3 token credential should greater than 0",


should be,
Please check other similar problems like this.

yuqi1129 · 2024-12-12T01:29:26Z

clients/client-python/gravitino/api/credential/s3_token_credential.py

+        """The s3 session token.
+
+        Returns:
+            The S3 session token.


You'd better use s3 or S3 in the whole class or in this PR uniformly.

jerryshao · 2024-12-12T02:11:41Z

clients/client-python/gravitino/api/credential/s3_token_credential.py

+            self._session_token, "The S3 session token should not be empty"
+        )
+        Precondition.check_argument(
+            self.expire_time_in_ms,


I think this is not a boolean expression, it should be a problem here.

jerryshao · 2024-12-12T02:12:37Z

clients/client-python/gravitino/api/credential/supports_credentials.py

+        """Retrieves an array of Credential objects.
+
+        Returns:
+            An array of Credential objects. In most cases the array only contains


Array or List? Please describe it correctly.

jerryshao · 2024-12-12T02:13:29Z

clients/client-python/gravitino/api/credential/supports_credentials.py

+        a catalog but the credential provider couldn't generate the credential
+        for the catalog, like S3 token credential provider only generate
+        credential for the specific object like Fileset,Table. There will be at
+         most one credential for one credential type.


Remove the prefix whitespace in this line.

jerryshao · 2024-12-12T06:01:31Z

Generally LGTM from the logic, @xunliu @noidname01 would you please help to review this PR, to see if it follows Python's convention.

noidname01 · 2024-12-12T06:49:01Z

LGTM!

xunliu · 2024-12-12T06:33:25Z

clients/client-python/gravitino/api/credential/credential.py

Look like lose two java function in the Python side

void initialize(Map<String, String> credentialInfo, long expireTimeInMs);

default Map<String, String> toProperties()
Don't we need these two function?

no, toProperties is used to serialize credential objects in server side, initialize is something like deserialize in client side, for python implementation, we only focus on the deserialize part, so create the credential with credentialInfo and expireTimeInMs.

xunliu · 2024-12-12T06:35:16Z

clients/client-python/gravitino/api/credential/gcs_token_credential.py

+    GCS_TOKEN_CREDENTIAL_TYPE = "gcs-token"
+    _GCS_TOKEN_NAME = "token"
+
+    _expire_time_in_ms = 0


Please add variable types for these variables

xunliu · 2024-12-12T06:36:13Z

clients/client-python/gravitino/api/credential/oss_token_credential.py

+    OSS_TOKEN_CREDENTIAL_TYPE = "oss-token"
+    _GRAVITINO_OSS_SESSION_ACCESS_KEY_ID = "oss-access-key-id"
+    _GRAVITINO_OSS_SESSION_SECRET_ACCESS_KEY = "oss-secret-access-key"
+    _GRAVITINO_OSS_TOKEN = "oss-security-token"


Please add variable types for these variables

xunliu · 2024-12-12T06:40:36Z

clients/client-python/gravitino/api/credential/s3_secret_key_credential.py

+    S3_SECRET_KEY_CREDENTIAL_TYPE = "s3-secret-key"
+    _GRAVITINO_S3_STATIC_ACCESS_KEY_ID = "s3-access-key-id"
+    _GRAVITINO_S3_STATIC_SECRET_ACCESS_KEY = "s3-secret-access-key"


Please add variable types for these variables

xunliu · 2024-12-12T06:48:54Z

clients/client-python/gravitino/utils/credential_utils.py

I think we can keep the consistent Java class name CredentialFactory.

xunliu · 2024-12-12T06:58:18Z

clients/client-python/gravitino/catalog/base_schema_catalog.py

@@ -74,6 +79,11 @@ def __init__(
        self.rest_client = rest_client
        self._catalog_namespace = catalog_namespace

+        metadata_object = MetadataObjectImpl([name], MetadataObject.Type.CATALOG)
+        self._credential_operations = MetadataObjectCredentialOperations(


I think better to change _credential_operations to _objectCredentialOperations, keep consistent Java side.
and add _objectCredentialOperations into BaseSchemaCatalog member variable.

xunliu · 2024-12-12T07:01:39Z

clients/client-python/gravitino/client/generic_fileset.py

+    def __init__(
+        self, fileset: FilesetDTO, rest_client: HTTPClient, full_namespace: Namespace
+    ):
+        self._fileset = fileset


Please add _fileset into GenericFileset member variable.

xunliu · 2024-12-12T07:02:52Z

clients/client-python/gravitino/client/metadata_object_credential_operations.py

+        self._rest_client = rest_client
+        metadata_object_type = metadata_object.type().value
+        metadata_object_name = metadata_object.name()
+        self._request_path = (
+            f"api/metalakes/{metalake_name}objects/{metadata_object_type}/"
+            f"{metadata_object_name}/credentials"
+        )


Please add _rest_client and _request_path into MetadataObjectCredentialOperations member variable.

xunliu

LGTM

…pache#5777) ### What changes were proposed in this pull request? supports credential API in python client ```python catalog = gravitino_client.load_catalog(catalog_name) catalog.as_fileset_catalog().support_credentials().get_credentials() fileset = catalog.as_fileset_catalog().load_fileset( NameIdentifier.of("schema", "fileset") ) credentials = fileset.support_credentials().get_credentials() ``` ### Why are the changes needed? Fix: apache#5623 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? 1. add UT 2. setup a Gravitino server which returns credential, test with python client

FANNG1 marked this pull request as draft December 6, 2024 01:06

FANNG1 force-pushed the credential-python branch from 5f02995 to 020bca1 Compare December 6, 2024 09:04

credential python client

e9b21b3

FANNG1 force-pushed the credential-python branch from 85f8540 to e9b21b3 Compare December 9, 2024 06:21

FANNG1 added 7 commits December 9, 2024 14:32

Initialize

409a718

Initialize

9f09986

Initialize

4a7e024

Initialize

44f77dd

Initialize

6ffd942

Initialize

b393616

Initialize

20a8a5e

FANNG1 changed the title ~~[SIP] Credential python client~~ [#5623] feat(python): supports credential API in python client Dec 9, 2024

FANNG1 marked this pull request as ready for review December 9, 2024 13:18

jerryshao requested review from noidname01, xunliu, xloya and yuqi1129 and removed request for xunliu December 10, 2024 02:32

jerryshao reviewed Dec 10, 2024

View reviewed changes

clients/client-python/gravitino/api/credential/credential.py Outdated Show resolved Hide resolved

clients/client-python/gravitino/api/credential/oss_token_credential.py Show resolved Hide resolved

FANNG1 added 3 commits December 10, 2024 14:25

fix comment

9fbf92b

add argument check

5193262

add argument check

a6572cb

xloya reviewed Dec 10, 2024

View reviewed changes

clients/client-python/gravitino/client/metadata_object_credential_operations.py Outdated Show resolved Hide resolved

clients/client-python/gravitino/dto/responses/credential_response.py Show resolved Hide resolved

FANNG1 added 2 commits December 11, 2024 10:10

fix comment

9a9ecc8

fix comment

c340f7e

yuqi1129 reviewed Dec 11, 2024

View reviewed changes

fix comment

593e05d

yuqi1129 reviewed Dec 12, 2024

View reviewed changes

jerryshao reviewed Dec 12, 2024

View reviewed changes

FANNG1 added 2 commits December 12, 2024 11:16

fix comment

0e30e90

fix comment

e1fa221

noidname01 approved these changes Dec 12, 2024

View reviewed changes

xunliu reviewed Dec 12, 2024

View reviewed changes

fix comment

35b543b

xunliu approved these changes Dec 12, 2024

View reviewed changes

xunliu merged commit c98c151 into apache:main Dec 12, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#5623] feat(python): supports credential API in python client #5777

[#5623] feat(python): supports credential API in python client #5777

FANNG1 commented Dec 6, 2024 •

edited

Loading

FANNG1 commented Dec 9, 2024

FANNG1 commented Dec 11, 2024

yuqi1129 Dec 12, 2024

FANNG1 Dec 12, 2024

yuqi1129 Dec 12, 2024

FANNG1 Dec 12, 2024

jerryshao Dec 12, 2024

FANNG1 Dec 12, 2024

jerryshao Dec 12, 2024

FANNG1 Dec 12, 2024

jerryshao Dec 12, 2024

FANNG1 Dec 12, 2024

jerryshao commented Dec 12, 2024

noidname01 commented Dec 12, 2024

xunliu Dec 12, 2024

FANNG1 Dec 12, 2024

xunliu Dec 12, 2024

FANNG1 Dec 12, 2024

xunliu Dec 12, 2024

FANNG1 Dec 12, 2024

xunliu Dec 12, 2024

FANNG1 Dec 12, 2024

xunliu Dec 12, 2024

FANNG1 Dec 12, 2024

xunliu Dec 12, 2024

FANNG1 Dec 12, 2024

xunliu Dec 12, 2024

FANNG1 Dec 12, 2024

FANNG1 Dec 12, 2024

xunliu Dec 12, 2024

FANNG1 Dec 12, 2024

xunliu left a comment

[#5623] feat(python): supports credential API in python client #5777

[#5623] feat(python): supports credential API in python client #5777

Conversation

FANNG1 commented Dec 6, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

FANNG1 commented Dec 9, 2024

FANNG1 commented Dec 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented Dec 12, 2024

noidname01 commented Dec 12, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xunliu left a comment

Choose a reason for hiding this comment

FANNG1 commented Dec 6, 2024 •

edited

Loading