SNOW-189106 Kafka Connector to Support External OAuth #671

sfc-gh-alhuang · 2023-07-24T23:14:38Z

Things to do before merging this PR

According to @sfc-gh-japatel, this PR should be merge after the OAuth is support in SDK.
Update the Ingest SDK version, and modify the test should make this work. All the tests required modified are commented with TODOs.
After this PR is checked in, update local profile.json to pass the test.

Change

Support OAuth authenticator of streaming ingest with KC. For more details, please refer to the doc. Jira link: SNOW-189106

Test

OAuth authenticator for streaming ingest sdk is not released yet, tests with streaming ingest client are currently disabled until SNOW-352846 is released. Three profile.json.gpg files are updated for OAuth testing.

sfc-gh-rcheng

Added some thoughts and requested some test changes. Don't have full context on this change, so lets wait for a review from toby/jay

src/main/java/com/snowflake/kafka/connector/SnowflakeSinkConnectorConfig.java

src/main/java/com/snowflake/kafka/connector/SnowflakeSinkConnector.java

src/main/java/com/snowflake/kafka/connector/Utils.java

src/test/java/com/snowflake/kafka/connector/SnowflakeSinkTaskForStreamingIT.java

src/test/java/com/snowflake/kafka/connector/internal/streaming/SnowflakeSinkServiceV2IT.java

codecov · 2023-07-25T20:44:11Z

Codecov Report

Merging #671 (ac23d7d) into master (52f0a8a) will increase coverage by 0.07%.
Report is 3 commits behind head on master.
The diff coverage is 91.72%.

@@            Coverage Diff             @@
##           master     #671      +/-   ##
==========================================
+ Coverage   87.88%   87.96%   +0.07%     
==========================================
  Files          50       50              
  Lines        4144     4261     +117     
  Branches      449      468      +19     
==========================================
+ Hits         3642     3748     +106     
- Misses        332      342      +10     
- Partials      170      171       +1

Files Changed	Coverage Δ
...owflake/kafka/connector/internal/SnowflakeURL.java	`96.87% <ø> (+3.12%)`	⬆️
...a/connector/internal/streaming/StreamingUtils.java	`88.77% <80.00%> (-1.59%)`	⬇️
...main/java/com/snowflake/kafka/connector/Utils.java	`91.51% <86.44%> (-1.19%)`	⬇️
...wflake/kafka/connector/SnowflakeSinkConnector.java	`94.49% <100.00%> (+3.67%)`	⬆️
.../kafka/connector/SnowflakeSinkConnectorConfig.java	`88.51% <100.00%> (+0.31%)`	⬆️
...wflake/kafka/connector/internal/InternalUtils.java	`93.79% <100.00%> (+1.48%)`	⬆️
...or/internal/SnowflakeConnectionServiceFactory.java	`96.55% <100.00%> (-0.12%)`	⬇️
...lake/kafka/connector/internal/SnowflakeErrors.java	`98.05% <100.00%> (+1.11%)`	⬆️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

sfc-gh-alhuang · 2023-07-25T23:56:35Z

pom.xml

@@ -156,6 +156,7 @@
                <version>2.22.0</version>
                <configuration>
                    <skipTests>${skipUnitTests}</skipTests>
+                    <trimStackTrace>false</trimStackTrace>


This will be deleted afterwards.

before you push this pr or in a separate PR?

I'll deleted this before pushing this PR.

.github/scripts/profile.json.gpg

sfc-gh-tjones · 2023-08-01T21:37:50Z

src/main/java/com/snowflake/kafka/connector/Utils.java

