[docs] Add GitOps configuration surfaces design proposal#16
[docs] Add GitOps configuration surfaces design proposal#16myasnikovdaniil wants to merge 5 commits into
Conversation
Proposes a single GitOps primitive (GitRepository + Kustomization + a scoped ServiceAccount) instantiated at two trust tiers: platform GitOps for admins (cluster scope) and tenant GitOps for tenants (namespace scope), plus a packageOverrides admin override layer. Generalizes cozystack/cozystack#2731 as the first increment. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a design proposal for GitOps Configuration Surfaces in Cozystack, defining a unified primitive to enable platform GitOps for admins and tenant GitOps for users. The review feedback highlights several critical areas for improvement: first, Helm's default list-merging behavior could inadvertently overwrite default lists (like VM images) when applying packageOverrides; second, a potential security bypass exists where tenants could omit or modify the sharding label on Kustomization resources to run them on the unrestricted global controller; and third, restricting the customizer's write access to cozy-system to protect the supply chain would conflict with the need for admins to manage system-level configurations (like the cozystack-values Secret) in that namespace.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| packageOverrides: | ||
| linstor: | ||
| autoDiskful: | ||
| minutes: 10 | ||
| vm-default-images: | ||
| images: | ||
| - name: ubuntu-24.04 | ||
| url: https://example/custom.img | ||
| ``` |
There was a problem hiding this comment.
When implementing the packageOverrides merge logic in the platform chart, keep in mind that standard Helm merge functions (like merge or mustMerge) overwrite entire lists/arrays (such as the images list under vm-default-images) rather than merging them by key or appending to them. If an admin only wants to override or add a single image, they might inadvertently wipe out all other default images. It would be beneficial to specify in the design how list-type overrides are handled or if custom template merging logic will be used.
There was a problem hiding this comment.
The packageOverrides example this comment sits on has been dropped — PackageValues is now the single override layer. The list-merge concern is addressed head-on: the Admin override layer section pins the policy as maps deep-merge, lists replace wholesale (standard Helm semantics), and calls out the vm-default-images images list as exactly this footgun — overriding one image replaces the whole list, so adding one means restating it; keyed/append-merge is an explicit per-package opt-in. (1620d3b)
|
|
||
| A tenant must not be able to (a) reconcile into another namespace, or (b) borrow a more privileged SA. Two options, recommendation first: | ||
|
|
||
| 1. **Dedicated tenant kustomize-controller shard** (recommended) — mirror the existing `flux-tenants` helm-controller shard. Run a kustomize-controller selecting `sharding.fluxcd.io/key: tenants`, started with `--no-cross-namespace-refs` and a default-service-account lockdown, so the restrictions apply *only* to tenant reconciliation and platform Kustomizations are unaffected. This matches a pattern Cozystack already uses. |
There was a problem hiding this comment.
Using a dedicated tenant kustomize-controller shard that filters by a sharding label (e.g., sharding.fluxcd.io/key: tenants) introduces a potential security bypass. If a tenant can create or modify Kustomization resources directly, they could omit or change this label to have their Kustomization reconciled by the default, unrestricted global kustomize-controller instead. To make this isolation robust, the design should include an admission policy (such as a ValidatingAdmissionPolicy or webhook) that enforces the sharding label on all tenant-created Kustomization resources, or configure the global controller to explicitly ignore resources in tenant namespaces.
There was a problem hiding this comment.
Reworked in 771ebec. The legacy flux-tenants shard this described is retired (flux-shard-operator now owns helm-controller sharding), so tenant isolation no longer "mirrors" it. Tool 2 now provisions a dedicated per-tenant kustomize-controller + SA (a separate sharding path from the helm pool), plus a fail-closed mutating webhook that stamps the shard label + serviceAccountName and a ValidatingAdmissionPolicy backstop — so a label-less or foreign-SA Kustomization is rejected at admission and never reaches the unrestricted main controller. The enforcement ships with the surface, not as deferred hardening.
| - **Tool 1 (admin):** repo write == cluster-admin, by design and by documentation. The curated ClusterRole in the prototype (patch-not-delete on Packages, no CRDs, etc.) is *defense against accident, not against intent* — anyone with repo write can escalate, so the docs must not over-claim a boundary. New trust surface introduced: an admin git repo + its credentials (protect like a kubeconfig), and a standing, self-healing reconciling credential whose blast radius equals an admin's. | ||
| - **Tool 1 footguns to narrow** (they cost nothing functionally): source access should not let the customizer rewrite the platform's *own* supply chain (the `cozystack-packages` OCIRepository in `cozy-system`), so scope write away from `cozy-system`; and field ownership on downstream Packages should eventually be enforced by a `ValidatingAdmissionPolicy` scoped to the customizer SA that rejects changes to fields currently managed by helm-controller (detected via `managedFields`), leaving the hand-owned bootstrap Package free. Until then, the `packageOverrides` layer keeps admins off chart-owned objects entirely, so the contract is mostly avoided rather than merely documented. | ||
| - **Tool 2 (tenant):** the surface runs as the tenant's own SA, so it can do nothing the tenant cannot already do via the API. The new risk is **cross-namespace / SA-borrowing**, closed by the tenant shard lockdown (`--no-cross-namespace-refs`, forced `serviceAccountName`). Tenant-supplied input = the contents of a tenant-owned repo, bounded by `cozy:tenant:*`. |
There was a problem hiding this comment.
There is a potential conflict between scoping the customizer's write access away from cozy-system (to protect the platform's supply chain like cozystack-packages) and the need for admins to manage system-level configurations. As noted in the Context section, critical admin configurations (such as cert-manager issuers or other platform values) are driven by resources residing in cozy-system (e.g., the cozystack-values Secret). If write access to cozy-system is completely blocked, admins won't be able to manage these via GitOps. Consider refining this to use fine-grained RBAC or a ValidatingAdmissionPolicy that specifically protects the supply chain resources (like OCIRepository or specific HelmReleases) while still allowing the customizer to manage other configurations in cozy-system.
There was a problem hiding this comment.
Good catch on the tension — resolved deliberately the other way. The customizer is a cluster-admin-equivalent admin tool, so it manages cozy-system (cert-manager issuers, the cozystack-values Secret, etc.) the same way Flux manages its own manifests — intentional, not a boundary to wall off. So rather than scope write away from cozy-system, any supply-chain protection (e.g. the cozystack-packages OCIRepository) is an opt-out guard, not a default exclusion. Updated the Security posture in 1620d3b.
Revising Tool 1: override layer as a
|
PackageValues (proposed) |
packageOverrides (alternative) |
|
|---|---|---|
| API change | new optional CRD + reconciler merge | none (chart-only) |
| Field-ownership race | eliminated (separate object) | sidestepped (funnel through chart) |
| RBAC delegation | per-package (write PackageValues, read Package) |
all-or-nothing (sub-key of one object) |
List/map merge (e.g. vm-default-images.images is a 16-entry list) |
one tested merge in the operator | per-package Go-template logic, untested |
| Override blob | one small reviewable resource per package | one god-object for all packages |
| Schema validation | validatable per package later | free-form JSON |
packageOverrides is a reasonable lighter-weight alternative — no Go, ships purely as chart templating — and is essentially PackageValues crammed into one chart-rendered blob. The tradeoff is that it sidesteps the field-ownership race instead of removing it, can't do per-package RBAC delegation, and pushes list-merge correctness into per-package templates. Since PackageValues is additive, shipping packageOverrides first wouldn't block it — but it would make the packageOverrides key throwaway.
Feedback welcome on the precedence chain (chart defaults < cozystack-values < Package < PackageValues) and the RBAC split before I flesh out the CRD.
…layer Recast Tool 1's override layer from a `packageOverrides` map on the bootstrap Package (merged into downstream Packages by the platform chart) to a dedicated `PackageValues` API kind that the operator merges into each HelmRelease. `packageOverrides` is kept as a lighter-weight chart-only alternative/interim. PackageValues removes the helm-controller force-ownership race structurally (the chart never writes PackageValues; the admin never writes the chart-owned Package), makes ownership enforceable by object-level RBAC, and is fully additive — absent a PackageValues, the operator builds HelmReleases as today. Updates the admin override layer, user-facing / upgrade / security / failure / testing / rollout sections, open question 3 (now PackageValues precedence and binding), and alternatives considered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
📝 WalkthroughWalkthroughAdds ChangesGitOps Configuration Surfaces Design Proposal
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@design-proposals/gitops-surfaces/README.md`:
- Around line 130-131: The proposal currently presents RBAC enforcement as a
mechanism that prevents field-level edits, but this is actually a
human-discipline contract with no API-level enforcement. Revise lines 130-131 to
clarify that the split design structurally avoids the contract violation (by
separating concerns into PackageValues and Package) rather than enforcing it via
RBAC alone. Additionally, update the text to explicitly state that the admission
policy is a prerequisite for production use, not an optional enhancement. Make
clear that without field-ownership enforcement at the API level, admins must
manually follow the documented protocol, and any deviation recreates the
helm-controller race conditions the PackageValues design was meant to prevent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7a0a413-b6cb-49ef-ba12-c1bd9336da65
📒 Files selected for processing (1)
design-proposals/gitops-surfaces/README.md
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Requesting changes — solid proposal, but the tenant-tier isolation argument has a real hole that should be resolved before this leaves Draft.
- The tenant shard is not self-securing. Flux's main controllers run with
--watch-label-selector=!sharding.fluxcd.io/key(confirmed in cozystack's own fluxcd manifests), so a Kustomization created without the shard label is reconciled by the main, unrestricted controller — no cross-namespace-ref block, no forced SA. A tenant withcozy:tenant:admincan create Kustomizations and evade the shard by omitting the label or setting a foreignspec.serviceAccountName. The shard needs an admission policy enforcing the label + SA on tenant-namespace Kustomizations as a prerequisite, not deferred hardening. (A tenant confined to its own SA is still bounded by that SA's RBAC — but nothing forces it onto its own SA once the main controller is in play.) - The
PackageValuesfield-ownership contract is discipline, not enforcement, until the ValidatingAdmissionPolicy lands: kustomize-controller always applies withclient.ForceOwnership(not gated byspec.force), so an admin editing a chart-owned Package field silently steals it from helm-controller. The doc reads as "structurally removed" in one section and "advisory until policy lands" in another — please make that consistent and treat the admission policy as a production prerequisite. - Open Question 1 is load-bearing: both the operator-embedded
internal/fluxinstallcontrollers and a flux-operator-managedFluxInstanceexist in the tree, and the legacy flux-tenants shard is being actively retired by a new flux-shard-operator — so "mirror the flux-tenants pattern" mirrors something on its way out. Which instance serves tenant Kustomizations, and where the lockdown flags live, needs resolving before Tool 2 is implementable.
All Flux/Helm behavior claims I checked are accurate. Minor: the cozy-system write-scoping tension (admin config lives there) deserves an explicit answer, and the list-vs-map merge policy (Open Q3) needs pinning down.
IvanHunters
dismissed
their stale review
Dismissing the review verdict: converting this to non-blocking feedback since this is a design proposal, not code. The technical points in the review comment still stand as suggestions to reconcile before implementation.
…ator The flux-tenants Deployment is retired; helm-controller sharding is now driven by flux-shard-operator (an auto-scaled shard pool), and there is no kustomize-controller sharding. Rework Tool 2 and Context accordingly: - Context: describe the embedded flux-aio control plane and flux-shard-operator; note tenants hold no Flux-CRD RBAC today, so Tool 2 is what creates the surface (and thus the isolation problem). - Tool 2 isolation: a dedicated kustomize-controller + SA per gitops-enabled tenant (a separate sharding path, not the helm-controller pool), a fail-closed mutating webhook that stamps the shard label + serviceAccountName, and a ValidatingAdmissionPolicy backstop. Enforced by construction and shipped with the surface, not deferred hardening. - Resolve Open Question 1: management-plane tenant reconciliation is the embedded flux-aio; the FluxInstance/multitenant lever governs child Kubernetes clusters only (migration 21 retired it here). - Disambiguate the deferred Tool 1 Package field-ownership VAP from the in-scope Tool 2 Kustomization admission enforcement. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…verrides Address the remaining review points as deliberate design decisions: - Package field-ownership: the direct-edit path on chart-owned Packages stays open by design (it is a cluster-admin tool; locking it risks removing tuning knobs admins can reach today). PackageValues is the recommended non-racing path, not a cage; the field-ownership VAP is an optional opt-out guard, never a prerequisite. Reconcile the "structurally removed" vs "advisory" wording toward this. - cozy-system: do not scope customizer writes away from cozy-system — same logic as Flux managing its own manifests. Supply-chain guards are opt-out, not a default namespace exclusion. - PackageValues merge policy fixed (maps deep-merge, lists replace wholesale; the vm-default-images image list is the footgun). packageOverrides is dropped in favor of PackageValues as the single override mechanism. - Resolve Open Question 4: tenant GitOps ships as a catalog app, not a Tenant CRD field. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…ted apps Tenants keep the curated Application abstraction: the GitOps surface grants the source/kustomize authoring needed to reconcile, but not raw HelmRelease or arbitrary-CR create rights. Since the per-tenant kustomize-controller runs as the tenant SA, "BYO from git" means Application CRs from git, not arbitrary HelmReleases — the tenant trust model is identical to today. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@design-proposals/gitops-surfaces/README.md`:
- Around line 206-207: The PackageValues binding contract is still ambiguous
because this section reopens the choice between 1:1-by-name and spec.packageRef
while the earlier example and failure mode already assume name binding. Update
the design text in the PackageValues section to make the binding rule explicit
and consistent, or mark the earlier example as provisional, so the README does
not imply two different contracts. Use the PackageValues heading and the binding
discussion around packageOverrides as the anchor for the edit.
- Line 184: The current guidance in the PackageValues behavior is to treat an
orphaned PackageValues object as a no-op, but this should be changed to surface
an explicit failure instead. Update the reconcile/validation handling around
PackageValues and the Package merge path so a missing target Package records a
status/validation error or fails reconciliation rather than silently succeeding.
Use the PackageValues and Package reconciliation logic to locate the
orphaned-object check and replace the no-op behavior with an explicit error
path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e932be38-f262-4573-b0d1-8f953447b8a4
📒 Files selected for processing (1)
design-proposals/gitops-surfaces/README.md
| - Admin manifest declares a chart-owned `Package` field directly (bypassing `PackageValues`) → the field is force-claimed from helm-controller, exactly as a hand-run `kubectl patch` does today. This stays available **by design** (it is an admin tool); teams that want it fenced can opt into the field-ownership VAP. `PackageValues` is the recommended non-racing path. | ||
| - Tenant Kustomization omits the shard label, references a source in another namespace, or names a foreign SA → the mutating webhook stamps the label + SA and the VAP rejects a violation, so it never reaches the unrestricted main kustomize-controller; the per-tenant controller's `--no-cross-namespace-refs` blocks the cross-namespace ref. | ||
| - Customizer disabled then re-enabled → resources re-adopt cleanly (SSA, `resource-policy: keep`); no duplicate-creation errors. | ||
| - `PackageValues` for a non-existent Package → no-op; the operator has no Package to merge it into, and merge keys for absent components are ignored. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Don't make orphaned PackageValues a silent no-op.
A typo, deleted Package, or rollout-order race would look successful while the override never applies. Please surface an explicit status/validation failure (or fail the reconcile) so GitOps doesn't hide broken config.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/gitops-surfaces/README.md` at line 184, The current guidance
in the PackageValues behavior is to treat an orphaned PackageValues object as a
no-op, but this should be changed to surface an explicit failure instead. Update
the reconcile/validation handling around PackageValues and the Package merge
path so a missing target Package records a status/validation error or fails
reconciliation rather than silently succeeding. Use the PackageValues and
Package reconciliation logic to locate the orphaned-object check and replace the
no-op behavior with an explicit error path.
| 3. **`PackageValues` binding** — the merge precedence and list-vs-map policy are now fixed under *Admin override layer* (maps deep-merge, lists replace wholesale). The remaining question is binding: 1:1-by-name (as drafted) vs an explicit `spec.packageRef`. (The `packageOverrides` chart layer is dropped in favor of `PackageValues`, so it is no longer an open question.) | ||
| 4. **Resolved (during review):** tenant GitOps is delivered as a **catalog app**, not a `Tenant` CRD field — it avoids core API surface and keeps the feature opt-in per tenant, decoupled from the `Tenant` lifecycle. (A `spec.gitops` field would be more discoverable but is not worth the API surface; see Alternatives.) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Resolve the PackageValues binding contract here.
This section reopens the choice between 1:1-by-name and spec.packageRef, but the earlier example and failure mode already assume name binding. Lock the contract in the main design or mark the earlier example as provisional so implementers don't infer different behaviors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/gitops-surfaces/README.md` around lines 206 - 207, The
PackageValues binding contract is still ambiguous because this section reopens
the choice between 1:1-by-name and spec.packageRef while the earlier example and
failure mode already assume name binding. Update the design text in the
PackageValues section to make the binding rule explicit and consistent, or mark
the earlier example as provisional, so the README does not imply two different
contracts. Use the PackageValues heading and the binding discussion around
packageOverrides as the anchor for the edit.
|
IvanHunters thanks — this was the review that reshaped the proposal. Pushed three commits ( #1 (tenant shard not self-securing) & #3 (Open Q1 / flux-tenants retiring): fully reworked. You're right that #2 (PackageValues = discipline, not enforcement): you're right the wording contradicted itself, and I reconciled it. But I've landed it the other way from "VAP as prerequisite": for the admin Package path we deliberately keep direct edits on chart-owned Packages open — it's a cluster-admin-equivalent tool, and fencing those fields by default risks silently removing tuning knobs admins reach today. So Minors: Also resolved Open Q2 (tenants stay on curated Applications — no raw HelmRelease/CR rights) and Q4 (catalog app). Thanks again for the flux-shard-operator pointer — that was the load-bearing correction. |
2 participants
Summary
Adds a design proposal — GitOps Configuration Surfaces — under
design-proposals/gitops-surfaces/.It defines a single GitOps primitive (
GitRepository+Kustomization+ a scopedServiceAccount) and instantiates it at two trust tiers:packageOverridesadmin override layer so component-parameter changes don't race helm-controller for field ownership.gitopsapp bound to the tenantServiceAccount, isolated via a dedicated tenant kustomize-controller shard.Covers admin package/parameter/golden-image configuration, tenant self-service GitOps, and arbitrary cluster resources outside the core API, plus cross-cutting secrets-as-code, DR/reproducibility, and policy-as-code.
Status
Draft — opening for community feedback per the design-proposal process. The first increment (admin surface) is already prototyped in cozystack/cozystack#2731.
Summary by CodeRabbit