Initial commit

skills/ansible-best-practices/anti-patterns/common-mistakes.md

@@ -0,0 +1,698 @@
+# Common Ansible Anti-Patterns and Mistakes
+
+## Overview
+
+This guide catalogs common mistakes found in Ansible playbooks and provides corrected examples based on Virgo-Core
+repository best practices.
+
+## 1. Not Using `set -euo pipefail` in Shell Scripts
+
+### ❌ Wrong
+
+```yaml
+- name: Run multi-line shell script
+  ansible.builtin.shell: |
+    command1
+    command2 | grep something
+    command3
+```
+
+**Problems:**
+
+- Pipe failures ignored (grep returns no matches = rc 1, but shell continues)
+- Undefined variables silently treated as empty strings
+- First command failure doesn't stop execution
+
+### ✅ Correct
+
+```yaml
+- name: Run multi-line shell script
+  ansible.builtin.shell: |
+    set -euo pipefail
+    command1
+    command2 | grep something
+    command3
+  args:
+    executable: /bin/bash
+```
+
+**Benefits:**
+
+- `-e`: Exit on first error
+- `-u`: Treat undefined variables as errors
+- `-o pipefail`: Pipe fails if any command in pipe fails
+- `executable: /bin/bash`: Ensures bash (not sh) interprets the script
+
+## 2. Using Shell When Command Suffices
+
+### ❌ Wrong
+
+```yaml
+- name: List files
+  ansible.builtin.shell: ls -la /tmp
+```
+
+**Problems:**
+
+- Unnecessary shell overhead
+- Shell injection risk if variables used
+- Less portable
+
+### ✅ Correct
+
+```yaml
+- name: List files
+  ansible.builtin.command: ls -la /tmp
+  changed_when: false
+```
+
+**Use `shell` ONLY when you need:**
+
+- Pipes: `cat file | grep pattern`
+- Redirects: `command > output.txt`
+- Environment expansion: `echo $HOME`
+- Shell built-ins: `source`, `cd`, etc.
+
+## 3. Missing `changed_when` on Command/Shell
+
+### ❌ Wrong
+
+```yaml
+- name: Check if VM exists
+  ansible.builtin.command: qm status 101
+```
+
+**Problem:** Reports "changed" even though it's a read-only check
+
+### ✅ Correct
+
+```yaml
+- name: Check if VM exists
+  ansible.builtin.command: qm status 101
+  register: vm_status
+  changed_when: false
+  failed_when: false
+```
+
+## 4. Missing `no_log` on Sensitive Tasks
+
+### ❌ Wrong
+
+```yaml
+- name: Create user with password
+  ansible.builtin.user:
+    name: myuser
+    password: "{{ user_password }}"
+  # Password will appear in logs!
+```
+
+**Problem:** Sensitive data appears in Ansible logs
+
+### ✅ Correct
+
+```yaml
+- name: Create user with password
+  ansible.builtin.user:
+    name: myuser
+    password: "{{ user_password }}"
+  no_log: true
+```
+
+**Always use `no_log: true` with:**
+
+- Passwords
+- API tokens
+- SSH keys
+- Certificates
+- Any PII or sensitive data
+
+## 5. Using Short Module Names
+
+### ❌ Wrong
+
+```yaml
+- name: Copy file
+  copy:
+    src: file.txt
+    dest: /tmp/file.txt
+
+- name: Install package
+  apt:
+    name: nginx
+    state: present
+```
+
+**Problem:** Short names are deprecated and will be removed
+
+### ✅ Correct
+
+```yaml
+- name: Copy file
+  ansible.builtin.copy:
+    src: file.txt
+    dest: /tmp/file.txt
+
+- name: Install package
+  ansible.builtin.apt:
+    name: nginx
+    state: present
+```
+
+**Use Fully Qualified Collection Names (FQCN):**
+
+- `ansible.builtin.copy` not `copy`
+- `ansible.builtin.command` not `command`
+- `community.proxmox.proxmox_kvm` not `proxmox_kvm`
+
+## 6. Hard-Coding Secrets
+
+### ❌ Wrong
+
+```yaml
+- name: Configure database
+  ansible.builtin.template:
+    src: db-config.j2
+    dest: /etc/app/db.yml
+  vars:
+    db_password: "MyPassword123"  # NEVER DO THIS!
+```
+
+**Problems:**
+
+- Secrets in version control
+- No audit trail
+- Difficult to rotate
+- Security violation
+
+### ✅ Correct
+
+```yaml
+- name: Retrieve database password
+  ansible.builtin.include_tasks: tasks/infisical-secret-lookup.yml
+  vars:
+    secret_name: 'DB_PASSWORD'
+    secret_var_name: 'db_password'
+
+- name: Configure database
+  ansible.builtin.template:
+    src: db-config.j2
+    dest: /etc/app/db.yml
+  vars:
+    db_password: "{{ db_password }}"
+  no_log: true
+```
+
+## 7. Not Handling "Already Exists" Gracefully
+
+### ❌ Wrong
+
+```yaml
+- name: Create API token
+  ansible.builtin.command: pveum user token add terraform@pam terraform-token
+  # Fails if token already exists
+```
+
+**Problem:** Playbook not idempotent - fails on second run
+
+### ✅ Correct
+
+```yaml
+- name: Create API token
+  ansible.builtin.command: pveum user token add terraform@pam terraform-token
+  register: token_result
+  changed_when: "'already exists' not in token_result.stderr"
+  failed_when:
+    - token_result.rc != 0
+    - "'already exists' not in token_result.stderr"
+```
+
+**Pattern from repository:** Handle expected errors gracefully
+
+## 8. Missing Task Names
+
+### ❌ Wrong
+
+```yaml
+- ansible.builtin.apt:
+    name: nginx
+    state: present
+
+- ansible.builtin.systemd:
+    name: nginx
+    state: started
+```
+
+**Problem:** Hard to understand playbook output
+
+### ✅ Correct
+
+```yaml
+- name: Install Nginx web server
+  ansible.builtin.apt:
+    name: nginx
+    state: present
+
+- name: Start Nginx service
+  ansible.builtin.systemd:
+    name: nginx
+    state: started
+    enabled: true
+```
+
+**ansible-lint will flag this:** `[name[missing]]`
+
+## 9. Using `when` Instead of `failed_when`
+
+### ❌ Wrong
+
+```yaml
+- name: Run command
+  ansible.builtin.command: some-command
+  register: result
+  ignore_errors: true
+
+- name: Fail if bad
+  ansible.builtin.fail:
+    msg: "Command failed"
+  when: result.rc != 0 and 'acceptable error' not in result.stderr
+```
+
+**Problem:** Two tasks instead of one, less clear
+
+### ✅ Correct
+
+```yaml
+- name: Run command
+  ansible.builtin.command: some-command
+  register: result
+  failed_when:
+    - result.rc != 0
+    - "'acceptable error' not in result.stderr"
+```
+
+## 10. Ignoring Return Codes
+
+### ❌ Wrong
+
+```yaml
+- name: Run deployment script
+  ansible.builtin.command: /usr/local/bin/deploy.sh
+  # No error checking at all
+```
+
+**Problem:** Failures go unnoticed
+
+### ✅ Correct
+
+```yaml
+- name: Run deployment script
+  ansible.builtin.command: /usr/local/bin/deploy.sh
+  register: deploy_result
+
+- name: Verify deployment succeeded
+  ansible.builtin.assert:
+    that:
+      - deploy_result.rc == 0
+      - "'SUCCESS' in deploy_result.stdout"
+    fail_msg: "Deployment failed: {{ deploy_result.stderr }}"
+```
+
+## 11. Not Using Handlers for Service Restarts
+
+### ❌ Wrong
+
+```yaml
+- name: Update Nginx config
+  ansible.builtin.copy:
+    src: nginx.conf
+    dest: /etc/nginx/nginx.conf
+
+- name: Restart Nginx
+  ansible.builtin.systemd:
+    name: nginx
+    state: restarted
+  # Always restarts, even if config didn't change
+```
+
+**Problem:** Unnecessary service restarts
+
+### ✅ Correct
+
+```yaml
+- name: Update Nginx config
+  ansible.builtin.copy:
+    src: nginx.conf
+    dest: /etc/nginx/nginx.conf
+  notify: Restart Nginx
+
+handlers:
+  - name: Restart Nginx
+    ansible.builtin.systemd:
+      name: nginx
+      state: restarted
+```
+
+**Benefits:**
+
+- Only restarts if config changes
+- Multiple tasks can trigger same handler
+- Handler runs once at end
+
+## 12. Using `with_items` Instead of `loop`
+
+### ❌ Wrong (Deprecated)
+
+```yaml
+- name: Install packages
+  ansible.builtin.apt:
+    name: "{{ item }}"
+    state: present
+  with_items:
+    - nginx
+    - docker.io
+    - python3-pip
+```
+
+**Problem:** `with_items` is deprecated
+
+### ✅ Correct
+
+```yaml
+- name: Install packages
+  ansible.builtin.apt:
+    name: "{{ item }}"
+    state: present
+  loop:
+    - nginx
+    - docker.io
+    - python3-pip
+```
+
+**Even better (single task):**
+
+```yaml
+- name: Install packages
+  ansible.builtin.apt:
+    name:
+      - nginx
+      - docker.io
+      - python3-pip
+    state: present
+```
+
+## 13. Not Validating Variables
+
+### ❌ Wrong
+
+```yaml
+- name: Create VM
+  community.proxmox.proxmox_kvm:
+    vmid: "{{ vm_id }}"
+    name: "{{ vm_name }}"
+    # ... config ...
+  # What if vm_id or vm_name is undefined?
+```
+
+**Problem:** Cryptic errors if variables missing
+
+### ✅ Correct
+
+```yaml
+- name: Validate VM variables
+  ansible.builtin.assert:
+    that:
+      - vm_id is defined
+      - vm_id is number
+      - vm_id >= 100
+      - vm_name is defined
+      - vm_name is match('^[a-z0-9-]+$')
+    fail_msg: |
+      Invalid VM configuration:
+      vm_id: {{ vm_id | default('UNDEFINED') }}
+      vm_name: {{ vm_name | default('UNDEFINED') }}
+
+- name: Create VM
+  community.proxmox.proxmox_kvm:
+    vmid: "{{ vm_id }}"
+    name: "{{ vm_name }}"
+    # ... config ...
+```
+
+## 14. Mixing Logic and Data
+
+### ❌ Wrong
+
+```yaml
+- name: Configure based on hostname
+  ansible.builtin.template:
+    src: app-config.j2
+    dest: /etc/app/config.yml
+  vars:
+    db_host: "{{ 'prod-db' if inventory_hostname == 'prod-server' else 'dev-db' }}"
+    # Logic in vars
+```
+
+**Problem:** Hard to maintain, not DRY
+
+### ✅ Correct
+
+**In `group_vars/prod.yml`:**
+
+```yaml
+db_host: prod-db
+```
+
+**In `group_vars/dev.yml`:**
+
+```yaml
+db_host: dev-db
+```
+
+**In playbook:**
+
+```yaml
+- name: Configure application
+  ansible.builtin.template:
+    src: app-config.j2
+    dest: /etc/app/config.yml
+```
+
+## 15. Not Using Tags
+
+### ❌ Wrong
+
+```yaml
+# No tags - must run entire playbook every time
+- name: Install packages
+  ansible.builtin.apt: ...
+
+- name: Configure service
+  ansible.builtin.template: ...
+
+- name: Start service
+  ansible.builtin.systemd: ...
+```
+
+### ✅ Correct
+
+```yaml
+- name: Install packages
+  ansible.builtin.apt: ...
+  tags: [install, packages]
+
+- name: Configure service
+  ansible.builtin.template: ...
+  tags: [config]
+
+- name: Start service
+  ansible.builtin.systemd: ...
+  tags: [service, start]
+```
+
+**Usage:**
+
+```bash
+# Only run config tasks
+ansible-playbook playbook.yml --tags config
+
+# Skip service start
+ansible-playbook playbook.yml --skip-tags start
+```
+
+## 16. Using Bare Variables in Templates
+
+### ❌ Wrong
+
+```jinja
+# templates/config.j2
+database_host: {{ db_host }}
+database_port: {{ db_port }}
+```
+
+**Problem:** YAML parsing errors if values contain special characters
+
+### ✅ Correct
+
+```jinja
+# templates/config.j2
+database_host: "{{ db_host }}"
+database_port: {{ db_port }}
+```
+
+**Rule:** Always quote strings, don't quote numbers/booleans
+
+## 17. Hardcoding Paths
+
+### ❌ Wrong
+
+```yaml
+- name: Copy script
+  ansible.builtin.copy:
+    src: scripts/deploy.sh
+    dest: /opt/myapp/deploy.sh
+  # Assumes specific directory structure
+```
+
+### ✅ Correct
+
+```yaml
+- name: Copy script
+  ansible.builtin.copy:
+    src: "{{ playbook_dir }}/../scripts/deploy.sh"
+    dest: "{{ app_install_dir }}/deploy.sh"
+  vars:
+    app_install_dir: /opt/myapp
+```
+
+## 18. Not Using Blocks for Related Tasks
+
+### ❌ Wrong
+
+```yaml
+- name: Task 1
+  ansible.builtin.command: task1
+  when: deploy_mode == 'production'
+
+- name: Task 2
+  ansible.builtin.command: task2
+  when: deploy_mode == 'production'
+
+- name: Task 3
+  ansible.builtin.command: task3
+  when: deploy_mode == 'production'
+```
+
+**Problem:** Repetitive conditions
+
+### ✅ Correct
+
+```yaml
+- name: Production deployment tasks
+  block:
+    - name: Task 1
+      ansible.builtin.command: task1
+
+    - name: Task 2
+      ansible.builtin.command: task2
+
+    - name: Task 3
+      ansible.builtin.command: task3
+
+  when: deploy_mode == 'production'
+```
+
+## 19. Using `sudo` Instead of `become`
+
+### ❌ Wrong
+
+```yaml
+- name: Install package
+  ansible.builtin.command: sudo apt install nginx
+```
+
+**Problems:**
+
+- Bypasses Ansible's privilege escalation
+- No become_user support
+- Less portable
+
+### ✅ Correct
+
+```yaml
+- name: Install package
+  ansible.builtin.apt:
+    name: nginx
+    state: present
+  become: true
+```
+
+## 20. Not Testing Playbooks
+
+### ❌ Wrong
+
+```bash
+# Write playbook, run directly in production
+ansible-playbook production.yml
+```
+
+### ✅ Correct
+
+```bash
+# 1. Syntax check
+ansible-playbook playbook.yml --syntax-check
+
+# 2. Lint
+ansible-lint playbook.yml
+
+# 3. Dry run (check mode)
+ansible-playbook playbook.yml --check
+
+# 4. Test in development
+ansible-playbook playbook.yml -l dev
+
+# 5. Limited rollout in production
+ansible-playbook playbook.yml -l prod --limit 1
+
+# 6. Full production deployment
+ansible-playbook playbook.yml -l prod
+```
+
+## Quick Reference: Ansible-Lint Rules
+
+Common rules flagged by ansible-lint:
+
+| Rule ID | Description | Fix |
+|---------|-------------|-----|
+| `name[missing]` | Task missing name | Add `name:` field |
+| `fqcn[action-core]` | Use FQCN for modules | `ansible.builtin.copy` not `copy` |
+| `no-changed-when` | Command without `changed_when` | Add `changed_when:` |
+| `risky-shell-pipe` | Shell pipe without `set -o pipefail` | Add `set -euo pipefail` |
+| `no-log-password` | Password without `no_log` | Add `no_log: true` |
+
+**Run ansible-lint:**
+
+```bash
+cd ansible
+ansible-lint playbooks/my-playbook.yml
+```
+
+## Summary: Best Practices Checklist
+
+- [ ] Use `set -euo pipefail` in all shell scripts
+- [ ] Use `changed_when: false` for read-only commands
+- [ ] Add `no_log: true` to sensitive tasks
+- [ ] Use FQCN for all modules
+- [ ] Handle "already exists" errors gracefully
+- [ ] Add descriptive names to all tasks
+- [ ] Validate variables with `assert`
+- [ ] Use handlers for service restarts
+- [ ] Store secrets in Infisical, not playbooks
+- [ ] Test with ansible-lint before committing
+- [ ] Use blocks to group related tasks
+- [ ] Add tags for selective execution
+- [ ] Verify critical operations after execution
+
+## Further Reading
+
+- [Ansible Best Practices](https://docs.ansible.com/ansible/latest/user_guide/playbooks_best_practices.html)
+- [Ansible-Lint Rules](https://ansible-lint.readthedocs.io/rules/)