From 5b521466145d129082d509d6fd87a641f000e8e6 Mon Sep 17 00:00:00 2001 From: elbehery Date: Mon, 12 Apr 2021 14:55:15 +0200 Subject: [PATCH 1/5] fix-nfs-storage-ipv6_add_square_brackets --- pkg/volume/nfs/nfs.go | 6 +++++- pkg/volume/nfs/nfs_test.go | 8 ++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/pkg/volume/nfs/nfs.go b/pkg/volume/nfs/nfs.go index db906a7b799..cb64855a68b 100644 --- a/pkg/volume/nfs/nfs.go +++ b/pkg/volume/nfs/nfs.go @@ -18,6 +18,7 @@ package nfs import ( "fmt" + "net" "os" "runtime" "time" @@ -121,7 +122,10 @@ func (plugin *nfsPlugin) newMounterInternal(spec *volume.Spec, pod *v1.Pod, moun if err != nil { return nil, err } - + // wrap ipv6 into `[ ]` + if net.ParseIP(source.Server).To16() != nil { + source.Server = fmt.Sprintf("[%s]", source.Server) + } return &nfsMounter{ nfs: &nfs{ volName: spec.Name(), diff --git a/pkg/volume/nfs/nfs_test.go b/pkg/volume/nfs/nfs_test.go index a53855ca9e6..a6fcd166277 100644 --- a/pkg/volume/nfs/nfs_test.go +++ b/pkg/volume/nfs/nfs_test.go @@ -181,6 +181,14 @@ func TestPluginVolume(t *testing.T) { doTestPlugin(t, volume.NewSpecFromVolume(vol)) } +func TestIPV6VolumeSource(t *testing.T) { + vol := &v1.Volume{ + Name: "vol1", + VolumeSource: v1.VolumeSource{NFS: &v1.NFSVolumeSource{Server: "0:0:0:0:0:0:0:1", Path: "/somepath", ReadOnly: false}}, + } + doTestPlugin(t, volume.NewSpecFromVolume(vol)) +} + func TestPluginPersistentVolume(t *testing.T) { vol := &v1.PersistentVolume{ ObjectMeta: metav1.ObjectMeta{ From 124b71bd11f3362d9bf51bf0c981dc682527747f Mon Sep 17 00:00:00 2001 From: elbehery Date: Tue, 13 Apr 2021 15:41:43 +0200 Subject: [PATCH 2/5] add test for ipv4 literal --- pkg/volume/nfs/nfs_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/volume/nfs/nfs_test.go b/pkg/volume/nfs/nfs_test.go index a6fcd166277..4e093b13304 100644 --- a/pkg/volume/nfs/nfs_test.go +++ b/pkg/volume/nfs/nfs_test.go @@ -189,6 +189,14 @@ func TestIPV6VolumeSource(t *testing.T) { doTestPlugin(t, volume.NewSpecFromVolume(vol)) } +func TestIPV4VolumeSource(t *testing.T) { + vol := &v1.Volume{ + Name: "vol1", + VolumeSource: v1.VolumeSource{NFS: &v1.NFSVolumeSource{Server: "127.0.0.1", Path: "/somepath", ReadOnly: false}}, + } + doTestPlugin(t, volume.NewSpecFromVolume(vol)) +} + func TestPluginPersistentVolume(t *testing.T) { vol := &v1.PersistentVolume{ ObjectMeta: metav1.ObjectMeta{ From e05a8403c8c9b7ea60b4f3b6d61f286cb7e14c1c Mon Sep 17 00:00:00 2001 From: elbehery Date: Tue, 13 Apr 2021 18:49:34 +0200 Subject: [PATCH 3/5] fix ipv6 test case --- pkg/volume/nfs/nfs_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/pkg/volume/nfs/nfs_test.go b/pkg/volume/nfs/nfs_test.go index 4e093b13304..9dc9fdd3ff6 100644 --- a/pkg/volume/nfs/nfs_test.go +++ b/pkg/volume/nfs/nfs_test.go @@ -18,6 +18,7 @@ package nfs import ( "fmt" + "net" "os" "testing" @@ -118,6 +119,18 @@ func doTestPlugin(t *testing.T, spec *volume.Spec) { if mounter == nil { t.Errorf("Got a nil Mounter") } + if nfsm, ok := mounter.(*nfsMounter); ok { + srvr := nfsm.server + vol, _, _ := getVolumeSource(spec) + expectedSrvr := vol.Server + // check cluster IP version + if net.ParseIP(expectedSrvr).To16() != nil { + expectedSrvr = fmt.Sprintf("[%s]", expectedSrvr) + } + if srvr != expectedSrvr { + t.Errorf("Unexpected nfs server, expected %q, got: %q", expectedSrvr, srvr) + } + } volumePath := mounter.GetPath() expectedPath := fmt.Sprintf("%s/pods/poduid/volumes/kubernetes.io~nfs/vol1", tmpDir) if volumePath != expectedPath { From f9befb90a4c230c32dd233607b86af47b5263648 Mon Sep 17 00:00:00 2001 From: elbehery Date: Tue, 13 Apr 2021 23:12:30 +0200 Subject: [PATCH 4/5] add pointer mutation review --- pkg/volume/nfs/nfs.go | 15 +++++++++------ pkg/volume/nfs/nfs_test.go | 30 +++++++++++++++++------------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/pkg/volume/nfs/nfs.go b/pkg/volume/nfs/nfs.go index cb64855a68b..29aa2e7dc3c 100644 --- a/pkg/volume/nfs/nfs.go +++ b/pkg/volume/nfs/nfs.go @@ -18,7 +18,7 @@ package nfs import ( "fmt" - "net" + netutil "k8s.io/utils/net" "os" "runtime" "time" @@ -122,10 +122,6 @@ func (plugin *nfsPlugin) newMounterInternal(spec *volume.Spec, pod *v1.Pod, moun if err != nil { return nil, err } - // wrap ipv6 into `[ ]` - if net.ParseIP(source.Server).To16() != nil { - source.Server = fmt.Sprintf("[%s]", source.Server) - } return &nfsMounter{ nfs: &nfs{ volName: spec.Name(), @@ -134,7 +130,7 @@ func (plugin *nfsPlugin) newMounterInternal(spec *volume.Spec, pod *v1.Pod, moun plugin: plugin, MetricsProvider: volume.NewMetricsStatFS(getPath(pod.UID, spec.Name(), plugin.host)), }, - server: source.Server, + server: getServerFromSource(source), exportPath: source.Path, readOnly: readOnly, mountOptions: util.MountOptionFromSpec(spec), @@ -326,3 +322,10 @@ func getVolumeSource(spec *volume.Spec) (*v1.NFSVolumeSource, bool, error) { return nil, false, fmt.Errorf("Spec does not reference a NFS volume type") } + +func getServerFromSource(source *v1.NFSVolumeSource) string { + if netutil.IsIPv6String(source.Server) { + return fmt.Sprintf("[%s]", source.Server) + } + return source.Server +} diff --git a/pkg/volume/nfs/nfs_test.go b/pkg/volume/nfs/nfs_test.go index 9dc9fdd3ff6..c01e2787276 100644 --- a/pkg/volume/nfs/nfs_test.go +++ b/pkg/volume/nfs/nfs_test.go @@ -18,7 +18,6 @@ package nfs import ( "fmt" - "net" "os" "testing" @@ -119,18 +118,6 @@ func doTestPlugin(t *testing.T, spec *volume.Spec) { if mounter == nil { t.Errorf("Got a nil Mounter") } - if nfsm, ok := mounter.(*nfsMounter); ok { - srvr := nfsm.server - vol, _, _ := getVolumeSource(spec) - expectedSrvr := vol.Server - // check cluster IP version - if net.ParseIP(expectedSrvr).To16() != nil { - expectedSrvr = fmt.Sprintf("[%s]", expectedSrvr) - } - if srvr != expectedSrvr { - t.Errorf("Unexpected nfs server, expected %q, got: %q", expectedSrvr, srvr) - } - } volumePath := mounter.GetPath() expectedPath := fmt.Sprintf("%s/pods/poduid/volumes/kubernetes.io~nfs/vol1", tmpDir) if volumePath != expectedPath { @@ -149,6 +136,23 @@ func doTestPlugin(t *testing.T, spec *volume.Spec) { if mounter.(*nfsMounter).readOnly { t.Errorf("The volume source should not be read-only and it is.") } + mntDevs, err := fake.List() + if err != nil { + t.Errorf("fakeMounter.List() failed: %v", err) + } + if len(mntDevs) != 1 { + t.Errorf("unexpected number of mounted devices. expected: %v, got %v", 1, len(mntDevs)) + } else { + if mntDevs[0].Type != "nfs" { + t.Errorf("unexpected type of mounted devices. expected: %v, got %v", "nfs", mntDevs[0].Type) + } + src, _, _ := getVolumeSource(spec) + srvr := getServerFromSource(src) + expectedDevice := fmt.Sprintf("%s:%s", srvr, src.Path) + if mntDevs[0].Device != expectedDevice { + t.Errorf("unexpected nfs device, expected %q, got: %q", expectedDevice, mntDevs[0].Device) + } + } log := fake.GetLog() if len(log) != 1 { t.Errorf("Mount was not called exactly one time. It was called %d times.", len(log)) From 859fe6899ca2dfcf0db98de23d6eb52649950d7e Mon Sep 17 00:00:00 2001 From: elbehery Date: Fri, 23 Apr 2021 12:36:01 +0200 Subject: [PATCH 5/5] add expectedDevice as arg for testing --- pkg/volume/nfs/nfs_test.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/pkg/volume/nfs/nfs_test.go b/pkg/volume/nfs/nfs_test.go index c01e2787276..d9d8cccb022 100644 --- a/pkg/volume/nfs/nfs_test.go +++ b/pkg/volume/nfs/nfs_test.go @@ -96,7 +96,7 @@ func TestRecycler(t *testing.T) { } } -func doTestPlugin(t *testing.T, spec *volume.Spec) { +func doTestPlugin(t *testing.T, spec *volume.Spec, expectedDevice string) { tmpDir, err := utiltesting.MkTmpdir("nfs_test") if err != nil { t.Fatalf("error creating temp dir: %v", err) @@ -146,9 +146,6 @@ func doTestPlugin(t *testing.T, spec *volume.Spec) { if mntDevs[0].Type != "nfs" { t.Errorf("unexpected type of mounted devices. expected: %v, got %v", "nfs", mntDevs[0].Type) } - src, _, _ := getVolumeSource(spec) - srvr := getServerFromSource(src) - expectedDevice := fmt.Sprintf("%s:%s", srvr, src.Path) if mntDevs[0].Device != expectedDevice { t.Errorf("unexpected nfs device, expected %q, got: %q", expectedDevice, mntDevs[0].Device) } @@ -195,7 +192,7 @@ func TestPluginVolume(t *testing.T) { Name: "vol1", VolumeSource: v1.VolumeSource{NFS: &v1.NFSVolumeSource{Server: "localhost", Path: "/somepath", ReadOnly: false}}, } - doTestPlugin(t, volume.NewSpecFromVolume(vol)) + doTestPlugin(t, volume.NewSpecFromVolume(vol), "localhost:/somepath") } func TestIPV6VolumeSource(t *testing.T) { @@ -203,7 +200,7 @@ func TestIPV6VolumeSource(t *testing.T) { Name: "vol1", VolumeSource: v1.VolumeSource{NFS: &v1.NFSVolumeSource{Server: "0:0:0:0:0:0:0:1", Path: "/somepath", ReadOnly: false}}, } - doTestPlugin(t, volume.NewSpecFromVolume(vol)) + doTestPlugin(t, volume.NewSpecFromVolume(vol), "[0:0:0:0:0:0:0:1]:/somepath") } func TestIPV4VolumeSource(t *testing.T) { @@ -211,7 +208,7 @@ func TestIPV4VolumeSource(t *testing.T) { Name: "vol1", VolumeSource: v1.VolumeSource{NFS: &v1.NFSVolumeSource{Server: "127.0.0.1", Path: "/somepath", ReadOnly: false}}, } - doTestPlugin(t, volume.NewSpecFromVolume(vol)) + doTestPlugin(t, volume.NewSpecFromVolume(vol), "127.0.0.1:/somepath") } func TestPluginPersistentVolume(t *testing.T) { @@ -226,7 +223,7 @@ func TestPluginPersistentVolume(t *testing.T) { }, } - doTestPlugin(t, volume.NewSpecFromPersistentVolume(vol, false)) + doTestPlugin(t, volume.NewSpecFromPersistentVolume(vol, false), "localhost:/somepath") } func TestPersistentClaimReadOnlyFlag(t *testing.T) {