-
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(secrets/gcs): Added GcsSecretEngine for decrypting secrets in GCS #380
Conversation
@cfieber mind taking a look at this please? |
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 looks reasonable to me, would not mind an eyeball from someone in @spinnaker/google-reviewers as I'm not familiar at all with the gcs APIs
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.
Left a few comments inline, happy to chat more about this. I think the biggest thing would be simplifying the logic around having two credentials objects lying around where every request tries both of them.
kork-secrets-gcp/src/main/java/com/netflix/spinnaker/kork/secrets/engines/GcsSecretEngine.java
Outdated
Show resolved
Hide resolved
kork-secrets-gcp/src/main/java/com/netflix/spinnaker/kork/secrets/engines/GcsSecretEngine.java
Outdated
Show resolved
Hide resolved
|
||
public class GoogleCredentials { | ||
|
||
private static final long CONNECT_TIMEOUT = TimeUnit.MINUTES.toMillis(2); |
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.
Since these timeouts are needed as int
everywhere they are used, I'd suggest just doing the cast directly here:
private static final int CONNECT_TIMEOUT = (int) TimeUnit.MINUTES.toMillis(2);
Also, 2 min seems like a pretty long connection timeout...how did you pick that value?
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 grabbed this code from how Halyard reads BOM's stored in GCS. I'll decrease the timeout to 20 seconds if that sounds better.
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 think 20 seconds sounds more reasonable for a connection timeout, and is more in line with other places in Spinnaker.
kork-secrets-gcp/src/main/java/com/netflix/spinnaker/kork/secrets/engines/GcsSecretEngine.java
Outdated
Show resolved
Hide resolved
kork-secrets-gcp/src/main/java/com/netflix/spinnaker/kork/secrets/engines/GcsSecretEngine.java
Outdated
Show resolved
Hide resolved
kork-secrets-gcp/src/main/java/com/netflix/spinnaker/kork/secrets/engines/GcsSecretEngine.java
Outdated
Show resolved
Hide resolved
kork-secrets-gcp/src/main/java/com/netflix/spinnaker/kork/secrets/engines/GcsSecretEngine.java
Outdated
Show resolved
Hide resolved
...-secrets-gcp/src/main/java/com/netflix/spinnaker/kork/secrets/engines/GoogleCredentials.java
Outdated
Show resolved
Hide resolved
...-secrets-gcp/src/main/java/com/netflix/spinnaker/kork/secrets/engines/GoogleCredentials.java
Outdated
Show resolved
Hide resolved
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.
Changes look great, thanks! I left one more small comment and replied to the above comment about connection timeout.
import java.security.GeneralSecurityException; | ||
import java.util.concurrent.TimeUnit; | ||
|
||
public class GoogleHttpUtils { |
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 know this isn't something we often do currently in the Spinnaker codebase, but I think this is a good candidate for being package-private, as presumably no other classes outside GcsSecretEngine
would need to know about it.
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.
👍 Looks good, thanks!
* refactor(core): Convert echo-core to Java Echo-core is almost completely java, with the exception of two easy-to-convert files. Finish the conversion and change the top-level source directory from groovy to java. Also, add some explicit gradle dependencies. * feat(core): Add support for requesting build info Add the required infrastructure for echo to request build info from Igor. * feat(core): Add pipeline post-processor functionality Add functionality to post-process pipelines before sending them to orca for execution. In particular, add a post-processor to add build info to the trigger, and a (not-yet-implemented) post-processor to inflate artifacts using a Jina template. * feat(core): Add igor endpoint to halyard base config * style(core): Address code review comments * refactor(core): Don't rely on autowiring for ordering Add functionality for ordering pipeline post-processors that doesn't depend on Spring autowiring the post-processors in the order specified by @order. * style(core): Address PR comments * Clean up EchoRetrofitConfig * Add retry to igor communication
New
GcsSecretEngine
for decrypting secrets of the formencrypted:gcs!b:bucket!f:file[!k:key-in-file]
.Some of the code is copied from how Halyard downloads Spinnaker BOM's from GCS.