-
Notifications
You must be signed in to change notification settings - Fork 245
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(vault): implements custom secret folder config #4385
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #4385 +/- ##
==========================================
+ Coverage 71.74% 74.96% +3.22%
==========================================
Files 919 1071 +152
Lines 18457 21478 +3021
Branches 1037 1175 +138
==========================================
+ Hits 13242 16102 +2860
- Misses 4756 4852 +96
- Partials 459 524 +65 ☔ View full report in Codecov by Sentry. |
This pull request is stale because it has been open for 7 days with no activity. |
@@ -71,6 +71,9 @@ public class HashicorpVaultExtension implements ServiceExtension { | |||
@Setting(value = "The URL path of the vault's /secret endpoint", defaultValue = VAULT_API_SECRET_PATH_DEFAULT) | |||
public static final String VAULT_API_SECRET_PATH = "edc.vault.hashicorp.api.secret.path"; | |||
|
|||
@Setting(value = "The path of the folder that the secret is stored in", required = false, defaultValue = "") |
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.
FWIW, the original is correct and more concise
@@ -71,6 +71,9 @@ public class HashicorpVaultExtension implements ServiceExtension { | |||
@Setting(value = "The URL path of the vault's /secret endpoint", defaultValue = VAULT_API_SECRET_PATH_DEFAULT) | |||
public static final String VAULT_API_SECRET_PATH = "edc.vault.hashicorp.api.secret.path"; | |||
|
|||
@Setting(value = "The path of the folder that the secret is stored in", required = false, defaultValue = "") | |||
public static final String VAULT_FOLDER_PATH = "edc.vault.hashicorp.folder"; |
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.
Should we maybe name it subfolder?
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 question is whether the path is relative or absolute, not if it is a subfolder
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.
I guess it depends on how you define relative/absolute in this case, right? 😄
For example the "full" URL would be: https://the-vault.com/v1/secrets/data/some/deep/deep/deep/folder/my-secret
This url splits up in the following parts:
https://the-vault.com/v1/secrets
(already covered byVAULT_URL
andVAULT_API_SECRET_PATH
)data
(ormetadata
)some/deep/deep/deep/folder
(will be covered byVAULT_FOLDER_PATH
my-secret
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.
An absolute path always starts with a scheme, so I don't think there is any ambiguity here. From the example, the path is always relative so it should explicitly state that, "The path of the folder relative to VAULT_FOLDER_PATH..."
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.
@SaschaIsele can you please align the changes
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.
changed the description to reflect the relative nature of the path
@@ -71,6 +71,9 @@ public class HashicorpVaultExtension implements ServiceExtension { | |||
@Setting(value = "The URL path of the vault's /secret endpoint", defaultValue = VAULT_API_SECRET_PATH_DEFAULT) | |||
public static final String VAULT_API_SECRET_PATH = "edc.vault.hashicorp.api.secret.path"; | |||
|
|||
@Setting(value = "The path of the folder that the secret is stored in", required = false, defaultValue = "") |
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.
FWIW, the original is correct and more concise
This pull request is stale because it has been open for 7 days with no activity. |
This pull request is stale because it has been open for 7 days with no activity. |
...t/vault-hashicorp/src/main/java/org/eclipse/edc/vault/hashicorp/HashicorpVaultExtension.java
Outdated
Show resolved
Hide resolved
42e8b4f
to
873e08d
Compare
Signed-off-by: Sascha Isele <sascha.isele.external@zf.com>
Signed-off-by: Sascha Isele <s.isele@cluetec.de>
873e08d
to
9b53e68
Compare
What this PR changes/adds
Adds the possibility to configure a folder for the storage of secrets inside Hashicorp Vault.
Why it does that
Additional feature, as described in the discussion and issue.
Linked Issue(s)
Closes #4384
Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.