Wpb 21356 remove bash dep #835
Conversation
…ntroduced the flags for cert_manager, clean old kubectl commands and fix ansible tags
…oy_k8s.sh to wire_values, made it idempotent
…loy_k8s.sh to wire_secrets playbook, removed dependency from zauth container
…eploy_k8s.sh to helm_install playbook
…ll_pkgs, converted native kubectl commands to ansible native tasks
… refactor clean_cluster playbook according to new changes
Veki301
left a comment
Veki301
left a comment
There was a problem hiding this comment.
I might not be the best for this, as I've not touched this solution since your first iteration, but I see no faults here, LGTM 👍
sghosh23
left a comment
sghosh23
left a comment
There was a problem hiding this comment.
There are lot of changes, I have some clarification questions which I asked here. Let me know if you need to more clarity.
| - minio_secret_key length: {{ minio_secret_key | length if minio_secret_key is defined else 'UNDEFINED' }} (expected 42) | ||
| quiet: yes | ||
|
|
||
| - name: Generate Ed25519 cryptographic keys for zauth |
There was a problem hiding this comment.
I guess this is where you are removing the zauth container dependency. I would love to have some explanation here.
What kind of issue got resolved?
Is this solution compatible with zauth porived secrets?
There was a problem hiding this comment.
we are free from zauth container dependency. Zauth is used for multiple other reasons which we don't use but since its flag changes led to failure in our secret creation playbooks, taking out the logic from the zauth library frees us from how zauth will be managed.
|
|
||
| - name: Detect if secrets are unchanged by comparing hashes | ||
| set_fact: | ||
| skip_secret_generation: "{{ (stored_secrets_hash.content | b64decode | trim) == current_secret_hash }}" |
There was a problem hiding this comment.
How this comparison works? If a new chart is added and I re-run the playbook on an existing deployment, will it evaluate as false to create new secret?
There was a problem hiding this comment.
yes, it is storing the has for one of the secret at the end of the playbook: https://github.com/wireapp/wire-server-deploy/blob/wpb-21356-remove-bash-dep/ansible/wiab-demo/wire_secrets.yml#L484 and at the next run it compares the hash in the subsequent runs.
Sorry I didn't follow where the chart is getting added? as of now all the charts are defined in host.yml inventory, if something is brought in which requires more secret management than it has to be done manually unless patched in wire_secrets.yml playbook.
There was a problem hiding this comment.
this skip_secret_generation gate is distributed into multiple tasks which gives me the impression of some breaking logics somewhere. But if you say it works as per design then its fine
| that: | ||
| - charts_to_deploy is defined | ||
| - additional_charts is defined | ||
| - values_dir is defined |
There was a problem hiding this comment.
values_dir is defined but wire_vars is not, is it overlooked?
There was a problem hiding this comment.
let me patch this
| environment: | ||
| BASE_DIR: "{{ ansible_user_dir }}/wire-server-deploy" | ||
|
|
||
| - name: Generate random strings for zrest, minio_access_key, and minio_secret_key |
There was a problem hiding this comment.
Shall we think of persisting these secrets or it is already persisting not generating on every run?
There was a problem hiding this comment.
these are persisted by comparing the hashes of the secret present in secrets.yaml file vs the wire_vars/wire_secrets file
https://github.com/wireapp/wire-server-deploy/pull/835/files#diff-0217c2a6efe32c1018226d8d237dd364c054d304c5b306af015491d8ed8dad5dR47
|
|
||
| - name: Generate Prometheus authentication credentials | ||
| block: | ||
| - name: Generate random string for prometheus basic auth password |
There was a problem hiding this comment.
is the secret persisting or generating a new one with a new run?
There was a problem hiding this comment.
wire_secrets.yaml playbook is idempotent, it tries to store hash of a secret created and then compares the hash again in second run, if the hash matches, it won't regenerate any secret. for postgresql, if the secret already exists in the k8s cluster, it won't overwrite it.
| - name: Parse and calculate hash of Zauth Publickey | ||
| set_fact: | ||
| current_secret_hash: "{{ (secrets_file_slurp.content | b64decode | from_yaml).brig.secrets.zAuth.publicKeys | hash('sha256') }}" | ||
| ignore_errors: true |
There was a problem hiding this comment.
Why error's are ignored here?
There was a problem hiding this comment.
in case the full yaml path isn't preset or someone updated the secret and it is not well formatted. The playbbok will not fail but consider it as non-existing value
| - pyyaml>=5.4.1 | ||
| executable: /usr/bin/pip3 | ||
| state: present | ||
| extra_args: "--break-system-packages" |
There was a problem hiding this comment.
Wow, break-system-packages what are we breaking here, is it bypassing some security?
There was a problem hiding this comment.
this is required to install pip packages - by default due to PEP 668 https://peps.python.org/pep-0668/
There was a problem hiding this comment.
I am adding a note for it in the documentation, will try to manage it better in the next iteration
ansible/wiab-demo/install_pkgs.yml
Outdated
| block: | ||
| - name: Detect system architecture | ||
| set_fact: | ||
| system_arch: "{{ 'amd64' if ansible_architecture == 'x86_64' else 'arm64' if ansible_architecture == 'aarch64' else 'amd64' }}" |
There was a problem hiding this comment.
Are we supporting arch64 ?
but you have helm checksum for amd64
There was a problem hiding this comment.
no, will remove it
| shell: | | ||
| #!/bin/bash | ||
| source_file="{{ item.item.path }}/values.yaml" | ||
| backup_file="{{ item.item.path }}/values.yaml.bak.{{ lookup('pipe', 'date +%Y%m%d_%H%M%S') }}" |
There was a problem hiding this comment.
I used "{{ ansible_date_time.iso8601_basic_short }}" as backup timestamp which provides a dynamic time always
There was a problem hiding this comment.
I carried the old format from the script, can switch to above
| (('minikube' not in ansible_skip_tags or | ||
| 'asset_host' not in ansible_skip_tags or | ||
| 'seed_containers' not in ansible_skip_tags) | ||
| and (ansible_skip_tags | length > 0)) |
There was a problem hiding this comment.
So when to tags are passed it wont run?
There was a problem hiding this comment.
yes, if all 3 tags are skipped it won't run but if anyone is passed or none is passed, it will run.
… in wire_secrets playbook and only consider amd64 architecture
|
@sghosh23 Thanks for the review, i have made some changes based on the review. |
|
Fixed: the values for demo deployments as per new wiab process
Changed: introduced the flags for cert_manager, clean old kubectl commands and fix ansible tags
Changed: move helm values handling logic from offline_deploy_k8s.sh to wire_values, made it idempotent
Added: helm secrets handling moved from offline_deploy_k8s.sh to wire_secrets playbook, removed zauth container dependency
Changed: added helm and kubernetes packages to install_pkgs, converted native kubectl commands to ansible native tasks
Changed: improved wiab-demo documentation and refactored clean_cluster playbook according to new changes
Removed: zauth container from demo bundle, copy only offline-env.sh
Change type
Basic information
Testing
Tracking
changelog.dKnowledge Transfer
Motivation
Objective
Reason
Use case