Skip to content

[feat][io] Support configuration secret interpolation#20901

Merged
michaeljmarshall merged 7 commits into
apache:masterfrom
michaeljmarshall:interpolate-io-secrets
Aug 16, 2023
Merged

[feat][io] Support configuration secret interpolation#20901
michaeljmarshall merged 7 commits into
apache:masterfrom
michaeljmarshall:interpolate-io-secrets

Conversation

@michaeljmarshall

@michaeljmarshall michaeljmarshall commented Jul 28, 2023

Copy link
Copy Markdown
Member

PIP: #20903
Relates to: #20862

Motivation

The primary motivation is to make it possible to configure Pulsar Connectors in a secure, non-plaintext way. See the PIP for background and relevant details. The new interpolation feature only applies when deploying with functions to Kubernetes.

Modifications

  • Add SecretsProvider#interpolateSecretForValue method with a default that maintains the current behavior.
  • Override interpolateSecretForValue in the EnvironmentBasedSecretsProvider so that configuration values formatted as ${my-env-var} will be replaced with the result of System.getEnv("my-env-var") if the result is not null.
  • Implement a recursive string interpolation method that will replace any configuration value that the interpolateSecretForValue implementation determines ought to be replaced.

Verifying this change

Tests are added/modified.

Documentation

  • doc-required

Matching PR in forked repository

PR in forked repository: michaeljmarshall#55

@michaeljmarshall michaeljmarshall added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. area/connector area/security type/PIP labels Jul 28, 2023
@michaeljmarshall michaeljmarshall added this to the 3.2.0 milestone Jul 28, 2023
@michaeljmarshall michaeljmarshall self-assigned this Jul 28, 2023
@github-actions github-actions Bot added doc-required Your PR changes impact docs and you will update later. and removed type/PIP labels Jul 28, 2023
@michaeljmarshall michaeljmarshall removed the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Jul 28, 2023
@michaeljmarshall michaeljmarshall marked this pull request as ready for review August 14, 2023 23:12

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

@michaeljmarshall

Copy link
Copy Markdown
Member Author

/pulsarbot rerun-failure-checks

@shibd shibd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/LGTM

}

@Override
public String interpolateSecretForValue(String value) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's better to add unit tests to the method separately.

@codecov-commenter

codecov-commenter commented Aug 16, 2023

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.88889% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.10%. Comparing base (d6734b7) to head (5e05025).
⚠️ Report is 1845 commits behind head on master.

Files with missing lines Patch % Lines
...ulsar/functions/instance/JavaInstanceRunnable.java 85.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #20901       +/-   ##
=============================================
+ Coverage     36.84%   73.10%   +36.25%     
- Complexity    12195    32255    +20060     
=============================================
  Files          1698     1875      +177     
  Lines        129852   139455     +9603     
  Branches      14161    15334     +1173     
=============================================
+ Hits          47843   101946    +54103     
+ Misses        75680    29448    -46232     
- Partials       6329     8061     +1732     
Flag Coverage Δ
inttests 24.23% <0.00%> (+0.10%) ⬆️
systests 25.13% <14.81%> (+0.03%) ⬆️
unittests 72.40% <88.88%> (+40.34%) ⬆️

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

Files with missing lines Coverage Δ
...cretsprovider/EnvironmentBasedSecretsProvider.java 100.00% <100.00%> (+100.00%) ⬆️
...sar/functions/secretsprovider/SecretsProvider.java 100.00% <100.00%> (ø)
...ulsar/functions/instance/JavaInstanceRunnable.java 71.85% <85.00%> (+9.98%) ⬆️

... and 1435 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@michaeljmarshall michaeljmarshall merged commit bfde0de into apache:master Aug 16, 2023
@michaeljmarshall michaeljmarshall deleted the interpolate-io-secrets branch August 16, 2023 14:20
michaeljmarshall added a commit to datastax/pulsar that referenced this pull request Aug 16, 2023
PIP: apache#20903
Relates to: apache#20862

The primary motivation is to make it possible to configure Pulsar Connectors in a secure, non-plaintext way. See the PIP for background and relevant details. The new interpolation feature only applies when deploying with functions to Kubernetes.

* Add `SecretsProvider#interpolateSecretForValue` method with a default that maintains the current behavior.
* Override `interpolateSecretForValue` in the `EnvironmentBasedSecretsProvider` so that configuration values formatted as `${my-env-var}` will be replaced with the result of `System.getEnv("my-env-var")` if the result is not `null`.
* Implement a recursive string interpolation method that will replace any configuration value that the `interpolateSecretForValue` implementation determines ought to be replaced.

Tests are added/modified.

- [x] `doc-required`

PR in forked repository: michaeljmarshall#55

(cherry picked from commit bfde0de)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/connector area/security doc-required Your PR changes impact docs and you will update later.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants