Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow toolbox containers to be created with a custom hostname #1007

Closed
wants to merge 3 commits into from
Closed

Allow toolbox containers to be created with a custom hostname #1007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
244bb82
feat: allow custom hostnames for new containers
buck-ross Jan 28, 2022
4f2cf0f
test: add tests for `toolbox create --hostname ??`
buck-ross Jan 28, 2022
a648118
doc: cover new `toolbox create --hostname` flag
buck-ross Jan 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions doc/toolbox-create.1.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ toolbox\-create - Create a new toolbox container

## SYNOPSIS
**toolbox create** [*--distro DISTRO* | *-d DISTRO*]
[*--hostname HOSTNAME*]
[*--image NAME* | *-i NAME*]
[*--release RELEASE* | *-r RELEASE*]
[*CONTAINER*]
Expand Down Expand Up @@ -85,6 +86,11 @@ Create a toolbox container for a different operating system DISTRO than the
host. Cannot be used with `--image`. Has to be coupled with `--release` unless
the selected DISTRO matches the host.

**--hostname** HOSTNAME

Initializes the netowork hostname of the toolbox container to the specified value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo alert: netowork

If not specified, this will default to **toolbox**.

**--image** NAME, **-i** NAME

Change the NAME of the image used to create the toolbox container. This is
Expand Down
27 changes: 24 additions & 3 deletions src/cmd/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"fmt"
"os"
"path/filepath"
"regexp"
"strings"
"time"

Expand All @@ -44,6 +45,7 @@ var (
createFlags struct {
container string
distro string
hostname string
image string
release string
}
Expand Down Expand Up @@ -78,6 +80,12 @@ func init() {
"",
"Create a toolbox container for a different operating system distribution than the host")

flags.StringVarP(&createFlags.hostname,
"hostname",
"",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: if you use flags.StringVar (ie., without the trailing P), then this empty parameter can be dropped.

"",
"Set the container's hostname (defaults to 'toolbox')")

flags.StringVarP(&createFlags.image,
"image",
"i",
Expand All @@ -95,6 +103,10 @@ func init() {
}

func create(cmd *cobra.Command, args []string) error {
// This regex filters out strings which are not valid hostnames, according to RFC-1123.
// Source: https://stackoverflow.com/a/106223
var hostnameRegexp = regexp.MustCompile("^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]*[a-zA-Z0-9])\\.)*([A-Za-z0-9]|[A-Za-z0-9][a-zA-Z0-9\\-]*[a-zA-Z0-9])$")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to have this regular expression directly in the code? Can't we use some Go package for it?


if utils.IsInsideContainer() {
if !utils.IsInsideToolboxContainer() {
return errors.New("this is not a toolbox container")
Expand All @@ -115,8 +127,13 @@ func create(cmd *cobra.Command, args []string) error {
return errors.New("options --image and --release cannot be used together")
}

if cmd.Flag("hostname").Changed && !hostnameRegexp.MatchString(cmd.Flag("hostname").Value.String()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use createFlags.hostname instead of cmd.Flag("hostname").Value.String().

return errors.New("invalid hostname")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be good to improve this error. See the examples in src/cmd/utils.go.

}

var container string
var containerArg string
var hostname = cmd.Flag("hostname").Value.String()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't necessary ...


if len(args) != 0 {
container = args[0]
Expand Down Expand Up @@ -158,14 +175,14 @@ func create(cmd *cobra.Command, args []string) error {
return err
}

if err := createContainer(container, image, release, true); err != nil {
if err := createContainer(container, image, release, hostname, true); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... because you can pass createFlags.hostname here.

return err
}

return nil
}

func createContainer(container, image, release string, showCommandToEnter bool) error {
func createContainer(container, image, release string, hostname string, showCommandToEnter bool) error {
if container == "" {
panic("container not specified")
}
Expand All @@ -178,6 +195,10 @@ func createContainer(container, image, release string, showCommandToEnter bool)
panic("release not specified")
}

if hostname == "" {
hostname = "toolbox"
}

enterCommand := getEnterCommand(container)

logrus.Debugf("Checking if container %s already exists", container)
Expand Down Expand Up @@ -392,7 +413,7 @@ func createContainer(container, image, release string, showCommandToEnter bool)
createArgs = append(createArgs, xdgRuntimeDirEnv...)

createArgs = append(createArgs, []string{
"--hostname", "toolbox",
"--hostname", hostname,
"--ipc", "host",
"--label", "com.github.containers.toolbox=true",
}...)
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ func runCommand(container string,
return nil
}

if err := createContainer(container, image, release, false); err != nil {
if err := createContainer(container, image, release, "", false); err != nil {
return err
}
} else if containersCount == 1 && defaultContainer {
Expand Down
19 changes: 19 additions & 0 deletions test/system/101-create.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,25 @@ teardown() {
assert_output --regexp "Created[[:blank:]]+fedora-toolbox-32"
}

@test "create: Create a container with a custom hostname ('test.host-name.local')" {
run $TOOLBOX -y create --hostname test.host-name.local
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be good to use --separate-stderr and be strict about the streams where things get written to.


assert_success
assert_output --partial "Enter with: toolbox enter"

run $TOOLBOX -y run -- hostname

assert_success
assert_output --partial "test.host-name.local"
}

@test "create: Create a container with an invalid hostname ('test.host-name.local.')" {
run $TOOLBOX -y create --hostname test.host-name.local.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto about --separate-stderr, and also use -1 to assert the exact exit code.


assert_failure
assert_line --index 0 "Error: invalid hostname"
}

@test "create: Try to create a container based on non-existent image" {
run $TOOLBOX -y create -i foo.org/bar

Expand Down