0011. Discovery does not choose its own candidates, and never auto-registers a weak identity

Status: Accepted · Date: 2026-10-25 · Design section: "Nodes" (discovery and approval)

Decided: Made TalosMachineSpec.Role optional; discovery never picks a role or auto-registers a weak identity

Context

Phase 3 asked for the half of "Nodes" that runs before approval: read a maintenance-mode node's hardware identity over the insecure connection maintenance mode offers, and turn a strong identity into a pending TalosMachine that approval later acts on. Two things had to be settled before any of that could be written.

Where does a candidate endpoint come from? "Nodes" lists two options -- "registered by IP or found by scanning a subnet" -- but nothing in the API types or the controller package picks one, and scanning a subnet is explicitly out of this phase's scope (it is its own piece of design: which interfaces, which CIDRs, how a result is deduplicated against a machine that already has a TalosMachine). Deciding it here would be choosing a design the task handing this phase down had already flagged as undecided. DiscoveryReconciler.Discover therefore takes the endpoint as a plain string argument. Whatever supplies it -- an operator registering one by IP, a future subnet scanner, a CLI command -- calls Discover the same way; nothing about this phase's shape commits to one of them. This is handed back to the design, not decided here.

(Resolved below, in "Discovery probes only declared ranges": the answer is neither free-form registration nor a scanner, but a declared range on TalosCluster.spec.discovery that EnumerateCandidates expands and DiscoverAll drives through Discover unchanged.)

Is TalosMachineIdentity sufficient? Yes, as given: uuid (required), serial and macAddresses (both optional) already hold exactly the tuple "Nodes" defines, and optional fields are the right shape for a key that can be weak. Approved on TalosMachineSpec already carries the approval gate TalosMachineReconciler.Reconcile already enforces (its first line: an unapproved machine is left untouched, no status write, nothing). Nothing needed adding to either.

role, on the other hand, did not fit. It was +required with an enum of controlplane/worker, but discovery creates the TalosMachine before approval assigns a role ("Nodes": approval "sets the role, labels, disk roles and basic networking"). A required field with no value yet is a type that cannot represent the state this phase needs to create. role is now +optional; nothing else on the identity or approval types changed.

Decision

1. TalosMachineSpec.Role becomes optional. A discovered, unapproved machine has no role until approval sets one; pkg/render already fails closed on an empty role (via machine.ParseType), so an operator who approves without setting a role gets a clear RenderFailed condition rather than a silently wrong config.
2. DiscoveryReconciler.Discover(ctx, cluster, endpoint) reads identity over a MaintenanceClient (a new, three-line interface mirroring TalosClient: one method, Identity, dialed through client.WithMaintenanceMode -- the same insecure, unauthenticated connection "Nodes" specifies) and classifies what it finds:
   • Weak (strongIdentity is false: UUID, serial or every permanent MAC missing) -- never auto-registered. Discover records a WeakIdentity Warning Event on the TalosCluster (address, whatever of UUID/serial/MACs/manufacturer were read) and creates nothing.
   • Strong, UUID unseen -- creates a new TalosMachine named after the UUID, Approved: false, Role unset.
   • Strong, UUID already named by an existing TalosMachine with the same serial and MAC set -- the same machine returning. Discover returns the existing object; nothing is created or modified.
   • Strong, UUID already named by an existing TalosMachine with a different serial or MAC set -- a collision "Nodes" says Ichor cannot resolve by itself (the UUID "may repeat"). Discover records an IdentityCollision Warning Event and leaves the existing machine untouched; nothing new is created.
3. The object's name is the UUID itself. SMBIOS UUIDs are already lowercase, hyphenated hex -- a valid Kubernetes name -- and using it turns "does a TalosMachine already exist for this identity" into a single Get, which is also what makes the collision case detectable at all: a List-and-filter approach would have no single place to apply "same name, different hardware."
4. Neither outcome ever sets Approved. The configuration reconciler's existing first line -- untouched by this change -- is therefore the only place that decides whether a machine gets configuration, and it already refuses anything unapproved.

Consequences

