[#9564] fix(authn): Fix Flink connector oauth2 mode #9618

jerqi · 2026-01-05T07:07:13Z

What changes were proposed in this pull request?

Fix Flink connector oauth2 mode.

Why are the changes needed?

This is a follow up PR.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

By hand.

...link/src/main/java/org/apache/gravitino/flink/connector/catalog/GravitinoCatalogManager.java

Copilot

Pull request overview

This PR fixes OAuth2 authentication mode configuration in the Flink connector. The changes address issues where OAuth2 configuration keys were not properly handled when building the Gravitino client.

Key changes:

Adds a constant for the "oauth2" string to improve maintainability
Fixes OAuth2 client configuration by removing OAuth-specific keys before passing config to GravitinoAdminClient
Updates documentation example to show correct credential format

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
GravitinoCatalogStoreFactoryOptions.java	Adds OAUTH2 constant to centralize the "oauth2" string value
GravitinoCatalogManager.java	Refactors OAuth2 client building logic to remove OAuth config keys from client config map and use the new OAUTH2 constant
flink-authentication-with-gravitino.md	Updates documentation to show correct OAuth2 credential format

Copilot · 2026-01-05T07:11:13Z

...link/src/main/java/org/apache/gravitino/flink/connector/catalog/GravitinoCatalogManager.java

-        String.format(
-            "OAuth2 authentication requires: %s, %s, %s, and %s",
+
+    // Remove OAuth config from client config map, Because


The word 'Because' should not be capitalized mid-sentence. Change 'Because' to 'because'.

Suggested change

// Remove OAuth config from client config map, Because

// Remove OAuth config from client config map, because

Copilot · 2026-01-05T07:11:13Z

...link/src/main/java/org/apache/gravitino/flink/connector/catalog/GravitinoCatalogManager.java

+    // Remove OAuth config from client config map, Because
+    // GravitinoAdminClient.builder().withClientConfig() does not recognize these keys.


The comment should explain why removing these keys is necessary. Consider clarifying that these OAuth2-specific keys would cause validation errors or unexpected behavior if passed to the GravitinoAdminClient builder.

Suggested change

// Remove OAuth config from client config map, Because

// GravitinoAdminClient.builder().withClientConfig() does not recognize these keys.

// Remove OAuth-specific config entries from the client config map. These keys are only

// used to construct the OAuth2 token provider and are not valid GravitinoAdminClient

// client configuration options; passing them to withClientConfig() could cause validation

// errors or other unexpected behavior.

Copilot · 2026-01-05T07:11:13Z

...link/src/main/java/org/apache/gravitino/flink/connector/catalog/GravitinoCatalogManager.java

+
+    // Remove OAuth config from client config map, Because
+    // GravitinoAdminClient.builder().withClientConfig() does not recognize these keys.
+    Set<String> oauthConfigKeys =
+        Sets.newHashSet(
+            GravitinoCatalogStoreFactoryOptions.AUTH_TYPE,
            GravitinoCatalogStoreFactoryOptions.OAUTH2_SERVER_URI,
            GravitinoCatalogStoreFactoryOptions.OAUTH2_CREDENTIAL,
            GravitinoCatalogStoreFactoryOptions.OAUTH2_TOKEN_PATH,
-            GravitinoCatalogStoreFactoryOptions.OAUTH2_SCOPE));
+            GravitinoCatalogStoreFactoryOptions.OAUTH2_SCOPE);
+    for (String key : oauthConfigKeys) {
+      config.remove(key);
+    }


The new logic for removing OAuth config keys from the client config map lacks test coverage. Add a unit test to verify that these specific keys are properly removed from the config before it's passed to the GravitinoAdminClient builder.

It's hard to add tests. I test it by hand.

jerqi · 2026-01-07T09:52:44Z

@FANNG1 Could u take a look at this pull request?

yuqi1129 · 2026-01-08T07:06:28Z

@jerqi
Does it need to cherry-pick to branch-1.1?

jerqi · 2026-01-08T07:08:28Z

@jerqi Does it need to cherry-pick to branch-1.1?

