From c3b966bc3f54af22db43ad7677c76903c4881d88 Mon Sep 17 00:00:00 2001 From: Martin Weinelt Date: Sat, 7 Aug 2021 14:30:46 +0200 Subject: [PATCH 01/16] Migrate managed ssh keys to ed25519 --- tasks/ssh_cluster_config.yml | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tasks/ssh_cluster_config.yml b/tasks/ssh_cluster_config.yml index b1c34f78..216a9a8c 100644 --- a/tasks/ssh_cluster_config.yml +++ b/tasks/ssh_cluster_config.yml @@ -9,19 +9,20 @@ user: name: root generate_ssh_key: yes - ssh_key_file: /root/.ssh/id_rsa - ssh_key_type: rsa + ssh_key_bits: 521 + ssh_key_file: /root/.ssh/id_ed25519 + ssh_key_type: ed25519 ssh_key_comment: "root@{{ inventory_hostname_short }}" - name: Fetch root SSH public key slurp: - src: /root/.ssh/id_rsa.pub - register: proxmox_root_id_rsa_pub + src: /root/.ssh/id_ed25519.pub + register: _proxmox_root_ssh_pubkey - name: Authorize all hosts' root SSH public keys authorized_key: user: root - key: "{{ hostvars[item].proxmox_root_id_rsa_pub.content | b64decode }}" + key: "{{ hostvars[item]._proxmox_root_ssh_pubkey.content | b64decode }}" with_items: "{{ groups[pve_group] }}" - name: Configure SSH clients for connecting to PVE cluster hosts @@ -32,7 +33,7 @@ content: | {% for host in groups[pve_group] %} Host {{ hostvars[host].pve_cluster_ssh_addrs | join(" ") }} - IdentityFile /root/.ssh/id_rsa + IdentityFile /root/.ssh/id_ed25519 Port {{ pve_ssh_port }} {% endfor %} From 04f0b365a5d6c51fc0ba312fb9eba5feadcbbbe2 Mon Sep 17 00:00:00 2001 From: Musee Ullah Date: Wed, 19 Apr 2023 00:31:02 +0900 Subject: [PATCH 02/16] Specify port in known_hosts when adding cluster nodes. Fixes #221 --- tasks/pve_add_node.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tasks/pve_add_node.yml b/tasks/pve_add_node.yml index f96d1f81..a27a1395 100644 --- a/tasks/pve_add_node.yml +++ b/tasks/pve_add_node.yml @@ -3,7 +3,7 @@ - name: Identify the SSH public key and SSH addresses of initial cluster host ansible.builtin.set_fact: _pve_cluster_host_key: "{{ ' '.join((hostvars[_init_node]._pve_ssh_public_key.content | b64decode).split()[:-1]) }}" - _pve_cluster_host_addresses: "{{ hostvars[_init_node].pve_cluster_ssh_addrs | join(',') }}" + _pve_cluster_host_addresses: "{{ hostvars[_init_node].pve_cluster_ssh_addrs | map('regex_replace', '^(.*)$', (pve_ssh_port == 22) | ternary('\\1', '[\\1]:' + (pve_ssh_port | string))) | join(',') }}" - name: Temporarily mark that cluster host as known in root user's known_hosts ansible.builtin.blockinfile: From 206af71160708d1c9ba74dd027183507c7d45997 Mon Sep 17 00:00:00 2001 From: Alexander Petermann Date: Thu, 19 Sep 2024 01:40:37 +0200 Subject: [PATCH 03/16] feature: make watchdog configuration more flexible --- README.md | 4 +++- tasks/kernel_module_cleanup.yml | 13 +++++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 6f4c438f..ca47e911 100644 --- a/README.md +++ b/README.md @@ -198,7 +198,9 @@ this group name as well, unless otherwise specified by `pve_cluster_clustername` Leaving this undefined will default to `proxmox`. `pve_watchdog` here enables IPMI watchdog support and configures PVE's HA -manager to use it. Leave this undefined if you don't want to configure it. +manager to use it. Use `None` or leave this undefined to use the default +proxmox software watchdog. If set to anything else, the value is expected to be +a watchdog kernel module. `pve_ssl_private_key` and `pve_ssl_certificate` point to the SSL certificates for pvecluster. Here, a file lookup is used to read the contents of a file in the diff --git a/tasks/kernel_module_cleanup.yml b/tasks/kernel_module_cleanup.yml index a29a5dfa..8ee79718 100644 --- a/tasks/kernel_module_cleanup.yml +++ b/tasks/kernel_module_cleanup.yml @@ -15,6 +15,11 @@ when: "not pve_zfs_enabled | bool" - block: + - name: ensure valid watchdog + ansible.builtin.set_fact: + pve_watchdog: softdog + when: pve_watchdog is not defined or pve_watchdog | lower == 'none' + - name: Re-enable nmi_watchdog via GRUB config lineinfile: dest: /etc/default/grub @@ -27,13 +32,13 @@ dest: /etc/modprobe.d/ipmi_watchdog.conf state: absent - - name: Load softdog + - name: Load watchdog driver modprobe: - name: softdog + name: "{{ pve_watchdog }}" - - name: Set PVE HA Manager watchdog configuration back to default + - name: Set PVE HA Manager watchdog to the configured driver copy: - content: "WATCHDOG_MODULE=softdog" + content: "WATCHDOG_MODULE={{ pve_watchdog }}" dest: /etc/default/pve-ha-manager mode: 0640 notify: From efc624e2d8b2d52d2295e6d256c9751505fa16c6 Mon Sep 17 00:00:00 2001 From: Julian Foad Date: Mon, 30 Sep 2024 09:01:57 +0000 Subject: [PATCH 04/16] README: update to mention Debian 12/bookworm. --- README.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index ca47e911..0ba46eb8 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ lae.proxmox Installs and configures Proxmox Virtual Environment 6.x/7.x/8.x on Debian servers. This role allows you to deploy and manage single-node PVE installations and PVE -clusters (3+ nodes) on Debian Buster (10) and Bullseye (11). You are able to +clusters (3+ nodes) on Debian Buster (10) and Bullseye (11) and Bookworm (12). You are able to configure the following with the assistance of this role: - PVE RBAC definitions (roles, groups, users, and access control lists) @@ -379,10 +379,12 @@ serially during a maintenance period.) It will also enable the IPMI watchdog. ## Role Variables +*About default values: Some of the default values are selected at run time and so can differ from the example listed here.* + ``` [variable]: [default] #[description/purpose] pve_group: proxmox # host group that contains the Proxmox hosts to be clustered together -pve_repository_line: "deb http://download.proxmox.com/debian/pve bullseye pve-no-subscription" # apt-repository configuration - change to enterprise if needed (although TODO further configuration may be needed) +pve_repository_line: "deb http://download.proxmox.com/debian/pve bookworm pve-no-subscription" # apt-repository configuration - change to enterprise if needed (although TODO further configuration may be needed) pve_remove_subscription_warning: true # patches the subscription warning messages in proxmox if you are using the community edition pve_extra_packages: [] # Any extra packages you may want to install, e.g. ngrep pve_run_system_upgrades: false # Let role perform system upgrades @@ -409,7 +411,7 @@ pve_zfs_enabled: no # Specifies whether or not to install and configure ZFS pack # pve_zfs_zed_email: "" # Should be set to an email to receive ZFS notifications pve_zfs_create_volumes: [] # List of ZFS Volumes to create (to use as PVE Storages). See section on Storage Management. pve_ceph_enabled: false # Specifies wheter or not to install and configure Ceph packages. See below for an example configuration. -pve_ceph_repository_line: "deb http://download.proxmox.com/debian/ceph-pacific bullseye main" # apt-repository configuration. Will be automatically set for 6.x and 7.x (Further information: https://pve.proxmox.com/wiki/Package_Repositories) +pve_ceph_repository_line: "deb http://download.proxmox.com/debian/ceph-pacific bookworm main" # apt-repository configuration. Will be automatically set for 6.x and 7.x (Further information: https://pve.proxmox.com/wiki/Package_Repositories) pve_ceph_network: "{{ (ansible_default_ipv4.network +'/'+ ansible_default_ipv4.netmask) | ansible.utils.ipaddr('net') }}" # Ceph public network # pve_ceph_cluster_network: "" # Optional, if the ceph cluster network is different from the public network (see https://pve.proxmox.com/pve-docs/chapter-pveceph.html#pve_ceph_install_wizard) pve_ceph_nodes: "{{ pve_group }}" # Host group containing all Ceph nodes @@ -905,8 +907,8 @@ It can be later removed by unsetting this role variable. When developing new features or fixing something in this role, you can test out your changes by using Vagrant (only libvirt is supported currently). The playbook can be found in `tests/vagrant` (so be sure to modify group variables -as needed). Be sure to test any changes on both Debian 10 and 11 (update the -Vagrantfile locally to use `debian/buster64`) before submitting a PR. +as needed). Be sure to test any changes on all supported versions of Debian (update the +Vagrantfile locally to use `debian/bookworm64`, `debian/bullseye64`, or `debian/buster64`) before submitting a PR. You can also specify an apt caching proxy (e.g. `apt-cacher-ng`, and it must run on port 3142) with the `APT_CACHE_HOST` environment variable to speed up From b663dc4f14fd89156434a22bf5f2ffe58a2bc159 Mon Sep 17 00:00:00 2001 From: lae Date: Fri, 15 Nov 2024 00:02:02 +0900 Subject: [PATCH 05/16] [lint] tasks/ssh_cluster_config.yml --- tasks/ssh_cluster_config.yml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tasks/ssh_cluster_config.yml b/tasks/ssh_cluster_config.yml index 216a9a8c..234f63b1 100644 --- a/tasks/ssh_cluster_config.yml +++ b/tasks/ssh_cluster_config.yml @@ -1,12 +1,12 @@ --- - name: Create SSH directory for root - file: + ansible.builtin.file: path: /root/.ssh/ state: directory - mode: 0700 + mode: "0700" - name: Create root SSH key pair for PVE - user: + ansible.builtin.user: name: root generate_ssh_key: yes ssh_key_bits: 521 @@ -15,18 +15,18 @@ ssh_key_comment: "root@{{ inventory_hostname_short }}" - name: Fetch root SSH public key - slurp: + ansible.builtin.slurp: src: /root/.ssh/id_ed25519.pub register: _proxmox_root_ssh_pubkey - name: Authorize all hosts' root SSH public keys - authorized_key: + ansible.posix.authorized_key: user: root key: "{{ hostvars[item]._proxmox_root_ssh_pubkey.content | b64decode }}" with_items: "{{ groups[pve_group] }}" - name: Configure SSH clients for connecting to PVE cluster hosts - blockinfile: + ansible.builtin.blockinfile: dest: /etc/ssh/ssh_config create: yes marker: "# {mark}: PVE host configuration options (managed by ansible)." @@ -38,7 +38,7 @@ {% endfor %} - name: Allow root logins from PVE cluster hosts - blockinfile: + ansible.builtin.blockinfile: dest: /etc/ssh/sshd_config marker: "# {mark}: Allow root logins from PVE hosts (managed by ansible)." content: | @@ -80,7 +80,7 @@ - "not (_pve_known_hosts_file.stat.islnk is defined and _pve_known_hosts_file.stat.islnk)" - name: Add PVE-provided ciphers to SSH client config - lineinfile: + ansible.builtin.lineinfile: line: "Ciphers {{ pve_ssh_ciphers }}" regexp: "^Ciphers .*" insertbefore: BOF From a28f7dbaee823741c1f64809e9ee54e3f8f69dd6 Mon Sep 17 00:00:00 2001 From: lae Date: Fri, 15 Nov 2024 02:24:50 +0900 Subject: [PATCH 06/16] Update YAMLLint rules to be inline with Ansible conventions --- .yamllint.yml | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/.yamllint.yml b/.yamllint.yml index ab8face2..f536726e 100644 --- a/.yamllint.yml +++ b/.yamllint.yml @@ -1,5 +1,17 @@ --- -yaml: - rules: - line-length: - max: 120 +extends: default +rules: + line-length: + max: 120 + braces: + max-spaces-inside: 1 + level: error + brackets: + max-spaces-inside: 1 + level: error + truthy: + allowed-values: + - 'true' + - 'yes' + - 'false' + - 'no' From 6fc0c7f254e0e293c17f951456f74437657ad2a5 Mon Sep 17 00:00:00 2001 From: lae Date: Fri, 15 Nov 2024 03:43:35 +0900 Subject: [PATCH 07/16] Update YAMLLint rules to be inline with Ansible conventions (addendum) --- .ansible-lint | 1 + .yamllint.yml | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/.ansible-lint b/.ansible-lint index 0727309c..c2b1d712 100644 --- a/.ansible-lint +++ b/.ansible-lint @@ -1,2 +1,3 @@ +--- skip_list: - no-handler diff --git a/.yamllint.yml b/.yamllint.yml index f536726e..b1da5e18 100644 --- a/.yamllint.yml +++ b/.yamllint.yml @@ -9,6 +9,12 @@ rules: brackets: max-spaces-inside: 1 level: error + comments: + min-spaces-from-content: 1 + comments-indentation: false + octal-values: + forbid-implicit-octal: true + forbid-explicit-octal: true truthy: allowed-values: - 'true' From 011a84a5b0928927c3dd7114f8165695a6afcb77 Mon Sep 17 00:00:00 2001 From: lae Date: Fri, 15 Nov 2024 06:27:55 +0900 Subject: [PATCH 08/16] add dependabot configuration for github actions --- .github/dependabot.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 .github/dependabot.yml diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 00000000..4f5db38f --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,12 @@ +--- +version: 2 +updates: + - package-ecosystem: github-actions + directory: /.github/ + schedule: + interval: weekly + groups: + actions-minor: + update-types: + - minor + - patch From b9a13c3dd27d5358eac54561177cf188279c4db8 Mon Sep 17 00:00:00 2001 From: lae Date: Fri, 15 Nov 2024 06:30:06 +0900 Subject: [PATCH 09/16] [actions] bump amplify workflow to v0.2.0 --- .github/workflows/amplify.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/amplify.yml b/.github/workflows/amplify.yml index 60d74b2e..17a1013a 100644 --- a/.github/workflows/amplify.yml +++ b/.github/workflows/amplify.yml @@ -19,4 +19,4 @@ jobs: - name: Checkout uses: actions/checkout@v4 - name: Amplify Runner - uses: amplify-security/runner-action@v0.1.0 + uses: amplify-security/runner-action@926f003f3c9695a93cbc4e2f1e64eb784dcacbfc # v0.2.0 From 37ec3ea0373e8d1866302e43078d0d81f5980e17 Mon Sep 17 00:00:00 2001 From: lae Date: Fri, 15 Nov 2024 06:53:53 +0900 Subject: [PATCH 10/16] [actions] update CI to only run vagrant if role files are updated --- .github/workflows/ci.yml | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 676ac102..09607c45 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,6 +5,7 @@ name: CI pull_request: {} push: branches: ["main"] + workflow_dispatch: {} concurrency: group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.sha }} @@ -18,23 +19,46 @@ permissions: contents: read jobs: + changes: + runs-on: ubuntu-latest + outputs: + role: ${{ steps.filter.outputs.role }} + steps: + - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 + - uses: dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36 # v3.0.2 + id: filter + with: + base: ${{ github.ref }} + filters: | + role: + - 'tasks/**' + - 'handlers/**' + - 'defaults/**' + - 'vars/**' + - 'files/**' + - 'library/**' + - 'module_utils/**' + - 'Vagrantfile' vagrant-deploy: + needs: ["changes"] + if: ${{ needs.changes.outputs.role == 'true' || github.event_name == 'workflow_dispatch' }} runs-on: ubuntu-22.04 steps: - uses: actions/checkout@v4 - run: sudo apt install nfs-kernel-server - run: sudo pipx inject ansible-core jmespath netaddr - run: ansible-galaxy install geerlingguy.ntp + # yamllint disable rule:line-length - name: setup vagrant run: | # Copyright The containerd Authors - # + # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at - # + # # http://www.apache.org/licenses/LICENSE-2.0 - # + # # Unless required by applicable law or agreed to in writing, software # distributed under the License is distributed on an "AS IS" BASIS, # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -50,6 +74,7 @@ jobs: sudo apt-get build-dep -y vagrant ruby-libvirt sudo apt-get install -y --no-install-recommends libxslt-dev libxml2-dev libvirt-dev ruby-bundler ruby-dev zlib1g-dev vagrant plugin install vagrant-libvirt + # yamllint enable rule:line-length - run: > sudo -E -u ${USER} ANSIBLE_STDOUT_CALLBACK=debug From b42b4d54a291a183fa3e5011865490e2c8572e64 Mon Sep 17 00:00:00 2001 From: iPaulTech <79581474+iPaulTech@users.noreply.github.com> Date: Sat, 16 Nov 2024 21:27:00 +0100 Subject: [PATCH 11/16] Update README.md --- README.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/README.md b/README.md index 0ba46eb8..802e71dc 100644 --- a/README.md +++ b/README.md @@ -902,6 +902,17 @@ pve_default_kernel_version: 1.0.1 This creates a pin on the `proxmox-default-kernel` package, which is [the method suggested by PVE](https://pve.proxmox.com/wiki/Roadmap#Kernel_6.8). It can be later removed by unsetting this role variable. +## Troubleshooting + +### The APT installation of proxmox-ve no longer responds, Ansible aborts, the SSH session stops. +Add this section to your ``ansible.cfg``. + +```yaml +[ssh_connection] +ssh_args = -o ServerAliveInterval=20 +``` +[Reference Issue](https://github.com/lae/ansible-role-proxmox/issues/279) + ## Developer Notes When developing new features or fixing something in this role, you can test out From cf670893c9e97b153a20cbf2b1433d79161df9fa Mon Sep 17 00:00:00 2001 From: "Dimitri B." Date: Mon, 18 Nov 2024 22:49:35 +0100 Subject: [PATCH 12/16] Implement `shared` and `prune_backups` options for `proxmox_storage` module - add `prune_backups` option and all associated consistency checks, mark `maxfiles` as deprecated - add `shared` option - mark `encryption_key` and `password` options as `no_log` to prevent logging secret values to console - fixes for typos --- library/proxmox_storage.py | 124 ++++++++++++++++++++++++++++++++++--- 1 file changed, 115 insertions(+), 9 deletions(-) diff --git a/library/proxmox_storage.py b/library/proxmox_storage.py index e1df83b3..5a86a915 100755 --- a/library/proxmox_storage.py +++ b/library/proxmox_storage.py @@ -47,6 +47,12 @@ type: list description: - List of cluster node names where this storage is usable. + shared: + required: false + type: bool + description: + - Indicate that this is a single storage with the same contents on all nodes (or all listed in the O(nodes) option). + - It will not make the contents of a local storage automatically accessible to other nodes, it just marks an already shared storage as such! path: required: false description: @@ -74,6 +80,39 @@ default: 0 description: - Maximal number of backup files per VM. 0 for unlimited. + - Deprecated, use O(prune_backups) instead. Replace either by C(keep-last) or, in case C(maxfiles) was C(0) for unlimited retention, by C(keep-all). + prune_backups: + required: false + type: list + elements: dict + description: + - Specifies how to prune backups. + - The retention options are processed in the order given. Each option only covers backups within its time period. The next option does not take care of already covered backups. It will only consider older backups. + suboptions: + option: + required: true + choices: + - keep-all + - keep-last + - keep-hourly + - keep-daily + - keep-weekly + - keep-monthly + - keep-yearly + description: + - The retention option to use. + - C(keep-all): Keep all backups. This option is mutually exclusive with the other options. + - C(keep-last): Keep the last n backups. + - C(keep-hourly): Keep backups for the last n hours. If there is more than one backup for a single hour, only the latest is kept. + - C(keep-daily): Keep backups for the last n days. If there is more than one backup for a single day, only the latest is kept. + - C(keep-weekly): Keep backups for the last n weeks. If there is more than one backup for a single week, only the latest is kept. Weeks start on Monday and end on Sunday. The software uses the ISO week date-system and handles weeks at the end of the year correctly. + - C(keep-monthly): Keep backups for the last n months. If there is more than one backup for a single month, only the latest is kept. + - C(keep-yearly): Keep backups for the last n years. If there is more than one backup for a single year, only the latest is kept. + value: + required: true + description: + - The number of backups to keep. + - For C(keep-all) option, this value must be a C(bool). For all other options, this value must be an C(int). export: required: false description: @@ -114,7 +153,7 @@ subdir: required: false - specifies the folder in the share dir to use for proxmox - (useful to seperate proxmox content from other content) + (useful to separate proxmox content from other content) domain: required: false - Specifies Realm to use for NTLM/LDAPS Authentification if using @@ -131,7 +170,9 @@ type: dir path: /mydir content: [ "images", "iso", "backup" ] - maxfiles: 3 + prune_backups: + - option: keep-all + value: 1 - name: Create an RBD storage type proxmox_storage: name: ceph1 @@ -170,7 +211,7 @@ name: cephfs1 type: cephfs content: [ "snippets", "vztmpl", "iso" ] - nodes: [ "proxmox1", "proxmox2"] + nodes: [ "proxmox1", "proxmox2" ] monhost: - 10.0.0.1 - 10.0.0.2 @@ -228,6 +269,7 @@ def __init__(self, module): self.disable = module.params['disable'] self.content = module.params['content'] self.nodes = module.params['nodes'] + self.shared = module.params['shared'] self.type = module.params['type'] # Remaining PVE API arguments (depending on type) past this point self.datastore = module.params['datastore'] @@ -241,6 +283,7 @@ def __init__(self, module): self.username = module.params['username'] self.krbd = module.params['krbd'] self.maxfiles = module.params['maxfiles'] + self.prune_backups = module.params['prune_backups'] self.server = module.params['server'] self.export = module.params['export'] self.options = module.params['options'] @@ -306,6 +349,8 @@ def prepare_storage_args(self): args['content'] = 'none' if self.nodes is not None: args['nodes'] = ','.join(self.nodes) + if self.shared is not None: + args['shared'] = 1 if self.shared else 0 if self.disable is not None: args['disable'] = 1 if self.disable else 0 if self.datastore is not None: @@ -355,8 +400,44 @@ def prepare_storage_args(self): if self.share is not None: args['share'] = self.share # end cifs - if self.maxfiles is not None and 'backup' not in self.content: - self.module.fail_json(msg="maxfiles is not allowed when there is no 'backup' in content") + if self.maxfiles is not None: + self.module.warn("'maxfiles' parameter is deprecated, use 'prune_backups' parameter instead") + if "backup" not in self.content: + self.module.fail_json( + msg="'maxfiles' parameter is not allowed when there is no 'backup' in 'content' parameter" + ) + if self.prune_backups is not None: + if "backup" not in self.content: + self.module.fail_json( + msg="'prune_backups' parameter is not allowed when there is no 'backup' in 'content' parameter" + ) + + if len(self.prune_backups) != len(set(cfg["option"] for cfg in self.prune_backups)): + self.module.fail_json(msg="'prune_backups' parameter has duplicate entries") + + keep_all_entries = [cfg for cfg in self.prune_backups if cfg["option"] == "keep-all"] + keep_all_entry = keep_all_entries[0] if len(keep_all_entries) > 0 else None + other_entries = [cfg for cfg in self.prune_backups if cfg["option"] != "keep-all"] + if keep_all_entry and len(other_entries) > 0: + self.module.fail_json( + msg="'keep-all' is mutually exclusive with other options in 'prune_backups' parameter" + ) + + if keep_all_entry and type(keep_all_entry["value"]) is not bool: + self.module.fail_json(msg="value of 'keep-all' option must be a boolean in 'prune_backups' parameter") + if any(type(cfg["value"]) is not int for cfg in other_entries): + self.module.fail_json( + msg="all values except for the 'keep-all' option must be integers in 'prune_backups' parameter" + ) + + args["prune-backups"] = ( + "keep-all={value}".format(value=(1 if keep_all_entry["value"] else 0)) + if keep_all_entry + else ",".join( + map(lambda cfg: "{}={}".format(cfg["option"], cfg["value"]), other_entries) + ) + ) + if self.krbd is not None and self.type != 'rbd': self.module.fail_json(msg="krbd is only allowed with 'rbd' storage type") @@ -429,21 +510,43 @@ def main(): content=dict(type='list', required=True, aliases=['storagetype']), disable=dict(required=False, type='bool', default=False), nodes=dict(type='list', required=False, default=None), + shared=dict(type='bool', required=False, default=None), type=dict(default=None, type='str', required=True, choices=["dir", "nfs", "rbd", "lvm", "lvmthin", "cephfs", "zfspool", "btrfs", "pbs", "cifs"]), # Remaining PVE API arguments (depending on type) past this point datastore=dict(default=None, type='str', required=False), - encryption_key=dict(default=None, type='str', required=False), + encryption_key=dict(default=None, type='str', required=False, no_log=True), fingerprint=dict(default=None, type='str', required=False), master_pubkey=dict(default=None, type='str', required=False), - password=dict(default=None, type='str', required=False), + password=dict(default=None, type='str', required=False, no_log=True), path=dict(default=None, required=False, type='str'), pool=dict(default=None, type='str', required=False), monhost=dict(default=None, type='list', required=False), username=dict(default=None, type='str', required=False), krbd=dict(default=None, type='bool', required=False), maxfiles=dict(default=None, type='int', required=False), + prune_backups=dict( + default=None, + type='list', + elements='dict', + required=False, + options=dict( + option=dict( + required=True, + choices=[ + 'keep-all', + 'keep-last', + 'keep-hourly', + 'keep-daily', + 'keep-weekly', + 'keep-monthly', + 'keep-yearly', + ], + ), + value=dict(required=True, type='raw'), + ), + ), export=dict(default=None, type='str', required=False), server=dict(default=None, type='str', required=False), options=dict(default=None, type='str', required=False), @@ -474,7 +577,10 @@ def main(): ], required_by={ "master_pubkey": "encryption_key" - } + }, + mutually_exclusive=[ + ["maxfiles", "prune_backups"], + ], ) storage = ProxmoxStorage(module) @@ -499,7 +605,7 @@ def main(): error = storage.create_storage() else: # modify storage (check mode is ok) - (updated_fields,error) = storage.modify_storage() + (updated_fields, error) = storage.modify_storage() if updated_fields: result['changed'] = True From 4591a38fdec817b9b71c4d53e208d7d4cda7883e Mon Sep 17 00:00:00 2001 From: "Dimitri B." Date: Tue, 19 Nov 2024 00:39:02 +0100 Subject: [PATCH 13/16] Add missing properties in doc string --- library/proxmox_storage.py | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/library/proxmox_storage.py b/library/proxmox_storage.py index 5a86a915..f5b7e250 100755 --- a/library/proxmox_storage.py +++ b/library/proxmox_storage.py @@ -17,19 +17,24 @@ name: required: true aliases: [ "storage", "storageid" ] + type: str description: - Name of the storage. type: required: true aliases: [ "storagetype" ] + type: str choices: [ "dir", "nfs", "rbd", "lvm", "lvmthin", "cephfs", "zfspool", "btrfs" ] description: - Type of storage, must be supported by Proxmox. disable: required: false + type: bool + default: false description: Disable the storage. state: required: false + type: str default: "present" choices: [ "present", "absent" ] description: @@ -38,6 +43,7 @@ required: true aliases: [ "storagecontent" ] type: list + elements: str choices: [ "images", "rootdir", "vztmpl", "backup", "iso", "snippets" ] description: - Contents supported by the storage, not all storage @@ -45,6 +51,7 @@ nodes: required: false type: list + elements: str description: - List of cluster node names where this storage is usable. shared: @@ -55,28 +62,34 @@ - It will not make the contents of a local storage automatically accessible to other nodes, it just marks an already shared storage as such! path: required: false + type: str description: - File system path. pool: required: false + type: str description: - Ceph/ZFS pool name. monhost: required: false type: list + elements: str description: - Monitor addresses of the ceph cluster. username: required: false + type: str description: - User name (RBD) who access to ceph cluster. krbd: required: false - default: 0 + type: bool + default: false description: - Always access rbd through krbd kernel module. maxfiles: required: false + type: int default: 0 description: - Maximal number of backup files per VM. 0 for unlimited. @@ -115,49 +128,60 @@ - For C(keep-all) option, this value must be a C(bool). For all other options, this value must be an C(int). export: required: false + type: str description: - NFS export path server: required: false + type: str description: - Server IP or DNS name. options: required: false + type: str description: - NFS mount options. vgname: required: false + type: str description: - LVM volume group name. This must point to an existing volume group. thinpool: required: false + type: str description: - The name of the LVM thin pool. sparse: required: false + type: bool description: - Use ZFS thin-provisioning. is_mountpoint: required: false + type: bool description: - Specifies whether or not the given path is an externally managed mountpoint. namespace: required: false + type: str description: - Specifies the Namespace that should be used on PBS share: required: false + type: str description: - - Specifies the CIFS-Share to use + - Specifies the CIFS share to use subdir: required: false - - specifies the folder in the share dir to use for proxmox - (useful to separate proxmox content from other content) + type: str + description: + - Specifies the folder in the share dir to use for proxmox (useful to separate proxmox content from other content) domain: required: false - - Specifies Realm to use for NTLM/LDAPS Authentification if using - an AD-Enabled share + type: str + description: + - Specifies Realm to use for NTLM/LDAPS authentication if using an AD-enabled share author: - Fabien Brachere (@fbrachere) From 280eaf6cd2e91228d21aaa81c475e5bdf83a4ce4 Mon Sep 17 00:00:00 2001 From: "Dimitri B." Date: Tue, 19 Nov 2024 17:58:27 +0100 Subject: [PATCH 14/16] Move prune_backups validation into method, add comments to clarify option format, fix documentation format issues --- library/proxmox_storage.py | 83 +++++++++++++++++++++----------------- 1 file changed, 46 insertions(+), 37 deletions(-) diff --git a/library/proxmox_storage.py b/library/proxmox_storage.py index f5b7e250..5a4f5ea1 100755 --- a/library/proxmox_storage.py +++ b/library/proxmox_storage.py @@ -46,8 +46,7 @@ elements: str choices: [ "images", "rootdir", "vztmpl", "backup", "iso", "snippets" ] description: - - Contents supported by the storage, not all storage - types support all content types. + - Contents supported by the storage, not all storage types support all content types. nodes: required: false type: list @@ -114,13 +113,13 @@ - keep-yearly description: - The retention option to use. - - C(keep-all): Keep all backups. This option is mutually exclusive with the other options. - - C(keep-last): Keep the last n backups. - - C(keep-hourly): Keep backups for the last n hours. If there is more than one backup for a single hour, only the latest is kept. - - C(keep-daily): Keep backups for the last n days. If there is more than one backup for a single day, only the latest is kept. - - C(keep-weekly): Keep backups for the last n weeks. If there is more than one backup for a single week, only the latest is kept. Weeks start on Monday and end on Sunday. The software uses the ISO week date-system and handles weeks at the end of the year correctly. - - C(keep-monthly): Keep backups for the last n months. If there is more than one backup for a single month, only the latest is kept. - - C(keep-yearly): Keep backups for the last n years. If there is more than one backup for a single year, only the latest is kept. + - "C(keep-all): Keep all backups. This option is mutually exclusive with the other options." + - "C(keep-last): Keep the last n backups." + - "C(keep-hourly): Keep backups for the last n hours. If there is more than one backup for a single hour, only the latest is kept." + - "C(keep-daily): Keep backups for the last n days. If there is more than one backup for a single day, only the latest is kept." + - "C(keep-weekly): Keep backups for the last n weeks. If there is more than one backup for a single week, only the latest is kept. Weeks start on Monday and end on Sunday. The software uses the ISO week date-system and handles weeks at the end of the year correctly." + - "C(keep-monthly): Keep backups for the last n months. If there is more than one backup for a single month, only the latest is kept." + - "C(keep-yearly): Keep backups for the last n years. If there is more than one backup for a single year, only the latest is kept." value: required: true description: @@ -426,47 +425,57 @@ def prepare_storage_args(self): # end cifs if self.maxfiles is not None: self.module.warn("'maxfiles' parameter is deprecated, use 'prune_backups' parameter instead") - if "backup" not in self.content: + if 'backup' not in self.content: self.module.fail_json( msg="'maxfiles' parameter is not allowed when there is no 'backup' in 'content' parameter" ) if self.prune_backups is not None: - if "backup" not in self.content: - self.module.fail_json( - msg="'prune_backups' parameter is not allowed when there is no 'backup' in 'content' parameter" - ) - - if len(self.prune_backups) != len(set(cfg["option"] for cfg in self.prune_backups)): - self.module.fail_json(msg="'prune_backups' parameter has duplicate entries") - - keep_all_entries = [cfg for cfg in self.prune_backups if cfg["option"] == "keep-all"] - keep_all_entry = keep_all_entries[0] if len(keep_all_entries) > 0 else None - other_entries = [cfg for cfg in self.prune_backups if cfg["option"] != "keep-all"] - if keep_all_entry and len(other_entries) > 0: - self.module.fail_json( - msg="'keep-all' is mutually exclusive with other options in 'prune_backups' parameter" - ) - - if keep_all_entry and type(keep_all_entry["value"]) is not bool: - self.module.fail_json(msg="value of 'keep-all' option must be a boolean in 'prune_backups' parameter") - if any(type(cfg["value"]) is not int for cfg in other_entries): - self.module.fail_json( - msg="all values except for the 'keep-all' option must be integers in 'prune_backups' parameter" - ) - - args["prune-backups"] = ( - "keep-all={value}".format(value=(1 if keep_all_entry["value"] else 0)) + # order is important for prune_backups, hence we accept a list of options instead of a dict + keep_all_entry, other_entries = self.validate_storage_prune_backups_option() + + # the format for the prune-backups argument is (see https://pve.proxmox.com/pve-docs/api-viewer/index.html#/storage/{storage}): + # [keep-all=<1|0>][,keep-daily=][,keep-hourly=][,keep-last=][,keep-monthly=][,keep-weekly=][,keep-yearly=] + args['prune-backups'] = ( + # keep-all is mutually exclusive with the other options, we checked that earlier + # example: "keep-all=1" + 'keep-all={}'.format(1 if keep_all_entry['value'] else 0) if keep_all_entry + # example: "keep-last=3,keep-hourly=6" else ",".join( - map(lambda cfg: "{}={}".format(cfg["option"], cfg["value"]), other_entries) + map(lambda cfg: '{}={}'.format(cfg['option'], cfg['value']), other_entries) ) ) - if self.krbd is not None and self.type != 'rbd': self.module.fail_json(msg="krbd is only allowed with 'rbd' storage type") return args + def validate_storage_prune_backups_option(self): + if 'backup' not in self.content: + self.module.fail_json( + msg="'prune_backups' parameter is not allowed when there is no 'backup' in 'content' parameter" + ) + + if len(self.prune_backups) != len(set(cfg['option'] for cfg in self.prune_backups)): + self.module.fail_json(msg="'prune_backups' parameter has duplicate entries") + + keep_all_entries = [cfg for cfg in self.prune_backups if cfg['option'] == 'keep-all'] + keep_all_entry = keep_all_entries[0] if len(keep_all_entries) > 0 else None + other_entries = [cfg for cfg in self.prune_backups if cfg['option'] != 'keep-all'] + if keep_all_entry and len(other_entries) > 0: + self.module.fail_json( + msg="'keep-all' is mutually exclusive with other options in 'prune_backups' parameter" + ) + + if keep_all_entry and type(keep_all_entry['value']) is not bool: + self.module.fail_json(msg="value of 'keep-all' option must be a boolean in 'prune_backups' parameter") + if any(type(cfg['value']) is not int for cfg in other_entries): + self.module.fail_json( + msg="all values except for the 'keep-all' option must be integers in 'prune_backups' parameter" + ) + + return keep_all_entry, other_entries + def create_storage(self): new_storage = self.prepare_storage_args() try: From b6cfd6f2b66af3f89a992f2ab5b663879ebfbaff Mon Sep 17 00:00:00 2001 From: Musee Ullah Date: Wed, 4 Dec 2024 08:07:38 +0900 Subject: [PATCH 15/16] [actions] don't run Amplify on PRs from forks GitHub does not mint OIDC tokens for externally sourced PRs so this workflow can't successfully run on those PRs. An alternative solution (like via an approval comment?) should be identified and implemented eventually to allow the workflow for previous contributors using their own forks. This also updates the workflow to run on the develop branch. --- .github/workflows/amplify.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/amplify.yml b/.github/workflows/amplify.yml index 17a1013a..267a8ba8 100644 --- a/.github/workflows/amplify.yml +++ b/.github/workflows/amplify.yml @@ -4,7 +4,7 @@ on: pull_request: {} workflow_dispatch: {} push: - branches: ["main"] + branches: ["main", "develop"] permissions: contents: read @@ -14,9 +14,9 @@ jobs: amplify-security-scan: name: Amplify Security Scan runs-on: ubuntu-latest - if: (github.actor != 'dependabot[bot]') + if: (!github.event.pull_request.head.repo.fork && github.actor != 'dependabot[bot]') steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - name: Amplify Runner uses: amplify-security/runner-action@926f003f3c9695a93cbc4e2f1e64eb784dcacbfc # v0.2.0 From 11cc9fb8b0378b26c8d058223c57ee007c9fe392 Mon Sep 17 00:00:00 2001 From: lae Date: Thu, 5 Dec 2024 00:23:42 +0900 Subject: [PATCH 16/16] [actions] use pull_request_target for amplify workflow set environment to "external for forks, which is configured to require approval in the repository settings --- .github/workflows/amplify.yml | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/.github/workflows/amplify.yml b/.github/workflows/amplify.yml index 267a8ba8..97170d61 100644 --- a/.github/workflows/amplify.yml +++ b/.github/workflows/amplify.yml @@ -1,7 +1,7 @@ --- name: Amplify Security on: - pull_request: {} + pull_request_target: {} workflow_dispatch: {} push: branches: ["main", "develop"] @@ -11,12 +11,23 @@ permissions: id-token: write jobs: + authorize: + environment: + ${{ github.event_name == 'pull_request_target' && + github.event.pull_request.head.repo.fork && 'external' || 'internal' }} + runs-on: ubuntu-latest + steps: + - run: true + amplify-security-scan: name: Amplify Security Scan + needs: authorize runs-on: ubuntu-latest - if: (!github.event.pull_request.head.repo.fork && github.actor != 'dependabot[bot]') + if: github.actor != 'dependabot[bot]' steps: - name: Checkout uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + ref: ${{ github.event.pull_request.head.sha || github.ref }} - name: Amplify Runner uses: amplify-security/runner-action@926f003f3c9695a93cbc4e2f1e64eb784dcacbfc # v0.2.0