• No new CRD, no new abstraction beyond the two small interface types (MaintenanceClient, HardwareIdentity) talosclient.go already has a direct precedent for.
• A weak or colliding identity leaves no TalosMachine behind at all. The only record is the Event on the TalosCluster. An operator resolving it has to go look at cluster Events, not a dedicated list -- an acceptable gap for this phase, since the approval UI that would normally surface this is explicitly later work ("Nodes", "Roadmap" phase 5).
• Because discovery never sets Approved and never loads the secrets bundle, it cannot be the path through which an unintended node gets configuration: the only writer that applies anything is TalosMachineReconciler, gated on Approved, which this phase does not touch.
• Discovery's candidate-selection question stays open. Whoever answers it next (registration CLI, subnet scanner, or both) implements it as a caller of Discover, not a change to Discover itself.

A second finding in the same file: the bundle Secret's namespace

While working in internal/controller/talosmachine_controller.go, a cluster-wide ClusterRole rule granting unrestricted get;list;watch on every Secret in every namespace was found reintroduced by commit 645f5be ("feat(controller): reconcile TalosMachine configuration"), which added loadBundle's read of TalosCluster.spec.secretsRef. Commit 823b04f had already removed an identical rule once, for a different Secret (ichor-webhook-server-cert), by moving it into a namespace-scoped Role. 645f5be's marker --

// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch

-- regenerates the same cluster-wide rule, confirmed by inspecting config/rbac/role.yaml after make manifests.

Why this one could not be narrowed the same way

823b04f's fix works because ichor-webhook-server-cert always lives in the controller's own namespace. The bundle Secret did not: loadBundle resolved it in cluster.Namespace -- whichever namespace that particular TalosCluster happens to live in -- and test/e2e/e2e_test.go creates its TalosCluster in ichor-e2e, not the controller's namespace, proving this is not hypothetical. A Role scoped to one fixed namespace can never grant access to a Secret whose namespace varies per TalosCluster; Kubernetes RBAC has no way to express "the Secret this field names, wherever its owner lives" below cluster scope.

Decision: resolve the bundle Secret in the controller's own namespace, not the TalosCluster's

### Deployment already describes the default placement:

The default is self-managed: Ichor runs inside the cluster it manages. ... A hub mode, where Ichor manages clusters from outside, is left open by the API but not built first.

One controller, one namespace, by default. Resolving secretsRef against that namespace rather than the TalosCluster's turns the open question above into a non-issue: the Secret's location is now fixed and known at manifest-generation time, so the RBAC marker can carry namespace=ichor-system (the same literal the webhook certificate markers already use) and make manifests emits a namespace-scoped Role, not a ClusterRole.

This is a change in meaning for a public field, not just an implementation detail: TalosClusterSpec.SecretsRef used to mean "the Secret in this TalosCluster's namespace" and now means "the Secret in the controller's own namespace." The TalosCluster itself is unaffected -- it may still live in any namespace the controller watches; only the bundle Secret's location changed. The field's doc comment in api/v1alpha1/taloscluster_types.go says so.

loadBundle takes the controller's namespace as an explicit parameter (TalosMachineReconciler.ControllerNamespace, set from POD_NAMESPACE in cmd/main.go, the same downward-API variable the webhook certificate rotator already uses) and fails outright if it is empty, rather than letting an empty namespace resolve against every namespace by accident.

dist/chart/chart_test.go gained a standing guard, checkNoClusterWideSecretsRead, asserting that no rendered ClusterRole grants get, list or watch on secrets without resourceNames. Run against the regression this ADR describes (the marker without namespace=), it fails with:

ClusterRole "ichor-manager-role" grants [get list watch] on secrets across every namespace with no
resourceNames; the bundle Secret is resolved in the controller's own namespace (ADR 0011), so this
permission must live in a namespace-scoped Role instead

rbac.namespaced's default is unchanged (false): that toggle scopes every rule in manager-role, not only the secrets one, to the release namespace, and nothing about this decision needs the rest of the controller's RBAC narrowed.

Consequence for hub mode

