feat(stream): add nkey and jwt auth methods for nats connector #12227

yufansong · 2023-09-12T07:28:02Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

This PR try to refactor the nats client connection code and add some new methods (credential) to connect. The nats rust package support it here.

The connection with/without passworda and username already test and correct.
Test credential connection methods in sink parts. If you want to test it, firstly you need to make sure you launch the nats server in this way which can enable the credential auth methods. Then you can connect it in this way.

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.
Add filed: connect_mode, value can be user_and_password, credential or plain.
When use credential, need to fill jwt and nkey
When use user_and_password need to fild username and password
If plain, no extra filed required.

code example:

-- plain
CREATE TABLE test
(
  id integer,
  name varchar,
)
WITH (
  connector='nats',
    server_url='0.0.0.0:4222',
    subject='event1',
    username='a',
    password='b',
    connect_mode='plain'
) FORMAT PLAIN ENCODE JSON;

-- user pass
CREATE TABLE test
(
  id integer,
  name varchar,
)
WITH (
  connector='nats',
    server_url='0.0.0.0:4222',
    subject='event1',
    username='a',
    password='b',
    connect_mode='user_and_password'
) FORMAT PLAIN ENCODE JSON;

-- make sure replace the jwt and nkey as your own value
CREATE SINK s_nats FROM personnel WITH (
    connector='nats',
    server_url='0.0.0.0:4222,0.0.0.0:4221',
    subject='event1',
    type='append-only',
    force_append_only='true',
    connect_mode='credential',
    jwt='xxxx,
    nkey='xxxx',

);

neverchanje · 2023-09-13T06:59:38Z

Thanks Yufan! I wonder how did you test this functionality? There is no unit test nor intergation test.

yufansong · 2023-09-13T07:04:24Z

Thanks Yufan! I wonder how did you test this functionality? There is no unit test nor intergation test.

Yeah, I have checked other intergration test code, will add it like others. For unit test, currently the nats logic is not complex. I didn't find too much nessesary to add it. What do you think?

I think it may take several days to write the test code. So after finishing this PR and that PR, and no more urgent feature appears, I will work in the test parts.

tabVersion · 2023-09-13T07:13:18Z

lets do some validation before merging into main

codecov · 2023-09-13T07:53:27Z

Codecov Report

Merging #12227 (a0b34e8) into main (f304ed2) will decrease coverage by 0.03%.
Report is 15 commits behind head on main.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main   #12227      +/-   ##
==========================================
- Coverage   69.85%   69.83%   -0.03%     
==========================================
  Files        1417     1417              
  Lines      235533   235599      +66     
==========================================
- Hits       164533   164531       -2     
- Misses      71000    71068      +68

