Extract variant branches into a generic extension mechanism - #4871
Open
caseydavenport wants to merge 69 commits into
Open
Extract variant branches into a generic extension mechanism#4871caseydavenport wants to merge 69 commits into
caseydavenport wants to merge 69 commits into
Conversation
Add WithContext/ComponentHandlerOption to NewComponentHandler (variadic, backward-compatible) and call operator.ApplyPatches in CreateOrUpdateOrDelete for components implementing render.Named.
Pulls the enterprise RBAC extra-rules and MULTI_INTERFACE_MODE env branches out of pkg/render/typha.go into a new pkg/enterprise package. The enterprise package registers a patch via operator.Patch on startup; pkg/render/typha.go now has zero IsEnterprise branches.
Calls enterprise.Register() at startup so the typha modifier is wired in. Builds an operator.Context in the installation reconciler and passes it to the component handler so registered modifiers receive reconcile-derived state.
Extracts the image override registry into a leaf pkg/imageoverride package (no render/operator transitive deps) to avoid the render→operator import cycle. operator.OverrideImage/ResolveImage now delegate there. Registers the enterprise node image override in pkg/enterprise. Removes the IsEnterprise image switch from render/node.go; FIPS handling is preserved via a post-resolve check.
…sion The OSS installation controller no longer directly creates the node-prometheus keypair or fetches the prometheus/esgw certs. Those are now handled by a registered InstallationExtension in pkg/enterprise. Port value derivation and the kube-controller TLS block remain in the OSS controller unchanged.
Moves the calico-node-metrics Service out of OSS node render and into the enterprise node modifier, where it derives ports from ctx.FelixConfiguration. Also exports NodeBGPReporterPort so the modifier can reference it.
# Conflicts: # pkg/controller/installation/core_controller_test.go # pkg/controller/utils/component.go # pkg/render/node.go
caseydavenport
commented
Jun 3, 2026
The registry package is renamed to extensions. The installation controller builds the render context through a registered factory, and the componentHandler applies registered modifiers to component output. The node and typha variant branches now live in enterprise modifiers, and the calico log directory is mounted for both variants.
Drop the functional-options builder for Inputs (one call site, all fields always set) in favor of a plain struct literal, and replace the single-method RenderContextFactory interface with a registered builder func. All three extension seams now register a func.
Register modifiers, image overrides, and the render context builder per variant. The registries now gate on the installation variant, so the enterprise funcs drop their self-gate guards (the IsEnterprise checks the PR set out to remove) and the image override drops its decline bool - it only runs for its own variant. Move the node prometheus reporter keypair mounting (volume, mount, cert-management init container, pod hash annotation) into the node modifier, and remove NodeConfiguration.PrometheusServerTLS along with the round-trip through the installation controller. Core node render no longer carries a prometheus mount; in calico the keypair is never created. Rename Extensible.Name() to ModifierKey() so an unrelated Name() method can't make a component modifier-eligible by accident.
Merge the per-component image override and modifier into a single
Extension{Image, Modify} registered once per (variant, component) via
extensions.Register, so all of a component's variance lives in one place.
The image half still lands in the imageoverride leaf so render resolves it
without an import cycle; the fan-out is internal to Register.
Rename the variant-level render context builder to Setup
(RegisterSetup/RunSetup). That names the two phases a reader has to hold:
Setup is the controller-side work that builds the RenderContext baton, and
Extension hooks are the pure render-time funcs. Three registries with three
key schemes become two concepts split by when they run.
modCtx read like "modifier context"; the value is an extensions.RenderContext, so name it for what it is.
Add a package doc that lays out the two-phase model (Setup vs Extension) so the whole seam is legible from `go doc`. Fix two comments that still called the setup a render context builder.
Add a per-component context channel: a component implements render.ExtensionContextProvider to hand its modifier config a modifier can't derive from the shared RenderContext (config only the component's controller has). The componentHandler reads it into RenderContext.Component before applying the modifier. node's setup-produced keypair keeps its own field; this is for component-config-derived inputs. Move windows's enterprise branches into a pkg/enterprise extension: the two windows image overrides, the node-metrics Service, the calico log volume (swapped in for the OSS cni-log mount), the enterprise felix env, the trusted DNS servers for openshift/rke2, and the prometheus reporter keypair mount. The windows component exposes its reporter port, keypair, and trusted bundle via ExtensionContext; the windows controller wires the render context into its handler. Core windows render is now OSS-only.
The stub component and applyExtensions helper for exercising a modifier through Set.Decorate were copy-pasted into the extensions, render, and enterprise test packages. Pull them into pkg/extensions/extensionstest so there's one copy, and have the three test packages import it.
The flat pkg/enterprise mixed every component's extension in one package. Split
it so each subpackage maps to the render component it extends and exposes a
Register func that New() composes:
enterprise/typha, guardian, apiserver, windows - self-contained.
enterprise/installation - the installation controller hook plus the node and
calico-kube-controllers modifiers, which share installationRenderData (kept
internal here).
enterprise/kubecontrollers - the es-calico-kube-controllers assembly plus the
enterprise kube-controllers cluster role rules it shares with the calico
modifier (exported as KubeControllersEnterpriseCommonRules).
The shared felix reporter-port helpers move with node (exported as
NodeReporterPort/ValidateReporterPort) so the windows hook can reuse them.
logstorage now imports enterprise/kubecontrollers for the es-kube-controllers
symbols instead of the old flat package. Each subpackage gets its own test
suite; the decorate helper is the shared extensionstest package.
The enterprise integration cases (managed-cluster resources, impersonation, SCC, the enterprise GuardianPolicy) ran base render plus the real modifier, so they pulled enterprise.New() into the render test package. Move them next to the modifier in pkg/enterprise/guardian, where they run against the real Set. What stays in render is the OSS render path: the OSS guardian-access policy and the public-CA deployment cases. Those never run the modifier, so render_test no longer depends on pkg/enterprise for guardian.
The CalicoEnterprise windows cases ran base render plus the real modifier (and needed the enterprise image overrides), so they pulled enterprise.New() into the render test package. Move them next to the modifier in pkg/enterprise/windows. The OSS render tests stay put, now built with an empty imageoverride.New() instead of the enterprise Set's overrides, so render_test no longer depends on pkg/enterprise for windows.
node_enterprise_test.go ran the real ExtendContext hook and node/typha modifiers, so move it whole into pkg/enterprise/installation next to those. node_test.go only ever rendered base output (no modifier), so its CalicoEnterprise cases are variant-blindness checks and stay put. Swap its cfg from the enterprise Set's image overrides to an empty imageoverride.New() so render_test no longer depends on pkg/enterprise for node.
The apiserver OSS tests went through the enterprise modifier only to pick up the Calico-variant cleanup deletes, but those deletes come from the base render itself (and the cleanup modifier is covered in pkg/enterprise/apiserver). Render the base component directly instead. With apiserver no longer reaching for the enterprise Set, nothing in the render test suite uses it, so remove enterprise_setup_test.go. pkg/render no longer depends on pkg/enterprise, in production or test code.
…lers The es-kube-controllers assembly lives in pkg/enterprise/kubecontrollers, but its render tests sat in pkg/render/kubecontrollers and pulled the enterprise package into the render test deps. Move them next to the assembly. The calico-kube-controllers (OSS) tests stay put, and pkg/render/kubecontrollers no longer depends on pkg/enterprise.
The ManagementClusterConnection controller renders Guardian for both OSS (Whisker) and Enterprise (managed-cluster tunnel), and had IsEnterprise branches woven through its reconcile: the management/managed mutual-exclusion check, the managed cluster version (CNX vs Calico), and the license-gated egress network policy. Add a clusterconnection ControllerExtension (pkg/enterprise/clusterconnection) with Validate (the mutual-exclusion check) and ExtendContext (CNXVersion + the license check), stashing the result as a render.GuardianRenderData in the render context. The controller reads it back generically to fill GuardianConfiguration, falling back to OSS defaults (CalicoVersion, the Whisker Guardian client keypair, egress disabled) when no extension is registered. No IsEnterprise left in the reconcile data path. Still variant-aware in the controller: the ManagementClusterConnection CR validation and defaulting (impersonation, public CA) and the ImageSet selection. Those would need a CR validator/defaulter companion to move, left as a follow-up.
Move the Calico Enterprise cases (enterprise images, the license/tier-gated calico-system policy, and impersonation) out of clusterconnection_controller_test.go into clusterconnection_controller_enterprise_test.go. The generic controller mechanics (default reconcile, the guardian finalizer, proxy settings, and the tigerastatus conditions) stay in the main file. Same specs, just relocated; the package-scope proxy helpers are shared between the two files.
ExtendContext handed back a render context while the controller context it was given already embeds one, leaving two near-identical copies in play. Return the updated controller context instead, so a single context flows through the reconcile and callers read its embedded render context.
The extension Set computes its variant's controller-phase options once at startup (ComputeOptions, run from main) and carries them onto each ControllerContext, so the shared types no longer name an enterprise-only setting. The ControllerContext holds an opaque Options the variant's hooks assert back out; the enterprise multi-tenant flag and its discovery live entirely in pkg/enterprise.
5 tasks
Master added WAF policy RBAC to the tigera user and network-admin cluster roles in pkg/render/apiserver.go, which this branch had already moved into pkg/enterprise/apiserver/extension.go. Ported those rules into the relocated functions so the permissions aren't lost.
Master added assertions that the tigera-ui-user and tigera-network-admin cluster roles grant WAF policy access. Those lived in the apiserver_test.go fixtures this branch removed, so assert the WAF rules directly on the rendered roles in the extension test instead.
…sions Port master's new enterprise features into the variant extension packages rather than dropping them: the fluent-bit log collector rename, per-tenant Linseed RBAC, the RBAC-UI rbacsync controller, and the WAF Gateway API surface. WAF keeps master's two-signal distinction - the applicationlayer controller and its RBAC stay wired whenever the GatewayAPI CR is present so it can de-program, while the active WAF surface stays gated on the extension being enabled.
… enterprise test mock The shared component handler clears stale ignore-annotation warnings on every successful apply, so the enterprise controller test's status mock needs those expectations like the base controller test already has.
Brian-McM
reviewed
Jul 27, 2026
ControllerContext stored the reconcile's context.Context as a field, which every hook then immediately unpacked as utils.GetX(cc.Ctx, cc.Client). The struct is copied by value and carries long-lived config alongside it, so a request-scoped context had no business living there, and Set.ComputeOptions already took its ctx explicitly. Validate and ExtendContext now take ctx as the first parameter like the rest of the codebase. Also renames the render.RenderContext parameters that were named ctx, since they shadow the meaning of ctx everywhere else.
None of these are contexts in the Go sense - they carry no cancellation and no request scope, they're the argument bundles the two extension points take. render.RenderContext also stuttered on its own package. render.RenderContext -> render.Inputs contexts.ControllerContext -> controller.Inputs contexts.ControllerName -> controller.Name render.GuardianExtensionContext -> render.GuardianExtensionInputs render.APIServerExtensionContext -> render.APIServerExtensionInputs render.ExtensionContextProvider -> render.ExtensionInputsProvider ExtendContext -> ExtendInputs pkg/controller/contexts moves up to pkg/controller so the type reads controller.Inputs. The four controllers that use it also import controller-runtime's controller package, so that one is now aliased to ctrl in those files - each uses it exactly once, for controller.Options.
caseydavenport
force-pushed
the
casey-variant-extensions
branch
from
July 27, 2026 20:33
39de688 to
03e702b
Compare
A modifier used to be registered against a bare component name, with its inputs type inferred from the modifier itself. Nothing tied the two together, so a modifier written for one component registered happily against another and only failed at render time, as a BUG log and a silently unmodified component. Registration now takes the component's ModifierKey, which pins the type of the inputs its modifier receives. Cfg comes from the key rather than from the modifier, so a mismatch does not compile. Keys can only be declared in render, next to the component they belong to. Every extension point gets its own inputs type, empty ones included - sharing a single empty type between components would let the compiler accept a modifier registered against the wrong one. That also means Variant.Modify and RegisterModifier collapse into one generic extensions.Modify.
A modifier finds the container it layers onto by name, and both sides were writing that name out independently: render built the container from a literal and the modifier matched on its own copy. install-cni existed three times over (render, the modifier, and the modifier's test fixture) with nothing tying them together, so renaming it in render left the modifier compiling and silently not matching. Render now declares those names and uses them at the container it names, and the modifiers match on the same constants. Picking containers out of a PodSpec also moves behind render.Container / render.Containers, which log rather than skip quietly when nothing matches - a modifier asking for a container that is no longer rendered is a bug on one side or the other. The modifier tests that ran against hand-built DaemonSets now run against real render output. A fixture only ever matches the shape render had when it was written, which is what let this go unnoticed: with the container renamed at the creation site, every affected enterprise suite now fails.
…sions Conflict in pkg/render/apiserver_test.go: master added two specs while this branch moved the Calico Enterprise block of that file out to pkg/enterprise/apiserver. The spec master added to the Calico block merges as is. The one it added to the enterprise block is ported over, covering the half that was not already there: the existing v3-CRD spec asserts the deployment survives, so this adds the APIService assertion. It runs post-modifier, since the base render is variant-blind here and queues the deployment for deletion until the enterprise modifier takes it back out.
Several were paragraphs restating what the declaration below them already said, which makes the code harder to scan rather than easier. No behavior change.
A modifier that can't find the container it is written against means render and the modifier have drifted apart. That is not recoverable, so render.MustContainer and extensions.Modify panic rather than logging and leaving the object half-modified. Not every container is guaranteed, though. The windows confd container only renders when BGP is enabled, so treating it as required would panic on every BGP-disabled Windows cluster. render.Container stays for that case and reports whether it found one. Also drops the render.ContainerName type: only three of the ten container-name constants used it, which forced a cast wherever they met a plain string.
It was embedded, so the field was called Inputs and construction read
Inputs: render.Inputs{...}. Naming it RenderInputs says where the field came
from at the point of use, at the cost of the promoted accesses getting longer.
caseydavenport
force-pushed
the
casey-variant-extensions
branch
from
July 27, 2026 23:02
1897453 to
d048a39
Compare
Brian-McM
reviewed
Jul 28, 2026
Every modifier key now pins a distinct inputs type, so a modifier registered against the wrong key fails to compile.
Brian-McM
reviewed
Jul 29, 2026
| // variant. The methods the controller calls (Decorate, Validate, ExtendInputs, | ||
| // Images, ResolveImage) are nil-safe, so the core operator's nil Set yields base | ||
| // behavior. | ||
| type Set struct { |
Contributor
There was a problem hiding this comment.
Been chewing on the Set plumbing and wanted to float an idea. Might be missing something, so push back freely — you've got more context on where this is headed.
The observation: a controller's extension seems fully determined by the time the controller is constructed. The variant is a startup fact (main resolves enterpriseCRDExists via discovery before any controller runs), and the controller-phase hooks are single-variant anyway — only the CalicoEnterprise bundle registers Controller/Validate/ExtendInputs/etc.; the Calico bundle only registers a render cleanup modifier. So the per-reconcile s.variant(install.Variant) lookup on the controller-phase methods isn't really routing — it's a run-or-no-op gate on a single implementation.
Where that leads: if the extension is resolved once (at construction), most of Set's methods stop earning their place:
Validate/ExtendInputs/SetupWatches/DefaultFelixConfiguration/Decorateare per-call routing wrappers — resolve variant, resolve the thing, call it. That "call it" is just a method onControllerExtension/Watcher/ the decorator. Hand the caller its resolved extension and these collapse into direct interface calls.Images/ResolveImageare already leaf pass-throughs (pkg/rendertakes the bare*imageoverride.Overridesand imports noextensions). Hand out the leaf.RegisterOptions/ComputeOptions/ theoptionsfield exist to compute the variant's options at startup and smuggle them onto everyInputs. If the enterprise extension owns and closes over its own options, that goes away — andcontroller.Inputs.Options anywith it.- What's left is
Variant()/variant()— registration and lookup. That's just a registry.
So the endpoint is Set-with-behavior → a dumb registry:
RegisterController(variant, name, ext) ; ControllerExtension(variant, name) ControllerExtension // null-object if none
RegisterModifier[Cfg](variant, key, fn); Decorator(variant) Decorator // identity if none
RegisterImage(variant, component, img) ; Images(variant) *imageoverride.OverridesBehavior moves onto the interfaces; the registry only stores and hands back resolved, null-safe extensions.
Why it might be worth it (the payoff isn't line count — the enterprise behavior stays put — it's shape):
- Core stays variant-blind, structurally.
pkg/controller/utilsdrops itsextensionsimport and the handler becomes as variant-agnostic aspkg/renderalready is — resolution happens at the edge, not in the hot path. - No god-object threading. Controllers stop carrying the whole multi-variant
Setand re-deriving the variant on every call; each resolves its own extension once. - Null-object replaces nil-safety. The
s == nilguards on every method and thenil Set"BUG" log incomponent.godisappear — a lookup that finds nothing returns a no-op, which is correct core behavior, not a bug worth logging. - The routing key can't be mismatched. No more
ci.Controllerset by hand at each call site as a routing field; identity is pinned once at wiring — same spirit asModifierKey[Cfg]pinning the render side. - Keeps the Calico-as-a-library seam clean. A registry that enterprise populates at import/startup and core controllers query by identity is exactly the extension point that model wants, without core needing to know who's wiring it.
Where I'm least sure: the variant key doesn't fully vanish — the render decorator and image overrides genuinely have two variants (the cleanup path), so those lookups still take a variant (resolved once, not per-call). And there may be a reason Set owns behavior that I'm not seeing — option-computation ordering, something about the downgrade path, or a future variant that isn't single-hook. If the god-object is load-bearing for something coming later, say so and I'll happily drop this.
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
4 participants
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.
This is phase 1 of prepping the operator for the monorepo merge, where the Calico and Calico Enterprise code paths eventually live apart. Today they share one codebase with
IsEnterprise()checks sprinkled through the render and controller code, and that coupling is the thing that makes the split hard.This PR pulls the enterprise-specific behavior out of the core code and behind a generic extension mechanism, so the enterprise build registers its own additions and the core operator stays variant-blind. After this, core render and controller code has no idea enterprise exists.
How it works
Enterprise registers extensions against a set keyed by variant. There are two extension points, one per phase of a reconcile:
The core operator registers nothing and runs the base path. All the enterprise wiring lives in
pkg/enterprise, one subpackage per component. After the split, that package is what the enterprise build constructs and the core build drops.Every extracted component (node, typha, guardian, windows, apiserver, kube-controllers) and the clusterconnection controller now run their enterprise behavior through this mechanism, with no
IsEnterprise()left in their core paths. Behavior is unchanged: the test gate is the existing core tests plus the relocated enterprise tests, which now run against the real extension set.A few shared-code cleanups and ergonomic refactors are left as follow-ups, tracked in CORE-13042.