Skip to content

feat: give workloads an interface they can claim - #360

Draft
scotwells wants to merge 16 commits into
mainfrom
feat/164-network-interface-claims
Draft

feat: give workloads an interface they can claim#360
scotwells wants to merge 16 commits into
mainfrom
feat/164-network-interface-claims

Conversation

@scotwells

Copy link
Copy Markdown
Contributor

Compute asks for a network interface the way a pod asks for storage. It declares what an interface needs, and gets back an object carrying everything required to bring up a NIC.

Related to #164. The compute side has to move before that issue closes.

What a consumer writes

A workload declares its interface once, and every replica gets one of its own:

networkInterfaceClaimTemplates:
  - metadata:
      name: eth0
    spec:
      network:
        name: default
      ipFamilies:
        - IPv6
        - IPv4
      reclaimPolicy: Retain
      addresses:
        - class: public-unicast-ipv4

Nothing here names a subnet, a prefix length, or an address. A consumer names a kind of address and the platform decides the rest. This is the same shape a StatefulSet uses for storage, and it behaves the same way: one declaration, one claim per slot, each outliving the instance that fills it.

What they get back

The addresses land on the object compute already watches, so answering "what address did this instance get" costs no second lookup:

status:
  addresses:
    - family: IPv6
      address: 2001:db8:a001::/96
      primary: true
    - family: IPv4
      address: 10.128.0.2/32
  externalAddresses:
    - family: IPv4
      address: 198.51.100.11
  conditions:
    - type: Bound       status: "True"
    - type: Allocated   status: "True"
    - type: Programmed  status: "True"
    - type: Ready       status: "True"

Today Instance declares a field for the address, prints it as a column, and never fills it in. A consumer running a get sees a blank where the answer belongs. This fills it in.

Why allocation and programming are reported apart

An address that exists is not an address that carries traffic. Allocation happens as soon as a claim exists. Programming waits for the data plane. Reporting them together would release an instance before its packets can move, so they stay separate and readiness needs both.

What a provider reads

One object, no traversal:

spec:
  network:
    name: default
  interfaceName: eth0
  mtu: 1460
  addresses:
    - family: IPv6
      address: 2001:db8:a001::/96
      gateway: 2001:db8:a001::1
      primary: true
    - family: IPv4
      address: 10.128.0.2/32
      gateway: 10.128.0.1

A provider bringing up a NIC needs the address, the gateway, the MTU, and the network. Today it assembles those by walking four resources, which means those four cannot change without breaking every provider, and a new provider has to learn all of them to bring up one interface.

An address that survives its instance

An interface outlives the instance holding it. Under a retain policy, deleting a workload's claim leaves the interface in place still holding its addresses, and a replacement of the same slot binds it again and comes back on the same address.

Event Delete Retain
Instance replaced or rescheduled same interface, same addresses same
Scale down then back up new interface, new addresses same interface, same addresses
Workload deleted then recreated new interface, new addresses same interface, same addresses

An interface survives a redeploy on its own. Surviving a scale down takes retain.

What stops being anyone else's problem

  1. Compute no longer creates network bindings or subnet claims, and no longer needs to know a network context exists.
  2. Every instance gets its own interface and its own addresses. Today one allocation is shared across a whole deployment, so instances cannot have distinct addresses.
  3. Providers read one resource rather than four.

When it cannot be satisfied

A claim that cannot bind says why. An exhausted pool, a family the network does not carry, and a namespace that names no project each report the cause rather than a generic not ready, because an instance stuck behind an unsatisfiable claim is a new way to be stuck and it needs to be legible.

Testing

The suites run against a real IPAM service in a two cluster environment, not a mock. Each one checks the addresses the operator publishes against the allocations IPAM actually holds, so a controller that invented an address would fail them. Two projects allocate through identical class names into separate address space, and neither can see the other's allocations.

Running IPAM for real found four defects that a mock would have passed, including one that leaked addresses and one that wedged a claim permanently. Each is fixed here.

Scope

The data plane path and the attachment that realizes an interface are a separate change. The programmed condition is defined and stays unknown until then. Nothing is removed: network bindings, subnet claims and subnets keep working, and providers move over on their own schedule.

The generated CRD reference had drifted from the types: Domain was missing
its refresh-attempt fields, HTTPProxy and TrafficProtectionPolicy were
several changes behind, and Connector, ConnectorAdvertisement,
ConnectorClass and LocationBinding had no generated page at all.

Produced by the generator with no hand edits. Separated from the work that
surfaced it so the diff stays reviewable.
Compute asks for an interface the way a pod asks for storage: it writes a
claim naming a network and what the interface needs, and the operator binds
it to an interface carrying everything required to configure a NIC.

Two properties follow. Compute stops reaching into networking internals, and
an interface becomes a thing that outlives the instance using it, which is
what makes a per-instance address and a retained address possible at once.

Key changes:
- NetworkInterfaceClaim states intent: network, families, extra addresses by
  class, and reclaim policy. It carries no location; the cell it lives in
  serves one.
