cifs volume resolves hostname correctly

[michaelkebe]

• fixes: Unable to mount CIFS using FQDN or 'addr' option - Ubuntu 22.10 host #46180

- What I did
Fixed this issue #46180. There is a old PR which claims to fix this #39250, but is hasn't.

The problem is that, the hostname in NFS and CIFS mounts are at different places.

NFS uses the addr value in the MountOpts.
CIFS uses the hostname part of the MountDevice URL.

- How I did it

Splitted the switch case statement of #39250 for NFS and CIFS. Used the net/url Parse function to extract the hostname. And handled the rest in the same way NFS did.

- How to verify it
Create a docker volume using a hostname instead of an IP address.
Start a container using that volume.

- Description for the changelog

CIFS volumes resolving FQDN correctly.

I am not a go hacker, so maybe it's not "go idiomatic".

[vvoland]

Thank you for contributing!
I have a couple of comments.

Could you also amend the commit message and remove the "Fixes #XXX" magic string? We tend to only include these in the PR bodies to avoid Github doing auto-close when commits are merged in forks or other repositories.

[vvoland]

Suggested change

	if deviceUrl.Host != "" && net.ParseIP(deviceUrl.Host).To4() == nil {
	if deviceUrl.Host != "" && net.ParseIP(deviceUrl.Host) == nil {

I know this was just taken from the previous code, but I'm wondering if there is a reason to restrict the lookup to IPv4 addresses only?

[thaJeztah]

Original PR for nfs was #27329

I don't see an immediate mention during review about IPv4, but there was a IsIP() helper (that was later removed?) #27329 (comment)

And I saw some mention of a complex RegEx that was later removed, so I wonder if the .To4() was just use as validation for "is it an IP address?")

We should look if we can use IPv6 addresses without issues (or if they need the bracketed ([::1]) notation or escaping)

[vvoland]

Looks like in the cifs case we should be able to resolve to an IPv6 address just fine. I just checked the mount.cifs userspace helper source and it seems to handle IPv6 addresses: https://github.com/Distrotech/cifs-utils/blob/0b0b81a533a8990d9a19e80b317a320c1d8a09d3/resolve_host.c#L37

The probable reason why the original code has the IPv4 restriction was to match the mount.nfs behavior which seems to only resolve IPv4 addresses (explicit AF_INET lookup): https://git.linux-nfs.org/?p=steved/nfs-utils.git;a=blob;f=utils/mount/network.c;h=01ead49f0008ecfaee3fba4671cc9192fa8f9b99;hb=HEAD#l268 (thanks @akerouanton for finding the source!).

[michaelkebe]

Adopted your suggestions and it works. @vvoland

[vvoland]

Good work, thanks! Just 2 more things regarding the Host field.

[michaelkebe]

@vvoland

Incorporated your suggestions.

I never stumpled upon CIFS with different than default port.
Trying to add one with

docker create volume ... --opt device="//some.host.name.com:445/some/path" ...

gives me this error, when running a container using this volume:

ERRO[2023-11-29T14:30:53.631287791Z] Handler for POST /v1.43/containers/d1d6043b1890daef2b28dac0ce8053a74dc0c0932a2947bc2ea1da39d7101fe4/start returned error: error while mounting volume '/var/lib/docker/volumes/foo/_data': error resolving passed in network volume address: lookup some.host.name.com:445: no such host
docker: Error response from daemon: error while mounting volume '/var/lib/docker/volumes/foo/_data': error resolving passed in network volume address: lookup some.host.name.com:445: no such host.

I have not enough knowledge about CIFS and its port usage, to comment on this.

With default ports it works fine.

[michaelkebe]

@vvoland

With stripping the port before the DNS query, it works with an added port.

... --opt device="//some.host.name.com:445/some/path" ...

[vvoland]

Thanks! Looks good to me, just needs a rebase on top of the master and dropping the hack/dind commits.

[michaelkebe]

Done

[thaJeztah]

Was looking at this, and we may need to have a check if deviceUrl.Port() is not empty; I did a quick check, and it looks like net.JoinHostPort unconditionally adds a colon (:), also if there's no port; https://go.dev/play/p/MrZXfaigVRF

package main

import (
	"fmt"
	"net"
)

func main() {
	fmt.Println(net.JoinHostPort("1.2.3.4", ""))
	fmt.Println(net.JoinHostPort("::1", ""))
}

Code above prints;

1.2.3.4:
[::1]:

[vvoland]

Thanks! Looking good, just needs one last touch.

Can you also apply Sebastiaan's suggestion (check if Port is empty)?
#46863 (comment)

Also, please make sure that you squash your commits (so the PR has only one commit).

[michaelkebe]

Done, have fun 😄

[vvoland]

Thanks, LGTM!

[thaJeztah]

Thanks! changes look good; some things we should look at in a follow-up (perhaps you're interested in working on these?)

• perhaps we should apply the same improvements we made for cifs to the nfs case as well (if it also handles IPv6?), I think nfs expects port to be a separate option so we probably don't need the JoinHostPort part, but maybe have to deal with formatting duet to : being used
• consider extracting parsing of these options, so that we can unit-test them

[thaJeztah]

LGTM

[michaelkebe]

Thanks! changes look good; some things we should look at in a follow-up (perhaps you're interested in working on these?)

• perhaps we should apply the same improvements we made for cifs to the nfs case as well (if it also handles IPv6?), I think nfs expects port to be a separate option so we probably don't need the JoinHostPort part, but maybe have to deal with formatting duet to : being used
• consider extracting parsing of these options, so that we can unit-test them

One thing I noticed. Adding a volume with

docker volume create --driver local --opt type=cifs --opt o=username=USERNAME,password=PASSWORD,vers=3,sec=ntlmsspi --opt device="//some.server.com:1234567890/SOME/PATH" VOLUME_NAME

works. It looks like the port is ignored?

[vvoland]

Yeah, looks like the port is expected to be passed as a separate port option, not the address.

docker volume create --driver local --opt type=cifs \
--opt o=username=USERNAME,password=PASSWORD,vers=3,sec=ntlmsspi,port=1234 \
--opt device="//some.server.com/SOME/PATH" VOLUME_NAME

Considering that the mount.cifs(8) also

$ mount -t cifs //example.xyz:1231/adsd /mnt
mount error: could not resolve address for example.xyz:1231: Unknown error

doesn't separate the port, perhaps we should do the same and just use the whole Host part. So basically reverting my suggestions from: #46863 (review) (sorry, it didn't occur to me that cifs uses a separate port option 😞)

Alternatively, we could error out with an explanation that user should pass the port option instead. WDYT? @thaJeztah

[thaJeztah]

Yes, perhaps an error would make sense, so that we don't diverge too much from what's expected. If we would do the parsing and conversion (host:port -> port=xxx), that would also introduce new challenges, such as if the user specifies BOTH host:port AND port=xxx in options.

[vvoland]

Yes, I wouldn't be in favor of the host:port -> port=xxx conversion, I think we want to minimize the translation we do between what user input and what we give to the kernel.

But, providing an extra, more user friendly warning is probably fine, since it also avoids user confusion and blaming the engine for not connecting to the right port.

[Mist-Hunter]

Previously, I had been using a CIFS option, addr= to mitigate this hostname resolution issue. Did this command get deprecated or removed? All my CIFS volumes and associated containers fail to load now due to 'invalid command' and not until rebuilding the volume with 'addr=' removed, will it start again. ( example of the command in question: https://serverfault.com/a/848166 )

I'm guessing this is intended, but thought I'd share this here in case anyone else ran into this issue. I initially rolled back to v24 and held the package until I could figure out what was going on.

This relates to

docker --version
Docker version 25.0.0, build e758fe5