Add MachineID and SystemTime to remotefs.OS interface#306
Conversation
MachineID: PosixFS reads /etc/machine-id, WinFS queries the registry MachineGuid via Get-ItemProperty. SystemTime: PosixFS runs date -u +%s, WinFS uses [DateTimeOffset]::UtcNow.ToUnixTimeSeconds(). Both parse the result as a Unix timestamp and return time.Time. Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
There was a problem hiding this comment.
Pull request overview
Adds host identity and clock introspection to the remotefs.OS abstraction so callers can retrieve a stable machine identifier and the remote host’s current UTC time via a common interface (Posix/Windows).
Changes:
- Extend
remotefs.OSwithMachineID()andSystemTime(). - Implement the new methods in
PosixFSandWinFSusing platform-appropriate commands. - Add unit tests covering normal and error/parse-failure cases for both platforms.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
remotefs/types.go |
Adds ErrEmptyMachineID and extends OS interface with MachineID + SystemTime. |
remotefs/posixfs.go |
Implements MachineID via /etc/machine-id and SystemTime via date -u +%s. |
remotefs/winfs.go |
Implements MachineID via registry MachineGuid and SystemTime via UtcNow.ToUnixTimeSeconds(). |
remotefs/hostinfo_test.go |
Adds tests for the new methods on both Posix and Windows mock runners. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // MachineID returns the unique machine ID from /etc/machine-id. | ||
| func (s *PosixFS) MachineID() (string, error) { | ||
| out, err := s.ExecOutput("cat /etc/machine-id") | ||
| if err != nil { | ||
| return "", fmt.Errorf("machine-id: %w", err) | ||
| } | ||
| if out == "" { | ||
| return "", ErrEmptyMachineID | ||
| } | ||
| return out, nil |
There was a problem hiding this comment.
MachineID is executed with the default ExecOutput options, which log stdout at debug level. Since this returns a stable unique identifier, it can unintentionally leak host-identifying data into logs. Consider using cmd.HideOutput() (or cmd.Sensitive()/redaction) for this command so the value isn’t logged by default.
| // MachineID returns the unique machine ID from the Windows registry. | ||
| func (s *WinFS) MachineID() (string, error) { | ||
| out, err := s.ExecOutput("(Get-ItemProperty -Path 'HKLM:\\SOFTWARE\\Microsoft\\Cryptography' -Name MachineGuid).MachineGuid", cmd.PS()) | ||
| if err != nil { | ||
| return "", fmt.Errorf("machine-id: %w", err) | ||
| } | ||
| if out == "" { | ||
| return "", ErrEmptyMachineID | ||
| } | ||
| return out, nil |
There was a problem hiding this comment.
MachineID is executed with the default ExecOutput options, which log stdout at debug level. Because MachineGuid is a stable unique identifier, this can leak host-identifying data into logs. Consider adding cmd.HideOutput() (or cmd.Sensitive()/redaction) to this ExecOutput call to avoid logging the value by default.
| mr.AddCommandOutput(rigtest.Equal("cat /etc/machine-id"), "") | ||
| fs := remotefs.NewPosixFS(mr) | ||
| _, err := fs.MachineID() | ||
| require.Error(t, err) |
There was a problem hiding this comment.
The "empty" MachineID subtest only checks that an error occurred. To ensure callers can reliably detect this condition, assert that the returned error matches remotefs.ErrEmptyMachineID (e.g., require.ErrorIs).
| require.Error(t, err) | |
| require.ErrorIs(t, err, remotefs.ErrEmptyMachineID) |
| mr.AddCommandOutput(rigtest.HasPrefix("powershell.exe"), "") | ||
| fs := remotefs.NewWindowsFS(mr) | ||
| _, err := fs.MachineID() | ||
| require.Error(t, err) |
There was a problem hiding this comment.
The Windows "empty" MachineID subtest only checks that an error occurred. To validate the contract of MachineID, consider asserting errors.Is(err, remotefs.ErrEmptyMachineID) rather than only require.Error.
| require.Error(t, err) | |
| require.ErrorIs(t, err, remotefs.ErrEmptyMachineID) |
Moving some stuff that is generic from k0sctl to rig
Breaking rig's v2 api here is not a problem as it's not released and not in use anywhere yet.