wbrawner/toolbox
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

cmd, pkg/utils: Split distro and release parsing and report better errors

Browse source 
Using a non-supported distribution via `--distro` resulted in a silent
fallback to the Fedora image which confuses users. When a faulty release
format was used with `--release` a message without any hint about the
correct format was shown to the user.

This separates distro and release parsing into two chunks that have
greater control over error reporting and provides more accurate error
reports for the user.

Fixes https://github.com/containers/toolbox/issues/937

https://github.com/containers/toolbox/pull/977
This commit is contained in:
Ondřej Míchal 2021-12-17 16:27:21 +02:00
parent 2af0b30ed3
commit 8b6418d8aa
9 changed files with 353 additions and 16 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

16
src/cmd/create.go
View file

@ -138,17 +138,23 @@ func create(cmd *cobra.Command, args []string) error {
}
}
var release string
if createFlags.release != "" {
distro, err := utils.ResolveDistro(createFlags.distro)
if err != nil {
err := createErrorInvalidDistro()
return err
}
release := createFlags.release
if release != "" {
var err error
release, err = utils.ParseRelease(createFlags.distro, createFlags.release)
release, err = utils.ParseRelease(distro, release)
if err != nil {
err := createErrorInvalidRelease()
err := createErrorInvalidRelease(distro)
return err
}
}
image, release, err := utils.ResolveImageName(createFlags.distro, createFlags.image, release)
image, release, err := utils.ResolveImageName(distro, createFlags.image, release)
if err != nil {
return err
}

16
src/cmd/enter.go
View file

@ -104,19 +104,25 @@ func enter(cmd *cobra.Command, args []string) error {
}
}
var release string
if enterFlags.release != "" {
distro, err := utils.ResolveDistro(enterFlags.distro)
if err != nil {
err := createErrorInvalidDistro()
return err
}
release := enterFlags.release
if release != "" {
defaultContainer = false
var err error
release, err = utils.ParseRelease(enterFlags.distro, enterFlags.release)
release, err = utils.ParseRelease(distro, release)
if err != nil {
err := createErrorInvalidRelease()
err := createErrorInvalidRelease(distro)
return err
}
}
image, release, err := utils.ResolveImageName(enterFlags.distro, "", release)
image, release, err := utils.ResolveImageName(distro, "", release)
if err != nil {
return err
}

16
src/cmd/run.go
View file

@ -102,14 +102,20 @@ func run(cmd *cobra.Command, args []string) error {
}
}
var release string
if runFlags.release != "" {
distro, err := utils.ResolveDistro(runFlags.distro)
if err != nil {
err := createErrorInvalidDistro()
return err
}
release := runFlags.release
if release != "" {
defaultContainer = false
var err error
release, err = utils.ParseRelease(runFlags.distro, runFlags.release)
release, err = utils.ParseRelease(distro, release)
if err != nil {
err := createErrorInvalidRelease()
err := createErrorInvalidRelease(distro)
return err
}
}
@ -125,7 +131,7 @@ func run(cmd *cobra.Command, args []string) error {
command := args
image, release, err := utils.ResolveImageName(runFlags.distro, "", release)
image, release, err := utils.ResolveImageName(distro, "", release)
if err != nil {
return err
}

15
src/cmd/utils.go
View file

@ -23,6 +23,8 @@ import (
"os/exec"
"strings"
"syscall"
"github.com/containers/toolbox/pkg/utils"
)
// askForConfirmation prints prompt to stdout and waits for response from the
@ -69,9 +71,20 @@ func createErrorContainerNotFound(container string) error {
return errors.New(errMsg)
}
func createErrorInvalidRelease() error {
func createErrorInvalidDistro() error {
var builder strings.Builder
fmt.Fprintf(&builder, "invalid argument for '--distro'\n")
fmt.Fprintf(&builder, "Supported values are: %s\n", strings.Join(utils.GetSupportedDistros(), " "))
fmt.Fprintf(&builder, "Run '%s --help' for usage.", executableBase)
errMsg := builder.String()
return errors.New(errMsg)
}
func createErrorInvalidRelease(distro string) error {
var builder strings.Builder
fmt.Fprintf(&builder, "invalid argument for '--release'\n")
fmt.Fprintf(&builder, "Supported values for distribution %s are in format: %s\n", distro, utils.GetReleaseFormat(distro))
fmt.Fprintf(&builder, "Run '%s --help' for usage.", executableBase)
errMsg := builder.String()

70
src/pkg/utils/utils.go
View file

@ -44,6 +44,7 @@ type Distro struct {
ContainerNamePrefix string
ImageBasename string
ParseRelease ParseReleaseFunc
ReleaseFormat string
Registry string
Repository string
RepositoryNeedsRelease bool
@ -60,6 +61,10 @@ const (
ContainerNameRegexp = "[a-zA-Z0-9][a-zA-Z0-9_.-]*"
)
var (
ErrUnsupportedDistro = errors.New("linux distribution is not supported")
)
var (
containerNamePrefixDefault = "fedora-toolbox"
@ -98,6 +103,7 @@ var (
"fedora-toolbox",
"fedora-toolbox",
parseReleaseFedora,
"<release>/f<release>",
"registry.fedoraproject.org",
"",
false,
@ -106,6 +112,7 @@ var (
"rhel-toolbox",
"toolbox",
parseReleaseRHEL,
"<major.minor>",
"registry.access.redhat.com",
"ubi8",
false,
@ -407,6 +414,20 @@ func GetMountOptions(target string) (string, error) {
return mountOptions, nil
}
// GetReleaseFormat returns the format string signifying supported release
// version formats.
//
// distro should be value found under ID in os-release.
//
// If distro is unsupported an empty string is returned.
func GetReleaseFormat(distro string) string {
if !IsDistroSupported(distro) {
return ""
}
return supportedDistros[distro].ReleaseFormat
}
func GetRuntimeDirectory(targetUser *user.User) (string, error) {
gid, err := strconv.Atoi(targetUser.Gid)
if err != nil {
@ -446,6 +467,15 @@ func GetRuntimeDirectory(targetUser *user.User) (string, error) {
return toolboxRuntimeDirectory, nil
}
// GetSupportedDistros returns a list of supported distributions
func GetSupportedDistros() []string {
var distros []string
for d := range supportedDistros {
distros = append(distros, d)
}
return distros
}
// HumanDuration accepts a Unix time value and converts it into a human readable
// string.
//
@ -454,6 +484,14 @@ func HumanDuration(duration int64) string {
return units.HumanDuration(time.Since(time.Unix(duration, 0))) + " ago"
}
// IsDistroSupported signifies if a distribution has a toolbx image for it.
//
// distro should be value found under ID in os-release.
func IsDistroSupported(distro string) bool {
_, ok := supportedDistros[distro]
return ok
}
// ImageReferenceCanBeID checks if 'image' might be the ID of an image
func ImageReferenceCanBeID(image string) bool {
matched, err := regexp.MatchString("^[a-f0-9]{6,64}$", image)
@ -598,6 +636,11 @@ func ShortID(id string) string {
return id
}
// ParseRelease assesses if the requested version of a distribution is in
// the correct format.
//
// If distro is an empty string, a default value (value under the
// 'general.distro' key in a config file or 'fedora') is assumed.
func ParseRelease(distro, release string) (string, error) {
if distro == "" {
distro = distroDefault
@ -712,6 +755,33 @@ func ResolveContainerName(container, image, release string) (string, error) {
return container, nil
}
// ResolveDistro assess if the requested distribution is supported and provides
// a default value if none is requested.
//
// If distro is empty, and the "general.distro" key in a config file is set,
// the value is read from the config file. If the key is not set, the default
// value ('fedora') is used instead.
func ResolveDistro(distro string) (string, error) {
logrus.Debug("Resolving distribution")
logrus.Debugf("Distribution: %s", distro)
if distro == "" {
distro = distroDefault
if viper.IsSet("general.distro") {
distro = viper.GetString("general.distro")
}
}
if !IsDistroSupported(distro) {
return "", ErrUnsupportedDistro
}
logrus.Debug("Resolved distribution")
logrus.Debugf("Distribution: %s", distro)
return distro, nil
}
// ResolveImageName standardizes the name of an image.
//
// If no image name is specified then the base image will reflect the platform of the host (even the version).

122
src/pkg/utils/utils_test.go
View file

@ -20,9 +20,45 @@ import (
"strconv"
"testing"
"github.com/spf13/viper"
"github.com/stretchr/testify/assert"
)
func TestGetReleaseFormat(t *testing.T) {
testCases := []struct {
name string
distro string
expected string
}{
{
"Unknown distro",
"foobar",
"",
},
{
"Known distro (fedora)",
"fedora",
supportedDistros["fedora"].ReleaseFormat,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
res := GetReleaseFormat(tc.distro)
assert.Equal(t, tc.expected, res)
})
}
}
func TestGetSupportedDistros(t *testing.T) {
refDistros := []string{"fedora", "rhel"}
distros := GetSupportedDistros()
for _, d := range distros {
assert.Contains(t, refDistros, d)
}
}
func TestImageReferenceCanBeID(t *testing.T) {
testCases := []struct {
name string
@ -74,6 +110,92 @@ func TestImageReferenceCanBeID(t *testing.T) {
}
}
func TestIsDistroSupport(t *testing.T) {
testCases := []struct {
name string
distro string
ok bool
}{
{
"Unsupported distro",
"foobar",
false,
},
{
"Supported distro (fedora)",
"fedora",
true,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
res := IsDistroSupported(tc.distro)
assert.Equal(t, tc.ok, res)
})
}
}
func TestResolveDistro(t *testing.T) {
testCases := []struct {
name string
distro string
expected string
configValue string
err bool
}{
{
"Default - no distro provided; config unset",
"",
distroDefault,
"",
false,
},
{
"Default - no distro provided; config set",
"",
"rhel",
"rhel",
false,
},
{
"Fedora",
"fedora",
"fedora",
"",
false,
},
{
"RHEL",
"rhel",
"rhel",
"",
false,
},
{
"FooBar; wrong distro",
"foobar",
"",
"",
true,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
if tc.configValue != "" {
viper.Set("general.distro", tc.configValue)
}
res, err := ResolveDistro(tc.distro)
assert.Equal(t, tc.expected, res)
if tc.err {
assert.NotNil(t, err)
}
})
}
}
func TestParseRelease(t *testing.T) {
testCases := []struct {
name string

38
test/system/101-create.bats
View file

@ -79,3 +79,41 @@ teardown() {
assert_line --index 1 "If it was a private image, log in with: podman login foo.org"
assert_line --index 2 "Use 'toolbox --verbose ...' for further details."
}
@test "create: Try to create a container based on unsupported distribution" {
local distro="foo"
run $TOOLBOX -y create -d "$distro"
assert_failure
assert_line --index 0 "Error: invalid argument for '--distro'"
# Distro names are in a hashtable and thus the order can change
assert_line --index 1 --regexp "Supported values are: (.?(fedora|rhel))+"
assert_line --index 2 "Run 'toolbox --help' for usage."
assert [ ${#lines[@]} -eq 3 ]
}
@test "create: Try to create a container based on Fedora but with wrong version" {
run $TOOLBOX -y create -d fedora -r foobar
assert_failure
assert_line --index 0 "Error: invalid argument for '--release'"
assert_line --index 1 "Supported values for distribution fedora are in format: <release>/f<release>"
assert_line --index 2 "Run 'toolbox --help' for usage."
assert [ ${#lines[@]} -eq 3 ]
}
@test "create: Try to create a container based on non-default distribution without providing version" {
local distro="fedora"
local system_id="$(get_system_id)"
if [ "$system_id" = "fedora" ]; then
distro="rhel"
fi
run $TOOLBOX -y create -d "$distro"
assert_failure
assert_line --index 0 "Error: release not found for non-default distribution $distro"
assert [ ${#lines[@]} -eq 1 ]
}

38
test/system/104-run.bats
View file

@ -48,6 +48,44 @@ teardown() {
assert_line --index 2 "Run 'toolbox --help' for usage."
}
@test "run: Try to run a command in a container based on unsupported distribution" {
local distro="foo"
run $TOOLBOX -y run -d "$distro" ls
assert_failure
assert_line --index 0 "Error: invalid argument for '--distro'"
# Distro names are in a hashtable and thus the order can change
assert_line --index 1 --regexp "Supported values are: (.?(fedora|rhel))+"
assert_line --index 2 "Run 'toolbox --help' for usage."
assert [ ${#lines[@]} -eq 3 ]
}
@test "run: Try to run a command in a container based on Fedora but with wrong version" {
run $TOOLBOX run -d fedora -r foobar
assert_failure
assert_line --index 0 "Error: invalid argument for '--release'"
assert_line --index 1 "Supported values for distribution fedora are in format: <release>/f<release>"
assert_line --index 2 "Run 'toolbox --help' for usage."
assert [ ${#lines[@]} -eq 3 ]
}
@test "run: Try to run a command in a container based on non-default distro without providing a version" {
local distro="fedora"
local system_id="$(get_system_id)"
if [ "$system_id" = "fedora" ]; then
distro="rhel"
fi
run $TOOLBOX run -d "$distro" ls
assert_failure
assert_line --index 0 "Error: release not found for non-default distribution $distro"
assert [ ${#lines[@]} -eq 1 ]
}
@test "run: Run echo 'Hello World' inside of the default container" {
create_default_container

38
test/system/105-enter.bats
View file

@ -54,6 +54,44 @@ teardown() {
assert_line --index 2 "Run 'toolbox --help' for usage."
}
@test "enter: Try to enter a container based on unsupported distribution" {
local distro="foo"
run $TOOLBOX -y enter -d "$distro"
assert_failure
assert_line --index 0 "Error: invalid argument for '--distro'"
# Distro names are in a hashtable and thus the order can change
assert_line --index 1 --regexp "Supported values are: (.?(fedora|rhel))+"
assert_line --index 2 "Run 'toolbox --help' for usage."
assert [ ${#lines[@]} -eq 3 ]
}
@test "enter: Try to enter a container based on Fedora but with wrong version" {
run $TOOLBOX enter -d fedora -r foobar
assert_failure
assert_line --index 0 "Error: invalid argument for '--release'"
assert_line --index 1 "Supported values for distribution fedora are in format: <release>/f<release>"
assert_line --index 2 "Run 'toolbox --help' for usage."
assert [ ${#lines[@]} -eq 3 ]
}
@test "enter: Try to enter a container based on non-default distro without providing a version" {
local distro="fedora"
local system_id="$(get_system_id)"
if [ "$system_id" = "fedora" ]; then
distro="rhel"
fi
run $TOOLBOX enter -d "$distro"
assert_failure
assert_line --index 0 "Error: release not found for non-default distribution $distro"
assert [ ${#lines[@]} -eq 1 ]
}
# TODO: Write the test
@test "enter: Enter the default toolbox" {
skip "Testing of entering toolboxes is not implemented"