mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-03 10:37:21 +09:00
fix(platform): correct what disabledPackages does to ouroboros (#4098)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does `bundles.disabledPackages` does not uninstall anything. Two comments in the platform chart said it does, offered next to `kubectl delete package.cozystack.io cozystack.ouroboros` as if the two were interchangeable. A name in that list only stops `_helpers.tpl` emitting the Package document, which is the same suppression you already get from leaving `publishing.proxyProtocol` false. Every emitted Package carries `helm.sh/resource-policy: keep`, and that annotation is there precisely so disabling a package stops deleting it, so Helm leaves the Package CR alone. The CR stays, the HelmRelease it owns stays, ouroboros keeps running, and the pre-delete hook never fires because nothing uninstalls the release. Deleting the Package CR is what gets there: GC takes the HelmRelease, Flux uninstalls, the hook runs on the way out. There was a second cost. The guard in the system bundle looks up the live Package CR and never reads `disabledPackages`, so an operator who followed the old advice hit the guard anyway with no idea why. New test case pins that. Comments only in the templates, so rendered output is unchanged. Chart suite green at 156 tests. ### Screenshots Not a UI change. <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> Walked the trigger map against the diff. One repo is reached, and I left its box empty rather than tick it without a link. `cozystack/website` has the same wrong instruction verbatim in three files: `content/en/docs/next/networking/hairpin-proxy-protocol.md:122`, and the identical line under `v1.4/` and `v1.5/`. It tells the reader that removing the Package CR "(or add it to `bundles.disabledPackages`)" triggers helm uninstall and fires the pre-delete hook. It also contradicts `operations/configuration/components.md:72` on the same site, which says `disabledPackages` will not remove components that are already installed. The site fix is cozystack/website#683. It turned out to be four files rather than three: `origin/main` there also carries a `v1.6` tree, and all four copies were byte-identical. Nothing else is reached: no package added or renamed, no `values.schema.json`, no version enum, no default, no key the Ansible role sets, nothing under `hack/`. - [ ] No downstream repository is affected by this change - [x] [cozystack/website](https://github.com/cozystack/website) - follow-up: https://github.com/cozystack/website/pull/683 - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(platform): the platform chart no longer says `bundles.disabledPackages` uninstalls a package. A name in that list only stops the Package document being rendered. `helm.sh/resource-policy: keep` leaves the existing Package CR on the cluster and the component keeps running, so deleting the Package CR is what uninstalls it. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Documentation** - Clarified proxy protocol safety guidance, including the effects of disabling the Ouroboros package and the limitations of using `bundles.disabledPackages`. - Documented that disabling package rendering does not remove an existing live package or stop its associated resources. - **Tests** - Added regression coverage confirming that disabling the package does not bypass the proxy protocol safety guard when the live package remains active. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
3 files changed
+38
-7
No files matched your search
@@ -186,11 +186,14 @@
|
||||
present-on-cluster Package. ouroboros keeps running, the
|
||||
cleanup hook never fires, the rewrite block stays live. The
|
||||
operator must `kubectl delete package.cozystack.io
|
||||
cozystack.ouroboros` (or add the package to
|
||||
bundles.disabledPackages) for the chart to actually uninstall
|
||||
and the hook to actually clean up. This guard fails the render
|
||||
on the dangerous transition path so the operator confirms
|
||||
they understand the asymmetry by setting
|
||||
cozystack.ouroboros` for the chart to actually uninstall and
|
||||
the hook to actually clean up. Adding the name to
|
||||
bundles.disabledPackages is not a second path to that: it only
|
||||
stops _helpers.tpl emitting the Package document, which is the
|
||||
same suppression the flag flip already performs, and
|
||||
resource-policy: keep leaves the live Package running. This
|
||||
guard fails the render on the dangerous transition path so the
|
||||
operator confirms they understand the asymmetry by setting
|
||||
publishing.proxyProtocolAcknowledgeUnclean: true.
|
||||
Known hole: an operator who manually `kubectl delete package
|
||||
cozystack.ouroboros` before flipping the flag back bypasses the
|
||||
|
||||
@@ -78,3 +78,23 @@ tests:
|
||||
path: metadata.annotations["helm.sh/resource-policy"]
|
||||
value: keep
|
||||
|
||||
# bundles.disabledPackages is not a route out of the guard. A name in
|
||||
# that list only stops _helpers.tpl emitting the Package document; the
|
||||
# guard keys off a lookup for the live Package CR, which the list
|
||||
# never touches. An operator who reaches for disabledPackages instead
|
||||
# of `kubectl delete package` still gets the failure, and ouroboros is
|
||||
# still installed when they get it.
|
||||
- it: still fails when cozystack.ouroboros is in bundles.disabledPackages but the Package CR is live
|
||||
set:
|
||||
bundles:
|
||||
system:
|
||||
enabled: true
|
||||
variant: isp-full
|
||||
disabledPackages:
|
||||
- cozystack.ouroboros
|
||||
publishing:
|
||||
proxyProtocol: false
|
||||
proxyProtocolAcknowledgeUnclean: false
|
||||
asserts:
|
||||
- failedTemplate:
|
||||
errorPattern: "publishing.proxyProtocol was previously enabled.*helm.sh/resource-policy: keep leaves the existing Package"
|
||||
@@ -259,8 +259,7 @@ publishing:
|
||||
#
|
||||
# kubectl delete package.cozystack.io cozystack.ouroboros
|
||||
#
|
||||
# or adds the package name to bundles.disabledPackages. THAT is the
|
||||
# path that triggers the chart's pre-delete Helm hook
|
||||
# THAT is the path that triggers the chart's pre-delete Helm hook
|
||||
# (charts/ouroboros/templates/coredns-cleanup-hook.yaml, vendored at
|
||||
# 0.8.0) — the hook quiesces the controller, sed-strips the
|
||||
# BEGIN/END block from the live kube-system/coredns Corefile, and
|
||||
@@ -269,6 +268,15 @@ publishing:
|
||||
# the normal disable path. Documentation of the hook's exact
|
||||
# behaviour lives in the upstream chart's README.
|
||||
#
|
||||
# bundles.disabledPackages does NOT do this. Putting this name in
|
||||
# that list only makes templates/_helpers.tpl skip emitting the
|
||||
# Package document — the same suppression as leaving proxyProtocol
|
||||
# false — and resource-policy: keep then stops Helm pruning a
|
||||
# Package it no longer renders. The Package CR stays, so does the
|
||||
# HelmRelease it owns, ouroboros keeps running, and the pre-delete
|
||||
# hook never fires. No platform values setting reaches that hook:
|
||||
# the operator has to delete the Package CR.
|
||||
#
|
||||
# Manual cleanup recipe (only if the chart's pre-delete hook fails
|
||||
# to land — controller pod stuck in CrashLoop blocking the quiesce
|
||||
# step, ConfigMap RBAC drift on the namespace, helm uninstall
|
||||
|
||||
Reference in new issue
Block a user