design: component workload overrides - #587
Open
Philip Lombardi (plombardi89) wants to merge 4 commits into
Open
Conversation
Addresses review findings 2, 3 and 4 on the component workload overrides proposal. Adds a security model section stating plainly that write access to the overrides ConfigMap is cluster-admin-equivalent. Field restriction cannot bound it: net-node runs hostNetwork, hostPID and privileged containers with hostPath mounts, storage-supervisor is privileged with hostPath mounts, and metalman is host-networked, so rejecting privileged or hostPath is a no-op and image, args, env or sidecar changes on those pods are root execution. The restrictions are therefore documented as integrity controls rather than a privilege boundary, with GVK and serviceAccountName called out as the two genuine security controls. Replaces the denylist with an allowlist, which fails closed rather than open for fields nobody enumerated. Protects GVK, identity, selector, serviceAccountName, host namespaces, the unbounded-cloud.io annotation prefix, and operator-declared volumes, and rejects all $-prefixed directives and explicit nulls rather than only $patch and $setElementOrder. Makes scheduling constraints additive. NodeSelectorTerms has no patchMergeKey, so a raw patch replaces the mandatory Site affinity that metalman and storage rely on and lets two Sites schedule onto the same nodes. Adds an apply-time assertion that the object is an apps/v1 Deployment or DaemonSet, because apply is GVK-directed and the operator holds escalate and bind on clusterrolebindings.
Covers review findings 5 through 10. Adds a failure and update semantics section. Validation is atomic across the whole ConfigMap before any write; on failure the operator retains last-known-good rather than reverting to un-overridden defaults, because a typo would otherwise strip resources and scheduling constraints from every component at once. Failure scope is tabulated, and desired versus applied hashes make divergence observable. In-memory retention and its restart behavior are stated rather than hidden. Fixes the watch. RequestSingletonAndAllSites lists Sites at event delivery time and drops the per-Site fan-out when that List fails, with no retry, and the singleton pass does not run Site components. Fan-out moves into Reconcile where a failed List returns an error and controller-runtime retries with backoff. Splits the compatibility promise into syntactic compatibility, which is guaranteed within an apiVersion, and target resolution against a release's actual container and volume names, which is not. An absent extraArgs container becomes an object-scoped resolution failure rather than a schema error, which previously contradicted the promise. Narrows the revert guarantee to fields the operator currently declares on objects it currently emits, and names the admission-mutation, competing-manager and no-pruning limits. Corrects the observability claims. Site declares no condition printer columns, SiteReconciler has no event recorder, and Ready means the apply succeeded rather than that the rollout is healthy. All three are now listed as implementation work. Drops overrides diff. A client-side render cannot be authoritative under version skew, the proposed Renderer signature was wrong for cluster components which render from all Sites, and ensureConfig writes before render so a pure render would either lie or perform writes. Replaced with overrides status, which reads back what the operator did. This removes the five-component refactor from the plan. Adds security and Site-isolation tests as the first tests written, and moves integration coverage onto a real API server so revert is asserted against managed fields rather than assumed.
…odel The security model and allowlist commits changed premises that earlier sections still relied on, leaving the document arguing two positions. Six fixes. The kustomize rejection claimed patching a pod template was "a materially smaller grant" because those workloads are already privileged. The security model uses the same fact to reach the opposite conclusion, that override write access is cluster-admin-equivalent. Rearticulated on containment instead: a patch surface can be pinned to apps/v1 Deployment and DaemonSet by validation, re-stamping and an apply-time assertion, whereas an overlay engine cannot be, because selecting group, version and kind is what it exists to do. The difference is an attacker who must pivot through a compromised node versus one who writes a ClusterRoleBinding directly. The typed-struct rejection read as self-contradiction once the surface became an allowlist. Added the distinction: a closed set of paths over a structural merge keeps values open and lets users address containers and volumes the operator never enumerated, which a Go field plus CRD schema per knob cannot. The merge table stated raw strategic-merge semantics for tolerations, nodeSelector and affinity, which the mechanism deliberately departs from to protect mandatory Site affinity. Table now separates raw behaviour from mechanism behaviour and names the three departures. The claim that a patch is exactly a kubectl patch --type=strategic body no longer held for those same three fields, where kubectl replaces and the mechanism appends. Qualified, with overrides validate named as the accurate check. Alternatives table row C carried pre-allowlist expressiveness and validation columns. Renamed the blast radius column to mechanism reach and noted it is not a security boundary, since option C write access is already cluster-admin-equivalent. Prior art concluded "bounded target, open content within it", which stopped describing the design when content became allowlisted. The bounded target is still supported by prior art; the content restriction is specific to this operator's privileged host-namespaced workloads and is now argued as such.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
designs/component-workload-overrides.md: a mechanism for users to customize the Deployments and DaemonSets that unbounded-operator generates and reconciles.Design document only. No code, no CRD changes, no
make generate.This description is self-contained and supersedes the earlier point-by-point review reply, which has been removed so there is one source of truth.
The problem
unbounded-operator generates and reconciles the workloads for five components:
net,machina,gantry(cluster singletons) andmetalman,storage(per-Site). A user's entire influence over their shape today is:spec.components.<c>.enabled(api/machina/v1alpha3/site_types.go:147)spec.components.metalman.replicas(:167) anddhcpAutoInterface(:161)--image-registryflag (cmd/unbounded-operator/main.go:76)The requirements collected from users and the team are considerably larger: resources, tolerations / nodeSelector / affinity, sidecars and volumes, environment variables, command arguments, imagePullSecrets, labels / annotations / priorityClassName, and container images.
There is no workaround, supported or otherwise. The operator applies with server-side apply and
ForceOwnership(internal/operator/component/env.go:242), so akubectl editis reverted on the next reconcile and a GitOps controller managing the same object fights the operator indefinitely. Customization has to be an input to the operator or it cannot exist.This reverses a documented stance.
site_types.go:141-143says components are deliberately not configurable, andarchitecture.md:189says "no Helm or Kustomize". §1 of the doc quotes both; PR 5 amends them.The solution space
MutatingAdmissionPolicyC is proposed. Every requirement above is a field inside a Deployment or DaemonSet the operator already emits. None needs object creation, renaming, or deletion.
Why not kustomize (§5.1)
This was the team's initial suggestion, so the doc argues it at length rather than dismissing it. Five reasons, of which two are load-bearing:
Unbounded transformation with no pruning. A kustomization can rename, delete, add, and re-kind resources. The operator never prunes; singletons are deliberately retained even when no Site enables them (
machina.go:57-69). An overlay that renames a DaemonSet orphans the original permanently and the operator recreates it next pass.GVK containment is impossible by construction. This is subtler than "kustomize grants more privilege", which is not actually true (see the security caveat below). A patch surface can be pinned to
apps/v1DeploymentandDaemonSetby validation, by re-stamping, and by an assertion immediately before apply. An overlay engine cannot be, because selecting group, version and kind is precisely what it exists to do; constraining it to two GVKs removes the reason to adopt it. The difference is between an attacker who must pivot through a compromised node and one who writes aClusterRoleBindingdirectly using theescalateandbindverbs the operator holds (deploy/unbounded-operator/02-rbac.yaml.tmpl:60-66).Also: adopting it freezes the
deploy/*/rendered/layout and every object name in it as public API; kustomize consumes a filesystem, which maps badly onto a cluster object; failed builds produce a string with no per-component attribution; and it promotessigs.k8s.io/kustomize/apifrom transitive (go.mod:336) to direct.Why not a typed struct (§5.2)
Sidecars, volumes, env, args and images cannot be covered by a closed set of fields in any meaningful sense. Projects that start there add a free-form pod template later and carry two overlapping surfaces. This is not contradicted by the allowlist, which is a closed set of paths rather than of fields: values stay open, and users can address containers and volumes the operator never enumerated, which a Go field plus CRD schema per knob cannot.
What is proposed
Storage. A user-owned ConfigMap
unbounded-component-overridesin the operator namespace. The operator only reads it; never creates, seeds, or writes it. Chosen overSite.specbecause thenet/machina/gantrysingletons resolve enablement as "any Site enables it" (machina.go:47-55), so a per-Site override field is ambiguous when Sites disagree. A cluster-scoped object dissolves that and avoids av1beta1conversion obligation. Documents are versioned by a requiredapiVersion.Merge.
k8s.io/apimachinery/pkg/util/strategicpatchwithNewPatchMetaFromStruct. No new dependency. Merge keys verified againstk8s.io/api@v0.36.3. The patch targets the whole workload object, sospec.replicasand workload metadata are reachable through the same field asspec.template.spec.*.Allowlist, not denylist (§8). Unenumerated paths are rejected, so fields added by future Kubernetes versions are denied by default. Protected and re-stamped: GVK, name, namespace, ownerReferences, finalizers, selector, selector-referenced template labels,
serviceAccountName, host namespaces, theunbounded-cloud.io/prefix, and operator-declared volumes. Also rejected: any$-prefixed key at any depth, and explicit nulls.Additive scheduling (§8.3).
NodeSelectorTermscarries nopatchMergeKey(k8s.io/api/core/v1/types.go:3778), so a raw patch supplyingnodeSelectorTermsreplaces the mandatory Site affinity thatmetalmanandstoragedepend on (metalman.go:170,storage.go:261-268), silently allowing two Sites' workloads onto the same nodes. Scheduling constraints are therefore ANDed into operator terms rather than patched. Same fornodeSelectorandtolerations.Failure semantics (§9). Validation is atomic across the whole ConfigMap before any write. On failure the operator retains last-known-good rather than reverting, because a single typo would otherwise strip resources and scheduling constraints from every component at once. Failure scope is tabulated;
override-hash-desiredandoverride-hash-appliedmake divergence observable.Application point (§10). Inside
Env.ApplyObject, the only place the four YAML-driven components and metalman's typed path converge, so both get identical semantics with no component file changes.CLI (§12).
kubectl unbounded overrides list,validate, andstatus.The security caveat
Write access to the overrides ConfigMap is equivalent to root on every node in every affected Site, and therefore to cluster-admin. This is a property of the mechanism, not a defect in it, and §4 states it plainly rather than implying a boundary that does not exist.
Restricting privileged containers and host mounts does not change this, because the workloads are already maximally privileged:
unbounded-net-nodehostNetwork,hostPID(deploy/net/node/03-daemonset.yaml.tmpl:32-33),privileged: trueon two containers (:53,:108), four hostPath mounts (:125-137)unbounded-storage-supervisorprivileged: true(04-daemonset.yaml.tmpl:64,100), three hostPath mounts (:103-111)metalmanHostNetwork: true(metalman.go:165)Rejecting
privileged: trueis a no-op against containers that are already privileged. RejectinghostPathis a no-op when the host root is already mounted. Meanwhile an image change, an args change, anLD_PRELOADenv injection, or a sidecar inheriting pod-levelhostPIDandhostNetworkare each arbitrary root execution on every node.A field allowlist bounds this only if image, args, env, sidecars and volumes are all excluded, which is option B and removes five of the eight requirements. The requirement set and containment are in direct conflict, and this design resolves it by accepting the privilege level explicitly.
Consequences:
serviceAccountNameare the two exceptions and are genuine security controls, because they escape the workload rather than damage it.machina-controller(deploy/machina/02-rbac.yaml.tmpl:15-17),metalman-controller(06-metalman-rbac.yaml.tmpl:59,132) andunbounded-net-controller(net/controller/02-rbac.yaml.tmpl:170,173,216) hold namespace-wide ConfigMap write with noresourceNames. A compromised component can write the overrides ConfigMap. PR H1 narrows these, but RBAC cannot scopecreatebyresourceNames, so a component retaining create can still seed the object when absent. Only a dedicated resource type closes this. That tradeoff is open question 1, not a settled decision.Other caveats
argsandcommandcarry nopatchMergeKey, so a patch replaces them wholesale and drops operator-injected flags.metalmanmakes this concrete: its args begin with theserve-pxesubcommand (metalman.go:108), so a replacing patch stops the container starting.extraArgsexists for the append case.net-controller andmetalmanusemaxSurge: 0withmaxUnavailable: 1because both are host-networked. A bad override yields a window with no available replica, and sinceReady=Trueonly means the apply succeeded, the Site will not report it.Sitedeclares no condition printer columns (site_types.go:25-33) andSiteReconcilerhas no event recorder; onlyLegacyReaperdoes. Both are implementation work in PR 3, not assumptions.overrides validateand last-known-good make the window inert rather than destructive.Review response
Changes Requested was raised against the first revision. All ten findings are addressed across 112fa7c, 5526b4d and 94cc276. Nine accepted; finding 1's remedy partially declined with reasoning below.
Sections were renumbered by two insertions:
escalateandbindon clusterrolebindings, so this was a direct path to cluster-admin, not merely "another resource". Fixed with three independent layers (§8.2, §8.4): validation rejection, post-merge re-stamp, and an apply-timeapps/v1assertion plus a defensive check inApplyObject. Recorded as constraint §3.7.NodeSelectorTermshas nopatchMergeKey, so any patch supplying it replaces Site affinity. §8.3 makes scheduling additive; §15 adds a regression test that two Sites cannot be co-scheduled through any permitted override.serviceAccountName, host namespaces,finalizers, reserved prefix, operator-declared volumes. Rejection extended to any$-prefixed key at any depth and explicit nulls. Also picked upretainKeysonVolumes(types.go:4145), where a partial patch drops sibling fields. Did not adopt the PodTemplate-plus-workload-fields split, becausespec.replicas,spec.strategyand workload metadata are all in the requirements and would need a parallel surface.watch.go:44-49drops the fan-out on List failure with no retry, andreconciler.go:179means the singleton pass never runs Site components. §10 moves fan-out into Reconcile where a failed List returns an error and controller-runtime retries. §15 adds a regression test that must fail against the original wiring.apiVersion) from target resolution against release-specific container and volume names (not guaranteed). An absentextraArgscontainer is reclassified as an object-scoped resolution failure.ensureConfigwrites before render (net.go:64,gantry.go:109), so a pure render would either omit the config hash or perform writes.overrides diffremoved (§12.1) and replaced withoverrides status. This removed the five-component render/apply refactor from the plan.EventRecorderas explicit work.managedFields, plus a test documenting that a competing manager's field survives.On finding 1
The finding is correct and the RBAC over-grants are real. Where this differs is on the remedy. Restricting privileged containers and host mounts does not work, for the reasons in the security caveat above: the workloads are already privileged, so those restrictions are no-ops, while image, args, env and sidecar changes remain root execution.
Rather than imply containment that cannot exist, §4 accepts the privilege level, documents it as cluster-admin-equivalent, gives required RBAC posture, and discloses the residual
creategap that PR H1 does not close. The dedicated-resource alternative is not dismissed; it is recorded as open question 1 with thecreatelimitation spelled out, so it can be re-argued on the merits. If the reviewer still favours it, that is the place to push and it will not be defended on principle.Implementation sequence (§14)
Env.ForComponent,ApplyObjectgate and kind assertion, retryable fan-out,ReconciledWithOverrides,EventRecorder,SiteOverridesprinter column.kubectl unbounded overrides list,validate,statusarchitecture.md:189,site_types.go:141-143,cli.md, and the access-control guidance from §4.3Only the printer column requires
make generate. No API version churn.Open questions (§17)
Please reply against the numbers. 1, 2 and 4 most need a second opinion.
creategap?ValidatingAdmissionPolicycovering schema,apiVersion, protected paths,$directives and nulls be worth the extra installed object, given it cannot do resolution?diffis rejected.spec.replicasvs the typedspec.components.metalman.replicas. Which wins?siteSelectordeferred in favour of asitesname list. Agree?serviceAccountNameis protected, but workload identity normally requires annotating the SA, and the operator reverts those underForceOwnership. A real gap neither this nor the existing escape hatch covers.Review notes
designs/is not published to the docs site;docs.yamlonly fires ondocs/**.ci.yamlhas no path filter, so the full matrix runs on this docs-only PR.Draft for team review. It flips toAcceptedin a final commit here once the open questions resolve, then squash merge.