No need. The branch 1.1 doesn't have the similar issue. I don't back port the pull request which bring this issue to branch-1.1.

FANNG1 · 2026-01-08T07:22:01Z

...link/src/main/java/org/apache/gravitino/flink/connector/catalog/GravitinoCatalogManager.java

    // Only OAuth is explicitly configured; otherwise follow Flink security (Kerberos if enabled,
    // simple auth otherwise).
-    if (AuthenticatorType.OAUTH.name().equalsIgnoreCase(authType)) {
+    if (GravitinoCatalogStoreFactoryOptions.OAUTH2.equalsIgnoreCase(authType)) {


Any reason for using a new constant?

OAUTH name is oauth. We need OAUTH2 here.
Spark use the constants in the catalog-common.
So I defined a new constant here.
I will refactor related code in the future. I can create an issue for this.

FANNG1 · 2026-01-08T07:22:37Z

...link/src/main/java/org/apache/gravitino/flink/connector/catalog/GravitinoCatalogManager.java

                authType, GravitinoCatalogStoreFactoryOptions.AUTH_TYPE));
      }

+      LOG.info("Flink security enabled: {}", UserGroupInformation.isSecurityEnabled());


How about simple by using one log?

OK, I just feel it will be a little long.

FANNG1 · 2026-01-08T07:24:51Z

LGTM, some minor comments

jerryshao · 2026-01-08T08:53:07Z

Shall we backport branch-1.1?

jerqi · 2026-01-08T09:07:03Z

Shall we backport branch-1.1?
This is the issue #9564 follow up PR. Because #9564 didn't back port to branch 1.1. So branch 1.1 won't have the similar issue.

[apache#9564] fix(authn): Fix Flink connector oauth2 mode

94386c4

jerqi force-pushed the fix_oauth2 branch from 5ef8d6e to 94386c4 Compare January 5, 2026 07:07

jerqi commented Jan 5, 2026

View reviewed changes

...link/src/main/java/org/apache/gravitino/flink/connector/catalog/GravitinoCatalogManager.java Show resolved Hide resolved

jerqi self-assigned this Jan 5, 2026

jerqi requested a review from Copilot January 5, 2026 07:10

Copilot AI reviewed Jan 5, 2026

View reviewed changes

jerqi requested a review from FANNG1 January 7, 2026 06:28

address comments

1c0ad9b

jerqi force-pushed the fix_oauth2 branch from d1d4e70 to 1c0ad9b Compare January 7, 2026 06:40

FANNG1 reviewed Jan 8, 2026

View reviewed changes

Use one log

31ac83c

FANNG1 approved these changes Jan 8, 2026

View reviewed changes

jerqi merged commit 38c420d into apache:main Jan 8, 2026
23 checks passed

jerqi deleted the fix_oauth2 branch January 8, 2026 08:52

	// Remove OAuth config from client config map, Because
	// Remove OAuth config from client config map, because

		// Remove OAuth config from client config map, Because
		// GravitinoAdminClient.builder().withClientConfig() does not recognize these keys.

-    // Remove OAuth config from client config map, Because
-    // GravitinoAdminClient.builder().withClientConfig() does not recognize these keys.
+    // Remove OAuth-specific config entries from the client config map. These keys are only
+    // used to construct the OAuth2 token provider and are not valid GravitinoAdminClient
+    // client configuration options; passing them to withClientConfig() could cause validation
+    // errors or other unexpected behavior.

[#9564] fix(authn): Fix Flink connector oauth2 mode #9618

[#9564] fix(authn): Fix Flink connector oauth2 mode #9618

Uh oh!

Conversation

jerqi commented Jan 5, 2026

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?

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

jerqi Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

jerqi Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

jerqi Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

jerqi commented Jan 7, 2026

Uh oh!

yuqi1129 commented Jan 8, 2026

Uh oh!

jerqi commented Jan 8, 2026

Uh oh!

FANNG1 Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

jerqi Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

FANNG1 Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

jerqi Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

FANNG1 commented Jan 8, 2026

Uh oh!

Uh oh!

jerryshao commented Jan 8, 2026

Uh oh!

jerqi commented Jan 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants