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

buck-ross · 2022-01-28T18:01:52Z

This feature consists of a new optional flag --hostname HOSTNAME, which can be used when invoking toolbox create to create a new container with a custom hostname.

This is essentially a re-implementation of #210 from scratch, written in golang.

This may also address many of the concerns brought up in the comments of #771, and could potentially assist in the adoption of a new default hostname, since it allows for more flexibility in that regard.

This PR includes:

A new feature, which is backwards compatible with all existing use-cases of toolbox create
Associated system tests for the new --hostname flag
Associated updates to the toolbox create documentation in the manual.

The new `--hostname` flag is now available when invoking `toolbox create` in order to create a container with a custom hostname. The new flag does not enforce network-resolvability of any custom hostnames, but it does require that the hostname is valid, per RFC 1123. To preserve backward-compatibility, the hostname will still default to "toolbox" if the flag is not specified. Signed-off-by: Buckley Ross <[email protected]> Change-Id: Ib3c0706e3a93a5748453e39998b9893e3e57c305

These tests ensure that the new `--hostname` flag works as intended. Signed-off-by: Buckley Ross <[email protected]> Change-Id: Id7b7775fe73a6600afe2dabb0d905be5ba17bb57

Updated the `toolbox create` manual page to cover the usage of the new `--hostname` flag for specifying custom hostnames when creating new containers. Signed-off-by: Buckley Ross <[email protected]> Change-Id: I16d7f63769c17aec1ff69d34ac8fe28f48827777

softwarefactory-project-zuul · 2022-01-28T18:37:32Z

Build succeeded.

unit-test : SUCCESS in 10m 02s
system-test-fedora-rawhide : SUCCESS in 33m 04s
system-test-fedora-35 : SUCCESS in 14m 31s
system-test-fedora-34 : SUCCESS in 15m 59s

debarshiray · 2022-10-14T00:51:19Z

This pull request has been replaced by #1134

Why was this replaced? Was this rebased against main?

debarshiray · 2022-10-14T01:47:22Z

src/cmd/create.go

@@ -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])$")


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

debarshiray · 2022-11-17T16:47:30Z

doc/toolbox-create.1.md

@@ -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.


Typo alert: netowork

debarshiray

This is an implementation of #563 As I mentioned there already, let's first try to improve the default hostname (see #969). Customizing the hostname might not always be necessary to distinguish one container from another (see #98).

Regardless, here are some comments on the specific changes in this pull request. Maybe they might be of help for future work.

debarshiray · 2022-11-17T16:52:27Z

src/cmd/create.go

@@ -78,6 +80,12 @@ func init() {
 		"",
 		"Create a toolbox container for a different operating system distribution than the host")

+	flags.StringVarP(&createFlags.hostname,
+		"hostname",
+		"",


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

debarshiray · 2022-11-17T16:55:41Z

test/system/101-create.bats

@@ -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


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

debarshiray · 2022-11-17T16:56:25Z

test/system/101-create.bats

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


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

debarshiray · 2022-11-17T16:58:46Z

src/cmd/create.go

@@ -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()) {


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

debarshiray · 2022-11-17T16:59:35Z

src/cmd/create.go

@@ -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()) {
+		return errors.New("invalid hostname")


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

debarshiray · 2022-11-17T17:00:12Z

src/cmd/create.go

 	var container string
 	var containerArg string
+	var hostname = cmd.Flag("hostname").Value.String()


This isn't necessary ...

debarshiray · 2022-11-17T17:00:32Z

src/cmd/create.go

@@ -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 {


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

buck-ross added 3 commits January 28, 2022 12:49

test: add tests for toolbox create --hostname ??

4f2cf0f

These tests ensure that the new `--hostname` flag works as intended. Signed-off-by: Buckley Ross <[email protected]> Change-Id: Id7b7775fe73a6600afe2dabb0d905be5ba17bb57

akdev1l mentioned this pull request Aug 17, 2022

fix: use FQDN with toolbox prefix #1086

Open

matthiasclasen mentioned this pull request Oct 3, 2022

Offer a better way to visually distinguish between containers #98

Open

nievesmontero mentioned this pull request Oct 13, 2022

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

Closed

matthiasclasen closed this Oct 13, 2022

debarshiray reviewed Oct 14, 2022

View reviewed changes

debarshiray reviewed Nov 17, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

buck-ross commented Jan 28, 2022

softwarefactory-project-zuul bot commented Jan 28, 2022

debarshiray commented Oct 14, 2022

debarshiray Oct 14, 2022

debarshiray Nov 17, 2022

debarshiray left a comment •

edited

Loading

debarshiray Nov 17, 2022

debarshiray Nov 17, 2022

debarshiray Nov 17, 2022

debarshiray Nov 17, 2022

debarshiray Nov 17, 2022

debarshiray Nov 17, 2022

debarshiray Nov 17, 2022

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

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

Conversation

buck-ross commented Jan 28, 2022

softwarefactory-project-zuul bot commented Jan 28, 2022

debarshiray commented Oct 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debarshiray left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debarshiray left a comment •

edited

Loading