- NetworkInterface is the answer: addresses, gateway, MTU, and the claim
  holding it. Addresses live in spec because they are the desired NIC
  configuration, not an observation.
- A claim binds once and never re-matches. The uid on claimRef distinguishes
  a claim from a later one of the same name, which is what lets a retained
  interface return to Available without a manual clearing step.
- NetworkInterfaceClaimTemplate lets a workload declare an interface once
  and materialize one claim per slot, as a StatefulSet does for storage.
- Allocated and Programmed are reported separately, so an instance is never
  told its network is ready before it can carry traffic.

Includes the enhancement document describing the model.
Binds a claim by allocating real addresses from the IPAM service, then
publishing an interface carrying them. Allocation is synchronous, so a
bound claim reports its addresses in the same reconcile.

The project an allocation belongs to is derived from the namespace the
claim lives in, and asserted per request by impersonation against one
shared upstream connection. A namespace naming no project allocates
nothing and says which label is missing, because allocating into the wrong
project is the worst outcome available here.

Key changes:
- One IPClaim per requested family and per named class, each deterministically
  named from the claim so a replacement instance finds the addresses that
  already exist.
- Allocation is all or nothing. A partial failure releases what it took
  rather than leaking it.
- Reclaim policy governs release: Delete removes the interface and its
  addresses, Retain returns the interface to Available still holding them,
  and a later claim of the same name rebinds it.
- Adoption is refused when the interface does not satisfy the claim, or
  holds a different reclaim policy, rather than reporting success over a
  partial interface.
- A finalizer on the interface releases its addresses when it is deleted
  directly, and a watch rebuilds an interface deleted under a live claim.
- Failures name what happened: an exhausted pool, a family the network does
  not carry, or an unresolvable project.
- An address whose allocation has vanished raises an event and a metric
  rather than renumbering a running workload.
The integration is not proven until a claim in a downstream control plane
gets a real address back from a real IPAM service. These suites assert
against IPAM directly rather than against the operator's own status, so a
controller that invented addresses would fail them.

Key changes:
- Deploy IPAM and its database onto the upstream cluster, gated on the
  APIService becoming available so a suite cannot start while it settles.
- Seed classes and pools for two projects with identical class names and
  disjoint ranges, so a routing bug shows up as the wrong address space
  rather than as a passing test.
- Wire a per-project impersonation kubeconfig, the same mechanism
  production uses, and verify the namespace labels round trip through the
  real decoder.
- Clear cluster scoped fixtures before every run and fail loudly when the
  clearing cannot happen, so a run that died mid-suite cannot poison the
  next one.
- Cover dual stack binding, project isolation, a namespace naming no
  project, a named class, both reclaim policies, interface deletion in both
  orders, and a family the network does not carry.
- Every assertion of an absence carries a positive control, so a suite
  cannot pass by failing to reach IPAM at all.
The claim controller recorded events through GetEventRecorderFor, which is
deprecated and fails CI lint under staticcheck SA1019. The replacement is not
drop-in: it posts through EventsV1Interface, so events land in the
events.k8s.io group rather than the core group and the existing RBAC grant no
longer authorizes them.

Key changes:
- record events through GetEventRecorder, carrying the interface as the
  related object now that the API supports one
- grant events.k8s.io/events instead of core events, since the core grant
  authorizes nothing on the new path
- assert the grant names events.k8s.io, so a rule left on the core group
  fails rather than passing while the call is forbidden
- post one event through a real broadcaster against envtest, which catches
  an event the API server rejects; a fake recorder accepts any action,
  including the empty one events.k8s.io/v1 forbids
The field read as though each family could have its own primary. An
interface holds at most one address per family, so what primary marks is
which family leads, and there is one for the whole interface.

Wording only. No behaviour or validation changes.
IPAM records an address's reclaim policy when the address is allocated and
decides its fate from that record, never re-reading the claim. Rewriting the
claim afterwards changed nothing, so three code paths existed to close a leak
they could not close.

In the release path the write was worse than useless: a transient failure on
it skipped the delete that follows, leaving the address held forever.

What still protects the address is unchanged: the reclaim policy is immutable
on a NetworkInterfaceClaim, and an interface is not adopted by a claim asking
for a different policy.

Key changes:
- set the reclaim policy when the address is claimed, and never after
- release addresses directly instead of rewriting a policy first
- keep reporting an address whose allocation has gone, which is the check
  that proved valuable
- drop tests and fakes covering the removed writes
The IPAM deployment vendored ten manifest files copied out of the pinned
module, guarded by a drift-check task. Take them from the bundle
milo-os/ipam publishes on every push to main instead, pinned by digest
and verified at fetch time to carry the commit go.mod pins, so the
manifests and the Go types NSO compiles against cannot drift apart.

Relocate the whole thing to config/dependencies/ipam, establishing
config/dependencies for services NSO depends on but does not own.

The published image is NOT used. ghcr.io/milo-os/ipam advertises an index
with linux/amd64 and linux/arm64, but both entries point at one layer and
that layer holds an x86-64 binary. On arm64 it runs under emulation and
panics inside pgx on the DELETE path, giving an apiserver that serves for
minutes and dies mid-suite. The image is still built locally until that
is fixed upstream.

