feat(core): Add experimental account storage API #5594

jvz · 2021-12-08T23:12:19Z

This adds an experimental API for storing and loading account
credentials definitions from an external durable store such as a SQL
database. Secrets can be referenced through the existing Kork
SecretEngine API which will be fetched on demand. Initial support is for
Kubernetes accounts given the lack of existing Kubernetes cluster
federation standards compared to other cloud providers, though this API
is made generic to allow for other cloud provider APIs to participate in
this system.

spinnaker/spinnaker#6525

clouddriver-web/src/main/groovy/com/netflix/spinnaker/clouddriver/Main.groovy

...pi/src/main/java/com/netflix/spinnaker/clouddriver/security/AccountDefinitionRepository.java

dbyron-sf · 2021-12-14T18:13:12Z

Hey @jvz I'm probably already supposed to know this, but does this PR enable dynamic refresh of the accounts, or are they still loaded only at startup?

jvz · 2021-12-14T18:15:30Z

Refreshing is done via credentials polling currently. If you enable that, then this gets refreshed along with any other credentials definition sources you have.

...r-core/src/main/java/com/netflix/spinnaker/clouddriver/security/AccountDefinitionSource.java

jvz · 2021-12-16T23:32:47Z

Alright, I've updated based on most of the feedback so far. I need to dig a little deeper into the SQL implementation to update the deletion behavior documented in spinnaker/gate#1494

...main/kotlin/com/netflix/spinnaker/clouddriver/sql/security/SqlAccountDefinitionRepository.kt

clouddriver-sql/src/main/resources/db/changelog/20210927-accounts.yml

jvz · 2021-12-20T21:27:59Z

I've updated the deletion behavior and made the revision history a little better. I've also removed some unused endpoints in the API.

dbyron-sf · 2021-12-20T21:31:25Z

Thanks @jvz I'm looking forward to taking another look, but at this point I don't think it'll be til the new year.

jvz · 2021-12-20T21:33:37Z

That's what I expected; just wanted to make sure I finished the PR updates before the same new year before I forgot about them!

jvz · 2021-12-20T21:36:44Z

Oh, also helps to push this to my fork and not just a private repo. Pushed!

dbyron-sf · 2022-01-04T18:36:21Z

@jvz I'm still struggling to catch up from the flood of messages after being off for awhile...this is still very much on my radar though...

jvz · 2022-01-04T20:25:03Z

No worries! I just got back from holidays, so I made sure to leave pings where applicable.

german-muzquiz

Is it correct to assume that for this to work, clouddriver.yml needs to enable the poller?

credentials:
  poller:
    enabled: true
    types:
      kubernetes:
        reloadFrequencyMs: 60000

...e/src/main/java/com/netflix/spinnaker/clouddriver/config/AccountDefinitionConfiguration.java

clouddriver-sql/clouddriver-sql.gradle

german-muzquiz · 2022-01-06T16:53:54Z

...pi/src/main/java/com/netflix/spinnaker/clouddriver/security/AccountDefinitionRepository.java

+   * @param name account name to look up history for
+   * @return history of account updates for the given account name
+   */
+  List<Revision> revisionHistory(String name);


This is great

...river-web/src/main/java/com/netflix/spinnaker/clouddriver/controllers/AccountController.java

This adds an experimental API for storing and loading account credentials definitions from an external durable store such as a SQL database. Secrets can be referenced through the existing Kork SecretEngine API which will be fetched on demand. Initial support is for Kubernetes accounts given the lack of existing Kubernetes cluster federation standards compared to other cloud providers, though this API is made generic to allow for other cloud provider APIs to participate in this system.

jvz · 2022-01-07T00:37:30Z

...e/src/main/java/com/netflix/spinnaker/clouddriver/config/AccountDefinitionConfiguration.java

+        var string = super.deserialize(p, ctxt);
+        if (EncryptedSecret.isEncryptedSecret(string)) {
+          return EncryptedSecret.isEncryptedFile(string)
+              ? secretManager.decryptAsFile(string).toString()


Should this add a file: prefix, too?

Apparently that's only used for SAML related files that may be used in gate or fiat, so I'd say this is fine without the file: prefix.

jvz · 2022-01-07T00:39:46Z

...river-web/src/main/java/com/netflix/spinnaker/clouddriver/controllers/AccountController.java

+  }
+
+  @GetMapping("/{accountName}/history")
+  @PreAuthorize("hasPermission(#accountName, 'ACCOUNT', 'READ')")


This should probably be changed to WRITE permissions before being exposed in Gate.

german-muzquiz · 2022-01-07T15:21:51Z

...e/src/main/java/com/netflix/spinnaker/clouddriver/config/AccountDefinitionConfiguration.java

