From d020b78056eb0a7a7de47850fd8f8f97c4733bfe Mon Sep 17 00:00:00 2001 From: Manohar Reddy Date: Fri, 7 May 2021 16:57:12 +0000 Subject: [PATCH] create subDir in CreateVolume and delete in DeleteVolume --- charts/latest/csi-driver-smb-v0.7.0.tgz | Bin 3251 -> 3265 bytes .../templates/csi-smb-controller.yaml | 3 + .../templates/rbac-csi-smb-controller.yaml | 3 + deploy/csi-smb-controller.yaml | 3 + deploy/example/storageclass-smb.yaml | 4 +- deploy/rbac-csi-smb-controller.yaml | 3 + go.mod | 2 +- pkg/smb/controllerserver.go | 234 +++++++++++++++++- pkg/smb/controllerserver_test.go | 228 ++++++++++++++--- pkg/smb/nodeserver.go | 22 -- pkg/smb/smb.go | 4 +- pkg/smb/smb_common_windows.go | 3 +- test/e2e/suite_test.go | 6 +- test/integration/run-test.sh | 2 +- vendor/modules.txt | 2 +- 15 files changed, 444 insertions(+), 75 deletions(-) diff --git a/charts/latest/csi-driver-smb-v0.7.0.tgz b/charts/latest/csi-driver-smb-v0.7.0.tgz index d67184f7e3b0cf09ff5896247d3ef2eab9d3d595..914fe5c9d594be8d96fdd4b874ca186f4fdf8ff6 100644 GIT binary patch delta 3182 zcmV-!43YD*8NnHlJbz(x;n?%y43v!IIFvUf9obTGfk&W6w4){8a@0xNX*uqu zQ5l^;2vS}PK7iVT&bcDZl^gPC^N=C;AqkQjwU>-WQEInDTjQzMkS-O;C zz)X#oOn*m~#b_8m=(`Z&0Rums8$$Q-02n9_H78g!867m!E0*<{hCcr+b6j(27i{00 zdHo3%eMSfU^x`)bLJKb{6>yVJaVd=$$u6qhs>JqIt#7UUt+Buz#942Vt2Vhtv@ad- z4lUOT`@e!9GLgJ_d3AfybJT;{e~eA}|53YB*iHYBkJ~4Q{r@G}?HEi#uM_(;sizW+ zKq(rIkPi{#Q}B`EXv$-O2#a+bcP#6cBQgeZU*wLsFOgJO#qv88F~Ab^4&Yg77$u4! z+tyo?0RuaKqre9LZ*|(o1^@4M+D8Zfe~I?^*!&hvXy73U3Joa(HI3jj&}t!L@X)u- z+03#G*~LSILm#OznA8yQq4`b>W@P9dA8q8rkcNO7I&aiu4P-Cx;Fu}oBq)>dTj#gr z#HlH-CLjd$0sXDhwUD6hk@-r4UOXJq2h^~aok8e-o9~He3=e=wgGY>k1xSG8u^=E( zsay$p(pmCdr$#u6oMqypvvL|rg+l@!YLrEsQl0>Zcm%#Zm=?HtNjhH1n55g9W$uMp zB|4RS3xLIpmgx5V0>??&v{oEBZkol)k*1TemsjSTf1B)HhvUZoHc;cPVftECzRvZK z1R_9xB}<@>2$+T-D%7tTdI|k zZAVk!% z`RA1nn|VxA7+IV1$^;hs&GHdVyEJAcKJi!KUL?5UgfkD_o?Wl+x{kPir6Z>#HAgOe z#FMlkrYsh4I~E|vocYN^t7RK275xONXvtCHRTT;_b1Sd)NwgS zlg6OM?N3Jz3LXmThy zV^?ffN3ZrXM6YDlCD~ek>2x57)byMR4?WgEGB%$7lbd^0glu~L(>`(w`0q*AJ=A}^ zNYm&4>~TM>K}etWHz%zDDBMaRg0tSoWh{pWGMjUbj&gyyOsqX#OLtceZP1p(7F z--yte=@sC)r^cTukNq_>zca62mHS039?`InER`Gp=Ps}ns<4h}7j&U;WZNHKINE?O z?4RqOd3~*W^WQdqu-H1)Rh^SxeagPn-@39nRL25K>iFu~VHs;(Ys{O+_7<8}M=s{9 zqo1wMxu+%Kro?DA>-Q5smHmEYMpmO9%^Z(vp-&W^z3+LiJ%9h@&%E~ zFBkj^Z{V#G)Yu(1$MN}gyfy}}-}5-@ZG)`S{z#&E3t#Pj|hmvtRD6-{1bc+C$zV7##wc5(dT69Ox+? z6&C=19+r_Wvn)Iau--m?Z!`xqY|3%tJJhg={$B*h7DLp`%G<2W81=M_!{qC38-LV+ z0CuY5U9y*Hue`5}_{lc9Op2RnQd~X7-HM5y7VF)i@^crYxA|l^@1ctHwc)(mm7P{W zy}MPY9g&gOUsc^~fqC8bSGpO02p-`PPJUPJ1N998M0vP4pyWj`JSMR0 zUB0^ph<%~*-J0my_Gv^dXsxK)lGWW^#GNFlSlqo8wSZ%H6Kf@>`R9R>mH3|+U}8Vb zOW+&ozmMFLLj8BU)o~B?pD)r1_+S1-V<-7#IAiX`Lh;GGD7=?rsr80Fs*-<~z^1=pT} zh*`2p*g94CN$@z5`s|#Za+lj2`n?fpNu`2NrJGAiGyyR%1S3%Tn9Zm(pXPUw^p`{h z`kSvqB^bUP1|$sAp!TS7TyC=yeDrygR=WBrzFaciAI9ldJ^2F-eHxDTb?CyI@b)0> zw)TUHc<>(}D)s2nP`OMrs(;I&PLR}LCOvel#;M1& delta 3168 zcmV-m44?DC8M7IXJb!I(+qU-aYkdmBD6szbpJdr_oJ~Mpv`MxVTazHp-F-1%1T=MQ z^M(>Nl5*o^{_Yo~WLuIY`6)@$%`8wm(mCWgIaFX#k>ErUVxY>b6+rZ`}4 zp02sB>vlUG^Xa;7@zZT}+izMO_qf$@yKdWkp`NRQWjsLe>-B!{6+nwWs|Gz||0Um*8AtZx= z{y<1T#3fZ+On*HT5a9%r2rl_hIfMtz`|o9wNZORaz$TI!4uwaHXrcd(V#cmHqh#u# zkHbr@t_4U4^-Ut_wcrz~BLaxk0q?(;&WH$y@}{ICTPiN_2=s_{wB%ckI%zvC$6Yrn zqZ0^0%4@*~u<@XCu1Is`hCEt7WXOF;g5(X1s8j-Pr+=;HpF5_Q_mC5!y-o;Af|yVO z?+M{?sPyne09N23C1S8FF~DT?YBH$4o`iE53>+e;HSDFbRQ3Zf$~suf<=?jK{LH#S)Xa>^UpHJHJ5h5_RX2s zpJ35vbkI*Peq$lD@S;)y*ZCBe(uk2{QSDYGwzq11wf0wIfjfv(Z;`7uStHt)4tR%_ zYlZx;Ac#yPZ(d&AUi2LGp!OeQUHyO5?iBL>@k(*aJ>>tFXt!f91-&lp)1sbAGy6t9(lAOCL$<9` zlLP}hf33h8|8I5L#|8iIcHGXv|6ig#J~qEa6B>9(fI;VMs$j4P7^CvIer3cW}%UauSru_^tC>vg6d0 zR}&C|`hfn@uxm%g!M5f6ezqG=>Mjq`@P`zyc&d@>md%s8p_m zJn1a?u1g~vMb5JD(N#GOrNSWr4>ignPAN}-Lp%cC9!v{dy`&tkWK7C!%`*4G>?Ass zd<%fZjF!yp`vs1ZvguiIJUxgyT1oZnB}eO@dko83THgugCAylyy(Duka~f6BVhcmsjE$>8foKxnR%`JMx4 zE-Pl$mN=!?Tku{^J>BxzfJ+Y<_+xRu5L~kpw?t+#(&zgVoPmb|LzL?hLVv*&e+kM3 zRns$urA+jYQ>h@Z(+z`71XYw^HU|ti1A!Igp za?#e(V&4}a<>}jZ@6XR~E_%KHDtaLH7p-R15r)~K0+U5@1B zzBo(Q8Axm01T5jNY+OKS!dV=^C++Sl(-G*a*I11cISLY{U@Ry1plqLJf7ftP?B;u9 zG@!*%5F%0Rp_c0gh3f$X1`j-P-B0D) zFi*Uh73S@&$^sHfBekt)06bQwM{zIFD`LLO%FNKk{Ij<4Vp7 zRt3pdlHJedHGK-;?9+ z;rz#oG`;`NZuiqOe+cQ*{^q1L0EJsAL~z#om^|evK?5>DIyIRv0TvhAc&rpA;~V%R zrXu@rJ>veyaLBJh6FAaG^JdHt&5GGg(FhJ|XZWT!ZHfvw<&I;LG$x}oZG)fHO+naxgQDdiG^D`%8yK$MN$({AOs5UAZaVbtgFVfA8|1RwUdE-Zbj$4{RPiJFrbuc&h{(WQX-}e4dVLV{m)rph*YWLe`rA~EL>bxW@T{q`rE|ZDKyWWlqpr(<|e~lUuN>*e` z8OoQZe`fHSnxt;%dgn;1*1NRxX}?64b1KYce7Yi1xp(d^%~pe#y71mJQB%IxNp~gA z%-d-H)bp4x-8RC1_k16C4gTwPPKxjUpR|t-=RaSh74Tnka_~vf-&ceDJ}2sH?qm^P z9X>BXedgkR@$RW^ekkW#x3lDA3=ie?KQITY|UQqn_JAw-umtCEEPjpx*6H z5&R9$m#ysDbHXj(iY{&?pYIgfS9D-(2UvCoICg&+w)`yY7HI3+cjq@Bf4jK3ySe!3 zu6K3z%iZ<++n-l^$Xf)XLm*Sa3ZOwGpr?3LTmX1jM!w9l@F2iCef-{N4rthv;+YPF2R)HlGaVJsdZEZ8-0C=Tobo-rbI}9nN+7YR@Oh}< zK^2oTolpj&%+K8{M1GEvy1-nQ{z{X9f8Y@w;pBJaK2YBvK$M4z4N6`F!(#%=-sgjB zfY=v0AG{&DwtX3~5wupE+LG1XUBsOvs94;+Cu#x5?k3hsP4o8yB`fhiF~G!rnwP-W zod0g0bc^qQw>ob7fd9QjE8u_mHR zVTzny+M2W4r03Za2=0b6$mwRNajeE%&_9j$FVDl5D5#Z#2c>IemVM3Eyg zLnRo#9R?%}(_rIKI4%A)ebY4YV#dxL{-DBSBprh z>$@jaXs-?l=fbC9*4oR)Fh2SulcrsE)ek?=(5K;OUw16L@NN&%Za03+5D)$fM5X>% z8t9f~LiJBJY!c#hkVv--t5IpQ5Q7#=Su&Y>OJp}mIozW@LL3jhCN1Vz{Y GU;qFY7Ex9J diff --git a/charts/latest/csi-driver-smb/templates/csi-smb-controller.yaml b/charts/latest/csi-driver-smb/templates/csi-smb-controller.yaml index 10b61b1dd2f..7e0527a3cf6 100755 --- a/charts/latest/csi-driver-smb/templates/csi-smb-controller.yaml +++ b/charts/latest/csi-driver-smb/templates/csi-smb-controller.yaml @@ -15,6 +15,7 @@ spec: {{ include "smb.labels" . | indent 6 }} app: csi-smb-controller spec: + dnsPolicy: ClusterFirstWithHostNet serviceAccountName: csi-smb-controller-sa nodeSelector: kubernetes.io/os: linux @@ -96,6 +97,8 @@ spec: env: - name: CSI_ENDPOINT value: unix:///csi/csi.sock + securityContext: + privileged: true volumeMounts: - mountPath: /csi name: socket-dir diff --git a/charts/latest/csi-driver-smb/templates/rbac-csi-smb-controller.yaml b/charts/latest/csi-driver-smb/templates/rbac-csi-smb-controller.yaml index 287c8781a2e..795e7700a70 100755 --- a/charts/latest/csi-driver-smb/templates/rbac-csi-smb-controller.yaml +++ b/charts/latest/csi-driver-smb/templates/rbac-csi-smb-controller.yaml @@ -37,6 +37,9 @@ rules: - apiGroups: ["coordination.k8s.io"] resources: ["leases"] verbs: ["get", "list", "watch", "create", "update", "patch"] + - apiGroups: [""] + resources: ["secrets"] + verbs: ["get"] --- kind: ClusterRoleBinding diff --git a/deploy/csi-smb-controller.yaml b/deploy/csi-smb-controller.yaml index 1b44fbe504a..a76cf900b53 100644 --- a/deploy/csi-smb-controller.yaml +++ b/deploy/csi-smb-controller.yaml @@ -14,6 +14,7 @@ spec: labels: app: csi-smb-controller spec: + dnsPolicy: ClusterFirstWithHostNet serviceAccountName: csi-smb-controller-sa nodeSelector: kubernetes.io/os: linux @@ -89,6 +90,8 @@ spec: env: - name: CSI_ENDPOINT value: unix:///csi/csi.sock + securityContext: + privileged: true volumeMounts: - mountPath: /csi name: socket-dir diff --git a/deploy/example/storageclass-smb.yaml b/deploy/example/storageclass-smb.yaml index 23b44adc690..7e45dd7ae02 100644 --- a/deploy/example/storageclass-smb.yaml +++ b/deploy/example/storageclass-smb.yaml @@ -12,7 +12,9 @@ parameters: source: "//smb-server.default.svc.cluster.local/share" csi.storage.k8s.io/node-stage-secret-name: "smbcreds" csi.storage.k8s.io/node-stage-secret-namespace: "default" - createSubDir: "false" # optional: create a sub dir for new volume + csi.storage.k8s.io/provisioner-secret-name: "smbcreds" + csi.storage.k8s.io/provisioner-secret-namespace: "default" + createSubDir: "true" # optional: create a sub dir for new volume reclaimPolicy: Retain # only retain is supported volumeBindingMode: Immediate mountOptions: diff --git a/deploy/rbac-csi-smb-controller.yaml b/deploy/rbac-csi-smb-controller.yaml index 26324c351c2..3261259543a 100644 --- a/deploy/rbac-csi-smb-controller.yaml +++ b/deploy/rbac-csi-smb-controller.yaml @@ -33,6 +33,9 @@ rules: - apiGroups: ["coordination.k8s.io"] resources: ["leases"] verbs: ["get", "list", "watch", "create", "update", "patch"] + - apiGroups: [""] + resources: ["secrets"] + verbs: ["get"] --- kind: ClusterRoleBinding diff --git a/go.mod b/go.mod index 215959e146e..4644541e6a4 100644 --- a/go.mod +++ b/go.mod @@ -25,7 +25,7 @@ require ( k8s.io/client-go v0.21.0 k8s.io/klog/v2 v2.8.0 k8s.io/kubernetes v1.21.0 - k8s.io/mount-utils v0.0.0 + k8s.io/mount-utils v0.21.1 k8s.io/utils v0.0.0-20201110183641-67b214c5f920 sigs.k8s.io/yaml v1.2.0 ) diff --git a/pkg/smb/controllerserver.go b/pkg/smb/controllerserver.go index 133f8c6bff7..26d7029c294 100644 --- a/pkg/smb/controllerserver.go +++ b/pkg/smb/controllerserver.go @@ -18,32 +18,132 @@ package smb import ( "context" + "fmt" + "os" + "path/filepath" + "regexp" + "strings" "github.com/container-storage-interface/spec/lib/go/csi" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "k8s.io/klog/v2" +) + +// smbVolume is an internal representation of a volume +// created by the provisioner. +type smbVolume struct { + // Volume id + id string + // Address of the SMB server. + sourceField string + // Subdirectory of the SMB server to create volumes under + subDir string + // size of volume + size int64 +} + +// Ordering of elements in the CSI volume id. +// ID is of the form {server}/{subDir}. +const ( + idsourceField = iota + idSubDir + totalIDElements // Always last ) // CreateVolume only supports static provisioning, no create volume action func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) (*csi.CreateVolumeResponse, error) { + name := req.GetName() + if len(name) == 0 { + return nil, status.Error(codes.InvalidArgument, "CreateVolume name must be provided") + } + + var volCap *csi.VolumeCapability volumeCapabilities := req.GetVolumeCapabilities() if len(volumeCapabilities) == 0 { return nil, status.Error(codes.InvalidArgument, "CreateVolume Volume capabilities must be provided") } - return &csi.CreateVolumeResponse{ - Volume: &csi.Volume{ - VolumeId: req.GetName(), - CapacityBytes: req.GetCapacityRange().GetRequiredBytes(), - VolumeContext: req.GetParameters(), - }, - }, nil + if len(volumeCapabilities) > 0 { + volCap = req.GetVolumeCapabilities()[0] + } + + reqCapacity := req.GetCapacityRange().GetRequiredBytes() + smbVol, err := d.newSMBVolume(name, reqCapacity, req.GetParameters()) + if err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) + } + + // check if create SubDir is enable in storage class parameters + parameters := req.GetParameters() + var createSubDir string + for k, v := range parameters { + switch strings.ToLower(k) { + case createSubDirField: + createSubDir = v + } + } + + secrets := req.GetSecrets() + if strings.EqualFold(createSubDir, "true") { + if len(secrets) > 0 { + // Mount smb base share so we can create a subdirectory + if err := d.internalMount(ctx, smbVol, volCap, secrets); err != nil { + return nil, status.Errorf(codes.Internal, "failed to mount smb server: %v", err.Error()) + } + defer func() { + if err = d.internalUnmount(ctx, smbVol); err != nil { + klog.Warningf("failed to unmount smb server: %v", err.Error()) + } + }() + // Create subdirectory under base-dir + // TODO: revisit permissions + internalVolumePath := d.getInternalVolumePath(smbVol) + if err = os.Mkdir(internalVolumePath, 0777); err != nil && !os.IsExist(err) { + return nil, status.Errorf(codes.Internal, "failed to make subdirectory: %v", err.Error()) + } + parameters[sourceField] = parameters[sourceField] + "/" + smbVol.subDir + } else { + klog.Warningf("CreateVolume: Volume secrets should be provided when createSubDir is true") + } + } + return &csi.CreateVolumeResponse{Volume: d.smbVolToCSI(smbVol, parameters)}, nil } // DeleteVolume only supports static provisioning, no delete volume action func (d *Driver) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest) (*csi.DeleteVolumeResponse, error) { - if len(req.GetVolumeId()) == 0 { - return nil, status.Error(codes.InvalidArgument, "Volume ID missing in request") + volumeID := req.GetVolumeId() + if volumeID == "" { + return nil, status.Error(codes.InvalidArgument, "volume id is empty") + } + smbVol, err := d.getSmbVolFromID(volumeID) + if err != nil { + // An invalid ID should be treated as doesn't exist + klog.Warningf("failed to get smb volume for volume id %v deletion: %v", volumeID, err) + return &csi.DeleteVolumeResponse{}, nil + } + + secrets := req.GetSecrets() + if len(secrets) > 0 { + // Mount smb base share so we can delete the subdirectory + if err = d.internalMount(ctx, smbVol, nil, secrets); err != nil { + return nil, status.Errorf(codes.Internal, "failed to mount smb server: %v", err.Error()) + } + defer func() { + if err = d.internalUnmount(ctx, smbVol); err != nil { + klog.Warningf("failed to unmount smb server: %v", err.Error()) + } + }() + + // Delete subdirectory under base-dir + internalVolumePath := d.getInternalVolumePath(smbVol) + klog.V(2).Infof("Removing subdirectory at %v", internalVolumePath) + if err = os.RemoveAll(internalVolumePath); err != nil { + return nil, status.Errorf(codes.Internal, "failed to delete subdirectory: %v", err.Error()) + } + } else { + klog.Warningf("DeleteVolume: Volume secrets should be provided") } + return &csi.DeleteVolumeResponse{}, nil } @@ -105,3 +205,119 @@ func (d *Driver) DeleteSnapshot(ctx context.Context, req *csi.DeleteSnapshotRequ func (d *Driver) ListSnapshots(ctx context.Context, req *csi.ListSnapshotsRequest) (*csi.ListSnapshotsResponse, error) { return nil, status.Error(codes.Unimplemented, "") } + +// Given a smbVolume, return a CSI volume id +func (d *Driver) getVolumeIDFromSmbVol(vol *smbVolume) string { + idElements := make([]string, totalIDElements) + idElements[idsourceField] = strings.Trim(vol.sourceField, "/") + idElements[idSubDir] = strings.Trim(vol.subDir, "/") + return strings.Join(idElements, "/") +} + +// Get working directory for CreateVolume and DeleteVolume +func (d *Driver) getInternalMountPath(vol *smbVolume) string { + // use default if empty + if d.workingMountDir == "" { + d.workingMountDir = "/tmp" + } + return filepath.Join(d.workingMountDir, vol.subDir) +} + +// Mount smb server at base-dir +func (d *Driver) internalMount(ctx context.Context, vol *smbVolume, volCap *csi.VolumeCapability, secrets map[string]string) error { + stagingPath := d.getInternalMountPath(vol) + + if volCap == nil { + volCap = &csi.VolumeCapability{ + AccessType: &csi.VolumeCapability_Mount{ + Mount: &csi.VolumeCapability_MountVolume{}, + }, + } + } + + klog.V(4).Infof("internally mounting %v at %v", sourceField, stagingPath) + _, err := d.NodeStageVolume(ctx, &csi.NodeStageVolumeRequest{ + StagingTargetPath: stagingPath, + VolumeContext: map[string]string{ + sourceField: vol.sourceField, + }, + VolumeCapability: volCap, + VolumeId: vol.id, + Secrets: secrets, + }) + return err +} + +// Unmount smb server at base-dir +func (d *Driver) internalUnmount(ctx context.Context, vol *smbVolume) error { + targetPath := d.getInternalMountPath(vol) + + // Unmount smb server at base-dir + klog.V(4).Infof("internally unmounting %v", targetPath) + _, err := d.NodeUnstageVolume(ctx, &csi.NodeUnstageVolumeRequest{ + VolumeId: vol.id, + StagingTargetPath: d.getInternalMountPath(vol), + }) + return err +} + +// Convert VolumeCreate parameters to an smbVolume +func (d *Driver) newSMBVolume(name string, size int64, params map[string]string) (*smbVolume, error) { + var sourceField string + + // Validate parameters (case-insensitive). + for k, v := range params { + switch strings.ToLower(k) { + case paramSource: + sourceField = v + } + } + + // Validate required parameters + if sourceField == "" { + return nil, fmt.Errorf("%v is a required parameter", paramSource) + } + + vol := &smbVolume{ + sourceField: sourceField, + subDir: name, + size: size, + } + vol.id = d.getVolumeIDFromSmbVol(vol) + + return vol, nil +} + +// Get internal path where the volume is created +// The reason why the internal path is "workingDir/subDir/subDir" is because: +// * the semantic is actually "workingDir/volId/subDir" and volId == subDir. +// * we need a mount directory per volId because you can have multiple +// CreateVolume calls in parallel and they may use the same underlying share. +// Instead of refcounting how many CreateVolume calls are using the same +// share, it's simpler to just do a mount per request. +func (d *Driver) getInternalVolumePath(vol *smbVolume) string { + return filepath.Join(d.getInternalMountPath(vol), vol.subDir) +} + +// Convert into smbVolume into a csi.Volume +func (d *Driver) smbVolToCSI(vol *smbVolume, parameters map[string]string) *csi.Volume { + return &csi.Volume{ + CapacityBytes: 0, // by setting it to zero, Provisioner will use PVC requested size as PV size + VolumeId: vol.id, + VolumeContext: parameters, + } +} + +// Given a CSI volume id, return a smbVolume +func (d *Driver) getSmbVolFromID(id string) (*smbVolume, error) { + volRegex := regexp.MustCompile("^([^/]+)/([^/]+)$") + tokens := volRegex.FindStringSubmatch(id) + if tokens == nil { + return nil, fmt.Errorf("Could not split %q into server, baseDir and subDir", id) + } + return &smbVolume{ + id: id, + sourceField: tokens[1], + subDir: tokens[2], + }, nil +} diff --git a/pkg/smb/controllerserver_test.go b/pkg/smb/controllerserver_test.go index d29a1f7ea74..f85a5ffd671 100644 --- a/pkg/smb/controllerserver_test.go +++ b/pkg/smb/controllerserver_test.go @@ -18,15 +18,26 @@ package smb import ( "context" + "fmt" + "os" + "path/filepath" "reflect" + "runtime" "testing" "github.com/container-storage-interface/spec/lib/go/csi" + "github.com/kubernetes-csi/csi-driver-smb/test/utils/testutil" "github.com/stretchr/testify/assert" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) +const ( + testServer = "test-server" + testCSIVolume = "test-csi" + testVolumeID = "test-server/test-csi" +) + func TestControllerGetCapabilities(t *testing.T) { d := NewFakeDriver() controlCap := []*csi.ControllerServiceCapability{ @@ -44,69 +55,216 @@ func TestControllerGetCapabilities(t *testing.T) { func TestCreateVolume(t *testing.T) { d := NewFakeDriver() - stdVolCap := []*csi.VolumeCapability{ + + // Setup workingMountDir + workingMountDir, err := os.Getwd() + if err != nil { + t.Errorf("failed to get current working directory") + } + d.workingMountDir = workingMountDir + + // Setup mounter + mounter, err := NewFakeMounter() + if err != nil { + t.Fatalf(fmt.Sprintf("failed to get fake mounter: %v", err)) + } + d.mounter = mounter + + sourceTest := testutil.GetWorkDirPath("test-csi", t) + + cases := []struct { + name string + req *csi.CreateVolumeRequest + resp *csi.CreateVolumeResponse + flakyWindowsErrorMessage string + expectErr bool + }{ { - AccessType: &csi.VolumeCapability_Mount{ - Mount: &csi.VolumeCapability_MountVolume{}, + name: "valid defaults", + req: &csi.CreateVolumeRequest{ + Name: testCSIVolume, + VolumeCapabilities: []*csi.VolumeCapability{ + { + AccessType: &csi.VolumeCapability_Mount{ + Mount: &csi.VolumeCapability_MountVolume{}, + }, + AccessMode: &csi.VolumeCapability_AccessMode{ + Mode: csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER, + }, + }, + }, + Parameters: map[string]string{ + paramSource: testServer, + createSubDirField: "true", + }, + Secrets: map[string]string{ + usernameField: "test", + passwordField: "test", + domainField: "test_doamin", + }, }, - AccessMode: &csi.VolumeCapability_AccessMode{ - Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER, + resp: &csi.CreateVolumeResponse{ + Volume: &csi.Volume{ + VolumeId: testVolumeID, + VolumeContext: map[string]string{ + paramSource: filepath.Join(testServer, testCSIVolume), + createSubDirField: "true", + }, + }, }, + flakyWindowsErrorMessage: fmt.Sprintf("volume(vol_1##) mount \"test-server\" on %#v failed with "+ + "smb mapping failed with error: rpc error: code = Unknown desc = NewSmbGlobalMapping failed.", + sourceTest), }, - } - - tests := []struct { - desc string - req csi.CreateVolumeRequest - expectedErr error - }{ { - desc: "Volume capabilities missing", - req: csi.CreateVolumeRequest{}, - expectedErr: status.Error(codes.InvalidArgument, "CreateVolume Volume capabilities must be provided"), + name: "name empty", + req: &csi.CreateVolumeRequest{ + VolumeCapabilities: []*csi.VolumeCapability{ + { + AccessType: &csi.VolumeCapability_Mount{ + Mount: &csi.VolumeCapability_MountVolume{}, + }, + AccessMode: &csi.VolumeCapability_AccessMode{ + Mode: csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER, + }, + }, + }, + Parameters: map[string]string{ + paramSource: testServer, + }, + }, + expectErr: true, }, { - desc: "Valid request", - req: csi.CreateVolumeRequest{ - VolumeCapabilities: stdVolCap, + name: "invalid create context", + req: &csi.CreateVolumeRequest{ + Name: testCSIVolume, + VolumeCapabilities: []*csi.VolumeCapability{ + { + AccessType: &csi.VolumeCapability_Mount{ + Mount: &csi.VolumeCapability_MountVolume{}, + }, + AccessMode: &csi.VolumeCapability_AccessMode{ + Mode: csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER, + }, + }, + }, + Parameters: map[string]string{ + "unknown-parameter": "foo", + }, }, - expectedErr: nil, + expectErr: true, }, } - for _, test := range tests { - _, err := d.CreateVolume(context.Background(), &test.req) - if !reflect.DeepEqual(err, test.expectedErr) { - t.Errorf("Unexpected error: %v", err) - } + for _, test := range cases { + test := test //pin + t.Run(test.name, func(t *testing.T) { + // Setup + _ = os.MkdirAll(filepath.Join(d.workingMountDir, testCSIVolume), os.ModePerm) + + // Run + resp, err := d.CreateVolume(context.TODO(), test.req) + + // Verify + if test.expectErr && err == nil { + t.Errorf("test %q failed; got success", test.name) + } + + // separate assertion for flaky error messages + if test.flakyWindowsErrorMessage != "" && runtime.GOOS == "windows" { + fmt.Println("Skipping checks on Windows ENV") + } else { + if !test.expectErr && err != nil { + t.Errorf("test %q failed: %v", test.name, err) + } + if !reflect.DeepEqual(resp, test.resp) { + t.Errorf("test %q failed: got resp %+v, expected %+v", test.name, resp, test.resp) + } + if !test.expectErr { + info, err := os.Stat(filepath.Join(d.workingMountDir, test.req.Name, test.req.Name)) + if err != nil { + t.Errorf("test %q failed: couldn't find volume subdirectory: %v", test.name, err) + } + if !info.IsDir() { + t.Errorf("test %q failed: subfile not a directory", test.name) + } + } + } + }) } } func TestDeleteVolume(t *testing.T) { d := NewFakeDriver() - tests := []struct { + // Setup workingMountDir + workingMountDir, err := os.Getwd() + if err != nil { + t.Errorf("failed to get current working directory") + } + d.workingMountDir = workingMountDir + + // Setup mounter + mounter, err := NewFakeMounter() + if err != nil { + t.Fatalf(fmt.Sprintf("failed to get fake mounter: %v", err)) + } + d.mounter = mounter + + cases := []struct { desc string - req csi.DeleteVolumeRequest + req *csi.DeleteVolumeRequest + resp *csi.DeleteVolumeResponse expectedErr error }{ { desc: "Volume ID missing", - req: csi.DeleteVolumeRequest{}, + req: &csi.DeleteVolumeRequest{}, + resp: nil, expectedErr: status.Error(codes.InvalidArgument, "Volume ID missing in request"), }, { - desc: "Valid request", - req: csi.DeleteVolumeRequest{VolumeId: "vol_1"}, + desc: "Valid request", + req: &csi.DeleteVolumeRequest{ + VolumeId: testVolumeID, + Secrets: map[string]string{ + usernameField: "test", + passwordField: "test", + domainField: "test_doamin", + }, + }, + resp: &csi.DeleteVolumeResponse{}, expectedErr: nil, }, } - - for _, test := range tests { - _, err := d.DeleteVolume(context.Background(), &test.req) - if !reflect.DeepEqual(err, test.expectedErr) { - t.Errorf("Unexpected error: %v", err) - } + for _, test := range cases { + test := test //pin + t.Run(test.desc, func(t *testing.T) { + // Setup + _ = os.MkdirAll(filepath.Join(d.workingMountDir, testCSIVolume), os.ModePerm) + _, _ = os.Create(filepath.Join(d.workingMountDir, testCSIVolume, testCSIVolume)) + // Run + resp, err := d.DeleteVolume(context.TODO(), test.req) + // Verify + if runtime.GOOS == "windows" { + // skip checks + fmt.Println("Skipping checks on Windows ENV") + } else { + if test.expectedErr == nil && err != nil { + t.Errorf("test %q failed: %v", test.desc, err) + } + if test.expectedErr != nil && err == nil { + t.Errorf("test %q failed; expected error %v, got success", test.desc, test.expectedErr) + } + if !reflect.DeepEqual(resp, test.resp) { + t.Errorf("test %q failed: got resp %+v, expected %+v", test.desc, resp, test.resp) + } + if _, err := os.Stat(filepath.Join(d.workingMountDir, testCSIVolume, testCSIVolume)); test.expectedErr == nil && !os.IsNotExist(err) { + t.Errorf("test %q failed: expected volume subdirectory deleted, it still exists", test.desc) + } + } + }) } } diff --git a/pkg/smb/nodeserver.go b/pkg/smb/nodeserver.go index 55e84c86b0d..56668ca75e2 100644 --- a/pkg/smb/nodeserver.go +++ b/pkg/smb/nodeserver.go @@ -20,7 +20,6 @@ import ( "fmt" "io/ioutil" "os" - "path/filepath" "runtime" "strings" "time" @@ -83,27 +82,6 @@ func (d *Driver) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolu return nil, fmt.Errorf("prepare publish failed for %s with error: %v", target, err) } - context := req.GetVolumeContext() - var createSubDir string - for k, v := range context { - switch strings.ToLower(k) { - case createSubDirField: - createSubDir = v - } - } - - if strings.EqualFold(createSubDir, "true") { - source = filepath.Join(source, req.GetVolumeId()) - klog.V(2).Infof("NodePublishVolume: createSubDir(%s) MkdirAll(%s) volumeID(%s)", createSubDir, source, volumeID) - if err := Mkdir(d.mounter, source, 0750); err != nil { - if os.IsExist(err) { - klog.Warningf("Mkdir(%s) failed with error: %v", source, err) - } else { - return nil, status.Errorf(codes.Internal, "Mkdir(%s) failed with error: %v", source, err) - } - } - } - klog.V(2).Infof("NodePublishVolume: mounting %s at %s with mountOptions: %v volumeID(%s)", source, target, mountOptions, volumeID) if err := d.mounter.Mount(source, target, "", mountOptions); err != nil { if removeErr := os.Remove(target); removeErr != nil { diff --git a/pkg/smb/smb.go b/pkg/smb/smb.go index 3b25e474fbf..72e75f9e318 100644 --- a/pkg/smb/smb.go +++ b/pkg/smb/smb.go @@ -31,6 +31,7 @@ import ( const ( DefaultDriverName = "smb.csi.k8s.io" createSubDirField = "createsubdir" + paramSource = "source" ) // Driver implements all interfaces of CSI drivers @@ -39,7 +40,8 @@ type Driver struct { mounter *mount.SafeFormatAndMount // A map storing all volumes with ongoing operations so that additional operations // for that same volume (as defined by VolumeID) return an Aborted error - volumeLocks *volumeLocks + volumeLocks *volumeLocks + workingMountDir string } // NewDriver Creates a NewCSIDriver object. Assumes vendor version is equal to driver version & diff --git a/pkg/smb/smb_common_windows.go b/pkg/smb/smb_common_windows.go index ae6a9aa9144..d2e035bfd4b 100644 --- a/pkg/smb/smb_common_windows.go +++ b/pkg/smb/smb_common_windows.go @@ -20,11 +20,10 @@ package smb import ( "fmt" - "os" - "github.com/kubernetes-csi/csi-driver-smb/pkg/mounter" "k8s.io/klog/v2" mount "k8s.io/mount-utils" + "os" ) func Mount(m *mount.SafeFormatAndMount, source, target, fsType string, mountOptions, sensitiveMountOptions []string) error { diff --git a/test/e2e/suite_test.go b/test/e2e/suite_test.go index f138318f3a5..1d55917ed1a 100644 --- a/test/e2e/suite_test.go +++ b/test/e2e/suite_test.go @@ -55,8 +55,10 @@ var ( } storageClassCreateSubDir = map[string]string{ "source": "//smb-server.default.svc.cluster.local/share", - "csi.storage.k8s.io/node-stage-secret-name": "smbcreds", - "csi.storage.k8s.io/node-stage-secret-namespace": "default", + "csi.storage.k8s.io/node-stage-secret-name": "smbcreds", + "csi.storage.k8s.io/node-stage-secret-namespace": "default", + "csi.storage.k8s.io/provisioner-secret-name": "smbcreds", + "csi.storage.k8s.io/provisioner-secret-namespace": "default", "createSubDir": "true", } ) diff --git a/test/integration/run-test.sh b/test/integration/run-test.sh index bf73ceda89e..934615dbfa2 100755 --- a/test/integration/run-test.sh +++ b/test/integration/run-test.sh @@ -76,7 +76,7 @@ echo 'Mount volume test:' sleep 2 echo "node stats test:" -csc node stats --endpoint "$endpoint" "$volumeid:$target_path:$staging_target_path" +"$CSC_BIN" node stats --endpoint "$endpoint" "$volumeid:$target_path:$staging_target_path" sleep 2 echo 'Unmount volume test:' diff --git a/vendor/modules.txt b/vendor/modules.txt index ba21167bba0..0d7255d5f5f 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -779,7 +779,7 @@ k8s.io/kubernetes/test/e2e/storage/podlogs k8s.io/kubernetes/test/e2e/storage/utils k8s.io/kubernetes/test/utils k8s.io/kubernetes/test/utils/image -# k8s.io/mount-utils v0.0.0 => k8s.io/mount-utils v0.21.1 +# k8s.io/mount-utils v0.21.1 => k8s.io/mount-utils v0.21.1 ## explicit k8s.io/mount-utils # k8s.io/utils v0.0.0-20201110183641-67b214c5f920 => k8s.io/utils v0.0.0-20201110183641-67b214c5f920