cifs volume resolves hostname correctly by michaelkebe · Pull Request #46863 · moby/moby

michaelkebe · 2023-11-29T08:38:41Z

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 · 2023-11-29T11:20:26Z

volume/local/local_unix.go

+		if err != nil {
+			return errors.Wrapf(err, "error parsing mount device url")
+		}
+		if deviceUrl.Host != "" && net.ParseIP(deviceUrl.Host).To4() == nil {


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?

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)

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

volume/local/local_unix.go

michaelkebe · 2023-11-29T12:16:16Z

Adopted your suggestions and it works. @vvoland

vvoland

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

volume/local/local_unix.go

michaelkebe · 2023-11-29T14:41:47Z

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

volume/local/local_unix.go

michaelkebe · 2023-11-29T14:59:48Z

@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 · 2023-11-29T15:39:38Z

Done

volume/local/local_unix.go

thaJeztah · 2023-11-29T16:17:57Z

volume/local/local_unix.go

+			if err != nil {
+				return errors.Wrapf(err, "error resolving passed in network volume address")
+			}
+			deviceUrl.Host = net.JoinHostPort(ipAddr.String(), deviceUrl.Port())


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

Co-authored-by: Paweł Gronowski <me@woland.xyz> Signed-off-by: Michael Kebe <michael.kebe@gmail.com>

michaelkebe · 2023-11-30T10:34:08Z

Done, have fun 😄

vvoland

Thanks, LGTM!

thaJeztah · 2023-11-30T11:27:23Z

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 · 2023-11-30T11:37:18Z

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 · 2023-11-30T12:09:38Z

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 · 2023-11-30T12:57:42Z

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 · 2023-11-30T13:00:52Z

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 · 2024-01-22T17:26:46Z

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

michaelkebe force-pushed the fix/cifs-volume-resolve-hostname branch from 98a5845 to d1585d7 Compare November 29, 2023 08:45

vvoland reviewed Nov 29, 2023

View reviewed changes

vvoland added status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. area/volumes Volumes kind/bugfix PR's that fix bugs labels Nov 29, 2023

vvoland added this to the 25.0.0 milestone Nov 29, 2023

michaelkebe force-pushed the fix/cifs-volume-resolve-hostname branch from c6e6ec5 to b1592f8 Compare November 29, 2023 12:14

vvoland requested changes Nov 29, 2023

View reviewed changes

volume/local/local_unix.go Outdated Show resolved Hide resolved

volume/local/local_unix.go Outdated Show resolved Hide resolved

vvoland changed the title ~~cifs volume resolves hostname correctly. Fixes #46180~~ cifs volume resolves hostname correctly Nov 29, 2023

michaelkebe force-pushed the fix/cifs-volume-resolve-hostname branch from 02ea516 to 5d0afdf Compare November 29, 2023 14:34

vvoland requested changes Nov 29, 2023

View reviewed changes

volume/local/local_unix.go Outdated Show resolved Hide resolved

michaelkebe force-pushed the fix/cifs-volume-resolve-hostname branch from 706847f to bbde9c4 Compare November 29, 2023 14:55

michaelkebe requested a review from tianon as a code owner November 29, 2023 14:55

vvoland reviewed Nov 29, 2023

View reviewed changes

michaelkebe force-pushed the fix/cifs-volume-resolve-hostname branch 2 times, most recently from db68959 to e4c29df Compare November 29, 2023 15:38

thaJeztah reviewed Nov 29, 2023

View reviewed changes

michaelkebe force-pushed the fix/cifs-volume-resolve-hostname branch from 0ae3168 to 4868be7 Compare November 30, 2023 09:03

michaelkebe requested a review from vvoland November 30, 2023 09:10

michaelkebe force-pushed the fix/cifs-volume-resolve-hostname branch from 4868be7 to 77478c5 Compare November 30, 2023 09:12

vvoland requested changes Nov 30, 2023

View reviewed changes

cifs volume resolves hostname correctly.

8ae94ca

Co-authored-by: Paweł Gronowski <me@woland.xyz> Signed-off-by: Michael Kebe <michael.kebe@gmail.com>

michaelkebe force-pushed the fix/cifs-volume-resolve-hostname branch from 77478c5 to 8ae94ca Compare November 30, 2023 10:33

vvoland approved these changes Nov 30, 2023

View reviewed changes

thaJeztah approved these changes Nov 30, 2023

View reviewed changes

thaJeztah merged commit faecc65 into moby:master Nov 30, 2023

thaJeztah added impact/changelog area/networking/ipv6 Networking labels Nov 30, 2023

michaelkebe deleted the fix/cifs-volume-resolve-hostname branch November 30, 2023 13:03

robmry mentioned this pull request Dec 1, 2023

Don't allow port in CIFS URL #46877

Merged

vvoland mentioned this pull request Jan 19, 2024

engine: add release notes for v25.0.0 docker/docs#18253

Merged

Herr-Sepp mentioned this pull request Jan 23, 2024

container with cifs volume and "addr"-option does not start with version 25 #47184

Closed

thaJeztah mentioned this pull request Jan 23, 2024

volume/local: Make host resolution backwards compatible #47185

Merged

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

Conversation

michaelkebe commented Nov 29, 2023 • edited by vvoland Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vvoland left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvoland Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

vvoland Nov 30, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelkebe commented Nov 29, 2023

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

michaelkebe commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

michaelkebe commented Nov 29, 2023

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

michaelkebe commented Nov 29, 2023

Uh oh!

Uh oh!

thaJeztah Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

michaelkebe commented Nov 30, 2023

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Nov 30, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

michaelkebe commented Nov 30, 2023

Uh oh!

vvoland commented Nov 30, 2023

Uh oh!

thaJeztah commented Nov 30, 2023

Uh oh!

vvoland commented Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mist-Hunter commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

michaelkebe commented Nov 29, 2023 •

edited by vvoland

Loading

vvoland left a comment •

edited

Loading

michaelkebe commented Nov 29, 2023 •

edited

Loading

vvoland commented Nov 30, 2023 •

edited

Loading

Mist-Hunter commented Jan 22, 2024 •

edited

Loading