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..57d2b27a0 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, @@ -242,8 +249,8 @@ func GetContainerNamePrefixForImage(image string) (string, error) { } func getDefaultImageForDistro(distro, release string) string { - if _, supportedDistro := supportedDistros[distro]; !supportedDistro { - distro = "fedora" + if !IsDistroSupported(distro) { + distro = distroDefault } distroObj, supportedDistro := supportedDistros[distro] @@ -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,16 +636,18 @@ 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 - if viper.IsSet("general.distro") { - distro = viper.GetString("general.distro") - } + distro, _ = ResolveDistro(distro) } - if _, supportedDistro := supportedDistros[distro]; !supportedDistro { - distro = "fedora" + if !IsDistroSupported(distro) { + distro = distroDefault } distroObj, supportedDistro := supportedDistros[distro] @@ -712,6 +752,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). @@ -726,10 +793,7 @@ func ResolveImageName(distroCLI, imageCLI, releaseCLI string) (string, string, e distro, image, release := distroCLI, imageCLI, releaseCLI if distroCLI == "" { - distro = distroDefault - if viper.IsSet("general.distro") { - distro = viper.GetString("general.distro") - } + distro, _ = ResolveDistro(distroCLI) } if distro != distroDefault && releaseCLI == "" && !viper.IsSet("general.release") { 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"