Flag	Coverage Δ
rust	`69.83% <0.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
src/connector/src/common.rs	`2.26% <0.00%> (-0.45%)`	⬇️
src/connector/src/source/nats/enumerator/mod.rs	`0.00% <0.00%> (ø)`
src/connector/src/source/nats/mod.rs	`0.00% <ø> (ø)`
src/connector/src/source/nats/source/message.rs	`0.00% <0.00%> (ø)`
src/connector/src/source/nats/source/reader.rs	`0.00% <0.00%> (ø)`
src/connector/src/source/nats/split.rs	`0.00% <0.00%> (ø)`

... and 3 files with indirect coverage changes

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

neverchanje · 2023-09-13T15:10:23Z

I just want to ensure there is manual testing at least. Once developed, we don't expect extra back-and-forth communication with users

…liest, by timestamp (#12176)

neverchanje · 2023-09-18T06:03:23Z

src/connector/src/common.rs

+    #[serde(rename = "nats.connect_mode")]
+    pub connect_mode: Option<String>,
+    #[serde(rename = "username")]
    pub user: Option<String>,
-    #[serde(rename = "nats.password")]
+    #[serde(rename = "password")]
    pub password: Option<String>,
+    #[serde(rename = "nats.jwt")]
+    pub jwt: Option<String>,
+    #[serde(rename = "nats.nkey")]
+    pub nkey: Option<String>,


I wanted to propose stripping off all 'nats.' or 'cassandra.' prefixes given that it has been specified in 'connector'.

tabVersion

rest LGTM, good work!

tabVersion · 2023-09-18T07:34:58Z

src/connector/src/common.rs

+                    ));
+                }
+            }
+            _ => {}


does it mean rw also accepts Nats without auth?
if so, user should connect with 'plain' or sth else, instead of leaving it empty.
also, we need to reject unrecognized string here.

tabVersion · 2023-09-18T07:47:52Z

src/connector/src/common.rs

+            "-----BEGIN NATS USER JWT-----\n{}\n------END NATS USER JWT------\n\n\
+                         ************************* IMPORTANT *************************\n\
+                         NKEY Seed printed below can be used to sign and prove identity.\n\
+                         NKEYs are sensitive and should be treated as secrets.\n\n\
+                         -----BEGIN USER NKEY SEED-----\n{}\n------END USER NKEY SEED------\n\n\
+                         *************************************************************",


incredible the func need the format

Yes, this is what the nats documents said, and also what syntropy told me

hzxa21

Cargo.lock changes are related to the nats dependency. LGTM!

github-actions bot added the type/feature label Sep 12, 2023

github-actions bot requested a review from a team as a code owner September 12, 2023 07:29

yufansong requested a review from tabVersion September 12, 2023 07:44

yufansong marked this pull request as draft September 12, 2023 07:45

yufansong requested a review from neverchanje September 12, 2023 07:46

yufansong mentioned this pull request Sep 13, 2023

authenticate NATS by JWT and NKEY #12220

Closed

add credential methods

9c9871e

yufansong force-pushed the yufan/nats-connect branch from ecae04e to 9c9871e Compare September 13, 2023 06:40

yufansong marked this pull request as ready for review September 13, 2023 06:41

Fix "cargo-hakari"

427e633

yufansong mentioned this pull request Sep 13, 2023

feat(stream): support set nats consumer deliver policy as latest, earliest, by sequence, by timestamp #12176

Merged

8 tasks

format code, change nats.user->username, nats.password->password

8cc5120

yufansong and others added 2 commits September 13, 2023 22:19

feat(stream): support set nats consumer deliver policy as latest, ear…

ddbf1f3

…liest, by timestamp (#12176)

add max bytes

dd8cfdf

neverchanje reviewed Sep 18, 2023

View reviewed changes

yufansong added 4 commits September 17, 2023 23:13

Merge branch 'main' into yufan/nats-connect

d3ed07c

remove prefix

ec484aa

remove prefix

64c7ac3

format check

1e25f4c

tabVersion approved these changes Sep 18, 2023

View reviewed changes

add plain

a0b34e8

yufansong requested a review from hzxa21 September 18, 2023 22:14

hzxa21 approved these changes Sep 19, 2023

View reviewed changes

yufansong added this pull request to the merge queue Sep 19, 2023

Merged via the queue into main with commit b17569d Sep 19, 2023

yufansong deleted the yufan/nats-connect branch September 19, 2023 03:41

xxchan mentioned this pull request Sep 20, 2023

chore: bump ed25519-dalek package version #11986

Closed

yufansong mentioned this pull request Sep 20, 2023

fix: remove cargo audit --ignore RUSTSEC-2022-0093 #12465

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stream): add nkey and jwt auth methods for nats connector #12227

feat(stream): add nkey and jwt auth methods for nats connector #12227

yufansong commented Sep 12, 2023 •

edited

Loading

neverchanje commented Sep 13, 2023

yufansong commented Sep 13, 2023 •

edited

Loading

tabVersion commented Sep 13, 2023

codecov bot commented Sep 13, 2023 •

edited

Loading

neverchanje commented Sep 13, 2023

neverchanje Sep 18, 2023

yufansong Sep 18, 2023

tabVersion left a comment

tabVersion Sep 18, 2023

tabVersion Sep 18, 2023

yufansong Sep 18, 2023 •

edited

Loading

hzxa21 left a comment

feat(stream): add nkey and jwt auth methods for nats connector #12227

feat(stream): add nkey and jwt auth methods for nats connector #12227

Conversation

yufansong commented Sep 12, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

neverchanje commented Sep 13, 2023

yufansong commented Sep 13, 2023 • edited Loading

tabVersion commented Sep 13, 2023

codecov bot commented Sep 13, 2023 • edited Loading

Codecov Report

neverchanje commented Sep 13, 2023

neverchanje Sep 18, 2023

Choose a reason for hiding this comment

yufansong Sep 18, 2023

Choose a reason for hiding this comment

tabVersion left a comment

Choose a reason for hiding this comment

tabVersion Sep 18, 2023

Choose a reason for hiding this comment

tabVersion Sep 18, 2023

Choose a reason for hiding this comment

yufansong Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

hzxa21 left a comment

Choose a reason for hiding this comment

yufansong commented Sep 12, 2023 •

edited

Loading

yufansong commented Sep 13, 2023 •

edited

Loading

codecov bot commented Sep 13, 2023 •

edited

Loading

yufansong Sep 18, 2023 •

edited

Loading