cli icon indicating copy to clipboard operation
cli copied to clipboard

feat(service): support comma-separated values for host-to-IP mappings

Open kosiew opened this issue 5 months ago • 3 comments

PR Title: support comma-separated values for --host-add in docker service update

Closes https://github.com/moby/moby/issues/50769

- What I did

  • Updated --host-add flag in service update to support comma-separated host mappings (e.g., --host-add a:1.1.1.1,b:2.2.2.2).
  • Introduced ListOptsCSV, a wrapper around ListOpts that implements pflag.SliceValue for parsing comma-separated lists.
  • Adjusted updateHosts to use pflag.SliceValue instead of ListOpts.
  • Updated unit tests in update_test.go to cover multiple host mappings provided in a single flag instance.
  • Updated CLI documentation to reflect support for comma-separated host mappings.

- How I did it

  • Implemented a new ListOptsCSV type in opts/opts.go that handles splitting and validating comma-separated values.
  • Switched flags.Var(&options.hosts, flagHostAdd, ...) to use opts.NewListOptsCSV(&options.hosts) in update.go.
  • Updated updateHosts to retrieve values via pflag.SliceValue.
  • Modified the service update reference docs to note that multiple entries can be separated by commas.

- How to verify it

  1. Run docker service update --host-add a:1.1.1.1,b:2.2.2.2 <service>.
  2. Inspect the service to confirm that both mappings (a → 1.1.1.1 and b → 2.2.2.2) are applied.
  3. Check that existing behavior (multiple --host-add flags) still works as expected.
  4. Run updated unit tests: go test ./cli/command/service/....

- Human readable description for the release notes

Added support for specifying multiple `--host-add` entries in a single flag instance using comma-separated values in `docker service update`.

- A picture of a cute animal (not mandatory but encouraged) 🦦

kosiew avatar Aug 25 '25 07:08 kosiew

I'm a bit on the fence on adding this; I think the problem is not (not directly) in the --host-add flag, but the --host-rm flag not validating.

As far as I'm aware, none of the service create/update options were designed to take a comma-separated list, but they can be specified multiple times.

Reading the related ticket https://github.com/moby/moby/issues/50769

It should probably behave the same way as --host-rm since "docker service update --host-rm host1:192.168.1.111,host2:192.168.1.222" my_service does work.

I don't think that's correct; it doesn't produce an error, but it doesn't work (so the problem here may be more along the lines of --host-rm missing validation;

Taking this example; a service with 4 "extra hosts" added;

docker service create --name foo --host one:127.0.0.1 --host two:127.0.0.1 --host three:127.0.0.1 --host four:127.0.0.1 nginx:alpine
docker service inspect --format '{{ json .Spec.TaskTemplate.ContainerSpec.Hosts }}' foo

["127.0.0.1 one","127.0.0.1 two","127.0.0.1 three","127.0.0.1 four"]

Using the --host-rm flag with comma-separated values (either full host:IP or just host gets silently ignored. This was likely to allow using --host-rm to specify hosts that were not present (i.e., making it idempotent), but in this case it resulted in the invalid value being removed;

docker service update --host-rm one:127.0.0.1,two:127.0.0.1 foo
docker service inspect --format '{{ json .Spec.TaskTemplate.ContainerSpec.Hosts }}' foo

["127.0.0.1 one","127.0.0.1 two","127.0.0.1 three","127.0.0.1 four"]

docker service update --host-rm one,two foo
docker service inspect --format '{{ json .Spec.TaskTemplate.ContainerSpec.Hosts }}' foo
["127.0.0.1 one","127.0.0.1 two","127.0.0.1 three","127.0.0.1 four"]

Specifying as separate options works;

docker service update --host-rm one:127.0.0.1 --host-rm two:127.0.0.1 foo
docker service inspect --format '{{ json .Spec.TaskTemplate.ContainerSpec.Hosts }}' foo
["127.0.0.1 three","127.0.0.1 four"]

docker service update --host-rm three --host-rm four foo
docker service inspect --format '{{ json .Spec.TaskTemplate.ContainerSpec.Hosts }}' foo
null

thaJeztah avatar Aug 25 '25 08:08 thaJeztah

Possibly changing both flagHostAdd and flagHostRm to have a validator would fix the issue;

diff --git a/cli/command/service/update.go b/cli/command/service/update.go
index 413d1a125b..f55d5df794 100644
--- a/cli/command/service/update.go
+++ b/cli/command/service/update.go
@@ -61,7 +61,7 @@ func newUpdateCommand(dockerCLI command.Cli) *cobra.Command {
 	flags.SetAnnotation(flagDNSOptionRemove, "version", []string{"1.25"})
 	flags.Var(newListOptsVar(), flagDNSSearchRemove, "Remove a DNS search domain")
 	flags.SetAnnotation(flagDNSSearchRemove, "version", []string{"1.25"})
-	flags.Var(newListOptsVar(), flagHostRemove, `Remove a custom host-to-IP mapping ("host:ip")`)
+	flags.Var(newListOptsVarWithValidator(opts.ValidateExtraHost), flagHostRemove, `Remove a custom host-to-IP mapping ("host:ip")`)
 	flags.SetAnnotation(flagHostRemove, "version", []string{"1.25"})
 	flags.Var(&options.labels, flagLabelAdd, "Add or update a service label")
 	flags.Var(&options.containerLabels, flagContainerLabelAdd, "Add or update a container label")
@@ -95,7 +95,7 @@ func newUpdateCommand(dockerCLI command.Cli) *cobra.Command {
 	flags.SetAnnotation(flagDNSOptionAdd, "version", []string{"1.25"})
 	flags.Var(&options.dnsSearch, flagDNSSearchAdd, "Add or update a custom DNS search domain")
 	flags.SetAnnotation(flagDNSSearchAdd, "version", []string{"1.25"})
-	flags.Var(&options.hosts, flagHostAdd, `Add a custom host-to-IP mapping ("host:ip")`)
+	flags.Var(newListOptsVarWithValidator(opts.ValidateExtraHost), flagHostAdd, `Add a custom host-to-IP mapping ("host:ip")`)
 	flags.SetAnnotation(flagHostAdd, "version", []string{"1.25"})
 	flags.BoolVar(&options.init, flagInit, false, "Use an init inside each service container to forward signals and reap processes")
 	flags.SetAnnotation(flagInit, "version", []string{"1.37"})

thaJeztah avatar Aug 25 '25 08:08 thaJeztah

hi @thaJeztah

Do you mean I should open and fix this new issue

Issue: --host-rm does not validate comma-separated values and silently ignores them ?

kosiew avatar Aug 26 '25 09:08 kosiew