Skip to content
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!: add support for secrets in manifests #307

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

protochron
Copy link
Contributor

@protochron protochron commented Jun 17, 2024

Feature or Problem

This adds support for secrets in wasmCloud application manifests. The
secrets themselves are actually secret references as outlined in
wasmCloud/wasmCloud#2190. Just like config, secrets can be specified at
the component or provider level or on a link.

Secret references themselves are actually implemented as an additional
kind of config stored in the same config data bucket. However, I opted
to implement a dedicated scaler for secrets that is largely a clone of
the existing ConfigScaler since the underlying data type is very
different than the arbitrary set of key/value pairs we use for config.

An example of what this looks like in a component is shown below:

spec:
  components:
    - name: http-component
      type: component
      properties:
        image: ghcr.io/wasmcloud/test-fetch-with-token:0.1.0-fake
        secrets:
          - name: some-api-token
            source:
              backend: nats-kv
              key: test-value
              version: 1
          - name: my-other-secret
            source:
              backend: aws-secrets-manager
              value: secret-name
              version: "be01a5fb-7ebb-4ae9-8ea0-0902e8940bc0"

This contains a breaking change to the way that we specify config on
links:

- type: link
  properties:
    namespace: wasmcloud
    package: postgres
    interfaces: [managed-query]
    target:
      name: sql-postgres
      secrets:
        - name: db-password
          source:
            backend: nats-kv
            key: myapp_db-password
            version: 1

Instead of using target_config and source_config, this renames them
to target and source respectively and adds keys for config and
secrets. The name of the target is now a key at the top
level of the target block, as seen above.

Related Issues

Release Information

Consumer Impact

Testing

Unit Test(s)

Acceptance or Integration

Manual Verification

@protochron protochron force-pushed the secrets_support branch 3 times, most recently from 14229b6 to 157ffca Compare July 11, 2024 16:12
@protochron protochron force-pushed the secrets_support branch 3 times, most recently from 4f00914 to 6039d41 Compare July 11, 2024 16:22
@protochron protochron changed the title Secrets support feat!: add support for secrets in manifests Jul 11, 2024
@protochron protochron marked this pull request as ready for review July 11, 2024 16:22
@protochron protochron force-pushed the secrets_support branch 3 times, most recently from da265d7 to f174e4c Compare July 11, 2024 16:58
Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

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

One or two nits, and a bigger request around backwards compatible manifests. I found myself wondering if we needed a SecretScaler at all given that it's just a wrapper around configuration, but I think the separation is worth it at the very least for clarity and also for the future where we may do more complex things in response to a secret.

crates/wadm-types/src/lib.rs Outdated Show resolved Hide resolved
crates/wadm-types/src/lib.rs Show resolved Hide resolved
Comment on lines 284 to 331
#[serde(default, skip_serializing_if = "Vec::is_empty")]
pub source_config: Vec<ConfigProperty>,
pub source: Option<ConfigDefinition>,
/// Configuration to apply to the target of the link
#[serde(default, skip_serializing_if = "Vec::is_empty")]
pub target_config: Vec<ConfigProperty>,
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be great to support the source_config and target_config properties here in order to support the current manifest format in a backwards-compatible deprecated way, e.g. if source_config is supplied but source is not, we fill in the source property at deserialization time. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went ahead and updated the code so that both source_config and target_config transformed into the new representation. We'll have to figure out how to prepare to remove them entirely.

Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

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

@protochron and I have discussed the issue I brought up and decided that it would be best to transform manifests of the "old" source/target config format to the new automatically using wash. I think it would be nice to return a good error instead of a failed deserialization error / failed validation error if possible, in the case where folks are using the Wadm API directly instead of using wash (e.g. the operator, right?)

Let's see if we can use a wash plugin for the translation

@protochron protochron force-pushed the secrets_support branch 3 times, most recently from 440bbf4 to a7684ff Compare July 18, 2024 21:20
Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

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

This PR largely looks really nice. Huge amount of changes, thanks for dealing with the copy/paste. Just a few requests, at least half are to remove comments/printlns

