feat(config): add vault provider #798

FrankYang0529 · 2022-10-01T04:51:13Z

resolve #661

FrankYang0529 · 2022-10-01T04:55:18Z

Hello maintainers, I would like to add a new vault config provider, but I'm not sure where is the best place to put it to the Engine. Do you have any suggestion? Thank you!

itowlson · 2022-10-02T21:07:29Z

@FrankYang0529 Thanks for this - great to see a secrets-friendly provider!

Re your question, config providers are currently wired up in the TriggerExecutorBuilder:

spin/crates/trigger/src/lib.rs

Line 104 in 13f7916

spin_config::ConfigHostComponent::new(self.default_config_providers(&app_uri)),

I believe you'd need to provide a way for an operator to add non-default config providers and specify their settings, e.g. a command line option.

It might be worth briefly writing up a proposal, either here or on the original issue, before diving into the code for that. The reason I suggest that is that we're likely to have other non-default config providers in future, and it's worth planning the Vault UI with one eye on scaling it to those as yet unknown providers. (For example, I'd be wary of a Vault-specific command line option a la --vault-config-source.)

lann · 2022-10-03T14:33:23Z

We have discussed in the past having some sort of "environment" or "profile" configuration outside of the application manifest for this sort of thing. This would also be useful for splitting "dev" vs "prod" config a la #389.

FrankYang0529 · 2022-10-04T14:29:49Z

Hi @itowlson and @lann, thanks for your suggestion! I will do some surveys and give a proposal first. 👍

FrankYang0529 · 2022-10-07T10:19:12Z

For issue #389 and #661, my proposal is as following:

Add a --config argument to TriggerExecutorCommand, so users can use it to load different settings in different environments like dev / prod.
The config file format will be like

[spin.config.provider.vault]
vault_addr = "http://127.0.0.1:8200"
vault_token = "root"

[wasm.config]
wasm_backtrace_details = true

Update update_wasmtime_config function for other configurations in wasmtime.
Update build function to handle other spin_config providers.

FrankYang0529 · 2022-10-18T12:36:12Z

Hi @itowlson, I updated the PR and added --config-file to load vault configuration in TriggerExecutorCommand. May you help me take a look? If it looks good, I will add a new e2e test case tomorrow. Thank you.

For wasmtime config like wasm_backtrace_details in #389, I will add in a follow-up PR.

Test step

Start vault

$ vault server -dev -dev-root-token-id root

Set a password

$ export VAULT_TOKEN=root
$ export VAULT_ADDR=http://127.0.0.1:8200
$ vault kv put secret/password value="test_password"
$ vault kv get secret/password

Start config-test app

$ cargo build
$ cd tests/http/config-test
$ ../../../target/debug/spin build
$ ../../../target/debug/spin up --config-file builder_config.toml

Test the app

$ curl -i http://127.0.0.1:3000
HTTP/1.1 200 OK
content-length: 26
date: Tue, 18 Oct 2022 12:34:40 GMT

Got password test_password

lann · 2022-10-18T15:03:20Z

crates/config/src/provider/vault.rs

+    pub fn new(url: impl AsRef<str>, token: impl AsRef<str>) -> Self {
+        Self {
+            url: url.as_ref().to_string(),
+            token: token.as_ref().to_string(),
+        }


Nit:

Suggested change

pub fn new(url: impl AsRef<str>, token: impl AsRef<str>) -> Self {

Self {

url: url.as_ref().to_string(),

token: token.as_ref().to_string(),

}

pub fn new(url: impl Into<String>, token: impl Into<String>) -> Self {

Self {

url: url.into(),

token: token.into(),

}

Updated it. Thank you.

lann · 2022-10-18T15:08:07Z

crates/config/src/provider/vault.rs

+#[derive(Debug, Deserialize, Serialize)]
+struct Secret {
+    value: String,
+}


It might make sense to either not derive Debug here or - if Debug is required by something else - write a custom impl that doesn't actually write out value. It is very easy to accidentally log secrets this way.

There is also https://docs.rs/secrecy/latest/secrecy/ for this

I'm not quite sure how to use secrecy package, so I just removed Debug here. Thanks!

lann · 2022-10-18T15:19:15Z

crates/config/src/provider/vault.rs

+        let keys = key.0.split('_').collect::<Vec<_>>();
+        if keys.len() == 1 {
+            return Err(anyhow!("vault key must contain the mount path"));
+        }
+        let mount = keys[0];
+        let path = keys[1..].join("/");
+        let secret: Secret = kv2::read(&client, mount, &path).await?;


This seems like an awkward way to handle paths, though admittedly the constraints on config keys make this tricky.

Would it make sense for the provider config to include "mount_path" and "prefix" settings to join with here? i.e. something like (untested):

// struct VaultProvider { mount: String, prefix: Option<String>,

Suggested change

let keys = key.0.split('_').collect::<Vec<_>>();

if keys.len() == 1 {

return Err(anyhow!("vault key must contain the mount path"));

}

let mount = keys[0];

let path = keys[1..].join("/");

let secret: Secret = kv2::read(&client, mount, &path).await?;

let path = match &self.prefix {

Some(prefix) => &format!("{prefix}/{key}"),

None => key.0,

};

let secret: Secret = kv2::read(&client, &self.mount, path).await?;

From vault document, we can enable many secret engines on different paths (mount). I am not sure whether users want to get secrets from different paths in an application. If yes, we probably need to leave flexibility here.

I am not sure whether users want to get secrets from different paths in an application.

That should be possible with the changes below by having multiple Vault providers. I think treating _ specially here is very confusing.

Make sense! Thank you. Updated it.

lann · 2022-10-18T15:25:40Z

crates/trigger/src/config.rs

+// Config for config providers and wasmtime config
+#[derive(Debug, Default, Deserialize)]
+pub struct TriggerExecutorBuilderConfig {
+    pub vault_config: Option<VaultConfig>,


I think it would be good for the file structure here to be slightly more generic, allowing for (future) provider plugins and multiple providers of the same type. Perhaps something like:

[[config_provider]] type = "vault" # url = ...

note that order would matter here; Providers are checked in order

This is pretty straightforward with serde: https://serde.rs/enum-representations.html#internally-tagged

I think it would be something like (untested):

#[derive(Debug, Default, Deserialize)] pub struct TriggerExecutorBuilderConfig { #[serde(rename = "config_provider", default)] pub config_providers: Vec<ConfigProvider>, } #[derive(Debug, Default, Deserialize)] #[serde(rename_all = "snake_case", tag = "type")] pub enum ConfigProvider { Vault(VaultConfig), // Could add later e.g.: Env(EnvConfig), }

Thanks for the suggestion! It looks much cleaner now!

lann · 2022-10-19T16:12:50Z

Looking pretty good to me. I'd like some others to review as well since --config-file is likely to be reused for some other things. We're pretty busy for the next couple of weeks with KubeCon so apologies if this takes a bit longer yet.

FrankYang0529 · 2022-10-19T16:17:41Z

Hi @lann, thanks for taking the time to review this. I also add e2e test for the vault config provider. However, there may have some issues to start vault server on Windows. Anyway, I will test it on my repo FrankYang0529#6. Looks forward to your new sharing on KubeCon. 👍🏻

FrankYang0529 · 2022-10-27T14:09:22Z

Hmm, not sure why we have following error in CI. I can run make test and make test-e2e on my laptop.

LINK : fatal error LNK1201: error writing to program database 'D:\a\spin\spin\target\debug\deps\spin.pdb'; check for insufficient disk space, invalid path, or insufficient privilege

FrankYang0529 · 2022-10-27T15:10:36Z

I changed to use stable-gnu for windows and the error message changed to No space left on device (ref).

@lann, is it possible to add larger runners? Default runners only have 14G SSD.

lann · 2022-10-27T15:19:41Z

@vdice if you get the chance could you check this out?

FrankYang0529 · 2022-10-29T06:12:09Z

I moved config test from e2e-tests feature to config-provider-tests, so we don't need to install Vault in CI. Probably, let's see how to create a larger runner in the future. Thanks.

vdice · 2022-10-31T17:48:59Z

@lann @FrankYang0529 would it help to split out some/all of the tests into separate workflow(s) from the main build, so they run on a separate runner? Or would we continue to run into space issues with the default drive size?

Either way, we'd want to track upgrading to larger runners in a separate issue, as it brings billing/cost implications:

Note: The larger runners do not use included entitlement minutes, and are not free for public repositories.

FrankYang0529 · 2022-11-01T12:29:49Z

@lann @FrankYang0529 would it help to split out some/all of the tests into separate workflow(s) from the main build, so they run on a separate runner? Or would we continue to run into space issues with the default drive size?

Yeah, I think we still have space issues, because we still need to install dependencies like bindle, hippo, nomad, etc.

Either way, we'd want to track upgrading to larger runners in a separate issue, as it brings billing/cost implications:

If we want to use larger runners, it's good to separate all tests, because it is calculated by minutes. We can try to minimize the cost.

fibonacci1729 · 2022-11-01T22:04:34Z

This all looks great! My only suggestion is to replace references to "config-" as "runtime-config-" (i.e flags and toml sections) to more precisely reflect the "usage semantics" of these configuration values (i.e. runtime). However, if people like it as is, consider this comment an ignoreable nit.

vdice · 2022-11-01T22:17:16Z

Do we want to add an example app that demonstrates use of the vault config provider as well -- for better visibility/awareness of the functionality?

Would be great to get some docs around this feature as well, perhaps with the aforementioned example; something along the lines of #798 (comment) (or whatever the latest flow might be). However, since we're now housing Spin documentation in fermyon/developer, this will have to be a follow-up.

Signed-off-by: Frank Yang <yangpoan@gmail.com>

FrankYang0529 · 2022-11-02T12:53:37Z

This all looks great! My only suggestion is to replace references to "config-" as "runtime-config-" (i.e flags and toml sections) to more precisely reflect the "usage semantics" of these configuration values (i.e. runtime). However, if people like it as is, consider this comment an ignoreable nit.

Yeah, rumtime-config-file sounds better to me. Updated it. Thank you.

lann

Thanks for your patience with us through KubeCon. I'm going to give others the rest of today for feedback but LGTM.

FrankYang0529 · 2022-11-02T14:11:15Z

Do we want to add an example app that demonstrates use of the vault config provider as well -- for better visibility/awareness of the functionality?

Create a draft PR in fermyon/developer#191. Thanks!

lann · 2022-11-03T12:32:28Z

Thanks!

radu-matei · 2022-11-03T16:31:15Z

This is looking GREAT, thank you so much for your contribution!

FrankYang0529 force-pushed the add-vault-config-provider branch from becd76c to b657297 Compare October 18, 2022 12:24

FrankYang0529 marked this pull request as ready for review October 18, 2022 12:52

lann reviewed Oct 18, 2022

View reviewed changes

FrankYang0529 force-pushed the add-vault-config-provider branch 2 times, most recently from 7f57e0a to 7418b3d Compare October 19, 2022 16:09

FrankYang0529 force-pushed the add-vault-config-provider branch 3 times, most recently from e8d336c to 62fbe7a Compare October 27, 2022 13:45

FrankYang0529 force-pushed the add-vault-config-provider branch from 62fbe7a to 6344b66 Compare October 29, 2022 05:24

FrankYang0529 requested a review from fibonacci1729 October 31, 2022 14:18

vdice mentioned this pull request Nov 1, 2022

CI: Runner disk usage #869

Closed

fibonacci1729 approved these changes Nov 1, 2022

View reviewed changes

feat(config): add vault provider

926709e

Signed-off-by: Frank Yang <yangpoan@gmail.com>

FrankYang0529 force-pushed the add-vault-config-provider branch from 6344b66 to 926709e Compare November 2, 2022 12:52

lann approved these changes Nov 2, 2022

View reviewed changes

FrankYang0529 mentioned this pull request Nov 2, 2022

feat(config): add vault provider fermyon/developer#191

Merged

7 tasks

lann requested a review from radu-matei November 2, 2022 14:24

vdice mentioned this pull request Nov 2, 2022

CI: Add runtime config provider tests to e2e #872

Open

lann merged commit 7d471e3 into fermyon:main Nov 3, 2022

FrankYang0529 deleted the add-vault-config-provider branch November 3, 2022 12:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(config): add vault provider #798

feat(config): add vault provider #798

FrankYang0529 commented Oct 1, 2022

FrankYang0529 commented Oct 1, 2022

itowlson commented Oct 2, 2022

lann commented Oct 3, 2022

FrankYang0529 commented Oct 4, 2022

FrankYang0529 commented Oct 7, 2022

FrankYang0529 commented Oct 18, 2022

lann Oct 18, 2022

FrankYang0529 Oct 19, 2022

lann Oct 18, 2022

fibonacci1729 Oct 18, 2022

FrankYang0529 Oct 19, 2022

lann Oct 18, 2022

FrankYang0529 Oct 19, 2022

lann Oct 19, 2022

FrankYang0529 Oct 19, 2022

lann Oct 18, 2022

FrankYang0529 Oct 19, 2022

lann commented Oct 19, 2022

FrankYang0529 commented Oct 19, 2022

FrankYang0529 commented Oct 27, 2022

FrankYang0529 commented Oct 27, 2022

lann commented Oct 27, 2022

FrankYang0529 commented Oct 29, 2022 •

edited

Loading

vdice commented Oct 31, 2022

FrankYang0529 commented Nov 1, 2022

fibonacci1729 commented Nov 1, 2022 •

edited

Loading

vdice commented Nov 1, 2022 •

edited

Loading

FrankYang0529 commented Nov 2, 2022

lann left a comment

FrankYang0529 commented Nov 2, 2022

lann commented Nov 3, 2022

radu-matei commented Nov 3, 2022

feat(config): add vault provider #798

feat(config): add vault provider #798

Conversation

FrankYang0529 commented Oct 1, 2022

FrankYang0529 commented Oct 1, 2022

itowlson commented Oct 2, 2022

lann commented Oct 3, 2022

FrankYang0529 commented Oct 4, 2022

FrankYang0529 commented Oct 7, 2022

FrankYang0529 commented Oct 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lann commented Oct 19, 2022

FrankYang0529 commented Oct 19, 2022

FrankYang0529 commented Oct 27, 2022

FrankYang0529 commented Oct 27, 2022

lann commented Oct 27, 2022

FrankYang0529 commented Oct 29, 2022 • edited Loading

vdice commented Oct 31, 2022

FrankYang0529 commented Nov 1, 2022

fibonacci1729 commented Nov 1, 2022 • edited Loading

vdice commented Nov 1, 2022 • edited Loading

FrankYang0529 commented Nov 2, 2022

lann left a comment

Choose a reason for hiding this comment

FrankYang0529 commented Nov 2, 2022

lann commented Nov 3, 2022

radu-matei commented Nov 3, 2022

FrankYang0529 commented Oct 29, 2022 •

edited

Loading

fibonacci1729 commented Nov 1, 2022 •

edited

Loading

vdice commented Nov 1, 2022 •

edited

Loading