PWX-38721: Fix for ip/host mismatch during FBDA mount. (#2467) (#2471)

* PWX-38721: Fix for ip/host mismatch during FBDA mount. (#2467) Signed-off-by: Priyanshu Pandey <[email protected]> * PWX-38721: Adding UTs and fixing multiple IP case. Signed-off-by: Priyanshu Pandey <[email protected]> * PWX-38721: Handling different source paths with same nfs endpoint and adding UTs. Signed-off-by: Priyanshu Pandey <[email protected]> --------- Signed-off-by: Priyanshu Pandey <[email protected]>
libopenstorage · Aug 28, 2024 · 467e378 · 467e378
1 parent d5b06ab
commit 467e378
Show file tree

Hide file tree

Showing 3 changed files with 294 additions and 3 deletions.
diff --git a/pkg/mount/mount.go b/pkg/mount/mount.go
@@ -10,6 +10,7 @@ import (
 	"errors"
 	"fmt"
 	"io/ioutil"
+	"net"
 	"os"
 	"os/exec"
 	"path"
@@ -451,6 +452,43 @@ func (m *Mounter) load(prefixes []*regexp.Regexp, fmp findMountPoint) error {
 	return nil
 }
 
+func resolveToIPs(hostPath string) []string {
+	index := strings.LastIndex(hostPath, ":")
+	if index != -1 {
+		hostPath = hostPath[:index]
+	}
+	ips, err := net.LookupIP(hostPath)
+	if err != nil || len(ips) == 0 {
+		return []string{hostPath} // Return the original input if resolution fails
+	}
+	// Convert all IP addresses to strings
+	ipStrings := make([]string, len(ips))
+	for i, ip := range ips {
+		ipStrings[i] = ip.String()
+	}
+	return ipStrings
+}
+
+func areSameIPs(ips1, ips2 []string) bool {
+	for _, ip1 := range ips1 {
+		for _, ip2 := range ips2 {
+			if ip1 == ip2 {
+				return true
+			}
+		}
+	}
+	return false
+}
+
+func extractSourcePath(hostPath string) string {
+	index := strings.LastIndex(hostPath, ":")
+	if index != -1 && index < len(hostPath)-1 {
+		return hostPath[index+1:]
+	}
+	// Return the original input if resolution fails
+	return hostPath
+}
+
 // Mount new mountpoint for specified device.
 func (m *Mounter) Mount(
 	minor int,
@@ -482,10 +520,34 @@ func (m *Mounter) Mount(
 			return ErrMountpathNotAllowed
 		}
 	}
+	resolveDNSOnMount := false
+	if _, ok := opts[options.OptionsResolveDNSOnMount]; ok {
+		resolveDNSOnMount = true
+	}
+	var err error
 	dev, ok := m.HasTarget(path)
-	if ok && dev != device {
-		logrus.Warnf("cannot mount %q,  device %q is mounted at %q", device, dev, path)
-		return ErrExist
+	if ok {
+		if dev != device {
+			err = ErrExist
+			if resolveDNSOnMount {
+				resolvedDevPath := extractSourcePath(dev)
+				resolvedDevicePath := extractSourcePath(device)
+				if resolvedDevPath == resolvedDevicePath {
+					// Resolve both using DNS lookup
+					resolvedDevIPs := resolveToIPs(dev)
+					resolvedDeviceIPs := resolveToIPs(device)
+					if areSameIPs(resolvedDevIPs, resolvedDeviceIPs) {
+						logrus.Infof("Device %q, is already mount at %q, as source path %q", device, path, dev)
+						return nil
+					}
+				}
+
+			}
+		}
+	}
+	if err != nil {
+		logrus.Warnf("Cannot mount %q, device %q is mounted at %q", device, dev, path)
+		return err
 	}
 	m.Lock()
 	info, ok := m.mounts[device]

diff --git a/pkg/mount/mount_test.go b/pkg/mount/mount_test.go
@@ -56,6 +56,10 @@ func allTests(t *testing.T, source, dest string) {
 	enoentUnmountTest(t, source, dest)
 	doubleUnmountTest(t, source, dest)
 	enoentUnmountTestWithoutOptions(t, source, dest)
+	doubleMountTest(t, source, dest)
+	mountTestHostMismatchFailure(t, source, dest)
+	mountTestHostMismatchSuccessWithOptions(t, source, dest)
+	mountTestPathMismatchFailureWithOptions(t, source, dest)
 	mountTestParallel(t, source, dest)
 	inspect(t, source, dest)
 	reload(t, source, dest)
@@ -112,6 +116,79 @@ func mountTest(t *testing.T, source, dest string) {
 	require.NoError(t, err, "Failed in unmount")
 }
 
+func doubleMountTest(t *testing.T, source, dest string) {
+	err := m.Mount(0, source, dest, "", syscall.MS_BIND, "", 0, nil)
+	require.NoError(t, err, "Failed in mount")
+
+	// Mount point is already created and new request lands on the same mount point
+	err = m.Mount(0, source, dest, "", syscall.MS_BIND, "", 0, nil)
+	require.NoError(t, err, "Unexpected error in mount")
+
+	err = m.Unmount(source, dest, 0, 0, nil)
+	require.NoError(t, err, "Failed in unmount")
+}
+
+func mountTestHostMismatchFailure(t *testing.T, source, dest string) {
+	cleandir("localhost:" + source)
+	cleandir("127.0.0.1:" + source)
+	err := m.Mount(0, "localhost:"+source, dest, "", syscall.MS_BIND, "", 0, nil)
+	require.NoError(t, err, "Failed in mount")
+
+	// Mount point is already created and new request lands on the same mount point
+	// but source paths are different
+	err = m.Mount(0, "127.0.0.1:"+source, dest, "", syscall.MS_BIND, "", 0, nil)
+	// Expected error as source paths are different
+	require.Error(t, err, "Expected error in mount")
+	require.Equal(t, err.Error(), "Mountpath already exists", "Expected \"Mountpath already exists\"")
+
+	err = m.Unmount("localhost:"+source, dest, 0, 0, nil)
+	require.NoError(t, err, "Failed in unmount")
+	shutdown(t, "localhost:"+source, dest)
+	shutdown(t, "127.0.0.1:"+source, dest)
+}
+
+func mountTestHostMismatchSuccessWithOptions(t *testing.T, source, dest string) {
+	opts := make(map[string]string)
+	opts[options.OptionsResolveDNSOnMount] = "true"
+	cleandir("localhost:" + source)
+	cleandir("127.0.0.1:" + source)
+	err := m.Mount(0, "localhost:"+source, dest, "", syscall.MS_BIND, "", 0, opts)
+	require.NoError(t, err, "Failed in mount")
+
+	// Mount point is already created and new request lands on the same mount point
+	// and source paths resolve to same IP with OptionsResolveDNSOnMount
+	err = m.Mount(0, "127.0.0.1:"+source, dest, "", syscall.MS_BIND, "", 0, opts)
+	// Expected success as source paths are different but resolve to same IP
+	require.NoError(t, err, "Failed in mount")
+
+	err = m.Unmount("localhost:"+source, dest, 0, 0, nil)
+	require.NoError(t, err, "Failed in unmount")
+	shutdown(t, "localhost:"+source, dest)
+	shutdown(t, "127.0.0.1:"+source, dest)
+}
+
+func mountTestPathMismatchFailureWithOptions(t *testing.T, source, dest string) {
+	opts := make(map[string]string)
+	opts[options.OptionsResolveDNSOnMount] = "true"
+	cleandir("localhost:" + source + "/path1")
+	cleandir("localhost:" + source + "/path2")
+	err := m.Mount(0, "localhost:"+source+"/path1", dest, "", syscall.MS_BIND, "", 0, nil)
+	require.NoError(t, err, "Failed in mount")
+
+	// Mount point is already created and new request lands on the same mount point
+	// but source paths are different even when NFS server is same.
+	// Unlikely in practice.
+	err = m.Mount(0, "localhost:"+source+"/path2", dest, "", syscall.MS_BIND, "", 0, nil)
+	// Expected error as source paths are different
+	require.Error(t, err, "Expected error in mount")
+	require.Equal(t, err.Error(), "Mountpath already exists", "Expected \"Mountpath already exists\"")
+
+	err = m.Unmount("localhost:"+source+"/path1", dest, 0, 0, nil)
+	require.NoError(t, err, "Failed in unmount")
+	shutdown(t, "localhost:"+source+"/path1", dest)
+	shutdown(t, "localhost:"+source+"/path2", dest)
+}
+
 func enoentUnmountTest(t *testing.T, source, dest string) {
 	opts := make(map[string]string)
 	opts[options.OptionsUnmountOnEnoent] = "true"
@@ -286,6 +363,153 @@ func makeFile(pathname string) error {
 	return nil
 }
 
+func TestResolveToIPs(t *testing.T) {
+	tests := []struct {
+		hostPath string
+		expected []string
+	}{
+		// Case: Valid hostname with path
+		{"localhost:/path", []string{"127.0.0.1"}},
+
+		// Case: Valid hostname without path
+		{"localhost", []string{"127.0.0.1"}},
+
+		// Case: Invalid hostname
+		{"invalidhost", []string{"invalidhost"}},
+
+		// Case: IP address with path
+		{"192.168.1.1:/path", []string{"192.168.1.1"}},
+
+		// Case: IP address without path
+		{"192.168.1.1", []string{"192.168.1.1"}},
+
+		// Case: Empty string
+		{"", []string{""}},
+	}
+	for _, test := range tests {
+		result := resolveToIPs(test.hostPath)
+		if !areSameIPs(result, test.expected) {
+			t.Errorf("resolveToIPs(%v) = %v; expected %v", test.hostPath, result, test.expected)
+		}
+	}
+}
+func TestAreSameIPs(t *testing.T) {
+	tests := []struct {
+		name     string
+		ips1     []string
+		ips2     []string
+		expected bool
+	}{
+		{
+			name:     "One matching IP",
+			ips1:     []string{"192.168.1.1", "192.168.1.2"},
+			ips2:     []string{"10.0.0.1", "192.168.1.2"},
+			expected: true,
+		},
+		{
+			name:     "No matching IPs",
+			ips1:     []string{"192.168.1.1", "192.168.1.2"},
+			ips2:     []string{"10.0.0.1", "10.0.0.2"},
+			expected: false,
+		},
+		{
+			name:     "Empty second slice",
+			ips1:     []string{"192.168.1.1", "192.168.1.2"},
+			ips2:     []string{},
+			expected: false,
+		},
+		{
+			name:     "Empty first slice",
+			ips1:     []string{},
+			ips2:     []string{"192.168.1.1", "192.168.1.2"},
+			expected: false,
+		},
+		{
+			name:     "Both slices empty",
+			ips1:     []string{},
+			ips2:     []string{},
+			expected: false,
+		},
+		{
+			name:     "Identical IPs in both slices",
+			ips1:     []string{"192.168.1.1", "192.168.1.2"},
+			ips2:     []string{"192.168.1.1", "192.168.1.2"},
+			expected: true,
+		},
+		{
+			name:     "Multiple matches",
+			ips1:     []string{"192.168.1.1", "10.0.0.1"},
+			ips2:     []string{"192.168.1.1", "10.0.0.1"},
+			expected: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := areSameIPs(tt.ips1, tt.ips2)
+			if result != tt.expected {
+				t.Errorf("areSameIPs(%v, %v) = %v; expected %v", tt.ips1, tt.ips2, result, tt.expected)
+			}
+		})
+	}
+}
+
+func TestExtractSourcePath(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		expected string
+	}{
+		{
+			name:     "Single colon with valid suffix",
+			input:    "a:/b",
+			expected: "/b",
+		},
+		{
+			name:     "Colon at the beginning",
+			input:    ":/path",
+			expected: "/path",
+		},
+		{
+			name:     "Multiple colons in string",
+			input:    "path:/to:/resource",
+			expected: "/resource",
+		},
+		{
+			name:     "Colon at the end",
+			input:    "path:/",
+			expected: "/",
+		},
+		{
+			name:     "No colon in string",
+			input:    "noColonHere",
+			expected: "noColonHere",
+		},
+		{
+			name:     "Empty string",
+			input:    "",
+			expected: "",
+		},
+		{
+			name:     "Colon only",
+			input:    ":",
+			expected: ":",
+		},
+		{
+			name:     "Colon followed by space",
+			input:    "path: /to/resource",
+			expected: " /to/resource",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := extractSourcePath(tt.input)
+			if result != tt.expected {
+				t.Errorf("extractSourcePath(%q) = %q; expected %q", tt.input, result, tt.expected)
+			}
+		})
+	}
 func TestSafeEmptyTrashDir(t *testing.T) {
 	sched.Init(time.Second)
 	m, err := New(NFSMount, nil, []*regexp.Regexp{regexp.MustCompile("")}, nil, []string{}, "")

diff --git a/pkg/options/options.go b/pkg/options/options.go
@@ -68,6 +68,11 @@ const (
 	// It is used to issue an umount system call to the requested path even
 	// if the entry for the path is not present in the mount table
 	OptionsUnmountOnEnoent = "UNMOUNT_ON_ENOENT"
+	// OptionsResolveDNSOnMount is an option to the following Opentstorage Volume API
+	// - Mount
+	// It is used to issue an mount system call to the requested path to resolve
+	// the path if there is a mismatch in the mount table and the source path.
+	OptionsResolveDNSOnMount = "RESOLVE_DNS_ON_MOUNT"
 )
 
 // IsBoolOptionSet checks if a boolean option key is set