Increase the validation coverage for the container & release options

Currently, the container name and release are only validated if they
were specified as command line options.  Neither the value of release
in the configuration file nor the container name generated from an
image are validated.

There's also a lot of repeated code in the command front-ends to
validate the container name and release.  This opens the door for
mistakes.  Any adjustment to the code must be repeated elsewhere, and
there are subtle interactions and overlaps between the validation code
and the code to resolve container and image names.

It's worth noting that the container and image name resolution happens
for both the command line and configuration file options, and generates
the container name from the image when necessary.

Therefore, validating everything while resolving cleans up the command
front-ends and increases the coverage of the validation.

This introduces the use of sentinel error values and custom error
implementations to identify the different errors that can occur while
resolving the container and images, so that they can be appropriately
shown to the user.

containers#937

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
 	}

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
 	}

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
 	}

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
 	}

src/cmd/utils.go

@@ -81,6 +81,17 @@ func createErrorInvalidContainer(containerArg string) error {
 	return errors.New(errMsg)
 }
 
+func createErrorInvalidImageForContainerName() error {
+	var builder strings.Builder
+	fmt.Fprintf(&builder, "invalid argument for '--image'\n")
+	fmt.Fprintf(&builder, "Image gives an invalid container name.\n")
+	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()
+				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")

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',
 )

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
+}

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,29 +633,29 @@ 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
 }
 
 func parseReleaseRHEL(release string) (string, error) {
 	if i := strings.IndexRune(release, '.'); i == -1 {
-		return "", errors.New("The release must be in the '<major>.<minor>' format.")
+		return "", &ParseReleaseError{"The release must be in the '<major>.<minor>' 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 '<major>.<minor>' format.")
+		return "", &ParseReleaseError{"The release must be in the '<major>.<minor>' 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")

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)

test/system/101-create.bats

@@ -56,6 +56,17 @@ teardown() {
   assert_line --index 2 "Run 'toolbox --help' for usage."
 }
 
+@test "create: Try to create a container with invalid custom image ('ß[email protected]€')" {
+  run $TOOLBOX create --image "ß[email protected]€"
+
+  assert_failure
+  assert_line --index 0 "Error: invalid argument for '--image'"
+  assert_line --index 1 "Image gives an invalid container name."
+  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