Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

Add support for configurable ExternalSecrets backendType #157

Merged

Conversation

soleares
Copy link
Collaborator

@soleares soleares commented Jun 29, 2021

Add support for SSM backend type for ExternalSecrets by adding a new placeholder value. It defaults the value to secretsManager so nothing changes for existing users.

@karlschriek
Copy link
Contributor

Looks good, I think you missed one though, in "distribution/kubeflow/pipelines/base/patches/sync_with_s3.py". It may however make more sense to change it to a Python variable here and put the placeholder in an ENV var.

All of the ExternalSecret resources also have "roleArn" fields which are only relevant for AWS. Does it matter if they are just left as is?

@soleares
Copy link
Collaborator Author

soleares commented Jul 1, 2021

@karlschriek Good catch. I've added it as an env variable.

I think the roleArn is a separate topic, but I've been thinking about it and wanting to chat with you or @davidspek about it.

  • The README lists
    • Option 1 - 1 role for all secrets
    • Option 2 - 1 role per namespace
  • The repo only supports option 2 out of the box. To get it to work with option 1 you need to:
    • Remove all of the roleArn annotations in ExternalSecrets so it doesn't assume a role and just uses the service account's role when syncing the secret.
    • Set all of the external secrets role arns in setup.conf to the same role.
  • Option 2 is preferred from a security standpoint according to a security colleague of mine. Meaning it's preferred to have roles per namespace or secret rather than having 1 role for all secrets.

So I think it might be better in the README to remove the options language and just explain how to setup secrets (which would be option 2) and then have an "alternative setup" section explaining how to setup option 1. I'm happy to take a pass at this if it makes sense. We could also move this to an Issue to discuss further. Thoughts?

@karlschriek
Copy link
Contributor

You are right of course, didn't think about that! Feel free to have a go at removing the "option" language and just offering "Option 2", I think that would be the right approach

@karlschriek
Copy link
Contributor

PR looks good though, thanks for the help!

@karlschriek karlschriek merged commit d4e8aba into argoflow:master Jul 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants