Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

Implement keywhiz.cli clone #1216

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Implement keywhiz.cli clone #1216

wants to merge 1 commit into from

Conversation

graysonchao
Copy link
Contributor

@graysonchao graysonchao commented Apr 21, 2023

Adds a new command, keywhiz.cli clone, that allows cloning a secret to a new name. This is intended to help resolve an issue where a user needs to recover an old version of their secret without rolling back the existing secret.

Example usage:

$ keywhiz.cli clone --name my-secret-name --new-name new-secret-name

@graysonchao graysonchao requested a review from a team as a code owner April 21, 2023 19:55
Comment on lines +34 to +37
public CloneSecretRequestV2 build() {
// throws IllegalArgumentException if content not valid base64.
return autoBuild();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment still applicable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, good call

Comment on lines +40 to +51
/**
* Static factory method used by Jackson for deserialization
*/
@SuppressWarnings("unused")
@JsonCreator public static CloneSecretRequestV2 fromParts(
@JsonProperty("name") String name,
@JsonProperty("newName") String newName) {
return builder()
.name(name)
.newName(newName)
.build();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess while we're on the subject of comments, since @JsonCreator literally means "static factory method for Jackson", we could probably ditch the comment.

Comment on lines +725 to +736
try {
Secret newSecret = secretController.builder(existingSecret.getName(),
existingSecret.getSecret(),
existingSecret.getCreatedBy(),
existingSecret.getExpiry())
.withDescription(existingSecret.getDescription())
.withMetadata(existingSecret.getMetadata())
.withOwnerName(existingSecret.getOwner())
.withType(existingSecret.getType().orElse(""))
.create();
newId = newSecret.getId();
} catch (DataAccessException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about the secret contents? I'm worried that we'll have a new top-level secret with no actual contents/content history pointing to it. Or is this handled magically by the SecretController logic? Or do we not actually want the whole history cloned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually you're right, the content history is lost. I had another implementation that used the SecretDAO directly and I think we need to do that + add a DAO method to clone the actual history.

@graysonchao graysonchao marked this pull request as draft April 21, 2023 22:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants