Skip to content
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

[improve][io] Make connectors load sensitive fields from secrets #21675

Merged
merged 7 commits into from
Dec 11, 2023

Conversation

jiangpengcheng
Copy link
Contributor

@jiangpengcheng jiangpengcheng commented Dec 6, 2023

Fixes #21674

Main Issue: #xyz

PIP: #xyz

Motivation

Some connectors are using config to pass sensitive fields , like token, password, which is not secure

Modifications

use the IOConfileUtils.loadWithSecrets to load config for connectors

Verifying this change

  • Make sure that the change passes the CI checks.

  • This change added tests and can be verified as follows:

    • Added unittests for connectors load sensitive fields from secrets

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: jiangpengcheng#18

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 6, 2023
@shibd shibd closed this Dec 7, 2023
@shibd shibd reopened this Dec 7, 2023
@shibd shibd added release/3.0.3 release/3.1.3 release/2.11.4 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages labels Dec 7, 2023
Copy link
Member

@shibd shibd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. left some small comments.

@jiangpengcheng
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@codecov-commenter
Copy link

Codecov Report

Merging #21675 (70fd8d2) into master (fe2d6d3) will increase coverage by 36.72%.
Report is 11 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #21675       +/-   ##
=============================================
+ Coverage     36.69%   73.42%   +36.72%     
- Complexity    12236    32775    +20539     
=============================================
  Files          1716     1893      +177     
  Lines        131130   140741     +9611     
  Branches      14323    15504     +1181     
=============================================
+ Hits          48116   103335    +55219     
+ Misses        76625    29308    -47317     
- Partials       6389     8098     +1709     
Flag Coverage Δ
inttests 24.13% <0.00%> (ø)
systests 24.71% <0.00%> (-0.06%) ⬇️
unittests 72.70% <100.00%> (+40.90%) ⬆️

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

Files Coverage Δ
...ava/org/apache/pulsar/io/common/IOConfigUtils.java 83.33% <100.00%> (ø)
.../org/apache/pulsar/io/dynamodb/DynamoDBSource.java 13.63% <100.00%> (ø)
...pache/pulsar/io/dynamodb/DynamoDBSourceConfig.java 48.21% <100.00%> (ø)
.../pulsar/io/influxdb/InfluxDBGenericRecordSink.java 80.00% <100.00%> (ø)
...he/pulsar/io/influxdb/v1/InfluxDBAbstractSink.java 83.33% <100.00%> (ø)
...ache/pulsar/io/influxdb/v1/InfluxDBSinkConfig.java 92.00% <100.00%> (ø)
...org/apache/pulsar/io/influxdb/v2/InfluxDBSink.java 72.46% <100.00%> (ø)
...ache/pulsar/io/influxdb/v2/InfluxDBSinkConfig.java 92.00% <100.00%> (ø)
...va/org/apache/pulsar/io/jdbc/JdbcAbstractSink.java 75.26% <100.00%> (+75.26%) ⬆️
...java/org/apache/pulsar/io/jdbc/JdbcSinkConfig.java 85.29% <100.00%> (+85.29%) ⬆️
... and 18 more

... and 1434 files with indirect coverage changes

@Technoboy- Technoboy- added this to the 3.2.0 milestone Dec 11, 2023
@Technoboy- Technoboy- merged commit 495b141 into apache:master Dec 11, 2023
47 of 48 checks passed
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jan 4, 2024
@lhotari
Copy link
Member

lhotari commented Jan 19, 2024

I think that this is a very useful improvement, but does this introduce a breaking change for existing users of the connectors?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-3.0 cherry-picked/branch-3.1 doc-not-needed Your PR changes do not impact docs ready-to-test release/2.11.5 release/3.0.3 release/3.1.3 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feat] Support load sensitive fields from secret for connectors
7 participants