From e2733992d808db1b85b501cd43ef8287c139c8f5 Mon Sep 17 00:00:00 2001 From: Amol Deodhar Date: Fri, 20 Sep 2024 16:03:41 -0400 Subject: [PATCH 1/2] golangci-lint: update config file and make relevant changes to the code --- .golanci.yml => .golangci.yml | 42 ++--- Dockerfile | 2 +- Dockerfile.dev | 4 + Makefile | 35 +++-- internal/driver/controllerserver.go | 144 +++++++++--------- internal/driver/controllerserver_helper.go | 68 +++++---- internal/driver/controllerserver_test.go | 34 +++-- internal/driver/driver.go | 20 +-- internal/driver/driver_test.go | 6 +- internal/driver/identityserver.go | 3 +- internal/driver/luks.go | 75 +++++---- internal/driver/metadata.go | 29 ++-- internal/driver/nodeserver.go | 23 +-- internal/driver/nodeserver_all.go | 15 +- internal/driver/nodeserver_helpers.go | 39 +++-- .../driver/nodeserver_helpers_linux_test.go | 4 +- internal/driver/nodeserver_helpers_test.go | 12 +- .../driver/nodeserver_luks_encryption_test.go | 8 +- internal/driver/nodeserver_test.go | 5 +- internal/driver/server.go | 25 ++- main.go | 32 ++-- pkg/linode-client/linode-client.go | 14 +- pkg/linode-volumes/utils.go | 12 +- pkg/mount-manager/device-utils.go | 24 +-- pkg/mount-manager/fs-utils.go | 1 + 25 files changed, 363 insertions(+), 313 deletions(-) rename .golanci.yml => .golangci.yml (76%) diff --git a/.golanci.yml b/.golangci.yml similarity index 76% rename from .golanci.yml rename to .golangci.yml index b13b9daf..d39d7b34 100644 --- a/.golanci.yml +++ b/.golangci.yml @@ -3,7 +3,8 @@ run: issues-exit-code: 1 output: - formats: colored-line-number + formats: + - format: colored-line-number linters-settings: errcheck: @@ -16,22 +17,22 @@ linters-settings: - default - blank - dot - - prefix(github.com/linode) + - prefix(github.com/linode/linode-blockstorage-csi-driver) govet: - check-shadowing: false + enable: + - shadow - golint: - min-confidence: 0.8 + gosec: + confidence: "medium" + excludes: + - G115 gofmt: simplify: true goimports: - local-prefixes: github.com/linode/ - - maligned: - suggest-new: true + local-prefixes: github.com/linode/linode-blockstorage-csi-driver dupl: threshold: 100 @@ -70,10 +71,10 @@ linters: enable: - asasalint - asciicheck - - bidicheck - bodyclose - containedctx - contextcheck + - copyloopvar - decorder - dogsled - dupl @@ -83,9 +84,7 @@ linters: - errname - errorlint - errcheck - - exportloopref - exhaustive - - exportloopref - forbidigo - forcetypeassert - gci @@ -103,13 +102,11 @@ linters: - maintidx - makezero - misspell - - mnd - nestif - nilerr - nilnil - noctx - nolintlint - - parlalleltest - prealloc - predeclared - reassign @@ -131,11 +128,23 @@ linters: issues: exclude-rules: - - path: _test\.go + # Exclude some linters from running on tests files. + - path: _test(ing)?\.go linters: + - gocyclo - maintidx + - errcheck - dupl - - exportloopref + - gosec + - copyloopvar + - unparam + - varnamelen + + # Ease some gocritic warnings on test files. + - path: _test\.go + text: "(unnamedResult|exitAfterDefer)" + linters: + - gocritic - text: "G101:" linters: @@ -149,7 +158,6 @@ issues: exclude-use-default: false new: false - max-per-linter: 0 max-same-issues: 0 exclude-files: - "zz_generated\\..+\\.go$" diff --git a/Dockerfile b/Dockerfile index 21f92d3c..b5d097f0 100644 --- a/Dockerfile +++ b/Dockerfile @@ -22,6 +22,6 @@ LABEL description="Linode CSI Driver" COPY --from=builder /bin/linode-blockstorage-csi-driver /linode -RUN apk add --no-cache e2fsprogs findmnt blkid cryptsetup xfsprogs lsblk +RUN apk add --no-cache e2fsprogs findmnt blkid cryptsetup xfsprogs COPY --from=builder /bin/linode-blockstorage-csi-driver /linode ENTRYPOINT ["/linode"] diff --git a/Dockerfile.dev b/Dockerfile.dev index 8f37882f..1612c75e 100644 --- a/Dockerfile.dev +++ b/Dockerfile.dev @@ -1,6 +1,7 @@ FROM golang:1.23.1-alpine AS builder # from makefile ARG REV +ARG GOLANGCI_LINT_VERSION RUN mkdir -p /linode WORKDIR /linode @@ -10,6 +11,7 @@ RUN apk add \ cryptsetup \ cryptsetup-libs \ cryptsetup-dev \ + curl \ e2fsprogs \ findmnt \ gcc \ @@ -26,4 +28,6 @@ COPY . . RUN CGO_ENABLED=1 go build -a -ldflags '-X main.vendorVersion='${REV}'' -o /bin/linode-blockstorage-csi-driver /linode RUN CGO_ENABLED=1 go install go.uber.org/mock/mockgen@latest +RUN curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin ${GOLANGCI_LINT_VERSION} + CMD ["sh"] diff --git a/Makefile b/Makefile index 39e9705d..b454e96c 100644 --- a/Makefile +++ b/Makefile @@ -1,17 +1,18 @@ -PLATFORM ?= linux/amd64 -REGISTRY_NAME ?= index.docker.io -DOCKER_USER ?= linode -IMAGE_NAME ?= linode-blockstorage-csi-driver -REV := $(shell git branch --show-current 2> /dev/null || echo "dev") +PLATFORM ?= linux/amd64 +REGISTRY_NAME ?= index.docker.io +DOCKER_USER ?= linode +IMAGE_NAME ?= linode-blockstorage-csi-driver +REV := $(shell git branch --show-current 2> /dev/null || echo "dev") ifdef DEV_TAG_EXTENSION -IMAGE_VERSION ?= $(REV)-$(DEV_TAG_EXTENSION) +IMAGE_VERSION ?= $(REV)-$(DEV_TAG_EXTENSION) else -IMAGE_VERSION ?= $(REV) +IMAGE_VERSION ?= $(REV) endif -IMAGE_TAG ?= $(REGISTRY_NAME)/$(DOCKER_USER)/$(IMAGE_NAME):$(IMAGE_VERSION) -GOLANGCI_LINT_IMG := golangci/golangci-lint:v1.59-alpine -RELEASE_DIR ?= release -DOCKERFILE ?= Dockerfile +IMAGE_TAG ?= $(REGISTRY_NAME)/$(DOCKER_USER)/$(IMAGE_NAME):$(IMAGE_VERSION) +GOLANGCI_LINT_IMG := golangci/golangci-lint:v1.59-alpine +RELEASE_DIR ?= release +DOCKERFILE ?= Dockerfile +GOLANGCI_LINT_VERSION ?= v1.61.0 ##################################################################### # OS / ARCH @@ -30,15 +31,15 @@ endif ##################################################################### .PHONY: fmt fmt: - docker run --rm --platform=$(PLATFORM) -it $(IMAGE_TAG) go fmt ./... + docker run --rm -w /workdir -v $(PWD):/workdir --platform=$(PLATFORM) -it $(IMAGE_TAG) go fmt ./... .PHONY: vet vet: fmt - docker run --rm --platform=$(PLATFORM) -it $(IMAGE_TAG) go vet ./... + docker run --rm -w /workdir -v $(PWD):/workdir --platform=$(PLATFORM) -it $(IMAGE_TAG) go vet ./... .PHONY: lint lint: vet - docker run --rm --platform=$(PLATFORM) --rm -v $(PWD):/app -w /app ${GOLANGCI_LINT_IMG} golangci-lint run -v + docker run --rm -w /workdir -v $(PWD):/workdir --platform=$(PLATFORM) -it $(IMAGE_TAG) golangci-lint run -v -c .golangci.yml --fix .PHONY: verify verify: @@ -69,7 +70,11 @@ build: .PHONY: docker-build docker-build: - DOCKER_BUILDKIT=1 docker build --platform=$(PLATFORM) --progress=plain -t $(IMAGE_TAG) --build-arg REV=$(IMAGE_VERSION) -f ./$(DOCKERFILE) . + DOCKER_BUILDKIT=1 docker build --platform=$(PLATFORM) --progress=plain \ + -t $(IMAGE_TAG) \ + --build-arg REV=$(IMAGE_VERSION) \ + --build-arg GOLANGCI_LINT_VERSION=$(GOLANGCI_LINT_VERSION) \ + -f ./$(DOCKERFILE) . .PHONY: docker-push docker-push: diff --git a/internal/driver/controllerserver.go b/internal/driver/controllerserver.go index c89bf0a0..8b36fc6d 100644 --- a/internal/driver/controllerserver.go +++ b/internal/driver/controllerserver.go @@ -16,6 +16,8 @@ import ( "github.com/linode/linode-blockstorage-csi-driver/pkg/logger" ) +const True = "true" + type ControllerServer struct { driver *LinodeDriver client linodeclient.LinodeClient @@ -54,17 +56,17 @@ func NewControllerServer(ctx context.Context, driver *LinodeDriver, client linod return cs, nil } -// CreateVolume provisions a new volume on behalf of a user, which can be used as a block device or mounted filesystem. -// This operation is idempotent, meaning multiple calls with the same parameters will not create duplicate volumes. +// CreateVolume provisions a new volume on behalf of a user, which can be used as a block device or mounted filesystem. +// This operation is idempotent, meaning multiple calls with the same parameters will not create duplicate volumes. // For more details, refer to the CSI Driver Spec documentation. func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) (*csi.CreateVolumeResponse, error) { - log, ctx, done := logger.GetLogger(ctx).WithMethod("CreateVolume") + log, _, done := logger.GetLogger(ctx).WithMethod("CreateVolume") defer done() name := req.GetName() log.V(2).Info("Processing request", "req", req) - if len(name) == 0 { + if name == "" { return &csi.CreateVolumeResponse{}, errNoVolumeName } @@ -88,7 +90,7 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol // this is still problematic because we strip "-" from volume-name-prefixes // that specifically requested "-". // Don't strip this when volume labels support sufficient length - condensedName := strings.Replace(name, "-", "", -1) + condensedName := strings.ReplaceAll(name, "-", "") preKey := linodevolumes.CreateLinodeVolumeKey(0, condensedName) @@ -98,12 +100,12 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol log.V(4).Info("CreateVolume details", "storage_size_giga_bytes", targetSizeGB, "volume_name", volumeName) volumeContext := make(map[string]string) - if req.Parameters[LuksEncryptedAttribute] == "true" { + if req.GetParameters()[LuksEncryptedAttribute] == True { // if luks encryption is enabled add a volume context - volumeContext[LuksEncryptedAttribute] = "true" + volumeContext[LuksEncryptedAttribute] = True volumeContext[PublishInfoVolumeName] = volumeName - volumeContext[LuksCipherAttribute] = req.Parameters[LuksCipherAttribute] - volumeContext[LuksKeySizeAttribute] = req.Parameters[LuksKeySizeAttribute] + volumeContext[LuksCipherAttribute] = req.GetParameters()[LuksCipherAttribute] + volumeContext[LuksKeySizeAttribute] = req.GetParameters()[LuksKeySizeAttribute] } // Attempt to get info about the source volume for @@ -122,7 +124,7 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol ctx, volumeName, targetSizeGB, - req.Parameters[VolumeTags], + req.GetParameters()[VolumeTags], sourceVolumeInfo, ) if err != nil { @@ -131,9 +133,7 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol // If the existing volume size differs from the requested size, we throw an error. if vol.Size != targetSizeGB { - if sourceVolumeInfo == nil { - return nil, errAlreadyExists("volume %d already exists of size %d", vol.ID, vol.Size) - } + return nil, errAlreadyExists("volume %d already exists of size %d", vol.ID, vol.Size) } statusPollTimeout := waitTimeout() @@ -188,7 +188,7 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol // function will return a success response without any error. // For more details, refer to the CSI Driver Spec documentation. func (cs *ControllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest) (*csi.DeleteVolumeResponse, error) { - log, ctx, done := logger.GetLogger(ctx).WithMethod("DeleteVolume") + log, _, done := logger.GetLogger(ctx).WithMethod("DeleteVolume") defer done() volID, statusErr := linodevolumes.VolumeIdAsInt("DeleteVolume", req) @@ -220,40 +220,40 @@ func (cs *ControllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVol return &csi.DeleteVolumeResponse{}, nil } -// ControllerPublishVolume attaches a volume to a specified node. -// It ensures the volume is not already attached to another node -// and that the node can accommodate the attachment. Returns +// ControllerPublishVolume attaches a volume to a specified node. +// It ensures the volume is not already attached to another node +// and that the node can accommodate the attachment. Returns // the device path if successful. // For more details, refer to the CSI Driver Spec documentation. -func (cs *ControllerServer) ControllerPublishVolume(ctx context.Context, req *csi.ControllerPublishVolumeRequest) (*csi.ControllerPublishVolumeResponse, error) { - log, ctx, done := logger.GetLogger(ctx).WithMethod("ControllerPublishVolume") +func (cs *ControllerServer) ControllerPublishVolume(ctx context.Context, req *csi.ControllerPublishVolumeRequest) (resp *csi.ControllerPublishVolumeResponse, err error) { + log, _, done := logger.GetLogger(ctx).WithMethod("ControllerPublishVolume") defer done() log.V(2).Info("Processing request", "req", req) - linodeID, statusErr := linodevolumes.NodeIdAsInt("ControllerPublishVolume", req) - if statusErr != nil { - return &csi.ControllerPublishVolumeResponse{}, statusErr + linodeID, err := linodevolumes.NodeIdAsInt("ControllerPublishVolume", req) + if err != nil { + return resp, err } volumeID, statusErr := linodevolumes.VolumeIdAsInt("ControllerPublishVolume", req) if statusErr != nil { - return &csi.ControllerPublishVolumeResponse{}, statusErr + return resp, err } - cap := req.GetVolumeCapability() - if cap == nil { - return &csi.ControllerPublishVolumeResponse{}, errNoVolumeCapability + volCap := req.GetVolumeCapability() + if volCap == nil { + return resp, errNoVolumeCapability } - if !validVolumeCapabilities([]*csi.VolumeCapability{cap}) { - return &csi.ControllerPublishVolumeResponse{}, errInvalidVolumeCapability([]*csi.VolumeCapability{cap}) + if !validVolumeCapabilities([]*csi.VolumeCapability{volCap}) { + return resp, errInvalidVolumeCapability([]*csi.VolumeCapability{volCap}) } volume, err := cs.client.GetVolume(ctx, volumeID) if linodego.IsNotFound(err) { - return &csi.ControllerPublishVolumeResponse{}, errVolumeNotFound(volumeID) + return resp, errVolumeNotFound(volumeID) } else if err != nil { - return &csi.ControllerPublishVolumeResponse{}, errInternal("get volume %d: %v", volumeID, err) + return resp, errInternal("get volume %d: %v", volumeID, err) } if volume.LinodeID != nil { if *volume.LinodeID == linodeID { @@ -261,34 +261,37 @@ func (cs *ControllerServer) ControllerPublishVolume(ctx context.Context, req *cs pvInfo := map[string]string{ devicePathKey: volume.FilesystemPath, } - return &csi.ControllerPublishVolumeResponse{ + resp = &csi.ControllerPublishVolumeResponse{ PublishContext: pvInfo, - }, nil + } + return resp, nil } - return &csi.ControllerPublishVolumeResponse{}, errVolumeAttached(volumeID, linodeID) + return resp, errVolumeAttached(volumeID, linodeID) } instance, err := cs.client.GetInstance(ctx, linodeID) if linodego.IsNotFound(err) { - return &csi.ControllerPublishVolumeResponse{}, errInstanceNotFound(linodeID) + return resp, errInstanceNotFound(linodeID) } else if err != nil { - return &csi.ControllerPublishVolumeResponse{}, errInternal("get linode instance %d: %v", linodeID, err) + return resp, errInternal("get linode instance %d: %v", linodeID, err) } log.V(4).Info("Checking if volume can be attached", "volume_id", volumeID, "node_id", linodeID) // Check to see if there is room to attach this volume to the instance. - if canAttach, err := cs.canAttach(ctx, instance); err != nil { - return &csi.ControllerPublishVolumeResponse{}, err + var canAttach bool + if canAttach, err = cs.canAttach(ctx, instance); err != nil { + return resp, err } else if !canAttach { // If we can, try and add a little more information to the error message // for the caller. - limit, err := cs.maxAllowedVolumeAttachments(ctx, instance) + var limit int + limit, err = cs.maxAllowedVolumeAttachments(ctx, instance) if errors.Is(err, errNilInstance) { - return &csi.ControllerPublishVolumeResponse{}, errInternal("cannot calculate max volume attachments for a nil instance") + return resp, errInternal("cannot calculate max volume attachments for a nil instance") } else if err != nil { - return &csi.ControllerPublishVolumeResponse{}, errMaxAttachments + return resp, errMaxAttachments } - return &csi.ControllerPublishVolumeResponse{}, errMaxVolumeAttachments(limit) + return resp, errMaxVolumeAttachments(limit) } // Whether or not the volume attachment should be persisted across @@ -301,21 +304,22 @@ func (cs *ControllerServer) ControllerPublishVolume(ctx context.Context, req *cs persist := false log.V(4).Info("Executing attach volume", "volume_id", volumeID, "node_id", linodeID) - if _, err := cs.client.AttachVolume(ctx, volumeID, &linodego.VolumeAttachOptions{ + if _, err = cs.client.AttachVolume(ctx, volumeID, &linodego.VolumeAttachOptions{ LinodeID: linodeID, PersistAcrossBoots: &persist, }); err != nil { code := codes.Internal - if apiErr, ok := err.(*linodego.Error); ok && strings.Contains(apiErr.Message, "is already attached") { + var apiErr *linodego.Error + if errors.As(err, &apiErr) && strings.Contains(apiErr.Message, "is already attached") { code = codes.Unavailable // Allow a retry if the volume is already attached: race condition can occur here } - return &csi.ControllerPublishVolumeResponse{}, status.Errorf(code, "attach volume: %v", err) + return resp, status.Errorf(code, "attach volume: %v", err) } log.V(4).Info("Waiting for volume to attach", "volume_id", volumeID) volume, err = cs.client.WaitForVolumeLinodeID(ctx, volumeID, &linodeID, waitTimeout()) if err != nil { - return &csi.ControllerPublishVolumeResponse{}, errInternal("wait for volume to attach: %v", err) + return resp, errInternal("wait for volume to attach: %v", err) } log.V(2).Info("Volume attached successfully", "volume_id", volume.ID, "node_id", *volume.LinodeID, "device_path", volume.FilesystemPath) @@ -323,9 +327,10 @@ func (cs *ControllerServer) ControllerPublishVolume(ctx context.Context, req *cs pvInfo := map[string]string{ devicePathKey: volume.FilesystemPath, } - return &csi.ControllerPublishVolumeResponse{ + resp = &csi.ControllerPublishVolumeResponse{ PublishContext: pvInfo, - }, nil + } + return resp, nil } // ControllerUnpublishVolume detaches a specified volume from a node. @@ -336,7 +341,7 @@ func (cs *ControllerServer) ControllerPublishVolume(ctx context.Context, req *cs // return a successful response without error. // For more details, refer to the CSI Driver Spec documentation. func (cs *ControllerServer) ControllerUnpublishVolume(ctx context.Context, req *csi.ControllerUnpublishVolumeRequest) (*csi.ControllerUnpublishVolumeResponse, error) { - log, ctx, done := logger.GetLogger(ctx).WithMethod("ControllerUnpublishVolume") + log, _, done := logger.GetLogger(ctx).WithMethod("ControllerUnpublishVolume") defer done() log.V(2).Info("Processing request", "req", req) @@ -384,7 +389,7 @@ func (cs *ControllerServer) ControllerUnpublishVolume(ctx context.Context, req * // It returns a confirmation response if the capabilities are valid, or an error if the volume does not exist // or if no capabilities were provided. Refer to the @CSI Driver Spec for more details. func (cs *ControllerServer) ValidateVolumeCapabilities(ctx context.Context, req *csi.ValidateVolumeCapabilitiesRequest) (*csi.ValidateVolumeCapabilitiesResponse, error) { - log, ctx, done := logger.GetLogger(ctx).WithMethod("ValidateVolumeCapabilities") + log, _, done := logger.GetLogger(ctx).WithMethod("ValidateVolumeCapabilities") defer done() log.V(2).Info("Processing request", "req", req) @@ -420,7 +425,7 @@ func (cs *ControllerServer) ValidateVolumeCapabilities(ctx context.Context, req // parameters as specified in the CSI Driver Spec. // For more details, refer to the CSI Driver Spec documentation. func (cs *ControllerServer) ListVolumes(ctx context.Context, req *csi.ListVolumesRequest) (*csi.ListVolumesResponse, error) { - log, ctx, done := logger.GetLogger(ctx).WithMethod("ListVolumes") + log, _, done := logger.GetLogger(ctx).WithMethod("ListVolumes") defer done() log.V(2).Info("Processing request", "req", req) @@ -436,7 +441,7 @@ func (cs *ControllerServer) ListVolumes(ctx context.Context, req *csi.ListVolume if startingToken != "" { startingPage, errParse := strconv.ParseInt(startingToken, 10, 64) if errParse != nil { - return &csi.ListVolumesResponse{}, status.Errorf(codes.Aborted, + return &csi.ListVolumesResponse{}, status.Errorf(codes.Aborted, "invalid starting token: %q", startingToken) } @@ -452,8 +457,8 @@ func (cs *ControllerServer) ListVolumes(ctx context.Context, req *csi.ListVolume } entries := make([]*csi.ListVolumesResponse_Entry, 0, len(volumes)) - for _, vol := range volumes { - key := linodevolumes.CreateLinodeVolumeKey(vol.ID, vol.Label) + for volumeNum := range volumes { + key := linodevolumes.CreateLinodeVolumeKey(volumes[volumeNum].ID, volumes[volumeNum].Label) // If the volume is attached to a Linode instance, add it to the // list. Note that in the Linode API, volumes can only be @@ -463,18 +468,18 @@ func (cs *ControllerServer) ListVolumes(ctx context.Context, req *csi.ListVolume // make(), since the CSI specification says this response field // is optional, and thus it should tolerate a nil slice. var publishedNodeIDs []string - if vol.LinodeID != nil { - publishedNodeIDs = append(publishedNodeIDs, strconv.Itoa(*vol.LinodeID)) + if volumes[volumeNum].LinodeID != nil { + publishedNodeIDs = append(publishedNodeIDs, strconv.Itoa(*volumes[volumeNum].LinodeID)) } entries = append(entries, &csi.ListVolumesResponse_Entry{ Volume: &csi.Volume{ VolumeId: key.GetVolumeKey(), - CapacityBytes: gbToBytes(vol.Size), + CapacityBytes: gbToBytes(volumes[volumeNum].Size), AccessibleTopology: []*csi.Topology{ { Segments: map[string]string{ - VolumeTopologyRegion: vol.Region, + VolumeTopologyRegion: volumes[volumeNum].Region, }, }, }, @@ -497,8 +502,8 @@ func (cs *ControllerServer) ListVolumes(ctx context.Context, req *csi.ListVolume return resp, nil } -// ControllerGetCapabilities retrieves the capabilities supported by the -// controller service implemented by this Plugin. It returns a response +// ControllerGetCapabilities retrieves the capabilities supported by the +// controller service implemented by this Plugin. It returns a response // containing the capabilities available for the CSI driver. // For more details, refer to the CSI Driver Spec documentation. func (cs *ControllerServer) ControllerGetCapabilities(ctx context.Context, req *csi.ControllerGetCapabilitiesRequest) (*csi.ControllerGetCapabilitiesResponse, error) { @@ -520,8 +525,8 @@ func (cs *ControllerServer) ControllerGetCapabilities(ctx context.Context, req * // and performs the resize operation. If the volume is successfully resized, // it returns the new capacity and indicates that no node expansion is required. // For more details, refer to the CSI Driver Spec documentation. -func (cs *ControllerServer) ControllerExpandVolume(ctx context.Context, req *csi.ControllerExpandVolumeRequest) (*csi.ControllerExpandVolumeResponse, error) { - log, ctx, done := logger.GetLogger(ctx).WithMethod("ControllerExpandVolume") +func (cs *ControllerServer) ControllerExpandVolume(ctx context.Context, req *csi.ControllerExpandVolumeRequest) (resp *csi.ControllerExpandVolumeResponse, err error) { + log, _, done := logger.GetLogger(ctx).WithMethod("ControllerExpandVolume") defer done() log.V(2).Info("Processing request", "req", req) @@ -533,38 +538,39 @@ func (cs *ControllerServer) ControllerExpandVolume(ctx context.Context, req *csi size, err := getRequestCapacitySize(req.GetCapacityRange()) if err != nil { - return &csi.ControllerExpandVolumeResponse{}, errInternal("get requested size from capacity range: %v", err) + return resp, errInternal("get requested size from capacity range: %v", err) } // Get the volume log.V(4).Info("Checking if volume exists", "volume_id", volumeID) vol, err := cs.client.GetVolume(ctx, volumeID) if err != nil { - return &csi.ControllerExpandVolumeResponse{}, errInternal("get volume: %v", err) + return resp, errInternal("get volume: %v", err) } // Is the caller trying to resize the volume to be smaller than it currently is? if vol.Size > bytesToGB(size) { - return &csi.ControllerExpandVolumeResponse{}, errResizeDown + return resp, errResizeDown } // Resize the volume log.V(4).Info("Calling API to resize volume", "volume_id", volumeID) - if err := cs.client.ResizeVolume(ctx, volumeID, bytesToGB(size)); err != nil { - return &csi.ControllerExpandVolumeResponse{}, errInternal("resize volume %d: %v", volumeID, err) + if err = cs.client.ResizeVolume(ctx, volumeID, bytesToGB(size)); err != nil { + return resp, errInternal("resize volume %d: %v", volumeID, err) } // Wait for the volume to become active log.V(4).Info("Waiting for volume to become active", "volume_id", volumeID) vol, err = cs.client.WaitForVolumeStatus(ctx, vol.ID, linodego.VolumeActive, waitTimeout()) if err != nil { - return &csi.ControllerExpandVolumeResponse{}, errInternal("timed out waiting for volume %d to become active: %v", volumeID, err) + return resp, errInternal("timed out waiting for volume %d to become active: %v", volumeID, err) } log.V(4).Info("Volume active", "vol", vol) log.V(2).Info("Volume resized successfully", "volume_id", volumeID) - return &csi.ControllerExpandVolumeResponse{ + resp = &csi.ControllerExpandVolumeResponse{ CapacityBytes: size, NodeExpansionRequired: false, - }, nil + } + return resp, nil } diff --git a/internal/driver/controllerserver_helper.go b/internal/driver/controllerserver_helper.go index deaea670..a6455110 100644 --- a/internal/driver/controllerserver_helper.go +++ b/internal/driver/controllerserver_helper.go @@ -9,9 +9,10 @@ import ( "time" "github.com/container-storage-interface/spec/lib/go/csi" + "github.com/linode/linodego" + linodevolumes "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-volumes" "github.com/linode/linode-blockstorage-csi-driver/pkg/logger" - "github.com/linode/linodego" ) // MinVolumeSizeBytes is the smallest allowed size for a Linode block storage @@ -23,6 +24,9 @@ import ( // convert to and from "GB" when interacting with the Linode API. const MinVolumeSizeBytes = 10 << 30 // 10GiB +// Set sentinel error +var ErrNotFound = errors.New("not found") + // bytesToGB is a convenience function that converts the given number of bytes // to gigabytes. // This function should be used when converting a CSI RPC type's capacity range @@ -130,7 +134,7 @@ func (cs *ControllerServer) getContentSourceVolume(ctx context.Context, contentS log.V(4).Info("Attempting to get content source volume") if contentSource == nil { - return nil, nil // Return nil if no content source is provided + return nil, ErrNotFound } // Check if the content source type is a volume @@ -252,10 +256,10 @@ func (cs *ControllerServer) cloneLinodeVolume(ctx context.Context, label string, // It returns the minimum size if no range is provided, or the required size if specified. // It ensures that the size is not negative and does not exceed the maximum limit. func getRequestCapacitySize(capRange *csi.CapacityRange) (int64, error) { - // If no capacity range is provided, return the minimum volume size - if capRange == nil { - return MinVolumeSizeBytes, nil - } + // If no capacity range is provided, return the minimum volume size + if capRange == nil { + return MinVolumeSizeBytes, nil + } // Volume MUST be at least this big. This field is OPTIONAL. reqSize := capRange.GetRequiredBytes() @@ -263,47 +267,47 @@ func getRequestCapacitySize(capRange *csi.CapacityRange) (int64, error) { // Volume MUST not be bigger than this. This field is OPTIONAL. maxSize := capRange.GetLimitBytes() - // Validate that at least one size is specified - if reqSize == 0 && maxSize == 0 { - return 0, errors.New("either RequiredBytes or LimitBytes must be set") - } + // Validate that at least one size is specified + if reqSize == 0 && maxSize == 0 { + return 0, errors.New("either RequiredBytes or LimitBytes must be set") + } - // Check for negative values - if reqSize < 0 || maxSize < 0 { - return 0, errors.New("RequiredBytes and LimitBytes must not be negative") - } + // Check for negative values + if reqSize < 0 || maxSize < 0 { + return 0, errors.New("RequiredBytes and LimitBytes must not be negative") + } - // Handle case where only required size is specified - if maxSize == 0 { - return adjustToMinimumSize(reqSize), nil - } + // Handle case where only required size is specified + if maxSize == 0 { + return adjustToMinimumSize(reqSize), nil + } - // Handle case where max size is less than minimum allowed - if maxSize < MinVolumeSizeBytes { - return 0, fmt.Errorf("limit bytes %v is less than minimum allowed bytes %v", maxSize, MinVolumeSizeBytes) - } + // Handle case where max size is less than minimum allowed + if maxSize < MinVolumeSizeBytes { + return 0, fmt.Errorf("limit bytes %v is less than minimum allowed bytes %v", maxSize, MinVolumeSizeBytes) + } - // Determine the final size - return determineOptimalSize(reqSize, maxSize), nil + // Determine the final size + return determineOptimalSize(reqSize, maxSize), nil } // adjustToMinimumSize ensures that the provided size is at least the minimum volume size. // If the size is less than MinVolumeSizeBytes, it returns MinVolumeSizeBytes; otherwise, it returns the original size. func adjustToMinimumSize(size int64) int64 { - if size < MinVolumeSizeBytes { - return MinVolumeSizeBytes - } - return size + if size < MinVolumeSizeBytes { + return MinVolumeSizeBytes + } + return size } // determineOptimalSize calculates the optimal size for a volume based on the required size and maximum size. // If the required size is zero or less than the maximum size, it returns the maximum size. // Otherwise, it returns the required size. func determineOptimalSize(reqSize, maxSize int64) int64 { - if reqSize == 0 || reqSize < maxSize { - return maxSize - } - return reqSize + if reqSize == 0 || reqSize < maxSize { + return maxSize + } + return reqSize } // validVolumeCapabilities checks if the provided volume capabilities are valid. diff --git a/internal/driver/controllerserver_test.go b/internal/driver/controllerserver_test.go index 322cb55f..b8ee5d3c 100644 --- a/internal/driver/controllerserver_test.go +++ b/internal/driver/controllerserver_test.go @@ -7,11 +7,13 @@ import ( "testing" "github.com/container-storage-interface/spec/lib/go/csi" + "github.com/linode/linodego" + linodeclient "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-client" linodevolumes "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-volumes" - "github.com/linode/linodego" ) +//nolint:gocognit // As simple as possible. func TestListVolumes(t *testing.T) { cases := map[string]struct { volumes []linodego.Volume @@ -83,11 +85,12 @@ func TestListVolumes(t *testing.T) { } resp, err := cs.ListVolumes(context.Background(), &csi.ListVolumesRequest{}) - if err != nil && !tt.throwErr { + switch { + case (err != nil && !tt.throwErr): t.Fatal("failed to list volumes:", err) - } else if err != nil && tt.throwErr { + case (err != nil && tt.throwErr): // expected failure - } else if err == nil && tt.throwErr { + case (err == nil && tt.throwErr): t.Fatal("should have failed to list volumes") } @@ -101,7 +104,7 @@ func TestListVolumes(t *testing.T) { var linodeVolume *linodego.Volume for _, v := range tt.volumes { key := linodevolumes.CreateLinodeVolumeKey(v.ID, v.Label) - if volume.VolumeId == key.GetVolumeKey() { + if volume.GetVolumeId() == key.GetVolumeKey() { v := v linodeVolume = &v break @@ -111,11 +114,11 @@ func TestListVolumes(t *testing.T) { t.Fatalf("no matching linode volume for %#v", volume) } - if want, got := int64(linodeVolume.Size<<30), volume.CapacityBytes; want != got { + if want, got := int64(linodeVolume.Size<<30), volume.GetCapacityBytes(); want != got { t.Errorf("mismatched volume size: want=%d got=%d", want, got) } for _, topology := range volume.GetAccessibleTopology() { - region, ok := topology.Segments[VolumeTopologyRegion] + region, ok := topology.GetSegments()[VolumeTopologyRegion] if !ok { t.Error("region not set in volume topology") } @@ -129,7 +132,7 @@ func TestListVolumes(t *testing.T) { t.Error("nil status") continue } - if status.VolumeCondition.Abnormal { + if status.GetVolumeCondition().GetAbnormal() { t.Error("abnormal volume condition") } @@ -179,35 +182,48 @@ func (c *fakeLinodeClient) ListInstanceDisks(_ context.Context, _ int, _ *linode return c.disks, nil } +//nolint:nilnil // TODO: re-work tests func (flc *fakeLinodeClient) GetInstance(context.Context, int) (*linodego.Instance, error) { return nil, nil } +//nolint:nilnil // TODO: re-work tests func (flc *fakeLinodeClient) GetVolume(context.Context, int) (*linodego.Volume, error) { return nil, nil } +//nolint:nilnil // TODO: re-work tests func (flc *fakeLinodeClient) CreateVolume(context.Context, linodego.VolumeCreateOptions) (*linodego.Volume, error) { return nil, nil } +//nolint:nilnil // TODO: re-work tests func (flc *fakeLinodeClient) CloneVolume(context.Context, int, string) (*linodego.Volume, error) { return nil, nil } +//nolint:nilnil // TODO: re-work tests func (flc *fakeLinodeClient) AttachVolume(context.Context, int, *linodego.VolumeAttachOptions) (*linodego.Volume, error) { return nil, nil } + func (flc *fakeLinodeClient) DetachVolume(context.Context, int) error { return nil } + +//nolint:nilnil // TODO: re-work tests func (flc *fakeLinodeClient) WaitForVolumeLinodeID(context.Context, int, *int, int) (*linodego.Volume, error) { return nil, nil } +//nolint:nilnil // TODO: re-work tests func (flc *fakeLinodeClient) WaitForVolumeStatus(context.Context, int, linodego.VolumeStatus, int) (*linodego.Volume, error) { return nil, nil } -func (flc *fakeLinodeClient) DeleteVolume(context.Context, int) error { return nil } + +func (flc *fakeLinodeClient) DeleteVolume(context.Context, int) error { return nil } + func (flc *fakeLinodeClient) ResizeVolume(context.Context, int, int) error { return nil } + +//nolint:nilnil // TODO: re-work tests func (flc *fakeLinodeClient) NewEventPoller(context.Context, any, linodego.EntityType, linodego.EventAction) (*linodego.EventPoller, error) { return nil, nil } diff --git a/internal/driver/driver.go b/internal/driver/driver.go index 931980af..58010364 100644 --- a/internal/driver/driver.go +++ b/internal/driver/driver.go @@ -23,13 +23,13 @@ import ( "sync" "github.com/container-storage-interface/spec/lib/go/csi" - 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" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "k8s.io/mount-utils" + + 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" ) // Name is the name of the driver provided by this package. @@ -83,7 +83,7 @@ func (linodeDriver *LinodeDriver) SetupLinodeDriver( volumeLabelPrefix string, encrypt Encryption, ) error { - log, ctx, done := logger.GetLogger(ctx).WithMethod("SetupLinodeDriver") + log, _, done := logger.GetLogger(ctx).WithMethod("SetupLinodeDriver") defer done() log.V(2).Info("Setting up LinodeDriver") @@ -129,19 +129,19 @@ func (linodeDriver *LinodeDriver) SetupLinodeDriver( return nil } -func (linodeDriver *LinodeDriver) ValidateControllerServiceRequest(ctx context.Context, c csi.ControllerServiceCapability_RPC_Type) error { +func (linodeDriver *LinodeDriver) ValidateControllerServiceRequest(ctx context.Context, rpcType csi.ControllerServiceCapability_RPC_Type) error { log, _, done := logger.GetLogger(ctx).WithMethod("ValidateControllerServiceRequest") defer done() - log.V(4).Info("Validating controller service request", "type", c) + log.V(4).Info("Validating controller service request", "type", rpcType) - if c == csi.ControllerServiceCapability_RPC_UNKNOWN { + if rpcType == csi.ControllerServiceCapability_RPC_UNKNOWN { log.V(4).Info("Unknown controller service capability, skipping validation") return nil } for _, cap := range linodeDriver.cscap { - if c == cap.GetRpc().Type { + if rpcType == cap.GetRpc().GetType() { log.V(4).Info("Controller service request validated successfully") return nil } @@ -155,7 +155,7 @@ func (linodeDriver *LinodeDriver) Run(ctx context.Context, endpoint string) { defer done() log.V(2).Info("Starting LinodeDriver", "name", linodeDriver.name) - if len(linodeDriver.volumeLabelPrefix) > 0 { + if linodeDriver.volumeLabelPrefix != "" { log.V(4).Info("BS Volume Prefix", "prefix", linodeDriver.volumeLabelPrefix) } diff --git a/internal/driver/driver_test.go b/internal/driver/driver_test.go index 2c647384..c58f77d3 100644 --- a/internal/driver/driver_test.go +++ b/internal/driver/driver_test.go @@ -6,11 +6,11 @@ import ( "os" "testing" - "github.com/linode/linode-blockstorage-csi-driver/mocks" - linodeclient "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-client" + "go.uber.org/mock/gomock" "k8s.io/mount-utils" - "go.uber.org/mock/gomock" + "github.com/linode/linode-blockstorage-csi-driver/mocks" + linodeclient "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-client" ) var ( diff --git a/internal/driver/identityserver.go b/internal/driver/identityserver.go index 46017430..332271bc 100644 --- a/internal/driver/identityserver.go +++ b/internal/driver/identityserver.go @@ -18,11 +18,12 @@ import ( "fmt" csi "github.com/container-storage-interface/spec/lib/go/csi" - "github.com/linode/linode-blockstorage-csi-driver/pkg/logger" "golang.org/x/net/context" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/wrapperspb" + + "github.com/linode/linode-blockstorage-csi-driver/pkg/logger" ) // IdentityServer implements the CSI Identity service for the Linode Block Storage CSI Driver. diff --git a/internal/driver/luks.go b/internal/driver/luks.go index b2a0e588..ea7b21d2 100644 --- a/internal/driver/luks.go +++ b/internal/driver/luks.go @@ -29,9 +29,8 @@ import ( "strconv" "strings" - utilexec "k8s.io/utils/exec" - cryptsetup "github.com/martinjungblut/go-cryptsetup" + utilexec "k8s.io/utils/exec" cryptsetupclient "github.com/linode/linode-blockstorage-csi-driver/pkg/cryptsetup-client" "github.com/linode/linode-blockstorage-csi-driver/pkg/logger" @@ -100,8 +99,8 @@ func NewLuksEncryption(executor utilexec.Interface, fileSystem mountmanager.File } } -func getLuksContext(secrets map[string]string, context map[string]string, lifecycle VolumeLifecycle) LuksContext { - if context[LuksEncryptedAttribute] != "true" { +func getLuksContext(secrets, volContext map[string]string, lifecycle VolumeLifecycle) LuksContext { + if volContext[LuksEncryptedAttribute] != True { return LuksContext{ EncryptionEnabled: false, VolumeLifecycle: lifecycle, @@ -109,9 +108,9 @@ func getLuksContext(secrets map[string]string, context map[string]string, lifecy } luksKey := secrets[LuksKeyAttribute] - luksCipher := context[LuksCipherAttribute] - luksKeySize := context[LuksKeySizeAttribute] - volumeName := context[PublishInfoVolumeName] + luksCipher := volContext[LuksCipherAttribute] + luksKeySize := volContext[LuksKeySizeAttribute] + volumeName := volContext[PublishInfoVolumeName] return LuksContext{ EncryptionEnabled: true, @@ -123,8 +122,9 @@ func getLuksContext(secrets map[string]string, context map[string]string, lifecy } } -func (e *Encryption) luksFormat(ctx context.Context, luksCtx LuksContext, source string) (string, error) { +func (e *Encryption) luksFormat(ctx context.Context, luksCtx *LuksContext, source string) (devicePath string, err error) { log := logger.GetLogger(ctx) + devicePath = "/dev/mapper/" + luksCtx.VolumeName // Set params keySize, err := strconv.Atoi(luksCtx.EncryptionKeySize) @@ -140,14 +140,14 @@ func (e *Encryption) luksFormat(ctx context.Context, luksCtx LuksContext, source } // Initialize the device using the path - log.V(4).Info("Initializing device to perform luks format ", source) + log.V(4).Info("Initializing device to perform luks format", "source", source) newLuksDevice, err := cryptsetupclient.NewLuksDevice(e.CryptSetup, source) if err != nil { return "", fmt.Errorf("initializing luks device to format: %w", err) } // Format the device - log.V(4).Info("Formatting luks device ", newLuksDevice.Identifier) + log.V(4).Info("Formatting luks device", "device path", source) err = newLuksDevice.Device.Format(cryptsetup.LUKS2{SectorSize: 512}, genericParams) if err != nil { return "", fmt.Errorf("formatting luks device: %w", err) @@ -155,7 +155,7 @@ func (e *Encryption) luksFormat(ctx context.Context, luksCtx LuksContext, source // Add keysot by volumekey log.V(4).Info("Adding keysot by volumekey") - if err := newLuksDevice.Device.KeyslotAddByVolumeKey(0, "", luksCtx.EncryptionKey); err != nil { + if err = newLuksDevice.Device.KeyslotAddByVolumeKey(0, "", luksCtx.EncryptionKey); err != nil { return "", fmt.Errorf("adding keysot by volumekey: %w", err) } defer newLuksDevice.Device.Free() @@ -166,17 +166,17 @@ func (e *Encryption) luksFormat(ctx context.Context, luksCtx LuksContext, source if err != nil { return "", fmt.Errorf("activating %s luks device %s volumekey %s: %w", newLuksDevice.Identifier, luksCtx.VolumeName, luksCtx.EncryptionKey, err) } - log.V(4).Info("The LUKS volume is now ready ", luksCtx.VolumeName) + log.V(4).Info("The LUKS volume is now ready", "volumeName", luksCtx.VolumeName) // Return the mapper path - return "/dev/mapper/" + luksCtx.VolumeName, nil + return devicePath, nil } -func (e *Encryption) luksOpen(ctx context.Context, luksCtx LuksContext, source string) (string, error) { +func (e *Encryption) luksOpen(ctx context.Context, luksCtx *LuksContext, source string) (string, error) { log := logger.GetLogger(ctx) // Initialize the device using the path - log.V(4).Info("Initializing device to perform luks open ", source) + log.V(4).Info("Initializing device to perform luks open", "source", source) newLuksDevice, err := cryptsetupclient.NewLuksDevice(e.CryptSetup, source) if err != nil { return "", fmt.Errorf("initializing luks device to open: %w", err) @@ -199,7 +199,7 @@ func (e *Encryption) luksOpen(ctx context.Context, luksCtx LuksContext, source s return "", fmt.Errorf("activating %s luks device %s volumekey %s: %w", newLuksDevice.Identifier, luksCtx.VolumeName, luksCtx.EncryptionKey, err) } - log.V(4).Info("The LUKS volume is now ready ", luksCtx.VolumeName) + log.V(4).Info("The LUKS volume is now ready ", "volumeName", luksCtx.VolumeName) // Return the mapper path return "/dev/mapper/" + luksCtx.VolumeName, nil @@ -208,28 +208,28 @@ func (e *Encryption) luksOpen(ctx context.Context, luksCtx LuksContext, source s func (e *Encryption) luksClose(ctx context.Context, volumeName string) error { log := logger.GetLogger(ctx) // Initialize the device by name - log.V(4).Info("Initalizing device to perform luks close ", volumeName) + log.V(4).Info("Initializing device to perform luks close", "volumeName", volumeName) newLuksDeviceByName, err := cryptsetupclient.NewLuksDeviceByName(e.CryptSetup, volumeName) - if err != nil { - log.V(4).Info("device is no longer active ", volumeName) - return nil - } - log.V(4).Info("Initalized device to perform luks close ", volumeName) + if err == nil { + log.V(4).Info("Initialized device to perform luks close", "volumeName", volumeName) - // Deactivating the device - log.V(4).Info("Deactivating and closing the volume ", volumeName) - if err := newLuksDeviceByName.Device.Deactivate(volumeName); err != nil { - return fmt.Errorf("deactivating %s luks device: %w", volumeName, err) - } + // Deactivating the device + log.V(4).Info("Deactivating and closing the volume", "volumeName", volumeName) + if err := newLuksDeviceByName.Device.Deactivate(volumeName); err != nil { + return fmt.Errorf("deactivating %s luks device: %w", volumeName, err) + } - // Freeing the device - log.V(4).Info("Releasing/Freeing the device ", volumeName) - if !newLuksDeviceByName.Device.Free() { - return errors.New("could not release/free the luks device") - } - log.V(4).Info("Released/Freed the device ", volumeName) + // Freeing the device + log.V(4).Info("Releasing/Freeing the device", "volumeName", volumeName) + if !newLuksDeviceByName.Device.Free() { + return errors.New("could not release/free the luks device") + } + log.V(4).Info("Released/Freed the device", "volumeName", volumeName) - log.V(4).Info("Released/Freed and Deactivated/Closed the volume ", volumeName) + log.V(4).Info("Released/Freed and Deactivated/Closed the volume", "volumeName", volumeName) + return nil + } + log.V(4).Info("device is no longer active", "volumeName", volumeName) return nil } @@ -243,7 +243,7 @@ func (e *Encryption) blkidValid(ctx context.Context, source string) (bool, error blkidCmd := "blkid" _, err := e.Exec.LookPath(blkidCmd) if err != nil { - if err == exec.ErrNotFound { + if errors.Is(err, exec.ErrNotFound) { return false, fmt.Errorf("%q executable invalid", blkidCmd) } return false, err @@ -255,11 +255,10 @@ func (e *Encryption) blkidValid(ctx context.Context, source string) (bool, error cmd := e.Exec.Command(blkidCmd, blkidArgs...) err = cmd.Run() if err != nil { - exitError, ok := err.(utilexec.ExitError) - if !ok { + var exitError utilexec.ExitError + if !errors.As(err, &exitError) { return false, fmt.Errorf("checking blkdid failed: %w cmd: %q, args: %q", err, blkidCmd, blkidArgs) } - // ws := exitError.Sys().(syscall.WaitStatus) exitCode = exitError.ExitStatus() if exitCode == 2 { return false, nil diff --git a/internal/driver/metadata.go b/internal/driver/metadata.go index 06e46018..26373ab6 100644 --- a/internal/driver/metadata.go +++ b/internal/driver/metadata.go @@ -9,8 +9,9 @@ import ( "strconv" metadata "github.com/linode/go-metadata" - "github.com/linode/linode-blockstorage-csi-driver/pkg/logger" "github.com/linode/linodego" + + "github.com/linode/linode-blockstorage-csi-driver/pkg/logger" ) // Metadata contains metadata about the node/instance the CSI node plugin @@ -51,7 +52,7 @@ func GetMetadata(ctx context.Context) (Metadata, error) { "region", data.Region, "memory", data.Specs.Memory) - metadata := Metadata{ + nodeMetadata := Metadata{ ID: data.ID, Label: data.Label, Region: data.Region, @@ -59,7 +60,7 @@ func GetMetadata(ctx context.Context) (Metadata, error) { } log.V(2).Info("Successfully completed") - return metadata, nil + return nodeMetadata, nil } // memoryToBytes converts the given amount of memory in MB, to bytes. @@ -87,7 +88,7 @@ var errNilClient = errors.New("nil client") // GetMetadataFromAPI attempts to retrieve metadata about the current // node/instance directly from the Linode API. func GetMetadataFromAPI(ctx context.Context, client *linodego.Client) (Metadata, error) { - log, ctx, done := logger.GetLogger(ctx).WithMethod("GetMetadataFromAPI") + log, _, done := logger.GetLogger(ctx).WithMethod("GetMetadataFromAPI") defer done() log.V(2).Info("Processing request") @@ -102,16 +103,18 @@ func GetMetadataFromAPI(ctx context.Context, client *linodego.Client) (Metadata, } log.V(4).Info("Opening LinodeIDPath", "path", LinodeIDPath) - f, err := os.Open(LinodeIDPath) + fileObj, err := os.Open(LinodeIDPath) if err != nil { return Metadata{}, fmt.Errorf("open: %w", err) } - defer f.Close() + defer func() { + err = fileObj.Close() + }() log.V(4).Info("Reading LinodeID from file") // Read in the file, but use a LimitReader to make sure we are not // reading in junk. - data, err := io.ReadAll(io.LimitReader(f, 1<<10)) + data, err := io.ReadAll(io.LimitReader(fileObj, 1<<10)) if err != nil { return Metadata{}, fmt.Errorf("read all: %w", err) } @@ -133,7 +136,7 @@ func GetMetadataFromAPI(ctx context.Context, client *linodego.Client) (Metadata, memory = memoryToBytes(instance.Specs.Memory) } - metadata := Metadata{ + nodeMetadata := Metadata{ ID: linodeID, Label: instance.Label, Region: instance.Region, @@ -141,11 +144,11 @@ func GetMetadataFromAPI(ctx context.Context, client *linodego.Client) (Metadata, } log.V(4).Info("Successfully retrieved metadata", - "instanceID", metadata.ID, - "instanceLabel", metadata.Label, - "region", metadata.Region, - "memory", metadata.Memory) + "instanceID", nodeMetadata.ID, + "instanceLabel", nodeMetadata.Label, + "region", nodeMetadata.Region, + "memory", nodeMetadata.Memory) log.V(2).Info("Successfully completed") - return metadata, nil + return nodeMetadata, nil } diff --git a/internal/driver/nodeserver.go b/internal/driver/nodeserver.go index 05a3283a..2bb0fd20 100644 --- a/internal/driver/nodeserver.go +++ b/internal/driver/nodeserver.go @@ -21,13 +21,14 @@ import ( "sync" "github.com/container-storage-interface/spec/lib/go/csi" + "github.com/linode/linodego" + "golang.org/x/net/context" + "k8s.io/mount-utils" + linodeclient "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-client" linodevolumes "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-volumes" "github.com/linode/linode-blockstorage-csi-driver/pkg/logger" mountmanager "github.com/linode/linode-blockstorage-csi-driver/pkg/mount-manager" - "github.com/linode/linodego" - "golang.org/x/net/context" - "k8s.io/mount-utils" ) type NodeServer struct { @@ -81,7 +82,7 @@ func NewNodeServer(ctx context.Context, linodeDriver *LinodeDriver, mounter *mou } func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolumeRequest) (*csi.NodePublishVolumeResponse, error) { - log, ctx, done := logger.GetLogger(ctx).WithMethod("NodePublishVolume") + log, _, done := logger.GetLogger(ctx).WithMethod("NodePublishVolume") defer done() volumeID := req.GetVolumeId() @@ -138,7 +139,7 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis } func (ns *NodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpublishVolumeRequest) (*csi.NodeUnpublishVolumeResponse, error) { - log, ctx, done := logger.GetLogger(ctx).WithMethod("NodeUnpublishVolume") + log, _, done := logger.GetLogger(ctx).WithMethod("NodeUnpublishVolume") defer done() targetPath := req.GetTargetPath() @@ -165,7 +166,7 @@ func (ns *NodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpu } func (ns *NodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolumeRequest) (*csi.NodeStageVolumeResponse, error) { - log, ctx, done := logger.GetLogger(ctx).WithMethod("NodeStageVolume") + log, _, done := logger.GetLogger(ctx).WithMethod("NodeStageVolume") defer done() volumeID := req.GetVolumeId() @@ -220,7 +221,7 @@ func (ns *NodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStageVol // Check if the volume mode is set to 'Block' // Do nothing else with the mount point for stage - if blk := req.VolumeCapability.GetBlock(); blk != nil { + if blk := req.GetVolumeCapability().GetBlock(); blk != nil { log.V(4).Info("Volume is a block volume", "volumeID", volumeID) return &csi.NodeStageVolumeResponse{}, nil } @@ -237,7 +238,7 @@ func (ns *NodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStageVol } func (ns *NodeServer) NodeUnstageVolume(ctx context.Context, req *csi.NodeUnstageVolumeRequest) (*csi.NodeUnstageVolumeResponse, error) { - log, ctx, done := logger.GetLogger(ctx).WithMethod("NodeUnstageVolume") + log, _, done := logger.GetLogger(ctx).WithMethod("NodeUnstageVolume") defer done() stagingTargetPath := req.GetStagingTargetPath() @@ -271,7 +272,7 @@ func (ns *NodeServer) NodeUnstageVolume(ctx context.Context, req *csi.NodeUnstag } func (ns *NodeServer) NodeExpandVolume(ctx context.Context, req *csi.NodeExpandVolumeRequest) (*csi.NodeExpandVolumeResponse, error) { - log, ctx, done := logger.GetLogger(ctx).WithMethod("NodeExpandVolume") + log, _, done := logger.GetLogger(ctx).WithMethod("NodeExpandVolume") defer done() volumeID := req.GetVolumeId() @@ -304,7 +305,7 @@ func (ns *NodeServer) NodeExpandVolume(ctx context.Context, req *csi.NodeExpandV log.V(2).Info("Successfully completed", "volumeID", volumeID) return &csi.NodeExpandVolumeResponse{ - CapacityBytes: req.CapacityRange.RequiredBytes, + CapacityBytes: req.GetCapacityRange().GetRequiredBytes(), }, nil } @@ -353,7 +354,7 @@ func (ns *NodeServer) NodeGetInfo(ctx context.Context, req *csi.NodeGetInfoReque } func (ns *NodeServer) NodeGetVolumeStats(ctx context.Context, req *csi.NodeGetVolumeStatsRequest) (*csi.NodeGetVolumeStatsResponse, error) { - log, ctx, done := logger.GetLogger(ctx).WithMethod("NodeGetVolumeStats") + log, _, done := logger.GetLogger(ctx).WithMethod("NodeGetVolumeStats") defer done() log.V(2).Info("Processing request", "req", req) diff --git a/internal/driver/nodeserver_all.go b/internal/driver/nodeserver_all.go index 63bbb749..0bc8f913 100644 --- a/internal/driver/nodeserver_all.go +++ b/internal/driver/nodeserver_all.go @@ -8,22 +8,23 @@ import ( "fmt" "github.com/container-storage-interface/spec/lib/go/csi" - "github.com/linode/linode-blockstorage-csi-driver/pkg/logger" "golang.org/x/sys/unix" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + + "github.com/linode/linode-blockstorage-csi-driver/pkg/logger" ) func nodeGetVolumeStats(ctx context.Context, req *csi.NodeGetVolumeStatsRequest) (*csi.NodeGetVolumeStatsResponse, error) { log := logger.GetLogger(ctx) - if req.VolumeId == "" || req.VolumePath == "" { + if req.GetVolumeId() == "" || req.GetVolumePath() == "" { return nil, status.Error(codes.InvalidArgument, "volume ID or path empty") } var statfs unix.Statfs_t // See http://man7.org/linux/man-pages/man2/statfs.2.html for details. - err := unix.Statfs(req.VolumePath, &statfs) + err := unix.Statfs(req.GetVolumePath(), &statfs) if err != nil && !errors.Is(err, unix.EIO) { if errors.Is(err, unix.ENOENT) { return nil, status.Errorf(codes.NotFound, "volume path not found: %v", err.Error()) @@ -40,9 +41,9 @@ func nodeGetVolumeStats(ctx context.Context, req *csi.NodeGetVolumeStatsRequest) response := &csi.NodeGetVolumeStatsResponse{ Usage: []*csi.VolumeUsage{ { - Available: int64(statfs.Bavail) * int64(statfs.Bsize), - Total: int64(statfs.Blocks) * int64(statfs.Bsize), - Used: (int64(statfs.Blocks) - int64(statfs.Bfree)) * int64(statfs.Bsize), + Available: int64(statfs.Bavail) * int64(statfs.Bsize), //nolint:unconvert // probably false positive because uint32 and int64 dont match + Total: int64(statfs.Blocks) * int64(statfs.Bsize), //nolint:unconvert // probably false positive because uint32 and int64 dont match + Used: int64(statfs.Blocks-statfs.Bfree) * int64(statfs.Bsize), //nolint:unconvert // probably false positive because uint32 and int64 dont match Unit: csi.VolumeUsage_BYTES, }, { @@ -58,6 +59,6 @@ func nodeGetVolumeStats(ctx context.Context, req *csi.NodeGetVolumeStatsRequest) }, } - log.V(2).Info("Successfully retrieved volume stats", "volumeID", req.VolumeId, "volumePath", req.VolumePath, "response", response) + log.V(2).Info("Successfully retrieved volume stats", "volumeID", req.GetVolumeId(), "volumePath", req.GetVolumePath(), "response", response) return response, nil } diff --git a/internal/driver/nodeserver_helpers.go b/internal/driver/nodeserver_helpers.go index a472a533..ec0a9def 100644 --- a/internal/driver/nodeserver_helpers.go +++ b/internal/driver/nodeserver_helpers.go @@ -22,20 +22,19 @@ import ( "strings" "github.com/container-storage-interface/spec/lib/go/csi" + "k8s.io/klog/v2" + "k8s.io/mount-utils" + utilexec "k8s.io/utils/exec" linodevolumes "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-volumes" "github.com/linode/linode-blockstorage-csi-driver/pkg/logger" mountmanager "github.com/linode/linode-blockstorage-csi-driver/pkg/mount-manager" - - "k8s.io/klog/v2" - "k8s.io/mount-utils" - utilexec "k8s.io/utils/exec" ) const ( defaultFSType = "ext4" - rwPermission = os.FileMode(0755) - ownerGroupReadWritePermissions = os.FileMode(0660) + rwPermission = os.FileMode(0o755) + ownerGroupReadWritePermissions = os.FileMode(0o660) ) // TODO: Figure out a better home for these interfaces @@ -147,23 +146,21 @@ func validateNodeUnpublishVolumeRequest(ctx context.Context, req *csi.NodeUnpubl // getFSTypeAndMountOptions retrieves the file system type and mount options from the given volume capability. // If the capability is not set, the default file system type and empty mount options will be returned. -func getFSTypeAndMountOptions(ctx context.Context, volumeCapability *csi.VolumeCapability) (string, []string) { +func getFSTypeAndMountOptions(ctx context.Context, volumeCapability *csi.VolumeCapability) (fsType string, mountOptions []string) { log := logger.GetLogger(ctx) log.V(4).Info("Entering getFSTypeAndMountOptions", "volumeCapability", volumeCapability) // Use default file system type if not specified in the volume capability - fsType := defaultFSType - // Use mount options from the volume capability if specified - var mountOptions []string + fsType = defaultFSType if mnt := volumeCapability.GetMount(); mnt != nil { // Use file system type from volume capability if specified - if mnt.FsType != "" { - fsType = mnt.FsType + if mnt.GetFsType() != "" { + fsType = mnt.GetFsType() } // Use mount options from volume capability if specified if mnt.MountFlags != nil { - mountOptions = mnt.MountFlags + mountOptions = mnt.GetMountFlags() } } @@ -218,7 +215,7 @@ func (ns *NodeServer) ensureMountPoint(ctx context.Context, path string, fs moun if err != nil { // Checking IsNotExist returns true. If true, it mean we need to create directory at the target path. if fs.IsNotExist(err) { - if err := fs.MkdirAll(path, rwPermission); err != nil { + if err = fs.MkdirAll(path, rwPermission); err != nil { return true, errInternal("Failed to create directory (%q): %v", path, err) } } else { @@ -247,7 +244,7 @@ func (ns *NodeServer) nodePublishVolumeBlock(ctx context.Context, req *csi.NodeP targetPathDir := filepath.Dir(targetPath) // Get the device path from the request - devicePath := req.PublishContext["devicePath"] + devicePath := req.GetPublishContext()["devicePath"] if devicePath == "" { return nil, errInternal("devicePath cannot be found") } @@ -268,7 +265,9 @@ func (ns *NodeServer) nodePublishVolumeBlock(ctx context.Context, req *csi.NodeP } return nil, errInternal("Failed to create file %s: %v", targetPath, err) } - file.Close() + defer func() { + err = file.Close() + }() // Mount the volume log.V(4).Info("Mounting volume", "devicePath", devicePath, "targetPath", targetPath, "mountOptions", mountOptions) @@ -303,11 +302,11 @@ func (ns *NodeServer) mountVolume(ctx context.Context, devicePath string, req *c fmtAndMountSource := devicePath // Check if LUKS encryption is enabled and prepare the LUKS volume if needed - luksContext := getLuksContext(req.Secrets, req.VolumeContext, VolumeLifecycleNodeStageVolume) + luksContext := getLuksContext(req.GetSecrets(), req.GetVolumeContext(), VolumeLifecycleNodeStageVolume) if luksContext.EncryptionEnabled { var err error log.V(4).Info("preparing luks volume", "devicePath", devicePath) - fmtAndMountSource, err = ns.formatLUKSVolume(ctx, devicePath, luksContext) + fmtAndMountSource, err = ns.formatLUKSVolume(ctx, devicePath, &luksContext) if err != nil { return err } @@ -329,7 +328,7 @@ func (ns *NodeServer) mountVolume(ctx context.Context, devicePath string, req *c // It checks if the device at devicePath is already formatted with LUKS encryption. // If not, it formats the device using the provided LuksContext. // Finally, it prepares the LUKS volume for mounting. -func (ns *NodeServer) formatLUKSVolume(ctx context.Context, devicePath string, luksContext LuksContext) (luksSource string, err error) { +func (ns *NodeServer) formatLUKSVolume(ctx context.Context, devicePath string, luksContext *LuksContext) (luksSource string, err error) { log := logger.GetLogger(ctx) log.V(4).Info("Entering formatLUKSVolume", "devicePath", devicePath, "luksContext", luksContext) @@ -346,7 +345,7 @@ func (ns *NodeServer) formatLUKSVolume(ctx context.Context, devicePath string, l // If device is not formatted, format it if !formatted { - log.V(4).Info("luks volume not yet formated... Attempting to format: ", devicePath) + log.V(4).Info("luks volume not yet formated... Attempting to format", "devicePath", devicePath) // Format the volume with LUKS encryption. if luksSource, err = ns.encrypt.luksFormat(ctx, luksContext, devicePath); err != nil { diff --git a/internal/driver/nodeserver_helpers_linux_test.go b/internal/driver/nodeserver_helpers_linux_test.go index 5f2b05d9..b32dcdb6 100644 --- a/internal/driver/nodeserver_helpers_linux_test.go +++ b/internal/driver/nodeserver_helpers_linux_test.go @@ -9,11 +9,11 @@ import ( "testing" "github.com/container-storage-interface/spec/lib/go/csi" - "github.com/linode/linode-blockstorage-csi-driver/mocks" - "go.uber.org/mock/gomock" "k8s.io/mount-utils" "k8s.io/utils/exec" + + "github.com/linode/linode-blockstorage-csi-driver/mocks" ) func TestNodeServer_mountVolume_linux(t *testing.T) { diff --git a/internal/driver/nodeserver_helpers_test.go b/internal/driver/nodeserver_helpers_test.go index 907a8b9a..169fef37 100644 --- a/internal/driver/nodeserver_helpers_test.go +++ b/internal/driver/nodeserver_helpers_test.go @@ -1,19 +1,19 @@ package driver import ( + "context" "fmt" "os" "reflect" "testing" - "context" - "github.com/container-storage-interface/spec/lib/go/csi" - "github.com/linode/linode-blockstorage-csi-driver/mocks" - linodevolumes "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-volumes" "go.uber.org/mock/gomock" "google.golang.org/grpc/status" "k8s.io/mount-utils" + + "github.com/linode/linode-blockstorage-csi-driver/mocks" + linodevolumes "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-volumes" ) // compareGRPCErrors compares two gRPC errors for equality. @@ -160,7 +160,7 @@ func Test_getFSTypeAndMountOptions(t *testing.T) { name: "Valid request - no volume capability set", volumeCapability: nil, wantFsType: "ext4", - wantMountOptions: *new([]string), + wantMountOptions: []string(nil), }, { name: "Valid request - volume capability set", @@ -243,7 +243,6 @@ func TestNodeServer_findDevicePath(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Create gomock controller ctrl := gomock.NewController(t) defer ctrl.Finish() @@ -342,7 +341,6 @@ func TestNodeServer_ensureMountPoint(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ctrl := gomock.NewController(t) defer ctrl.Finish() diff --git a/internal/driver/nodeserver_luks_encryption_test.go b/internal/driver/nodeserver_luks_encryption_test.go index 8b123795..c82ed5fb 100644 --- a/internal/driver/nodeserver_luks_encryption_test.go +++ b/internal/driver/nodeserver_luks_encryption_test.go @@ -9,11 +9,11 @@ import ( "testing" "github.com/container-storage-interface/spec/lib/go/csi" - "github.com/linode/linode-blockstorage-csi-driver/mocks" - "go.uber.org/mock/gomock" "k8s.io/mount-utils" "k8s.io/utils/exec" + + "github.com/linode/linode-blockstorage-csi-driver/mocks" ) func TestNodeServer_mountVolume_luks(t *testing.T) { @@ -343,7 +343,6 @@ func TestNodeServer_closeLuksMountSource(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ctrl := gomock.NewController(t) defer ctrl.Finish() @@ -406,7 +405,6 @@ func TestNodeServer_formatLUKSVolume(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // SetUp() ctrl := gomock.NewController(t) @@ -435,7 +433,7 @@ func TestNodeServer_formatLUKSVolume(t *testing.T) { encrypt: NewLuksEncryption(mockExec, mockFileSystem, mockCryptSetupClient), } - got, err := ns.formatLUKSVolume(context.Background(), tt.devicePath, tt.luksContext) + got, err := ns.formatLUKSVolume(context.Background(), tt.devicePath, &tt.luksContext) if (err != nil) != tt.wantErr { t.Errorf("NodeServer.formatLUKSVolume() error = %v, wantErr %v", err, tt.wantErr) return diff --git a/internal/driver/nodeserver_test.go b/internal/driver/nodeserver_test.go index cbf506e1..e903a762 100644 --- a/internal/driver/nodeserver_test.go +++ b/internal/driver/nodeserver_test.go @@ -5,10 +5,11 @@ import ( "reflect" "testing" - linodeclient "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-client" - mountmanager "github.com/linode/linode-blockstorage-csi-driver/pkg/mount-manager" "github.com/linode/linodego" "k8s.io/mount-utils" + + linodeclient "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-client" + mountmanager "github.com/linode/linode-blockstorage-csi-driver/pkg/mount-manager" ) func TestNewNodeServer(t *testing.T) { diff --git a/internal/driver/server.go b/internal/driver/server.go index 837490ca..037c78b5 100644 --- a/internal/driver/server.go +++ b/internal/driver/server.go @@ -20,10 +20,9 @@ import ( "os" "sync" + csi "github.com/container-storage-interface/spec/lib/go/csi" "google.golang.org/grpc" "k8s.io/klog/v2" - - csi "github.com/container-storage-interface/spec/lib/go/csi" ) // Defines Non blocking GRPC server interfaces @@ -70,26 +69,27 @@ func (s *nonBlockingGRPCServer) serve(endpoint string, ids csi.IdentityServer, c grpc.UnaryInterceptor(logGRPC), } - u, err := url.Parse(endpoint) + urlObj, err := url.Parse(endpoint) if err != nil { klog.Fatal(err.Error()) } var addr string - if u.Scheme == "unix" { - addr = u.Path - if err := os.Remove(addr); err != nil && !os.IsNotExist(err) { + switch scheme := urlObj.Scheme; scheme { + case "unix": + addr = urlObj.Path + if err = os.Remove(addr); err != nil && !os.IsNotExist(err) { klog.Fatalf("Failed to remove %s, error: %s", addr, err.Error()) } - } else if u.Scheme == "tcp" { - addr = u.Host - } else { - klog.Fatalf("%v endpoint scheme not supported", u.Scheme) + case "tcp": + addr = urlObj.Host + default: + klog.Fatalf("%v endpoint scheme not supported", urlObj.Scheme) } - klog.V(4).Infof("Start listening with scheme %v, addr %v", u.Scheme, addr) - listener, err := net.Listen(u.Scheme, addr) + klog.V(4).Infof("Start listening with scheme %v, addr %v", urlObj.Scheme, addr) + listener, err := net.Listen(urlObj.Scheme, addr) if err != nil { klog.Fatalf("Failed to listen: %v", err) } @@ -112,5 +112,4 @@ func (s *nonBlockingGRPCServer) serve(endpoint string, ids csi.IdentityServer, c if err := server.Serve(listener); err != nil { klog.Fatalf("Failed to serve: %v", err) } - } diff --git a/main.go b/main.go index 745bb402..fa622dc1 100644 --- a/main.go +++ b/main.go @@ -51,7 +51,7 @@ type configuration struct { // Name of the current node, when running as the node plugin. // - // deprecated: This is not needed as the CSI driver now uses the Linode + // Deprecated: This is not needed as the CSI driver now uses the Linode // Metadata Service to source information about the current // node/instance. It will be removed in a future change. nodeName string @@ -69,22 +69,18 @@ func loadConfig() configuration { } func main() { - klog.InitFlags(nil) - _ = flag.Set("logtostderr", "true") - flag.Parse() - // Create a base context with the logger ctx := context.Background() log := logger.NewLogger(ctx) ctx = context.WithValue(ctx, logger.LoggerKey{}, log) - undoMaxprocs, maxprocsError := maxprocs.Set(maxprocs.Logger(func(msg string, keysAndValues ...interface{}) { - log.Klogr.WithValues("component", "maxprocs", "version", maxprocs.Version).V(2).Info(fmt.Sprintf(msg, keysAndValues...)) - })) - defer undoMaxprocs() - if maxprocsError != nil { - log.Error(maxprocsError, "Failed to set GOMAXPROCS") + klog.InitFlags(nil) + if err := flag.Set("logtostderr", "true"); err != nil { + log.Error(err, "Fatal error") + os.Exit(0) } + flag.Parse() + maxProcs(log) if err := handle(ctx); err != nil { log.Error(err, "Fatal error") @@ -94,6 +90,16 @@ func main() { os.Exit(0) } +func maxProcs(log *logger.Logger) { + undoMaxprocs, maxprocsError := maxprocs.Set(maxprocs.Logger(func(msg string, keysAndValues ...interface{}) { + log.Klogr.WithValues("component", "maxprocs", "version", maxprocs.Version).V(2).Info(fmt.Sprintf(msg, keysAndValues...)) + })) + defer undoMaxprocs() + if maxprocsError != nil { + log.Error(maxprocsError, "Failed to set GOMAXPROCS") + } +} + func handle(ctx context.Context) error { log := logger.GetLogger(ctx) @@ -113,7 +119,7 @@ func handle(ctx context.Context) error { uaPrefix := fmt.Sprintf("LinodeCSI/%s", vendorVersion) cloudProvider, err := linodeclient.NewLinodeClient(cfg.linodeToken, uaPrefix, cfg.linodeURL) if err != nil { - return fmt.Errorf("failed to set up linode client: %s", err) + return fmt.Errorf("failed to set up linode client: %w", err) } mounter := mountmanager.NewSafeMounter() @@ -141,7 +147,7 @@ func handle(ctx context.Context) error { cfg.volumeLabelPrefix, encrypt, ); err != nil { - return fmt.Errorf("setup driver: %v", err) + return fmt.Errorf("setup driver: %w", err) } linodeDriver.Run(ctx, cfg.csiEndpoint) diff --git a/pkg/linode-client/linode-client.go b/pkg/linode-client/linode-client.go index f6f2b0c5..5425ad3f 100644 --- a/pkg/linode-client/linode-client.go +++ b/pkg/linode-client/linode-client.go @@ -33,7 +33,7 @@ type LinodeClient interface { NewEventPoller(context.Context, any, linodego.EntityType, linodego.EventAction) (*linodego.EventPoller, error) } -func NewLinodeClient(token, ua string, apiURL string) (*linodego.Client, error) { +func NewLinodeClient(token, ua, apiURL string) (*linodego.Client, error) { // Use linodego built-in http client which supports setting root CA cert linodeClient := linodego.NewClient(nil) linodeClient.SetUserAgent(ua) @@ -58,17 +58,17 @@ func NewLinodeClient(token, ua string, apiURL string) (*linodego.Client, error) // getAPIURLComponents returns the API URL components (base URL, api version) given an input URL. // This is necessary due to some recent changes with how linodego handles // client.SetBaseURL(...) and client.SetAPIVersion(...) -func getAPIURLComponents(apiURL string) (string, string, error) { - u, err := url.Parse(apiURL) +func getAPIURLComponents(apiURL string) (host, version string, err error) { + urlObj, err := url.Parse(apiURL) if err != nil { return "", "", err } - version := "" - host := fmt.Sprintf("%s://%s", u.Scheme, u.Host) + version = "" + host = fmt.Sprintf("%s://%s", urlObj.Scheme, urlObj.Host) - if strings.ReplaceAll(u.Path, "/", "") == "" { - pathSegments := strings.Split(u.Path, "/") + if strings.ReplaceAll(urlObj.Path, "/", "") == "" { + pathSegments := strings.Split(urlObj.Path, "/") // The API version will be the last path value version = pathSegments[len(pathSegments)-1] } diff --git a/pkg/linode-volumes/utils.go b/pkg/linode-volumes/utils.go index 76654941..1aba0cd0 100644 --- a/pkg/linode-volumes/utils.go +++ b/pkg/linode-volumes/utils.go @@ -32,11 +32,11 @@ func hashStringToInt(b string) int { func VolumeIdAsInt(caller string, w withVolume) (int, error) { strVolID := w.GetVolumeId() - if len(caller) != 0 { - caller = caller + " " + if caller != "" { + caller += " " } - if len(strVolID) == 0 { + if strVolID == "" { return 0, status.Errorf(codes.InvalidArgument, "%sVolume ID must be provided", caller) } @@ -53,11 +53,11 @@ func VolumeIdAsInt(caller string, w withVolume) (int, error) { func NodeIdAsInt(caller string, w withNode) (int, error) { strNodeID := w.GetNodeId() - if len(caller) != 0 { - caller = caller + " " + if caller != "" { + caller += " " } - if len(strNodeID) == 0 { + if strNodeID == "" { return 0, status.Errorf(codes.InvalidArgument, "%sNode ID must be provided", caller) } diff --git a/pkg/mount-manager/device-utils.go b/pkg/mount-manager/device-utils.go index 99e34003..510d4976 100644 --- a/pkg/mount-manager/device-utils.go +++ b/pkg/mount-manager/device-utils.go @@ -56,15 +56,15 @@ func NewDeviceUtils() *deviceUtils { } // Returns list of all /dev/disk/by-id/* paths for given PD. -func (m *deviceUtils) GetDiskByIdPaths(deviceName string, partition string) []string { +func (m *deviceUtils) GetDiskByIdPaths(deviceName, partition string) []string { devicePaths := []string{ path.Join(diskByIdPath, diskLinodePrefix+deviceName), path.Join(diskByIdPath, diskScsiLinodePrefix+deviceName), } if partition != "" { - for i, path := range devicePaths { - devicePaths[i] = path + diskPartitionSuffix + partition + for i, devicePath := range devicePaths { + devicePaths[i] = devicePath + diskPartitionSuffix + partition } } @@ -85,11 +85,11 @@ func (m *deviceUtils) VerifyDevicePath(devicePaths []string) (string, error) { klog.Errorf("udevadmChangeToNewDrives failed with: %v", err) } - for _, path := range devicePaths { - if pathExists, err := pathExists(path); err != nil { - return "", fmt.Errorf("error checking if path exists: %v", err) + for _, devicePath := range devicePaths { + if pathExists, err := pathExists(devicePath); err != nil { + return "", fmt.Errorf("error checking if path exists: %w", err) } else if pathExists { - return path, nil + return devicePath, nil } } @@ -148,13 +148,13 @@ func udevadmChangeToDrive(drivePath string) error { } // PathExists returns true if the specified path exists. -func pathExists(path string) (bool, error) { - _, err := os.Stat(path) +func pathExists(devicePath string) (bool, error) { + _, err := os.Stat(devicePath) if err == nil { return true, nil - } else if os.IsNotExist(err) { + } + if os.IsNotExist(err) { return false, nil - } else { - return false, err } + return false, err } diff --git a/pkg/mount-manager/fs-utils.go b/pkg/mount-manager/fs-utils.go index df906842..8ea912c4 100644 --- a/pkg/mount-manager/fs-utils.go +++ b/pkg/mount-manager/fs-utils.go @@ -41,6 +41,7 @@ func (OSFileSystem) Remove(path string) error { return os.Remove(path) } +//nolint:gosec // intentional variable to open file func (OSFileSystem) OpenFile(name string, flag int, perm os.FileMode) (FileInterface, error) { return os.OpenFile(name, flag, perm) } From 68dc5ce49a172fabd23dd5550d0d050123b0eb5c Mon Sep 17 00:00:00 2001 From: Amol Deodhar Date: Mon, 23 Sep 2024 13:59:27 -0400 Subject: [PATCH 2/2] fix linter issues after rebase --- internal/driver/controllerserver.go | 228 ++++-------------- internal/driver/controllerserver_helper.go | 73 +++--- .../driver/controllerserver_helper_test.go | 18 +- internal/driver/errors.go | 3 + 4 files changed, 100 insertions(+), 222 deletions(-) diff --git a/internal/driver/controllerserver.go b/internal/driver/controllerserver.go index 05bd6f73..8568be0d 100644 --- a/internal/driver/controllerserver.go +++ b/internal/driver/controllerserver.go @@ -15,8 +15,6 @@ import ( "github.com/linode/linode-blockstorage-csi-driver/pkg/logger" ) -const True = "true" - type ControllerServer struct { driver *LinodeDriver client linodeclient.LinodeClient @@ -64,17 +62,10 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol log.V(2).Info("Processing request", "req", req) - if name == "" { - return &csi.CreateVolumeResponse{}, errNoVolumeName - } - - // validate volume capabilities - volCapabilities := req.GetVolumeCapabilities() - if len(volCapabilities) == 0 { - return &csi.CreateVolumeResponse{}, errNoVolumeCapabilities - } - if !validVolumeCapabilities(volCapabilities) { - return &csi.CreateVolumeResponse{}, errInvalidVolumeCapability(volCapabilities) + // Validate the incoming request to ensure it meets the necessary criteria. + // This includes checking for required fields and valid volume capabilities. + if err := cs.validateCreateVolumeRequest(ctx, req); err != nil { + return &csi.CreateVolumeResponse{}, err } // Prepare the volume parameters such as name and SizeGB from the request. @@ -84,98 +75,25 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol return &csi.CreateVolumeResponse{}, err } - // to avoid mangled requests for existing volumes with hyphen, - // we only strip them out on creation when k8s invented the name - // this is still problematic because we strip "-" from volume-name-prefixes - // that specifically requested "-". - // Don't strip this when volume labels support sufficient length - condensedName := strings.ReplaceAll(name, "-", "") - - preKey := linodevolumes.CreateLinodeVolumeKey(0, condensedName) + // Create volume context + volContext := cs.createVolumeContext(ctx, req) - volumeName := preKey.GetNormalizedLabelWithPrefix(cs.driver.volumeLabelPrefix) - targetSizeGB := bytesToGB(size) - - log.V(4).Info("CreateVolume details", "storage_size_giga_bytes", targetSizeGB, "volume_name", volumeName) - - volumeContext := make(map[string]string) - if req.GetParameters()[LuksEncryptedAttribute] == True { - // if luks encryption is enabled add a volume context - volumeContext[LuksEncryptedAttribute] = True - volumeContext[PublishInfoVolumeName] = volumeName - volumeContext[LuksCipherAttribute] = req.GetParameters()[LuksCipherAttribute] - volumeContext[LuksKeySizeAttribute] = req.GetParameters()[LuksKeySizeAttribute] - } - - // Attempt to get info about the source volume for - // volume cloning if the datasource is provided in the PVC. - // sourceVolumeInfo will be null if no content source is defined. - contentSource := req.GetVolumeContentSource() - sourceVolumeInfo, err := cs.getContentSourceVolume(ctx, contentSource) + // Attempt to retrieve information about a source volume if the request includes a content source. + // This is important for scenarios where the volume is being cloned from an existing one. + sourceVolInfo, err := cs.getContentSourceVolume(ctx, req.GetVolumeContentSource()) if err != nil { return &csi.CreateVolumeResponse{}, err } - // Attempt to create the volume while respecting idempotency. - // If the content source is defined, the source volume will be cloned to create a new volume. - log.V(4).Info("Calling API to create volume", "volumeName", volumeName) - vol, err := cs.attemptCreateLinodeVolume( - ctx, - volumeName, - targetSizeGB, - req.GetParameters()[VolumeTags], - sourceVolumeInfo, - ) + // Create the volume + vol, err := cs.createAndWaitForVolume(ctx, volName, sizeGB, req.GetParameters()[VolumeTags], sourceVolInfo) if err != nil { return &csi.CreateVolumeResponse{}, err } - // If the existing volume size differs from the requested size, we throw an error. - if vol.Size != targetSizeGB { - return nil, errAlreadyExists("volume %d already exists of size %d", vol.ID, vol.Size) - } - - statusPollTimeout := waitTimeout() - - // If we're cloning the volume we should extend the timeout - if sourceVolumeInfo != nil { - statusPollTimeout = cloneTimeout() - } - - log.V(4).Info("Waiting for volume to be active", "volumeID", vol.ID) - if _, err := cs.client.WaitForVolumeStatus( - ctx, vol.ID, linodego.VolumeActive, statusPollTimeout); err != nil { - return &csi.CreateVolumeResponse{}, errInternal("Timed out waiting for volume %d to be active: %v", vol.ID, err) - } - - log.V(4).Info("Volume is active", "volumeID", vol.ID) - - key := linodevolumes.CreateLinodeVolumeKey(vol.ID, vol.Label) - resp := &csi.CreateVolumeResponse{ - Volume: &csi.Volume{ - VolumeId: key.GetVolumeKey(), - CapacityBytes: size, - AccessibleTopology: []*csi.Topology{ - { - Segments: map[string]string{ - VolumeTopologyRegion: vol.Region, - }, - }, - }, - VolumeContext: volumeContext, - }, - } + // Prepare and return response + resp := cs.prepareCreateVolumeResponse(ctx, vol, size, volContext, sourceVolInfo, req.GetVolumeContentSource()) - // Append the content source to the response - if sourceVolumeInfo != nil { - resp.Volume.ContentSource = &csi.VolumeContentSource{ - Type: &csi.VolumeContentSource_Volume{ - Volume: &csi.VolumeContentSource_VolumeSource{ - VolumeId: contentSource.GetVolume().GetVolumeId(), - }, - }, - } - } log.V(2).Info("CreateVolume response", "response", resp) return resp, nil } @@ -229,105 +147,57 @@ func (cs *ControllerServer) ControllerPublishVolume(ctx context.Context, req *cs log.V(2).Info("Processing request", "req", req) - linodeID, err := linodevolumes.NodeIdAsInt("ControllerPublishVolume", req) + // Validate the request and get Linode ID and Volume ID + linodeID, volumeID, err := cs.validateControllerPublishVolumeRequest(ctx, req) if err != nil { return resp, err } - volumeID, statusErr := linodevolumes.VolumeIdAsInt("ControllerPublishVolume", req) - if statusErr != nil { + // 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, linodeID) + if err != nil { return resp, err } - - volCap := req.GetVolumeCapability() - if volCap == nil { - return resp, errNoVolumeCapability - } - if !validVolumeCapabilities([]*csi.VolumeCapability{volCap}) { - return resp, errInvalidVolumeCapability([]*csi.VolumeCapability{volCap}) + // If devicePath is not empty, the volume is already attached + if devicePath != "" { + return &csi.ControllerPublishVolumeResponse{ + PublishContext: map[string]string{ + devicePathKey: devicePath, + }, + }, nil } - volume, err := cs.client.GetVolume(ctx, volumeID) - if linodego.IsNotFound(err) { - return resp, errVolumeNotFound(volumeID) - } else if err != nil { - return resp, errInternal("get volume %d: %v", volumeID, err) - } - if volume.LinodeID != nil { - if *volume.LinodeID == linodeID { - log.V(4).Info("Volume already attached to instance", "volume_id", volume.ID, "node_id", *volume.LinodeID, "device_path", volume.FilesystemPath) - pvInfo := map[string]string{ - devicePathKey: volume.FilesystemPath, - } - resp = &csi.ControllerPublishVolumeResponse{ - PublishContext: pvInfo, - } - return resp, nil - } - return resp, errVolumeAttached(volumeID, linodeID) + // Retrieve and validate the instance associated with the Linode ID + instance, err := cs.getInstance(ctx, linodeID) + if err != nil { + return resp, err } - instance, err := cs.client.GetInstance(ctx, linodeID) - if linodego.IsNotFound(err) { - return resp, errInstanceNotFound(linodeID) - } else if err != nil { - return resp, errInternal("get linode instance %d: %v", linodeID, err) + // Check if the instance can accommodate the volume attachment + if capErr := cs.checkAttachmentCapacity(ctx, instance); capErr != nil { + return resp, capErr } - log.V(4).Info("Checking if volume can be attached", "volume_id", volumeID, "node_id", linodeID) - // Check to see if there is room to attach this volume to the instance. - var canAttach bool - if canAttach, err = cs.canAttach(ctx, instance); err != nil { - return resp, err - } else if !canAttach { - // If we can, try and add a little more information to the error message - // for the caller. - var limit int - limit, err = cs.maxAllowedVolumeAttachments(ctx, instance) - if errors.Is(err, errNilInstance) { - return resp, errInternal("cannot calculate max volume attachments for a nil instance") - } else if err != nil { - return resp, errMaxAttachments - } - return resp, errMaxVolumeAttachments(limit) - } - - // Whether or not the volume attachment should be persisted across - // boots. - // - // Setting this to true will limit the maximum number of attached - // volumes to 8 (eight), minus any instance disks, since volume - // attachments get persisted by adding them to the instance's boot - // config. - persist := false - - log.V(4).Info("Executing attach volume", "volume_id", volumeID, "node_id", linodeID) - if _, err = cs.client.AttachVolume(ctx, volumeID, &linodego.VolumeAttachOptions{ - LinodeID: linodeID, - PersistAcrossBoots: &persist, - }); err != nil { - code := codes.Internal - var apiErr *linodego.Error - if errors.As(err, &apiErr) && strings.Contains(apiErr.Message, "is already attached") { - code = codes.Unavailable // Allow a retry if the volume is already attached: race condition can occur here - } - return resp, status.Errorf(code, "attach volume: %v", err) + // Attach the volume to the specified instance + if attachErr := cs.attachVolume(ctx, volumeID, linodeID); attachErr != nil { + return resp, attachErr } 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()) if err != nil { - return resp, errInternal("wait for volume to attach: %v", err) + return resp, err } log.V(2).Info("Volume attached successfully", "volume_id", volume.ID, "node_id", *volume.LinodeID, "device_path", volume.FilesystemPath) - pvInfo := map[string]string{ - devicePathKey: volume.FilesystemPath, - } + // Return the response with the device path of the attached volume resp = &csi.ControllerPublishVolumeResponse{ - PublishContext: pvInfo, + PublishContext: map[string]string{ + devicePathKey: volume.FilesystemPath, + }, } return resp, nil } @@ -387,7 +257,7 @@ func (cs *ControllerServer) ControllerUnpublishVolume(ctx context.Context, req * // ValidateVolumeCapabilities checks if the requested volume capabilities are supported by the volume. // It returns a confirmation response if the capabilities are valid, or an error if the volume does not exist // or if no capabilities were provided. Refer to the @CSI Driver Spec for more details. -func (cs *ControllerServer) ValidateVolumeCapabilities(ctx context.Context, req *csi.ValidateVolumeCapabilitiesRequest) (*csi.ValidateVolumeCapabilitiesResponse, error) { +func (cs *ControllerServer) ValidateVolumeCapabilities(ctx context.Context, req *csi.ValidateVolumeCapabilitiesRequest) (resp *csi.ValidateVolumeCapabilitiesResponse, err error) { log, _, done := logger.GetLogger(ctx).WithMethod("ValidateVolumeCapabilities") defer done() @@ -409,7 +279,7 @@ func (cs *ControllerServer) ValidateVolumeCapabilities(ctx context.Context, req return &csi.ValidateVolumeCapabilitiesResponse{}, errInternal("get volume: %v", err) } - resp := &csi.ValidateVolumeCapabilitiesResponse{} + resp = &csi.ValidateVolumeCapabilitiesResponse{} if validVolumeCapabilities(volumeCapabilities) { resp.Confirmed = &csi.ValidateVolumeCapabilitiesResponse_Confirmed{VolumeCapabilities: volumeCapabilities} } @@ -456,8 +326,8 @@ func (cs *ControllerServer) ListVolumes(ctx context.Context, req *csi.ListVolume } entries := make([]*csi.ListVolumesResponse_Entry, 0, len(volumes)) - for volumeNum := range volumes { - key := linodevolumes.CreateLinodeVolumeKey(volumes[volumeNum].ID, volumes[volumeNum].Label) + for volNum := range volumes { + key := linodevolumes.CreateLinodeVolumeKey(volumes[volNum].ID, volumes[volNum].Label) // If the volume is attached to a Linode instance, add it to the // list. Note that in the Linode API, volumes can only be @@ -467,18 +337,18 @@ func (cs *ControllerServer) ListVolumes(ctx context.Context, req *csi.ListVolume // make(), since the CSI specification says this response field // is optional, and thus it should tolerate a nil slice. var publishedNodeIDs []string - if volumes[volumeNum].LinodeID != nil { - publishedNodeIDs = append(publishedNodeIDs, strconv.Itoa(*volumes[volumeNum].LinodeID)) + if volumes[volNum].LinodeID != nil { + publishedNodeIDs = append(publishedNodeIDs, strconv.Itoa(*volumes[volNum].LinodeID)) } entries = append(entries, &csi.ListVolumesResponse_Entry{ Volume: &csi.Volume{ VolumeId: key.GetVolumeKey(), - CapacityBytes: gbToBytes(volumes[volumeNum].Size), + CapacityBytes: gbToBytes(volumes[volNum].Size), AccessibleTopology: []*csi.Topology{ { Segments: map[string]string{ - VolumeTopologyRegion: volumes[volumeNum].Region, + VolumeTopologyRegion: volumes[volNum].Region, }, }, }, diff --git a/internal/driver/controllerserver_helper.go b/internal/driver/controllerserver_helper.go index 4ca03cd4..af2cecc4 100644 --- a/internal/driver/controllerserver_helper.go +++ b/internal/driver/controllerserver_helper.go @@ -10,6 +10,8 @@ import ( "github.com/container-storage-interface/spec/lib/go/csi" "github.com/linode/linodego" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" linodevolumes "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-volumes" "github.com/linode/linode-blockstorage-csi-driver/pkg/logger" @@ -22,10 +24,10 @@ import ( // volume endpoints deal with "GB". // Internally, the driver will deal with sizes and capacities in bytes, but // convert to and from "GB" when interacting with the Linode API. -const MinVolumeSizeBytes = 10 << 30 // 10GiB - -// Set sentinel error -var ErrNotFound = errors.New("not found") +const ( + MinVolumeSizeBytes = 10 << 30 // 10GiB + True = "true" +) // bytesToGB is a convenience function that converts the given number of bytes // to gigabytes. @@ -129,12 +131,12 @@ func (cs *ControllerServer) maxAllowedVolumeAttachments(ctx context.Context, ins // getContentSourceVolume retrieves information about the Linode volume to clone from. // It returns a LinodeVolumeKey if a valid source volume is found, or an error if the source is invalid. -func (cs *ControllerServer) getContentSourceVolume(ctx context.Context, contentSource *csi.VolumeContentSource) (*linodevolumes.LinodeVolumeKey, error) { +func (cs *ControllerServer) getContentSourceVolume(ctx context.Context, contentSource *csi.VolumeContentSource) (volKey *linodevolumes.LinodeVolumeKey, err error) { log := logger.GetLogger(ctx) log.V(4).Info("Attempting to get content source volume") if contentSource == nil { - return nil, ErrNotFound + return volKey, nil // Return nil if no content source is provided } // Check if the content source type is a volume @@ -148,18 +150,18 @@ func (cs *ControllerServer) getContentSourceVolume(ctx context.Context, contentS } // Parse the volume ID from the content source - volumeInfo, err := linodevolumes.ParseLinodeVolumeKey(sourceVolume.GetVolumeId()) + volKey, err = linodevolumes.ParseLinodeVolumeKey(sourceVolume.GetVolumeId()) if err != nil { return nil, errInternal("parse volume info from content source: %v", err) } - if volumeInfo == nil { + if volKey == nil { return nil, errInternal("processed *LinodeVolumeKey is nil") // Throw an internal error if the processed LinodeVolumeKey is nil } // Retrieve the volume data using the parsed volume ID - volumeData, err := cs.client.GetVolume(ctx, volumeInfo.VolumeID) + volumeData, err := cs.client.GetVolume(ctx, volKey.VolumeID) if err != nil { - return nil, errInternal("get volume %d: %v", volumeInfo.VolumeID, err) + return nil, errInternal("get volume %d: %v", volKey.VolumeID, err) } if volumeData == nil { return nil, errInternal("source volume *linodego.Volume is nil") // Throw an internal error if the processed linodego.Volume is nil @@ -171,7 +173,7 @@ func (cs *ControllerServer) getContentSourceVolume(ctx context.Context, contentS } log.V(4).Info("Content source volume", "volumeData", volumeData) - return volumeInfo, nil + return volKey, nil } // attemptCreateLinodeVolume creates a Linode volume while ensuring idempotency. @@ -347,7 +349,7 @@ func (cs *ControllerServer) validateCreateVolumeRequest(ctx context.Context, req defer log.V(4).Info("Exiting validateCreateVolumeRequest()") // Check if the volume name is empty; if so, return an error indicating no volume name was provided. - if len(req.GetName()) == 0 { + if req.GetName() == "" { return errNoVolumeName } @@ -369,7 +371,7 @@ func (cs *ControllerServer) validateCreateVolumeRequest(ctx context.Context, req // prepareVolumeParams prepares the volume parameters for creation. // It extracts the capacity range from the request, calculates the size, // and generates a normalized volume name. Returns the volume name and size in GB. -func (cs *ControllerServer) prepareVolumeParams(ctx context.Context, req *csi.CreateVolumeRequest) (string, int, int64, error) { +func (cs *ControllerServer) prepareVolumeParams(ctx context.Context, req *csi.CreateVolumeRequest) (volumeName string, targetSizeGB int, size int64, err error) { log := logger.GetLogger(ctx) log.V(4).Info("Entering prepareVolumeParams()", "req", req) defer log.V(4).Info("Exiting prepareVolumeParams()") @@ -377,15 +379,15 @@ func (cs *ControllerServer) prepareVolumeParams(ctx context.Context, req *csi.Cr // Retrieve the capacity range from the request to determine the size limits for the volume. capRange := req.GetCapacityRange() // Get the requested size in bytes, handling any potential errors. - size, err := getRequestCapacitySize(capRange) + size, err = getRequestCapacitySize(capRange) if err != nil { return "", 0, 0, err } - condensedName := strings.Replace(req.GetName(), "-", "", -1) + condensedName := strings.ReplaceAll(req.GetName(), "-", "") preKey := linodevolumes.CreateLinodeVolumeKey(0, condensedName) - volumeName := preKey.GetNormalizedLabelWithPrefix(cs.driver.volumeLabelPrefix) - targetSizeGB := bytesToGB(size) + volumeName = preKey.GetNormalizedLabelWithPrefix(cs.driver.volumeLabelPrefix) + targetSizeGB = bytesToGB(size) log.V(4).Info("Volume parameters prepared", "volumeName", volumeName, "targetSizeGB", targetSizeGB) return volumeName, targetSizeGB, size, nil @@ -400,11 +402,11 @@ func (cs *ControllerServer) createVolumeContext(ctx context.Context, req *csi.Cr volumeContext := make(map[string]string) - if req.Parameters[LuksEncryptedAttribute] == "true" { - volumeContext[LuksEncryptedAttribute] = "true" + if req.GetParameters()[LuksEncryptedAttribute] == True { + volumeContext[LuksEncryptedAttribute] = True volumeContext[PublishInfoVolumeName] = req.GetName() - volumeContext[LuksCipherAttribute] = req.Parameters[LuksCipherAttribute] - volumeContext[LuksKeySizeAttribute] = req.Parameters[LuksKeySizeAttribute] + volumeContext[LuksCipherAttribute] = req.GetParameters()[LuksCipherAttribute] + volumeContext[LuksKeySizeAttribute] = req.GetParameters()[LuksKeySizeAttribute] } log.V(4).Info("Volume context created", "volumeContext", volumeContext) @@ -447,7 +449,7 @@ func (cs *ControllerServer) createAndWaitForVolume(ctx context.Context, name str // prepareCreateVolumeResponse constructs a CreateVolumeResponse from the created volume details. // It includes the volume ID, capacity, accessible topology, and any relevant context or content source. -func (cs *ControllerServer) prepareCreateVolumeResponse(ctx context.Context, vol *linodego.Volume, size int64, context map[string]string, sourceInfo *linodevolumes.LinodeVolumeKey, contentSource *csi.VolumeContentSource) *csi.CreateVolumeResponse { +func (cs *ControllerServer) prepareCreateVolumeResponse(ctx context.Context, vol *linodego.Volume, size int64, volContext map[string]string, sourceInfo *linodevolumes.LinodeVolumeKey, contentSource *csi.VolumeContentSource) *csi.CreateVolumeResponse { log := logger.GetLogger(ctx) log.V(4).Info("Entering prepareCreateVolumeResponse()", "vol", vol) defer log.V(4).Info("Exiting prepareCreateVolumeResponse()") @@ -464,7 +466,7 @@ func (cs *ControllerServer) prepareCreateVolumeResponse(ctx context.Context, vol }, }, }, - VolumeContext: context, + VolumeContext: volContext, }, } @@ -485,32 +487,32 @@ func (cs *ControllerServer) prepareCreateVolumeResponse(ctx context.Context, vol // It extracts the Linode ID and Volume ID from the request and checks if the // volume capability is provided and valid. If any validation fails, it returns // an appropriate error. -func (cs *ControllerServer) validateControllerPublishVolumeRequest(ctx context.Context, req *csi.ControllerPublishVolumeRequest) (int, int, error) { +func (cs *ControllerServer) validateControllerPublishVolumeRequest(ctx context.Context, req *csi.ControllerPublishVolumeRequest) (linodeID, volumeID int, err error) { log := logger.GetLogger(ctx) log.V(4).Info("Entering validateControllerPublishVolumeRequest()", "req", req) defer log.V(4).Info("Exiting validateControllerPublishVolumeRequest()") // extract the linode ID from the request - linodeID, statusErr := linodevolumes.NodeIdAsInt("ControllerPublishVolume", req) - if statusErr != nil { - return 0, 0, statusErr + linodeID, err = linodevolumes.NodeIdAsInt("ControllerPublishVolume", req) + if err != nil { + return 0, 0, err } // extract the volume ID from the request - volumeID, statusErr := linodevolumes.VolumeIdAsInt("ControllerPublishVolume", req) - if statusErr != nil { - return 0, 0, statusErr + volumeID, err = linodevolumes.VolumeIdAsInt("ControllerPublishVolume", req) + if err != nil { + return 0, 0, err } // retrieve the volume capability from the request - cap := req.GetVolumeCapability() + volCap := req.GetVolumeCapability() // return an error if no volume capability is provided - if cap == nil { + if volCap == nil { return 0, 0, errNoVolumeCapability } // return an error if the volume capability is invalid - if !validVolumeCapabilities([]*csi.VolumeCapability{cap}) { - return 0, 0, errInvalidVolumeCapability([]*csi.VolumeCapability{cap}) + if !validVolumeCapabilities([]*csi.VolumeCapability{volCap}) { + return 0, 0, errInvalidVolumeCapability([]*csi.VolumeCapability{volCap}) } log.V(4).Info("Validation passed", "linodeID", linodeID, "volumeID", volumeID) @@ -611,7 +613,8 @@ func (cs *ControllerServer) attachVolume(ctx context.Context, volumeID, linodeID if err != nil { code := codes.Internal // Default error code is Internal. // Check if the error indicates that the volume is already attached. - if apiErr, ok := err.(*linodego.Error); ok && strings.Contains(apiErr.Message, "is already attached") { + var apiErr *linodego.Error + if errors.As(err, &apiErr) && strings.Contains(apiErr.Message, "is already attached") { code = codes.Unavailable // Allow a retry if the volume is already attached: race condition can occur here } return status.Errorf(code, "attach volume: %v", err) diff --git a/internal/driver/controllerserver_helper_test.go b/internal/driver/controllerserver_helper_test.go index c48b7a15..f6263c30 100644 --- a/internal/driver/controllerserver_helper_test.go +++ b/internal/driver/controllerserver_helper_test.go @@ -9,12 +9,13 @@ import ( "testing" "github.com/container-storage-interface/spec/lib/go/csi" - "github.com/linode/linode-blockstorage-csi-driver/mocks" - linodevolumes "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-volumes" "github.com/linode/linodego" "go.uber.org/mock/gomock" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + + "github.com/linode/linode-blockstorage-csi-driver/mocks" + linodevolumes "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-volumes" ) func TestPrepareCreateVolumeResponse(t *testing.T) { @@ -781,8 +782,8 @@ func TestCheckAttachmentCapacity(t *testing.T) { }, }, setupMocks: func() { - mockClient.EXPECT().ListInstanceDisks(gomock.Any(), 456, gomock.Any()).Return([]linodego.InstanceDisk{linodego.InstanceDisk{ID: 1}, linodego.InstanceDisk{ID: 2}}, nil).AnyTimes() - mockClient.EXPECT().ListInstanceVolumes(gomock.Any(), 456, gomock.Any()).Return([]linodego.Volume{linodego.Volume{ID: 1}, linodego.Volume{ID: 2}, linodego.Volume{ID: 3}, linodego.Volume{ID: 4}, linodego.Volume{ID: 5}, linodego.Volume{ID: 6}}, nil) + 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) }, expectedError: errMaxVolumeAttachments(6), }, @@ -825,7 +826,7 @@ func TestAttemptGetContentSourceVolume(t *testing.T) { contentSource: nil, setupMocks: func() {}, expectedResult: nil, - expectedError: nil, + expectedError: errNilSource, }, { name: "Invalid content source type", @@ -984,11 +985,12 @@ func TestAttachVolume(t *testing.T) { err := cs.attachVolume(context.Background(), tc.volumeID, tc.linodeID) - if tc.expectedError == nil && err != nil { + switch { + case tc.expectedError == nil && err != nil: t.Errorf("expected no error, got %v", err) - } else if tc.expectedError != nil && err == nil { + case tc.expectedError != nil && err == nil: t.Errorf("expected error %v, got nil", tc.expectedError) - } else if tc.expectedError != nil && err != nil { + case tc.expectedError != nil && err != nil: if tc.expectedError.Error() != err.Error() { t.Errorf("expected error %v, got %v", tc.expectedError, err) } diff --git a/internal/driver/errors.go b/internal/driver/errors.go index fdf90075..c043df8e 100644 --- a/internal/driver/errors.go +++ b/internal/driver/errors.go @@ -19,6 +19,9 @@ var ( errNoStagingTargetPath = status.Error(codes.InvalidArgument, "staging target path is not set") errNoTargetPath = status.Error(codes.InvalidArgument, "target path is not set") + // errNilSource is a general-purpose error used to indicate a nil source the volume will be created from + errNilSource = errInternal("nil source volume") + // errNilInstance is a general-purpose error used to indicate a nil // [github.com/linode/linodego.Instance] was passed as an argument to a // function.