From 4ed66807a039e343bea17796540adb3b8f39f402 Mon Sep 17 00:00:00 2001 From: Clark Boylan Date: Sat, 1 Feb 2020 14:40:51 -0800 Subject: [PATCH] Use unique loop vars to avoid conflicts We have to be careful about avoiding outer loop loop_var conflicts in ansible. Because the zuul-jobs roles are meant to be reconsumed elsewhere we should not use 'item' loopvars and instead set them to something a bit more unique. We use a zj_ prefix to try and be unique to this repo and document this convention. Change-Id: I20b9327a914890e9eafcb2b36dc8c23fb472bc8f --- doc/source/policy.rst | 24 +++++++++++++++++++ roles/collect-container-logs/tasks/main.yaml | 4 ++-- roles/promote-docker-image/tasks/main.yaml | 6 +++-- .../tasks/main.yaml | 8 ++++--- roles/run-buildset-registry/tasks/main.yaml | 4 +++- roles/upload-docker-image/tasks/main.yaml | 6 +++-- roles/upload-docker-image/tasks/push.yaml | 4 ++-- roles/validate-zone-db/tasks/find.yaml | 4 +++- roles/validate-zone-db/tasks/main.yaml | 4 +++- 9 files changed, 50 insertions(+), 14 deletions(-) diff --git a/doc/source/policy.rst b/doc/source/policy.rst index 560658aec..d19148ede 100644 --- a/doc/source/policy.rst +++ b/doc/source/policy.rst @@ -153,6 +153,30 @@ If relevant, the specific steps where the privilege escalation occurs should be documented so that they can be reproduced when configuring hosts. If possible, they should be grouped in a separate playbook that can be applied to hosts manually. +Ansible Loops in Roles +********************** + +Because the Ansible roles contained in this repo are expected to be +pretty universally applicable in Zuul systems we must write them +defensively to work around some Ansible behaviors. In particular +nesting Ansible loops using the default `loop_var` of `item` is not +safe. + +Roles in this repo should override the default `loop_var` in loops +and use a variable name prefixed with `zj_` to make them more unique. +The idea is this will avoid conflicts with the calling level which +may use `include_role` in a loop creating a `loop_var` conflict. + +For example:: + + command: echo {{ zj_number }} + loop: + - one + - two + - three + loop_control: + loop_var: zj_number + Installing Dependencies in Roles ******************************** diff --git a/roles/collect-container-logs/tasks/main.yaml b/roles/collect-container-logs/tasks/main.yaml index 65f2b4a23..beb658efd 100644 --- a/roles/collect-container-logs/tasks/main.yaml +++ b/roles/collect-container-logs/tasks/main.yaml @@ -13,8 +13,8 @@ # We can't use the default 'item' because roles may be used in # higher level loops and 'item' could conflict in that case. loop_control: - loop_var: loop_container_name - shell: "{{ container_command }} logs {{ loop_container_name }} &> {{ ansible_user_dir }}/zuul-output/logs/{{ container_command }}/{{ loop_container_name }}.txt" + loop_var: zj_container_name + shell: "{{ container_command }} logs {{ zj_container_name }} &> {{ ansible_user_dir }}/zuul-output/logs/{{ container_command }}/{{ zj_container_name }}.txt" args: executable: /bin/bash ignore_errors: true diff --git a/roles/promote-docker-image/tasks/main.yaml b/roles/promote-docker-image/tasks/main.yaml index 9f1d030e4..4116f3f52 100644 --- a/roles/promote-docker-image/tasks/main.yaml +++ b/roles/promote-docker-image/tasks/main.yaml @@ -1,10 +1,12 @@ - name: Verify repository names when: | docker_credentials.repository is defined - and not item.repository | regex_search(docker_credentials.repository) + and not zj_image.repository | regex_search(docker_credentials.repository) loop: "{{ docker_images }}" + loop_control: + loop_var: zj_image fail: - msg: "{{ item.repository }} not permitted by {{ docker_credentials.repository }}" + msg: "{{ zj_image.repository }} not permitted by {{ docker_credentials.repository }}" # This is used by the delete tasks - name: Get dockerhub JWT token no_log: true diff --git a/roles/pull-from-intermediate-registry/tasks/main.yaml b/roles/pull-from-intermediate-registry/tasks/main.yaml index 56a5cf9e5..236bba19c 100644 --- a/roles/pull-from-intermediate-registry/tasks/main.yaml +++ b/roles/pull-from-intermediate-registry/tasks/main.yaml @@ -84,13 +84,15 @@ - name: Pull artifacts from intermediate registry command: >- skopeo --insecure-policy copy - {{ item.url }} - docker://127.0.0.1:{{ socat_port }}/{{ item.metadata.repository | regex_replace('^docker\.io/(.*)', '\1') }}:{{ item.metadata.tag }} + {{ zj_zuul_artifact.url }} + docker://127.0.0.1:{{ socat_port }}/{{ zj_zuul_artifact.metadata.repository | regex_replace('^docker\.io/(.*)', '\1') }}:{{ zj_zuul_artifact.metadata.tag }} retries: 3 register: result until: result is success - when: "'metadata' in item and item.metadata.type | default('') == 'container_image'" + when: "'metadata' in zj_zuul_artifact and zj_zuul_artifact.metadata.type | default('') == 'container_image'" loop: "{{ zuul.artifacts | default([]) }}" + loop_control: + loop_var: zj_zuul_artifact always: - name: Remove docker user config command: "shred ~/.docker/config.json" diff --git a/roles/run-buildset-registry/tasks/main.yaml b/roles/run-buildset-registry/tasks/main.yaml index 853f80d45..11504c4a1 100644 --- a/roles/run-buildset-registry/tasks/main.yaml +++ b/roles/run-buildset-registry/tasks/main.yaml @@ -19,10 +19,12 @@ - name: Ensure registry volume directories exists file: state: directory - path: "{{ buildset_registry_root }}/{{ item }}" + path: "{{ buildset_registry_root }}/{{ zj_dir }}" loop: - tls - conf + loop_control: + loop_var: zj_dir - name: Generate registry secrets set_fact: registry_password: "{{ lookup('password', '/dev/null') }}" diff --git a/roles/upload-docker-image/tasks/main.yaml b/roles/upload-docker-image/tasks/main.yaml index 15490903d..5052e3cae 100644 --- a/roles/upload-docker-image/tasks/main.yaml +++ b/roles/upload-docker-image/tasks/main.yaml @@ -1,10 +1,12 @@ - name: Verify repository names when: | docker_credentials.repository is defined - and not item.repository | regex_search(docker_credentials.repository) + and not zj_image.repository | regex_search(docker_credentials.repository) loop: "{{ docker_images }}" + loop_control: + loop_var: zj_image fail: - msg: "{{ item.repository }} not permitted by {{ docker_credentials.repository }}" + msg: "{{ zj_image.repository }} not permitted by {{ docker_credentials.repository }}" - name: Log in to dockerhub command: "docker login -u {{ docker_credentials.username }} -p {{ docker_credentials.password }}" no_log: true diff --git a/roles/upload-docker-image/tasks/push.yaml b/roles/upload-docker-image/tasks/push.yaml index d47c9c91b..150782ab9 100644 --- a/roles/upload-docker-image/tasks/push.yaml +++ b/roles/upload-docker-image/tasks/push.yaml @@ -1,8 +1,8 @@ - name: Upload tag to dockerhub - command: "docker push {{ image.repository }}:change_{{ zuul.change }}_{{ image_tag }}" + command: "docker push {{ image.repository }}:change_{{ zuul.change }}_{{ zj_image_tag }}" loop: "{{ image.tags | default(['latest']) }}" loop_control: - loop_var: image_tag + loop_var: zj_image_tag register: result until: result.rc == 0 retries: 3 diff --git a/roles/validate-zone-db/tasks/find.yaml b/roles/validate-zone-db/tasks/find.yaml index e2092cdba..10d043856 100644 --- a/roles/validate-zone-db/tasks/find.yaml +++ b/roles/validate-zone-db/tasks/find.yaml @@ -8,5 +8,7 @@ - name: Build zone.db file list set_fact: - zone_db_files: '{{ zone_db_files + [ [item.path.split("/")[-2], item.path] ] }}' + zone_db_files: '{{ zone_db_files + [ [zj_zone_db_found_file.path.split("/")[-2], zj_zone_db_found_file.path] ] }}' loop: "{{ zone_db_found_files['files'] }}" + loop_control: + loop_var: zj_zone_db_found_file diff --git a/roles/validate-zone-db/tasks/main.yaml b/roles/validate-zone-db/tasks/main.yaml index 8d11b34c5..4b3c17613 100644 --- a/roles/validate-zone-db/tasks/main.yaml +++ b/roles/validate-zone-db/tasks/main.yaml @@ -9,5 +9,7 @@ when: not zone_db_files - name: 'Run checkzone' - command: '/usr/sbin/named-checkzone {{ item[0] }} {{ item[1] }}' + command: '/usr/sbin/named-checkzone {{ zj_zone_db_file[0] }} {{ zj_zone_db_file[1] }}' loop: "{{ zone_db_files }}" + loop_control: + loop_var: zj_zone_db_file