Key changes:
- Fetch the bundle with crane, pinned by digest, and verify its revision
  annotation names the commit go.mod pins
- Add crane to the pinned tools; it installs with go install, which the
  flux CLI's replace directives prevent
- Split config/dependencies/ipam into overlay/ (self-contained, so
  validate-kustomizations builds it on a clean checkout) and
  root-kustomization.yaml (composed at task time in a staging directory)
- Delete the vendored manifests and the vendor-drift check
- Warn on the suite-running tasks that concurrent invocations are unsafe:
  the interface suites use fixed names in shared namespaces
An interface recorded the UID of the claim holding it, but nothing compared
it when deciding which claim a bound interface belonged to. The one place
that did read it guarded a race that cannot happen, because a claim carrying
a finalizer blocks a second claim of the same name from existing.

A stale UID could only ever read as "no claim holds this", which releases an
address a running workload is still using. A name comparison cannot fail that
way.

Key changes:
- match a bound interface on the claim name
- treat an interface as held when a claim of that name exists and is not
  being deleted, so a deleted claim's addresses are still released
An interface recorded the uid of the claim holding it, copying the field a
persistent volume uses to detect a claim deleted and recreated under the
same name. That detection exists to move a volume to Released, a state this
model deliberately does not have.

Nothing consulted it when binding. A claim name identifies the slot an
interface serves, and a claim carrying a finalizer keeps its name reserved
until its release completes, so a later claim of that name is always the
same slot asking again.
The claim's status embeds the same address type as the interface, so the
earlier wording change left its generated output behind.
The uid removal left a stale fragment in the generated reference.
config/dependencies/ipam held test data alongside deployment
configuration. The IPClass/IPPool seeds, the ipam-e2e-* namespaces and
the identity the suites impersonate exist for the e2e suites and are
consumed only by e2e runs, so they move next to those suites. Nothing
about deploying IPAM needs them.

Also fixes the CI failure from the OCI change. crane was added to the
tools task, but the workflow running this suite installs its binaries
from an explicit list and `up` never depends on tools, so the fetch
worked only on machines that already had crane. It is now installed on
demand inside the fetch path, which fixes every caller including a fresh
clone, and both e2e workflows now install tools the same way.

Key changes:
- Move fixtures and namespaces.yaml to test/e2e/fixtures/ipam, a
  directory with no chainsaw-test.yaml, which chainsaw walks past
- Split the e2e-tenant-tester binding out of the deployment's RBAC into
  the fixtures, bound to the operator's own role rather than restating it
- Add ipam-tenant-rbac so anything impersonating the tester publishes
  that grant first; ipam-deploy prunes it from the deployment binding
- Install crane on demand; unify test-e2e.yml onto task test-infra:tools
- Widen the concurrency warnings: a bare fixture task pulls the seeds and
  the tenant RBAC out from under a suite already running
Events outlive a run by about an hour and the claim names are fixed, so an
event left by an earlier run could satisfy the next one. The assertion would
have kept passing the day the controller stopped reporting.

Clears the object's events before forcing the reconcile. Also points the
range table reference at its new home.
The database was a plain Deployment, chosen because this env runs no
Flux and IPAM ships CNPG as a Flux HelmRelease. Take the chart through
kustomize --enable-helm instead, the way cert-manager is installed here,
and declare the database as a CNPG Cluster. IPAM installs the operator
and leaves the Cluster to the consumer, so that resource is ours.

Pinned to chart 0.29.0 (CNPG 1.30.0) rather than tracking a range: a CI
run should not change because upstream published a release.

The operator immediately caught a misconfiguration the Deployment
tolerated silently — a memory request below shared_buffers, which the
old spec carried unnoticed.

Key changes:
- Install the operator at config/dependencies/postgres-operator, mirroring
  IPAM's own layout, with kubeVersion declared so the chart's >=1.29
  constraint is satisfied when rendering offline
- Declare Cluster ipam-db, bootstrapped from the secret IPAM already
  reads, so its DSN stays a constant instead of being assembled from
  CNPG's generated credentials
- Gate on the CRD, the operator rollout, and a server-side dry run that
  proves the webhook admits, before any Cluster applies
- Replace the postgres readiness gate with the Cluster's Ready condition
  plus a real pg_isready as the role IPAM connects as; the migrate init
  container needs a database accepting connections, not a scheduled pod
Two consecutive CI runs failed this scenario at 420.84s, twice, which is
the budget rather than variance. Every assertion passed; the failure is
in cleanup, and the dump shows the downstream gateway still actively
reconciling the scenario's Gateways when chainsaw gave up, so the drain
was incomplete rather than blocked.

The 420s figure was calibrated before this environment gained a database
operator and its pod, on a runner that already hosts two kind clusters.
Nothing about the scenario changed; what it shares the runner with did.

Key changes:
- Raise cleanup to 900s. The budget is a ceiling and not a cost, so it
  is set well clear of the worst drain seen rather than just above it —
  this is the second time 420s has been exceeded and a third
  recalibration would say more about the number than the drain
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant