Add config options for access-key, secret-key #76
Conversation
sed-i
changed the title
There was a problem hiding this comment.
Thanks @sed-i, def. an improvement we need to do. What I would recommend instead is to use Juju's user-secret feature. We have another example already doing so. It is also compliant with terraform, as juju provider allows to create secrets as resources.
We have a very similar pattern for object-storage-integrator, which is used with azure, here. I recommend:
- remove the
access-keyandsecret-keyconfigs and create one config namedcredentials - pass
credentialsas a typesecret.
Also, can you update README and the config description to explain how to use juju add-secret, as described here.
| Access Key (account) for connecting to the object storage. | ||
| This config option takes precedence over values passed via the sync-s3-credentials action. | ||
| This value must be unset for the action to have effect. | ||
| secret-key: |
There was a problem hiding this comment.
If we are handling secrets, then we should use user secrets here. One example
| secret-key: | ||
| type: string | ||
| description: | | ||
| Access Key Secret (password) for connecting to the object storage. |
There was a problem hiding this comment.
| Access Key Secret (password) for connecting to the object storage. | |
| Create a secret in juju with: `juju add-secret ...` | |
| Pass the secret ID as this config option instead. |
| # We sync credentials only if one of the config options is given, because we do | ||
| # not want to accidentally wipe credentials previously set by the sync action. | ||
| access_key = self.config.get("access-key") | ||
| secret_key = self.config.get("secret-key") |
There was a problem hiding this comment.
| secret_key = self.config.get("secret-key") | |
| try: | |
| secret_credentials_id = self.config.get("credentials") | |
| access_key = model.get_secret(id=secret_credentials_id).get_content(refresh=True).get("access-key") | |
| secret_key = model.get_secret(id=secret_credentials_id).get_content(refresh=True).get("secret-key") | |
| except ...: |
As shown here
| if access_key or secret_key: | ||
| # This will be called twice, but it's ok because ops buffers relation writes. | ||
| self._sync_s3_credentials( | ||
| self.config.get("access-key"), self.config.get("secret-key") |
There was a problem hiding this comment.
Same comment as above, replace it for the secret contents instead.
|
Hi Leon, It is an intentional design choice not to have access and secret keys as config values because this approach is not secure. On the other hand, we cannot move to user secrets yet because we need to support Juju 2.9 deployments. |
|
Hi @delgod, |
|
Can we reconsider this per @sed-i's comment? |
Problem
For COS we're now using s3-integrator in all deployments.
With terraform, running actions on a deployment is a bit involved.
Solution
Add config options for access-key, secret-key.
Since the secrets are already stored in reldata, not much additional harm having them also in the config.
Fixes #62.
Side-effect
Since we can now have credentials from two sources (action, config), the new logic introduced in this PR has a new side effect: even when the config options are cleared via
juju config s3i access-key= secret-key= # or juju config s3i --reset access-key,secret-keythe credentials still stay in app data. To clear app data after config values were cleared, we'd need to follow-up with the action,