diff --git a/src/cmd/create.go b/src/cmd/create.go index 60755be..c722f3d 100644 --- a/src/cmd/create.go +++ b/src/cmd/create.go @@ -147,28 +147,12 @@ func create(cmd *cobra.Command, args []string) error { containerArg = "--container" } - if container != "" { - if !utils.IsContainerNameValid(container) { - err := createErrorInvalidContainer(containerArg) - return err - } - } - - var release string - if createFlags.release != "" { - var err error - release, err = utils.ParseRelease(createFlags.distro, createFlags.release) - if err != nil { - hint := err.Error() - err := createErrorInvalidRelease(hint) - return err - } - } - - container, image, release, err := utils.ResolveContainerAndImageNames(container, + container, image, release, err := resolveContainerAndImageNames(container, + containerArg, createFlags.distro, createFlags.image, - release) + createFlags.release) + if err != nil { return err } diff --git a/src/cmd/enter.go b/src/cmd/enter.go index 7b9452e..9a94c3d 100644 --- a/src/cmd/enter.go +++ b/src/cmd/enter.go @@ -100,27 +100,18 @@ func enter(cmd *cobra.Command, args []string) error { if container != "" { defaultContainer = false - - if !utils.IsContainerNameValid(container) { - err := createErrorInvalidContainer(containerArg) - return err - } } - var release string if enterFlags.release != "" { defaultContainer = false - - var err error - release, err = utils.ParseRelease(enterFlags.distro, enterFlags.release) - if err != nil { - hint := err.Error() - err := createErrorInvalidRelease(hint) - return err - } } - container, image, release, err := utils.ResolveContainerAndImageNames(container, enterFlags.distro, "", release) + container, image, release, err := resolveContainerAndImageNames(container, + containerArg, + enterFlags.distro, + "", + enterFlags.release) + if err != nil { return err } diff --git a/src/cmd/rootMigrationPath.go b/src/cmd/rootMigrationPath.go index f3b9f45..5c97a4f 100644 --- a/src/cmd/rootMigrationPath.go +++ b/src/cmd/rootMigrationPath.go @@ -60,7 +60,7 @@ func rootRunImpl(cmd *cobra.Command, args []string) error { return nil } - container, image, release, err := utils.ResolveContainerAndImageNames("", "", "", "") + container, image, release, err := resolveContainerAndImageNames("", "", "", "", "") if err != nil { return err } diff --git a/src/cmd/run.go b/src/cmd/run.go index 20965f5..68d6b52 100644 --- a/src/cmd/run.go +++ b/src/cmd/run.go @@ -100,24 +100,10 @@ func run(cmd *cobra.Command, args []string) error { if runFlags.container != "" { defaultContainer = false - - if !utils.IsContainerNameValid(runFlags.container) { - err := createErrorInvalidContainer("--container") - return err - } } - var release string if runFlags.release != "" { defaultContainer = false - - var err error - release, err = utils.ParseRelease(runFlags.distro, runFlags.release) - if err != nil { - hint := err.Error() - err := createErrorInvalidRelease(hint) - return err - } } if len(args) == 0 { @@ -131,7 +117,12 @@ func run(cmd *cobra.Command, args []string) error { command := args - container, image, release, err := utils.ResolveContainerAndImageNames(runFlags.container, runFlags.distro, "", release) + container, image, release, err := resolveContainerAndImageNames(runFlags.container, + "--container", + runFlags.distro, + "", + runFlags.release) + if err != nil { return err } diff --git a/src/cmd/utils.go b/src/cmd/utils.go index 42d4428..911d0a7 100644 --- a/src/cmd/utils.go +++ b/src/cmd/utils.go @@ -81,6 +81,17 @@ func createErrorInvalidContainer(containerArg string) error { return errors.New(errMsg) } +func createErrorInvalidImageForContainerName(container string) error { + var builder strings.Builder + fmt.Fprintf(&builder, "invalid argument for '--image'\n") + fmt.Fprintf(&builder, "Container name %s generated from image is invalid.\n", container) + fmt.Fprintf(&builder, "Container names must match '%s'.\n", utils.ContainerNameRegexp) + fmt.Fprintf(&builder, "Run '%s --help' for usage.", executableBase) + + errMsg := builder.String() + return errors.New(errMsg) +} + func createErrorInvalidRelease(hint string) error { var builder strings.Builder fmt.Fprintf(&builder, "invalid argument for '--release'\n") @@ -101,6 +112,45 @@ func getUsageForCommonCommands() string { return usage } +func resolveContainerAndImageNames(container, containerArg, distroCLI, imageCLI, releaseCLI string) ( + string, string, string, error, +) { + container, image, release, err := utils.ResolveContainerAndImageNames(container, + distroCLI, + imageCLI, + releaseCLI) + + if err != nil { + var errContainer *utils.ContainerError + var errParseRelease *utils.ParseReleaseError + + if errors.As(err, &errContainer) { + if errors.Is(err, utils.ErrContainerNameInvalid) { + if containerArg == "" { + panicMsg := fmt.Sprintf("unexpected %T without containerArg: %s", err, err) + panic(panicMsg) + } + + err := createErrorInvalidContainer(containerArg) + return "", "", "", err + } else if errors.Is(err, utils.ErrContainerNameFromImageInvalid) { + err := createErrorInvalidImageForContainerName(errContainer.Container) + return "", "", "", err + } else { + panicMsg := fmt.Sprintf("unexpected %T: %s", err, err) + panic(panicMsg) + } + } else if errors.As(err, &errParseRelease) { + err := createErrorInvalidRelease(errParseRelease.Hint) + return "", "", "", err + } else { + return "", "", "", err + } + } + + return container, image, release, nil +} + // showManual tries to open the specified manual page using man on stdout func showManual(manual string) error { manBinary, err := exec.LookPath("man") diff --git a/src/meson.build b/src/meson.build index 30b771a..9adc42a 100644 --- a/src/meson.build +++ b/src/meson.build @@ -20,6 +20,7 @@ sources = files( 'cmd/utils.go', 'pkg/podman/podman.go', 'pkg/shell/shell.go', + 'pkg/utils/errors.go', 'pkg/utils/utils.go', 'pkg/version/version.go', ) diff --git a/src/pkg/utils/errors.go b/src/pkg/utils/errors.go new file mode 100644 index 0000000..754e5a1 --- /dev/null +++ b/src/pkg/utils/errors.go @@ -0,0 +1,44 @@ +/* + * Copyright © 2022 Red Hat Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package utils + +import ( + "fmt" +) + +type ContainerError struct { + Container string + Image string + Err error +} + +type ParseReleaseError struct { + Hint string +} + +func (err *ContainerError) Error() string { + errMsg := fmt.Sprintf("%s: %s", err.Container, err.Err) + return errMsg +} + +func (err *ContainerError) Unwrap() error { + return err.Err +} + +func (err *ParseReleaseError) Error() string { + return err.Hint +} diff --git a/src/pkg/utils/utils.go b/src/pkg/utils/utils.go index acef5a5..0994a95 100644 --- a/src/pkg/utils/utils.go +++ b/src/pkg/utils/utils.go @@ -117,6 +117,10 @@ var ( var ( ContainerNameDefault string + + ErrContainerNameFromImageInvalid = errors.New("container name generated from image is invalid") + + ErrContainerNameInvalid = errors.New("container name is invalid") ) func init() { @@ -605,16 +609,9 @@ func ShortID(id string) string { return id } -func ParseRelease(distro, release string) (string, error) { +func parseRelease(distro, release string) (string, error) { if distro == "" { - distro = distroDefault - if viper.IsSet("general.distro") { - distro = viper.GetString("general.distro") - } - } - - if _, supportedDistro := supportedDistros[distro]; !supportedDistro { - distro = distroFallback + panic("distro not specified") } distroObj, supportedDistro := supportedDistros[distro] @@ -636,11 +633,11 @@ func parseReleaseFedora(release string) (string, error) { releaseN, err := strconv.Atoi(release) if err != nil { logrus.Debugf("Parsing release %s as an integer failed: %s", release, err) - return "", errors.New("The release must be a positive integer.") + return "", &ParseReleaseError{"The release must be a positive integer."} } if releaseN <= 0 { - return "", errors.New("The release must be a positive integer.") + return "", &ParseReleaseError{"The release must be a positive integer."} } return release, nil @@ -648,17 +645,17 @@ func parseReleaseFedora(release string) (string, error) { func parseReleaseRHEL(release string) (string, error) { if i := strings.IndexRune(release, '.'); i == -1 { - return "", errors.New("The release must be in the '.' format.") + return "", &ParseReleaseError{"The release must be in the '.' format."} } releaseN, err := strconv.ParseFloat(release, 32) if err != nil { logrus.Debugf("Parsing release %s as a float failed: %s", release, err) - return "", errors.New("The release must be in the '.' format.") + return "", &ParseReleaseError{"The release must be in the '.' format."} } if releaseN <= 0 { - return "", errors.New("The release must be a positive number.") + return "", &ParseReleaseError{"The release must be a positive number."} } return release, nil @@ -730,6 +727,11 @@ func ResolveContainerAndImageNames(container, distroCLI, imageCLI, releaseCLI st } } + release, err := parseRelease(distro, release) + if err != nil { + return "", "", "", err + } + if imageCLI == "" { image = getDefaultImageForDistro(distro, release) @@ -765,6 +767,14 @@ func ResolveContainerAndImageNames(container, distroCLI, imageCLI, releaseCLI st if tag != "" { container = container + "-" + tag } + + if !IsContainerNameValid(container) { + return "", "", "", &ContainerError{container, image, ErrContainerNameFromImageInvalid} + } + } else { + if !IsContainerNameValid(container) { + return "", "", "", &ContainerError{container, "", ErrContainerNameInvalid} + } } logrus.Debug("Resolved container and image names") diff --git a/src/pkg/utils/utils_test.go b/src/pkg/utils/utils_test.go index ec68036..9197182 100644 --- a/src/pkg/utils/utils_test.go +++ b/src/pkg/utils/utils_test.go @@ -150,7 +150,7 @@ func TestParseRelease(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - release, err := ParseRelease(tc.inputDistro, tc.inputRelease) + release, err := parseRelease(tc.inputDistro, tc.inputRelease) if tc.ok { assert.NoError(t, err) diff --git a/test/system/101-create.bats b/test/system/101-create.bats index 1c9c2d7..e03518c 100644 --- a/test/system/101-create.bats +++ b/test/system/101-create.bats @@ -56,6 +56,19 @@ teardown() { assert_line --index 2 "Run 'toolbox --help' for usage." } +@test "create: Try to create a container with invalid custom image ('ßpeci@l.Nam€')" { + local image="ßpeci@l.Nam€" + + run $TOOLBOX create --image "$image" + + assert_failure + assert_line --index 0 "Error: invalid argument for '--image'" + assert_line --index 1 "Container name $image generated from image is invalid." + assert_line --index 2 "Container names must match '[a-zA-Z0-9][a-zA-Z0-9_.-]*'." + assert_line --index 3 "Run 'toolbox --help' for usage." + assert [ ${#lines[@]} -eq 4 ] +} + @test "create: Create a container with a distro and release options ('fedora'; f32)" { pull_distro_image fedora 32