cmd: check-host-config rewritten in Go (HMS-9805)

[lzap]

Yeah. Cozy winter evening happened.

I wrote the whole framework and implemented two checks and then asked AI to implement the rest. Only checkers in the check package were generated and reviewed by me, all the rest is my own code. Let me know how you like it. I intentionally did not make any changes to the behavior, this is pure refactoring which will open doors to more features like using YAML to find which tests are missing etc.

Also the output is just pure text, but it could be maybe gotest now or something that CI understands and can parse for nicer results. But one step after another, I would rather keep it simple for now.

Now, I really wanted this to be fast so everything runs in parallel, but logs from all goroutines are collected and presented in a nicer way sequentially so the output is not all messed up. I heavily use what is called context function mocking where the platform functions (Exec, Exists, Grep) can be overriden with a mock via special context value. The key type can be not exported making it impossible to mess around with the functions from other packages.

The hard part is now to integrate this in the CICD. We want to build the binaries for each PR and then upload them somewhere and take it from here. For now, I am settling up with simply running "go build" for each test and then copying the binaries over to the image the same way as we did for the script. If we cache GOCACHE maybe this might not be a bad idea at all.

You can safely run this locally, I have reviewed and tested everything:

$ go run ./cmd/check-host-config -config test/configs/all-customizations.json 
[main           ] Loading build config from test/configs/all-customizations.json
[files          ] Checking existence of file: /etc/systemd/system/custom.service
[files          ] Checking if file exists: /etc/systemd/system/custom.service
[files          ] File does not exist: /etc/systemd/system/custom.service
[hostname       ] Executing: hostname
[hostname       ] Command succeeded: hostname
[hostname       ] Comparing 'dev.home.lan' with expected hostname 'my-host'
[cacerts        ] Parsing CA cert 1
[cacerts        ] Extracting serial from CA cert 1: 27894af897dd2423607045716438a725f28a6d0b
[cacerts        ] Extracting CN from CA cert 1: Test CA for osbuild
[cacerts        ] Checking CA cert 1 anchor file serial '27894af897dd2423607045716438a725f28a6d0b'
[cacerts        ] Checking if file exists: /etc/pki/ca-trust/source/anchors/27894af897dd2423607045716438a725f28a6d0b.pem
[cacerts        ] File does not exist: /etc/pki/ca-trust/source/anchors/27894af897dd2423607045716438a725f28a6d0b.pem
[srv-disabled   ] Checking disabled service: bluetooth.service
[srv-disabled   ] Executing: systemctl is-enabled bluetooth.service
[srv-disabled   ] Command succeeded: systemctl
[srv-masked     ] Executing: systemctl list-unit-files --state=masked
[srv-masked     ] Command succeeded: systemctl
[srv-masked     ] Checking masked service: nfs-server
[users          ] Checking user: user1
[users          ] Executing: id user1
[users          ] Command failed: id (exit code: exit status 1)
[srv-enabled    ] Checking enabled service: sshd.service
[srv-enabled    ] Executing: systemctl is-enabled sshd.service
[srv-enabled    ] Command succeeded: systemctl
[srv-enabled    ] Service was enabled service=sshd.service state=enabled
[srv-enabled    ] Checking enabled service: custom.service
[srv-enabled    ] Executing: systemctl is-enabled custom.service
[srv-enabled    ] Command failed: systemctl (exit code: exit status 4)
[fw-srv-disabled] Checking disabled firewall service: telnet
[fw-srv-disabled] Executing: sudo firewall-cmd --query-service=telnet
[fw-srv-disabled] Command failed: sudo (exit code: exit status 1)
[fw-srv-disabled] Firewall service was disabled service=telnet state=no
[fw-srv-enabled ] Checking enabled firewall service: ftp
[fw-srv-enabled ] Executing: sudo firewall-cmd --query-service=ftp
[fw-srv-enabled ] Command failed: sudo (exit code: exit status 1)
[fw-ports       ] Checking enabled firewall port: 1337/udp
[fw-ports       ] Executing: sudo firewall-cmd --query-port=1337/udp
[fw-ports       ] Command failed: sudo (exit code: exit status 1)
Results:
✅ Firewall Services Disabled Check
⚠️  Hostname Check (warn: hostname does not match, got dev.home.lan expected my-host)
❌ Files Check (fail: file does not exist: /etc/systemd/system/custom.service)
❌ CA Certs Check (fail: file missing for cert 1 at /etc/pki/ca-trust/source/anchors/27894af897dd2423607045716438a725f28a6d0b.pem)
❌ Services Disabled Check (fail: service is not disabled: bluetooth.service state: enabled)
❌ Services Masked Check (fail: service is not masked: nfs-server)
❌ Users Check (fail: user does not exist: user1)
❌ Services Enabled Check (fail: service is not enabled: custom.service error: exit status 4)
❌ Firewall Services Enabled Check (fail: firewall service is not enabled: ftp error: exit status 1)
❌ Firewall Ports Check (fail: firewall port is not enabled: 1337:udp error: exit status 1)