+                  try {
+                    return e.getKey() + "=" + URLEncoder.encode(e.getValue(), "UTF-8");
+                  } catch (UnsupportedEncodingException ex) {
+                    throw SnowflakeErrors.ERROR_1004.getException(ex);


Not covered by code coverage.

I think this could only happens if the encoder name is not UTF-8. Ref

sfc-gh-tjones · 2023-08-01T21:38:06Z

src/main/java/com/snowflake/kafka/connector/Utils.java

+            return respBody.get(tokenType).toString().replaceAll("^\"|\"$", "");
+          }
+        }
+      } catch (Exception ignored) {


Why is this being ignored?

I think it could be ignored and retries until the maximum retries count is reach.

lets add a logger.warn here and print out message from exception.

sfc-gh-tjones

@sfc-gh-japatel @sfc-gh-rcheng can you be the primary reviewers for this? Thanks!

sfc-gh-japatel · 2023-08-03T22:22:21Z

src/main/java/com/snowflake/kafka/connector/Utils.java

+  public static final String SF_AUTHENTICATOR = "snowflake.authenticator";
+  public static final String SF_OAUTH_CLIENT_ID = "snowflake.oauth.client.id";
+  public static final String SF_OAUTH_CLIENT_SECRET = "snowflake.oauth.client.secret";
+  public static final String SF_OAUTH_REFRESH_TOKEN = "snowflake.refresh.token";


thinking out loud: do we need oauth here as well to be consistent? since it only applies to oauth
snowflake.oauth.refresh.token

Forgot to add oauth here. Added oauth prefix since cliend_id might seems misleading, what do you think?

makes sense to me

src/main/java/com/snowflake/kafka/connector/Utils.java

src/main/java/com/snowflake/kafka/connector/SnowflakeSinkConnectorConfig.java

sfc-gh-japatel · 2023-08-03T22:31:36Z

src/main/java/com/snowflake/kafka/connector/SnowflakeSinkConnectorConfig.java

+            AUTHENTICATOR,
+            Type.STRING,
+            "",
+            Importance.LOW,


any reason this is low and rest are high importance?

I thought this is an optional parameter when we use jwt. While client_id , client_secret and refresh_token is required when using oauth. Do you think we should make all of the low?

src/main/java/com/snowflake/kafka/connector/Utils.java

src/main/java/com/snowflake/kafka/connector/SnowflakeSinkConnectorConfig.java

src/main/java/com/snowflake/kafka/connector/Utils.java

sfc-gh-japatel · 2023-08-03T22:50:26Z

src/main/java/com/snowflake/kafka/connector/Utils.java

+
+      // Exponential backoff retries
+      try {
+        Thread.sleep((1L << retries) * 1000L);


where is the retry logic?

Do we have a retry logic that can be reused in KC?

src/main/java/com/snowflake/kafka/connector/internal/InternalUtils.java

src/main/java/com/snowflake/kafka/connector/internal/OAuthConstants.java

sfc-gh-japatel · 2023-08-15T21:04:46Z

src/test/java/com/snowflake/kafka/connector/ConnectorConfigTest.java

@@ -7,6 +7,7 @@
 import static com.snowflake.kafka.connector.internal.TestUtils.getConfig;
 import static org.junit.Assert.assertEquals;

+import com.snowflake.kafka.connector.internal.OAuthConstants;


overall I am seeing great value addition for all types of tests.
I dont see End to End tests though, do you plan to add it?

Discussed offline. Will added in another PR.

src/main/java/com/snowflake/kafka/connector/Utils.java

sfc-gh-japatel

lgtm!
Suggested some minor changes but none of them are blocker. Looking forward to end to end tests. Thanks for adding UT, IT tests but running it real kafka connect env might have its own issues which might not unravel here.

Good stuff @sfc-gh-alhuang

sfc-gh-tzhang

Left some comments, PTAL! My main concern is about code duplication, please see if anything could be reused, thanks!

sfc-gh-tzhang · 2023-08-16T01:38:51Z

src/main/java/com/snowflake/kafka/connector/SnowflakeSinkConnector.java

-        || connectorConfigs.getOrDefault(Utils.PRIVATE_KEY_PASSPHRASE, "").contains("${file:"))
+    // If using snowflake_jwt and authentication, and private key or private key passphrase is
+    // provided through file, skip validation
+    if (connectorConfigs


Looks like we're adding a new option, could you ask @sfc-gh-xhuang or @sfc-gh-lema to update our online doc?

src/main/java/com/snowflake/kafka/connector/Utils.java

src/main/java/com/snowflake/kafka/connector/internal/OAuthConstants.java

sfc-gh-tzhang · 2023-08-16T01:56:15Z

src/main/java/com/snowflake/kafka/connector/Utils.java

+
+      // Exponential backoff retries
+      try {
+        Thread.sleep((1L << retries) * 1000L);


Do we have a retry logic that can be reused in KC?

sfc-gh-tzhang

LGTM, ship it!

src/main/java/com/snowflake/kafka/connector/Utils.java

src/test/java/com/snowflake/kafka/connector/SnowflakeSinkTaskForStreamingIT.java

eliax1996 · 2024-03-19T09:03:57Z

The linked doc in the pr isn't accessible freely :)
don't know if its intentional but opens a form to require the permission accessing it

add oauth

c49a670

sfc-gh-alhuang requested review from sfc-gh-japatel, sfc-gh-tzhang, sfc-gh-tjones, sfc-gh-rcheng and a team as code owners July 24, 2023 23:14

fix bug and add surefire log

f1d7abd

sfc-gh-rcheng requested changes Jul 25, 2023

View reviewed changes

try to pass merge gate ajajjaaajaja

b0f82d7

resolve revi's change request

8e22a60

sfc-gh-alhuang commented Jul 25, 2023

View reviewed changes

sfc-gh-alhuang added 2 commits July 25, 2023 19:15

remove httpclient log

6613c7d

increase code coverage

ac23d7d

sfc-gh-xhuang reviewed Jul 27, 2023

View reviewed changes

.github/scripts/profile.json.gpg Outdated Show resolved Hide resolved

should be resetgit add pom.xml

fc1c46a

sfc-gh-tjones reviewed Aug 1, 2023

View reviewed changes

sfc-gh-japatel reviewed Aug 3, 2023

View reviewed changes

src/main/java/com/snowflake/kafka/connector/Utils.java Outdated Show resolved Hide resolved

sfc-gh-japatel reviewed Aug 3, 2023

View reviewed changes

src/main/java/com/snowflake/kafka/connector/SnowflakeSinkConnectorConfig.java Outdated Show resolved Hide resolved

sfc-gh-japatel reviewed Aug 3, 2023

View reviewed changes

src/main/java/com/snowflake/kafka/connector/SnowflakeSinkConnectorConfig.java Show resolved Hide resolved

sfc-gh-japatel reviewed Aug 3, 2023

View reviewed changes

src/main/java/com/snowflake/kafka/connector/Utils.java Outdated Show resolved Hide resolved

sfc-gh-japatel reviewed Aug 3, 2023

View reviewed changes

src/main/java/com/snowflake/kafka/connector/SnowflakeSinkConnectorConfig.java Outdated Show resolved Hide resolved

sfc-gh-japatel reviewed Aug 3, 2023

View reviewed changes

src/main/java/com/snowflake/kafka/connector/Utils.java Outdated Show resolved Hide resolved

sfc-gh-japatel reviewed Aug 3, 2023

View reviewed changes

src/main/java/com/snowflake/kafka/connector/Utils.java Show resolved Hide resolved

sfc-gh-japatel reviewed Aug 3, 2023

View reviewed changes

src/main/java/com/snowflake/kafka/connector/internal/InternalUtils.java Outdated Show resolved Hide resolved

sfc-gh-alhuang added 4 commits August 9, 2023 13:40

fix

033a4e3

fix pom.xml

0b02275

Merge branch 'master' into alhuang-oauth

22be9a3

format

ed0669b

sfc-gh-alhuang requested a review from sfc-gh-japatel August 14, 2023 18:40

sfc-gh-tzhang requested review from sfc-gh-tjones, sfc-gh-xhuang and sfc-gh-rcheng August 15, 2023 05:14

sfc-gh-japatel reviewed Aug 15, 2023

View reviewed changes

src/main/java/com/snowflake/kafka/connector/internal/OAuthConstants.java Outdated Show resolved Hide resolved

sfc-gh-japatel reviewed Aug 15, 2023

View reviewed changes

fix OAuth constants

202ec5c

sfc-gh-japatel reviewed Aug 15, 2023

View reviewed changes

src/main/java/com/snowflake/kafka/connector/Utils.java Outdated Show resolved Hide resolved

sfc-gh-japatel reviewed Aug 15, 2023

View reviewed changes

src/main/java/com/snowflake/kafka/connector/Utils.java Outdated Show resolved Hide resolved

sfc-gh-japatel reviewed Aug 15, 2023

View reviewed changes

src/main/java/com/snowflake/kafka/connector/Utils.java Outdated Show resolved Hide resolved

sfc-gh-japatel approved these changes Aug 15, 2023

View reviewed changes

sfc-gh-tzhang reviewed Aug 16, 2023

View reviewed changes

refactor

89417b6

sfc-gh-alhuang requested a review from sfc-gh-tzhang August 17, 2023 15:58

sfc-gh-tzhang approved these changes Aug 17, 2023

View reviewed changes

src/main/java/com/snowflake/kafka/connector/Utils.java Show resolved Hide resolved

src/main/java/com/snowflake/kafka/connector/Utils.java Show resolved Hide resolved

sfc-gh-alhuang commented Aug 17, 2023

View reviewed changes

src/test/java/com/snowflake/kafka/connector/SnowflakeSinkTaskForStreamingIT.java Show resolved Hide resolved

add todo

0a1d613

sfc-gh-rcheng approved these changes Aug 17, 2023

View reviewed changes

sfc-gh-alhuang merged commit 13cd421 into master Aug 18, 2023
30 checks passed

sfc-gh-alhuang deleted the alhuang-oauth branch August 18, 2023 19:59

khsoneji pushed a commit to confluentinc/snowflake-kafka-connector that referenced this pull request Oct 12, 2023

SNOW-189106 Kafka Connector to Support External OAuth (snowflakedb#671)

dc0852f

khsoneji pushed a commit to confluentinc/snowflake-kafka-connector that referenced this pull request Oct 12, 2023

SNOW-189106 Kafka Connector to Support External OAuth (snowflakedb#671)

97763f9

khsoneji pushed a commit to confluentinc/snowflake-kafka-connector that referenced this pull request Oct 12, 2023

SNOW-189106 Kafka Connector to Support External OAuth (snowflakedb#671)

c71cd16

khsoneji pushed a commit to confluentinc/snowflake-kafka-connector that referenced this pull request Oct 12, 2023

SNOW-189106 Kafka Connector to Support External OAuth (snowflakedb#671)

e40375f

EduardHantig pushed a commit to streamkap-com/snowflake-kafka-connector that referenced this pull request Feb 1, 2024

SNOW-189106 Kafka Connector to Support External OAuth (snowflakedb#671)

65d7991

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-189106 Kafka Connector to Support External OAuth #671

SNOW-189106 Kafka Connector to Support External OAuth #671

sfc-gh-alhuang commented Jul 24, 2023 •

edited

Loading

sfc-gh-rcheng left a comment

codecov bot commented Jul 25, 2023 •

edited

Loading

sfc-gh-alhuang Jul 25, 2023

sfc-gh-japatel Aug 3, 2023

sfc-gh-alhuang Aug 3, 2023

sfc-gh-tjones Aug 1, 2023

sfc-gh-alhuang Aug 2, 2023

sfc-gh-tjones Aug 1, 2023

sfc-gh-alhuang Aug 1, 2023

sfc-gh-japatel Aug 15, 2023

sfc-gh-tjones left a comment

sfc-gh-japatel Aug 3, 2023

sfc-gh-alhuang Aug 3, 2023

sfc-gh-xhuang Aug 4, 2023

sfc-gh-japatel Aug 3, 2023

sfc-gh-alhuang Aug 3, 2023

sfc-gh-japatel Aug 3, 2023

sfc-gh-alhuang Aug 3, 2023

sfc-gh-tzhang Aug 16, 2023

sfc-gh-japatel Aug 15, 2023

sfc-gh-alhuang Aug 15, 2023

sfc-gh-japatel left a comment

sfc-gh-tzhang left a comment

sfc-gh-tzhang Aug 16, 2023

sfc-gh-tzhang Aug 16, 2023

sfc-gh-tzhang left a comment

eliax1996 commented Mar 19, 2024

SNOW-189106 Kafka Connector to Support External OAuth #671

SNOW-189106 Kafka Connector to Support External OAuth #671

Conversation

sfc-gh-alhuang commented Jul 24, 2023 • edited Loading

Things to do before merging this PR

Change

Test

sfc-gh-rcheng left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 25, 2023 • edited Loading

Codecov Report

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

sfc-gh-tjones 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

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

sfc-gh-japatel left a comment

Choose a reason for hiding this comment

sfc-gh-tzhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-tzhang left a comment

Choose a reason for hiding this comment

eliax1996 commented Mar 19, 2024

sfc-gh-alhuang commented Jul 24, 2023 •

edited

Loading

codecov bot commented Jul 25, 2023 •

edited

Loading