+        var string = super.deserialize(p, ctxt);
+        if (EncryptedSecret.isEncryptedSecret(string)) {
+          return EncryptedSecret.isEncryptedFile(string)
+              ? secretManager.decryptAsFile(string).toString()


Apparently that's only used for SAML related files that may be used in gate or fiat, so I'd say this is fine without the file: prefix.

...r-web/src/main/java/com/netflix/spinnaker/clouddriver/controllers/CredentialsController.java

* feat(core): Add experimental account storage API This adds an experimental API for storing and loading account credentials definitions from an external durable store such as a SQL database. Secrets can be referenced through the existing Kork SecretEngine API which will be fetched on demand. Initial support is for Kubernetes accounts given the lack of existing Kubernetes cluster federation standards compared to other cloud providers, though this API is made generic to allow for other cloud provider APIs to participate in this system. * Combine AccountController with CredentialsController * Fix account permissions and add comments * Add alpha annotations * Use correct annotations for rest APIs

jvz requested review from cfieber and german-muzquiz as code owners December 8, 2021 23:12

jvz mentioned this pull request Dec 8, 2021

feat(web): Expose experimental account storage API spinnaker/gate#1494

Merged

dbyron-sf reviewed Dec 14, 2021

View reviewed changes

clouddriver-web/src/main/groovy/com/netflix/spinnaker/clouddriver/Main.groovy Outdated Show resolved Hide resolved

dbyron-sf reviewed Dec 14, 2021

View reviewed changes

...pi/src/main/java/com/netflix/spinnaker/clouddriver/security/AccountDefinitionRepository.java Show resolved Hide resolved

german-muzquiz reviewed Dec 14, 2021

View reviewed changes

...r-core/src/main/java/com/netflix/spinnaker/clouddriver/security/AccountDefinitionSource.java Show resolved Hide resolved

jvz force-pushed the account-api branch from 1ea8a93 to d621f29 Compare December 16, 2021 23:31

jvz commented Dec 16, 2021

View reviewed changes

...main/kotlin/com/netflix/spinnaker/clouddriver/sql/security/SqlAccountDefinitionRepository.kt Outdated Show resolved Hide resolved

jvz commented Dec 16, 2021

View reviewed changes

clouddriver-sql/src/main/resources/db/changelog/20210927-accounts.yml Outdated Show resolved Hide resolved

jvz commented Dec 17, 2021

View reviewed changes

clouddriver-sql/src/main/resources/db/changelog/20210927-accounts.yml Outdated Show resolved Hide resolved

jvz force-pushed the account-api branch from d621f29 to 9e588cb Compare December 20, 2021 21:36

jvz requested review from german-muzquiz and dbyron-sf January 4, 2022 18:22

german-muzquiz reviewed Jan 6, 2022

View reviewed changes

jvz force-pushed the account-api branch from 9e588cb to 9bd86bd Compare January 7, 2022 00:36

jvz commented Jan 7, 2022

View reviewed changes

jvz requested a review from german-muzquiz January 7, 2022 00:42

german-muzquiz approved these changes Jan 7, 2022

View reviewed changes

Combine AccountController with CredentialsController

e04f34e

dbyron-sf reviewed Jan 7, 2022

View reviewed changes

...r-web/src/main/java/com/netflix/spinnaker/clouddriver/controllers/CredentialsController.java Show resolved Hide resolved

jvz added 2 commits January 7, 2022 16:27

Fix account permissions and add comments

15bb7ae

Add alpha annotations

7d1c7ee

dbyron-sf approved these changes Jan 7, 2022

View reviewed changes

dbyron-sf added the ready to merge Approved and ready for a merge label Jan 7, 2022

Use correct annotations for rest APIs

d0455ac

mergify bot merged commit 264af19 into spinnaker:master Jan 7, 2022

mergify bot added the auto merged Merged automatically by a bot label Jan 7, 2022

jvz deleted the account-api branch January 7, 2022 23:24

spinnakerbot added the target-release/1.28 label Jan 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): Add experimental account storage API #5594

feat(core): Add experimental account storage API #5594

jvz commented Dec 8, 2021

dbyron-sf commented Dec 14, 2021

jvz commented Dec 14, 2021

jvz commented Dec 16, 2021

jvz commented Dec 20, 2021

dbyron-sf commented Dec 20, 2021

jvz commented Dec 20, 2021

jvz commented Dec 20, 2021

dbyron-sf commented Jan 4, 2022

jvz commented Jan 4, 2022

german-muzquiz left a comment

german-muzquiz Jan 6, 2022

jvz Jan 7, 2022

german-muzquiz Jan 7, 2022

jvz Jan 7, 2022

german-muzquiz Jan 7, 2022

feat(core): Add experimental account storage API #5594

feat(core): Add experimental account storage API #5594

Conversation

jvz commented Dec 8, 2021

dbyron-sf commented Dec 14, 2021

jvz commented Dec 14, 2021

jvz commented Dec 16, 2021

jvz commented Dec 20, 2021

dbyron-sf commented Dec 20, 2021

jvz commented Dec 20, 2021

jvz commented Dec 20, 2021

dbyron-sf commented Jan 4, 2022

jvz commented Jan 4, 2022

german-muzquiz left a comment

Choose a reason for hiding this comment

german-muzquiz Jan 6, 2022

Choose a reason for hiding this comment

jvz Jan 7, 2022

Choose a reason for hiding this comment

german-muzquiz Jan 7, 2022

Choose a reason for hiding this comment

jvz Jan 7, 2022

Choose a reason for hiding this comment

german-muzquiz Jan 7, 2022

Choose a reason for hiding this comment