feat(kubernetes)!: remove FluxCD addon - #3379

Code Review

This pull request removes the FluxCD addon from the Kubernetes application as a breaking change, deleting the associated packages, templates, schemas, and documentation. To handle existing installations, a new migration script (54 -> 55) is introduced to orphan the FluxCD HelmReleases in tenant clusters during upgrade. Feedback on the migration script highlights three issues: first, the use of pipefail in a #!/bin/sh script is a bashism that will cause failures on POSIX-compliant shells like dash; second, the JSON patch to remove /metadata/finalizers can fail if the field is already absent, so a merge patch setting it to null is recommended; and third, using printf instead of echo when piping the list of HelmReleases prevents an unnecessary loop execution when no releases are found.

Reviewed with the cozy-review methodology, focusing on the breaking-change aspects. Verdict: LGTM with non-blocking notes. No blocking findings. The FluxCD addon removal is complete and consistent, and the upgrade path for existing clusters is safe.

Breaking-change review

• Migration 54 removes the finalizer and deletes the child tenant HelmReleases by two hardcoded chartRef.name values that match the removed packages/apps/kubernetes/templates/helmreleases/fluxcd.yaml template byte-for-byte.
• Ordering is guaranteed: migration-hook.yaml:34 uses helm.sh/hook: pre-upgrade,pre-install, so the migration runs before the platform re-applies the fluxcd-less ExternalArtifacts.
• targetVersion bumped from 54 to 55 via the shared version-stamp helper. No rebase collision (max on main is 53).
• Upgrade does not hard-fail: the generated openAPISchema in kubernetes-rd/cozyrds/kubernetes.yaml does not set additionalProperties: false, so a leftover addons.fluxcd field in a stored CR is not rejected at upgrade time and is pruned on the next manual apply.
• make generate is up to date (schema / types.go / README), Go builds, no dangling references.
• Both bootstrap signals are false positives: the removed-identifiers enum is FluxInstance.spec.cluster.size inside the fully-removed vendored CRD (not a cozystack values enum); the charts-direct-edit files are all deleted in full (status D), so there is no in-place edit to be reverted by make update.

Non-blocking notes

• No test for migration 54 (a destructive operation across all tenant namespaces). A bats/e2e test is recommended.
• The || true on the kubectl calls in the migration masks a possible finalizer-removal failure (low likelihood, since an apiserver failure would fail the whole run).

NOT LGTM. The removal itself is clean and the ordering is right, but migration 54 swallows the one failure it exists to prevent, and it ships without the test the other migrations in this tree have.

Blockers

Migration 54 deletes the HelmRelease even when the finalizer patch failed

The migration's whole point is stated in its own header: drop the Flux finalizer so the delete does not trigger a Helm uninstall inside the tenant. Both steps carry || true, and the delete is unconditional, so a finalizer patch that fails for any reason (webhook, RBAC, conflict, transient apiserver error) is followed by a delete that sets deletionTimestamp on an object that still has the finalizer. helm-controller then does exactly the uninstall the migration was written to avoid, and the tenant loses its Flux.

The suspend patch is not a second line of defence here. It is the finalizer, and only the finalizer, that keeps the delete inert.

Same shape one level up. hrs=$(kubectl get ... | jq ... || true) with set -eu and no pipefail means a failed kubectl get produces an empty list, the loop runs zero times, and stamp_cozystack_version 55 runs anyway. Migrations never re-run, so those tenants keep their finalizers, and the next reconcile of the addon-less Kubernetes chart prunes the HelmReleases through the normal path with the finalizer intact. Same uninstall, just later and with nothing in the logs pointing at it. An earlier review read this as low risk because an apiserver failure would fail the whole run. Without pipefail, and with || true on the assignment, it does not.

Every migration from 42 through 53 uses set -euo pipefail, and none of them uses || true except where the swallowed case is named in a comment. 54 is the only one on set -eu.

Verify the finalizer is gone before deleting, and let the migration fail if it is not. A migration that stops is recoverable; one that stamps 55 on a half-done fleet is not.

No test, and this tree tests its migrations

hack/migration-50-etcd-adopt.bats, hack/migration-seaweedfs-db-adopt.bats, hack/monitoring-pvc-backfill-migration.bats and hack/kubernetes-md0-migration.bats all drive the real migration scripts against a fake kubectl inside the migrations image's own base. The seaweedfs one pins fail-closed behaviour as an explicit property, with the reasoning spelled out in its header: a swallowed error permanently leaves at-risk tenants exposed, because migrations never re-run. That is the property 54 gets wrong, and the harness for testing it already exists.

