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.

Sign up for GitHub

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

Jump to bottom

Minio does not restart minio if someone changes access-key/secret-key via juju config #47

Closed
ca-scribner opened this issue Mar 23, 2022 · 0 comments · Fixed by #56
Closed

Minio does not restart minio if someone changes access-key/secret-key via juju config #47

ca-scribner opened this issue Mar 23, 2022 · 0 comments · Fixed by #56
Assignees
@ca-scribner
Labels
bug Something isn't working

Comments

@ca-scribner
Copy link
Contributor

ca-scribner commented Mar 23, 2022 •
edited
Loading

When credentials are updated via juju config minio access-key=somethingNew, the credential secret in k8s is updated but this secret update does not automatically get used by the minio workload’s pod because a change to a secret used by a pod does not cause the pod to restart and reload the secret.

Possible solutions:

  • have the charm initiate a restart of the pod whenever the config changes. This could be achieved directly via lightkube or maybe juju, or indirectly by using a spec annotation that includes a hash of the config
  • similar to above, this solution where you treat your configmap as immutable and on any config change create a new configmap and then modify the existing deployment to point to the new configmap. k8s will only scale down if the configmap is valid
  • use an existing controller like Reloader that automatically restarts workloads when secrets are refreshed.

Note that any solution restarting minio might result in downtime/interruption. The actual minio workload will go down. This shoudl be documented properly. There might be more nuanced solutions, too (can we communicate with the running minio workload to tell it to update the credentials? That sounds like a possible sidecar thing).
@ca-scribner ca-scribner added the bug Something isn't working label Mar 23, 2022
@ca-scribner ca-scribner self-assigned this Mar 23, 2022
ca-scribner added a commit that referenced this issue Mar 28, 2022
@ca-scribner
feat: add hash of config to env vars
cd774fe 
Fixes #47

This adds a hash of the current config to environment variables in order to trigger creating a new pod whenever config changes.  This ensures minio will restart whenever someone does `juju config minio access-key=newKey` or similar.  This also adds unit tests for the new code

Implementation:
* refactor handling the default value for secret-key (moved to `_get_secret_key()`) to make it reusable
* add `_generate_config_hash()` to create the hash of the current config
@ca-scribner ca-scribner mentioned this issue Mar 29, 2022
Closed
ca-scribner added a commit that referenced this issue Mar 31, 2022
@ca-scribner
feat: add hash of config to env vars
6dfb597 
Fixes #47

This adds a hash of the current config to environment variables in order to trigger creating a new pod whenever config changes.  This ensures minio will restart whenever someone does `juju config minio access-key=newKey` or similar.  This also adds unit tests for the new code

Implementation:
* refactor handling the default value for secret-key (moved to `_get_secret_key()`) to make it reusable
* add `_generate_config_hash()` to create the hash of the current config
@ca-scribner ca-scribner mentioned this issue Apr 1, 2022
Merged
ca-scribner added a commit that referenced this issue Apr 1, 2022
@ca-scribner
feat: add hash of config to env vars
df53245 
Fixes #47

This adds a hash of the current config to environment variables in order to trigger creating a new pod whenever config changes.  This ensures minio will restart whenever someone does `juju config minio access-key=newKey` or similar.  This also adds unit tests for the new code

Implementation:
* refactor handling the default value for secret-key (moved to `_get_secret_key()`) to make it reusable
* add `_generate_config_hash()` to create the hash of the current config
ca-scribner added a commit that referenced this issue Apr 1, 2022
@ca-scribner
feat: add hash of config to env vars
be04573 
Fixes #47

This adds a hash of the current config to environment variables in order to trigger creating a new pod whenever config changes.  This ensures minio will restart whenever someone does `juju config minio access-key=newKey` or similar.  This also adds unit tests for the new code

Implementation:
* refactor handling the default value for secret-key (moved to `_get_secret_key()`) to make it reusable
* add `_generate_config_hash()` to create the hash of the current config
DomFleischmann pushed a commit that referenced this issue Apr 4, 2022
@ca-scribner
fix: credential refresh on config change (#50)
882908f 
* feat: add hash of config to env vars

Fixes #47

This adds a hash of the current config to environment variables in order to trigger creating a new pod whenever config changes.  This ensures minio will restart whenever someone does `juju config minio access-key=newKey` or similar.  This also adds unit tests for the new code

Implementation:
* refactor handling the default value for secret-key (moved to `_get_secret_key()`) to make it reusable
* add `_generate_config_hash()` to create the hash of the current config

* fix: _get_secret_key try/except caught wrong exception

Fixes bug where we catch errors about missing data in self._stored.  Was using `except KeyError`, but `self._stored` raises an `AttributeError` if data is not available.

* fix: linting/formatting

* feat: Add integration test for credential refresh

* fix: test_refresh_credentials

Updates test_refresh_credentials to use the refactored connect_client_to_server.
DomFleischmann pushed a commit that referenced this issue Apr 7, 2022
@ca-scribner
fix: credential refresh on config change (#56)
ff66ebc 
* Refactor integration tests (#49)

* feat: make integration test idempotent
* refactor: encapsulate logic for tests, moving connect-to-minio logic into a reusable helper so it can be used by multiple tests
* refactor: add arg for access/secret_key in connect_client_to_server
* feat: make connection test attempts auto-retry using tenacity.retry
* fix: unpin black to fix linting/formatting

* fix: credential refresh on config change (#50)

* feat: add hash of config to env vars

Fixes #47

This adds a hash of the current config to environment variables in order to trigger creating a new pod whenever config changes.  This ensures minio will restart whenever someone does `juju config minio access-key=newKey` or similar.  This also adds unit tests for the new code

Implementation:
* refactor handling the default value for secret-key (moved to `_get_secret_key()`) to make it reusable
* add `_generate_config_hash()` to create the hash of the current config

* fix: _get_secret_key try/except caught wrong exception

Fixes bug where we catch errors about missing data in self._stored.  Was using `except KeyError`, but `self._stored` raises an `AttributeError` if data is not available.

* fix: linting/formatting

* feat: Add integration test for credential refresh

* fix: test_refresh_credentials

Updates test_refresh_credentials to use the refactored connect_client_to_server.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ca-scribner ca-scribner

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: credential refresh on config change canonical/minio-operator
1 participant
@ca-scribner