[BUG] Unknown boot script serve attempts crash

[rainest]

Describe the bug
Attempts to serve the unknown machine boot script attempt to access etcd, even in SQL mode. If etcd is not available, this (and likely most other etcd access attempts) result in a API handler goroutine panic and an empty API response.

To Reproduce

Add a machine without a ComponentEndpoint and associated boot script:

ochami default smd iface add x1000c0s0b0n0 bc:24:11:40:03:a3 internal,192.168.8.128
ochami default smd component add x1000c0s0b0n0 1

ochami  bss boot params set --mac bc:24:11:40:03:a3 --initrd=http://192.168.8.12/images/alma/initramfs --kernel=http://192.168.8.12/images/alma/vmlinuz
ochami  bss boot image set --mac bc:24:11:40:03:a3  live:http://192.168.8.12/images/alma/alma.sqsh

Attempt to retrieve this machine's boothscript with some architecture:

ochami bss boot script get --mac bc:24:11:40:03:a3 --arch i386

You can probably also just use an entirely unrecognized MAC and trigger the same result, but I didn't try.

Observed behavior

This machine pulls a matching bootscript by MAC in BSS lookup code, but not a matching ComponentEndpoint. Lack of a component endpoint satisfies the unknown condition and executes unknownBootScrtipt(), which fails when it attempts to retrieve state:

Panic stack trace

2025/04/25 21:21:21 http: panic serving 10.42.1.185:57042: runtime error: slice bounds out of range [-1:]
goroutine 434 [running]:
net/http.(*conn).serve.func1()
	/opt/hostedtoolcache/go/1.23.4/x64/src/net/http/server.go:1947 +0xbe
panic({0x110df20?, 0xc000644318?})
	/opt/hostedtoolcache/go/1.23.4/x64/src/runtime/panic.go:785 +0x132
github.com/go-chi/chi/middleware.prettyStack.decorateFuncCallLine({}, {0xc0005e6283, 0x1f}, 0x1, 0x8)
	/home/runner/go/pkg/mod/github.com/go-chi/chi@v1.5.5/middleware/recoverer.go:130 +0x525
github.com/go-chi/chi/middleware.prettyStack.decorateLine({}, {0xc0005e6283?, 0x12b5?}, 0x1, 0x8)
	/home/runner/go/pkg/mod/github.com/go-chi/chi@v1.5.5/middleware/recoverer.go:106 +0x154
github.com/go-chi/chi/middleware.prettyStack.parse({}, {0xc000516000, 0x12b5, 0xc000778518?}, {0x102bec0, 0x1c836b0})
	/home/runner/go/pkg/mod/github.com/go-chi/chi@v1.5.5/middleware/recoverer.go:89 +0x3bc
github.com/go-chi/chi/middleware.PrintPrettyStack({0x102bec0, 0x1c836b0})
	/home/runner/go/pkg/mod/github.com/go-chi/chi@v1.5.5/middleware/recoverer.go:46 +0x3b
github.com/go-chi/chi/middleware.(*defaultLogEntry).Panic(0x439c05?, {0x102bec0?, 0x1c836b0?}, {0x439c40?, 0xc0004a96c0?, 0xc000778660?})
	/home/runner/go/pkg/mod/github.com/go-chi/chi@v1.5.5/middleware/logger.go:165 +0x25
github.com/go-chi/chi/middleware.Recoverer.func1.1()
	/home/runner/go/pkg/mod/github.com/go-chi/chi@v1.5.5/middleware/recoverer.go:28 +0xc8
panic({0x102bec0?, 0x1c836b0?})
	/opt/hostedtoolcache/go/1.23.4/x64/src/runtime/panic.go:785 +0x132
main.checkState(0x0)
	/home/runner/work/bss/bss/cmd/boot-script-service/scn.go:229 +0x50
main.unknownBootScript({0xc0000ea1bd, 0x4}, {0xc000039248, 0x11}, {0xc0000f0130, 0xd}, 0xfa4000, 0x680bfcc7, {0xc0000f0149, 0x7}, ...)
	/home/runner/work/bss/bss/cmd/boot-script-service/default_api.go:732 +0x3c5
main.BootscriptGet({0x7fcc31b3cc48, 0xc000018880}, 0xc0008df7c0)
	/home/runner/work/bss/bss/cmd/boot-script-service/default_api.go:864 +0xdd3
main.bootScript({0x7fcc31b3cc48?, 0xc000018880?}, 0x0?)
	/home/runner/work/bss/bss/cmd/boot-script-service/routers.go:152 +0xf0

I don't fully understand this panic--I think the indicated issue is actually in the Chi recover logic (which I care less about--it's a Chi bug, but realistically there's not much Chi can do to help if our API code dies) and the original BSS panic is masked.

Expected behavior
BSS serves some boot script appropriate to unknown machines, without crashing.

Additional context

The access attempt at https://github.com/OpenCHAMI/bss/blob/main/cmd/boot-script-service/scn.go#L229 is also the line that panics.

