#26639: Local NFS volumes do not resolve hostnames by dattatrayakumbhar · Pull Request #27329 · moby/moby

dattatrayakumbhar · 2016-10-12T20:14:08Z

fixes #26639

- What I did
Implemented code for mounting local NFS with hostname

How I did it
Added method GetAddress, to get address/hostname from options. Added another Method IsIP, to verify if option has IP or Address. If option has hostname then used LookupHost built in function to convert to IPAddress.

- How to verify it

Build a developer container with latest docker/master and this PR
Start docker daemon with copying binaries with hack/make.sh binary.
Create docker volume with "docker volume create -o type=nfs -o o=addr=nfs-server,rw -o device=:/nfsdir --name test4"
Note. Please make sure “nfs-server” is resolvable with Ip address.
Create container with “docker run -it --rm -v test4:/storage datkumbh/nfs sh”
Verify that container created successfully and nfsdir mounted in /storage.

- Description for the changelog

Added a GetAddress method in volume/local/local_unix.go
Added a IsIP method in volume/local/local_unix.go to verify if hostname is IP or return blank
Added a support to convert hostname to IpAddress if option is wit hostname

*- A picture of a cute animal *

cpuguy83

The host lookup should be done before every mount() syscall rather than caching addresses.

cpuguy83 · 2016-10-13T12:19:33Z

volume/local/local_unix.go

No need for strings.Contains, a lookup to if addr := GetAddress(v.opts.MountOpts); addr != "" { ... } should suffice.

cpuguy83 · 2016-10-13T12:20:14Z

volume/local/local_unix.go

No need for IsIP. net.ResolveIPAddr will work just fine here if it is a hostname or an IP.

cpuguy83 · 2016-10-13T12:20:29Z

volume/local/local_unix.go

Please don't export this.

cpuguy83 · 2016-10-13T12:20:41Z

volume/local/local_unix.go

function is not needed.

cpuguy83 · 2016-10-13T12:25:46Z

volume/local/local_unix.go

cpuguy83 · 2016-10-13T12:26:17Z

08:35:30 These files are not properly gofmt'd:
08:35:30 - volume/local/local_unix.go

justincormack · 2016-10-13T10:37:11Z

volume/local/local_unix.go

This would also match maddr= or something on some other mount type. Also we should probably just do this for nfs, as addr might mean something else on other mount types (there might be other network filesystems with a similar issue but these could be added).

justincormack · 2016-10-13T10:37:38Z

volume/local/local_unix.go

This is not a helpful log message

justincormack · 2016-10-13T10:37:48Z

volume/local/local_unix.go

Not helpful

justincormack · 2016-10-13T10:38:49Z

volume/local/local_unix.go

justincormack · 2016-10-13T10:40:19Z

volume/local/local_test.go

The opslist should already split on , so I don't see how this case could happen?

thaJeztah · 2016-10-15T20:02:07Z

volume/local/local_unix.go

Looks like this can be removed instead of commented

thaJeztah · 2016-10-15T20:08:21Z

volume/local/local_unix.go

You can do

