feat(stalwart): spread each node group over nodes and give each one a PDB
Publish Helm Chart with patched Application Version / publish-helm-chart (push) Successful in 7s
Publish Stalwart Helm Chart / Publish Stalwart Helm Chart (push) Successful in 7s
SonarQube Analysis / SonarQube Analysis (push) Successful in 27s

A two-replica node group was redundancy on paper: nothing kept both pods off
one node, and no PodDisruptionBudget kept a drain from taking both at once.
Both objects are now rendered per node group, gated on that group's COMPUTED
replica count being >= 2.

The distribution is a promise, not an effort: maxSkew 1 on
kubernetes.io/hostname with whenUnsatisfiable DoNotSchedule, selecting only
this group's own pods, so an "mx" and a "full" pod may share a node. The
selector comes from the chart (stalwart.groupLabels), never from the operator -
a spread constraint whose selector matches no pod is trivially satisfied,
distributes nothing and reports nothing.

nodeAffinityPolicy: Ignore is set explicitly, and that is the load-bearing
detail. Its API default is Honor, under which a narrowing nodeSelector shrinks
the set of counted domains until both replicas fit into one: skew 0, both pods
co-located, no Pending, no event, and a selector that is correct - so no check
can see it. With Ignore the same narrowing gives skew 2 and the second pod is
Pending. A render abort was the alternative and would also have forbidden a
nodeSelector spanning five nodes, which is legal.

A user entry colliding with the chart's own on (topologyKey,
whenUnsatisfiable) aborts the render naming both values, because the API
rejects the duplicate with a message about a list index and an apply-based path
merges the two over patchMergeKey: topologyKey silently, dropping one selector.

Minor rather than major, proved with a diff rather than argued: with the
shipped defaults (full: 1, mx: 0) and with ci/stalwart/test-values.yaml,
helm template renders byte-for-byte what 1.0.1+upv0.16.24 rendered - both
diffs empty.

New in CI, over every chart on every PR: ci/run-selector-match-test.sh holds
every rendered PodDisruptionBudget selector and every topologySpreadConstraints
labelSelector against the labels of the pod templates in the same render. Helm
itself is the YAML parser (ci/yaml2json), because the runners have neither
pyyaml nor yq and a hand-written parser could read nothing and report nothing.
podAffinity/podAntiAffinity are out of scope; matchExpressions-only selectors
are reported as not checked rather than counted as passing. Two anti-vacuum
guards: a chart with an overlay under ci/selector-match/ that renders no
selector fails, and a run that checks no selector at all fails.

Fixes #96
This commit was merged in pull request #97.
This commit is contained in:
jklein authored and jklein committed 2026-10-07 19:36:04 +00:00
1 parent 1698fc0aeb
commit 6ade74db80
14 files changed
+1125 -8

No files matched your search

