From f8c230a664610b3b9b4a63c89f216c7937e0565b Mon Sep 17 00:00:00 2001 From: "ske-renovate-operator[bot]" <289664651+ske-renovate-operator[bot]@users.noreply.github.com> Date: Sat, 12 Sep 2026 20:41:16 +0000 Subject: [PATCH 1/3] Update dependency chainguard-dev/apko to v1.3.0 (#1654) Co-authored-by: ske-renovate-operator[bot] <289664651+ske-renovate-operator[bot]@users.noreply.github.com> Signed-off-by: Florian Thienel --- hack/tools.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hack/tools.mk b/hack/tools.mk index 09a249d7..adb3d22d 100644 --- a/hack/tools.mk +++ b/hack/tools.mk @@ -11,7 +11,7 @@ GOLANGCI_LINT_VERSION ?= v2.13.2 # renovate: datasource=github-releases depName=uber-go/mock MOCKGEN_VERSION ?= v0.6.0 # renovate: datasource=github-releases depName=chainguard-dev/apko -APKO_VERSION ?= v1.2.45 +APKO_VERSION ?= v1.3.0 # renovate: datasource=github-releases depName=ko-build/ko KO_VERSION ?= v0.19.1 From e5e13fece7a34f59b5a28a6d847ff1880052a94e Mon Sep 17 00:00:00 2001 From: Florian Thienel Date: Mon, 14 Sep 2026 16:06:10 +0200 Subject: [PATCH 2/3] remove the client side checks that are already done on the IaaS side; handle IN_USE as useable attachment status --- pkg/csi/blockstorage/controllerserver.go | 57 +++------- pkg/csi/blockstorage/controllerserver_test.go | 53 ++++++++-- pkg/csi/blockstorage/sanity_test.go | 12 ++- pkg/stackit/client/iaas.go | 100 ++++++------------ pkg/stackit/client/iaas_test.go | 81 +++++++++++--- pkg/stackit/client/mock/iaas_mock.go | 15 ++- pkg/stackit/stackiterrors/errors.go | 9 ++ pkg/stackit/stackiterrors/errors_test.go | 29 +++++ 8 files changed, 214 insertions(+), 142 deletions(-) diff --git a/pkg/csi/blockstorage/controllerserver.go b/pkg/csi/blockstorage/controllerserver.go index fdf778d9..7981616d 100644 --- a/pkg/csi/blockstorage/controllerserver.go +++ b/pkg/csi/blockstorage/controllerserver.go @@ -351,45 +351,26 @@ func (cs *controllerServer) ControllerPublishVolume(ctx context.Context, req *cs return nil, status.Error(codes.InvalidArgument, "[ControllerPublishVolume] Volume capability must be provided") } - vol, err := cloud.GetVolume(ctx, volumeID) - if err != nil { - if stackiterrors.IsNotFound(err) { - return nil, status.Errorf(codes.NotFound, "[ControllerPublishVolume] Volume %s not found", volumeID) - } - return nil, status.Errorf(codes.Internal, "[ControllerPublishVolume] get volume failed with error %v", err) - } - - _, err = cloud.GetServer(ctx, instanceID) - if err != nil { - if stackiterrors.IsNotFound(err) { - return nil, status.Errorf(codes.NotFound, "[ControllerPublishVolume] Instance %s not found", instanceID) - } - return nil, status.Errorf(codes.Internal, "[ControllerPublishVolume] GetInstanceByID failed with error %v", err) - } - - // If Volume is already mounted to target instanceID, return OK - if vol.ServerId != nil && *vol.ServerId == instanceID { - return &csi.ControllerPublishVolumeResponse{}, nil - } - - if vol.GetStatus() != stackitclient.VolumeAvailableStatus { - return nil, status.Errorf(codes.Internal, "[ControllerPublishVolume] Volume %s is not in an READY state. Got:%s Want:%s", volumeID, vol.GetStatus(), stackitclient.VolumeAvailableStatus) - } - + // No pre-checks: IaaS validates the request (volume/server existence, state, + // attach limits) and returns the corresponding error from the attach API. payload := iaas.AddVolumeToServerPayload{ DeleteOnTermination: new(false), } - _, err = cloud.AttachVolume(ctx, instanceID, volumeID, payload) - if err != nil { - // Trigger's an immediate `NodeGetInfo` RPC call when MutableCSINodeAllocatableCount is enabled - if stackiterrors.IsTooManyDevicesError(err) { - return nil, status.Errorf(codes.ResourceExhausted, "[ControllerPublishVolume] Node can't accept any more volumes %v. All PCIe lanes are exhausted!", err) - } + switch err := cloud.AttachVolume(ctx, instanceID, volumeID, payload); { + case err == nil: + case stackiterrors.IsTooManyDevicesError(err): + return nil, status.Errorf(codes.ResourceExhausted, "[ControllerPublishVolume] Node can't accept any more volumes %v. All PCIe lanes are exhausted!", err) + case stackiterrors.IsNotFound(err): + return nil, status.Errorf(codes.NotFound, "[ControllerPublishVolume] volume %s or server %s not found: %v", volumeID, instanceID, err) + case stackiterrors.IsConflict(err): + // The attachment already exists. WaitDiskAttached verifies whether the attachment is ours before we report success. + klog.V(4).Infof("[ControllerPublishVolume] AttachVolume %s on %s conflicted, verifying attachment state: %v", volumeID, instanceID, err) + default: klog.Errorf("Failed to AttachVolume: %v", err) return nil, status.Errorf(codes.Internal, "[ControllerPublishVolume] Attach Volume failed with error %v", err) } - err = cloud.WaitDiskAttached(ctx, instanceID, volumeID) + err := cloud.WaitDiskAttached(ctx, instanceID, volumeID) if err != nil { klog.Errorf("Failed to WaitDiskAttached: %v", err) return nil, status.Errorf(codes.Internal, "[ControllerPublishVolume] failed to attach volume: %v", err) @@ -412,16 +393,10 @@ func (cs *controllerServer) ControllerUnpublishVolume(ctx context.Context, req * if volumeID == "" { return nil, status.Error(codes.InvalidArgument, "[ControllerUnpublishVolume] Volume ID must be provided") } - _, err := cloud.GetServer(ctx, instanceID) - if err != nil { - if stackiterrors.IsNotFound(err) { - klog.V(3).Infof("ControllerUnpublishVolume assuming volume %s is detached, because node %s does not exist", volumeID, instanceID) - return &csi.ControllerUnpublishVolumeResponse{}, nil - } - return nil, status.Errorf(codes.Internal, "[ControllerUnpublishVolume] GetInstanceByID failed with error %v", err) - } - err = cloud.DetachVolume(ctx, instanceID, volumeID) + // No server existence pre-check: IaaS returns not-found when the server (or the + // attachment) is already gone, which we treat as a successful detach below. + err := cloud.DetachVolume(ctx, instanceID, volumeID) if err != nil { if stackiterrors.IsNotFound(err) { klog.V(3).Infof("ControllerUnpublishVolume assuming volume %s is detached, because it does not exist", volumeID) diff --git a/pkg/csi/blockstorage/controllerserver_test.go b/pkg/csi/blockstorage/controllerserver_test.go index c3ec0fc1..537a9039 100644 --- a/pkg/csi/blockstorage/controllerserver_test.go +++ b/pkg/csi/blockstorage/controllerserver_test.go @@ -654,29 +654,53 @@ var _ = Describe("ControllerServer test", Ordered, func() { }) }) Describe("ControllerPublishVolume", func() { - It("should successfully attach volume to node", func() { + It("should attach the volume without any pre-checks", func() { req := &csi.ControllerPublishVolumeRequest{ VolumeId: "fake", NodeId: "fake", VolumeCapability: stdVolCap, } - iaasClient.EXPECT().GetVolume(gomock.Any(), req.VolumeId).Return(&iaas.Volume{Status: new("AVAILABLE")}, nil) - iaasClient.EXPECT().GetServer(gomock.Any(), "fake").Return(&iaas.Server{}, nil) - iaasClient.EXPECT().AttachVolume(gomock.Any(), req.NodeId, req.VolumeId, gomock.Any()).Return(req.VolumeId, nil) + iaasClient.EXPECT().AttachVolume(gomock.Any(), req.NodeId, req.VolumeId, gomock.Any()).Return(nil) iaasClient.EXPECT().WaitDiskAttached(gomock.Any(), req.NodeId, req.VolumeId).Return(nil) _, err := fakeCs.ControllerPublishVolume(context.Background(), req) Expect(err).To(Not(HaveOccurred())) }) + It("should verify the attachment when the attach API reports a conflict", func() { + req := &csi.ControllerPublishVolumeRequest{ + VolumeId: "fake", + NodeId: "fake", + VolumeCapability: stdVolCap, + } + iaasClient.EXPECT().AttachVolume(gomock.Any(), req.NodeId, req.VolumeId, gomock.Any()).Return(&oapierror.GenericOpenAPIError{ + StatusCode: http.StatusConflict, + }) + iaasClient.EXPECT().WaitDiskAttached(gomock.Any(), req.NodeId, req.VolumeId).Return(nil) + _, err := fakeCs.ControllerPublishVolume(context.Background(), req) + Expect(err).To(Not(HaveOccurred())) + }) + + It("should return not found when the attach API reports not found", func() { + req := &csi.ControllerPublishVolumeRequest{ + VolumeId: "fake", + NodeId: "fake", + VolumeCapability: stdVolCap, + } + iaasClient.EXPECT().AttachVolume(gomock.Any(), req.NodeId, req.VolumeId, gomock.Any()).Return(&oapierror.GenericOpenAPIError{ + StatusCode: http.StatusNotFound, + }) + _, err := fakeCs.ControllerPublishVolume(context.Background(), req) + Expect(err).To(HaveOccurred()) + Expect(status.Code(err)).To(Equal(codes.NotFound)) + }) + It("should return resource exhausted when node cannot attach more disks", func() { req := &csi.ControllerPublishVolumeRequest{ VolumeId: "fake", NodeId: "fake", VolumeCapability: stdVolCap, } - iaasClient.EXPECT().GetVolume(gomock.Any(), req.VolumeId).Return(&iaas.Volume{Status: new("AVAILABLE")}, nil) - iaasClient.EXPECT().GetServer(gomock.Any(), req.NodeId).Return(&iaas.Server{}, nil) - iaasClient.EXPECT().AttachVolume(gomock.Any(), req.NodeId, req.VolumeId, gomock.Any()).Return("", &oapierror.GenericOpenAPIError{ + iaasClient.EXPECT().AttachVolume(gomock.Any(), req.NodeId, req.VolumeId, gomock.Any()).Return(&oapierror.GenericOpenAPIError{ StatusCode: http.StatusForbidden, Body: []byte("maximum allowed number of disk devices"), }) @@ -688,17 +712,28 @@ var _ = Describe("ControllerServer test", Ordered, func() { }) }) Describe("ControllerUnpublishVolume", func() { - It("should successfully detach volume from node", func() { + It("should detach the volume without a server pre-check", func() { req := &csi.ControllerUnpublishVolumeRequest{ VolumeId: "fake", NodeId: "fake", } - iaasClient.EXPECT().GetServer(gomock.Any(), "fake").Return(&iaas.Server{}, nil) iaasClient.EXPECT().DetachVolume(gomock.Any(), req.NodeId, req.VolumeId).Return(nil) iaasClient.EXPECT().WaitDiskDetached(gomock.Any(), req.NodeId, req.VolumeId).Return(nil) _, err := fakeCs.ControllerUnpublishVolume(context.Background(), req) Expect(err).To(Not(HaveOccurred())) }) + + It("should treat a not-found detach as success", func() { + req := &csi.ControllerUnpublishVolumeRequest{ + VolumeId: "fake", + NodeId: "fake", + } + iaasClient.EXPECT().DetachVolume(gomock.Any(), req.NodeId, req.VolumeId).Return(&oapierror.GenericOpenAPIError{ + StatusCode: http.StatusNotFound, + }) + _, err := fakeCs.ControllerUnpublishVolume(context.Background(), req) + Expect(err).To(Not(HaveOccurred())) + }) }) Describe("ControllerGetVolume", func() { It("should get volume successfully", func() { diff --git a/pkg/csi/blockstorage/sanity_test.go b/pkg/csi/blockstorage/sanity_test.go index 38fcad39..8f827a2a 100644 --- a/pkg/csi/blockstorage/sanity_test.go +++ b/pkg/csi/blockstorage/sanity_test.go @@ -65,7 +65,7 @@ var _ = Describe("CSI sanity test", Ordered, func() { createdVolumes := make(map[string]*iaas.Volume) createdSnapshots := make(map[string]*iaas.Snapshot) createdBackups := make(map[string]*iaas.Backup) - createdInstances := make(map[string]*iaas.Server) + createdInstances := map[string]*iaas.Server{FakeInstanceID: {}} // --- Mock Mounter Setup --- mountPoints := make([]mountutils.MountPoint, 0) @@ -323,14 +323,18 @@ var _ = Describe("CSI sanity test", Ordered, func() { gomock.Any(), // instanceID gomock.Any(), // volumeID gomock.Any(), // payload - ).DoAndReturn(func(_ context.Context, instanceID string, volumeID string, _ iaas.AddVolumeToServerPayload) (string, error) { + ).DoAndReturn(func(_ context.Context, instanceID string, volumeID string, _ iaas.AddVolumeToServerPayload) error { + // IaaS validates volume and server existence and returns not-found for either. + if _, ok := createdInstances[instanceID]; !ok { + return &oapierror.GenericOpenAPIError{StatusCode: http.StatusNotFound} + } vol, ok := createdVolumes[volumeID] if !ok { - return "", &oapierror.GenericOpenAPIError{StatusCode: http.StatusNotFound} + return &oapierror.GenericOpenAPIError{StatusCode: http.StatusNotFound} } vol.ServerId = new(instanceID) vol.Status = new("ATTACHED") - return *vol.Id, nil + return nil }).AnyTimes() iaasClient.EXPECT().WaitDiskAttached( diff --git a/pkg/stackit/client/iaas.go b/pkg/stackit/client/iaas.go index 56ed1588..918acdfb 100644 --- a/pkg/stackit/client/iaas.go +++ b/pkg/stackit/client/iaas.go @@ -11,7 +11,6 @@ import ( sdkconfig "github.com/stackitcloud/stackit-sdk-go/core/config" iaas "github.com/stackitcloud/stackit-sdk-go/services/iaas/v2api" "k8s.io/apimachinery/pkg/util/wait" - "k8s.io/klog/v2" "k8s.io/utils/ptr" ) @@ -40,7 +39,7 @@ type IaaSClient interface { CreateVolume(ctx context.Context, payload iaas.CreateVolumePayload) (*iaas.Volume, error) DeleteVolume(ctx context.Context, volumeID string) error - AttachVolume(ctx context.Context, serverID, volumeID string, payload iaas.AddVolumeToServerPayload) (string, error) + AttachVolume(ctx context.Context, serverID, volumeID string, payload iaas.AddVolumeToServerPayload) error DetachVolume(ctx context.Context, serverID, volumeID string) error GetVolume(ctx context.Context, volumeID string) (*iaas.Volume, error) GetVolumesByName(ctx context.Context, volName string) ([]iaas.Volume, error) @@ -55,6 +54,7 @@ type IaaSClient interface { const ( VolumeAvailableStatus = "AVAILABLE" VolumeAttachedStatus = "ATTACHED" + VolumeInUseStatus = "IN_USE" operationFinishInitDelay = 1 * time.Second operationFinishFactor = 1.1 operationFinishSteps = 10 @@ -96,6 +96,9 @@ const ( var volumeErrorStates = [...]string{"ERROR", "ERROR_BACKING-UP", "ERROR_DELETING", "ERROR_RESIZING", "ERROR_RESTORING-BACKUP", "ERROR_KMS-ENCRYPTION-PARAMS"} +// volumeUsableStatuses are the volume statuses that mark an attachment as usable on the node. +var volumeUsableStatuses = []string{VolumeAttachedStatus, VolumeInUseStatus} + func NewIaaSClient(region, projectID string, options []sdkconfig.ConfigurationOption) (IaaSClient, error) { apiClient, err := iaas.NewAPIClient(options...) if err != nil { @@ -361,28 +364,16 @@ func (i *iaasClient) DeleteVolume(ctx context.Context, volumeID string) error { return err } -func (i *iaasClient) AttachVolume(ctx context.Context, serverID, volumeID string, payload iaas.AddVolumeToServerPayload) (string, error) { - volume, err := i.GetVolume(ctx, volumeID) - if err != nil { - return "", err - } - - if volume.ServerId != nil && serverID == *volume.ServerId { - klog.V(4).Infof("Disk %s is already attached to instance %s", volumeID, serverID) - return *volume.Id, nil - } - - _, err = withResponseID(ctx, func(ctx context.Context) (any, error) { +// AttachVolume attaches a volume to a server. It performs no pre-checks: IaaS +// validates the request and returns the corresponding error from the create API. +func (i *iaasClient) AttachVolume(ctx context.Context, serverID, volumeID string, payload iaas.AddVolumeToServerPayload) error { + _, err := withResponseID(ctx, func(ctx context.Context) (any, error) { return i.Client. AddVolumeToServer(ctx, i.projectID, i.region, serverID, volumeID). AddVolumeToServerPayload(payload). Execute() }) - if err != nil { - return "", err - } - - return volume.GetId(), nil + return err } func (i *iaasClient) GetVolume(ctx context.Context, volumeID string) (*iaas.Volume, error) { @@ -471,13 +462,19 @@ func (i *iaasClient) WaitDiskAttached(ctx context.Context, instanceID, volumeID } err := wait.ExponentialBackoff(backoff, func() (bool, error) { - attached, err := i.diskIsAttached(ctx, instanceID, volumeID) - if err != nil && !stackiterrors.IsNotFound(err) { - // if this is a race condition indicate the volume is deleted - // during sleep phase, ignore the error and return attach=false - return false, err + volume, err := i.GetVolume(ctx, volumeID) + if err != nil { + // A volume deleted during the sleep phase must not abort the loop as a + // hard error; ignore the not-found and keep polling until the timeout. + return false, stackiterrors.IgnoreNotFound(err) + } + if serverID := volume.GetServerId(); serverID != "" && serverID != instanceID { + return false, fmt.Errorf("volume %s is attached to server %s, not %s", volumeID, serverID, instanceID) + } + if slices.Contains(volumeErrorStates[:], volume.GetStatus()) { + return false, fmt.Errorf("volume %s is in error state %s", volumeID, volume.GetStatus()) } - return attached, nil + return volume.GetServerId() == instanceID && slices.Contains(volumeUsableStatuses, volume.GetStatus()), nil }) if wait.Interrupted(err) { @@ -495,11 +492,11 @@ func (i *iaasClient) WaitDiskDetached(ctx context.Context, instanceID, volumeID } err := wait.ExponentialBackoff(backoff, func() (bool, error) { - attached, err := i.diskIsAttached(ctx, instanceID, volumeID) + volume, err := i.GetVolume(ctx, volumeID) if err != nil { return false, err } - return !attached, nil + return volume.GetServerId() != instanceID, nil }) if wait.Interrupted(err) { @@ -509,37 +506,17 @@ func (i *iaasClient) WaitDiskDetached(ctx context.Context, instanceID, volumeID return err } +// DetachVolume detaches a volume from a server. It performs no pre-checks: IaaS +// handles the state validation and returns not-found when the attachment is gone. func (i *iaasClient) DetachVolume(ctx context.Context, serverID, volumeID string) error { - volume, err := i.GetVolume(ctx, volumeID) - if err != nil { - return err - } - - if *volume.Status == VolumeAvailableStatus { - klog.V(2).Infof("Volume: %s has been detached from compute: %s ", *volume.Id, serverID) - return nil - } - - if *volume.Status != VolumeAttachedStatus { - return fmt.Errorf("can not detach volume %s, its status is %s", *volume.Name, *volume.Status) - } - - if volume.ServerId != nil && *volume.ServerId == serverID { - _, err := withResponseID(ctx, func(ctx context.Context) (any, error) { - err := i.Client.RemoveVolumeFromServer(ctx, i.projectID, i.region, serverID, volumeID).Execute() - if err != nil { - return nil, fmt.Errorf("failed to detach volume %s from compute %s : %w", *volume.Name, serverID, err) - } - return nil, nil - }) + _, err := withResponseID(ctx, func(ctx context.Context) (any, error) { + err := i.Client.RemoveVolumeFromServer(ctx, i.projectID, i.region, serverID, volumeID).Execute() if err != nil { - return err + return nil, fmt.Errorf("failed to detach volume %s from compute %s : %w", volumeID, serverID, err) } - - klog.V(2).Infof("Successfully detached volume: %s from compute: %s", *volume.Id, serverID) - } - - return nil + return nil, nil + }) + return err } func (i *iaasClient) WaitVolumeTargetStatusWithCustomBackoff(ctx context.Context, volumeID string, tStatus []string, backoff *wait.Backoff) error { @@ -566,19 +543,6 @@ func (i *iaasClient) WaitVolumeTargetStatusWithCustomBackoff(ctx context.Context return waitErr } -// diskIsAttached queries if a volume is attached to a compute instance -func (i *iaasClient) diskIsAttached(ctx context.Context, instanceID, volumeID string) (bool, error) { - volume, err := i.GetVolume(ctx, volumeID) - if err != nil { - return false, err - } - - if volume.ServerId != nil && *volume.ServerId == instanceID { - return true, nil - } - return false, nil -} - // diskIsUsed returns true whether a disk is attached to any node func (i *iaasClient) diskIsUsed(ctx context.Context, volumeID string) (bool, error) { volume, err := i.GetVolume(ctx, volumeID) diff --git a/pkg/stackit/client/iaas_test.go b/pkg/stackit/client/iaas_test.go index 172b15d6..70258efd 100644 --- a/pkg/stackit/client/iaas_test.go +++ b/pkg/stackit/client/iaas_test.go @@ -538,29 +538,33 @@ var _ = Describe("Volume", func() { }) Context("Attach/Detach Volume", func() { - It("AttachVolume calls API when not already attached", func() { - mockIaaSClient.EXPECT().GetVolume(gomock.Any(), gomock.Any(), gomock.Any(), volumeID). - Return(iaas.ApiGetVolumeRequest{ApiService: mockIaaSClient}) - mockIaaSClient.EXPECT().GetVolumeExecute(gomock.Any()).Return(&iaas.Volume{Id: new(volumeID), ServerId: nil}, nil) - + It("AttachVolume calls the API directly without any pre-checks", func() { mockIaaSClient.EXPECT().AddVolumeToServer(gomock.Any(), gomock.Any(), gomock.Any(), serverID, volumeID). Return(iaas.ApiAddVolumeToServerRequest{ApiService: mockIaaSClient}) mockIaaSClient.EXPECT().AddVolumeToServerExecute(gomock.Any()).Return( &iaas.VolumeAttachment{VolumeId: new(volumeID), ServerId: new(serverID)}, nil) - id, err := client.AttachVolume(context.Background(), serverID, volumeID, iaas.AddVolumeToServerPayload{}) + err := client.AttachVolume(context.Background(), serverID, volumeID, iaas.AddVolumeToServerPayload{}) Expect(err).ToNot(HaveOccurred()) - Expect(id).To(Equal(volumeID)) }) - It("DetachVolume fails if status is not Available", func() { - mockIaaSClient.EXPECT().GetVolume(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). - Return(iaas.ApiGetVolumeRequest{ApiService: mockIaaSClient}) - mockIaaSClient.EXPECT().GetVolumeExecute(gomock.Any()).Return(&iaas.Volume{Name: new("volume-1"), Id: new(volumeID), Status: new("CREATING")}, nil) + It("DetachVolume calls the API directly without any pre-checks", func() { + mockIaaSClient.EXPECT().RemoveVolumeFromServer(gomock.Any(), gomock.Any(), gomock.Any(), serverID, volumeID). + Return(iaas.ApiRemoveVolumeFromServerRequest{ApiService: mockIaaSClient}) + mockIaaSClient.EXPECT().RemoveVolumeFromServerExecute(gomock.Any()).Return(nil) + + err := client.DetachVolume(context.Background(), serverID, volumeID) + Expect(err).ToNot(HaveOccurred()) + }) + + It("DetachVolume wraps the API error", func() { + mockIaaSClient.EXPECT().RemoveVolumeFromServer(gomock.Any(), gomock.Any(), gomock.Any(), serverID, volumeID). + Return(iaas.ApiRemoveVolumeFromServerRequest{ApiService: mockIaaSClient}) + mockIaaSClient.EXPECT().RemoveVolumeFromServerExecute(gomock.Any()).Return(fmt.Errorf("boom")) err := client.DetachVolume(context.Background(), serverID, volumeID) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("its status is CREATING")) + Expect(err.Error()).To(ContainSubstring("failed to detach volume")) }) }) @@ -604,5 +608,58 @@ var _ = Describe("Volume", func() { err := client.WaitDiskAttached(context.Background(), serverID, volumeID) Expect(err).To(HaveOccurred()) }) + + DescribeTable("WaitDiskAttached succeeds once the volume is attached and usable", + func(status string) { + mockIaaSClient.EXPECT().GetVolume(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(iaas.ApiGetVolumeRequest{ApiService: mockIaaSClient}) + mockIaaSClient.EXPECT().GetVolumeExecute(gomock.Any()). + Return(&iaas.Volume{Id: new(volumeID), ServerId: new(serverID), Status: new(status)}, nil) + + Expect(client.WaitDiskAttached(context.Background(), serverID, volumeID)).To(Succeed()) + }, + Entry("ATTACHED", VolumeAttachedStatus), + Entry("IN_USE", VolumeInUseStatus), + ) + + It("WaitDiskAttached fails fast when the volume is attached to another server", func() { + mockIaaSClient.EXPECT().GetVolume(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(iaas.ApiGetVolumeRequest{ApiService: mockIaaSClient}) + mockIaaSClient.EXPECT().GetVolumeExecute(gomock.Any()). + Return(&iaas.Volume{Id: new(volumeID), ServerId: new("other-server"), Status: new(VolumeAttachedStatus)}, nil) + + err := client.WaitDiskAttached(context.Background(), serverID, volumeID) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("attached to server other-server")) + }) + + It("WaitDiskAttached fails fast when the volume is in an error state", func() { + mockIaaSClient.EXPECT().GetVolume(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(iaas.ApiGetVolumeRequest{ApiService: mockIaaSClient}) + mockIaaSClient.EXPECT().GetVolumeExecute(gomock.Any()). + Return(&iaas.Volume{Id: new(volumeID), ServerId: new(serverID), Status: new("ERROR")}, nil) + + err := client.WaitDiskAttached(context.Background(), serverID, volumeID) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("error state ERROR")) + }) + + It("WaitDiskDetached succeeds once the server association is cleared", func() { + mockIaaSClient.EXPECT().GetVolume(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(iaas.ApiGetVolumeRequest{ApiService: mockIaaSClient}) + mockIaaSClient.EXPECT().GetVolumeExecute(gomock.Any()). + Return(&iaas.Volume{Id: new(volumeID), ServerId: nil, Status: new(VolumeAvailableStatus)}, nil) + + Expect(client.WaitDiskDetached(context.Background(), serverID, volumeID)).To(Succeed()) + }) + + It("WaitDiskDetached propagates a GetVolume error", func() { + mockIaaSClient.EXPECT().GetVolume(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(iaas.ApiGetVolumeRequest{ApiService: mockIaaSClient}) + mockIaaSClient.EXPECT().GetVolumeExecute(gomock.Any()).Return(nil, fmt.Errorf("boom")) + + err := client.WaitDiskDetached(context.Background(), serverID, volumeID) + Expect(err).To(HaveOccurred()) + }) }) }) diff --git a/pkg/stackit/client/mock/iaas_mock.go b/pkg/stackit/client/mock/iaas_mock.go index a538deb4..345f089d 100644 --- a/pkg/stackit/client/mock/iaas_mock.go +++ b/pkg/stackit/client/mock/iaas_mock.go @@ -43,12 +43,11 @@ func (m *MockIaaSClient) EXPECT() *MockIaaSClientMockRecorder { } // AttachVolume mocks base method. -func (m *MockIaaSClient) AttachVolume(ctx context.Context, serverID, volumeID string, payload v2api.AddVolumeToServerPayload) (string, error) { +func (m *MockIaaSClient) AttachVolume(ctx context.Context, serverID, volumeID string, payload v2api.AddVolumeToServerPayload) error { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "AttachVolume", ctx, serverID, volumeID, payload) - ret0, _ := ret[0].(string) - ret1, _ := ret[1].(error) - return ret0, ret1 + ret0, _ := ret[0].(error) + return ret0 } // AttachVolume indicates an expected call of AttachVolume. @@ -64,19 +63,19 @@ type MockIaaSClientAttachVolumeCall struct { } // Return rewrite *gomock.Call.Return -func (c *MockIaaSClientAttachVolumeCall) Return(arg0 string, arg1 error) *MockIaaSClientAttachVolumeCall { - c.Call = c.Call.Return(arg0, arg1) +func (c *MockIaaSClientAttachVolumeCall) Return(arg0 error) *MockIaaSClientAttachVolumeCall { + c.Call = c.Call.Return(arg0) return c } // Do rewrite *gomock.Call.Do -func (c *MockIaaSClientAttachVolumeCall) Do(f func(context.Context, string, string, v2api.AddVolumeToServerPayload) (string, error)) *MockIaaSClientAttachVolumeCall { +func (c *MockIaaSClientAttachVolumeCall) Do(f func(context.Context, string, string, v2api.AddVolumeToServerPayload) error) *MockIaaSClientAttachVolumeCall { c.Call = c.Call.Do(f) return c } // DoAndReturn rewrite *gomock.Call.DoAndReturn -func (c *MockIaaSClientAttachVolumeCall) DoAndReturn(f func(context.Context, string, string, v2api.AddVolumeToServerPayload) (string, error)) *MockIaaSClientAttachVolumeCall { +func (c *MockIaaSClientAttachVolumeCall) DoAndReturn(f func(context.Context, string, string, v2api.AddVolumeToServerPayload) error) *MockIaaSClientAttachVolumeCall { c.Call = c.Call.DoAndReturn(f) return c } diff --git a/pkg/stackit/stackiterrors/errors.go b/pkg/stackit/stackiterrors/errors.go index 1b1f127a..76c17977 100644 --- a/pkg/stackit/stackiterrors/errors.go +++ b/pkg/stackit/stackiterrors/errors.go @@ -61,6 +61,15 @@ func IsInvalidError(err error) bool { return oAPIError.StatusCode == http.StatusBadRequest } +func IsConflict(err error) bool { + oAPIError, ok := genericOpenAPIError(err) + if !ok { + return false + } + + return oAPIError.StatusCode == http.StatusConflict +} + func genericOpenAPIError(err error) (*oapiError.GenericOpenAPIError, bool) { var oAPIError *oapiError.GenericOpenAPIError if ok := errors.As(err, &oAPIError); !ok { diff --git a/pkg/stackit/stackiterrors/errors_test.go b/pkg/stackit/stackiterrors/errors_test.go index e3be05b8..2090947c 100644 --- a/pkg/stackit/stackiterrors/errors_test.go +++ b/pkg/stackit/stackiterrors/errors_test.go @@ -123,4 +123,33 @@ var _ = Describe("Errors", func() { }) }) }) + + Describe("IsConflict", func() { + Context("when error is a Conflict error", func() { + It("should return true", func() { + err := &oapiError.GenericOpenAPIError{StatusCode: http.StatusConflict} + Expect(IsConflict(err)).To(BeTrue()) + }) + }) + + Context("when error is not a Conflict error", func() { + It("should return false", func() { + err := &oapiError.GenericOpenAPIError{StatusCode: http.StatusInternalServerError} + Expect(IsConflict(err)).To(BeFalse()) + }) + }) + + Context("when error is not an OAPI error", func() { + It("should return false", func() { + err := errors.New("some error") + Expect(IsConflict(err)).To(BeFalse()) + }) + }) + + Context("when error is nil", func() { + It("should return false", func() { + Expect(IsConflict(nil)).To(BeFalse()) + }) + }) + }) }) From 257a964ecd0d9b26c36c06a2ba2bab0817511ed6 Mon Sep 17 00:00:00 2001 From: Florian Thienel Date: Tue, 15 Sep 2026 13:55:58 +0200 Subject: [PATCH 3/3] do not set encryptionConfig when copying from backup, snapshot, or volume source --- pkg/csi/blockstorage/controllerserver.go | 8 +- pkg/csi/blockstorage/controllerserver_test.go | 74 +++++++++++++++++++ 2 files changed, 78 insertions(+), 4 deletions(-) diff --git a/pkg/csi/blockstorage/controllerserver.go b/pkg/csi/blockstorage/controllerserver.go index 7981616d..06ab8692 100644 --- a/pkg/csi/blockstorage/controllerserver.go +++ b/pkg/csi/blockstorage/controllerserver.go @@ -237,10 +237,10 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol } } - // The encryption config is already set for volumes created from snapshot or volume. We MUST never set it when - // restoring from snapshot or volume. - // This is not true for volumeSourceType == Backup. The encryptionConfig must be set BUT the parameters can be different. - if volParams.Encrypted != nil && (volumeSourceType == "" || volumeSourceType == stackitclient.BackupSource) { + // A volume created from a content source (backup, snapshot or volume) inherits its + // encryption from that source; IaaS sets it. We MUST never send EncryptionParameters + // for such a restore. Only a fresh volume (i.e. without a source source) takes encryption parameters. + if volParams.Encrypted != nil && volumeSourceType == "" { encrypted, err := strconv.ParseBool(*volParams.Encrypted) if err != nil { return nil, status.Error(codes.InvalidArgument, "parameter encrypted must be of type boolean") diff --git a/pkg/csi/blockstorage/controllerserver_test.go b/pkg/csi/blockstorage/controllerserver_test.go index 537a9039..b6ada3b6 100644 --- a/pkg/csi/blockstorage/controllerserver_test.go +++ b/pkg/csi/blockstorage/controllerserver_test.go @@ -82,6 +82,80 @@ var _ = Describe("ControllerServer test", Ordered, func() { Expect(resp.Volume.CapacityBytes).To(Equal(util.GIBIBYTE * 20)) }) + It("should set encryption parameters for a fresh encrypted volume", func() { + req := &csi.CreateVolumeRequest{ + Name: "encrypted volume", + VolumeCapabilities: stdVolCaps, + CapacityRange: stdCapRange, + Parameters: map[string]string{ + "encrypted": "true", + "type": "perf1", + "kmsServiceAccount": "sa", + "kmsKeyID": "kid", + "kmsKeyringID": "krid", + "kmsKeyVersion": "1", + }, + } + + iaasClient.EXPECT().GetVolumesByName(gomock.Any(), "encrypted volume").Return([]iaas.Volume{}, nil) + + var captured iaas.CreateVolumePayload + iaasClient.EXPECT().CreateVolume(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, payload iaas.CreateVolumePayload) (*iaas.Volume, error) { + captured = payload + return &iaas.Volume{Id: new("volume-id"), Size: new(int64(20))}, nil + }) + iaasClient.EXPECT().WaitVolumeTargetStatusWithCustomBackoff(gomock.Any(), "volume-id", gomock.Any(), gomock.Any()).Return(nil) + + _, err := fakeCs.CreateVolume(context.Background(), req) + Expect(err).ToNot(HaveOccurred()) + Expect(captured.Source).To(BeNil()) + Expect(captured.EncryptionParameters).ToNot(BeNil()) + Expect(captured.EncryptionParameters.KekKeyId).To(Equal("kid")) + }) + + It("should never set encryption parameters when restoring from a backup source", func() { + req := &csi.CreateVolumeRequest{ + Name: "backup restore", + VolumeCapabilities: stdVolCaps, + CapacityRange: stdCapRange, + Parameters: map[string]string{ + "encrypted": "true", + "type": "perf1", + "kmsServiceAccount": "sa", + "kmsKeyID": "kid", + "kmsKeyringID": "krid", + "kmsKeyVersion": "1", + }, + VolumeContentSource: &csi.VolumeContentSource{ + Type: &csi.VolumeContentSource_Snapshot{ + Snapshot: &csi.VolumeContentSource_SnapshotSource{SnapshotId: "source-id"}, + }, + }, + } + + iaasClient.EXPECT().GetVolumesByName(gomock.Any(), "backup restore").Return([]iaas.Volume{}, nil) + // Snapshot lookup misses, so the source is resolved as a backup. + iaasClient.EXPECT().GetSnapshot(gomock.Any(), "source-id"). + Return(nil, &oapierror.GenericOpenAPIError{StatusCode: http.StatusNotFound}) + iaasClient.EXPECT().GetBackup(gomock.Any(), "source-id"). + Return(&iaas.Backup{Id: new("source-id"), Status: new(stackitclient.SnapshotReadyStatus)}, nil) + + var captured iaas.CreateVolumePayload + iaasClient.EXPECT().CreateVolume(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, payload iaas.CreateVolumePayload) (*iaas.Volume, error) { + captured = payload + return &iaas.Volume{Id: new("volume-id"), Size: new(int64(20))}, nil + }) + iaasClient.EXPECT().WaitVolumeTargetStatusWithCustomBackoff(gomock.Any(), "volume-id", gomock.Any(), gomock.Any()).Return(nil) + + _, err := fakeCs.CreateVolume(context.Background(), req) + Expect(err).ToNot(HaveOccurred()) + Expect(captured.Source).ToNot(BeNil()) + Expect(captured.Source.Type).To(Equal(string(stackitclient.BackupSource))) + Expect(captured.EncryptionParameters).To(BeNil()) + }) + It("should not accept an empty volume name", func() { req := &csi.CreateVolumeRequest{ Name: "",