Two cases are enough: the ordering (suspend, then finalizer, then delete, with the delete gated on the finalizer being gone) and the selection by chartRef.name across namespaces.

Non-blocking

.github/CODEOWNERS:78-79 still owns /packages/system/fluxcd/ and /packages/system/fluxcd-operator/, both deleted here.

The PR body says grep -riE 'affero|AGPL' packages/ returns nothing after the change. On the branch it returns two hits in packages/system/monitoring — comments explaining why Grafana is not rebuilt. No AGPL payload, but the claim as written does not hold.

A tenant that had the addon keeps a Flux the platform no longer manages and no longer exposes a toggle for. enabled: false used to uninstall it. Now nothing does, and neither the release note nor any doc says how an operator removes it by hand. One line in the release note would cover it.

NOT LGTM. Two of the three round-1 items are properly closed, but the fail-closed rewrite has one || true left in it, on exactly the line the whole revision exists to protect.

Business context: Removes the last AGPL-3.0 vendored content (the default-off tenant FluxCD addon) as a breaking change for CNCF governance; tenants that had it enabled keep their running Flux via an orphaning migration.

Blockers

The verification read is fail-open

The gate added in this revision reads the finalizer back with 2>/dev/null || true (packages/core/platform/images/migrations/migrations/54:63-64). A read that fails yields an empty remaining, and empty passes the gate: delete and stamp proceed on an object whose state was never checked. Reproduced against the real script inside the pinned alpine base with a verify read that exits 1: exit code 0, the delete ran, version stamped to 55. The header says "no || true" and "the delete is gated on the finalizer actually being gone"; line 64 disagrees with both.

On blast radius, to be fair: helm-controller v1.5.0's reconcileDelete skips the uninstall for suspended objects and drops the finalizer itself, so on the shipped controller even the worst case does not uninstall the tenant's Flux. But that fence lives in the controller, not in this script, and the header sells the finalizer gate as the guarantee.

--ignore-not-found closes it in one line: a concurrently-deleted object still reads as empty with exit 0, a real API error aborts under set -e.

remaining=$(kubectl -n "$ns" get helmrelease "$name" --ignore-not-found \
  -o jsonpath='{.metadata.finalizers}')

While in the file: the header's claim that removing the finalizer is the ONLY thing keeping the delete inert is not accurate for this controller version, suspend alone protects too. Worth rewording so the next reader does not re-derive it.

The failure knobs exist and nothing uses them

The fake (hack/testdata/migration-54-fluxcd/kubectl:13-15) plumbs FAKE_LIST_FAIL, FAKE_PATCH_FAIL and FAKE_DELETE_FAIL, and its own header says a fake that can only succeed cannot test fail-closed. Neither test uses any of them, and there is no knob for the verify read failing, which is how the blocker above stayed invisible. Two tests close this: a verify-read-fail knob pinning the fix above (read fails, migration aborts before delete and before stamping), and a FAKE_LIST_FAIL case (scan fails, abort, no stamp); that one also pins the busybox-ash pipefail behavior the bats header calls load-bearing. The seaweedfs sibling this file imitates pins exactly these shapes.

The e2e harness still submits the removed addon

hack/e2e-chainsaw/_lib/run-kubernetes.sh:290-292 still puts fluxcd: {enabled: false, valuesOverride: {}} into the Kubernetes CR it applies. It no-ops silently since the schema is not strict, but it references a field this PR removes. Delete the three lines.

Non-blocking

The patch, get and delete calls use bare helmrelease while the fleet scan uses the fully-qualified helmreleases.helm.toolkit.fluxcd.io. Qualifying all of them keeps the migration immune to short-name collisions.

Everything else from round 1 checks out. The ordering holds: the migration is a pre-upgrade hook and the manifests that update the tenant-consumed artifacts apply only after hooks succeed, so a failed migration blocks the upgrade with the old chart still in force. The chartRef selection matches the removed template literally and that form shipped unchanged since before v1.0.0. The metadata-only finalizer strip triggers no reconcile on this controller (the watch predicate ignores it), so there is no re-add race. Leftover addons.fluxcd in stored tenant values is inert. CODEOWNERS is fixed, the AGPL grep claim in the body now holds, and the release note documents manual removal.

NOT LGTM. The migration is now right; the tests that pin it are not. Every negated assertion in the new bats file is a no-op, so the half of the fail-closed contract that says "and it must not have deleted or stamped" is not actually enforced.

Blockers

The ! assertions never fail the test