crates/wadm-types/oam/README.md Outdated Show resolved Hide resolved
crates/wadm-types/oam/README.md Outdated Show resolved Hide resolved
crates/wadm-types/src/lib.rs Outdated Show resolved Hide resolved
crates/wadm-types/src/validation.rs Outdated Show resolved Hide resolved
crates/wadm/oam/README.md Outdated Show resolved Hide resolved
crates/wadm/src/scaler/manager.rs Outdated Show resolved Hide resolved
tests/fixtures/manifests/simple.wadm.yaml Outdated Show resolved Hide resolved
Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

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

LGTM! Just a test failing it seems

This adds support for secrets in wasmCloud application manifests. The
secrets themselves are actually _secret references_ as outlined in
wasmCloud/wasmCloud#2190. Just like config, secrets can be specified at
the component or provider level or on a link.

Secret references themselves are actually implemented as an additional
kind of config stored in the same config data bucket. However, I opted
to implement a dedicated scaler for secrets that is largely a clone of
the existing ConfigScaler since the underlying data type is very
different than the arbitrary set of key/value pairs we use for config.

An example of what this looks like in a component is shown below:

```yaml
spec:
  components:
    - name: http-component
      type: component
      properties:
        image: ghcr.io/wasmcloud/test-fetch-with-token:0.1.0-fake
        secrets:
          - name: some-api-token
            source:
              backend: nats-kv
              key: test-value
              version: 1
          - name: my-other-secret
            source:
              backend: aws-secrets-manager
              value: secret-name
              version: "be01a5fb-7ebb-4ae9-8ea0-0902e8940bc0"
```

This contains a breaking change to the way that we specify config on
links:

```yaml
- type: link
  properties:
    namespace: wasmcloud
    package: postgres
    interfaces: [managed-query]
    target:
      name: sql-postgres
      secrets:
        - name: db-password
          source:
            backend: nats-kv
            key: myapp_db-password
            version: 1
```

Instead of using `target_config` and `source_config`, this renames them
to `target` and `source` respectively and adds keys for `config` and
`secrets`. The name of the target is now now a key at the top level of
the `target` block, as seen above.

Signed-off-by: Dan Norris <protochron@users.noreply.github.com>
@brooksmtownsend brooksmtownsend merged commit 737aa82 into wasmCloud:main Jul 19, 2024
4 checks passed
@protochron protochron deleted the secrets_support branch July 19, 2024 17:19
protochron added a commit to protochron/wadm that referenced this pull request Jul 19, 2024
In wasmCloud#307 we introduced secrets support, which works by storing secrets
references as config in NATS KV. This fixes a bug in the way that we
were serializing the configuration as JSON where we were overwriting the
policy field which is required.
protochron added a commit to protochron/wadm that referenced this pull request Jul 19, 2024
In wasmCloud#307 we introduced secrets support, which works by storing secrets
references as config in NATS KV. This fixes a bug in the way that we
were serializing the configuration as JSON where we were overwriting the
policy field which is required.

Signed-off-by: Dan Norris <protochron@users.noreply.github.com>
protochron added a commit to protochron/wadm that referenced this pull request Jul 19, 2024
In wasmCloud#307 we introduced secrets support, which works by storing secrets
references as config in NATS KV. This fixes a bug in the way that we
were serializing the configuration as JSON where we were overwriting the
policy field which is required.

Signed-off-by: Dan Norris <protochron@users.noreply.github.com>
protochron added a commit to protochron/wadm that referenced this pull request Jul 19, 2024
In wasmCloud#307 we introduced secrets support, which works by storing secrets
references as config in NATS KV. This fixes a bug in the way that we
were serializing the configuration as JSON where we were overwriting the
policy field which is required.

Signed-off-by: Dan Norris <protochron@users.noreply.github.com>
brooksmtownsend pushed a commit that referenced this pull request Jul 19, 2024
In #307 we introduced secrets support, which works by storing secrets
references as config in NATS KV. This fixes a bug in the way that we
were serializing the configuration as JSON where we were overwriting the
policy field which is required.

Signed-off-by: Dan Norris <protochron@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants