From 377687cbbef7da9d4ed351c0278c8b3427800331 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc=20Palac=C3=ADn?= <19353116+ProtossGP32@users.noreply.github.com> Date: Fri, 2 Oct 2026 19:34:55 +0200 Subject: [PATCH] fix(k3s_server): skip k3s-init log collection when the destination is unset (#721) * fix(k3s_server): skip k3s-init log collection when the destination is unset The guard on the k3s-init diagnostics include used a boolean default for the ANSIBLE_K3S_LOG_DIR lookup so that the when conditional would be a real boolean. That is unnecessary: `log_destination | default('') != ''` is a comparison and always evaluates to a bool, so ansible-core 2.19 never sees a non-boolean conditional here. The boolean default also defeats the guard. With ANSIBLE_K3S_LOG_DIR unset, log_destination is the bool False, and `False != ''` is True in Python because False == 0. The include therefore runs on every provisioning run, executing the journalctl, systemctl, ip, ss, iptables, service_facts, ping and wait_for diagnostics in fetch_k3s_init_logs.yml, then creating a directory literally named "False" in the operator's working directory. Use an empty-string default so the unset case renders "" and the guard skips. CI did not catch this because .github/workflows/test.yml always exports ANSIBLE_K3S_LOG_DIR. Add a regression test that parses the role, renders the real log_destination expression for both the unset and set cases, and asserts that no boolean default has been reintroduced. * chore(site): wrap the over-long kubeconfig fetch conditional --- .../scripts/test-k3s-init-log-destination.py | 112 ++++++++++++++++++ .pre-commit-config.yaml | 6 + roles/k3s_server/tasks/main.yml | 13 +- site.yml | 4 +- 4 files changed, 129 insertions(+), 6 deletions(-) create mode 100755 .github/scripts/test-k3s-init-log-destination.py diff --git a/.github/scripts/test-k3s-init-log-destination.py b/.github/scripts/test-k3s-init-log-destination.py new file mode 100755 index 0000000..0297a6e --- /dev/null +++ b/.github/scripts/test-k3s-init-log-destination.py @@ -0,0 +1,112 @@ +#!/usr/bin/env python3 +"""Assert the k3s-init log destination guard skips when unset. + +``roles/k3s_server/tasks/main.yml`` fetches k3s-init diagnostics only when +``ANSIBLE_K3S_LOG_DIR`` is set. The lookup default must be an empty string: +Jinja stringifies the boolean ``False`` to ``'False'``, which is non-empty, so +a boolean default makes the guard pass and the role creates a directory +literally named ``False`` in the operator's working directory on every run. + +This test extracts the real ``log_destination`` expression from the role and +renders it for both the unset and set cases. +""" + +from __future__ import print_function + +import os +import re +import subprocess + +import yaml +from jinja2 import Environment + +ROLE = os.path.join("roles", "k3s_server", "tasks", "main.yml") + + +def repo_root(): + return subprocess.check_output( + ["git", "rev-parse", "--show-toplevel"], text=True + ).strip() + + +def fail(message): + raise SystemExit("k3s-init log destination test failed: " + message) + + +def find_log_task(root): + """Return the 'Save logs of k3s-init.service' task from the role.""" + with open(os.path.join(root, ROLE), "r") as handle: + tasks = yaml.safe_load(handle) + + def walk(entries): + for entry in entries or []: + if not isinstance(entry, dict): + continue + if "Save logs of k3s-init.service" in str(entry.get("name", "")): + return entry + for key in ("block", "always", "rescue"): + found = walk(entry.get(key)) + if found: + return found + return None + + task = walk(tasks) + if task is None: + fail("could not find the 'Save logs of k3s-init.service' task in " + ROLE) + return task + + +def env_lookup(name, default=None, **kwargs): + """Stand-in for the env lookup: unset names fall back to the default.""" + return {"ANSIBLE_K3S_LOG_DIR": None}.get(name, default) + + +def render(task, env, value): + """Render log_destination the way Ansible folds a '>-' scalar.""" + expression = str(task["vars"]["log_destination"]).strip() + template = env.from_string(expression) + + def lookup(*args, **kwargs): + name = args[1] if len(args) > 1 else kwargs.get("name") + if name != "ANSIBLE_K3S_LOG_DIR": + return kwargs.get("default") + return kwargs["default"] if value is None else value + + return template.render(lookup=lookup).strip() + + +def main(): + root = repo_root() + task = find_log_task(root) + env = Environment() + + unset = render(task, env, None) + if unset != "": + fail( + "log_destination resolved to %r when ANSIBLE_K3S_LOG_DIR is unset; " + "it must be an empty string so the guard skips" % unset + ) + if task["when"].strip() == "": + fail("the guard on the log task is empty, so it would always run") + + set_path = render(task, env, "/var/log/k3s-init") + if set_path != "/var/log/k3s-init": + fail( + "log_destination resolved to %r when ANSIBLE_K3S_LOG_DIR is set; " + "expected the path to be passed through unchanged" % set_path + ) + + # A stray default= would leave the boolean in place even though the + # expression still renders empty, so assert the source no longer carries + # it. This is the exact regression that produced the False/ directory. + if re.search(r"default\s*=\s*False", yaml.safe_dump(task)): + fail( + "log_destination still uses a boolean default; Jinja would " + "stringify it to 'False' and create a directory named False" + ) + + print("k3s-init log destination regression test passed") + + +if __name__ == "__main__": + main() diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 3d224a3..20fdc83 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -64,6 +64,12 @@ repos: language: system pass_filenames: false files: ^roles/k3s_server/tasks/(main|join_master)\.yml$|^\.github/scripts/test-k3s-server-bootstrap\.sh$ + - id: k3s-init-log-destination-test + name: K3s init log destination test + entry: .github/scripts/test-k3s-init-log-destination.py + language: system + pass_filenames: false + files: ^roles/k3s_server/tasks/main\.yml$|^\.github/scripts/test-k3s-init-log-destination\.py$ - id: unique-hostname-precheck-test name: Unique hostname precheck test entry: .github/scripts/test-unique-hostname-precheck.sh diff --git a/roles/k3s_server/tasks/main.yml b/roles/k3s_server/tasks/main.yml index 5f6c248..916e375 100644 --- a/roles/k3s_server/tasks/main.yml +++ b/roles/k3s_server/tasks/main.yml @@ -124,13 +124,18 @@ always: - name: Save logs of k3s-init.service ansible.builtin.include_tasks: fetch_k3s_init_logs.yml - # ANSIBLE_K3S_LOG_DIR is a path string when set; evaluate it as a boolean - # so the conditional is a real boolean (ansible-core 2.19 rejects string - # conditionals derived from env vars). + # The env lookup default must be an empty string, not the boolean + # False. The `!= ''` comparison below always evaluates to a bool, so + # ansible-core 2.19 never sees a non-boolean conditional and no boolean + # default is needed. With a boolean default, an unset ANSIBLE_K3S_LOG_DIR + # leaves log_destination as the bool False, and `False != ''` is True + # because False == 0. The guard would then always pass, running the + # diagnostics on every provisioning run and creating a directory + # literally named 'False' in the operator's working directory. when: log_destination | default('') != '' vars: log_destination: >- - {{ lookup('ansible.builtin.env', 'ANSIBLE_K3S_LOG_DIR', default=False) }} + {{ lookup('ansible.builtin.env', 'ANSIBLE_K3S_LOG_DIR', default='') }} - name: Kill the temporary service used for initialization ansible.builtin.systemd: name: k3s-init diff --git a/site.yml b/site.yml index bd3e103..028330c 100644 --- a/site.yml +++ b/site.yml @@ -85,5 +85,5 @@ dest: ./kubeconfig flat: true when: >- - ansible_facts.hostname == hostvars[groups[group_name_master | default('master')][0]] - ['ansible_facts']['hostname'] + ansible_facts.hostname == + hostvars[groups[group_name_master | default('master')][0]]['ansible_facts']['hostname']