Do not make read-only mounts recursively read-only by default (also updates Docker client module to v25)

[AkihiroSuda]

Docker v25 (API v1.44) treats read-only mounts as recursively read-only by default, but this appeared to be too much breaking for Kubernetes.

So cri-dockerd hasto disable RRO by setting BindOptions.ReadOnlyNonRecursive.

Fix #309

Should be merged after:

• CI: run SELinux tests with Rocky Linux 8 #310

[thaJeztah]

Hm... this feels "wrong". Did we not expose the right options in the API types? I think the backend is not expected to be used externally (should we consider making that internal?).

[thaJeztah]

Maybe it's expected in cri-dockerd though if it's implementing API and Backend (but I honestly would have to look at how cri-dockerd is implemented). Thought I'd at least leave a comment that this stood out to me.

[AkihiroSuda]

dockerbackend.ContainerCreateConfig is used here

cri-dockerd/libdocker/kube_docker_client.go

Lines 143 to 167 in 8ae7a0e

	func (d *kubeDockerClient) CreateContainer(
	opts dockertypes.ContainerCreateConfig,
	) (*dockercontainer.CreateResponse, error) {
	ctx, cancel := context.WithTimeout(context.Background(), d.timeout)
	defer cancel()
	// we provide an explicit default shm size as to not depend on docker daemon.
	if opts.HostConfig != nil && opts.HostConfig.ShmSize <= 0 {
	opts.HostConfig.ShmSize = defaultShmSize
	}
	createResp, err := d.client.ContainerCreate(
	ctx,
	opts.Config,
	opts.HostConfig,
	opts.NetworkingConfig,
	nil,
	opts.Name,
	)
	if ctxErr := contextError(ctx); ctxErr != nil {
	return nil, ctxErr
	}
	if err != nil {
	return nil, err
	}
	return &createResp, nil
	}

I guess it is possible to remove this dependency, but it is beyond the topic of this PR.

[thaJeztah]

Yes, it looks like the client (still) has this awkward signature to pass all those individual structs as separate arguments, but uses a local (non-exported) "ad-hoc" struct to create the equivalent for the request (grouping them in a single struct); https://github.com/moby/moby/blob/e61c425cc283ed85a6a87ab4750b52389aea4021/client/container_create.go#L15-L24

type configWrapper struct {
	*container.Config
	HostConfig       *container.HostConfig
	NetworkingConfig *network.NetworkingConfig
}

// ContainerCreate creates a new container based on the given configuration.
// It can be associated with a name, but it's not mandatory.
func (cli *Client) ContainerCreate(ctx context.Context, config *container.Config, hostConfig *container.HostConfig, networkingConfig *network.NetworkingConfig, platform *ocispec.Platform, containerName string) (container.CreateResponse, error) {
	var response container.CreateResponse

https://github.com/moby/moby/blob/e61c425cc283ed85a6a87ab4750b52389aea4021/client/container_create.go#L75-L81

	body := configWrapper{
		Config:           config,
		HostConfig:       hostConfig,
		NetworkingConfig: networkingConfig,
	}

	serverResp, err := cli.post(ctx, "/containers/create", query, body, **nil)

We should probably consider to either change that signature, and define a CreateOptions struct for the client, or (perhaps) for cri-dockerd to (for now) define a similar struct on its side for the time being

/cc @neersighted

[AkihiroSuda]

@nwneisen Could you take a look?

[nwneisen]

LGTM

[AkihiroSuda]

rebased