From e40e699e5f3e00f1305a32aa99c3f02edb7374fa Mon Sep 17 00:00:00 2001 From: joshsiv-mulga Date: Sun, 11 Oct 2026 09:47:40 +1000 Subject: [PATCH] refactor: inject vpcd's IMDS instance-state readers from the instance state boundary --- architecture/instancestate/allowlist_test.go | 11 +++-- architecture/layering/allowlist_test.go | 1 + docs/PACKAGE_BOUNDARY_MIGRATION.md | 7 +-- .../ec2/guestmetadata/instance_lookup.go | 12 +++--- spinifex/domains/ec2/guestmetadata/service.go | 2 +- .../domains/ec2/instance/state/export_test.go | 24 +++++++++++ .../ec2/instance/state/imds_readers.go} | 43 ++++++++----------- .../ec2/instance/state/imds_readers_test.go} | 22 +++++----- spinifex/operator/cli/service.go | 3 ++ spinifex/vpcd/export_test.go | 24 ----------- spinifex/vpcd/vpcd.go | 19 +++++--- 11 files changed, 87 insertions(+), 81 deletions(-) create mode 100644 spinifex/domains/ec2/instance/state/export_test.go rename spinifex/{vpcd/imds_instance_state.go => domains/ec2/instance/state/imds_readers.go} (50%) rename spinifex/{vpcd/imds_instance_state_test.go => domains/ec2/instance/state/imds_readers_test.go} (75%) delete mode 100644 spinifex/vpcd/export_test.go diff --git a/architecture/instancestate/allowlist_test.go b/architecture/instancestate/allowlist_test.go index 3a64de5c3..875d8f737 100644 --- a/architecture/instancestate/allowlist_test.go +++ b/architecture/instancestate/allowlist_test.go @@ -68,12 +68,11 @@ var allowlist = []allowance{ {File: "spinifex/daemon/eni_orphan_reaper.go", Symbol: "JetStreamManager.ListInstanceRecords", Inv: "INV-12"}, {File: "spinifex/daemon/eni_orphan_reaper.go", Symbol: "JetStreamManager.ListTerminatedInstanceRecords", Inv: "INV-12"}, - {File: "spinifex/vpcd/imds_instance_state.go", Symbol: "daemon.LocalState", Inv: "INV-13"}, - {File: "spinifex/vpcd/imds_instance_state.go", Symbol: "daemon.LocalStatePath", Inv: "INV-13"}, - {File: "spinifex/vpcd/imds_instance_state.go", Symbol: "daemon.ReadLocalState", Inv: "INV-13"}, - {File: "spinifex/vpcd/imds_instance_state.go", Symbol: "daemon.InstanceStateBucket", Inv: "INV-13"}, - {File: "spinifex/vpcd/imds_instance_state.go", Symbol: "JetStreamManager.InitKVBucket", Inv: "INV-13"}, - {File: "spinifex/vpcd/imds_instance_state.go", Symbol: "vm.InstanceRecord", Inv: "INV-13"}, + {File: "spinifex/domains/ec2/instance/state/imds_readers.go", Symbol: "daemon.LocalState", Inv: "INV-13"}, + {File: "spinifex/domains/ec2/instance/state/imds_readers.go", Symbol: "daemon.LocalStatePath", Inv: "INV-13"}, + {File: "spinifex/domains/ec2/instance/state/imds_readers.go", Symbol: "daemon.ReadLocalState", Inv: "INV-13"}, + {File: "spinifex/domains/ec2/instance/state/imds_readers.go", Symbol: "daemon.InstanceStateBucket", Inv: "INV-13"}, + {File: "spinifex/domains/ec2/instance/state/imds_readers.go", Symbol: "JetStreamManager.InitKVBucket", Inv: "INV-13"}, {File: "spinifex/domains/ec2/guestmetadata/instance_lookup.go", Symbol: "vm.InstanceRecord", Inv: "INV-14"}, {File: "spinifex/domains/ec2/guestmetadata/instance_lookup.go", Symbol: "vm.VMFromRecord", Inv: "INV-14"}, diff --git a/architecture/layering/allowlist_test.go b/architecture/layering/allowlist_test.go index 2b3877f46..4a75e158b 100644 --- a/architecture/layering/allowlist_test.go +++ b/architecture/layering/allowlist_test.go @@ -67,6 +67,7 @@ var allowlist = []allowance{ {From: "domains/ec2/instance", To: "domains/network/topology", Debt: "EC2 → network implementation", Reason: "EC2 reads network implementation; clears with the EC2-owned projection"}, {From: "domains/ec2/instance", To: "runtime/compute/gpu", Debt: "domains/ec2/instance", Reason: "VM/GPU runtime state read directly; needs a domain-owned projection"}, {From: "domains/ec2/instance", To: "runtime/compute/vm", Debt: "domains/ec2/instance", Reason: "VM/GPU runtime state read directly; needs a domain-owned projection"}, + {From: "domains/ec2/instance/state", To: "daemon", Debt: "domains/ec2/instance/state (dependency, IMDS readers)", Reason: "wraps the daemon's local-file reader and JetStreamManager bucket open unchanged for vpcd's IMDS lookup"}, {From: "domains/ec2/instance/state", To: "runtime/compute/vm", Debt: "domains/ec2/instance/state (dependency)", Reason: "the record codec wraps the legacy vm.InstanceRecord alias and its spec and status types"}, {From: "domains/ec2/volume", To: "runtime/compute/vm", Debt: "Domain → runtime", Reason: "VM runtime state read directly; needs a domain-owned projection"}, diff --git a/docs/PACKAGE_BOUNDARY_MIGRATION.md b/docs/PACKAGE_BOUNDARY_MIGRATION.md index 45fc8b6c8..4dfe1a0e4 100644 --- a/docs/PACKAGE_BOUNDARY_MIGRATION.md +++ b/docs/PACKAGE_BOUNDARY_MIGRATION.md @@ -334,6 +334,7 @@ while the branch is in flight; they do not describe the umbrella repository's | `8db9771fb` | `spinifex/runtime/compute/vm/record_codec.go` | `spinifex/domains/ec2/instance/state` | The codec moves unchanged to the package that is the eventual owner of the persisted instance record and its evolution; `vm.InstanceRecord` and its spec and status types stay in `runtime/compute/vm` as the temporary legacy types it wraps. The domain-to-runtime import is recorded below as `domains/ec2/instance/state` (dependency) debt and allow-listed against it; the instance-state ratchet names the new owner file and its allowlist is unchanged. Full preflight passed except the recorded Go 1.27.0 advisories. | | `c617914a1` | (codec behaviour only) | — | A changed array whose elements as read carry an unknown member at any depth is refused with `ErrRecordExtensionConflict` instead of being replaced and losing the member; unchanged arrays and changed arrays without element extensions behave as before. Still no production caller. Full preflight passed except the recorded Go 1.27.0 advisories. | | `0d7b4266d` | (codec behaviour only) | — | The extension-conflict refusal becomes uniform: a capable writer may not remove or replace a known object, map entry or array whose stored value carries unknown structured extension fields, and `Encode` returns `ErrRecordExtensionConflict` naming the changed container and the extension. Removal without extensions stays allowed. Still no production caller. Full preflight passed except the recorded Go 1.27.0 advisories. | +| `c6a0a3ba8` | `spinifex/vpcd/imds_instance_state.go` | `spinifex/domains/ec2/instance/state/imds_readers.go` | The local-file reader (`LocalVMReader`/`NewLocalVMReader`, formerly `localVMStateReader`) and the instance-record loader opener (`OpenInstanceRecordLoader`, formerly `newInstanceRecordLoader`) move unchanged to the instance-state boundary; `vpcd.Config` gains `LocalVMs guestmetadata.LocalStateReader` and `OpenInstanceRecords func(ctx, nc) (guestmetadata.RecordLoader, error)`, wired by `operator/cli` with `state.NewLocalVMReader(nodeConfig.DataDir)` and `state.OpenInstanceRecordLoader`, so `vpcd` no longer imports `daemon` directly. `guestmetadata`'s own narrow interfaces are exported (`LocalStateReader`, `RecordLoader`) with no semantic change so `vpcd.Config` can name them. The opener's absence is now a fatal startup error rather than an unchecked call, matching the existing mandatory-open behaviour. The instance-state ratchet's INV-13 entries move with the file; `OpenInstanceRecordLoader` returning `guestmetadata.RecordLoader` directly (so the two packages' function types match exactly) removes the file's local `recordLoader` interface and its `vm.InstanceRecord` reference, so INV-13 now cites 5 symbols rather than 6. `vpcd` still runs the legacy bucket-opening path (`InitKVBucket`, the version gate and the instance migrations) through the injected opener; that is ADR-0007 debt (INV-13) and is deliberately not fixed here. Full preflight passed except the recorded Go 1.27.0 advisories. | The EC2 contract row is intentionally different from the directory moves: it creates a compatibility boundary. `ec2.cmd.*` retains its deployed one-token NATS @@ -407,7 +408,7 @@ These residuals were recorded by individual slices and must be resolved once the - `handlers/sts/oidc_jwks.go:14` imports `handlers/eks` for `AccountBucketName` (line 101) and `OIDCJWKSKey` (line 110), the Q-22 STS-to-EKS dependency ADR-0001 prohibits in its present form. Removal condition: an EKS-owned OIDC projection or an identity-owned contract that STS consumes instead of the EKS implementation package. - `handlers/elbv2/{agent_types,haproxy,health_checker,nginx}.go` import `lbagent` (`HealthReport`, `ServerStatus`, `CertDir`, `NginxPIDPath`, `NginxCertDir`), a domain reading an agent's implementation package rather than a declared wire contract. Removal condition: the health-report and cert/PID-path vocabulary moves behind a guest/controller contract the domain and the agent both consume. - `handlers/iam/service_impl.go:30` imports `admin` for `GenerateAWSSecretKey` (line 641) only, a domain-to-operator edge; the `admin.AccountID`, `admin.AccountName` and `admin.EncryptedSecret` uses are fields of a local `*AdminBootstrapData` parameter, not the package. It clears with the remaining `admin` entry, once the key generators have an IAM-credentials owner. -- `vpcd/imds_instance_state.go:10` imports `daemon` for `LocalState`, `ReadLocalState`, `LocalStatePath`, `NewJetStreamManager` and `InstanceStateBucket`, a guest-facing daemon component reading the daemon's own implementation rather than a declared local-state contract. Removal condition: a daemon-owned local-state capability or contract that vpcd consumes instead of the `daemon` package directly. +- `domains/ec2/instance/state/imds_readers.go` (dependency, IMDS readers) imports `daemon` for `LocalState`, `ReadLocalState`, `LocalStatePath`, `NewJetStreamManager` and `InstanceStateBucket`. The local-file reader and the instance-record loader's `JetStreamManager` open, which `vpcd/imds_instance_state.go` held directly, moved here unchanged and are now injected into `vpcd` through `guestmetadata.LocalStateReader` and `guestmetadata.RecordLoader`. Removal condition: the local-file reader and the instance-record loader/bucket open move into `domains/ec2/instance/state` under ADR-0007's state work, with the daemon consuming them. - Advisory: 27 production files across `domains/{acm,dns,ec2/*,network/host,network/ovn,ochre}` import `bootstrap/config` directly rather than through typed constructor inputs, which ADR-0001's `config` row expects. `bootstrap/preflight/preflight.go:15` imports `runtime/host/command` and `services.go:13` imports `operator/host/systemd`; both are expected bootstrap-to-runtime/operator edges for startup validation and are not domain-rule violations. - `admin` (remaining): the operator-only code moved to `operator/admin`. What remains has non-operator consumers and awaits owners: `DefaultAccountID`/`DefaultAccountName` (IAM/account bootstrap), `GenerateAWSAccessKey`/`GenerateAWSSecretKey` (IAM credentials) and `PromoteSystemImage` (the existing `domains/ec2/image`). - `domains/dns`: imports `runtime/compute/vm` (domain-to-runtime), `domains/network/reconcile` for the reconcile lock and `bootstrap/config` for node configuration. @@ -547,7 +548,7 @@ Line numbers are the first use in each file. | INV-10 | `daemon/instance_recovery.go:85`–`:546` | `daemon` | daemon | Lists and decodes records, claims, releases and abandons recovery, reloads a record. Typed CAS writes. | | INV-11 | `daemon/dns_reconcile.go:38`, `:48`, `:179` | `daemon` | daemon | Constructs keys from the prefix, names the bucket, lists records for DNS. | | INV-12 | `daemon/eni_orphan_reaper.go:26`–`:27` | `daemon` | daemon | Lists live and terminated records to find orphaned ENIs. | -| INV-13 | `vpcd/imds_instance_state.go:24`–`:94` | `vpcd` | vpcd | Reads and caches the daemon's local file by path and mtime; constructs a `JetStreamManager` and calls `InitKVBucket`, which runs the version gate and the instance migrations; decodes records. | +| INV-13 | `domains/ec2/instance/state/imds_readers.go:20`–`:92` | `domains/ec2/instance/state` | vpcd | Reads and caches the daemon's local file by path and mtime; constructs a `JetStreamManager` and calls `InitKVBucket`, which runs the version gate and the instance migrations; decodes records. Injected into `vpcd` through `guestmetadata.LocalStateReader` and `guestmetadata.RecordLoader`. | | INV-14 | `domains/ec2/guestmetadata/instance_lookup.go:60`, `:120` | `guestmetadata` | vpcd (IMDS) | Loads a record through a `LoadInstanceRecord` interface satisfied by the manager and converts it to `vm.VM`. | | INV-15 | `domains/network/reconcile/intent.go:48`, `:444`–`:474` | `network/reconcile` | vpcd | Duplicates the bucket name and prefix as literals, opens the bucket by name with no gate, gets `i.` and decodes `vm.InstanceRecord`. | | INV-16 | `runtime/roles/awsgw/awsgw.go:573`–`:633` | `awsgw` | gateway | Opens the bucket by name with no migration hook for the instance cache and the quota reconcile loop. | @@ -658,7 +659,7 @@ series above; "needs decision" is open. | `gateway/tagging` | The cross-service tagging API (`GetResources`) and its authorization. | Decided: a new `domains/tagging` boundary owns the cross-resource tagging API and aggregation; individual resource domains keep ownership of their own tag storage and expose narrow tagging capabilities. Stays debt until those per-domain capabilities exist; clears when `domains/tagging` aggregates only through them. | | `services/viperblockd` | — | Done. The adapter is `providers/ebs/viperblock` (with the read-only `dirty` reader), the shared vocabulary is `contracts/viperblockd/legacy/v1`, and the role is `runtime/roles/viperblockd`. `services/` is empty. | | `utils` | — | Done and deleted. `ec2_subject.go` went to `contracts/ec2/v1`, the vpcd protocol to `contracts/network/v1` with the typed client in `domains/network/projection`, and the master-key loader to `domains/ec2/ebs/encryption`. | -| `vpcd` | `imds_instance_state.go` reads `daemon.LocalState`, `ReadLocalState`, `LocalStatePath`, `NewJetStreamManager` and `InstanceStateBucket` directly (recorded above). | Its inter-process boundary now exists (`contracts/network/v1`, with `domains/network/subscribers` as the consumer and `domains/network/projection` as the producer client). Moving `vpcd` to `runtime/roles/vpcd` remains blocked on a separate design task: a daemon-owned local-state capability or contract vpcd can consume instead of the `daemon` package. | +| `vpcd` | Nothing of its own; the IMDS local-file reader and record-space opener moved to `domains/ec2/instance/state` and are injected through `guestmetadata.LocalStateReader`/`RecordLoader` (recorded above), so `vpcd` no longer imports `daemon` directly. | Its inter-process boundary now exists (`contracts/network/v1`, with `domains/network/subscribers` as the consumer and `domains/network/projection` as the producer client), and the `vpcd` → `daemon` dependency is removed. Moving `vpcd` to `runtime/roles/vpcd` is next, pending review. | | `daemon` | Reserved as `runtime/daemon`'s home only; not split or moved. | Deferred: Q-28's write-side consolidation is a prerequisite ADR-0001 names explicitly. | | `admin` | `DefaultAccountID`/`DefaultAccountName` (consumed by `daemon`, `gateway`, `runtime/roles/awsgw`, `operator/cli`), `GenerateAWSAccessKey`/`GenerateAWSSecretKey` (`handlers/iam`, `operator/cli`) and `images_promote.go` (`daemon`, `gateway/spx`, `operator/cli`). The operator use cases moved to `operator/admin`. | Deferred to their owners: IAM/account bootstrap, IAM credentials, and the existing `domains/ec2/image`. | | `accountteardown` | Direct calls into `domains/ec2/awsapi/*` action adapters (recorded above). | Deferred: the ADR's `accountteardown` row replaces this with domain-owned cleanup capabilities. | diff --git a/spinifex/domains/ec2/guestmetadata/instance_lookup.go b/spinifex/domains/ec2/guestmetadata/instance_lookup.go index aa5334e4d..1b8df1017 100644 --- a/spinifex/domains/ec2/guestmetadata/instance_lookup.go +++ b/spinifex/domains/ec2/guestmetadata/instance_lookup.go @@ -46,17 +46,17 @@ func retryGather[T any](ctx context.Context, label, instanceID string, fn func() return out, err } -// localStateReader reads this node's on-disk VM state. Narrowed to what IMDS +// LocalStateReader reads this node's on-disk VM state. Narrowed to what IMDS // consumes: it does not know the state lives in a file, so caching and // invalidation are the implementation's business, not this package's. -type localStateReader interface { +type LocalStateReader interface { LocalVM(instanceID string) (*vm.VM, error) } -// recordLoader is the shared instance record space accessor the fallback +// RecordLoader is the shared instance record space accessor the fallback // path needs. Satisfied by *daemon.JetStreamManager; narrowed to an interface // so the fallback can be exercised without a live JetStream connection. -type recordLoader interface { +type RecordLoader interface { LoadInstanceRecord(instanceID string) (*vm.InstanceRecord, error) } @@ -65,8 +65,8 @@ type recordLoader interface { // VM state is the authoritative, instant answer. It falls back to a direct // read of the shared instance record space and never fans out over NATS. type localInstanceLookup struct { - local localStateReader // nil-safe: local hit is skipped without it - records recordLoader // nil-safe: fallback is skipped without it + local LocalStateReader // nil-safe: local hit is skipped without it + records RecordLoader // nil-safe: fallback is skipped without it } var _ instanceLookup = (*localInstanceLookup)(nil) diff --git a/spinifex/domains/ec2/guestmetadata/service.go b/spinifex/domains/ec2/guestmetadata/service.go index eb956d35d..fb78efecc 100644 --- a/spinifex/domains/ec2/guestmetadata/service.go +++ b/spinifex/domains/ec2/guestmetadata/service.go @@ -98,7 +98,7 @@ type IMDSServiceImpl struct { // nil, degrading that source to a miss rather than failing IMDS to start — // the caller (vpcd) owns their construction and availability. // ctx bounds the bucket opens only; each served request carries its own. -func NewIMDSServiceImpl(ctx context.Context, natsConn *nats.Conn, sts stsAssumer, iamSvc profileLookup, pubKeys publicKeyLookup, localState localStateReader, records recordLoader, listTaps listTapsFunc, baseDomain, internalDomain, servicesDomain, caCertPath string, resolverIPs []string, bind HostBindAddrs) (*IMDSServiceImpl, error) { +func NewIMDSServiceImpl(ctx context.Context, natsConn *nats.Conn, sts stsAssumer, iamSvc profileLookup, pubKeys publicKeyLookup, localState LocalStateReader, records RecordLoader, listTaps listTapsFunc, baseDomain, internalDomain, servicesDomain, caCertPath string, resolverIPs []string, bind HostBindAddrs) (*IMDSServiceImpl, error) { if bind.Meta == "" || bind.DNS == "" { return nil, errors.New("IMDS host bind addresses required (use NewHostBindAddrs)") } diff --git a/spinifex/domains/ec2/instance/state/export_test.go b/spinifex/domains/ec2/instance/state/export_test.go new file mode 100644 index 000000000..1a05843c5 --- /dev/null +++ b/spinifex/domains/ec2/instance/state/export_test.go @@ -0,0 +1,24 @@ +package state + +import ( + "github.com/mulgadc/spinifex/spinifex/daemon" + "github.com/mulgadc/spinifex/spinifex/runtime/compute/vm" +) + +// LocalVMReaderForTest is the local instance state reader's consumer-facing +// shape, exported so an external test can exercise it without reaching into +// package internals. +type LocalVMReaderForTest interface { + LocalVM(instanceID string) (*vm.VM, error) +} + +// NewLocalVMReaderForTest builds a reader over dataDir. A non-nil readState +// replaces the parse step, so a test can count parses and prove the +// mtime/size cache is doing its job. +func NewLocalVMReaderForTest(dataDir string, readState func(path string) (*daemon.LocalState, error)) LocalVMReaderForTest { + r := NewLocalVMReader(dataDir) + if readState != nil { + r.readState = readState + } + return r +} diff --git a/spinifex/vpcd/imds_instance_state.go b/spinifex/domains/ec2/instance/state/imds_readers.go similarity index 50% rename from spinifex/vpcd/imds_instance_state.go rename to spinifex/domains/ec2/instance/state/imds_readers.go index 499c418ce..5be4f0eac 100644 --- a/spinifex/vpcd/imds_instance_state.go +++ b/spinifex/domains/ec2/instance/state/imds_readers.go @@ -1,4 +1,4 @@ -package vpcd +package state import ( "context" @@ -8,16 +8,16 @@ import ( "time" "github.com/mulgadc/spinifex/spinifex/daemon" + "github.com/mulgadc/spinifex/spinifex/domains/ec2/guestmetadata" "github.com/mulgadc/spinifex/spinifex/foundation/state/kvstore" "github.com/mulgadc/spinifex/spinifex/runtime/compute/vm" "github.com/nats-io/nats.go" ) -// localVMStateReader adapts this node's on-disk instance state to IMDS's -// local-first lookup. vpcd is the composition root for both the network and -// compute planes IMDS straddles, so this adapter — not domains/ec2/guestmetadata — is -// what knows the state lives in a daemon-owned file. -type localVMStateReader struct { +// LocalVMReader adapts a node's on-disk instance state to IMDS's +// local-first lookup. The composition root wires it with that node's data +// directory into vpcd's guestmetadata construction. +type LocalVMReader struct { dataDir string // readState parses the local state file; overridable in tests to count @@ -30,16 +30,17 @@ type localVMStateReader struct { cached *daemon.LocalState } -func newLocalVMStateReader(dataDir string) *localVMStateReader { - return &localVMStateReader{dataDir: dataDir} +var _ guestmetadata.LocalStateReader = (*LocalVMReader)(nil) + +// NewLocalVMReader builds a reader over dataDir, this node's data directory. +func NewLocalVMReader(dataDir string) *LocalVMReader { + return &LocalVMReader{dataDir: dataDir} } // LocalVM reads the instance straight off this node's on-disk VM state, -// reusing the last parse while the file's mtime and size are unchanged. IMDS -// serves concurrent requests across per-tap responders, so this is -// mutex-guarded; a stat or parse failure clears the cache and returns the -// error rather than serving a stale hit. -func (r *localVMStateReader) LocalVM(instanceID string) (*vm.VM, error) { +// reusing the last parse while mtime and size are unchanged; concurrent +// callers are mutex-guarded, and a stat or parse failure clears the cache. +func (r *LocalVMReader) LocalVM(instanceID string) (*vm.VM, error) { r.mu.Lock() defer r.mu.Unlock() @@ -74,18 +75,10 @@ func (r *localVMStateReader) LocalVM(instanceID string) (*vm.VM, error) { return r.cached.VMS[instanceID], nil } -// recordLoader mirrors domains/ec2/guestmetadata's own narrow instance-record accessor -// interface, so a construction failure below returns a true nil interface -// rather than a *daemon.JetStreamManager typed nil boxed into one. -type recordLoader interface { - LoadInstanceRecord(instanceID string) (*vm.InstanceRecord, error) -} - -// newInstanceRecordLoader opens the shared instance record space, retrying while -// JetStream comes up. Failing here is fatal by design: without it an instance -// this node does not hold locally resolves to nothing, and IMDS serves that as -// "no such instance" — a guest that boots with no key and no error anywhere. -func newInstanceRecordLoader(ctx context.Context, nc *nats.Conn) (recordLoader, error) { +// OpenInstanceRecordLoader opens the shared instance record space, retrying +// while JetStream comes up. A failure here is fatal by design: without it an +// unresolved instance serves IMDS "no such instance" with no error anywhere. +func OpenInstanceRecordLoader(ctx context.Context, nc *nats.Conn) (guestmetadata.RecordLoader, error) { jsm, err := daemon.NewJetStreamManager(nc) if err != nil { return nil, fmt.Errorf("jetstream manager: %w", err) diff --git a/spinifex/vpcd/imds_instance_state_test.go b/spinifex/domains/ec2/instance/state/imds_readers_test.go similarity index 75% rename from spinifex/vpcd/imds_instance_state_test.go rename to spinifex/domains/ec2/instance/state/imds_readers_test.go index e09a50ec6..eaec3b9fd 100644 --- a/spinifex/vpcd/imds_instance_state_test.go +++ b/spinifex/domains/ec2/instance/state/imds_readers_test.go @@ -1,12 +1,12 @@ -package vpcd_test +package state_test import ( "os" "testing" "github.com/mulgadc/spinifex/spinifex/daemon" + "github.com/mulgadc/spinifex/spinifex/domains/ec2/instance/state" "github.com/mulgadc/spinifex/spinifex/runtime/compute/vm" - "github.com/mulgadc/spinifex/spinifex/vpcd" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -18,30 +18,30 @@ func writeInstanceState(t *testing.T, dataDir string, vms map[string]*vm.VM) { require.NoError(t, daemon.WriteLocalStateBytes(daemon.LocalStatePath(dataDir), data)) } -func TestLocalVMStateReader_MissingFileIsAMiss(t *testing.T) { - r := vpcd.NewLocalVMStateReaderForTest(t.TempDir(), nil) +func TestLocalVMReader_MissingFileIsAMiss(t *testing.T) { + r := state.NewLocalVMReaderForTest(t.TempDir(), nil) v, err := r.LocalVM("i-absent") require.NoError(t, err) assert.Nil(t, v) } -func TestLocalVMStateReader_ResolvesAKnownInstance(t *testing.T) { +func TestLocalVMReader_ResolvesAKnownInstance(t *testing.T) { dataDir := t.TempDir() writeInstanceState(t, dataDir, map[string]*vm.VM{"i-a": {ID: "i-a", InstanceType: "t3.micro"}}) - r := vpcd.NewLocalVMStateReaderForTest(dataDir, nil) + r := state.NewLocalVMReaderForTest(dataDir, nil) v, err := r.LocalVM("i-a") require.NoError(t, err) require.NotNil(t, v) assert.Equal(t, "t3.micro", v.InstanceType) } -func TestLocalVMStateReader_CachesParseUntilFileChanges(t *testing.T) { +func TestLocalVMReader_CachesParseUntilFileChanges(t *testing.T) { dataDir := t.TempDir() writeInstanceState(t, dataDir, map[string]*vm.VM{"i-a": {ID: "i-a"}}) parses := 0 - r := vpcd.NewLocalVMStateReaderForTest(dataDir, func(path string) (*daemon.LocalState, error) { + r := state.NewLocalVMReaderForTest(dataDir, func(path string) (*daemon.LocalState, error) { parses++ return daemon.ReadLocalState(path) }) @@ -58,15 +58,15 @@ func TestLocalVMStateReader_CachesParseUntilFileChanges(t *testing.T) { assert.Equal(t, 2, parses, "a changed file must be re-parsed") } -// TestLocalVMStateReader_CorruptFileDoesNotServeStaleCache proves a stat/parse +// TestLocalVMReader_CorruptFileDoesNotServeStaleCache proves a stat/parse // failure clears the cache and surfaces the error rather than replaying the // last good parse — the caller (localInstanceLookup) treats this as a // fall-through to the record space, not a silently stale answer. -func TestLocalVMStateReader_CorruptFileDoesNotServeStaleCache(t *testing.T) { +func TestLocalVMReader_CorruptFileDoesNotServeStaleCache(t *testing.T) { dataDir := t.TempDir() writeInstanceState(t, dataDir, map[string]*vm.VM{"i-a": {ID: "i-a"}}) - r := vpcd.NewLocalVMStateReaderForTest(dataDir, nil) + r := state.NewLocalVMReaderForTest(dataDir, nil) v, err := r.LocalVM("i-a") require.NoError(t, err) require.NotNil(t, v, "warm the cache with a good parse") diff --git a/spinifex/operator/cli/service.go b/spinifex/operator/cli/service.go index 4a5867424..bdb0f4837 100644 --- a/spinifex/operator/cli/service.go +++ b/spinifex/operator/cli/service.go @@ -12,6 +12,7 @@ import ( "github.com/mulgadc/bluebottle/pkg/otelsetup" "github.com/mulgadc/spinifex/spinifex/bootstrap/config" "github.com/mulgadc/spinifex/spinifex/domains/dns" + "github.com/mulgadc/spinifex/spinifex/domains/ec2/instance/state" "github.com/mulgadc/spinifex/spinifex/domains/network/external" "github.com/mulgadc/spinifex/spinifex/providers/ebs/viperblock" "github.com/mulgadc/spinifex/spinifex/runtime/roles/awsgw" @@ -799,6 +800,8 @@ var vpcdStartCmd = &cobra.Command{ EgressBlockExemptVPCs: clusterConfig.Network.EgressBlockExemptVPCs, IMDSHostMetaIP: clusterConfig.Network.IMDSHostMetaIP, IMDSHostDNSIP: clusterConfig.Network.IMDSHostDNSIP, + LocalVMs: state.NewLocalVMReader(nodeConfig.DataDir), + OpenInstanceRecords: state.OpenInstanceRecordLoader, }) if err != nil { return fmt.Errorf("create vpcd service: %w", err) diff --git a/spinifex/vpcd/export_test.go b/spinifex/vpcd/export_test.go deleted file mode 100644 index 0e86cea8d..000000000 --- a/spinifex/vpcd/export_test.go +++ /dev/null @@ -1,24 +0,0 @@ -package vpcd - -import ( - "github.com/mulgadc/spinifex/spinifex/daemon" - "github.com/mulgadc/spinifex/spinifex/runtime/compute/vm" -) - -// LocalStateReaderForTest is the local instance state reader's consumer-facing -// shape, exported so an external test can exercise it without reaching into -// package internals. -type LocalStateReaderForTest interface { - LocalVM(instanceID string) (*vm.VM, error) -} - -// NewLocalVMStateReaderForTest builds a reader over dataDir. A non-nil -// readState replaces the parse step, so a test can count parses and prove the -// mtime/size cache is doing its job. -func NewLocalVMStateReaderForTest(dataDir string, readState func(path string) (*daemon.LocalState, error)) LocalStateReaderForTest { - r := newLocalVMStateReader(dataDir) - if readState != nil { - r.readState = readState - } - return r -} diff --git a/spinifex/vpcd/vpcd.go b/spinifex/vpcd/vpcd.go index 9ac4fb8d8..0145d6483 100644 --- a/spinifex/vpcd/vpcd.go +++ b/spinifex/vpcd/vpcd.go @@ -184,6 +184,12 @@ type Config struct { // EgressBlockExemptVPCs are VPC IDs the WAN egress block skips — the // operator workaround for a tenant with a legitimate need. EgressBlockExemptVPCs []string + // LocalVMs reads this node's on-disk VM state for IMDS's local-first + // lookup. The composition root wires domains/ec2/instance/state's reader. + LocalVMs guestmetadata.LocalStateReader + // OpenInstanceRecords opens the shared instance record space over vpcd's + // NATS connection. Required: vpcd fails to start without it. + OpenInstanceRecords func(ctx context.Context, nc *nats.Conn) (guestmetadata.RecordLoader, error) } // Service implements the Spinifex service interface for vpcd. @@ -580,10 +586,13 @@ func launchService(cfg *Config) error { } return live, nil } - // vpcd is the composition root for both planes IMDS straddles: it wires the - // on-disk VM state reader and the shared record space, so domains/ec2/guestmetadata - // itself never needs to import the compute plane beyond vm.VM. - records, err := newInstanceRecordLoader(ctx, nc) + // vpcd is the composition root for both planes IMDS straddles: + // domains/ec2/instance/state supplies the on-disk VM state reader and the + // record-space opener, so guestmetadata never imports the compute plane. + if cfg.OpenInstanceRecords == nil { + return fmt.Errorf("open instance record space: no instance record opener configured") + } + records, err := cfg.OpenInstanceRecords(ctx, nc) if err != nil { return fmt.Errorf("open instance record space: %w", err) } @@ -593,7 +602,7 @@ func launchService(cfg *Config) error { guestmetadata.NewNATSSTSAssumer(nc), guestmetadata.NewNATSProfileLookup(nc), guestmetadata.NewNATSPublicKeyLookup(nc), - newLocalVMStateReader(cfg.DataDir), + cfg.LocalVMs, records, listTaps, cfg.NorthstarBaseDomain,