docker stack: allow '=' separator in extra_hosts #4860
Conversation
cli/compose/loader/loader.go
Outdated
| } | ||
|
|
||
| func transformListOrMapping(listOrMapping any, sep string, allowNil bool) any { | ||
| func transformListOrMapping(listOrMapping any, sep string, allowNil bool, allowSeps []string) any { |
There was a problem hiding this comment.
Could it make sense to change the return type here to []string instead of any now that these are the only options?
There was a problem hiding this comment.
Yes, might as well - done.
cli/compose/loader/loader.go
Outdated
| } | ||
| } | ||
| 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 |
There was a problem hiding this comment.
This description could be moved to be a proper docstring for the transformListOrMapping func below for better IDE integration
| - "alpha:50.31.209.229" | ||
| - "zulu:ff02::1" | ||
| - "host.docker.internal:host-gateway" | ||
| - "whiskey=ff02::2" | ||
| - "foxtrot=[ff02::3]" |
There was a problem hiding this comment.
Do we support ipv6 addresses with [] brackets and the : separator? If so we're missing a test for that
| @@ -1324,16 +1326,23 @@ services: | |||
| image: busybox | |||
| extra_hosts: | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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]>
robmry
force-pushed
the
4859_compose_extra_hosts_eq_sep
branch
from
ab9d208 to
c986d09
Compare
Codecov Report
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 |
| result := make([]string, 0, len(value)) | ||
| for _, entry := range value { | ||
| for i, allowSep := range allowSeps { | ||
| entry := fmt.Sprint(entry) |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| result := make([]string, 0, len(value)) | ||
| for _, entry := range value { | ||
| for i, allowSep := range allowSeps { | ||
| entry := fmt.Sprint(entry) |
There was a problem hiding this comment.
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
|
Added "cherry-pick" in case there will be another 25.0.x patch release to include it in. |
|
A 25.0.x patch would be great |
- What I did
The compose file format allows
=as a separator in extra hosts, anddocker compose configuses 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'.