-
Notifications
You must be signed in to change notification settings - Fork 564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(connector): MSK connection by AWS IAM #16625
Conversation
|
GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
---|---|---|---|---|---|
9425213 | Triggered | Generic Password | ff2a92c | e2e_test/source/cdc/cdc.validate.postgres.slt | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
license-eye has totally checked 5070 files.
Valid | Invalid | Ignored | Fixed |
---|---|---|---|
2168 | 1 | 2901 | 0 |
Click to see the invalid file list
- src/connector/src/source/kafka/client_context.rs
Cargo.toml
Outdated
@@ -118,7 +118,7 @@ axum = "=0.7.4" # TODO: 0.7.5+ does not work with current toolchain | |||
etcd-client = { package = "madsim-etcd-client", version = "0.4" } | |||
futures-async-stream = "0.2.9" | |||
hytra = "0.1" | |||
rdkafka = { package = "madsim-rdkafka", version = "0.3.4", features = [ | |||
rdkafka = { package = "madsim-rdkafka", version = "0.4.0", features = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to refactor rdkafka. madsim-rs/madsim#207
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bumped to v0.4.1 to add some missing APIs madsim-rs/madsim#208
Didn't dive into details. /// SASL mechanism if SASL is enabled. Currently support PLAIN, SCRAM and GSSAPI.
#[serde(rename = "properties.sasl.mechanism")]
sasl_mechanism: Option<String>, |
Also users can provide ak/sk and assume role arn. Just like kinesis and pulsar source. |
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Please update the "Documentation" section in PR description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM
// AWS_MSK_IAM | ||
if self.is_aws_msk_iam() { | ||
config.set("security.protocol", "SASL_SSL"); | ||
config.set("sasl.mechanism", "OAUTHBEARER"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OAUTHBEARER
was mentioned as an alternative way in https://github.com/aws/aws-msk-iam-auth?tab=readme-ov-file#configuring-a-kafka-client-to-use-aws-iam-with-sasl-oauthbearer-mechanism. Not quite understand this part, may I ask the difference between sasl.mechanism = AWS_MSK_IAM
and OAUTHBEARER
? Is the first one actually implemented with the second one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AWS_MSK_IAM is not a rdkafka value while OAUTHBEARER is. When using AWS_MSK_IAM, rdkafka mechanism will be set to OAUTHBEARER. I use AWS_MSK_IAM because we want't user to explicitly tell RW that an AWS service was used instead of implicitly by providing ak/sk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rest LGTM
- name: region | ||
field_type: String | ||
required: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about adding a aws
prefix for these props? I am not sure if GCP or Azure has the same stuff 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also happens to those using AwsAuthProp, including bigquery and pulsar😅
https://github.com/risingwavelabs/risingwave/pull/13513/files#diff-c38146b9d974f4299ee4a603f82bbde9fda633c8f959dbb676fcfa7276d1291eR458-R460
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe open another pr to add prefix for AwsAuthProp while keeping the backward compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
Signed-off-by: Runji Wang <wangrunji0408@163.com> Co-authored-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com> Co-authored-by: Runji Wang <wangrunji0408@163.com>
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Use AWS IAM to build the connection for MSK.
Checklist
./risedev check
(or alias,./risedev c
)Documentation
To use AWS IAM to connect to MSK,
properties.sasl.mechanism
should be set toAWS_MSK_IAM
.Also the following field:
region
(optional) the region MSK region located.access_key
(required) AWS access key ID for accessing the MSK.secret_key
(required) should be provided. AWS seceret key for accessing the MSK.arn
(optional)external_id
(optional)session_token
(optional)
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.