[fix] Create s3 secrets for OS4 test and production environment #621
Conversation
There was a problem hiding this comment.
Thanks for the excellent work! I assume this pull request goes with another one from https://github.com/epfl-si/wp-operator ? If so, please link both pull requests (both ways) from their respective description.
rosamaggi
force-pushed
the
fix/s3_secrets
branch
from
7328643 to
0e1e938
Compare
There was a problem hiding this comment.
Alllmost there ☺ Here are some additional requests to reduce the coupling between these changes and epfl-si/wp-operator#33 .
| @@ -53,10 +39,21 @@ | |||
| saferpay_prod_customerid: "{{ saferpay.prod.customerid | eyaml(eyaml_keys) | string }}" | |||
| saferpay_prod_terminalid: "{{ saferpay.prod.terminalid | eyaml(eyaml_keys) | string }}" | |||
|
|
|||
| - tags: always | |||
| include_vars: "backup-secrets-{{ inventory_deployment_stage }}.yml" | |||
| - name: MariaDB operator restore secret (OpenShift 3) | |||
There was a problem hiding this comment.
- Please rename the task to
Credentials required to restore from previous (OpenShift 3) production backups
- Please merge with
Secret/epfl-migration-restic, below - And finally, I'm not too fond of this living under a different Ansible tag than the corresponding ConfigMap. Please consider moving these Secrets into
ansible/roles/wordpress-namespace/tasks/wp-operator.yml.
|
|
||
| - name: MariaDB operator restore secret | ||
| - name: MariaDB operator backup secret (OpenShift 4) |
There was a problem hiding this comment.
Please rename the task to
MariaDB operator backup / restore credentials
and likewise, consider moving this into ansible/roles/wordpress-namespace/tasks/wp-operator.yml.
| resticPassword: "{{ _epfl_migration_secrets.resticPassword }}" | ||
|
|
||
| s3_backup_credentials: | ||
| production: |
There was a problem hiding this comment.
As stated in a previous review comment: the caller of s3_backup_credentials needs not know that this variable depends on inventory_deployment_stage. Please move the computation here, and remove the first level of this two-level dict.
| @@ -20,7 +20,7 @@ spec: | |||
| # - ReadWriteOnce | |||
| storage: | |||
| s3: | |||
| bucket: svc0041-b80382f4fba20c6c1d9dc1bebefc5583 | |||
| bucket: svc0041-5da0d377aa221ddc24a6110fee695f69 | |||
There was a problem hiding this comment.
Please also explain here in a comment, where the bucket name comes from.
| @@ -7,7 +7,7 @@ spec: | |||
| mariaDbRef: | |||
| name: mariadb-min | |||
| s3: | |||
| bucket: svc0041-b80382f4fba20c6c1d9dc1bebefc5583 | |||
| bucket: svc0041-5da0d377aa221ddc24a6110fee695f69 | |||
There was a problem hiding this comment.
Please also explain here in a comment, where the bucket name comes from.
| else "50m" }} | ||
| memory: 64Mi | ||
| # This is the default: | ||
| maxRetention: 720h | ||
| storage: | ||
| s3: | ||
| bucket: "{{ s3_bucket }}" | ||
| bucket: "{{ s3_backup_credentials[inventory_deployment_stage]['bucket_name'] }}" |
There was a problem hiding this comment.
Please see my other review comment in s3-secrets-vars.yml below: this line shouldn't have to dereference by inventory_deployment_stage; rather, the knowledge of inventory_deployment_stage should be moved into s3-secrets-vars.yml.
| keyId: "{{ s3_backup_credentials[inventory_deployment_stage]['keyId'] }}" | ||
| accessSecret: "{{ s3_backup_credentials[inventory_deployment_stage]['accessSecret'] }}" | ||
|
|
||
| - name: Restic secret (OpenShift 3) |
There was a problem hiding this comment.
(Pro memoria) Please merge with the other EPFL Secret above.
| apiVersion: v1 | ||
| kind: Secret | ||
| metadata: | ||
| name: epfl-migrations-s3-credentials |
There was a problem hiding this comment.
Please don't keep migrations a plural in the name of the merged Secret, as the rest of this PR uses migration / MIGRATION throughout.
| name: epfl-migrations-s3 | ||
| namespace: "{{ inventory_namespace }}" | ||
| data: | ||
| EPFL_MIGRATION_BUCKET: "{{ s3_epfl_migration_credentials['bucket_name'] }}" |
There was a problem hiding this comment.
(Here and in other places, once the previous review comments have been heeded) You can now say
s3_epfl_migration_credentials.bucket_name
instead of s3_epfl_migration_credentials['bucket_name']. Please be sure to search for bucket_name in the whole PR and do this change everywhere.
| name: wp-backups | ||
| namespace: "{{ inventory_namespace }}" | ||
| data: | ||
| S3_BACKUP_BUCKET: "{{ s3_backup_credentials[inventory_deployment_stage]['bucket_name'] }}" |
There was a problem hiding this comment.
Likewise, please remove the inventory_deployment_stage reference here.
These secrets are for use by the MariaDB operator and (after epfl-si/wp-operator#33 is merged) the WordPress operator.