This decision is deliberately tied to the self-managed default. If hub mode (TalosCluster objects managed from outside the clusters they describe, "Deployment") is ever built, "the controller's own namespace" stops being a safe place to assume every managed cluster's bundle lives -- a hub managing clusters across teams or tenants is exactly the case a single shared namespace of Secrets was meant to avoid. Hub mode's design has to revisit this RBAC shape explicitly: a namespace-scoped Role per managed namespace (one per tenant), or an explicit opt-in ClusterRole for operators who accept the wider blast radius -- not a silent cluster-wide grant by default. This is called out here so that whoever designs hub mode does not have to rediscover it.

A third finding: role must not be silently defaulted

TalosMachineSpec.Role becoming optional (this ADR's main decision) raised the question of what happens when an approved machine still has no role -- discovery never sets one, and approval is not built yet. The assumption when Role was loosened was that pkg/render's existing machine.ParseType call would fail closed on an empty string.

It does not. Talos machinery's machine.ParseType("") returns TypeWorker, nil -- not an error -- because it exists to parse config documents, where an absent machineType field has always defaulted to worker. Used on TalosMachineSpec.Role directly, that default is wrong: an approved machine with no role would have silently rendered (and applied) a worker configuration instead of failing visibly.

pkg/render.Render now checks m.Spec.Role == "" explicitly, ahead of ParseType, and fails with "render: machine role is not set". The reconciler's existing RenderFailed condition path (unchanged) is what surfaces this to an operator, the same way any other render failure does. pkg/render/render_test.go's TestRenderBundleLeak gained an "empty role" case alongside its existing "unparseable role" one, confirming the explicit failure and that no bundle material leaks into its message.

Enforcing this earlier -- at admission, when a machine is approved without a role -- was considered and not done: TalosMachineCustomValidator is registered for verbs=delete only (ADR 0006, guarded by chart_test.go's checkWebhookInvariants), and widening it to create/update is a bigger, deliberate scope change that ADR made on purpose. The render-time check is sufficient for this phase's guarantee -- nothing ever applies a worker configuration to a machine that was not explicitly given that role -- without reopening ADR 0006.

Discovery probes only declared ranges

This ADR's opening section left one question unanswered on purpose: where a candidate endpoint for Discover comes from. It is answered now.

Decision: TalosCluster.spec.discovery declares addresses and CIDRs; empty means off

spec.discovery (TalosClusterDiscovery) holds two optional lists, addresses and cidrs. Both empty -- the default, on every TalosCluster that doesn't set the field at all -- means discovery is disabled: nothing in EnumerateCandidates or DiscoverAll ever opens a connection. Ichor never discovers a range by itself. No subnet scan, no listening for a beacon, no default range: every candidate traces back to something an operator wrote into the CR. EnumerateCandidates pairs each declared address with Talos's fixed apid port (constants.ApidPort, 50000) -- the only port this ever dials; DiscoverAll then calls the existing Discover once per candidate, unchanged from this ADR's first section.

Three reasons, the third the deciding one:

1. A CR per machine would make creating the CR the approval. "Nodes" describes a pending-approval list a human acts on; a machine has to be able to appear on it without a human operation. A declared range (one CR, written once, well before any specific machine exists) keeps discovery and approval two separate steps; a CR-per-candidate scheme collapses them into one.
2. An unbounded scan opens connections on networks nobody told the tooling about, and is unreliable besides (ARP timeouts, firewalls, mixed-MTU segments all behave differently across a scan Ichor didn't choose the shape of).
3. ### Trust already assumes a separate provisioning network exists: "A separate provisioning network is assumed; the docs say so." A declared CIDR or address list is that network, named explicitly. This decision adds no new assumption to the design -- it gives the API a place to write down an assumption the design already made.

Validation lives in the CRD, not the controller

Both fields are CEL-validated (k8s.io/apiserver's IP/CIDR CEL library, available to CRD x-kubernetes-validations by default -- confirmed against envtest's Kubernetes 1.36): addresses must each satisfy isIP(a) && ip(a).family() == 4; cidrs must each satisfy isCIDR(c) && cidr(c).ip().family() == 4 && cidr(c).prefixLength() >= 24. IPv6 is rejected on both -- consistent with the design's own non-goals ("IPv6 ... Later, maybe") and avoiding a /24-sized numeric threshold that would mean something absurd for IPv6 (a /24 of IPv6 is still 2^104 addresses). family() and prefixLength() return CEL int, not string, which is not obvious from the function names and cost an iteration to find (envtest rejected == '4' with a type error before accepting == 4).

A naive self.all(...) rule on an unbounded list is itself rejected by the API server's own CEL cost estimator ("estimated rule cost exceeds budget"), because the estimator assumes worst-case string length when none is declared. +kubebuilder:validation:items:MaxLength (not plain MaxLength, which controller-gen refuses on a slice field: "must apply maxlength to a textual value, found type array") on each list brings the estimate back under budget.

Why a /24 cap on cidrs, and why a cap at all

A cap exists. cidrs accepts at most four entries (MaxItems=4), each /24 or smaller (prefixLength() >= 24, so at most 256 addresses); addresses accepts at most 64 entries directly. A /16 is 65536 addresses and a /8 is 16777216 -- both large enough to turn one TalosCluster edit into a connection-attempt budget no fixed concurrency and timeout combination keeps politely bounded, and both far bigger than any single provisioning network a rack or two of hardware actually needs. Capped and rejected at admission (not accepted and left to the controller to cope with) because the CRD is the layer that can refuse before anything runs, the same reasoning MaxItems already uses elsewhere in this schema.

Worst case with the caps: 64 + 4×256 = 1088 candidates.

Rate limiting: fixed values, not settings

discoveryConcurrency = 32 and discoveryDialTimeout = 2 * time.Second, both in internal/controller/discovery.go, neither exposed as a field anywhere -- nobody asked for that knob, and a fixed worst case is the property that matters, not tunability. The per-candidate deadline covers the whole of one Discover call: the insecure maintenance-mode dial and identity read, and the Kubernetes API Get/Create that follow for a strong identity. That is deliberate, not an oversight: an earlier version bounded only the dial, at 750ms, and go test's own parallel package run (every package's envtest instance competing for the same CPU) occasionally made the Kubernetes API calls alone take longer than that, which DiscoverAll then silently treated as "unreachable" -- turning ordinary API latency into a lost discovery. 2s absorbs that without meaningfully changing the worst case: 1088 candidates / 32 workers × 2s ≈ 68 seconds, still well short of "hangs."

Silence is the point, not a gap

DiscoverAll does not log or event per unreachable candidate, and neither does Discover's own connect/read error paths (checked again here: they return a plain error, nothing is logged on that branch). A declared provisioning network answers on a handful of addresses and says nothing on the rest, by construction -- that is what "separate provisioning network" means in practice, not every address having a machine behind it. Reporting every miss would make the one candidate worth an operator's attention indistinguishable from a thousand that were never going to answer.

Tests never reach a real socket

Every test in internal/controller/discovery_test.go supplies its own MaintenanceClient factory; none ever constructs NewMachineryMaintenanceClient, the only thing in this codebase that calls client.WithMaintenanceMode and actually dials. Addresses used in tests are RFC 5737 TEST-NET-1 (192.0.2.0/24) and TEST-NET-2 (198.51.100.0/24) -- documentation-only ranges that are never real networks -- on top of the factory-level guarantee, not instead of it. The concurrency-bound and timeout tests simulate latency with time.Sleep and <-ctx.Done(), never a dial.

What is still open

DiscoverAll is not wired into cmd/main.go or any ctrl.Manager trigger. Which candidates to probe is now decided; when DiscoverAll runs for a given TalosCluster -- on a timer, on a watch event, from a CLI command -- is a separate question this phase still does not answer, for the same reason the original endpoint question was deferred: it is its own design decision, not implied by anything settled here.

• Addendum (ADR 0086): the same identity answering in maintenance mode at a new address is recorded in status.observedEndpoint, never in spec.endpoint.
• Addendum (ADR 0108): a TalosMachine's spec.endpoint must be an address of spec.discovery (or the user needs update on talosmachines/endpoint), and approving needs approve on talosmachines/approval. Discovery-created machines are unaffected.