Skip to content

Conversation

@ChunyiLyu
Copy link
Contributor

@ChunyiLyu ChunyiLyu commented Jun 6, 2022

This closes #386

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed
Note to contributors: remember to re-generate client set if there are any API changes

Summary Of Changes

Users will be able to reference a vault secret path spec.secretBackend.vault.secretPath to configure username and password for schema sync parameters.

  • require either spec.secretBackend.vault.secretPath or spec.secretBackend.vault, but not both (verified in webhook)
  • tested in integration tests suite (and manually verified behavior on create/update/delete)
  • spec.secretBackend.vault.secretPath is updatable
  • example schemasync.rabbitmq.com using vault
---
apiVersion: rabbitmq.com/v1beta1
kind: SchemaReplication
metadata:
  name: upstream
spec:
  endpoints: "test:134556"
  secretBackend:
    vault:
      secretPath: secret/data/rabbitmq/config
  rabbitmqClusterReference:
    name: vault-default-user

Additional Context

ChunyiLyu added 2 commits June 1, 2022 17:29
- error string was duplicated. For example:
"error": "unable to retrieve credentials from secret store:
unable to read Vault secret: unable to read Vault secret:
Copy link
Contributor

@coro coro left a comment

Choose a reason for hiding this comment

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

LGTM! I've one case where some invalid YAML can make it through to the controller; I'll leave it up to you if you would like to fix it or not.

"do not provide both secretBackend.vault.secretPath and upstreamSecret"))
}

if s.Spec.UpstreamSecret == nil && s.Spec.SecretBackend.Vault == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't catch Vault being non-nil but with the secret unset, e.g.:

---
apiVersion: rabbitmq.com/v1beta1
kind: SchemaReplication
metadata:
  name: upstream
spec:
  endpoints: "test:134556"
  secretBackend:
    vault: {}
  rabbitmqClusterReference:
    name: hello-world

This is later caught in the controller, but it's not validated upfront by this webhook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point and updated.

@ChunyiLyu ChunyiLyu merged commit 4bb0d0c into main Jun 8, 2022
@ChunyiLyu ChunyiLyu deleted the schemasync-vault branch June 8, 2022 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

schemareplication cr should support getting username/password from Vault

4 participants