Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions pkg/daemon/command_runner.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package daemon

import (
"fmt"
"os/exec"
"strings"

"k8s.io/klog/v2"
)

// CommandRunner abstracts command execution for testing and flexibility.
type CommandRunner interface {
// RunGetOut executes a command and returns its stdout output.
// On error, stderr is included in the error message.
RunGetOut(command string, args ...string) ([]byte, error)
}

// CommandRunnerOS is the production implementation that executes real OS commands.
type CommandRunnerOS struct{}

// RunGetOut executes a command, logs it, and returns stdout.
// On error, stderr is included in the error message (truncated to 256 chars).
func (r *CommandRunnerOS) RunGetOut(command string, args ...string) ([]byte, error) {
klog.Infof("Running captured: %s %s", command, strings.Join(args, " "))
cmd := exec.Command(command, args...)
rawOut, err := cmd.Output()
if err != nil {
errtext := ""
if e, ok := err.(*exec.ExitError); ok {
// Trim to max of 256 characters
errtext = fmt.Sprintf("\n%s", truncate(string(e.Stderr), 256))
}
return nil, fmt.Errorf("error running %s %s: %s%s", command, strings.Join(args, " "), err, errtext)
}
return rawOut, nil
}

// TODO: Delete this function to always consume the interface instance
// Tracking story: https://issues.redhat.com/browse/MCO-1924
//
// Conserved the old signature to avoid a big footprint bugfix with this change
func runGetOut(command string, args ...string) ([]byte, error) {
return (&CommandRunnerOS{}).RunGetOut(command, args...)
}

// MockCommandRunner is a test implementation that returns pre-configured outputs.
type MockCommandRunner struct {
outputs map[string][]byte
errors map[string]error
}

// RunGetOut returns pre-configured output or error based on the command string.
// It matches commands using "command arg1 arg2..." as the key.
// Returns an error if no matching output or error is found.
func (m *MockCommandRunner) RunGetOut(command string, args ...string) ([]byte, error) {
key := command + " " + strings.Join(args, " ")
if out, ok := m.outputs[key]; ok {
return out, nil
}
if err, ok := m.errors[key]; ok {
return nil, err
}
return nil, fmt.Errorf("no output for command %s found", command)
}
93 changes: 93 additions & 0 deletions pkg/daemon/command_runner_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
package daemon

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestCommandRunnerOS_RunGetOut(t *testing.T) {
runner := &CommandRunnerOS{}

// Test successful command with no output
o, err := runner.RunGetOut("true")
assert.Nil(t, err)
assert.Equal(t, len(o), 0)

// Test failed command
o, err = runner.RunGetOut("false")
assert.NotNil(t, err)

// Test successful command with output
o, err = runner.RunGetOut("echo", "hello")
assert.Nil(t, err)
assert.Equal(t, string(o), "hello\n")

// Test command that outputs to stderr and exits with error
// base64 encode "oops" so we can't match on the command arguments
o, err = runner.RunGetOut("/bin/sh", "-c", "echo hello; echo b29vcwo== | base64 -d 1>&2; exit 1")
assert.Error(t, err)
errtext := err.Error()
assert.Contains(t, errtext, "exit status 1\nooos\n")

// Test command that doesn't exist
o, err = runner.RunGetOut("/usr/bin/test-failure-to-exec-this-should-not-exist", "arg")
assert.Error(t, err)
}

func TestMockCommandRunner_RunGetOut(t *testing.T) {
// Test mock with pre-configured output
mock := &MockCommandRunner{
outputs: map[string][]byte{
"echo hello": []byte("hello\n"),
"rpm-ostree status": []byte("State: idle\n"),
},
errors: map[string]error{},
}

o, err := mock.RunGetOut("echo", "hello")
assert.Nil(t, err)
assert.Equal(t, []byte("hello\n"), o)

o, err = mock.RunGetOut("rpm-ostree", "status")
assert.Nil(t, err)
assert.Equal(t, []byte("State: idle\n"), o)

// Test mock with pre-configured error
mock = &MockCommandRunner{
outputs: map[string][]byte{},
errors: map[string]error{
"cmd --fail": assert.AnError,
},
}

o, err = mock.RunGetOut("cmd", "--fail")
assert.Error(t, err)
assert.Equal(t, assert.AnError, err)

// Test mock with no matching command (should return error)
mock = &MockCommandRunner{
outputs: map[string][]byte{},
errors: map[string]error{},
}

o, err = mock.RunGetOut("unknown", "command")
assert.Error(t, err)
assert.Contains(t, err.Error(), "no output for command")
}