+461
View File
@@ -0,0 +1,461 @@
#!/usr/bin/env bash
# Selector-match render test: every label selector a chart renders is held
# against the labels of the pod templates in the SAME render.
#
# Usage: ci/run-selector-match-test.sh [chart ...] (no argument = all charts)
#
# WHAT IT GUARDS AGAINST, AND WHY A GREEN ROLLOUT DOES NOT (Gitea #96)
#
# Two kinds of object in this repository select pods by label, and BOTH of them
# fail silently when the selector matches nothing:
#
# topologySpreadConstraints - a constraint whose labelSelector matches no pod
# is TRIVIALLY SATISFIED. It is a condition over zero pods, so every
# placement meets it: nothing is distributed, the rollout is green, no event
# is emitted and no log line appears. With whenUnsatisfiable: DoNotSchedule
# this is worse rather than better, because the object then reads like a
# guarantee that two replicas sit on different nodes.
# PodDisruptionBudget - a budget whose selector matches no pod does its
# arithmetic over zero pods and never blocks an eviction. `kubectl drain`
# takes the whole group at once while the object sits in the namespace
# looking like protection.
#
# Neither is a render error, neither is an install-test failure, and neither is
# visible in a diff: the chart that breaks it is usually a chart that RENAMED a
# label somewhere else. Charts keep their selector and their pod labels in one
# helper for that reason (stalwart.groupLabels, keycloak's app label) - but one
# definition only helps while nobody writes a second one, and that is the part a
# test can hold.
#
# Over ALL charts and not the changed ones, for the same reason as
# ci/run-quantity-render-test.sh: the mistake arrives by copy into a new chart,
# whose own diff nobody reads for it, and running over everything means there is
# no list to maintain - adding a chart enrols it.
#
# HOW THE YAML IS PARSED, AND WHY NOT WITH A PARSER WRITTEN HERE
#
# The runners have neither pyyaml nor yq - measured, the same constraint
# ci/run-trivy-config.sh and ci/run-quantity-render-test.sh work under. Writing
# a YAML reader in awk or sed was the obvious alternative and is the wrong one:
# its failure mode is a parser that quietly reads nothing, finds no selector,
# reports no mismatch, and leaves this test green for a chart it never looked
# at - the exact class of signal this repository removes rather than adds.
#
# So HELM is the parser, which is a YAML implementation that is already
# installed, already trusted, and already the one that produced the manifests.
# ci/yaml2json is a one-template chart that prints its values back as JSON; the
# rendered manifest stream is handed to it as a VALUES FILE and comes back as
# JSON that python3 reads with its standard library. What IS done textually is
# only the document split, on a line that is exactly "---" in column one, and
# the consequence of getting that wrong is not a false pass: the fragment stops
# being valid YAML and helm exits 1.
#
# WHAT IS DELIBERATELY NOT CHECKED
#
# - podAffinity / podAntiAffinity. They carry labelSelectors too, but their
# semantics are "relative to pods that exist", and an anti-affinity term
# matching nothing is a legitimate configuration rather than a mistake.
# Out of scope, and named so that the gap is not mistaken for coverage.
# - selectors expressed ONLY through matchExpressions. Evaluating a set-based
# requirement means reimplementing label-selector semantics, which is a
# second parser by another name. Those are REPORTED as "not checked" and
# are never counted as passing, because a green tick for a code path nobody
# entered is worse than an honest gap (README, "Two rules for every check
# added here").
#
# AND THE ANTI-VACUUM GUARDS, because this test can be green over nothing. A
# chart renders a PDB or a spread constraint only above a replica threshold, and
# every chart's CI fixtures sit below it. Two rules therefore apply:
#
# 1. ci/selector-match/<chart>.yaml is an overlay applied last, whose only job
# is to push a chart over that threshold. A chart that HAS one and still
# renders no selector FAILS - the overlay exists for exactly that, so an
# overlay that stopped working is a finding and not a shrug.
# 2. If no selector at all was checked across the whole run, the run fails.
set -euo pipefail
ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
CI_DIR="$ROOT/ci"
YAML2JSON="$CI_DIR/yaml2json"
OVERLAY_DIR="$CI_DIR/selector-match"
RELEASE="selector"
WORK="$(mktemp -d)"
trap 'rm -rf "$WORK"' EXIT
# --- Which charts -----------------------------------------------------------
CHARTS=()
if [ "$#" -gt 0 ]; then
CHARTS=("$@")
else
for DIR in "$ROOT"/*/; do
[ -f "$DIR/Chart.yaml" ] || continue
CHARTS+=("$(basename "$DIR")")
done
fi
if [ "${#CHARTS[@]}" -eq 0 ]; then
echo "ERROR: no chart directories found under $ROOT" >&2
exit 1
fi
FAILED=""
CHECKED=""
NOT_EXERCISED=""
WITH_GAPS=""
TOTAL_SELECTORS=0
SUMMARY_ROWS=""
for CHART in "${CHARTS[@]}"; do
if [ ! -f "$ROOT/$CHART/Chart.yaml" ]; then
echo "ERROR: '$CHART' is not a chart directory" >&2
exit 1
fi
# Same reason as in ci/run-quantity-render-test.sh: a chart with a declared
# dependency cannot be rendered at all while charts/ is empty, and this job
# runs over EVERY chart on EVERY pull request - so without this line a PR
# touching nothing but a README would go red on the charts that have one.
"$ROOT/ci/helm-dependencies.sh" "$ROOT/$CHART"
# Values: the same precedence the other render tests use, so a chart's CI
# values are maintained in one place and satisfy its required() values.
VALUES_ARGS=()
VALUES_LABEL="chart defaults"
if [ -f "$CI_DIR/$CHART/test-values.yaml" ]; then
VALUES_ARGS=(-f "$CI_DIR/$CHART/test-values.yaml")
VALUES_LABEL="ci/$CHART/test-values.yaml"
elif [ -f "$CI_DIR/trivy/values/$CHART.yaml" ]; then
VALUES_ARGS=(-f "$CI_DIR/trivy/values/$CHART.yaml")
VALUES_LABEL="ci/trivy/values/$CHART.yaml"
fi
HAS_OVERLAY=""
if [ -f "$OVERLAY_DIR/$CHART.yaml" ]; then
# Last, so it wins over the install-test values.
VALUES_ARGS+=(-f "$OVERLAY_DIR/$CHART.yaml")
VALUES_LABEL="$VALUES_LABEL + ci/selector-match/$CHART.yaml"
HAS_OVERLAY=1
fi
echo "::group::$CHART ($VALUES_LABEL)"
RENDER="$WORK/$CHART.manifest.yaml"
if ! helm template "$RELEASE" "$ROOT/$CHART" \
${VALUES_ARGS[@]+"${VALUES_ARGS[@]}"} >"$RENDER" 2>"$WORK/err"; then
sed 's/^/ /' "$WORK/err"
echo "::endgroup::"
echo "::error title=selector-match $CHART::$CHART does not render with its CI values, so no selector could be checked. Reproduce: helm template $RELEASE $CHART $(printf '%s ' "${VALUES_ARGS[@]+${VALUES_ARGS[@]}}")"
FAILED="$FAILED $CHART"
continue
fi
# --- The document split, and the only textual step in this script ----------
# One YAML sequence item per rendered document, content indented by four and
# the item marker on a line of its own - so a document whose first line is a
# comment ("# Source: ...") needs no special case. A wrong split does not
# produce a false pass: the result stops being parsable and helm exits 1.
DOCS="$WORK/$CHART.docs.yaml"
awk '
BEGIN { print "docs:"; started = 0 }
/^---[ \t]*$/ { print " -"; started = 1; next }
{
if (!started) { print " -"; started = 1 }
if (length($0) == 0) print ""; else print " " $0
}
' "$RENDER" >"$DOCS"
JSON="$WORK/$CHART.json"
if ! helm template y2j "$YAML2JSON" -f "$DOCS" 2>"$WORK/err" \
| awk '/^\{/ { print; exit }' >"$JSON"; then
sed 's/^/ /' "$WORK/err"
echo "::endgroup::"
echo "::error title=selector-match $CHART::the rendered manifests of $CHART could not be re-read as YAML by ci/yaml2json. That is a parse failure, not a selector finding: either the render contains a document this script's '---' split cut in the wrong place, or the manifest is not valid YAML. Helm's own message is in the job log."
FAILED="$FAILED $CHART"
continue
fi
if [ ! -s "$JSON" ]; then
echo "::endgroup::"
echo "::error title=selector-match $CHART::ci/yaml2json produced no JSON for $CHART - the manifests were not re-read, so nothing was checked."
FAILED="$FAILED $CHART"
continue
fi
# --- The comparison --------------------------------------------------------
# Inline, as in ci/run-quantity-render-test.sh, and on the standard library
# only - json, because the YAML is already gone by this point. It writes one
# "VERDICT|detail" line per selector and never decides the exit code itself:
# the loop below does, so a verdict can never be lost to a swallowed status.
set +e
python3 - "$CHART" "$JSON" >"$WORK/$CHART.out" 2>"$WORK/$CHART.pyerr" <<'PY'
import json, sys
chart, path = sys.argv[1], sys.argv[2]
docs = json.load(open(path)).get("docs") or []
# A pod template is any value of a key named "template" that carries a spec with
# containers. Found by walking rather than by enumerating kinds, so a CronJob's
# spec.jobTemplate.spec.template is covered without a special case, and so is
# whatever workload kind a chart adds next.
def pod_templates(node, path="$"):
out = []
if isinstance(node, dict):
for key, value in node.items():
here = "%s.%s" % (path, key)
if (key == "template" and isinstance(value, dict)
and isinstance(value.get("spec"), dict)
and "containers" in value["spec"]):
out.append((here, value))
out.extend(pod_templates(value, here))
elif isinstance(node, list):
for i, value in enumerate(node):
out.extend(pod_templates(value, "%s[%d]" % (path, i)))
return out
def labels_of(template):
meta = template.get("metadata")
labels = meta.get("labels") if isinstance(meta, dict) else None
return labels if isinstance(labels, dict) else {}
def name_of(doc):
meta = doc.get("metadata")
if isinstance(meta, dict) and meta.get("name"):
return str(meta["name"])
return "<unnamed %s>" % doc.get("kind", "object")
def matches(selector, labels):
"""matchLabels only. Every key must be present with the same value; the
comparison is on the string form, because YAML 'true' and the string
"true" are the same label value to Kubernetes."""
for key, value in selector.items():
if key not in labels or str(labels[key]) != str(value):
return False
return True
def classify(selector, where):
"""(matchLabels dict, None) when it can be evaluated, (None, reason) when
it cannot. A selector this script cannot evaluate is REPORTED, never
counted as a pass."""
if selector is None:
return None, ("%s has NO selector at all. For a topology spread "
"constraint that means it matches no pod and is "
"therefore vacuously satisfied; for a PodDisruptionBudget "
"it is rejected by the API server" % where)
if not isinstance(selector, dict):
return None, "%s selector is not a mapping (%r)" % (where, selector)
match_labels = selector.get("matchLabels")
if isinstance(match_labels, dict) and match_labels:
if selector.get("matchExpressions"):
# matchLabels AND matchExpressions: the matchLabels half is still
# a necessary condition, so checking it is sound - it just is not
# the whole selector. Said out loud rather than passed off as full
# coverage.
return match_labels, None
return match_labels, None
if selector.get("matchExpressions"):
return None, ("%s selects only through matchExpressions, which this "
"test does not evaluate - doing so would mean "
"reimplementing label-selector semantics, i.e. a second "
"parser. Reported, not counted as passing" % where)
if match_labels == {} or selector == {}:
return {}, None # the empty selector matches every pod
return None, "%s selector has neither matchLabels nor matchExpressions" % where
# Rooted at "<Kind>/<name>" rather than at "$", because a chart with one
# workload per node group renders several documents whose internal path is
# identical - and then every finding would name the same spot.
all_templates = []
for doc in docs:
if isinstance(doc, dict):
root = "%s/%s" % (doc.get("kind", "?"), name_of(doc))
all_templates.extend(pod_templates(doc, root))
lines = []
# 1. Every topologySpreadConstraints entry, against ITS OWN pod template. That
# is the strict and the correct comparison: the failure being guarded
# against is a constraint that does not count the very pods it sits on.
for where, template in all_templates:
spread = template.get("spec", {}).get("topologySpreadConstraints")
if not isinstance(spread, list):
continue
labels = labels_of(template)
for i, constraint in enumerate(spread):
if not isinstance(constraint, dict):
lines.append("FAIL|%s: topologySpreadConstraints[%d] at %s is not a mapping"
% (chart, i, where))
continue
key = constraint.get("topologyKey", "<none>")
tag = "spread[%d] on %s at %s" % (i, key, where)
match_labels, reason = classify(constraint.get("labelSelector"), tag)
if match_labels is None:
if constraint.get("labelSelector") is None:
lines.append("FAIL|%s: %s" % (chart, reason))
else:
lines.append("SKIP|%s: %s" % (chart, reason))
continue
if matches(match_labels, labels):
lines.append("OK|%s: %s matches its own pod template's labels (%s)"
% (chart, tag, ", ".join("%s=%s" % kv for kv in sorted(match_labels.items())) or "empty selector, matches every pod"))
else:
lines.append("FAIL|%s: %s selects %s but the pod template it is "
"attached to carries %s - the constraint therefore "
"matches NONE of its own pods, is trivially satisfied, "
"and distributes nothing while the rollout stays green. "
"Render the chart and compare the selector with the pod "
"template's labels; the usual cause is a label renamed "
"in one of the two places"
% (chart, tag,
", ".join("%s=%s" % kv for kv in sorted(match_labels.items())),
", ".join("%s=%s" % kv for kv in sorted(labels.items())) or "no labels at all"))
# 2. Every PodDisruptionBudget, against ALL pod templates of the render: a
# budget has no owning template, it only has to select some pod that exists.
pdbs = [d for d in docs if isinstance(d, dict) and d.get("kind") == "PodDisruptionBudget"]
for doc in pdbs:
tag = "PodDisruptionBudget %s" % name_of(doc)
spec = doc.get("spec") if isinstance(doc.get("spec"), dict) else {}
match_labels, reason = classify(spec.get("selector"), tag)
if match_labels is None:
lines.append(("FAIL|%s: %s" if spec.get("selector") is None else "SKIP|%s: %s")
% (chart, reason))
continue
if not all_templates:
lines.append("FAIL|%s: %s is rendered but this render contains no pod "
"template at all, so its selector cannot match anything"
% (chart, tag))
continue
hits = [w for w, t in all_templates if matches(match_labels, labels_of(t))]
if hits:
lines.append("OK|%s: %s matches %d pod template(s): %s"
% (chart, tag, len(hits), ", ".join(hits)))
else:
known = sorted({"%s=%s" % kv for _, t in all_templates for kv in labels_of(t).items()})
lines.append("FAIL|%s: %s selects %s, which matches NONE of the %d pod "
"template(s) in the same render. A PDB that matches no pod "
"never blocks an eviction, so a node drain takes every "
"replica at once while the object looks like protection. "
"Labels actually present on the pod templates: %s"
% (chart, tag,
", ".join("%s=%s" % kv for kv in sorted(match_labels.items())),
len(all_templates), "; ".join(known) or "none"))
for line in lines:
print(line)
PY
RC=$?
set -e
if [ "$RC" -gt 1 ]; then
sed 's/^/ /' "$WORK/$CHART.pyerr"
echo "::endgroup::"
echo "::error title=selector-match $CHART::the selector comparison itself failed for $CHART (python exit $RC). Treated as a failure, never as a pass."
FAILED="$FAILED $CHART"
continue
fi
CHART_FAIL=""
CHART_OK=0
CHART_SKIP=0
while IFS='|' read -r VERDICT DETAIL; do
case "$VERDICT" in
OK)
CHART_OK=$((CHART_OK + 1))
echo " ok $DETAIL"
;;
SKIP)
CHART_SKIP=$((CHART_SKIP + 1))
echo " gap $DETAIL"
;;
FAIL)
CHART_FAIL=1
echo " FAIL $DETAIL"
echo "::error title=selector-match $CHART::$DETAIL"
;;
*)
[ -n "$VERDICT" ] && echo " $VERDICT $DETAIL"
;;
esac
done <"$WORK/$CHART.out"
echo "::endgroup::"
TOTAL_SELECTORS=$((TOTAL_SELECTORS + CHART_OK))
if [ -n "$CHART_FAIL" ]; then
FAILED="$FAILED $CHART"
SUMMARY_ROWS="${SUMMARY_ROWS}| \`$CHART\` | **failed** | $CHART_OK | $CHART_SKIP |"$'\n'
echo "$CHART: FAILED"
elif [ "$CHART_OK" -gt 0 ]; then
CHECKED="$CHECKED $CHART"
[ "$CHART_SKIP" -gt 0 ] && WITH_GAPS="$WITH_GAPS $CHART"
SUMMARY_ROWS="${SUMMARY_ROWS}| \`$CHART\` | ok | $CHART_OK | $CHART_SKIP |"$'\n'
echo "$CHART: ok ($CHART_OK selector(s) matched, $CHART_SKIP not checked)"
elif [ -n "$HAS_OVERLAY" ]; then
# The overlay's ONE job is to push this chart over the threshold at which
# it renders a selector. If it did not, this test is vacuum-green for the
# chart it was written for, which is the failure the overlay prevents.
FAILED="$FAILED $CHART"
SUMMARY_ROWS="${SUMMARY_ROWS}| \`$CHART\` | **failed** | 0 | $CHART_SKIP |"$'\n'
echo "::error title=selector-match $CHART::$CHART has an overlay at ci/selector-match/$CHART.yaml but rendered no PodDisruptionBudget and no topologySpreadConstraints, so nothing was checked. That overlay exists only to make those objects render - either it no longer reaches the threshold (replica count, a feature switch) or the chart stopped rendering them. A green tick here would be a tick for a check that ran over nothing."
echo "$CHART: FAILED (overlay present, no selector rendered)"
else
NOT_EXERCISED="$NOT_EXERCISED $CHART"
[ "$CHART_SKIP" -gt 0 ] && WITH_GAPS="$WITH_GAPS $CHART"
echo "$CHART: no PodDisruptionBudget and no topologySpreadConstraints rendered - nothing to check."
fi
done
# --- Report ------------------------------------------------------------------
count() { set -- $1; echo "$#"; }
listing() { set -- $1; [ "$#" -eq 0 ] && { echo "–"; return; }; printf '`%s`' "$1"; shift; for C in "$@"; do printf ', `%s`' "$C"; done; echo; }
SUMMARY="${GITHUB_STEP_SUMMARY:-/dev/null}"
{
echo "### Selector-match render test (Gitea #96)"
echo
echo "Every \`PodDisruptionBudget\` selector and every \`topologySpreadConstraints\` label"
echo "selector in a chart's render, held against the labels of the pod templates in the"
echo "**same** render. A selector that matches no pod is not a render error and not an"
echo "install-test failure: the spread constraint is then trivially satisfied and the"
echo "budget never blocks an eviction, both silently."
echo
echo "| Result | Count | Charts |"
echo "|---|---|---|"
echo "| Selectors checked and matching | $(count "$CHECKED") | $(listing "$CHECKED") |"
echo "| No such object rendered | $(count "$NOT_EXERCISED") | $(listing "$NOT_EXERCISED") |"
echo "| Failed | $(count "$FAILED") | $(listing "$FAILED") |"
echo
echo "$TOTAL_SELECTORS selector(s) verified in total."
if [ -n "$WITH_GAPS" ]; then
echo
echo "Charts with selectors this test does **not** evaluate (\`matchExpressions\` only — reported, never counted as passing): $(listing "$WITH_GAPS")."
fi
if [ -n "$SUMMARY_ROWS" ]; then
echo
echo "| Chart | Result | Checked | Not checked |"
echo "| --- | --- | --- | --- |"
printf '%s' "$SUMMARY_ROWS"
fi
} >>"$SUMMARY"
echo "===================== selector match test ======================"
echo "checked: $(count "$CHECKED")${CHECKED:+ -$CHECKED}"
echo "not exercised: $(count "$NOT_EXERCISED")${NOT_EXERCISED:+ -$NOT_EXERCISED}"
echo "failed: $(count "$FAILED")${FAILED:+ -$FAILED}"
echo "selectors verified: $TOTAL_SELECTORS"
echo "================================================================"
if [ -n "$FAILED" ]; then
echo "ERROR: selectors do not match the pod labels they are meant to select in:$FAILED" >&2
exit 1
fi
# The second anti-vacuum guard. Reaching zero means every chart fell into "no
# such object rendered", which is indistinguishable from a broken harness - and
# it is the state this test would silently decay into if the overlays were ever
# dropped.
if [ "$TOTAL_SELECTORS" -eq 0 ]; then
echo "ERROR: not a single selector was checked in this run. Either no chart renders a PodDisruptionBudget or a topologySpreadConstraints any more, or the overlays under ci/selector-match/ no longer push one over its replica threshold. A pass over zero assertions is not a pass." >&2
exit 1
fi