-
Notifications
You must be signed in to change notification settings - Fork 450
OCPBUGS-62714: Temporary policy.json for PIS rpm-ostree rebasing #5345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) | ||
} |
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) | ||
} |
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"` | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The values come from
machine-config-operator/templates/common/_base/files/container-storage.yaml
Lines 13 to 20 in 18a0dc6
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