func TestMockCommandRunner_Priority(t *testing.T) {
// Test that outputs take priority over errors
mock := &MockCommandRunner{
outputs: map[string][]byte{
"test cmd": []byte("output\n"),
},
errors: map[string]error{
"test cmd": assert.AnError,
},
}

o, err := mock.RunGetOut("test", "cmd")
assert.Nil(t, err)
assert.Equal(t, []byte("output\n"), o)
}
13 changes: 11 additions & 2 deletions pkg/daemon/daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,12 @@ type Daemon struct {

// Ensures that only a single syncOSImagePullSecrets call can run at a time.
osImageMux *sync.Mutex

// Abstraction for running commands against the OS
cmdRunner CommandRunner

// Bare minimal podman client
podmanInterface PodmanInterface
}

// CoreOSDaemon protects the methods that should only be called on CoreOS variants
Expand Down Expand Up @@ -307,10 +313,11 @@ func New(
}

var nodeUpdaterClient *RpmOstreeClient

cmdRunner := &CommandRunnerOS{}
podmanInterface := NewPodmanExec(cmdRunner)
// Only pull the osImageURL from OSTree when we are on RHCOS or FCOS
if hostos.IsCoreOSVariant() {
nodeUpdaterClientVal := NewNodeUpdaterClient()
nodeUpdaterClientVal := NewNodeUpdaterClient(cmdRunner, podmanInterface)
nodeUpdaterClient = &nodeUpdaterClientVal
err := nodeUpdaterClient.Initialize()
if err != nil {
Expand Down Expand Up @@ -348,6 +355,8 @@ func New(
configDriftMonitor: NewConfigDriftMonitor(),
osImageMux: &sync.Mutex{},
irreconcilableReporter: NewNoOpIrreconcilableReporterImpl(),
cmdRunner: cmdRunner,
podmanInterface: podmanInterface,
}, nil
}

Expand Down
68 changes: 0 additions & 68 deletions pkg/daemon/image_manager_helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,8 @@ import (
"errors"
"fmt"
"os"
"os/exec"
"path/filepath"
"strings"
"time"

"github.com/opencontainers/go-digest"
pivotutils "github.com/openshift/machine-config-operator/pkg/daemon/pivot/utils"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/klog/v2"
)
Expand All @@ -33,76 +28,13 @@ const (

type imageSystem string

// imageInspection is a public implementation of
// https://github.com/containers/skopeo/blob/82186b916faa9c8c70cfa922229bafe5ae024dec/cmd/skopeo/inspect.go#L20-L31
type imageInspection struct {
Name string `json:",omitempty"`
Tag string `json:",omitempty"`
Digest digest.Digest
RepoDigests []string
Created *time.Time
DockerVersion string
Labels map[string]string
Architecture string
Os string
Layers []string
}

// BootedImageInfo stores MCO interested bootec image info
type BootedImageInfo struct {
OSImageURL string
ImageVersion string
BaseChecksum string
}

func podmanInspect(imgURL string) (imgdata *imageInspection, err error) {
// Pull the container image if not already available
var authArgs []string
if _, err := os.Stat(kubeletAuthFile); err == nil {
authArgs = append(authArgs, "--authfile", kubeletAuthFile)
}
args := []string{"pull", "-q"}
args = append(args, authArgs...)
args = append(args, imgURL)
_, err = pivotutils.RunExt(numRetriesNetCommands, "podman", args...)
if err != nil {
return
}

inspectArgs := []string{"inspect", "--type=image"}
inspectArgs = append(inspectArgs, fmt.Sprintf("%s", imgURL))
var output []byte
output, err = runGetOut("podman", inspectArgs...)
if err != nil {
return
}
var imagedataArray []imageInspection
err = json.Unmarshal(output, &imagedataArray)
if err != nil {
err = fmt.Errorf("unmarshaling podman inspect: %w", err)
return
}
imgdata = &imagedataArray[0]
return

}

// runGetOut executes a command, logging it, and return the stdout output.
func runGetOut(command string, args ...string) ([]byte, error) {
klog.Infof("Running captured: %s %s", command, strings.Join(args, " "))
cmd := exec.Command(command, args...)
rawOut, err := cmd.Output()
if err != nil {
errtext := ""
if e, ok := err.(*exec.ExitError); ok {
// Trim to max of 256 characters
errtext = fmt.Sprintf("\n%s", truncate(string(e.Stderr), 256))
}
return nil, fmt.Errorf("error running %s %s: %s%s", command, strings.Join(args, " "), err, errtext)
}
return rawOut, nil
}

// truncate a string using runes/codepoints as limits.
// This specifically will avoid breaking a UTF-8 value.
func truncate(input string, limit int) string {
Expand Down
89 changes: 89 additions & 0 deletions pkg/daemon/podman.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
package daemon

import (
"encoding/json"
"fmt"
)

// PodmanStorageConfig contains storage configuration from Podman.
type PodmanStorageConfig struct {
GraphDriverName string `json:"graphDriverName"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of curiosity are these names and fields copied over from podman somewhere? Or just what we need to construct the full pull spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The values come from

# Default storage driver, must be set for proper operation.
driver = "overlay"
# Temporary storage location
runroot = "/run/containers/storage"
# Primary Read/Write location of container storage
graphroot = "/var/lib/containers/storage"

I foud podman the more convenient way to get them instead of reading the file, as podman will take into consideration user overrides placed in ~/.config/containers/storage.conf

GraphRoot string `json:"graphRoot"`
}

// PodmanInfo contains system information from Podman.
type PodmanInfo struct {
Store PodmanStorageConfig `json:"store"`
}

// PodmanImageInfo contains image metadata from Podman.
type PodmanImageInfo struct {
ID string `json:"Id"`
Digest string `json:"Digest"`
RepoDigests []string `json:"RepoDigests"`
RepoDigest string `json:"-"` // Filled with matching digest from RepoDigests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a bit confused about this field at first, but I guess the intent here is that Id, Digest, RepoDigests will get unmarshed from podimage images, and instead of having a new field we pass around, we have a pre-filtered RepoDigest field we populate after the fact?

Interesting pattern that I'm not sure we employ elsewhere and should work, so I'm not against it, just wanted to make sure I'm understanding that right

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You got it right. I did it to avoid passing a tuple everywhere, but, if it's not too clear I can always follow that other approach.

}

// PodmanInterface abstracts podman operations for testing and flexibility.
type PodmanInterface interface {
// GetPodmanImageInfoByReference retrieves image info for the given reference.
// Returns nil if no image is found.
GetPodmanImageInfoByReference(reference string) (*PodmanImageInfo, error)
// GetPodmanInfo retrieves Podman system information.
GetPodmanInfo() (*PodmanInfo, error)
}

// PodmanExecInterface is the production implementation that executes real podman commands.
type PodmanExecInterface struct {
cmdRunner CommandRunner
}

// NewPodmanExec creates a new PodmanExecInterface with the given command runner.
func NewPodmanExec(commandRunner CommandRunner) *PodmanExecInterface {
return &PodmanExecInterface{cmdRunner: commandRunner}
}

// GetPodmanImageInfoByReference retrieves image info for the given reference.
// It executes 'podman images --format=json --filter reference=<reference>'.
// Returns nil if no image is found.
// The RepoDigest field is populated if the reference matches one of the RepoDigests.
func (p *PodmanExecInterface) GetPodmanImageInfoByReference(reference string) (*PodmanImageInfo, error) {
output, err := p.cmdRunner.RunGetOut("podman", "images", "--format=json", "--filter", "reference="+reference)
if err != nil {
return nil, err
}
var podmanInfos []PodmanImageInfo
if err := json.Unmarshal(output, &podmanInfos); err != nil {
return nil, fmt.Errorf("failed to decode podman image ls output: %v", err)
}
if len(podmanInfos) == 0 {

return nil, nil
}

info := &podmanInfos[0]
// Fill the custom RepoDigest field with the digest that matches the
// requested reference as it's convenient to be used by the caller
for _, digest := range info.RepoDigests {
if digest == reference {
info.RepoDigest = digest
break
}
}

return info, nil
}

// GetPodmanInfo retrieves Podman system information.
// It executes 'podman system info --format=json'.
func (p *PodmanExecInterface) GetPodmanInfo() (*PodmanInfo, error) {
output, err := p.cmdRunner.RunGetOut("podman", "system", "info", "--format=json")
if err != nil {
return nil, err
}
var podmanInfo PodmanInfo
if err := json.Unmarshal(output, &podmanInfo); err != nil {
return nil, fmt.Errorf("failed to decode podman system info output: %v", err)
}
return &podmanInfo, nil
}
Loading