add operations for interacting with kv metadata #561
Conversation
|
@zak905 Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@zak905 Thank you for signing the Contributor License Agreement! |
| @@ -96,7 +96,7 @@ public void initialize() { | |||
|
|
|||
| if (this.prepareVault.getVersion() | |||
| .isGreaterThanOrEqualTo(VERSIONING_INTRODUCED_WITH)) { | |||
| this.prepareVault.disableGenericVersioning(); | |||
| //this.prepareVault.disableGenericVersioning(); | |||
There was a problem hiding this comment.
I know this is not the right fix, but I was just wondering why using /versioned path and unmounting the already provided /secret. In real world settings, /secret is the default right?
There was a problem hiding this comment.
This is true if you look at it from a most recent version perspective. We're running our test against various Vault versions that do not contain the KV version 2 backend so we're creating a stable setup. Also, newer Vault versions were used to mount the versioned backend at /secret so we need to revert this change.
spring-vault-core/pom.xml
Outdated
| @@ -75,6 +75,11 @@ | |||
| <artifactId>jackson-databind</artifactId> | |||
| </dependency> | |||
|
|
|||
| <dependency> | |||
| <groupId>com.fasterxml.jackson.datatype</groupId> | |||
There was a problem hiding this comment.
We should rather not pull in the JSR-310 support here as this dependency change has a few downstream consequences (such as customizing the ObjectMapper).
| } | ||
|
|
||
|
|
||
| public VaultKeyValueMetadataTemplate(VaultOperations vaultOperations) { |
There was a problem hiding this comment.
We need to pass on the path for the kv backend, similar to VaultVersionedKeyValueTemplate.
| private static final ObjectMapper OBJECT_MAPPER; | ||
|
|
||
| static { | ||
| ObjectMapper mapper = new ObjectMapper(); |
There was a problem hiding this comment.
Given the need for another dependency, we should rather parse JSR-310 dates ourselves (see VaultVersionedKeyValueTemplate.getDate(…))
| @Override | ||
| public void delete(String path) { | ||
| Assert.hasText(path, "Path must not be empty"); | ||
| vaultOperations.doWithSession(restOperations -> { |
There was a problem hiding this comment.
vaultOperations.delete(…) should be sufficient.
| Assert.notNull(body, "Body must not be null"); | ||
| vaultOperations.doWithSession(restOperations -> { | ||
| try { | ||
| restOperations.put("/secret/metadata/" + path, body); |
There was a problem hiding this comment.
vaultOperations.write(…) should be sufficient.
There was a problem hiding this comment.
it seems like write does a POST while here we need a PUT
| * @author Zakaria Amine | ||
| */ | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public class VaultMetadataResponse { |
There was a problem hiding this comment.
As mentioned earlier, let's perform deserialization of this type ourselves. That way, we get the most control over the object and we can reuse Versioned.Metadata for versions instead of exposing a Jackson-specific type.
We should make this type also immutable because the response object does not change once constructed.
| import com.fasterxml.jackson.annotation.JsonIgnoreProperties; | ||
|
|
||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public class VaultMetadataResponseWrapper { |
There was a problem hiding this comment.
With custom deserialization, there's no need for this type. An inner class (VaultMetadataResponseWrapper extends VaultResponseSupport<Map<String, Object>>) in VaultKeyValueMetadataTemplate should be sufficient.
| private VaultKeyValueMetadataOperations vaultKeyValueMetadataOperations; | ||
|
|
||
| VaultKeyValueMetadataTemplateIntegrationTests() { | ||
| super("secret", VaultKeyValueOperationsSupport.KeyValueBackend.versioned()); |
There was a problem hiding this comment.
This path should be versioned for our arrangement.
spring-vault-dependencies/pom.xml
Outdated
| @@ -86,6 +86,12 @@ | |||
| <version>${jackson-databind.version}</version> | |||
| </dependency> | |||
|
|
|||
| <dependency> | |||
There was a problem hiding this comment.
Should be removed as per comment in the other pom.xml.
|
|
||
| @ExtendWith(SpringExtension.class) | ||
| @ContextConfiguration(classes = VaultIntegrationTestConfiguration.class) | ||
| public class VaultKeyValueMetadataTemplateIntegrationTests extends AbstractVaultKeyValueTemplateIntegrationTests { |
There was a problem hiding this comment.
Nit: This test should be guarded so it does not run on Vault versions prior to 0.10.0 (@RequiresVaultVersion(VaultInitializer.VERSIONING_INTRODUCED_WITH_VALUE))
There was a problem hiding this comment.
The annotation is already on the parent class, should I put it here as well?
|
Hey, thanks for the code review. I did a quick skim through the comments. I am gonna address them soon. |
|
@mp911de I addressed pretty much all the comments, happy to hear your feedback. |
|
Thanks a lot. I'm going to take the pull request from here. |
|
Thank you for your contribution. That's merged and polished now. |
Reformat sources from space indents to tabs. Introduce DurationParser to represent java.time.Duration using Go's Duration format. Reduce visibility of implementation to package level. Simplify code. Update tests to work with rerunning tests. Reorder methods. Add license headers. Original pull request: gh-561. Resolves gh-432.
|
Welcome! Thanks. |
as promised in #432, here is the functionality for interacting with a kv metadata. Looking forward to hear your feedback.