diff --git a/pkg/csi/manila/manilaclient/client.go b/pkg/csi/manila/manilaclient/client.go index 24f0d7fbfa..20e8695d38 100644 --- a/pkg/csi/manila/manilaclient/client.go +++ b/pkg/csi/manila/manilaclient/client.go @@ -82,6 +82,10 @@ func (c Client) GrantAccess(ctx context.Context, shareID string, opts shares.Gra return shares.GrantAccess(ctx, c.c, shareID, opts).Extract() } +func (c Client) RevokeAccess(ctx context.Context, shareID string, accessID string) error { + return shares.RevokeAccess(ctx, c.c, shareID, shares.RevokeAccessOpts{AccessID: accessID}).ExtractErr() +} + func (c Client) GetSnapshotByID(ctx context.Context, snapID string) (*snapshots.Snapshot, error) { return snapshots.Get(ctx, c.c, snapID).Extract() } diff --git a/pkg/csi/manila/manilaclient/interface.go b/pkg/csi/manila/manilaclient/interface.go index a0ec8c7aa6..bcd9d2ed6d 100644 --- a/pkg/csi/manila/manilaclient/interface.go +++ b/pkg/csi/manila/manilaclient/interface.go @@ -42,6 +42,7 @@ type Interface interface { GetAccessRights(ctx context.Context, shareID string) ([]shares.AccessRight, error) GrantAccess(ctx context.Context, shareID string, opts shares.GrantAccessOptsBuilder) (*shares.AccessRight, error) + RevokeAccess(ctx context.Context, shareID string, accessID string) error GetSnapshotByID(ctx context.Context, snapID string) (*snapshots.Snapshot, error) GetSnapshotByName(ctx context.Context, snapName string) (*snapshots.Snapshot, error) diff --git a/pkg/csi/manila/shareadapters/accessrights.go b/pkg/csi/manila/shareadapters/accessrights.go new file mode 100644 index 0000000000..403bcd74a9 --- /dev/null +++ b/pkg/csi/manila/shareadapters/accessrights.go @@ -0,0 +1,94 @@ +/* +Copyright 2026 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package shareadapters + +import ( + "context" + "fmt" + "time" + + "github.com/gophercloud/gophercloud/v2/openstack/sharedfilesystems/v2/shares" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/cloud-provider-openstack/pkg/csi/manila/manilaclient" + "k8s.io/klog/v2" +) + +const ( + accessRuleStateActive = "active" + accessRuleStateError = "error" + accessRuleStateQueuedApply = "queued_to_apply" + accessRuleStateApplying = "applying" + + waitForAccessRuleTimeout = 3 + waitForAccessRuleRetries = 10 +) + +func waitForAccessRuleActive(ctx context.Context, manilaClient manilaclient.Interface, shareID, accessID string) (*shares.AccessRight, error) { + var ( + backoff = wait.Backoff{ + Duration: time.Second * waitForAccessRuleTimeout, + Factor: 1.2, + Steps: waitForAccessRuleRetries, + } + result *shares.AccessRight + ) + + err := wait.ExponentialBackoff(backoff, func() (bool, error) { + rights, err := manilaClient.GetAccessRights(ctx, shareID) + if err != nil { + return false, fmt.Errorf("failed to get access rights for share %s: %v", shareID, err) + } + + for i := range rights { + if rights[i].ID != accessID { + continue + } + + switch rights[i].State { + case accessRuleStateActive: + result = &rights[i] + return true, nil + case accessRuleStateError: + revokeErr := manilaClient.RevokeAccess(ctx, shareID, accessID) + if revokeErr != nil { + klog.Errorf("failed to revoke errored access rule %s for share %s: %v", accessID, shareID, revokeErr) + } + return false, fmt.Errorf("access rule %s for share %s is in error state", accessID, shareID) + case accessRuleStateQueuedApply, accessRuleStateApplying: + klog.V(4).Infof("access rule %s for share %s is in state %s, retrying...", accessID, shareID, rights[i].State) + return false, nil + default: + return false, fmt.Errorf("access rule %s for share %s is in unexpected state %s", accessID, shareID, rights[i].State) + } + } + + return false, fmt.Errorf("access rule %s not found for share %s", accessID, shareID) + }) + + if err != nil { + if wait.Interrupted(err) { + revokeErr := manilaClient.RevokeAccess(ctx, shareID, accessID) + if revokeErr != nil { + klog.Errorf("failed to revoke timed-out access rule %s for share %s: %v", accessID, shareID, revokeErr) + } + return nil, fmt.Errorf("timed out waiting for access rule %s for share %s to become active", accessID, shareID) + } + return nil, err + } + + return result, nil +} diff --git a/pkg/csi/manila/shareadapters/accessrights_test.go b/pkg/csi/manila/shareadapters/accessrights_test.go new file mode 100644 index 0000000000..ee046f5e26 --- /dev/null +++ b/pkg/csi/manila/shareadapters/accessrights_test.go @@ -0,0 +1,194 @@ +/* +Copyright 2026 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package shareadapters + +import ( + "context" + "strings" + "testing" + + "github.com/gophercloud/gophercloud/v2/openstack/sharedfilesystems/v2/messages" + "github.com/gophercloud/gophercloud/v2/openstack/sharedfilesystems/v2/shares" + "github.com/gophercloud/gophercloud/v2/openstack/sharedfilesystems/v2/sharetypes" + "github.com/gophercloud/gophercloud/v2/openstack/sharedfilesystems/v2/snapshots" + "k8s.io/cloud-provider-openstack/pkg/csi/manila/manilaclient" +) + +var _ manilaclient.Interface = &mockManilaClient{} + +type mockManilaClient struct { + callCount int + accessRights [][]shares.AccessRight + revokeCalled bool +} + +func (m *mockManilaClient) GetMicroversion() string { return "" } +func (m *mockManilaClient) SetMicroversion(_ string) {} +func (m *mockManilaClient) GetShareByID(_ context.Context, _ string) (*shares.Share, error) { + return nil, nil +} +func (m *mockManilaClient) GetShareByName(_ context.Context, _ string) (*shares.Share, error) { + return nil, nil +} +func (m *mockManilaClient) CreateShare(_ context.Context, _ shares.CreateOptsBuilder) (*shares.Share, error) { + return nil, nil +} +func (m *mockManilaClient) DeleteShare(_ context.Context, _ string) error { return nil } +func (m *mockManilaClient) ExtendShare(_ context.Context, _ string, _ shares.ExtendOptsBuilder) error { + return nil +} +func (m *mockManilaClient) GetExportLocations(_ context.Context, _ string) ([]shares.ExportLocation, error) { + return nil, nil +} +func (m *mockManilaClient) SetShareMetadata(_ context.Context, _ string, _ shares.SetMetadataOptsBuilder) (map[string]string, error) { + return nil, nil +} +func (m *mockManilaClient) GrantAccess(_ context.Context, _ string, _ shares.GrantAccessOptsBuilder) (*shares.AccessRight, error) { + return nil, nil +} +func (m *mockManilaClient) GetSnapshotByID(_ context.Context, _ string) (*snapshots.Snapshot, error) { + return nil, nil +} +func (m *mockManilaClient) GetSnapshotByName(_ context.Context, _ string) (*snapshots.Snapshot, error) { + return nil, nil +} +func (m *mockManilaClient) CreateSnapshot(_ context.Context, _ snapshots.CreateOptsBuilder) (*snapshots.Snapshot, error) { + return nil, nil +} +func (m *mockManilaClient) DeleteSnapshot(_ context.Context, _ string) error { return nil } +func (m *mockManilaClient) GetExtraSpecs(_ context.Context, _ string) (sharetypes.ExtraSpecs, error) { + return nil, nil +} +func (m *mockManilaClient) GetShareTypes(_ context.Context) ([]sharetypes.ShareType, error) { + return nil, nil +} +func (m *mockManilaClient) GetShareTypeIDFromName(_ context.Context, _ string) (string, error) { + return "", nil +} +func (m *mockManilaClient) GetUserMessages(_ context.Context, _ messages.ListOptsBuilder) ([]messages.Message, error) { + return nil, nil +} + +func (m *mockManilaClient) GetAccessRights(_ context.Context, _ string) ([]shares.AccessRight, error) { + idx := m.callCount + if idx >= len(m.accessRights) { + idx = len(m.accessRights) - 1 + } + m.callCount++ + return m.accessRights[idx], nil +} + +func (m *mockManilaClient) RevokeAccess(_ context.Context, _ string, _ string) error { + m.revokeCalled = true + return nil +} + +func TestWaitForAccessRuleAlreadyActive(t *testing.T) { + mock := &mockManilaClient{ + accessRights: [][]shares.AccessRight{ + { + {ID: "ar-1", ShareID: "share-1", State: "active", AccessKey: "key123"}, + }, + }, + } + + result, err := waitForAccessRuleActive(context.Background(), mock, "share-1", "ar-1") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.ID != "ar-1" { + t.Errorf("expected ID ar-1, got %s", result.ID) + } + if result.AccessKey != "key123" { + t.Errorf("expected AccessKey key123, got %s", result.AccessKey) + } + if mock.callCount != 1 { + t.Errorf("expected 1 call, got %d", mock.callCount) + } +} + +func TestWaitForAccessRuleTransientToActive(t *testing.T) { + mock := &mockManilaClient{ + accessRights: [][]shares.AccessRight{ + { + {ID: "ar-1", ShareID: "share-1", State: "queued_to_apply"}, + }, + { + {ID: "ar-1", ShareID: "share-1", State: "applying"}, + }, + { + {ID: "ar-1", ShareID: "share-1", State: "active", AccessKey: "key456"}, + }, + }, + } + + result, err := waitForAccessRuleActive(context.Background(), mock, "share-1", "ar-1") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.State != "active" { + t.Errorf("expected state active, got %s", result.State) + } + if result.AccessKey != "key456" { + t.Errorf("expected AccessKey key456, got %s", result.AccessKey) + } + if mock.callCount != 3 { + t.Errorf("expected 3 calls, got %d", mock.callCount) + } +} + +func TestWaitForAccessRuleErrorState(t *testing.T) { + mock := &mockManilaClient{ + accessRights: [][]shares.AccessRight{ + { + {ID: "ar-1", ShareID: "share-1", State: "queued_to_apply"}, + }, + { + {ID: "ar-1", ShareID: "share-1", State: "error"}, + }, + }, + } + + _, err := waitForAccessRuleActive(context.Background(), mock, "share-1", "ar-1") + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "error state") { + t.Errorf("expected 'error state' in error message, got: %s", err.Error()) + } + if !mock.revokeCalled { + t.Error("expected RevokeAccess to be called") + } +} + +func TestWaitForAccessRuleNotFound(t *testing.T) { + mock := &mockManilaClient{ + accessRights: [][]shares.AccessRight{ + { + {ID: "ar-other", ShareID: "share-1", State: "active"}, + }, + }, + } + + _, err := waitForAccessRuleActive(context.Background(), mock, "share-1", "ar-1") + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "not found") { + t.Errorf("expected 'not found' in error message, got: %s", err.Error()) + } +} diff --git a/pkg/csi/manila/shareadapters/cephfs.go b/pkg/csi/manila/shareadapters/cephfs.go index 4d3dcfe40b..5d9d9f1215 100644 --- a/pkg/csi/manila/shareadapters/cephfs.go +++ b/pkg/csi/manila/shareadapters/cephfs.go @@ -20,11 +20,9 @@ import ( "context" "fmt" "strings" - "time" "github.com/gophercloud/gophercloud/v2" "github.com/gophercloud/gophercloud/v2/openstack/sharedfilesystems/v2/shares" - "k8s.io/apimachinery/pkg/util/wait" manilautil "k8s.io/cloud-provider-openstack/pkg/csi/manila/util" "k8s.io/cloud-provider-openstack/pkg/util" "k8s.io/klog/v2" @@ -74,37 +72,12 @@ func (Cephfs) GetOrGrantAccesses(ctx context.Context, args *GrantAccessArgs) ([] if err != nil { return nil, fmt.Errorf("failed to grant access right: %v", err) } - if result.AccessKey == "" { - // Wait till a ceph key is assigned to the access right - backoff := wait.Backoff{ - Duration: time.Second * 5, - Factor: 1.2, - Steps: 10, - } - wait_err := wait.ExponentialBackoff(backoff, func() (bool, error) { - rights, err := args.ManilaClient.GetAccessRights(ctx, args.Share.ID) - if err != nil { - return false, fmt.Errorf("error get access rights for share %s: %v", args.Share.ID, err) - } - if len(rights) == 0 { - return false, fmt.Errorf("cannot find the access right we've just created") - } - for _, r := range rights { - if r.AccessTo == accessRightClient && r.AccessKey != "" { - accessRight = &r - return true, nil - } - } - klog.V(4).Infof("Access key for %s is not set yet, retrying...", accessRightClient) - return false, nil - }) - if wait_err != nil { - return nil, fmt.Errorf("timed out while attempting to get access rights for share %s: %v", args.Share.ID, err) - } + accessRight, err = waitForAccessRuleActive(ctx, args.ManilaClient, args.Share.ID, result.ID) + if err != nil { + return nil, err } } return []shares.AccessRight{*accessRight}, nil - } func (Cephfs) BuildVolumeContext(args *VolumeContextArgs) (volumeContext map[string]string, err error) { diff --git a/pkg/csi/manila/shareadapters/nfs.go b/pkg/csi/manila/shareadapters/nfs.go index c74672e77e..a92f25f580 100644 --- a/pkg/csi/manila/shareadapters/nfs.go +++ b/pkg/csi/manila/shareadapters/nfs.go @@ -65,7 +65,11 @@ func (NFS) GetOrGrantAccesses(ctx context.Context, args *GrantAccessArgs) ([]sha if err != nil { return nil, fmt.Errorf("failed to grant access right: %v", err) } - rights = append(rights, *right) + activeRight, err := waitForAccessRuleActive(ctx, args.ManilaClient, args.Share.ID, right.ID) + if err != nil { + return nil, err + } + rights = append(rights, *activeRight) } } diff --git a/tests/sanity/manila/fakemanilaclient.go b/tests/sanity/manila/fakemanilaclient.go index a1531485bb..ad37920e09 100644 --- a/tests/sanity/manila/fakemanilaclient.go +++ b/tests/sanity/manila/fakemanilaclient.go @@ -216,12 +216,27 @@ func (c fakeManilaClient) GrantAccess(_ context.Context, shareID string, opts sh accessRight.ID = intToStr(fakeAccessRightID) accessRight.ShareID = shareID + accessRight.State = "active" fakeAccessRights[fakeAccessRightID] = accessRight fakeAccessRightID++ return accessRight, nil } +func (c fakeManilaClient) RevokeAccess(_ context.Context, shareID string, accessID string) error { + if !shareExists(shareID) { + return gophercloud.ErrResourceNotFound{} + } + + id := strToInt(accessID) + if _, ok := fakeAccessRights[id]; !ok { + return gophercloud.ErrResourceNotFound{} + } + + delete(fakeAccessRights, id) + return nil +} + func (c fakeManilaClient) GetSnapshotByID(_ context.Context, snapID string) (*snapshots.Snapshot, error) { s, ok := fakeSnapshots[strToInt(snapID)] if !ok {