-
Notifications
You must be signed in to change notification settings - Fork 174
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(kork/secrets): Adding SecretManager #219
Conversation
f1aa86d
to
7e474a7
Compare
SecretEngine secretEngine = secretEngineRegistry.getEngine(encryptedSecret.getEngineIdentifier()); | ||
if (secretEngine == null) { | ||
throw new InvalidSecretFormatException("Secret Engine does not exist: " + encryptedSecret.getEngineIdentifier()); | ||
|
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.
Extra newline
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.
LGTM, a few comments
SecretEngine secretEngine = secretEngineRegistry.getEngine(encryptedSecret.getEngineIdentifier()); | ||
if (secretEngine == null) { | ||
throw new InvalidSecretFormatException("Secret Engine does not exist: " + encryptedSecret.getEngineIdentifier()); | ||
|
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.
Extra newline
* @param filePathOrEncrypted | ||
* @return path to temporary file that contains decrypted contents | ||
*/ | ||
public String decryptFile(String filePathOrEncrypted) { |
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.
If you're returning a path to a file, I would recommend returning Path
instead
tempFile.deleteOnExit(); | ||
return tempFile.getAbsolutePath(); | ||
} catch (IOException e) { | ||
throw new SecretDecryptionException(); |
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.
This should have a reference to e
at the least
*/ | ||
public String decrypt(String configValue) { | ||
EncryptedSecret encryptedSecret = EncryptedSecret.parse(configValue); | ||
if (encryptedSecret == null) { return configValue; } |
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.
This should live on a newline
public class SecretManager { | ||
|
||
@Autowired | ||
private SecretEngineRegistry secretEngineRegistry; |
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 pattern we've been following (to make this easier for testing & such) is to @Autowire
the constructor. This removes the need for the package-private setter below, and allows you to treat this as final
.
final private SecretEngineRegistry secretEngineRegistry;
@Autowired
SecretManager(SecretEngineRegistry secretEngineRegistry) {
this.secretEngineRegistry = secretEngineRegistry;
}
kork-secrets/kork-secrets.gradle
Outdated
|
||
testCompile (group: 'org.springframework', name: 'spring-test', version: '4.3.20.RELEASE') | ||
testCompile group: 'org.springframework.boot', name: 'spring-boot-test', version: '1.5.14.RELEASE' | ||
testCompile group: 'org.mockito', name: 'mockito-core', version: '2.23.4' |
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.
@link108 small nit, could you make these more consistent? either remove the (
or add it to the others.
Part of a series of PRs for the Secrets Management Project. This PR
adds SecretManager and SecretEngineRegistry.
spinnaker/spinnaker#3649