[BUG] Possible regression creating host paths by default for bind mounts

[mvdbos]

Description

Creating this bug report on request of @thaJeztah.

In this comment on #2781, the following behaviour is described for the creation of host paths for volumes/mounts:

So the short syntax does have the "create host path" behaviour:
...
whereas the long syntax does not:

services:
  foo:
    volumes:
      - type: bind
        source: ./path/on/host
        target: /path/in/container

unless you explicitly add the following:

        ...
        bind:
          create_host_path: true

Unfortunately, the conclusion that the long syntax does not create the host path automatically is incorrect. Host paths are always created, unless create_host_path: false is explicitly set. This issue is not caused by the spec implementation used by the CLI, but by the CLI itself. This may be a regression introduced in #12734.

In create.go there is this piece of logic when creating the mounts/volumes:

​    switch m.Type {
    case mount.TypeBind:
      // `Mount` is preferred but does not offer option to created host path if missing
      // so `Bind` API is used here with raw volume string
      // see https://github.com/moby/moby/issues/43483
      v := findVolumeByTarget(service.Volumes, m.Target)
      if v != nil {
        if v.Type != types.VolumeTypeBind {
          v.Source = m.Source
        }
        if !bindRequiresMountAPI(v.Bind) {
          source := m.Source
          if vol := findVolumeByName(p.Volumes, m.Source); vol != nil {
            source = m.Source
          }
          binds = append(binds, toBindString(source, v))
          continue
        }
      }
    case mount.TypeVolume:
      v := findVolumeByTarget(service.Volumes, m.Target)
      vol := findVolumeByName(p.Volumes, m.Source)
      if v != nil && vol != nil {
        // Prefer the bind API if no advanced option is used, to preserve backward compatibility
        if !volumeRequiresMountAPI(v.Volume) {
          binds = append(binds, toBindString(vol.Name, v))
          continue
        }
      }

Here we can see that both bind mounts and volume mounts are converted to a bind string if the mount does not use any features which require the mount API. This is checked in bindRequiresMountAPI. Let's have a look at that check:

func bindRequiresMountAPI(bind *types.ServiceVolumeBind) bool {
	switch {
	case bind == nil:
		return false
	case !bind.CreateHostPath:
		return true
	case bind.Propagation != "":
		return true
	case bind.Recursive != "":
		return true
	default:
		return false
	}
}

Note that if CreateHostPath is not explicitly set to false, and no other bind options are set in the yaml, the case bind == nil will match. This is because the whole ServiceVolumeBindpointer is unmarshaled to nil if it is not in the yaml, since it is tagged omitempty. It was probably expected to be present with all its values set to zero, to match the case !bind.CreateHostPath

This implementation results in it being treated as a normal volume, and therefore the host path will be created automatically later on, even though the value of CreateHostPath, i.e. create_host_path is not set to to true.

Steps To Reproduce

1. Remove lines 371-372 from pkg/compose/create_test.go.
2. Run Test_buildContainerVolumes. It now fails.

This testcase should still be successful if bind mounts only create host paths if explicitly asked to with:

bind:
    create_host_path: true

Compose Version

v2.34.0-desktop.1 and in main

Docker Environment

Anything else?

No response

[ndeloof]

Host paths are always created, unless create_host_path: false is explicitly set

Indeed. create_host_path: true is the default value, this attribute only has an actual effect if set to false, and force Docker Compose to rely on the mount API to pass this extra flag
Not a bug

[sparrowt]

https://docs.docker.com/reference/compose-file/services/ doesn't mention a default and in fact the text here:

create_host_path: Creates a directory at the source path on host if there is nothing present. Compose does nothing if there is something present at the path. This is automatically implied by short syntax for backward compatibility with docker-compose legacy.

where it says "This is automatically implied by short syntax" could be interpreted as implying that it is not automatically implied in the "long syntax".

Is there some missing documentation about this? At the very least it is ambiguous.

Also has this behaviour changed in recent years? I can't be 100% confident but I'm pretty sure that when I wrote this comment it was based on my empirical observation of the behaviour i.e. that create_host_path was defaulting to false in my experience if not specified.

[ndeloof]

create_host_path was introduced to support this specific case and always has been set as true by default

[mvdbos]

Regardless of the intended behaviour and whether it has changed or not: I agree with @sparrowt that the documentation on this could be made a bit more explicit. The recurring questions/comments on this prove that it is currently ambiguous.

Happy to make a minimal PR for that if we agree.

[ndeloof]

help on docs improvements is always welcome

[mvdbos]

PR: compose-spec/compose-spec#599