diff --git a/.github/scripts/test-metallb-deploy-condition.py b/.github/scripts/test-metallb-deploy-condition.py new file mode 100644 index 0000000..36712e1 --- /dev/null +++ b/.github/scripts/test-metallb-deploy-condition.py @@ -0,0 +1,170 @@ +#!/usr/bin/env python3 +"""Regression test for the MetalLB deploy conditions. + +The MetalLB manifest (roles/k3s_server/tasks/main.yml) and the MetalLB pool +(roles/k3s_server_post/tasks/main.yml) are included under a `when` condition +that decides whether MetalLB provides load balancing. A previous change (#683) +guarded `cilium_bgp` but accidentally skipped MetalLB whenever a non-BGP +Cilium CNI was in use (`cilium_iface` defined), breaking the cilium + MetalLB +scenario. + +This test loads the real `when` expressions from both task files and evaluates +them against representative variable sets, asserting MetalLB is deployed in +every topology except when kube-vip owns the VIP range or Cilium BGP is enabled. +""" + +from __future__ import print_function + +import os +import re +import subprocess + +import yaml +from jinja2 import Environment + +METALLB_WHEN = ( + "kube_vip_lb_ip_range is not defined and " + "not (cilium_bgp | default(false) | bool)" +) + + +def repo_root(): + return subprocess.check_output( + ["git", "rev-parse", "--show-toplevel"], text=True + ).strip() + + +def fail(message): + raise SystemExit("MetalLB deploy condition test failed: " + message) + + +def extract_when(path, task_name): + """Return the `when:` expression string for the named task.""" + with open(path, encoding="utf-8") as handle: + doc = yaml.safe_load(handle) + for task in doc: + if task.get("name") == task_name: + when = task.get("when") + return (when or "").strip() + return None + + +def evaluate(when, variables): + """Evaluate a `when` expression against variables using Jinja2.""" + env = Environment() + + def fake_bool(value): + # Minimal stand-in for Ansible's truthiness filter used by `| bool`. + if isinstance(value, bool): + return value + if value is None: + return False + return str(value).lower() in ("1", "true", "yes", "on") + + env.filters["bool"] = fake_bool + template = env.from_string("{{ " + when + " }}") + rendered = template.render(**variables) + # The expression renders to the literal strings "True"/"False". + if rendered == "True": + return True + if rendered == "False": + return False + fail("condition did not render to a boolean: {0!r}".format(rendered)) + + +def assert_deployment(when, variables, expected, label): + result = evaluate(when, variables) + verdict = "deploy" if result else "skip" + expected_verdict = "deploy" if expected else "skip" + if result != expected: + fail( + "{0}: expected to {1} MetalLB but the condition chose to {2} " + "(vars: {3})".format(label, expected_verdict, verdict, variables) + ) + + +def scenarios(): + """Yield (variables, expected_deploy, label) pairs.""" + yield ( + # Default Flannel inventory (all.yml sets cilium_bgp: false). + { + "cilium_bgp": False, + "cilium_iface": None, + }, + True, + "flannel default (cilium_bgp: false)", + ) + yield ( + # Calico CNI with no Cilium variable in scope (issue #644): cilium_bgp + # is genuinely undefined, so `default(false)` must keep MetalLB on. + { + "calico_iface": "eth1", + }, + True, + "calico, cilium_bgp undefined (#644)", + ) + yield ( + # Cilium CNI with BGP disabled: MetalLB must still be deployed. + { + "cilium_bgp": False, + "cilium_iface": "eth1", + }, + True, + "cilium non-BGP (regression catch)", + ) + yield ( + # Cilium CNI with BGP enabled: Cilium provides the LB, skip MetalLB. + { + "cilium_bgp": True, + "cilium_iface": "eth1", + }, + False, + "cilium BGP enabled", + ) + yield ( + # kube-vip is the load balancer provider: skip MetalLB. + { + "kube_vip_lb_ip_range": "192.168.30.80-192.168.30.90", + "cilium_bgp": False, + }, + False, + "kube-vip owns the VIP range", + ) + + +def main(): + root = repo_root() + server_tasks = os.path.join(root, "roles", "k3s_server", "tasks", "main.yml") + server_post_tasks = os.path.join( + root, "roles", "k3s_server_post", "tasks", "main.yml" + ) + + server_when = extract_when(server_tasks, "Deploy metallb manifest") + server_post_when = extract_when(server_post_tasks, "Deploy metallb pool") + + if server_when is None: + fail("could not find 'Deploy metallb manifest' when condition") + if server_post_when is None: + fail("could not find 'Deploy metallb pool' when condition") + + for when, source in ( + (server_when, "k3s_server/tasks/main.yml"), + (server_post_when, "k3s_server_post/tasks/main.yml"), + ): + if when != METALLB_WHEN: + fail( + "{0} when condition changed unexpectedly:\n" + " expected: {1}\n got: {2}".format(source, METALLB_WHEN, when) + ) + + for variables, expected, label in scenarios(): + assert_deployment(server_when, variables, expected, "server " + label) + assert_deployment( + server_post_when, variables, expected, "server_post " + label + ) + + print("MetalLB deploy condition regression test passed for all scenarios") + + +if __name__ == "__main__": + main() diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5dfdbd5..fe726b7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,10 +32,13 @@ concurrency: jobs: pre: + if: github.actor != 'dependabot[bot]' uses: ./.github/workflows/cache.yml needs: [lint] lint: + if: github.actor != 'dependabot[bot]' uses: ./.github/workflows/lint.yml test: + if: github.actor != 'dependabot[bot]' uses: ./.github/workflows/test.yml needs: [pre, lint] diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 9057fd8..871a131 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -84,6 +84,15 @@ repos: language: system pass_filenames: false files: ^roles/k3s_server/tasks/metallb\.yml$|^\.github/scripts/test-metallb-remote-read\.sh$ + - id: metallb-deploy-condition-test + name: MetalLB deploy condition test + entry: python3 .github/scripts/test-metallb-deploy-condition.py + language: python + additional_dependencies: + - Jinja2>=3.1 + - PyYAML + pass_filenames: false + files: ^roles/k3s_server/tasks/main\.yml$|^roles/k3s_server_post/tasks/main\.yml$|^\.github/scripts/test-metallb-deploy-condition\.py$ # noqa yaml[line-length] - id: default-interface-test name: default interface test entry: python3 .github/scripts/test-default-interface.py diff --git a/roles/k3s_server/tasks/main.yml b/roles/k3s_server/tasks/main.yml index ae406e0..b0e543f 100644 --- a/roles/k3s_server/tasks/main.yml +++ b/roles/k3s_server/tasks/main.yml @@ -27,7 +27,11 @@ - name: Deploy metallb manifest ansible.builtin.include_tasks: metallb.yml tags: metallb - when: kube_vip_lb_ip_range is not defined and (cilium_bgp is not defined or cilium_iface is not defined) + # Deploy MetalLB unless kube-vip owns the load balancer IP range, or Cilium + # BGP is enabled (Cilium then provides its own load balancing). The cilium_bgp + # default keeps this safe when Cilium variables are not in scope at all (#644) + # while still deploying MetalLB when a non-BGP Cilium CNI is in use. + when: kube_vip_lb_ip_range is not defined and not (cilium_bgp | default(false) | bool) - name: Deploy kube-vip manifest ansible.builtin.include_tasks: kube-vip.yml diff --git a/roles/k3s_server_post/defaults/main.yml b/roles/k3s_server_post/defaults/main.yml index 6ac138a..ec03c3b 100644 --- a/roles/k3s_server_post/defaults/main.yml +++ b/roles/k3s_server_post/defaults/main.yml @@ -23,6 +23,13 @@ cilium_mode: native cilium_tag: v1.20.0 cilium_cli_tag: v0.19.7 +# Shared retry/delay for remote manifest, asset downloads, and waiting on +# Kubernetes resources. The CI runner's resolver intermittently times out on +# GitHub-hosted domains and the kube API can transiently return +# ServiceUnavailable, so retry transient DNS/network/API failures. +download_retries: 5 +download_delay: 10 + cluster_cidr: 10.52.0.0/16 enable_bpf_masquerade: true kube_proxy_replacement: true diff --git a/roles/k3s_server_post/tasks/main.yml b/roles/k3s_server_post/tasks/main.yml index dbb38dc..95345b6 100644 --- a/roles/k3s_server_post/tasks/main.yml +++ b/roles/k3s_server_post/tasks/main.yml @@ -12,7 +12,11 @@ - name: Deploy metallb pool ansible.builtin.include_tasks: metallb.yml tags: metallb - when: kube_vip_lb_ip_range is not defined and (cilium_bgp is not defined or cilium_iface is not defined) + # Deploy MetalLB unless kube-vip owns the load balancer IP range, or Cilium + # BGP is enabled (Cilium then provides its own load balancing). The cilium_bgp + # default keeps this safe when Cilium variables are not in scope at all (#644) + # while still deploying MetalLB when a non-BGP Cilium CNI is in use. + when: kube_vip_lb_ip_range is not defined and not (cilium_bgp | default(false) | bool) - name: Remove tmp directory used for manifests ansible.builtin.file: diff --git a/roles/k3s_server_post/tasks/metallb.yml b/roles/k3s_server_post/tasks/metallb.yml index 63afcea..27fd780 100644 --- a/roles/k3s_server_post/tasks/metallb.yml +++ b/roles/k3s_server_post/tasks/metallb.yml @@ -54,6 +54,12 @@ {% if item.condition | default(False) -%}{{ item.condition }}{%- endif %} --timeout='{{ metal_lb_available_timeout }}' 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_wait_result + until: metallb_wait_result.rc == 0 + retries: "{{ download_retries }}" + delay: "{{ download_delay }}" run_once: true with_items: - description: controller