cozytest.sh runs each test body under set -e and injects return 0 at the closing brace, and POSIX set -e ignores failures of commands negated with !. So ! grep -qE -- "^DELETE " "$FAKE_CMDLOG" does nothing when the DELETE line IS there: grep succeeds, the negation yields rc 1, set -e skips it by design, and the body falls through to the injected return 0. Verified on the harness itself: a scratch test whose log contains a DELETE line and whose body is exactly that negation reports Test OK.

Concretely: the finalizer-sticks and verify-read-fail tests enforce only [ "$rc" -ne 0 ], so a regression that deletes or stamps before aborting still passes. The list-fail test's "nothing touched, nothing stamped" clauses are all vacuous, leaving the rc check as its only real assertion. Test 1's ! grep some-other-app is backstopped by the exact -eq 4 counts, so selection survives. The comments say "and it deleted nothing and stamped nothing"; the checks behind those words do not exist.

The fix is mechanical, per line:

if grep -qE -- "^DELETE " "$FAKE_CMDLOG"; then echo "unexpected DELETE" >&2; return 1; fi

An explicit return 1 propagates fine; only fall-through hits the injected return 0. For the record, the same ! pattern predates this PR in the seaweedfs and etcd adopt suites; that is tracked separately and is not this PR's to fix.

Round provenance in committed text

The comment above the verify-read-fail test says "Round 2 of this migration replaced 2>/dev/null || true on that read with --ignore-not-found", and the last commit's message says "pinning the round-2 fix". A reader of the tree or git log has no round 2. Describe the property instead: the read uses --ignore-not-found rather than 2>/dev/null || true so a failed read aborts instead of reading as finalizer-gone. Same reword for the commit message.

Non-blocking

The testdata realignment sits in the last commit, so at the previous commit the suite is red (its own message says the verify read matched the fleet-scan branch). This repo merges with merge commits, so that intermediate lands in history and breaks bisect over the range; you are rewriting these commits anyway, fold it in.

What held up: the fail-open verify read is properly closed (--ignore-not-found, no || true left in code, header now matches the code); all kubectl calls are fully qualified; the e2e harness block is gone; the fake's matching order is right (the verify read is matched before the fleet scan by the jsonpath token) and the verify-read-fail pin itself is non-vacuous, since reintroducing || true makes the enforced [ "$rc" -ne 0 ] fail; all four tests are green on a linux docker host. A stale addons.fluxcd left in a parent's values passes schema validation (the schema sets no additionalProperties: false), so no parent-values cleanup step is needed in the migration. The red "Unit & controller tests" on this PR is inherited: main's own run-kubernetes-talos-diagnostics_test.bats is missing from the frozen EXIT-trap list in hack/cozyreport.bats, and both files are byte-identical between this branch and main. Not this diff's doing, but it needs a one-line fix on main before either of the open migration PRs can merge green.

LGTM. Both blockers are gone: every negation is now if grep ...; then ...; return 1; fi, and the round wording is out of both the test comment and the commit message. The testdata realignment is folded in, so the two commits stand on their own.

I checked the load-bearing parts of the migration rather than just the wording. The chartRef selector matches the two names the removed template actually emitted, the migration hook is pre-upgrade,pre-install so the finalizer drop really does precede any pruning, targetVersion: 55 lines up with stamp_cozystack_version 55, and the new bats file is picked up by the hack/*.bats wildcard in bats-unit-tests. Nothing named addons.fluxcd survives outside the changelogs. Migration 53 also bumped targetVersion without touching the migrations image digest, so leaving that pin alone matches precedent.

E2E is red on the install flake, not on this diff: tenant-root/monitoring and seaweedfs time out behind vlstorage-db-vlstorage-generic-0 failing autoplace with "Not enough available nodes", with a lineage.cozystack.io webhook deadline alongside. The API owner gate is the other red check, since api/apps/v1alpha1/kubernetes loses a field. The two downstream items in the description need issues, otherwise they are a paragraph in a merged PR.

LGTM. Rebased this onto main myself after #3406 landed, since both PRs claimed migration 54.

What the rebase changed, nothing else: the migration file is renumbered 54 to 55, its header and stamp_cozystack_version now read 56, targetVersion goes 55 to 56, and hack/migration-54-fluxcd-orphan.bats plus its testdata directory are renamed to 55 with the numbers inside them updated. The redis migration that came in with #3406 stays at 54, untouched.

Worth knowing for the next collision: migrations/54 conflicted add/add and was loud, but values.yaml merged clean on its own because both branches wrote the same 55. Resolving only what git points at would have left targetVersion: 55 next to a migration 55 that stamps 56, and the runner loops seq 54 54, so the migration would never run. make migrations-target-check catches exactly that; it reports targetVersion=56 >= 56 (latest migration 55) now, and the platform suite is 138/138.