From 8b6418d8aa6b1573a413e084e83bd1f695475406 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20M=C3=ADchal?= Date: Fri, 17 Dec 2021 16:27:21 +0200 Subject: [PATCH] cmd, pkg/utils: Split distro and release parsing and report better errors 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 --- src/cmd/create.go | 16 +++-- src/cmd/enter.go | 16 +++-- src/cmd/run.go | 16 +++-- src/cmd/utils.go | 15 ++++- src/pkg/utils/utils.go | 70 +++++++++++++++++++++ src/pkg/utils/utils_test.go | 122 ++++++++++++++++++++++++++++++++++++ test/system/101-create.bats | 38 +++++++++++ test/system/104-run.bats | 38 +++++++++++ test/system/105-enter.bats | 38 +++++++++++ 9 files changed, 353 insertions(+), 16 deletions(-) diff --git a/src/cmd/create.go b/src/cmd/create.go index d9930680a..3c1b06c99 100644 --- a/src/cmd/create.go +++ b/src/cmd/create.go @@ -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 } diff --git a/src/cmd/enter.go b/src/cmd/enter.go index 24bd09798..779f000f0 100644 --- a/src/cmd/enter.go +++ b/src/cmd/enter.go @@ -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 } diff --git a/src/cmd/run.go b/src/cmd/run.go index c01389e3b..bc0701333 100644 --- a/src/cmd/run.go +++ b/src/cmd/run.go @@ -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 } diff --git a/src/cmd/utils.go b/src/cmd/utils.go index a201f8165..ea332b366 100644 --- a/src/cmd/utils.go +++ b/src/cmd/utils.go @@ -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() diff --git a/src/pkg/utils/utils.go b/src/pkg/utils/utils.go index 3119fee74..c7ca233a4 100644 --- a/src/pkg/utils/utils.go +++ b/src/pkg/utils/utils.go @@ -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, + "/f", "registry.fedoraproject.org", "", false, @@ -106,6 +112,7 @@ var ( "rhel-toolbox", "toolbox", parseReleaseRHEL, + "", "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). diff --git a/src/pkg/utils/utils_test.go b/src/pkg/utils/utils_test.go index 329228154..2a5e45a14 100644 --- a/src/pkg/utils/utils_test.go +++ b/src/pkg/utils/utils_test.go @@ -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 diff --git a/test/system/101-create.bats b/test/system/101-create.bats index 0e06e3a65..8990fccda 100644 --- a/test/system/101-create.bats +++ b/test/system/101-create.bats @@ -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: /f" + 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 ] +} diff --git a/test/system/104-run.bats b/test/system/104-run.bats index e4574a699..c50921444 100644 --- a/test/system/104-run.bats +++ b/test/system/104-run.bats @@ -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: /f" + 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 diff --git a/test/system/105-enter.bats b/test/system/105-enter.bats index 82f5c83da..ec255ca5c 100644 --- a/test/system/105-enter.bats +++ b/test/system/105-enter.bats @@ -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: /f" + 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"