diff --git a/Makefile b/Makefile index 1484b884..77835b87 100644 --- a/Makefile +++ b/Makefile @@ -159,6 +159,7 @@ generate-mock: mockgen -source=pkg/cryptsetup-client/cryptsetup_client.go -destination=mocks/mock_cryptsetupclient.go -package=mocks mockgen -source=internal/driver/metadata.go -destination=mocks/mock_metadata.go -package=mocks mockgen -source=pkg/hwinfo/hwinfo.go -destination=mocks/mock_hwinfo.go -package=mocks + mockgen -source=pkg/filesystem-stats/filesystem_stats.go -destination=mocks/mock_filesystemstatter.go -package=mocks .PHONY: test test: @@ -173,6 +174,10 @@ e2e-test: csi-sanity-test: KUBECONFIG=$(KUBECONFIG) ./tests/csi-sanity/run-tests.sh +.PHONY: sanity-test +sanity-test: + go test ./tests/sanity -v + .PHONY: upstream-e2e-tests upstream-e2e-tests: OS=$(OS) ARCH=$(ARCH_SHORT) K8S_VERSION=$(K8S_VERSION) KUBECONFIG=$(KUBECONFIG) ./tests/upstream-e2e/run-tests.sh diff --git a/go.mod b/go.mod index c5f1d0fe..30272e58 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,7 @@ require ( github.com/google/uuid v1.6.0 github.com/ianschenck/envflag v0.0.0-20140720210342-9111d830d133 github.com/jaypipes/ghw v0.21.1 + github.com/kubernetes-csi/csi-test/v5 v5.4.0 github.com/linode/go-metadata v0.2.3 github.com/linode/linodego v1.62.0 github.com/martinjungblut/go-cryptsetup v0.0.0-20220520180014-fd0874fd07a6 @@ -47,9 +48,12 @@ require ( github.com/go-openapi/jsonreference v0.20.2 // indirect github.com/go-openapi/swag v0.23.0 // indirect github.com/go-resty/resty/v2 v2.17.0 // indirect + github.com/go-task/slim-sprig/v3 v3.0.0 // indirect github.com/gogo/protobuf v1.3.2 // indirect github.com/google/gnostic-models v0.7.0 // indirect + github.com/google/go-cmp v0.7.0 // indirect github.com/google/go-querystring v1.1.0 // indirect + github.com/google/pprof v0.0.0-20241210010833-40e02aabc2ad // indirect github.com/grpc-ecosystem/grpc-gateway/v2 v2.27.3 // indirect github.com/jaypipes/pcidb v1.1.1 // indirect github.com/josharian/intern v1.0.0 // indirect @@ -59,6 +63,8 @@ require ( github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect + github.com/onsi/ginkgo/v2 v2.22.0 // indirect + github.com/onsi/gomega v1.36.1 // indirect github.com/pkg/errors v0.9.1 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/prometheus/client_model v0.6.2 // indirect @@ -77,11 +83,13 @@ require ( golang.org/x/term v0.38.0 // indirect golang.org/x/text v0.32.0 // indirect golang.org/x/time v0.12.0 // indirect + golang.org/x/tools v0.39.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20251202230838-ff82c1b0f217 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20251202230838-ff82c1b0f217 // indirect gopkg.in/evanphx/json-patch.v4 v4.12.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/ini.v1 v1.66.6 // indirect + gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect howett.net/plist v1.0.2-0.20250314012144-ee69052608d9 // indirect k8s.io/kube-openapi v0.0.0-20250710124328-f3f2b991d03b // indirect diff --git a/go.sum b/go.sum index 8502cb5c..206a39cc 100644 --- a/go.sum +++ b/go.sum @@ -48,8 +48,8 @@ github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX github.com/google/go-querystring v1.1.0 h1:AnCroh3fv4ZBgVIf1Iwtovgjaw/GiKJo8M8yD/fhyJ8= github.com/google/go-querystring v1.1.0/go.mod h1:Kcdr2DB4koayq7X8pmAG4sNG59So17icRSOU623lUBU= github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= -github.com/google/pprof v0.0.0-20241029153458-d1b30febd7db h1:097atOisP2aRj7vFgYQBbFN4U4JNXUNYpxael3UzMyo= -github.com/google/pprof v0.0.0-20241029153458-d1b30febd7db/go.mod h1:vavhavw2zAxS5dIdcRluK6cSGGPlZynqzFM8NdvU144= +github.com/google/pprof v0.0.0-20241210010833-40e02aabc2ad h1:a6HEuzUHeKH6hwfN/ZoQgRgVIWFJljSWa/zetS2WTvg= +github.com/google/pprof v0.0.0-20241210010833-40e02aabc2ad/go.mod h1:vavhavw2zAxS5dIdcRluK6cSGGPlZynqzFM8NdvU144= github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/grpc-ecosystem/grpc-gateway/v2 v2.27.3 h1:NmZ1PKzSTQbuGHw9DGPFomqkkLWMC+vZCkfs+FHv1Vg= @@ -78,6 +78,8 @@ github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= +github.com/kubernetes-csi/csi-test/v5 v5.4.0 h1:u5DgYNIreSNO2+u4Nq2Wpl+bbakRSjNyxZHmDTAqnYA= +github.com/kubernetes-csi/csi-test/v5 v5.4.0/go.mod h1:anAJKFUb/SdHhIHECgSKxC5LSiLzib+1I6mrWF5Hve8= github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0SNc= github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw= github.com/linode/go-metadata v0.2.3 h1:tGTVXJdVYI2e50jljW81C1Anmux7NfVX0MC6CgiJTyc= @@ -98,10 +100,10 @@ github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee h1:W5t00kpgFd github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= -github.com/onsi/ginkgo/v2 v2.21.0 h1:7rg/4f3rB88pb5obDgNZrNHrQ4e6WpjonchcpuBRnZM= -github.com/onsi/ginkgo/v2 v2.21.0/go.mod h1:7Du3c42kxCUegi0IImZ1wUQzMBVecgIHjR1C+NkhLQo= -github.com/onsi/gomega v1.35.1 h1:Cwbd75ZBPxFSuZ6T+rN/WCb/gOc6YgFBXLlZLhC7Ds4= -github.com/onsi/gomega v1.35.1/go.mod h1:PvZbdDc8J6XJEpDK4HCuRBm8a6Fzp9/DmhC9C7yFlog= +github.com/onsi/ginkgo/v2 v2.22.0 h1:Yed107/8DjTr0lKCNt7Dn8yQ6ybuDRQoMGrNFKzMfHg= +github.com/onsi/ginkgo/v2 v2.22.0/go.mod h1:7Du3c42kxCUegi0IImZ1wUQzMBVecgIHjR1C+NkhLQo= +github.com/onsi/gomega v1.36.1 h1:bJDPBO7ibjxcbHMgSCoo4Yj18UWbKDlLwX1x9sybDcw= +github.com/onsi/gomega v1.36.1/go.mod h1:PvZbdDc8J6XJEpDK4HCuRBm8a6Fzp9/DmhC9C7yFlog= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= @@ -236,6 +238,8 @@ gopkg.in/inf.v0 v0.9.1 h1:73M5CoZyi3ZLMOyDlQh031Cx6N9NDJ2Vvfl76EDAgDc= gopkg.in/inf.v0 v0.9.1/go.mod h1:cWUDdTG/fYaXco+Dcufb5Vnc6Gp2YChqWtbxRZE0mXw= gopkg.in/ini.v1 v1.66.6 h1:LATuAqN/shcYAOkv3wl2L4rkaKqkcgTBQjOyYDvcPKI= gopkg.in/ini.v1 v1.66.6/go.mod h1:pNLf8WUiyNEtQjuu5G5vTm06TEv9tsIgeAvK8hOrP4k= +gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY= +gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/driver/controllerserver.go b/internal/driver/controllerserver.go index 1a043d74..a8d37e67 100644 --- a/internal/driver/controllerserver.go +++ b/internal/driver/controllerserver.go @@ -26,6 +26,22 @@ type ControllerServer struct { csi.UnimplementedControllerServer } +// checkPublishCompatibility verifies if a re-publish request is compatible with existing publish. +// Uses volume tags to track readonly state. If a volume was published as readonly, it will have +// a tag "csi-readonly-". If no such tag exists, the volume is assumed to be read-write. +// Returns nil if compatible, or an AlreadyExists error if incompatible. +func checkPublishCompatibility(volume *linodego.Volume, linodeID int, readonly bool) error { + wasReadOnly := volumeHasReadOnlyTag(volume, linodeID) + + if wasReadOnly != readonly { + return status.Errorf(codes.AlreadyExists, + "volume %d already published to node %d with readonly=%v, cannot re-publish with readonly=%v", + volume.ID, linodeID, wasReadOnly, readonly) + } + + return nil +} + // NewControllerServer instantiates a new RPC service that implements the // CSI [Controller Service RPC] endpoints. // @@ -189,28 +205,35 @@ func (cs *ControllerServer) ControllerPublishVolume(ctx context.Context, req *cs } // Check if the volume exists and is valid. - // If the volume is already attached to the specified instance, it returns its device path. - devicePath, err := cs.getAndValidateVolume(ctx, volumeID, instance) + volume, err := cs.getAndValidateVolume(ctx, volumeID, instance) if err != nil { observability.RecordMetrics(observability.ControllerPublishVolumeTotal, observability.ControllerPublishVolumeDuration, observability.Failed, functionStartTime) return resp, err } - // If devicePath is not empty, the volume is already attached - if devicePath != "" { - observability.RecordMetrics(observability.ControllerPublishVolumeTotal, observability.ControllerPublishVolumeDuration, observability.Failed, functionStartTime) + + readonly := req.GetReadonly() + + // If volume is already attached to this instance, handle idempotency + if volume.LinodeID != nil && *volume.LinodeID == instance.ID { + if err := checkPublishCompatibility(volume, linodeID, readonly); err != nil { + observability.RecordMetrics(observability.ControllerPublishVolumeTotal, observability.ControllerPublishVolumeDuration, observability.Failed, functionStartTime) + return resp, err + } + observability.RecordMetrics(observability.ControllerPublishVolumeTotal, observability.ControllerPublishVolumeDuration, observability.Completed, functionStartTime) + log.V(2).Info("Volume already attached (idempotent)", "volume_id", volumeID, "device_path", volume.FilesystemPath) return &csi.ControllerPublishVolumeResponse{ - PublishContext: map[string]string{ - devicePathKey: devicePath, - }, + PublishContext: map[string]string{devicePathKey: volume.FilesystemPath}, }, nil } // Check if the instance can accommodate the volume attachment if capErr := cs.checkAttachmentCapacity(ctx, instance); capErr != nil { + log.V(2).Info("Cannot attach volume: capacity limit reached", "volume_id", volumeID, "node_id", linodeID, "error", capErr) observability.RecordMetrics(observability.ControllerPublishVolumeTotal, observability.ControllerPublishVolumeDuration, observability.Failed, functionStartTime) return resp, capErr } + log.V(4).Info("Attaching volume to instance", "volume_id", volumeID, "node_id", linodeID) // Attach the volume to the specified instance if attachErr := cs.attachVolume(ctx, volumeID, linodeID); attachErr != nil { observability.RecordMetrics(observability.ControllerPublishVolumeTotal, observability.ControllerPublishVolumeDuration, observability.Failed, functionStartTime) @@ -219,12 +242,19 @@ func (cs *ControllerServer) ControllerPublishVolume(ctx context.Context, req *cs log.V(4).Info("Waiting for volume to attach", "volume_id", volumeID) // Wait for the volume to be successfully attached to the instance - volume, err := cs.client.WaitForVolumeLinodeID(ctx, volumeID, &linodeID, waitTimeout()) + volume, err = cs.client.WaitForVolumeLinodeID(ctx, volumeID, &linodeID, waitTimeout()) if err != nil { observability.RecordMetrics(observability.ControllerPublishVolumeTotal, observability.ControllerPublishVolumeDuration, observability.Failed, functionStartTime) return resp, err } + // Ensure the readonly tag state is correct: + // - If publishing as readonly: add the tag + // - If publishing as read-write: remove any stale readonly tag (from a previous failed unpublish) + if err := cs.syncReadOnlyTag(ctx, volumeID, volume, linodeID, readonly); err != nil { + log.V(2).Info("Failed to sync readonly tag on volume", "volume_id", volumeID, "error", err) + } + // Record function completion observability.RecordMetrics(observability.ControllerPublishVolumeTotal, observability.ControllerPublishVolumeDuration, observability.Completed, functionStartTime) @@ -273,8 +303,11 @@ func (cs *ControllerServer) ControllerUnpublishVolume(ctx context.Context, req * return &csi.ControllerUnpublishVolumeResponse{}, errInternal("get volume %d: %v", volumeID, err) } + // Parse nodeID early so we can use it for tag cleanup later + var linodeID int if req.GetNodeId() != "" { - linodeID, statusErr := linodevolumes.NodeIdAsInt("ControllerUnpublishVolume", req) + var statusErr error + linodeID, statusErr = linodevolumes.NodeIdAsInt("ControllerUnpublishVolume", req) if statusErr != nil { observability.RecordMetrics(observability.ControllerUnpublishVolumeTotal, observability.ControllerUnpublishVolumeDuration, observability.Failed, functionStartTime) return &csi.ControllerUnpublishVolumeResponse{}, statusErr @@ -299,11 +332,19 @@ func (cs *ControllerServer) ControllerUnpublishVolume(ctx context.Context, req * } log.V(4).Info("Waiting for volume to detach") - if _, err := cs.client.WaitForVolumeLinodeID(ctx, volumeID, nil, waitTimeout()); err != nil { + volume, err = cs.client.WaitForVolumeLinodeID(ctx, volumeID, nil, waitTimeout()) + if err != nil { observability.RecordMetrics(observability.ControllerUnpublishVolumeTotal, observability.ControllerUnpublishVolumeDuration, observability.Failed, functionStartTime) return &csi.ControllerUnpublishVolumeResponse{}, errInternal("wait for volume %d to detach: %v", volumeID, err) } + // Remove readonly tag if nodeID was provided + if linodeID != 0 { + if err := cs.syncReadOnlyTag(ctx, volumeID, volume, linodeID, false); err != nil { + log.V(2).Info("Failed to remove readonly tag from volume", "volume_id", volumeID, "error", err) + } + } + // Record function completion observability.RecordMetrics(observability.ControllerUnpublishVolumeTotal, observability.ControllerUnpublishVolumeDuration, observability.Completed, functionStartTime) @@ -565,3 +606,15 @@ func getVolumeResponse(volume *linodego.Volume) (csiVolume *csi.Volume, publishe return } + +// syncReadOnlyTag ensures the readonly tag state is correct for a volume. +// If readonly is true, adds the tag; otherwise removes it if present. +// Returns nil if no update was needed or the update succeeded. +func (cs *ControllerServer) syncReadOnlyTag(ctx context.Context, volumeID int, volume *linodego.Volume, linodeID int, readonly bool) error { + newTags, updated := setReadOnlyTag(volume.Tags, linodeID, readonly) + if !updated { + return nil + } + _, err := cs.client.UpdateVolume(ctx, volumeID, linodego.VolumeUpdateOptions{Tags: &newTags}) + return err +} diff --git a/internal/driver/controllerserver_helper.go b/internal/driver/controllerserver_helper.go index 8fd33a9b..03e95a4a 100644 --- a/internal/driver/controllerserver_helper.go +++ b/internal/driver/controllerserver_helper.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "slices" "strings" "time" @@ -65,6 +66,11 @@ func cloneTimeout() int { } const ( + // VolumeReadOnlyTagPrefix is the prefix used for tagging volumes published as read-only. + // Format: "csi-readonly-" (e.g., "csi-readonly-12345678") + // Tag length: 13 + up to 10 digits = max 23 chars (within 3-50 limit) + VolumeReadOnlyTagPrefix = "csi-readonly-" + // VolumeTags is the parameter key used for passing a comma-separated list // of tags to the Linode API. VolumeTags = Name + "/volumeTags" @@ -178,7 +184,7 @@ func (cs *ControllerServer) getContentSourceVolume(ctx context.Context, contentS // Parse the volume ID from the content source volKey, err = linodevolumes.ParseLinodeVolumeKey(sourceVolume.GetVolumeId()) if err != nil { - return nil, errInternal("parse volume info from content source: %v", err) + return nil, errNotFound("parse volume info from content source: %v", err) } if volKey == nil { return nil, errInternal("processed *LinodeVolumeKey is nil") // Throw an internal error if the processed LinodeVolumeKey is nil @@ -186,7 +192,9 @@ func (cs *ControllerServer) getContentSourceVolume(ctx context.Context, contentS // Retrieve the volume data using the parsed volume ID volumeData, err := cs.client.GetVolume(ctx, volKey.VolumeID) - if err != nil { + if linodego.IsNotFound(err) { + return nil, errVolumeNotFound(volKey.VolumeID) + } else if err != nil { return nil, errInternal("get volume %d: %v", volKey.VolumeID, err) } if volumeData == nil { @@ -678,15 +686,13 @@ func (cs *ControllerServer) validateControllerPublishVolumeRequest(ctx context.C // getAndValidateVolume retrieves the volume by its ID and run checks. // // It performs the following checks: -// 1. If the volume is found and already attached to the specified Linode instance, -// it returns the device path of the volume. -// 2. If the volume is not found, it returns an error indicating that the volume does not exist. -// 3. If the volume is attached to a different instance, it returns an error indicating +// 1. If the volume is not found, it returns an error indicating that the volume does not exist. +// 2. If the volume is attached to a different instance, it returns an error indicating // that the volume is already attached elsewhere. // -// Additionally, it checks if the volume and instance are in the same region based on -// the provided volume context. If they are not in the same region, it returns an internal error. -func (cs *ControllerServer) getAndValidateVolume(ctx context.Context, volumeID int, instance *linodego.Instance) (string, error) { +// Returns the volume object. Caller should check volume.LinodeID to determine if already attached +// to the target instance, and use volume.FilesystemPath for the device path. +func (cs *ControllerServer) getAndValidateVolume(ctx context.Context, volumeID int, instance *linodego.Instance) (*linodego.Volume, error) { log, ctx := logger.GetLogger(ctx) log.V(4).Info("Entering getAndValidateVolume()", "volumeID", volumeID, "linodeID", instance.ID) defer log.V(4).Info("Exiting getAndValidateVolume()") @@ -697,21 +703,44 @@ func (cs *ControllerServer) getAndValidateVolume(ctx context.Context, volumeID i volume, err := cs.client.GetVolume(ctx, volumeID) if linodego.IsNotFound(err) { - return "", errVolumeNotFound(volumeID) + return nil, errVolumeNotFound(volumeID) } else if err != nil { - return "", errInternal("get volume %d: %v", volumeID, err) + return nil, errInternal("get volume %d: %v", volumeID, err) } if volume.LinodeID != nil { if *volume.LinodeID == instance.ID { log.V(4).Info("Volume already attached to instance", "volume_id", volume.ID, "node_id", *volume.LinodeID, "device_path", volume.FilesystemPath) - return volume.FilesystemPath, nil + return volume, nil } - return "", errVolumeAttached(volumeID, *volume.LinodeID) + return nil, errVolumeAttached(volumeID, *volume.LinodeID) } log.V(4).Info("Volume validated and is not attached to instance", "volume_id", volume.ID, "node_id", instance.ID) - return "", nil + return volume, nil +} + +// setReadOnlyTag updates the volume tags to reflect the readonly state for the given node. +// Returns the updated tags and whether an update is needed. +func setReadOnlyTag(existingTags []string, nodeID int, readonly bool) ([]string, bool) { + tag := fmt.Sprintf("%s%d", VolumeReadOnlyTagPrefix, nodeID) + idx := slices.Index(existingTags, tag) + hasTag := idx != -1 + + if readonly == hasTag { + return existingTags, false // already in desired state + } + + if readonly { + return append(existingTags, tag), true + } + return slices.Delete(existingTags, idx, idx+1), true +} + +// volumeHasReadOnlyTag checks if the volume has a read-only tag for the given node. +func volumeHasReadOnlyTag(volume *linodego.Volume, nodeID int) bool { + tag := fmt.Sprintf("%s%d", VolumeReadOnlyTagPrefix, nodeID) + return slices.Contains(volume.Tags, tag) } // getInstance retrieves the Linode instance by its ID. If the @@ -734,6 +763,9 @@ func (cs *ControllerServer) getInstance(ctx context.Context, linodeID int) (*lin // If any other error occurs, return an internal error. return nil, errInternal("get linode instance %d: %v", linodeID, err) } + if instance == nil { + return nil, errInstanceNotFound(linodeID) + } log.V(4).Info("Instance retrieved", "instance", instance) return instance, nil diff --git a/internal/driver/controllerserver_helper_test.go b/internal/driver/controllerserver_helper_test.go index cf99202a..210c8468 100644 --- a/internal/driver/controllerserver_helper_test.go +++ b/internal/driver/controllerserver_helper_test.go @@ -864,7 +864,7 @@ func TestGetAndValidateVolume(t *testing.T) { volumeID int linode *linodego.Instance setupMocks func() - expectedResult string + expectedVolume *linodego.Volume expectedError error }{ { @@ -880,8 +880,12 @@ func TestGetAndValidateVolume(t *testing.T) { FilesystemPath: "/dev/disk/by-id/scsi-0Linode_Volume_test-volume", }, nil) }, - expectedResult: "/dev/disk/by-id/scsi-0Linode_Volume_test-volume", - expectedError: nil, + expectedVolume: &linodego.Volume{ + ID: 123, + LinodeID: &[]int{456}[0], + FilesystemPath: "/dev/disk/by-id/scsi-0Linode_Volume_test-volume", + }, + expectedError: nil, }, { name: "Volume found but not attached", @@ -897,8 +901,12 @@ func TestGetAndValidateVolume(t *testing.T) { Region: "us-east", }, nil) }, - expectedResult: "", - expectedError: nil, + expectedVolume: &linodego.Volume{ + ID: 123, + LinodeID: nil, + Region: "us-east", + }, + expectedError: nil, }, { name: "Volume found but attached to different instance", @@ -912,7 +920,7 @@ func TestGetAndValidateVolume(t *testing.T) { LinodeID: &[]int{789}[0], }, nil) }, - expectedResult: "", + expectedVolume: nil, expectedError: errVolumeAttached(123, 789), }, { @@ -927,7 +935,7 @@ func TestGetAndValidateVolume(t *testing.T) { Message: "Not Found", }) }, - expectedResult: "", + expectedVolume: nil, expectedError: errVolumeNotFound(123), }, { @@ -939,7 +947,7 @@ func TestGetAndValidateVolume(t *testing.T) { setupMocks: func() { mockClient.EXPECT().GetVolume(gomock.Any(), 123).Return(nil, errors.New("API error")) }, - expectedResult: "", + expectedVolume: nil, expectedError: errInternal("get volume 123: API error"), }, } @@ -954,26 +962,24 @@ func TestGetAndValidateVolume(t *testing.T) { t.Errorf("expected error %v, got %v", tc.expectedError, err) } - if tc.expectedResult != result { - t.Errorf("expected result %s, got %s", tc.expectedResult, result) + if tc.expectedVolume == nil && result != nil { + t.Errorf("expected nil volume, got %v", result) + } else if tc.expectedVolume != nil { + if result == nil { + t.Errorf("expected volume %v, got nil", tc.expectedVolume) + } else if result.ID != tc.expectedVolume.ID { + t.Errorf("expected volume ID %d, got %d", tc.expectedVolume.ID, result.ID) + } } }) } } func TestCheckAttachmentCapacity(t *testing.T) { - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - mockClient := mocks.NewMockLinodeClient(ctrl) - cs := &ControllerServer{ - client: mockClient, - } - testCases := []struct { name string instance *linodego.Instance - setupMocks func() + setupMocks func(m *mocks.MockLinodeClient) expectedError error }{ { @@ -984,9 +990,9 @@ func TestCheckAttachmentCapacity(t *testing.T) { Memory: 4096, }, }, - setupMocks: func() { - mockClient.EXPECT().ListInstanceVolumes(gomock.Any(), 123, gomock.Any()).Return([]linodego.Volume{}, nil) - mockClient.EXPECT().ListInstanceDisks(gomock.Any(), 123, gomock.Any()).Return([]linodego.InstanceDisk{}, nil) + setupMocks: func(m *mocks.MockLinodeClient) { + m.EXPECT().ListInstanceVolumes(gomock.Any(), 123, gomock.Any()).Return([]linodego.Volume{}, nil) + m.EXPECT().ListInstanceDisks(gomock.Any(), 123, gomock.Any()).Return([]linodego.InstanceDisk{}, nil) }, expectedError: nil, }, @@ -998,23 +1004,72 @@ func TestCheckAttachmentCapacity(t *testing.T) { Memory: 1024, }, }, - setupMocks: func() { - mockClient.EXPECT().ListInstanceDisks(gomock.Any(), 456, gomock.Any()).Return([]linodego.InstanceDisk{{ID: 1}, {ID: 2}}, nil).AnyTimes() - mockClient.EXPECT().ListInstanceVolumes(gomock.Any(), 456, gomock.Any()).Return([]linodego.Volume{{ID: 1}, {ID: 2}, {ID: 3}, {ID: 4}, {ID: 5}, {ID: 6}}, nil) + setupMocks: func(m *mocks.MockLinodeClient) { + m.EXPECT().ListInstanceDisks(gomock.Any(), 456, gomock.Any()).Return([]linodego.InstanceDisk{{ID: 1}, {ID: 2}}, nil).AnyTimes() + m.EXPECT().ListInstanceVolumes(gomock.Any(), 456, gomock.Any()).Return([]linodego.Volume{{ID: 1}, {ID: 2}, {ID: 3}, {ID: 4}, {ID: 5}, {ID: 6}}, nil) }, expectedError: errMaxVolumeAttachments(6), }, + { + name: "ListInstanceVolumes error", + instance: &linodego.Instance{ + ID: 789, + Specs: &linodego.InstanceSpec{ + Memory: 4096, + }, + }, + setupMocks: func(m *mocks.MockLinodeClient) { + m.EXPECT().ListInstanceDisks(gomock.Any(), 789, gomock.Any()).Return([]linodego.InstanceDisk{}, nil) + m.EXPECT().ListInstanceVolumes(gomock.Any(), 789, gomock.Any()).Return(nil, errors.New("API error")) + }, + expectedError: errInternal("list instance volumes: API error"), + }, + { + name: "ListInstanceDisks error in canAttach", + instance: &linodego.Instance{ + ID: 101, + Specs: &linodego.InstanceSpec{ + Memory: 4096, + }, + }, + setupMocks: func(m *mocks.MockLinodeClient) { + m.EXPECT().ListInstanceDisks(gomock.Any(), 101, gomock.Any()).Return(nil, errors.New("disk API error")) + }, + expectedError: errInternal("list instance disks: disk API error"), + }, + { + name: "Nil instance specs", + instance: &linodego.Instance{ + ID: 102, + Specs: nil, + }, + setupMocks: func(m *mocks.MockLinodeClient) { + // No mocks needed - should fail before API calls due to nil specs + }, + expectedError: errNilInstance, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - tc.setupMocks() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockClient := mocks.NewMockLinodeClient(ctrl) + cs := &ControllerServer{ + client: mockClient, + } + + tc.setupMocks(mockClient) err := cs.checkAttachmentCapacity(context.Background(), tc.instance) if err != nil && !reflect.DeepEqual(tc.expectedError, err) { t.Errorf("expected error %v, got %v", tc.expectedError, err) } + if err == nil && tc.expectedError != nil { + t.Errorf("expected error %v, got nil", tc.expectedError) + } }) } } @@ -1084,7 +1139,7 @@ func TestGetContentSourceVolume(t *testing.T) { }, setupMocks: func() {}, expectedResult: nil, - expectedError: errInternal("parse volume info from content source: invalid linode volume id: \"test\""), + expectedError: errNotFound("parse volume info from content source: invalid linode volume id: \"test\""), }, { name: "Valid content source, matching region", @@ -1390,3 +1445,158 @@ func Test_getRegionFromTopology(t *testing.T) { }) } } + +func TestSetReadOnlyTag(t *testing.T) { + tests := []struct { + name string + existingTags []string + nodeID int + readonly bool + expectedTags []string + expectedUpdated bool + }{ + { + name: "add to empty tags", + existingTags: []string{}, + nodeID: 123, + readonly: true, + expectedTags: []string{"csi-readonly-123"}, + expectedUpdated: true, + }, + { + name: "add to existing tags", + existingTags: []string{"other-tag"}, + nodeID: 123, + readonly: true, + expectedTags: []string{"other-tag", "csi-readonly-123"}, + expectedUpdated: true, + }, + { + name: "add when tag already exists - no update", + existingTags: []string{"other-tag", "csi-readonly-123"}, + nodeID: 123, + readonly: true, + expectedTags: []string{"other-tag", "csi-readonly-123"}, + expectedUpdated: false, + }, + { + name: "add to nil tags", + existingTags: nil, + nodeID: 123, + readonly: true, + expectedTags: []string{"csi-readonly-123"}, + expectedUpdated: true, + }, + { + name: "remove existing tag", + existingTags: []string{"other-tag", "csi-readonly-123"}, + nodeID: 123, + readonly: false, + expectedTags: []string{"other-tag"}, + expectedUpdated: true, + }, + { + name: "remove when tag not present - no update", + existingTags: []string{"other-tag", "csi-readonly-456"}, + nodeID: 123, + readonly: false, + expectedTags: []string{"other-tag", "csi-readonly-456"}, + expectedUpdated: false, + }, + { + name: "remove from single tag list", + existingTags: []string{"csi-readonly-123"}, + nodeID: 123, + readonly: false, + expectedTags: []string{}, + expectedUpdated: true, + }, + { + name: "remove from empty tags - no update", + existingTags: []string{}, + nodeID: 123, + readonly: false, + expectedTags: []string{}, + expectedUpdated: false, + }, + { + name: "large node ID", + existingTags: []string{}, + nodeID: 1234567890, + readonly: true, + expectedTags: []string{"csi-readonly-1234567890"}, + expectedUpdated: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, updated := setReadOnlyTag(tt.existingTags, tt.nodeID, tt.readonly) + if !reflect.DeepEqual(got, tt.expectedTags) { + t.Errorf("setReadOnlyTag() tags = %v, want %v", got, tt.expectedTags) + } + if updated != tt.expectedUpdated { + t.Errorf("setReadOnlyTag() updated = %v, want %v", updated, tt.expectedUpdated) + } + }) + } +} + +func TestVolumeHasReadOnlyTag(t *testing.T) { + tests := []struct { + name string + volume *linodego.Volume + nodeID int + expected bool + }{ + { + name: "has matching tag", + volume: &linodego.Volume{ + Tags: []string{"other-tag", "csi-readonly-123"}, + }, + nodeID: 123, + expected: true, + }, + { + name: "no matching tag", + volume: &linodego.Volume{ + Tags: []string{"other-tag", "csi-readonly-456"}, + }, + nodeID: 123, + expected: false, + }, + { + name: "empty tags", + volume: &linodego.Volume{ + Tags: []string{}, + }, + nodeID: 123, + expected: false, + }, + { + name: "nil tags", + volume: &linodego.Volume{ + Tags: nil, + }, + nodeID: 123, + expected: false, + }, + { + name: "partial match should not match", + volume: &linodego.Volume{ + Tags: []string{"csi-readonly-1234"}, + }, + nodeID: 123, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := volumeHasReadOnlyTag(tt.volume, tt.nodeID) + if got != tt.expected { + t.Errorf("volumeHasReadOnlyTag() = %v, want %v", got, tt.expected) + } + }) + } +} diff --git a/internal/driver/controllerserver_test.go b/internal/driver/controllerserver_test.go index def04700..583215ce 100644 --- a/internal/driver/controllerserver_test.go +++ b/internal/driver/controllerserver_test.go @@ -212,14 +212,197 @@ func TestControllerPublishVolume(t *testing.T) { }, expectLinodeClientCalls: func(m *mocks.MockLinodeClient) { m.EXPECT().GetInstance(gomock.Any(), gomock.Any()).Return(&linodego.Instance{ID: 1003, Specs: &linodego.InstanceSpec{Memory: 16 << 10}}, nil) - m.EXPECT().GetVolume(gomock.Any(), gomock.Any()).Return(&linodego.Volume{ID: 1001, LinodeID: createLinodeID(1003), Size: 10, Status: linodego.VolumeActive}, nil).AnyTimes() - m.EXPECT().WaitForVolumeLinodeID(gomock.Any(), 630706045, gomock.Any(), gomock.Any()).Return(&linodego.Volume{ID: 1001, LinodeID: createLinodeID(1003), Size: 10, Status: linodego.VolumeActive}, nil) - m.EXPECT().AttachVolume(gomock.Any(), 630706045, gomock.Any()).Return(&linodego.Volume{ID: 1001, LinodeID: createLinodeID(1003), Size: 10, Status: linodego.VolumeActive}, nil) + // Volume NOT attached initially (LinodeID = nil) + m.EXPECT().GetVolume(gomock.Any(), gomock.Any()).Return(&linodego.Volume{ID: 630706045, LinodeID: nil, Size: 10, Status: linodego.VolumeActive}, nil) m.EXPECT().ListInstanceVolumes(gomock.Any(), 1003, gomock.Any()).Return([]linodego.Volume{{ID: 1001, LinodeID: createLinodeID(1003), Size: 10, Status: linodego.VolumeActive}}, nil) m.EXPECT().ListInstanceDisks(gomock.Any(), 1003, gomock.Any()).Return([]linodego.InstanceDisk{}, nil) + m.EXPECT().AttachVolume(gomock.Any(), 630706045, gomock.Any()).Return(&linodego.Volume{ID: 630706045, LinodeID: createLinodeID(1003), Size: 10, Status: linodego.VolumeActive}, nil) + m.EXPECT().WaitForVolumeLinodeID(gomock.Any(), 630706045, gomock.Any(), gomock.Any()).Return(&linodego.Volume{ID: 630706045, LinodeID: createLinodeID(1003), Size: 10, Status: linodego.VolumeActive, FilesystemPath: "/dev/sda"}, nil) }, expectedError: nil, }, + { + name: "idempotent publish with compatible readonly (both RW)", + req: &csi.ControllerPublishVolumeRequest{ + VolumeId: "1004-testvol", + NodeId: "2004", + VolumeCapability: &csi.VolumeCapability{ + AccessType: &csi.VolumeCapability_Mount{ + Mount: &csi.VolumeCapability_MountVolume{}, + }, + AccessMode: &csi.VolumeCapability_AccessMode{ + Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER, + }, + }, + VolumeContext: map[string]string{ + VolumeTopologyRegion: "us-east", + }, + Readonly: false, + }, + resp: &csi.ControllerPublishVolumeResponse{ + PublishContext: map[string]string{ + "devicePath": "/dev/disk/by-id/scsi-0Linode_Volume_test", + }, + }, + expectLinodeClientCalls: func(m *mocks.MockLinodeClient) { + m.EXPECT().GetInstance(gomock.Any(), 2004).Return(&linodego.Instance{ID: 2004, Specs: &linodego.InstanceSpec{Memory: 16 << 10}}, nil) + // Volume already attached to the same instance, no readonly tag = RW + m.EXPECT().GetVolume(gomock.Any(), 1004).Return(&linodego.Volume{ + ID: 1004, + LinodeID: createLinodeID(2004), + Size: 10, + Status: linodego.VolumeActive, + FilesystemPath: "/dev/disk/by-id/scsi-0Linode_Volume_test", + Tags: []string{}, + }, nil) + }, + expectedError: nil, + }, + { + name: "idempotent publish with incompatible readonly flag (was RW, requesting RO)", + req: &csi.ControllerPublishVolumeRequest{ + VolumeId: "1005-testvol", + NodeId: "2005", + VolumeCapability: &csi.VolumeCapability{ + AccessType: &csi.VolumeCapability_Mount{ + Mount: &csi.VolumeCapability_MountVolume{}, + }, + AccessMode: &csi.VolumeCapability_AccessMode{ + Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER, + }, + }, + VolumeContext: map[string]string{ + VolumeTopologyRegion: "us-east", + }, + Readonly: true, // Requesting readonly but volume was published as RW (no tag) + }, + expectLinodeClientCalls: func(m *mocks.MockLinodeClient) { + m.EXPECT().GetInstance(gomock.Any(), 2005).Return(&linodego.Instance{ID: 2005, Specs: &linodego.InstanceSpec{Memory: 16 << 10}}, nil) + // Volume already attached to the same instance, no readonly tag = was RW + m.EXPECT().GetVolume(gomock.Any(), 1005).Return(&linodego.Volume{ + ID: 1005, + LinodeID: createLinodeID(2005), + Size: 10, + Status: linodego.VolumeActive, + FilesystemPath: "/dev/disk/by-id/scsi-0Linode_Volume_test", + Tags: []string{}, // No readonly tag = was RW + }, nil) + }, + expectedError: errAlreadyExists("volume 1005 already published to node 2005 with readonly=false, cannot re-publish with readonly=true"), + }, + { + name: "idempotent publish with incompatible readonly flag (was RO, requesting RW)", + req: &csi.ControllerPublishVolumeRequest{ + VolumeId: "1006-testvol", + NodeId: "2006", + VolumeCapability: &csi.VolumeCapability{ + AccessType: &csi.VolumeCapability_Mount{ + Mount: &csi.VolumeCapability_MountVolume{}, + }, + AccessMode: &csi.VolumeCapability_AccessMode{ + Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER, + }, + }, + VolumeContext: map[string]string{ + VolumeTopologyRegion: "us-east", + }, + Readonly: false, // Requesting RW but volume was published as readonly (has tag) + }, + expectLinodeClientCalls: func(m *mocks.MockLinodeClient) { + m.EXPECT().GetInstance(gomock.Any(), 2006).Return(&linodego.Instance{ID: 2006, Specs: &linodego.InstanceSpec{Memory: 16 << 10}}, nil) + // Volume already attached, has readonly tag = was published as readonly + m.EXPECT().GetVolume(gomock.Any(), 1006).Return(&linodego.Volume{ + ID: 1006, + LinodeID: createLinodeID(2006), + Size: 10, + Status: linodego.VolumeActive, + FilesystemPath: "/dev/disk/by-id/scsi-0Linode_Volume_test", + Tags: []string{"csi-readonly-2006"}, // Has readonly tag = was RO + }, nil) + }, + expectedError: errAlreadyExists("volume 1006 already published to node 2006 with readonly=true, cannot re-publish with readonly=false"), + }, + { + name: "publish fails due to max attachments reached", + req: &csi.ControllerPublishVolumeRequest{ + VolumeId: "1007-testvol", + NodeId: "2007", + VolumeCapability: &csi.VolumeCapability{ + AccessType: &csi.VolumeCapability_Mount{ + Mount: &csi.VolumeCapability_MountVolume{}, + }, + AccessMode: &csi.VolumeCapability_AccessMode{ + Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER, + }, + }, + VolumeContext: map[string]string{ + VolumeTopologyRegion: "us-east", + }, + Readonly: false, + }, + expectLinodeClientCalls: func(m *mocks.MockLinodeClient) { + m.EXPECT().GetInstance(gomock.Any(), 2007).Return(&linodego.Instance{ + ID: 2007, + Specs: &linodego.InstanceSpec{Memory: 1024}, // Low memory = fewer allowed attachments + }, nil) + // Volume not attached + m.EXPECT().GetVolume(gomock.Any(), 1007).Return(&linodego.Volume{ + ID: 1007, + LinodeID: nil, + Size: 10, + Status: linodego.VolumeActive, + }, nil) + // checkAttachmentCapacity: already at max attachments + m.EXPECT().ListInstanceDisks(gomock.Any(), 2007, gomock.Any()).Return([]linodego.InstanceDisk{{ID: 1}, {ID: 2}}, nil).AnyTimes() + m.EXPECT().ListInstanceVolumes(gomock.Any(), 2007, gomock.Any()).Return([]linodego.Volume{ + {ID: 1}, {ID: 2}, {ID: 3}, {ID: 4}, {ID: 5}, {ID: 6}, + }, nil) + }, + expectedError: errMaxVolumeAttachments(6), + }, + { + name: "publish fails when WaitForVolumeLinodeID times out", + req: &csi.ControllerPublishVolumeRequest{ + VolumeId: "1008-testvol", + NodeId: "2008", + VolumeCapability: &csi.VolumeCapability{ + AccessType: &csi.VolumeCapability_Mount{ + Mount: &csi.VolumeCapability_MountVolume{}, + }, + AccessMode: &csi.VolumeCapability_AccessMode{ + Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER, + }, + }, + VolumeContext: map[string]string{ + VolumeTopologyRegion: "us-east", + }, + Readonly: false, + }, + expectLinodeClientCalls: func(m *mocks.MockLinodeClient) { + m.EXPECT().GetInstance(gomock.Any(), 2008).Return(&linodego.Instance{ + ID: 2008, + Specs: &linodego.InstanceSpec{Memory: 16 << 10}, + }, nil) + // Volume not attached + m.EXPECT().GetVolume(gomock.Any(), 1008).Return(&linodego.Volume{ + ID: 1008, + LinodeID: nil, + Size: 10, + Status: linodego.VolumeActive, + }, nil) + // checkAttachmentCapacity: can attach + m.EXPECT().ListInstanceDisks(gomock.Any(), 2008, gomock.Any()).Return([]linodego.InstanceDisk{}, nil) + m.EXPECT().ListInstanceVolumes(gomock.Any(), 2008, gomock.Any()).Return([]linodego.Volume{}, nil) + // AttachVolume succeeds + m.EXPECT().AttachVolume(gomock.Any(), 1008, gomock.Any()).Return(&linodego.Volume{ + ID: 1008, + LinodeID: createLinodeID(2008), + }, nil) + // WaitForVolumeLinodeID fails (timeout) + m.EXPECT().WaitForVolumeLinodeID(gomock.Any(), 1008, gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("timed out waiting for volume attachment")) + }, + expectedError: fmt.Errorf("timed out waiting for volume attachment"), + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -708,6 +891,11 @@ func (flc *fakeLinodeClient) DeleteVolume(context.Context, int) error { return n func (flc *fakeLinodeClient) ResizeVolume(context.Context, int, int) error { return nil } +//nolint:nilnil // TODO: re-work tests +func (flc *fakeLinodeClient) UpdateVolume(context.Context, int, linodego.VolumeUpdateOptions) (*linodego.Volume, error) { + return nil, nil +} + //nolint:nilnil // TODO: re-work tests func (flc *fakeLinodeClient) NewEventPoller(context.Context, any, linodego.EntityType, linodego.EventAction) (*linodego.EventPoller, error) { return nil, nil @@ -1078,3 +1266,89 @@ func TestControllerGetVolume(t *testing.T) { }) } } + +func TestCheckPublishCompatibility(t *testing.T) { + tests := []struct { + name string + volume *linodego.Volume + linodeID int + readonly bool + wantErr bool + }{ + { + name: "no readonly tag, requesting read-write - should succeed", + volume: &linodego.Volume{ + ID: 100, + Tags: []string{"other-tag"}, + }, + linodeID: 200, + readonly: false, + wantErr: false, + }, + { + name: "no readonly tag, requesting readonly - should fail", + volume: &linodego.Volume{ + ID: 101, + Tags: []string{"other-tag"}, + }, + linodeID: 201, + readonly: true, + wantErr: true, + }, + { + name: "has readonly tag, requesting readonly - should succeed", + volume: &linodego.Volume{ + ID: 102, + Tags: []string{"csi-readonly-202", "other-tag"}, + }, + linodeID: 202, + readonly: true, + wantErr: false, + }, + { + name: "has readonly tag, requesting read-write - should fail", + volume: &linodego.Volume{ + ID: 103, + Tags: []string{"csi-readonly-203"}, + }, + linodeID: 203, + readonly: false, + wantErr: true, + }, + { + name: "has readonly tag for different node - should succeed (no tag for this node)", + volume: &linodego.Volume{ + ID: 104, + Tags: []string{"csi-readonly-999"}, + }, + linodeID: 204, + readonly: false, + wantErr: false, + }, + { + name: "empty tags, requesting read-write - should succeed", + volume: &linodego.Volume{ + ID: 105, + Tags: []string{}, + }, + linodeID: 205, + readonly: false, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotErr := checkPublishCompatibility(tt.volume, tt.linodeID, tt.readonly) + if gotErr != nil { + if !tt.wantErr { + t.Errorf("checkPublishCompatibility() failed: %v", gotErr) + } + return + } + if tt.wantErr { + t.Fatal("checkPublishCompatibility() succeeded unexpectedly") + } + }) + } +} diff --git a/internal/driver/driver.go b/internal/driver/driver.go index 1b8c2a31..80430afa 100644 --- a/internal/driver/driver.go +++ b/internal/driver/driver.go @@ -94,6 +94,8 @@ func (linodeDriver *LinodeDriver) SetupLinodeDriver( metricsPort string, enableTracing string, tracingPort string, + fsStatter FilesystemStatter, + hw hwinfo.HardwareInfo, ) error { log, ctx := logger.GetLogger(ctx) log, done := logger.WithMethod(log, "SetupLinodeDriver") @@ -123,8 +125,7 @@ func (linodeDriver *LinodeDriver) SetupLinodeDriver( log.V(2).Info("Setting up RPC Servers") - hw := hwinfo.NewHardwareInfo() - linodeDriver.ns, err = NewNodeServer(ctx, linodeDriver, mounter, deviceUtils, metadata, encrypt, resizeFs, hw) + linodeDriver.ns, err = NewNodeServer(ctx, linodeDriver, mounter, deviceUtils, metadata, encrypt, resizeFs, hw, fsStatter) if err != nil { return fmt.Errorf("new node server: %w", err) } diff --git a/internal/driver/driver_test.go b/internal/driver/driver_test.go index e81e0f63..7003115e 100644 --- a/internal/driver/driver_test.go +++ b/internal/driver/driver_test.go @@ -10,6 +10,7 @@ import ( "k8s.io/mount-utils" "github.com/linode/linode-blockstorage-csi-driver/mocks" + "github.com/linode/linode-blockstorage-csi-driver/pkg/hwinfo" linodeclient "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-client" mountmanager "github.com/linode/linode-blockstorage-csi-driver/pkg/mount-manager" ) @@ -61,7 +62,8 @@ func TestDriverSuite(t *testing.T) { metricsPort := "10251" enableTracing := "true" tracingPort := "4318" - if err := linodeDriver.SetupLinodeDriver(context.Background(), fakeCloudProvider, mounter, deviceUtils, resizeFs, md, driver, vendorVersion, bsPrefix, encrypt, enableMetrics, metricsPort, enableTracing, tracingPort); err != nil { + hw := hwinfo.NewHardwareInfo() + if err := linodeDriver.SetupLinodeDriver(context.Background(), fakeCloudProvider, mounter, deviceUtils, resizeFs, md, driver, vendorVersion, bsPrefix, encrypt, enableMetrics, metricsPort, enableTracing, tracingPort, nil, hw); err != nil { t.Fatalf("Failed to setup Linode Driver: %v", err) } diff --git a/internal/driver/filesystem_statter.go b/internal/driver/filesystem_statter.go new file mode 100644 index 00000000..40cf7568 --- /dev/null +++ b/internal/driver/filesystem_statter.go @@ -0,0 +1,6 @@ +package driver + +import filesystemstats "github.com/linode/linode-blockstorage-csi-driver/pkg/filesystem-stats" + +// FilesystemStatter is an alias to make it easier to use in the driver package. +type FilesystemStatter = filesystemstats.FilesystemStatter diff --git a/internal/driver/nodeserver.go b/internal/driver/nodeserver.go index f848c6b8..881c0195 100644 --- a/internal/driver/nodeserver.go +++ b/internal/driver/nodeserver.go @@ -44,6 +44,7 @@ type NodeServer struct { metadata Metadata encrypt Encryption resizeFs mountmanager.ResizeFSer + fsStatter FilesystemStatter // TODO: Only lock mutually exclusive calls and make locking more fine grained mux sync.Mutex @@ -52,7 +53,7 @@ type NodeServer struct { var _ csi.NodeServer = &NodeServer{} -func NewNodeServer(ctx context.Context, linodeDriver *LinodeDriver, mounter *mountmanager.SafeFormatAndMount, deviceUtils devicemanager.DeviceUtils, metadata Metadata, encrypt Encryption, resize mountmanager.ResizeFSer, hw hwinfo.HardwareInfo) (*NodeServer, error) { +func NewNodeServer(ctx context.Context, linodeDriver *LinodeDriver, mounter *mountmanager.SafeFormatAndMount, deviceUtils devicemanager.DeviceUtils, metadata Metadata, encrypt Encryption, resize mountmanager.ResizeFSer, hw hwinfo.HardwareInfo, fsStatter FilesystemStatter) (*NodeServer, error) { log, _ := logger.GetLogger(ctx) log.V(4).Info("Creating new NodeServer") @@ -69,7 +70,6 @@ func NewNodeServer(ctx context.Context, linodeDriver *LinodeDriver, mounter *mou log.Error(nil, "DeviceUtils is nil") return nil, fmt.Errorf("deviceUtils is nil") } - ns := &NodeServer{ driver: linodeDriver, mounter: mounter, @@ -78,6 +78,7 @@ func NewNodeServer(ctx context.Context, linodeDriver *LinodeDriver, mounter *mou encrypt: encrypt, resizeFs: resize, hardwareInfo: hw, + fsStatter: fsStatter, } log.V(4).Info("NodeServer created successfully") @@ -481,5 +482,5 @@ func (ns *NodeServer) NodeGetVolumeStats(ctx context.Context, req *csi.NodeGetVo log.V(2).Info("Processing request", "req", req) - return nodeGetVolumeStats(ctx, req) + return nodeGetVolumeStats(ctx, req, ns.fsStatter) } diff --git a/internal/driver/nodeserver_all.go b/internal/driver/nodeserver_all.go index edd116e8..cee83f3b 100644 --- a/internal/driver/nodeserver_all.go +++ b/internal/driver/nodeserver_all.go @@ -15,10 +15,7 @@ import ( "github.com/linode/linode-blockstorage-csi-driver/pkg/logger" ) -// unixStatfs is used to mock the unix.Statfs function. -var unixStatfs = unix.Statfs - -func nodeGetVolumeStats(ctx context.Context, req *csi.NodeGetVolumeStatsRequest) (*csi.NodeGetVolumeStatsResponse, error) { +func nodeGetVolumeStats(ctx context.Context, req *csi.NodeGetVolumeStatsRequest, fsStatter FilesystemStatter) (*csi.NodeGetVolumeStatsResponse, error) { log, _ := logger.GetLogger(ctx) if req.GetVolumeId() == "" || req.GetVolumePath() == "" { @@ -27,7 +24,7 @@ func nodeGetVolumeStats(ctx context.Context, req *csi.NodeGetVolumeStatsRequest) var statfs unix.Statfs_t // See http://man7.org/linux/man-pages/man2/statfs.2.html for details. - err := unixStatfs(req.GetVolumePath(), &statfs) + err := fsStatter.Statfs(req.GetVolumePath(), &statfs) switch { case errors.Is(err, unix.EIO): // EIO is returned when the filesystem is not mounted. diff --git a/internal/driver/nodeserver_all_test.go b/internal/driver/nodeserver_all_test.go index 500820cb..8432bee5 100644 --- a/internal/driver/nodeserver_all_test.go +++ b/internal/driver/nodeserver_all_test.go @@ -14,11 +14,19 @@ import ( "google.golang.org/grpc/status" ) +type testFilesystemStatter struct { + statfsFunc func(string, *unix.Statfs_t) error +} + +func (t *testFilesystemStatter) Statfs(path string, stat *unix.Statfs_t) error { + return t.statfsFunc(path, stat) +} + func TestNodeGetVolumeStats(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() - mockStatfs := func(path string, stat *unix.Statfs_t) error { + mockFsStatter := func(path string, stat *unix.Statfs_t) error { switch path { case "/valid/path": stat.Blocks = 1000 @@ -37,7 +45,8 @@ func TestNodeGetVolumeStats(t *testing.T) { } } - unixStatfs = mockStatfs + // Create a simple implementation for testing + testStatter := &testFilesystemStatter{statfsFunc: mockFsStatter} testCases := []struct { name string @@ -115,7 +124,7 @@ func TestNodeGetVolumeStats(t *testing.T) { VolumePath: tc.volumePath, } - resp, err := nodeGetVolumeStats(ctx, req) + resp, err := nodeGetVolumeStats(ctx, req, testStatter) if tc.expectedErr != nil { require.EqualError(t, err, tc.expectedErr.Error()) diff --git a/internal/driver/nodeserver_helpers_test.go b/internal/driver/nodeserver_helpers_test.go index b33abaff..c2856c37 100644 --- a/internal/driver/nodeserver_helpers_test.go +++ b/internal/driver/nodeserver_helpers_test.go @@ -419,7 +419,6 @@ func Test_validateNodeUnpublishVolumeRequest(t *testing.T) { req *csi.NodeUnpublishVolumeRequest wantErr bool }{ - // TODO: Add test cases. { name: "Valid request", req: &csi.NodeUnpublishVolumeRequest{ @@ -428,6 +427,11 @@ func Test_validateNodeUnpublishVolumeRequest(t *testing.T) { }, wantErr: false, }, + { + name: "Missing volume ID and target path", + req: &csi.NodeUnpublishVolumeRequest{}, + wantErr: true, + }, { name: "Missing volume ID", req: &csi.NodeUnpublishVolumeRequest{ @@ -437,7 +441,7 @@ func Test_validateNodeUnpublishVolumeRequest(t *testing.T) { wantErr: true, }, { - name: "Missing staging target path", + name: "Missing target path", req: &csi.NodeUnpublishVolumeRequest{ VolumeId: "vol-123", TargetPath: "", diff --git a/internal/driver/nodeserver_test.go b/internal/driver/nodeserver_test.go index 9cb7c2f6..372dcd99 100644 --- a/internal/driver/nodeserver_test.go +++ b/internal/driver/nodeserver_test.go @@ -860,7 +860,7 @@ func TestNewNodeServer(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := NewNodeServer(context.Background(), tt.args.linodeDriver, tt.args.mounter, tt.args.deviceUtils, tt.args.metadata, tt.args.encrypt, tt.args.resizeFs, tt.args.hw) + got, err := NewNodeServer(context.Background(), tt.args.linodeDriver, tt.args.mounter, tt.args.deviceUtils, tt.args.metadata, tt.args.encrypt, tt.args.resizeFs, tt.args.hw, nil) if (err != nil) != tt.wantErr { t.Errorf("NewNodeServer() error = %v, wantErr %v", err, tt.wantErr) return diff --git a/main.go b/main.go index 0de6bc2a..6f5f6d3f 100644 --- a/main.go +++ b/main.go @@ -31,6 +31,8 @@ import ( cryptsetupclient "github.com/linode/linode-blockstorage-csi-driver/pkg/cryptsetup-client" devicemanager "github.com/linode/linode-blockstorage-csi-driver/pkg/device-manager" filesystem "github.com/linode/linode-blockstorage-csi-driver/pkg/filesystem" + filesystemstats "github.com/linode/linode-blockstorage-csi-driver/pkg/filesystem-stats" + "github.com/linode/linode-blockstorage-csi-driver/pkg/hwinfo" linodeclient "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-client" "github.com/linode/linode-blockstorage-csi-driver/pkg/logger" mountmanager "github.com/linode/linode-blockstorage-csi-driver/pkg/mount-manager" @@ -150,6 +152,8 @@ func handle(ctx context.Context) error { cryptSetup := cryptsetupclient.NewCryptSetup() encrypt := driver.NewLuksEncryption(mounter.Exec, fileSystem, cryptSetup) resizer := mount.NewResizeFs(mounter.Exec) + fsStatter := filesystemstats.NewFilesystemStatter() + hw := hwinfo.NewHardwareInfo() nodeMetadata, err := driver.GetNodeMetadata(ctx, cloudProvider, cfg.nodeName, fileSystem) if err != nil { @@ -171,6 +175,8 @@ func handle(ctx context.Context) error { cfg.metricsPort, cfg.enableTracing, cfg.tracingPort, + fsStatter, + hw, ); err != nil { return fmt.Errorf("setup driver: %w", err) } diff --git a/mocks/mock_cryptsetupclient.go b/mocks/mock_cryptsetupclient.go index e16b5910..8804fef7 100644 --- a/mocks/mock_cryptsetupclient.go +++ b/mocks/mock_cryptsetupclient.go @@ -21,6 +21,7 @@ import ( type MockDevice struct { ctrl *gomock.Controller recorder *MockDeviceMockRecorder + isgomock struct{} } // MockDeviceMockRecorder is the mock recorder for MockDevice. @@ -186,6 +187,7 @@ func (mr *MockDeviceMockRecorder) VolumeKeyGet(keyslot, passphrase any) *gomock. type MockCryptSetupClient struct { ctrl *gomock.Controller recorder *MockCryptSetupClientMockRecorder + isgomock struct{} } // MockCryptSetupClientMockRecorder is the mock recorder for MockCryptSetupClient. diff --git a/mocks/mock_device.go b/mocks/mock_device.go index 8336065d..4a481858 100644 --- a/mocks/mock_device.go +++ b/mocks/mock_device.go @@ -19,6 +19,7 @@ import ( type MockDeviceUtils struct { ctrl *gomock.Controller recorder *MockDeviceUtilsMockRecorder + isgomock struct{} } // MockDeviceUtilsMockRecorder is the mock recorder for MockDeviceUtils. diff --git a/mocks/mock_filesystem.go b/mocks/mock_filesystem.go index bd02b93e..76b869fa 100644 --- a/mocks/mock_filesystem.go +++ b/mocks/mock_filesystem.go @@ -22,6 +22,7 @@ import ( type MockFileInterface struct { ctrl *gomock.Controller recorder *MockFileInterfaceMockRecorder + isgomock struct{} } // MockFileInterfaceMockRecorder is the mock recorder for MockFileInterface. @@ -89,6 +90,7 @@ func (mr *MockFileInterfaceMockRecorder) Write(arg0 any) *gomock.Call { type MockFileSystem struct { ctrl *gomock.Controller recorder *MockFileSystemMockRecorder + isgomock struct{} } // MockFileSystemMockRecorder is the mock recorder for MockFileSystem. diff --git a/mocks/mock_filesystemstatter.go b/mocks/mock_filesystemstatter.go new file mode 100644 index 00000000..ab46bc58 --- /dev/null +++ b/mocks/mock_filesystemstatter.go @@ -0,0 +1,55 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: pkg/filesystem-stats/filesystem_stats.go +// +// Generated by this command: +// +// mockgen -source=pkg/filesystem-stats/filesystem_stats.go -destination=mocks/mock_filesystemstatter.go -package=mocks +// + +// Package mocks is a generated GoMock package. +package mocks + +import ( + reflect "reflect" + + gomock "go.uber.org/mock/gomock" + unix "golang.org/x/sys/unix" +) + +// MockFilesystemStatter is a mock of FilesystemStatter interface. +type MockFilesystemStatter struct { + ctrl *gomock.Controller + recorder *MockFilesystemStatterMockRecorder + isgomock struct{} +} + +// MockFilesystemStatterMockRecorder is the mock recorder for MockFilesystemStatter. +type MockFilesystemStatterMockRecorder struct { + mock *MockFilesystemStatter +} + +// NewMockFilesystemStatter creates a new mock instance. +func NewMockFilesystemStatter(ctrl *gomock.Controller) *MockFilesystemStatter { + mock := &MockFilesystemStatter{ctrl: ctrl} + mock.recorder = &MockFilesystemStatterMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockFilesystemStatter) EXPECT() *MockFilesystemStatterMockRecorder { + return m.recorder +} + +// Statfs mocks base method. +func (m *MockFilesystemStatter) Statfs(path string, stat *unix.Statfs_t) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Statfs", path, stat) + ret0, _ := ret[0].(error) + return ret0 +} + +// Statfs indicates an expected call of Statfs. +func (mr *MockFilesystemStatterMockRecorder) Statfs(path, stat any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Statfs", reflect.TypeOf((*MockFilesystemStatter)(nil).Statfs), path, stat) +} diff --git a/mocks/mock_linodeclient.go b/mocks/mock_linodeclient.go index 1be3cd5d..33e8cbd5 100644 --- a/mocks/mock_linodeclient.go +++ b/mocks/mock_linodeclient.go @@ -21,6 +21,7 @@ import ( type MockLinodeClient struct { ctrl *gomock.Controller recorder *MockLinodeClientMockRecorder + isgomock struct{} } // MockLinodeClientMockRecorder is the mock recorder for MockLinodeClient. @@ -247,6 +248,21 @@ func (mr *MockLinodeClientMockRecorder) ResizeVolume(arg0, arg1, arg2 any) *gomo return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResizeVolume", reflect.TypeOf((*MockLinodeClient)(nil).ResizeVolume), arg0, arg1, arg2) } +// UpdateVolume mocks base method. +func (m *MockLinodeClient) UpdateVolume(arg0 context.Context, arg1 int, arg2 linodego.VolumeUpdateOptions) (*linodego.Volume, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "UpdateVolume", arg0, arg1, arg2) + ret0, _ := ret[0].(*linodego.Volume) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// UpdateVolume indicates an expected call of UpdateVolume. +func (mr *MockLinodeClientMockRecorder) UpdateVolume(arg0, arg1, arg2 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateVolume", reflect.TypeOf((*MockLinodeClient)(nil).UpdateVolume), arg0, arg1, arg2) +} + // WaitForVolumeLinodeID mocks base method. func (m *MockLinodeClient) WaitForVolumeLinodeID(arg0 context.Context, arg1 int, arg2 *int, arg3 int) (*linodego.Volume, error) { m.ctrl.T.Helper() diff --git a/mocks/mock_metadata.go b/mocks/mock_metadata.go index 4ff27fcf..ea71f8d1 100644 --- a/mocks/mock_metadata.go +++ b/mocks/mock_metadata.go @@ -22,6 +22,7 @@ import ( type MockMetadataClient struct { ctrl *gomock.Controller recorder *MockMetadataClientMockRecorder + isgomock struct{} } // MockMetadataClientMockRecorder is the mock recorder for MockMetadataClient. @@ -60,6 +61,7 @@ func (mr *MockMetadataClientMockRecorder) GetInstance(ctx any) *gomock.Call { type MockKubeClient struct { ctrl *gomock.Controller recorder *MockKubeClientMockRecorder + isgomock struct{} } // MockKubeClientMockRecorder is the mock recorder for MockKubeClient. diff --git a/pkg/filesystem-stats/filesystem_stats.go b/pkg/filesystem-stats/filesystem_stats.go new file mode 100644 index 00000000..b059c449 --- /dev/null +++ b/pkg/filesystem-stats/filesystem_stats.go @@ -0,0 +1,23 @@ +package filesystemstats + +import "golang.org/x/sys/unix" + +// FilesystemStatter provides an interface for getting filesystem statistics. +// This interface allows for easier mocking in tests. +type FilesystemStatter interface { + // Statfs returns filesystem statistics for the given path. + Statfs(path string, stat *unix.Statfs_t) error +} + +// UnixFilesystemStatter implements FilesystemStatter using the real unix.Statfs system call. +type UnixFilesystemStatter struct{} + +// Statfs calls the unix.Statfs system call. +func (u *UnixFilesystemStatter) Statfs(path string, stat *unix.Statfs_t) error { + return unix.Statfs(path, stat) +} + +// NewFilesystemStatter creates a new FilesystemStatter using the real unix.Statfs. +func NewFilesystemStatter() FilesystemStatter { + return &UnixFilesystemStatter{} +} diff --git a/pkg/linode-client/linode_client.go b/pkg/linode-client/linode_client.go index 5e850cd9..9f3c360a 100644 --- a/pkg/linode-client/linode_client.go +++ b/pkg/linode-client/linode_client.go @@ -27,6 +27,7 @@ type LinodeClient interface { DeleteVolume(context.Context, int) error ResizeVolume(context.Context, int, int) error + UpdateVolume(context.Context, int, linodego.VolumeUpdateOptions) (*linodego.Volume, error) NewEventPoller(context.Context, any, linodego.EntityType, linodego.EventAction) (*linodego.EventPoller, error) } diff --git a/tests/sanity/helpers_test.go b/tests/sanity/helpers_test.go new file mode 100644 index 00000000..7c409321 --- /dev/null +++ b/tests/sanity/helpers_test.go @@ -0,0 +1,35 @@ +// Copyright 2024 Linode LLC +// SPDX-License-Identifier: Apache-2.0 + +//go:build linux + +package sanity_test + +import ( + "io" + "os" +) + +func createDir(p string) (string, error) { + if err := os.MkdirAll(p, 0o755); err != nil { + return "", err + } + return p, nil +} + +// fakeCmd implements exec.Cmd for mock executor +type fakeCmd struct{} + +func (f *fakeCmd) CombinedOutput() ([]byte, error) { return nil, nil } +func (f *fakeCmd) Output() ([]byte, error) { return nil, nil } +func (f *fakeCmd) SetDir(dir string) {} +func (f *fakeCmd) SetStdin(in io.Reader) {} +func (f *fakeCmd) SetStdout(out io.Writer) {} +func (f *fakeCmd) SetStderr(out io.Writer) {} +func (f *fakeCmd) SetEnv(env []string) {} +func (f *fakeCmd) StdoutPipe() (io.ReadCloser, error) { return io.NopCloser(nil), nil } +func (f *fakeCmd) StderrPipe() (io.ReadCloser, error) { return io.NopCloser(nil), nil } +func (f *fakeCmd) Start() error { return nil } +func (f *fakeCmd) Wait() error { return nil } +func (f *fakeCmd) Run() error { return nil } +func (f *fakeCmd) Stop() {} diff --git a/tests/sanity/mock_linode_client_test.go b/tests/sanity/mock_linode_client_test.go new file mode 100644 index 00000000..3d480c9e --- /dev/null +++ b/tests/sanity/mock_linode_client_test.go @@ -0,0 +1,246 @@ +// Copyright 2024 Linode LLC +// SPDX-License-Identifier: Apache-2.0 + +//go:build linux + +package sanity_test + +import ( + "context" + "encoding/json" + "fmt" + + "github.com/linode/linodego" + "go.uber.org/mock/gomock" + + "github.com/linode/linode-blockstorage-csi-driver/mocks" +) + +// setupLinodeClientExpectations configures MockLinodeClient with stateful behavior +// +//nolint:gocognit // just for test setup +func setupLinodeClientExpectations(mock *mocks.MockLinodeClient, store *volumeStore) { + // CreateVolume + mock.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, opts linodego.VolumeCreateOptions) (*linodego.Volume, error) { + store.mu.Lock() + defer store.mu.Unlock() + + id := int(store.nextID.Add(1)) + vol := &linodego.Volume{ + ID: id, + Label: opts.Label, + Region: opts.Region, + Size: opts.Size, + Status: linodego.VolumeActive, + } + if opts.LinodeID != 0 { + vol.LinodeID = &opts.LinodeID + } + store.volumes[id] = vol + return vol, nil + }).AnyTimes() + + // GetVolume + mock.EXPECT().GetVolume(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, id int) (*linodego.Volume, error) { + store.mu.RLock() + defer store.mu.RUnlock() + + vol, ok := store.volumes[id] + if !ok { + return nil, &linodego.Error{Code: 404, Message: "volume not found"} + } + return vol, nil + }).AnyTimes() + + // ListVolumes - must respect filter for label + mock.EXPECT().ListVolumes(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, opts *linodego.ListOptions) ([]linodego.Volume, error) { + store.mu.RLock() + defer store.mu.RUnlock() + + vols := make([]linodego.Volume, 0, len(store.volumes)) + + // Parse filter to check for label + var labelFilter string + if opts != nil && opts.Filter != "" { + var filter map[string]string + if err := json.Unmarshal([]byte(opts.Filter), &filter); err == nil { + labelFilter = filter["label"] + } + } + + for _, v := range store.volumes { + // If a label filter is specified, only return matching volumes + if labelFilter != "" && v.Label != labelFilter { + continue + } + vols = append(vols, *v) + } + return vols, nil + }).AnyTimes() + + // DeleteVolume + mock.EXPECT().DeleteVolume(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, id int) error { + store.mu.Lock() + defer store.mu.Unlock() + + if _, ok := store.volumes[id]; !ok { + return &linodego.Error{Code: 404, Message: "volume not found"} + } + delete(store.volumes, id) + return nil + }).AnyTimes() + + // AttachVolume + mock.EXPECT().AttachVolume(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, volID int, opts *linodego.VolumeAttachOptions) (*linodego.Volume, error) { + store.mu.Lock() + defer store.mu.Unlock() + + vol, ok := store.volumes[volID] + if !ok { + return nil, &linodego.Error{Code: 404, Message: "volume not found"} + } + + // Check if volume is already attached to a different instance + if vol.LinodeID != nil && *vol.LinodeID != opts.LinodeID { + return nil, &linodego.Error{Code: 400, Message: "volume already attached to different instance"} + } + + // If already attached to this instance, this is idempotent - just return + if vol.LinodeID != nil && *vol.LinodeID == opts.LinodeID { + return vol, nil + } + + vol.LinodeID = &opts.LinodeID + vol.FilesystemPath = fmt.Sprintf("/dev/disk/by-id/scsi-0Linode_Volume_%s", vol.Label) + return vol, nil + }).AnyTimes() + + // DetachVolume + mock.EXPECT().DetachVolume(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, volID int) error { + store.mu.Lock() + defer store.mu.Unlock() + + vol, ok := store.volumes[volID] + if !ok { + return &linodego.Error{Code: 404, Message: "volume not found"} + } + vol.LinodeID = nil + return nil + }).AnyTimes() + + // ResizeVolume + mock.EXPECT().ResizeVolume(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, volID int, size int) error { + store.mu.Lock() + defer store.mu.Unlock() + + vol, ok := store.volumes[volID] + if !ok { + return &linodego.Error{Code: 404, Message: "volume not found"} + } + vol.Size = size + return nil + }).AnyTimes() + + // WaitForVolumeStatus + mock.EXPECT().WaitForVolumeStatus(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, volID int, _ linodego.VolumeStatus, _ int) (*linodego.Volume, error) { + store.mu.RLock() + defer store.mu.RUnlock() + + vol, ok := store.volumes[volID] + if !ok { + return nil, &linodego.Error{Code: 404, Message: "volume not found"} + } + return vol, nil + }).AnyTimes() + + // WaitForVolumeLinodeID + mock.EXPECT().WaitForVolumeLinodeID(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, volID int, _ *int, _ int) (*linodego.Volume, error) { + store.mu.RLock() + defer store.mu.RUnlock() + + vol, ok := store.volumes[volID] + if !ok { + return nil, &linodego.Error{Code: 404, Message: "volume not found"} + } + return vol, nil + }).AnyTimes() + + // GetInstance + mock.EXPECT().GetInstance(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, id int) (*linodego.Instance, error) { + if id == instanceID { + return store.instance, nil + } + return nil, &linodego.Error{Code: 404, Message: "instance not found"} + }).AnyTimes() + + // ListInstances + mock.EXPECT().ListInstances(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, _ *linodego.ListOptions) ([]linodego.Instance, error) { + return []linodego.Instance{*store.instance}, nil + }).AnyTimes() + + // ListInstanceVolumes + mock.EXPECT().ListInstanceVolumes(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, instID int, _ *linodego.ListOptions) ([]linodego.Volume, error) { + store.mu.RLock() + defer store.mu.RUnlock() + + vols := make([]linodego.Volume, 0) + for _, v := range store.volumes { + if v.LinodeID != nil && *v.LinodeID == instID { + vols = append(vols, *v) + } + } + return vols, nil + }).AnyTimes() + + // ListInstanceDisks + mock.EXPECT().ListInstanceDisks(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, _ int, _ *linodego.ListOptions) ([]linodego.InstanceDisk, error) { + return []linodego.InstanceDisk{ + {ID: 1, Label: "boot", Size: 25600}, + }, nil + }).AnyTimes() + + // CloneVolume + mock.EXPECT().CloneVolume(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, volID int, label string) (*linodego.Volume, error) { + store.mu.Lock() + defer store.mu.Unlock() + + srcVol, ok := store.volumes[volID] + if !ok { + return nil, &linodego.Error{Code: 404, Message: "volume not found"} + } + + id := int(store.nextID.Add(1)) + vol := &linodego.Volume{ + ID: id, + Label: label, + Region: srcVol.Region, + Size: srcVol.Size, + Status: linodego.VolumeActive, + } + store.volumes[id] = vol + return vol, nil + }).AnyTimes() + + // GetRegion + mock.EXPECT().GetRegion(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, regionID string) (*linodego.Region, error) { + return &linodego.Region{ID: regionID}, nil + }).AnyTimes() + + // NewEventPoller + mock.EXPECT().NewEventPoller(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() +} diff --git a/tests/sanity/mock_setup_test.go b/tests/sanity/mock_setup_test.go new file mode 100644 index 00000000..394d1380 --- /dev/null +++ b/tests/sanity/mock_setup_test.go @@ -0,0 +1,173 @@ +// Copyright 2024 Linode LLC +// SPDX-License-Identifier: Apache-2.0 + +//go:build linux + +package sanity_test + +import ( + "os" + "sync" + + "github.com/jaypipes/ghw" + "go.uber.org/mock/gomock" + "golang.org/x/sys/unix" + + "github.com/linode/linode-blockstorage-csi-driver/mocks" +) + +// setupMockExpectations configures all other mocks to accept any calls +func setupMockExpectations( + mockCtrl *gomock.Controller, + mockMounter *mocks.MockMounter, + mockExecutor *mocks.MockExecutor, + mockFormater *mocks.MockFormater, + mockDeviceUtils *mocks.MockDeviceUtils, + mockResizeFS *mocks.MockResizeFSer, + mockFileSystem *mocks.MockFileSystem, + mockCryptSetup *mocks.MockCryptSetupClient, + mockFsStatter *mocks.MockFilesystemStatter, +) { + // Track mounted paths for IsLikelyNotMountPoint and published paths for Stats + mountedPaths := make(map[string]bool) + publishedPaths := make(map[string]bool) + var mountMu sync.Mutex + + // Mounter expectations + mockMounter.EXPECT().Mount(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(source, target, fstype string, options []string) error { + mountMu.Lock() + defer mountMu.Unlock() + mountedPaths[target] = true + publishedPaths[target] = true + return nil + }).AnyTimes() + mockMounter.EXPECT().MountSensitive(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(source, target, fstype string, options, sensitiveOptions []string) error { + mountMu.Lock() + defer mountMu.Unlock() + mountedPaths[target] = true + return nil + }).AnyTimes() + mockMounter.EXPECT().MountSensitiveWithoutSystemd(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(source, target, fstype string, options, sensitiveOptions []string) error { + mountMu.Lock() + defer mountMu.Unlock() + mountedPaths[target] = true + return nil + }).AnyTimes() + mockMounter.EXPECT().MountSensitiveWithoutSystemdWithMountFlags(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(source, target, fstype string, options, sensitiveOptions, mountFlags []string) error { + mountMu.Lock() + defer mountMu.Unlock() + mountedPaths[target] = true + return nil + }).AnyTimes() + mockMounter.EXPECT().Unmount(gomock.Any()).DoAndReturn( + func(target string) error { + mountMu.Lock() + defer mountMu.Unlock() + delete(mountedPaths, target) + return nil + }).AnyTimes() + mockMounter.EXPECT().List().Return(nil, nil).AnyTimes() + mockMounter.EXPECT().IsMountPoint(gomock.Any()).DoAndReturn( + func(file string) (bool, error) { + mountMu.Lock() + defer mountMu.Unlock() + return mountedPaths[file], nil + }).AnyTimes() + mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Any()).DoAndReturn( + func(file string) (bool, error) { + mountMu.Lock() + defer mountMu.Unlock() + return !mountedPaths[file], nil + }).AnyTimes() + mockMounter.EXPECT().GetMountRefs(gomock.Any()).Return(nil, nil).AnyTimes() + mockMounter.EXPECT().CanSafelySkipMountPointCheck().Return(false).AnyTimes() + + // Executor expectations + mockExecutor.EXPECT().Command(gomock.Any(), gomock.Any()).Return(&fakeCmd{}).AnyTimes() + mockExecutor.EXPECT().CommandContext(gomock.Any(), gomock.Any(), gomock.Any()).Return(&fakeCmd{}).AnyTimes() + mockExecutor.EXPECT().LookPath(gomock.Any()).Return("", nil).AnyTimes() + + // Formater expectations + mockFormater.EXPECT().FormatAndMount(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + + // DeviceUtils expectations + mockDeviceUtils.EXPECT().GetDiskByIdPaths(gomock.Any(), gomock.Any()).Return([]string{"/dev/disk/by-id/test"}).AnyTimes() + mockDeviceUtils.EXPECT().VerifyDevicePath(gomock.Any()).Return("/dev/sda", nil).AnyTimes() + + // ResizeFS expectations + mockResizeFS.EXPECT().Resize(gomock.Any(), gomock.Any()).Return(true, nil).AnyTimes() + mockResizeFS.EXPECT().NeedResize(gomock.Any(), gomock.Any()).Return(false, nil).AnyTimes() + + // FileSystem expectations + mockFileSystem.EXPECT().IsNotExist(gomock.Any()).DoAndReturn(os.IsNotExist).AnyTimes() + mockFileSystem.EXPECT().MkdirAll(gomock.Any(), gomock.Any()).DoAndReturn(os.MkdirAll).AnyTimes() + mockFileSystem.EXPECT().Stat(gomock.Any()).DoAndReturn(os.Stat).AnyTimes() + mockFileSystem.EXPECT().Remove(gomock.Any()).Return(nil).AnyTimes() + mockFileSystem.EXPECT().OpenFile(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() + mockFileSystem.EXPECT().Open(gomock.Any()).Return(nil, nil).AnyTimes() + mockFileSystem.EXPECT().Glob(gomock.Any()).Return(nil, nil).AnyTimes() + mockFileSystem.EXPECT().EvalSymlinks(gomock.Any()).Return("", nil).AnyTimes() + + // CryptSetup expectations - create a mock device to return + mockDevice := mocks.NewMockDevice(mockCtrl) + mockDevice.EXPECT().Format(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + mockDevice.EXPECT().KeyslotAddByVolumeKey(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + mockDevice.EXPECT().ActivateByVolumeKey(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + mockDevice.EXPECT().ActivateByPassphrase(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + mockDevice.EXPECT().VolumeKeyGet(gomock.Any(), gomock.Any()).Return(nil, 0, nil).AnyTimes() + mockDevice.EXPECT().Load(gomock.Any()).Return(nil).AnyTimes() + mockDevice.EXPECT().Free().Return(true).AnyTimes() + mockDevice.EXPECT().Dump().Return(0).AnyTimes() + mockDevice.EXPECT().Type().Return("LUKS2").AnyTimes() + mockDevice.EXPECT().Deactivate(gomock.Any()).Return(nil).AnyTimes() + + mockCryptSetup.EXPECT().Init(gomock.Any()).Return(mockDevice, nil).AnyTimes() + mockCryptSetup.EXPECT().InitByName(gomock.Any()).Return(mockDevice, nil).AnyTimes() + + // FilesystemStatter expectations + mockFsStatter.EXPECT().Statfs(gomock.Any(), gomock.Any()).DoAndReturn( + func(path string, stat *unix.Statfs_t) error { + // Only return stats for paths that were actually published/mounted + // This ensures NodeGetVolumeStats fails for non-existent or unpublished volumes + mountMu.Lock() + isPublished := publishedPaths[path] + mountMu.Unlock() + + if !isPublished { + // Path was never published - return ENOENT + return unix.ENOENT + } + + // Return valid filesystem stats for published paths + stat.Blocks = 1000000 + stat.Bfree = 500000 + stat.Bavail = 450000 + stat.Files = 10000 + stat.Ffree = 5000 + stat.Bsize = 4096 + return nil + }).AnyTimes() +} + +// setupHardwareInfoExpectations configures MockHardwareInfo with stateful behavior +func setupHardwareInfoExpectations(mock *mocks.MockHardwareInfo) { + // Mock Block() to return 1 boot disk (QEMU vendor, SCSI controller) + // This simulates a real Linode instance with 1 boot disk + // With 4GB RAM, maxVolumeAttachments = 8, so 8 - 1 = 7 volumes can be attached + mock.EXPECT().Block().DoAndReturn( + func() (*ghw.BlockInfo, error) { + return &ghw.BlockInfo{ + Disks: []*ghw.Disk{ + { + Name: "sda", + Vendor: "QEMU", + StorageController: ghw.STORAGE_CONTROLLER_SCSI, + }, + }, + }, nil + }).AnyTimes() +} diff --git a/tests/sanity/sanity_test.go b/tests/sanity/sanity_test.go new file mode 100644 index 00000000..20f5543a --- /dev/null +++ b/tests/sanity/sanity_test.go @@ -0,0 +1,141 @@ +// Copyright 2024 Linode LLC +// SPDX-License-Identifier: Apache-2.0 + +//go:build linux + +package sanity_test + +import ( + "context" + "fmt" + "os" + "path/filepath" + "testing" + + sanity "github.com/kubernetes-csi/csi-test/v5/pkg/sanity" + "go.uber.org/mock/gomock" + "google.golang.org/grpc" + "google.golang.org/grpc/credentials/insecure" + "k8s.io/mount-utils" + + "github.com/linode/linode-blockstorage-csi-driver/internal/driver" + "github.com/linode/linode-blockstorage-csi-driver/mocks" + mountmanager "github.com/linode/linode-blockstorage-csi-driver/pkg/mount-manager" +) + +const ( + driverName = "linodebs.csi.linode.com" + vendorVersion = "test" + instanceID = 12345 + region = "us-east" + // Minimum volume size for Linode is 10 GiB + minVolumeSize = 10 << 30 +) + +func TestSanity(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Errorf("Test panicked: %v", r) + } + }() + + ctx := context.Background() + tmpDir := t.TempDir() + + endpoint := fmt.Sprintf("unix://%s/csi.sock", tmpDir) + targetPath := filepath.Join(tmpDir, "target") + stagingPath := filepath.Join(tmpDir, "staging") + + // Setup gomock controller + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + // Create volume store for stateful mock behavior + store := newVolumeStore() + + // Create all mocks + mockLinodeClient := mocks.NewMockLinodeClient(mockCtrl) + mockMounter := mocks.NewMockMounter(mockCtrl) + mockExecutor := mocks.NewMockExecutor(mockCtrl) + mockFormater := mocks.NewMockFormater(mockCtrl) + mockDeviceUtils := mocks.NewMockDeviceUtils(mockCtrl) + mockResizeFS := mocks.NewMockResizeFSer(mockCtrl) + mockFileSystem := mocks.NewMockFileSystem(mockCtrl) + mockCryptSetup := mocks.NewMockCryptSetupClient(mockCtrl) + mockFsStatter := mocks.NewMockFilesystemStatter(mockCtrl) + + mockHwInfo := mocks.NewMockHardwareInfo(mockCtrl) + + // Setup mock expectations + setupLinodeClientExpectations(mockLinodeClient, store) + setupMockExpectations(mockCtrl, mockMounter, mockExecutor, mockFormater, mockDeviceUtils, mockResizeFS, mockFileSystem, mockCryptSetup, mockFsStatter) + setupHardwareInfoExpectations(mockHwInfo) + + // Create SafeFormatAndMount with mocks + mounter := &mountmanager.SafeFormatAndMount{ + SafeFormatAndMount: &mount.SafeFormatAndMount{ + Interface: mockMounter, + Exec: mockExecutor, + }, + Formater: mockFormater, + } + + // Create encryption with mocks + encryption := driver.NewLuksEncryption(mockExecutor, mockFileSystem, mockCryptSetup) + + // Setup metadata + metadata := driver.Metadata{ + ID: instanceID, + Label: fmt.Sprintf("linode%d", instanceID), + Region: region, + Memory: 4 << 30, // 4 GiB + } + + // Create and setup the driver + linodeDriver := driver.GetLinodeDriver(ctx) + + // Setup driver + err := linodeDriver.SetupLinodeDriver( + ctx, + mockLinodeClient, + mounter, + mockDeviceUtils, + mockResizeFS, + metadata, + driverName, + vendorVersion, + "", + encryption, + "false", // enableMetrics + "", + "false", // enableTracing + "", + mockFsStatter, + mockHwInfo, + ) + if err != nil { + t.Fatalf("Failed to setup driver: %v", err) + } + + // Start the driver + go linodeDriver.Run(ctx, endpoint) + + // Configure sanity tests + config := sanity.TestConfig{ + TargetPath: targetPath, + StagingPath: stagingPath, + Address: endpoint, + DialOptions: []grpc.DialOption{grpc.WithTransportCredentials(insecure.NewCredentials())}, + IDGen: sanity.DefaultIDGenerator{}, + TestVolumeSize: minVolumeSize, + TestVolumeAccessType: "mount", + CreateTargetDir: createDir, + CreateStagingDir: createDir, + RemoveTargetPath: os.RemoveAll, + RemoveStagingPath: os.RemoveAll, + TestNodeVolumeAttachLimit: true, + } + + // Run sanity tests + sanity.Test(t, config) +} diff --git a/tests/sanity/volume_store_test.go b/tests/sanity/volume_store_test.go new file mode 100644 index 00000000..31a4dc6d --- /dev/null +++ b/tests/sanity/volume_store_test.go @@ -0,0 +1,39 @@ +// Copyright 2024 Linode LLC +// SPDX-License-Identifier: Apache-2.0 + +//go:build linux + +package sanity_test + +import ( + "sync" + "sync/atomic" + + "github.com/linode/linodego" +) + +// volumeStore provides stateful storage for mock LinodeClient +type volumeStore struct { + mu sync.RWMutex + volumes map[int]*linodego.Volume + publishedCapTypes map[int]string // Track capability type per volume (mount/block) - currently unused + nextID atomic.Int64 + instance *linodego.Instance +} + +func newVolumeStore() *volumeStore { + vs := &volumeStore{ + volumes: make(map[int]*linodego.Volume), + publishedCapTypes: make(map[int]string), + instance: &linodego.Instance{ + ID: instanceID, + Label: "linode12345", + Specs: &linodego.InstanceSpec{ + Memory: 4096, // 4 GiB in MB + }, + Region: region, + }, + } + vs.nextID.Store(1000) + return vs +}