if err != nil { return errors.Wrapf(err, "unable to resolve hostname" }

So that you don't need an "else".

Perhaps include the actual hostname in the error message, as that may be valuable information

thaJeztah · 2016-10-15T20:24:36Z

volume/local/local_unix.go

Do we need a regex here for MountType? Or simply v.opts.MountType == "nfs"

Also, given that we'll still check if an addr is actually set below, checking addr= may not even be nescessary here (but open to suggestions)

thaJeztah · 2016-10-15T20:33:44Z

volume/local/local_unix.go

Should we replace the value of v.opts.MountOps here, or should we use a copy, and leave the original options as-is? @cpuguy83 ?

Copy please

dattatrayakumbhar · 2016-10-18T16:00:09Z

Updated PR with changed.Can someone please review it ?

dattatrayakumbhar · 2016-10-20T05:09:39Z

@thaJeztah @cpuguy83
Updated PR. Please take a look.

thaJeztah · 2016-10-20T05:32:44Z

volume/local/local_unix.go

nit: to reduce code duplication, you could put opts1 := v.opts.MountOpts outside the if ... (at line 72), and always use the copy (opts1) at line 85

Thanks @thaJeztah.
Removed duplication. Updated PR with same detail

thaJeztah

LGTM, but left one suggestion

thaJeztah · 2016-10-20T15:27:36Z

ping @justincormack PTAL

dattatrayakumbhar · 2016-10-24T08:15:50Z

@justincormack , Can you please take a look ?

cpuguy83 · 2016-10-24T12:54:40Z

volume/local/local_unix.go

Should be compiled at the package level rather than on every mount call.

But do we really need to match the regex here vs just iterating the opts with getAddress?
In most cases the opts list should be small enough to not matter.
WDYT?

Should also always check errors.

Yes, I suggested doing that in #27329 (comment), perhaps we should skip the regex (since getAddress is checking for it anyway to verify it's not empty)

Yes I think It can be in getAddress function ,Will remove it.

cpuguy83 · 2016-10-24T12:58:44Z

volume/local/local_unix.go

This wrap message is not really adding any context and just duplicating what is already there since the error from net will be lookup <addrValue>: no such host.

Maybe something like error resolving passed in nfs address.
Also, errors should always be started as lower case.

changed error message

thaJeztah · 2016-10-25T21:25:45Z

ping @cpuguy83 PTAL

@dattatrayakumbhar04 can you squash your commits?

cpuguy83 · 2016-10-27T00:14:15Z

volume/local/local_test.go

getAddress is only defined for linux, but not for Windows.
Can probably move to a platform independent file.

dattatrayakumbhar · 2016-10-28T05:48:09Z

@cpuguy83 @justincormack @thaJeztah
Updated PR and all checks are passed now.
Please let me know if any changes need to be done

cpuguy83

LGTM

thaJeztah · 2016-10-31T19:03:35Z

volume/local/local_unix.go

should we replace, including the addr=? Is there a chance another option has the same string?

e.g.

mountOpts = strings.Replace(mountOpts, "addr=" + addrValue, "addr=" + ipAddr.String(), 1)

(maybe not needed, just wondering)

dattatrayakumbhar · 2016-11-07T06:05:13Z

@cpuguy83 @thaJeztah @justincormack
Updated PR with changes.

thaJeztah

LGTM

justincormack · 2016-11-07T17:33:43Z

volume/local/local.go

This needs to check for addr= at the start of the item.

justincormack · 2016-11-07T17:34:34Z

volume/local/local_unix.go

please avoid unrelated whitespace changes

justincormack · 2016-11-07T17:34:39Z

volume/local/local_unix.go

@justincormack
I have made changes PTAL

Signed-off-by: dattatrayakumbhar04 <dattatraya.kumbhar@gslab.com>

justincormack · 2016-11-09T13:46:52Z

LGTM from under the sea!

GordonTheTurtle added the status/0-triage label Oct 12, 2016

dattatrayakumbhar force-pushed the 26639_nfs_volume_with_hostname branch 2 times, most recently from da8360c to 275f850 Compare October 13, 2016 08:34

cpuguy83 requested changes Oct 13, 2016

View reviewed changes

justincormack suggested changes Oct 13, 2016

View reviewed changes

thaJeztah reviewed Oct 15, 2016

View reviewed changes

volume/local/local_unix.go Outdated

Copy link

Member

thaJeztah Oct 15, 2016

Choose a reason for hiding this comment

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

Looks like this can be removed instead of commented

thaJeztah requested changes Oct 15, 2016

View reviewed changes

thaJeztah added status/2-code-review and removed status/0-triage labels Oct 15, 2016

thaJeztah reviewed Oct 15, 2016

View reviewed changes

thaJeztah reviewed Oct 20, 2016

View reviewed changes

thaJeztah approved these changes Oct 20, 2016

View reviewed changes

dattatrayakumbhar force-pushed the 26639_nfs_volume_with_hostname branch 2 times, most recently from f14def9 to 522be5e Compare October 20, 2016 15:20

cpuguy83 requested changes Oct 24, 2016

View reviewed changes

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 25, 2016

dattatrayakumbhar force-pushed the 26639_nfs_volume_with_hostname branch from 7fc7a31 to 18a2457 Compare October 25, 2016 13:55

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 25, 2016

thaJeztah added the impact/changelog label Oct 25, 2016

dattatrayakumbhar force-pushed the 26639_nfs_volume_with_hostname branch 2 times, most recently from 3457450 to 20cd153 Compare October 26, 2016 06:22

cpuguy83 reviewed Oct 27, 2016

View reviewed changes

dattatrayakumbhar force-pushed the 26639_nfs_volume_with_hostname branch from 20cd153 to 7d3acb5 Compare October 27, 2016 07:59

cpuguy83 approved these changes Oct 31, 2016

View reviewed changes

thaJeztah reviewed Oct 31, 2016

View reviewed changes

dattatrayakumbhar force-pushed the 26639_nfs_volume_with_hostname branch 2 times, most recently from 638c95e to ce87725 Compare November 1, 2016 03:11

thaJeztah approved these changes Nov 7, 2016

View reviewed changes

justincormack suggested changes Nov 7, 2016

View reviewed changes

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Nov 8, 2016

dattatrayakumbhar force-pushed the 26639_nfs_volume_with_hostname branch from 95656ec to 1273d65 Compare November 8, 2016 05:28

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Nov 8, 2016

moby#26639: Local NFS volumes do not resolve hostnames

668fa8a

Signed-off-by: dattatrayakumbhar04 <dattatraya.kumbhar@gslab.com>

dattatrayakumbhar force-pushed the 26639_nfs_volume_with_hostname branch from 1273d65 to 668fa8a Compare November 8, 2016 08:43

justincormack approved these changes Nov 9, 2016

View reviewed changes

justincormack merged commit 5020905 into moby:master Nov 9, 2016

GordonTheTurtle added this to the 1.13.0 milestone Nov 9, 2016

zh99998 mentioned this pull request Feb 26, 2017

Local NFS4 volumes do not resolve hostnames #31365

Closed

thaJeztah mentioned this pull request May 22, 2019

Enable DNS Lookups for CIFS Volumes #39250

Merged

mshogren mentioned this pull request Dec 23, 2020

Name resolution of NFSv4 servers doesnt seem to work portainer/portainer#3920

Closed

thaJeztah mentioned this pull request Nov 29, 2023

cifs volume resolves hostname correctly #46863

Merged

Conversation

dattatrayakumbhar commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Oct 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dattatrayakumbhar commented Oct 18, 2016

Uh oh!

dattatrayakumbhar commented Oct 20, 2016

Uh oh!

thaJeztah Oct 20, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 20, 2016

Uh oh!

dattatrayakumbhar commented Oct 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dattatrayakumbhar commented Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

dattatrayakumbhar commented Oct 12, 2016 •

edited

Loading

thaJeztah Oct 20, 2016 •

edited

Loading

dattatrayakumbhar commented Oct 28, 2016 •

edited

Loading

thaJeztah Oct 31, 2016 •

edited

Loading