Best (strong) guess is that kvstore is uninitialized and the panic occurs because we effectively call <null>.method() anywhere we try to use it.

kvstore is a package-global struct. It's assigned to an actual struct in the kvOpen() function, which only runs in the useSQL == false case.

[rainest]

In general, use of package global structs is bad practice for the reason seen here: the compiler can't force you to initialize it, but it can force you to pass it to its users, which allows you to better understand where it's used and better mock it for tests. As-is, the use of package globals for shared brokers to other systems--an unfortunately common practice in CSM code--will lead to similar problems down the line.

IDK what all these are, but these are all uses of kvstore throughout the code that will have similar problems:

17:03:21-0700 baqytgul $ git status         
HEAD detached at v1.32.0
nothing to commit, working tree clean
17:03:23-0700 baqytgul $ git grep -rn kvstore
cmd/boot-script-service/boot_data.go:128:               val, exists, err = kvstore.Get(key)
cmd/boot-script-service/boot_data.go:174:               err = kvstore.Store(key, value)
cmd/boot-script-service/boot_data.go:175:               debugf("kvstore.Store(%s, %s) -> %v\n", key, value, err)
cmd/boot-script-service/boot_data.go:189:       return kvstore.GetRange(keyBase+keyMin, keyBase+keyMax)
cmd/boot-script-service/boot_data.go:193:       return kvstore.GetRange(makeKey(imtype, keyMin), makeKey(imtype, keyMax))
cmd/boot-script-service/boot_data.go:210:       kvstore.DistTimedLock(5)
cmd/boot-script-service/boot_data.go:211:       defer kvstore.DistUnlock()
cmd/boot-script-service/boot_data.go:296:       _, exists, err := kvstore.Get(key)
cmd/boot-script-service/boot_data.go:300:               err = kvstore.Delete(key)
cmd/boot-script-service/boot_data.go:318:                       err = kvstore.Delete(key)
cmd/boot-script-service/boot_data.go:816:               if err := kvstore.Store(key, timestamp); err != nil {
cmd/boot-script-service/boot_data.go:835:       return kvstore.GetRange(rangeStart, rangeEnd)
cmd/boot-script-service/boot_data.go:911:       timestampString, exists, err := kvstore.Get(key)
cmd/boot-script-service/boot_data.go:931:       return kvstore.GetRange(paramsPfx+keyMin, paramsPfx+keyMax)
cmd/boot-script-service/boot_data.go:968:       val, exists, err := kvstore.Get(key)
cmd/boot-script-service/boot_data.go:1202:      kvl, err := kvstore.GetRange(keyMin, keyMax)
cmd/boot-script-service/main.go:92:     kvstore             hmetcd.Kvi
cmd/boot-script-service/main.go:187:            kvstore, err = hmetcd.Open(url, opts)
cmd/boot-script-service/scn.go:211:     if err = kvstore.Store(UpdateTimestampKey, timestamp); err != nil {
cmd/boot-script-service/scn.go:229:             timestamp, exists, _ = kvstore.Get(UpdateTimestampKey)
cmd/boot-script-service/serviceAPI.go:231:      err = kvstore.Store("/bss/etcdTest", string(data))
cmd/boot-script-service/serviceAPI.go:236:      data, exists, err := kvstore.Get("/bss/etcdTest")

I haven't done a review of how the SQL and etcd libraries are used, but if we want to keep them, we probably want a wrapper interface with various getBSSThingA() writeBSSThingB() functions, and implementations of each for the two stores. That said, I'm pretty okay just tossing etcd.

We could attempt a review of the interface and its implementations (BSS uses the Kvi implementation, which may be the only one--I didn't review yet) to try and create a Postgres shim we can use as a module replace. The hms-hmetcd code is at least pretty short, and I think it's used widely in CSM code.

Writing a more proper replacement that's more visible to its clients may be (nebulously) cleaner, but there's lots of CSM services and I don't have a perfect understanding of all their data models, only a vague understanding of the few I've dug into the source of.

I was also curious if Kine, the project used by K3S to present an "etcd" that's actually some SQL engine would be useful, but they explicitly say:

Implements a subset of etcdAPI (not usable at all for general purpose etcd)

and don't go into further detail about what's in/out. IDK whether the bits they do implement would be sufficient without testing, but it may be worth considering.

[alexlovelltroy]

See OpenCHAMI/cloud-init#71

The global state in BSS makes it rather hostile to testing. I think we may want to preserve the API, but consider an alternate implementation.

[synackd]

We could attempt a review of the interface and its implementations (BSS uses the Kvi implementation, which may be the only one--I didn't review yet) to try and create a Postgres shim we can use as a module replace. The hms-hmetcd code is at least pretty short, and I think it's used widely in CSM code.

I've tried to look at refactoring BSS into using an interface for a storage backend, but its codebase is quite nonconducive to it.

I discovered this when writing the Postgres backend, and had to compromise by conditionally using Postgres in the write functions instead of Etcd. A byproduct of this is that sometimes BSS expects the kv store to be initialized even when using Postgres which causes panics, as I suspect might be happening here.

I think having an API-compliant alternative might be more straightforward than refactoring.

[rainest]

Another round of code archaeology to figure out how services are using etcd and what'd be required to pretend that Postgres is an etcd:

tl;dr

• Two OpenCHAMI services use hms-hmetcd. Three other CSM services use
  hms-etcd. I don't know if we expect to port any of those three and am not
  that familiar with their role in CSM.
• The interface should be possible to reimplement in SQL if we want to replace
  the module.
• Some interface functions are used infrequently or never, and can probably be
  discarded in favor of client-side refactors where they are used.
• On further review, Kine probably does implement
  enough etcd API surface to serve as a drop-in shim, though it would require
  running an instance of the Kine service (it is an etcd API server, not a
  client-side replacement module).

Gory details

Services using hms-hmetcd

• hbtd (heartbeat tracking service)
• bss (boot script service)
• pcs (power control service)
• fas (firmware action service)
• hmnfd (state change notification service) (aka scn)

Of these, I do not expect we'll use FAS. I'm unsure about HBTD or HMNFD, which
appear to be support services for populating and propagating some SMD data.

---

Interface overview and SQL reimplementation considerations

The interface
includes Store(), Get(), Delete(), and Close() functions that do what
you expect for a KV store. Open() outside the interface accepts either
connection details or a "mem" magic string to create an in-memory instance.
These should be trivial to implement in SQL.

If further includes several specialized functions:

GetRange() is a kinda strange function that gets all KV pairs within two
string bounds, using string < and > comparison. Apparently a lot of keys
(other than xnames) encode group membership information? I wasn't able to get a
good idea from the examples I found. This is implemented within etcd
but handled client-side for in-memory. This isn't that efficient. Postgres
probably has some equivalent in SELECT statements? IDK.

TempKey() creates a garbage value key and deletes it when the application
exits. Using some shared prefix and a random suffix allows you to determine the
number of application instances running using GetRange(). HBTD is the only
service that uses this pattern, and it implements the suffix/search/count
behavior itself. Could be re-implemented as a counter or table inserts/deletes.
The etcd implementation uses the Lease interface.

Transaction() atomically sets a one of two keys to one of two values based on
the outcome of a key/value comparison. This is a subset of the equivalent etcd functionality,
which allows a wider range of comparisons and actions. Nothing uses this except
the hms-etcd tests. Toss it.

TAS() atomically sets a key to a value if it is unset or if the key is equal
to a test value. PCS is the only user of this, for something related to
transitions (IDK they don't comment their exported functions). Presumably a
transaction SELECT, compare, and maybe UPDATE is equivalent.

Watch(), WatchWithCB(), and WatchCBCancel() watch a key and return its
new value when it changes, with an optional callback to execute. Nothing uses
these other than hms-hmetcd tests (I think, at least--I reviewed search results
and only saw a same-name function used in some Kubernetes API interactions).
Toss em.

DistLock(), DistTimedLock(), DistTimedLock() are used by BSS to prevent multiple instances from trying to store an image at once and by PCS to manage leader/follower elections. The Postgres advisory lock feature is apparently a good fit for this use case. If we want to refactor it out, BSS can probably live without, PCS I expect less so.

---

kine viability

I expect that Kine implements basic CRUD actions. While there is no documented
supported/not supported etcd API surface, it does
appear to support etcd ranges and transactions,
which should be sufficient to provide a pretend etcd to hms-hmetcd.

It also appears to support leases,
which we sort of care about for TempKey().

It also appears to support watches,
though we don't care about those at all.

---

The in-memory implementation

hms-hmetcd includes both the main etcd-backed implementation kvs_etcd and
an in-memory implementation kvs_mem, which is mostly a
package var singleton map[string]string.

This implementation is mostly of interest for reference, since it directly
implements some behavior that's otherwise hidden behind etcd API calls.

Instances of kvs_mem create mutexes that gate access to (most--for some
reason GetRange() entirely ignores the locks) the map, but you'll be haunted
by spooky concurrency ghosts if you ever create multiple instances. This
implementation fully ignores DistLock() and similar, though it seems like it
should mock them with a local lock.

[rainest]

Partial implementation at https://github.com/rainest/hms-hmetcd/tree/feat/pg/postgres (currently https://github.com/rainest/hms-hmetcd/tree/64d4b7370baf65f33dfa56b9256d30c61824cb20/postgres)

This would (almost certainly) have to be an all-or-nothing store: you wouldn't be able to mix and match it and purpose-built tables like the current useSQL blocks in BSS: your naive KV "etcd" code will otherwise find some values missing, unless you write to both. If you keep the useSQL blocks around, but remove the else around the etcd KV code, you can use this as a less efficient stopgap to writing SQL for all functions.

Considered the dedicated hstore KV feature in Postgres, but it looks like this is more for storing varied KV objects instead of one-to-one keys and values? The examples are unfortunately kinda sparse, but https://stackoverflow.com/questions/58591344/benefits-of-hstore-over-a-key-value-table-with-postgresql appears to support that.