The result codes indicate a test which failed as the first one, starting from 10 (1-9 are reserved). These goroutines can fail in random order, so it must not be used for any workarounds it is purely informative.

[achilleas-k]

I'd like to see this split into multiple commits. The changes outside of cmd/check-host-config/ can be separate from the introduction of the new tool.

The introduction of the tool itself could also be multiple commits, like introducing the main parts and then the individual checks, but I don't feel as strongly about this (one commit for the whole thing isn't too bad).

More generally: I like the check interface. Each little module is nicely defined and clear. But looking at everything going on in main() I have to wonder what it's all for. There seems to be a lot going on for something that should mostly be "call Run() on a list of checks and collect the results". Maybe it could be broken down a bit into functions so its easier to read if it's all necessary.

[achilleas-k]

It's not very clear to me why this (and the other overrides like it) is needed. Do we use it anywhere, or is it just for mocking?

[lzap]

Only for thread-safe mocking, yes.

See, I value a good mocking absolutely essential in this case where CICD is very expensive, I want to be able to easily create as many reliable tests as I need in order to improve chance there are no bugs before I push and wait 3 hours until I get my results. The context mocking is not very common, but pretty useful approach.

Btw this rewrite alone revealed two issues both in modularity and OpenSCAP when the checks were not entirely correct. So it already paid off.

[lzap]

I added a package.go with further explanation:

// Package mockos provides mockable OS functions for testing. Use this package in
// host checks to allow mocking of OS interactions during unit tests. The package
// provides WithXXXFunc functions to set mock implementations in a context.Context,
// and corresponding XXXFunc functions to retrieve them. If no mock is set, the real
// OS functions are used.
package mockos

[lzap]

The introduction of the tool itself could also be multiple commits, like introducing the main parts and then the individual checks, but I don't feel as strongly about this (one commit for the whole thing isn't too bad).

While generally I agree with isolated commits, here it does not make too much sense as the commit makes sense as a whole. I do not like creating "artificial history" when it was not how the code was really written. Can do this if you insist.

But looking at everything going on in main() I have to wonder what it's all for.

Yeah, I see two confusing parts. Argument parsing is a big unreadable, I was aiming for both because I thought it is good to be backward compatible if someone uses this directly. But now that I think about it, let's break it. I am settling with Go flags because these are shorter to parse than argument + environmental variable.

The special goroutine which collects logs is not necessary, but I really want this to be readable. Because all checks are executed in parallel, the output would be totally unreadable without this. We really, really should do something with unreadable logs on CICD and we have to start somewhere. This check binary will be very often the one that fails and I want a clear output with as much logging as possible. This is why logging is quite elaborated and is being collected and ordered correctly.

Maybe it could be broken down a bit into functions so its easier to read if it's all necessary.

Absolutely, good idea. I moved check slice into checks.go and extracted code into functions.

[achilleas-k]

This will make reviews harder

For me at least, a single final commit that changes some config files and modifies all the manifest checksums doesn't affect the reviewing much. As long as it's isolated from the rest of the changes, I can review the PR as if it's not there by viewing the rest of the commits individually (or in ranges, now that GH web UI supports it). So don't worry about that.

[achilleas-k]

The check always succeeds.

[achilleas-k]

You mentioned at some point (can't remember where or when) that you had a hard time rebasing and rewriting this because you had to wait for CI jobs that can take hours.

Just ftr, for anyone reading this, it's possible to quickly test it locally by:

1. Building an image: ./test/scripts/build-image fedora-43 generic-qcow2 ./test/configs/all-customizations.json.
2. pip install . (in a venv preferably) to make vmtest available.
3. Running the boot test on the image: ./test/scripts/boot-image fedora-43 generic-qcow2 ./test/configs/all-customizations.json

Step 3 takes about 15 secs to run on my machine, making it very easy to iterate locally on the code.

[lzap]

Damn, yeah, nice catch. I am bumping the seed once again then. Rebased.

[achilleas-k]

Thanks again! Nice work. I went through everything carefully again and have a few nitpicks and minor comments. Approving anyway, since they can be handled in follow-ups.

[achilleas-k]

systemctl list-units supports --output=json, which would be cleaner here. Though, I don't know what version introduced the feature, so maybe it's not available on RHEL 8.10.

[lzap]

Yeah that is RHEL9+ only.

[achilleas-k]

We could check permissions and ownership as well. And check data when it's embedded in the blueprint. Let's do it in a follow-up though.

[lzap]

Yes but I aim for no changes in this PR, except fixing obvious bugs. There is much more we can do now pretty easily.

[lzap]

Rebased and squashed all your comments, thanks. The only exception being listing unit tests but this is not a big deal it is only in case of error. Any other new functionality I would like to do as a followups.

[lzap]

Only resolved manifest conflicts after auto-merge was enabled, can I get acks again @achilleas-k @supakeen thanks :-)