docker stack: allow '=' separator in extra_hosts by robmry · Pull Request #4860 · docker/cli

robmry · 2024-02-07T16:57:34Z

- What I did

The compose file format allows = as a separator in extra hosts, and docker compose config uses it instead of :.

Accept either separator in docker stack deploy, converting to : for the engine's API.

Also, remove brackets from IP addresses, as they're allowed by the compose file (although not generated by docker compose config).

Fixes #4859

- How I did it

Similar to changes in the CLI's --add-hosts, buildx and compose - check for an = separator first, then try :, and strip the brackets.

- How to verify it

As described in #4859 - plus updated unit tests.

- Description for the changelog

Accept '=' separators and '[ipv6]' in compose files for 'docker stack deploy'.

krissetto · 2024-02-07T17:29:31Z

 }

-func transformListOrMapping(listOrMapping any, sep string, allowNil bool) any {
+func transformListOrMapping(listOrMapping any, sep string, allowNil bool, allowSeps []string) any {


Could it make sense to change the return type here to []string instead of any now that these are the only options?

Yes, might as well - done.

krissetto · 2024-02-07T17:30:36Z

-	}
-}
+var transformHostsList TransformerFunc = func(data any) (any, error) {
+	// Transform pairs of host and IP addr that may be represented as a map, or a list


This description could be moved to be a proper docstring for the transformListOrMapping func below for better IDE integration

krissetto · 2024-02-07T17:33:36Z

      - "zulu:ff02::1"
-      - "host.docker.internal:host-gateway"
+      - "whiskey=ff02::2"
+      - "foxtrot=[ff02::3]"


Do we support ipv6 addresses with [] brackets and the : separator? If so we're missing a test for that

Yes, added.

krissetto · 2024-02-07T17:34:36Z

@@ -1324,16 +1326,23 @@ services:
    image: busybox
    extra_hosts:


Generally, maybe it could make sense to think about writing a small func that takes in the valid separators etc and generates the list of all possible (acceptable) permutations, just to avoid maybe forgetting some possibilities behind and eventually hitting unexpected bugs. What do you think?

I think maybe that can wait until there are more permutations to deal with? (None of this code actually cares that the addresses are addresses, so the IPv4/IPv6 distinction isn't really material ... it's just sorting out the separator and removing brackets.)

extra_hosts in the compose file format allows '=' as a separator, and brackets around IP addresses, the engine API doesn't. So, transform the values when reading a compose file for 'docker stack'. Signed-off-by: Rob Murray <[email protected]>

codecov-commenter · 2024-02-07T17:57:46Z

Codecov Report

Merging #4860 (c986d09) into master (324309b) will increase coverage by 0.01%.
Report is 1 commits behind head on master.
The diff coverage is 92.30%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4860      +/-   ##
==========================================
+ Coverage   61.29%   61.31%   +0.01%     
==========================================
  Files         287      287              
  Lines       20041    20058      +17     
==========================================
+ Hits        12285    12298      +13     
- Misses       6865     6867       +2     
- Partials      891      893       +2

krissetto · 2024-02-07T18:24:28Z

+		result := make([]string, 0, len(value))
+		for _, entry := range value {
+			for i, allowSep := range allowSeps {
+				entry := fmt.Sprint(entry)


I'm guessing this is for stringify-ing entry since it's of type any in this switch case. What are the differences between casting it to string rather than using this method?

I'd have personally used a cast something like entry, ok = entry.(string) so we could handle eventual errors, but my go experience isn't all that profound and i might be missing details :)

Yes, it's probably a string, so casting it should work ... but, the old code didn't depend on it and I didn't want to change that. The map-handling half of this function calls toStringList(), which Sprintf's the any value to turn it into a string - so I went with that.

Yeah I agree It's clearly not a big issue since the other parts have been using the strategy to begin with, but maybe we could adjust those as well down the line.. I'm not sure if there are any performance implications either. Just food for thought

I think in this case fmt.Sprint would allocate a new string whereas you already have a string hiding as an any type. Casting in this case would not allocate any more memory.

In this case, I'm not sure if you'd always only get a string from this slice of any so I think sticking with the safer router of using fmt would be best imo

For context https://pkg.go.dev/fmt#Sprint
You can also see in line 278 of the source it allocates more memory newPrinter and probably later a new buffer.
https://cs.opensource.google/go/go/+/refs/tags/go1.22.0:src/fmt/print.go;l=277

Benehiko · 2024-02-08T08:05:24Z

+		result := make([]string, 0, len(value))
+		for _, entry := range value {
+			for i, allowSep := range allowSeps {
+				entry := fmt.Sprint(entry)


I think in this case fmt.Sprint would allocate a new string whereas you already have a string hiding as an any type. Casting in this case would not allocate any more memory.

In this case, I'm not sure if you'd always only get a string from this slice of any so I think sticking with the safer router of using fmt would be best imo

For context https://pkg.go.dev/fmt#Sprint
You can also see in line 278 of the source it allocates more memory newPrinter and probably later a new buffer.
https://cs.opensource.google/go/go/+/refs/tags/go1.22.0:src/fmt/print.go;l=277

thaJeztah

LGTM

thaJeztah · 2024-02-08T16:09:22Z

Added "cherry-pick" in case there will be another 25.0.x patch release to include it in.

MoogyG · 2024-02-19T23:07:02Z

A 25.0.x patch would be great

robmry self-assigned this Feb 7, 2024

robmry added area/stack area/networking version/25.0 labels Feb 7, 2024

sam-thibault reviewed Feb 7, 2024

View reviewed changes

Comment thread cli/compose/loader/loader_test.go

krissetto reviewed Feb 7, 2024

View reviewed changes

robmry force-pushed the 4859_compose_extra_hosts_eq_sep branch from ab9d208 to c986d09 Compare February 7, 2024 17:57

krissetto reviewed Feb 7, 2024

View reviewed changes

sam-thibault approved these changes Feb 7, 2024

View reviewed changes

krissetto approved these changes Feb 7, 2024

View reviewed changes

Benehiko reviewed Feb 8, 2024

View reviewed changes

Benehiko approved these changes Feb 8, 2024

View reviewed changes

thaJeztah added this to the 26.0.0 milestone Feb 8, 2024

thaJeztah approved these changes Feb 8, 2024

View reviewed changes

thaJeztah merged commit 34797d1 into docker:master Feb 8, 2024

thaJeztah added status/4-merge process/cherry-pick labels Feb 8, 2024

robmry deleted the 4859_compose_extra_hosts_eq_sep branch February 8, 2024 18:19

vvoland mentioned this pull request Mar 4, 2024

[25.0 backport] docker stack: allow '=' separator in extra_hosts #4910

Merged

vvoland added process/cherry-picked and removed process/cherry-pick labels Mar 4, 2024

Conversation

robmry commented Feb 7, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Feb 8, 2024

Uh oh!

MoogyG commented Feb 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codecov-commenter commented Feb 7, 2024 •

edited

Loading