From 4dc333a7d354d520334166636840ad617751242b Mon Sep 17 00:00:00 2001 From: Techno Tim Date: Fri, 7 Aug 2026 22:36:02 -0500 Subject: [PATCH] fix(metallb): verify the metallb-system namespace actually exists (#702) * fix(metallb): verify the metallb-system namespace actually exists - change the Test metallb-system namespace task to run k3s kubectl get namespace metallb-system instead of the bare -n metallb-system, which printed kubectl usage and always exited 0 - add a regression test that asserts the task uses an explicit get and fails if it ever regresses to the usage-only form - wire the new test into pre-commit * fix(metallb): retry the namespace check on transient kube API errors - retry the explicit get namespace check until it succeeds - extend the regression test to assert the retry wiring * fix(metallb): retry the webhook endpoint check on transient kube API errors - retry the webhook-service endpoint get until it succeeds - extend the regression test to cover the webhook task too --- .github/scripts/test-metallb-namespace.py | 108 ++++++++++++++++++++++ .pre-commit-config.yaml | 8 ++ roles/k3s_server_post/tasks/metallb.yml | 14 ++- 3 files changed, 129 insertions(+), 1 deletion(-) create mode 100644 .github/scripts/test-metallb-namespace.py diff --git a/.github/scripts/test-metallb-namespace.py b/.github/scripts/test-metallb-namespace.py new file mode 100644 index 00000000..77aa7cdf --- /dev/null +++ b/.github/scripts/test-metallb-namespace.py @@ -0,0 +1,108 @@ +#!/usr/bin/env python3 +"""Regression test for the MetalLB converge checks. + +The MetalLB tasks in roles/k3s_server_post/tasks/metallb.yml must actually +verify resources through an explicit kubectl get, and must retry on a +transient kube API error while MetalLB converges. + +The "Test metallb-system namespace" task previously ran `k3s kubectl -n +metallb-system` with no subcommand, which only printed a usage page and always +exited 0, so it always succeeded even when the namespace did not exist (issue +#350). It must instead run an explicit `get namespace metallb-system`, which +returns non-zero when the namespace is absent. + +An explicit get actually contacts the API server, so these tasks need the same +retry wiring as their siblings (register, until rc == 0, retries, delay). A +bare get with no retry would otherwise abort the converge play on a transient +kube API error while MetalLB converges. +""" + +from __future__ import print_function + +import os +import subprocess + +import yaml + + +def repo_root(): + return subprocess.check_output( + ["git", "rev-parse", "--show-toplevel"], text=True + ).strip() + + +def fail(message): + raise SystemExit("MetalLB namespace test failed: " + message) + + +def find_task(tasks, name): + for entry in tasks: + if entry.get("name") == name: + return entry + fail("could not find the '{0}' task".format(name)) + return None + + +def command_text(task): + cmd = task.get("ansible.builtin.command") + if not cmd: + cmd = task.get("command") + if not cmd: + fail("task does not use ansible.builtin.command") + return cmd if isinstance(cmd, str) else " ".join(cmd) + + +def check_explicit_get(task, name, needle): + text = command_text(task) + if needle not in text: + fail( + "command does not run '{0}'; the task would only print usage and " + "never verify the resource (got: {1!r})".format(needle, text) + ) + + +def check_retry_wiring(task, name): + # The sibling k3s_server_post metallb tasks retry kubectl because the kube + # API can briefly be unavailable while MetalLB converges. Without the same + # retry, a transient API error aborts the whole converge play. + if not task.get("register"): + fail( + "{0} does not register a result; without retry wiring a transient " + "kube API error aborts the converge play".format(name) + ) + if not isinstance(task.get("until"), str) or "rc == 0" not in task["until"]: + fail( + "{0} does not retry on rc == 0; the kube API can transiently fail " + "while MetalLB converges and abort the play".format(name) + ) + if task.get("retries") is None: + fail("{0} is missing retries".format(name)) + if task.get("delay") is None: + fail("{0} is missing delay".format(name)) + + +def main(): + task_file = os.path.join( + repo_root(), "roles", "k3s_server_post", "tasks", "metallb.yml" + ) + with open(task_file, encoding="utf-8") as handle: + tasks = yaml.safe_load(handle) + + namespace_task = find_task(tasks, "Test metallb-system namespace") + # A bare `-n metallb-system` with no subcommand prints kubectl usage and + # always exits 0, so it never proves the namespace exists. The fix must + # use an explicit get. + check_explicit_get(namespace_task, "Test metallb-system namespace", + "get namespace metallb-system") + check_retry_wiring(namespace_task, "Test metallb-system namespace") + + webhook_task = find_task(tasks, "Test metallb-system webhook-service endpoint") + check_explicit_get(webhook_task, "Test metallb-system webhook-service endpoint", + "get endpoints") + check_retry_wiring(webhook_task, "Test metallb-system webhook-service endpoint") + + print("MetalLB namespace check regression test passed") + + +if __name__ == "__main__": + main() diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 4858b556..9f7421b9 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -113,6 +113,14 @@ repos: - Jinja2>=3.1 pass_filenames: false files: ^roles/k3s_server_post/templates/metallb\.crs\.j2$|^\.github/scripts/test-metallb-interfaces\.py$ + - id: metallb-namespace-test + name: MetalLB namespace test + entry: python3 .github/scripts/test-metallb-namespace.py + language: python + additional_dependencies: + - PyYAML + pass_filenames: false + files: ^roles/k3s_server_post/tasks/metallb\.yml$|^\.github/scripts/test-metallb-namespace\.py$ - id: metallb-deploy-condition-test name: MetalLB deploy condition test entry: python3 .github/scripts/test-metallb-deploy-condition.py diff --git a/roles/k3s_server_post/tasks/metallb.yml b/roles/k3s_server_post/tasks/metallb.yml index 5e814c15..e06c9069 100644 --- a/roles/k3s_server_post/tasks/metallb.yml +++ b/roles/k3s_server_post/tasks/metallb.yml @@ -40,8 +40,14 @@ - name: Test metallb-system namespace ansible.builtin.command: >- - {{ k3s_kubectl_binary | default('k3s kubectl') }} -n metallb-system + {{ k3s_kubectl_binary | default('k3s kubectl') }} get namespace metallb-system changed_when: false + # The kube API can briefly return ServiceUnavailable while MetalLB converges, + # which would otherwise abort the whole converge play on a transient error. + register: metallb_namespace_result + until: metallb_namespace_result.rc == 0 + retries: "{{ download_retries }}" + delay: "{{ download_delay }}" with_items: "{{ groups[group_name_master | default('master')] }}" run_once: true @@ -99,6 +105,12 @@ ansible.builtin.command: >- {{ k3s_kubectl_binary | default('k3s kubectl') }} -n metallb-system get endpoints {{ metallb_webhook_service_name }} changed_when: false + # The kube API can briefly return ServiceUnavailable while MetalLB converges, + # which would otherwise abort the whole converge play on a transient error. + register: metallb_webhook_result + until: metallb_webhook_result.rc == 0 + retries: "{{ download_retries }}" + delay: "{{ download_delay }}" with_items: "{{ groups[group_name_master | default('master')] }}" run_once: true