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

docker stack: allow '=' separator in extra_hosts #4860

Merged
merged 1 commit into from Feb 8, 2024
Merged

docker stack: allow '=' separator in extra_hosts #4860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 41 additions & 11 deletions cli/compose/loader/loader.go
View file
Open in desktop
Expand Up @@ -328,7 +328,7 @@ func createTransformHook(additionalTransformers ...Transformer) mapstructure.Dec
reflect.TypeOf(types.MappingWithEquals{}): transformMappingOrListFunc("=", true),
reflect.TypeOf(types.Labels{}): transformMappingOrListFunc("=", false),
reflect.TypeOf(types.MappingWithColon{}): transformMappingOrListFunc(":", false),
reflect.TypeOf(types.HostsList{}): transformListOrMappingFunc(":", false),
reflect.TypeOf(types.HostsList{}): transformHostsList,
reflect.TypeOf(types.ServiceVolumeConfig{}): transformServiceVolumeConfig,
reflect.TypeOf(types.BuildConfig{}): transformBuildConfig,
reflect.TypeOf(types.Duration(0)): transformStringToDuration,
Expand Down Expand Up @@ -808,28 +808,58 @@ var transformStringList TransformerFunc = func(data any) (any, error) {
}
}

func transformMappingOrListFunc(sep string, allowNil bool) TransformerFunc {
return func(data any) (any, error) {
return transformMappingOrList(data, sep, allowNil), nil
}
}
var transformHostsList TransformerFunc = func(data any) (any, error) {
Benehiko marked this conversation as resolved.
Show resolved Hide resolved
hl := transformListOrMapping(data, ":", false, []string{"=", ":"})

func transformListOrMappingFunc(sep string, allowNil bool) TransformerFunc {
return func(data any) (any, error) {
return transformListOrMapping(data, sep, allowNil), nil
// Remove brackets from IP addresses if present (for example "[::1]" -> "::1").
result := make([]string, 0, len(hl))
for _, hip := range hl {
host, ip, _ := strings.Cut(hip, ":")
if len(ip) > 2 && ip[0] == '[' && ip[len(ip)-1] == ']' {
ip = ip[1 : len(ip)-1]
}
result = append(result, fmt.Sprintf("%s:%s", host, ip))
}
return result, nil
}

func transformListOrMapping(listOrMapping any, sep string, allowNil bool) any {
// transformListOrMapping transforms pairs of strings that may be represented as
// a map, or a list of '=' or ':' separated strings, into a list of ':' separated
// strings.
func transformListOrMapping(listOrMapping any, sep string, allowNil bool, allowSeps []string) []string {
switch value := listOrMapping.(type) {
case map[string]any:
return toStringList(value, sep, allowNil)
case []any:
return listOrMapping
result := make([]string, 0, len(value))
for _, entry := range value {
for i, allowSep := range allowSeps {
entry := fmt.Sprint(entry)
Copy link
Contributor

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

k, v, ok := strings.Cut(entry, allowSep)
if ok {
// Entry uses this allowed separator. Add it to the result, using
// sep as a separator.
result = append(result, fmt.Sprintf("%s%s%s", k, sep, v))
break
} else if i == len(allowSeps)-1 {
// No more separators to try, keep the entry if allowNil.
if allowNil {
result = append(result, k)
}
}
}
}
return result
}
panic(errors.Errorf("expected a map or a list, got %T: %#v", listOrMapping, listOrMapping))
}

func transformMappingOrListFunc(sep string, allowNil bool) TransformerFunc {
return func(data any) (any, error) {
return transformMappingOrList(data, sep, allowNil), nil
}
}

func transformMappingOrList(mappingOrList any, sep string, allowNil bool) any {
switch values := mappingOrList.(type) {
case map[string]any:
Expand Down
13 changes: 12 additions & 1 deletion cli/compose/loader/loader_test.go
View file
Open in desktop
Expand Up @@ -1302,12 +1302,14 @@ services:
extra_hosts:
"zulu": "162.242.195.82"
"alpha": "50.31.209.229"
"beta": "[fd20:f8a7:6e5b::2]"
"host.docker.internal": "host-gateway"
`)
assert.NilError(t, err)

expected := types.HostsList{
"alpha:50.31.209.229",
"beta:fd20:f8a7:6e5b::2",
"host.docker.internal:host-gateway",
"zulu:162.242.195.82",
}
Expand All @@ -1324,16 +1326,25 @@ services:
image: busybox
extra_hosts:
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

- "zulu:162.242.195.82"
- "whiskey=162.242.195.83"
- "alpha:50.31.209.229"
- "zulu:ff02::1"
- "host.docker.internal:host-gateway"
- "whiskey=ff02::2"
sam-thibault marked this conversation as resolved.
Show resolved Hide resolved
- "foxtrot=[ff02::3]"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, added.

- "bravo:[ff02::4]"
- "host.docker.internal=host-gateway"
- "noaddress"
`)
assert.NilError(t, err)

expected := types.HostsList{
"zulu:162.242.195.82",
"whiskey:162.242.195.83",
"alpha:50.31.209.229",
"zulu:ff02::1",
"whiskey:ff02::2",
"foxtrot:ff02::3",
"bravo:ff02::4",
"host.docker.internal:host-gateway",
}

Expand Down