From eefd638556153c57ef5ab81eae22c585165440ab Mon Sep 17 00:00:00 2001 From: Goutham Pacha Ravi Date: Fri, 19 Jun 2026 12:46:55 -0700 Subject: [PATCH 1/2] [manila-csi-plugin] Allow shareAccessID and shareAccessIDs to coexist Remove the precludes relationship between shareAccessID and shareAccessIDs so that both parameters can be set simultaneously. When both are present, shareAccessIDs takes precedence (existing behavior in getAccessIDs). This restores backwards compatibility for consumers that set both fields during migration from the deprecated shareAccessID to shareAccessIDs. Fixes: https://github.com/kubernetes/cloud-provider-openstack/issues/3131 --- pkg/csi/manila/adapters_test.go | 66 +++++++++++++++++ pkg/csi/manila/options/shareoptions.go | 4 +- pkg/csi/manila/options/shareoptions_test.go | 82 +++++++++++++++++++++ 3 files changed, 150 insertions(+), 2 deletions(-) create mode 100644 pkg/csi/manila/adapters_test.go create mode 100644 pkg/csi/manila/options/shareoptions_test.go diff --git a/pkg/csi/manila/adapters_test.go b/pkg/csi/manila/adapters_test.go new file mode 100644 index 0000000000..c3d4b15c82 --- /dev/null +++ b/pkg/csi/manila/adapters_test.go @@ -0,0 +1,66 @@ +/* +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 manila + +import ( + "testing" + + "k8s.io/cloud-provider-openstack/pkg/csi/manila/options" +) + +func TestGetAccessIDs(t *testing.T) { + tests := []struct { + name string + opts *options.NodeVolumeContext + expected []string + }{ + { + name: "shareAccessIDs takes precedence over shareAccessID", + opts: &options.NodeVolumeContext{ShareAccessIDs: "new-1,new-2", ShareAccessID: "old-1"}, + expected: []string{"new-1", "new-2"}, + }, + { + name: "only shareAccessIDs", + opts: &options.NodeVolumeContext{ShareAccessIDs: "id-1,id-2,id-3"}, + expected: []string{"id-1", "id-2", "id-3"}, + }, + { + name: "only shareAccessID (deprecated)", + opts: &options.NodeVolumeContext{ShareAccessID: "id-1"}, + expected: []string{"id-1"}, + }, + { + name: "neither set", + opts: &options.NodeVolumeContext{}, + expected: nil, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + result := getAccessIDs(tc.opts) + if len(result) != len(tc.expected) { + t.Fatalf("got %v, want %v", result, tc.expected) + } + for i := range result { + if result[i] != tc.expected[i] { + t.Errorf("index %d: got %q, want %q", i, result[i], tc.expected[i]) + } + } + }) + } +} diff --git a/pkg/csi/manila/options/shareoptions.go b/pkg/csi/manila/options/shareoptions.go index af6668e9b6..fb3c2950dc 100644 --- a/pkg/csi/manila/options/shareoptions.go +++ b/pkg/csi/manila/options/shareoptions.go @@ -43,8 +43,8 @@ type ControllerVolumeContext struct { type NodeVolumeContext struct { ShareID string `name:"shareID" value:"optionalIf:shareName=." precludes:"shareName"` ShareName string `name:"shareName" value:"optionalIf:shareID=." precludes:"shareID"` - ShareAccessID string `name:"shareAccessID" value:"optionalIf:shareAccessIDs=." precludes:"shareAccessIDs"` // Keep this for backwards compatibility - ShareAccessIDs string `name:"shareAccessIDs" value:"optionalIf:shareAccessID=." precludes:"shareAccessID"` + ShareAccessID string `name:"shareAccessID" value:"optional"` // Deprecated: use shareAccessIDs + ShareAccessIDs string `name:"shareAccessIDs" value:"optional"` // Preferred; takes precedence over shareAccessID // Adapter options diff --git a/pkg/csi/manila/options/shareoptions_test.go b/pkg/csi/manila/options/shareoptions_test.go new file mode 100644 index 0000000000..f7826e5a10 --- /dev/null +++ b/pkg/csi/manila/options/shareoptions_test.go @@ -0,0 +1,82 @@ +/* +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 options + +import ( + "testing" +) + +func TestNewNodeVolumeContextShareAccessIDBackwardsCompat(t *testing.T) { + tests := []struct { + name string + data map[string]string + wantAccessID string + wantAccessIDs string + wantErr bool + }{ + { + name: "only shareAccessIDs set", + data: map[string]string{ + "shareID": "share-1", + "shareAccessIDs": "access-1,access-2", + }, + wantAccessIDs: "access-1,access-2", + }, + { + name: "only shareAccessID set (deprecated)", + data: map[string]string{ + "shareID": "share-1", + "shareAccessID": "access-1", + }, + wantAccessID: "access-1", + }, + { + name: "both set, should not error", + data: map[string]string{ + "shareID": "share-1", + "shareAccessID": "access-old", + "shareAccessIDs": "access-new-1,access-new-2", + }, + wantAccessID: "access-old", + wantAccessIDs: "access-new-1,access-new-2", + }, + { + name: "neither set", + data: map[string]string{ + "shareID": "share-1", + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + opts, err := NewNodeVolumeContext(tc.data) + if (err != nil) != tc.wantErr { + t.Fatalf("unexpected error: %v", err) + } + if tc.wantErr { + return + } + if opts.ShareAccessID != tc.wantAccessID { + t.Errorf("ShareAccessID = %q, want %q", opts.ShareAccessID, tc.wantAccessID) + } + if opts.ShareAccessIDs != tc.wantAccessIDs { + t.Errorf("ShareAccessIDs = %q, want %q", opts.ShareAccessIDs, tc.wantAccessIDs) + } + }) + } +} From 653de24498f954447cd972ff0708e431d5112d90 Mon Sep 17 00:00:00 2001 From: Goutham Pacha Ravi Date: Mon, 22 Jun 2026 23:57:23 -0700 Subject: [PATCH 2/2] [manila-csi-plugin] Restore --nodeid and --nodeaz as optional overrides Un-deprecate the --nodeid and --nodeaz flags so that they can be used as alternatives to the metadata service for node identity. When set, the flag values are used directly; when unset, the driver falls back to the metadata service as before. This allows Manila CSI to work on nodes that are not OpenStack VMs, such as bare metal clusters or edge deployments, where the metadata service is unavailable. --- cmd/manila-csi-plugin/main.go | 21 ++--- pkg/csi/manila/driver.go | 45 ++++++++- pkg/csi/manila/metadata_test.go | 146 +++++++++++++++++++++++++++++ tests/sanity/manila/sanity_test.go | 2 +- 4 files changed, 199 insertions(+), 15 deletions(-) create mode 100644 pkg/csi/manila/metadata_test.go diff --git a/cmd/manila-csi-plugin/main.go b/cmd/manila-csi-plugin/main.go index f1505b3f98..7557424f53 100644 --- a/cmd/manila-csi-plugin/main.go +++ b/cmd/manila-csi-plugin/main.go @@ -88,6 +88,8 @@ func main() { ManilaClientBuilder: manilaClientBuilder, CSIClientBuilder: csiClientBuilder, ClusterID: clusterID, + NodeID: nodeID, + NodeAZ: nodeAZ, PVCLister: csi.GetPVCLister(), } @@ -104,10 +106,12 @@ func main() { } if provideNodeService { - // Initialize metadata - metadata := metadata.GetMetadataProvider("") + var md metadata.IMetadata + if nodeID == "" || (withTopology && nodeAZ == "") { + md = metadata.GetMetadataProvider("") + } - err = d.SetupNodeService(metadata) + err = d.SetupNodeService(nodeID, nodeAZ, md) if err != nil { klog.Fatalf("Driver node service initialization failed: %v", err) } @@ -126,15 +130,8 @@ func main() { cmd.PersistentFlags().StringVar(&driverName, "drivername", "manila.csi.openstack.org", "name of the driver") - cmd.PersistentFlags().StringVar(&nodeID, "nodeid", "", "this node's ID") - if err := cmd.PersistentFlags().MarkDeprecated("nodeid", "This option is now ignored by the driver. It will be removed in a future release."); err != nil { - klog.Fatalf("Unable to mark flag nodeid to be deprecated: %v", err) - } - - cmd.PersistentFlags().StringVar(&nodeAZ, "nodeaz", "", "this node's availability zone") - if err := cmd.PersistentFlags().MarkDeprecated("nodeaz", "This option is now ignored by the driver. It will be removed in a future release."); err != nil { - klog.Fatalf("Unable to mark flag nodeaz to be deprecated: %v", err) - } + cmd.PersistentFlags().StringVar(&nodeID, "nodeid", "", "this node's ID. When set, the metadata service is not used to retrieve the node ID.") + cmd.PersistentFlags().StringVar(&nodeAZ, "nodeaz", "", "this node's availability zone. When set, the metadata service is not used to retrieve the availability zone.") cmd.PersistentFlags().StringVar(&runtimeConfigFile, "runtime-config-file", "", "path to the runtime configuration file") diff --git a/pkg/csi/manila/driver.go b/pkg/csi/manila/driver.go index 30d1b1f5d3..04603106ad 100644 --- a/pkg/csi/manila/driver.go +++ b/pkg/csi/manila/driver.go @@ -72,12 +72,43 @@ type DriverOpts struct { ServerCSIEndpoint string FwdCSIEndpoint string + NodeID string + NodeAZ string + ManilaClientBuilder manilaclient.Builder CSIClientBuilder csiclient.Builder PVCLister v1.PersistentVolumeClaimLister } +type staticMetadata struct { + nodeID string + nodeAZ string +} + +func (m *staticMetadata) GetInstanceID() (string, error) { return m.nodeID, nil } +func (m *staticMetadata) GetAvailabilityZone() (string, error) { return m.nodeAZ, nil } + +type overrideMetadata struct { + nodeID string + nodeAZ string + fallback metadata.IMetadata +} + +func (m *overrideMetadata) GetInstanceID() (string, error) { + if m.nodeID != "" { + return m.nodeID, nil + } + return m.fallback.GetInstanceID() +} + +func (m *overrideMetadata) GetAvailabilityZone() (string, error) { + if m.nodeAZ != "" { + return m.nodeAZ, nil + } + return m.fallback.GetAvailabilityZone() +} + type nonBlockingGRPCServer struct { wg sync.WaitGroup server *grpc.Server @@ -174,9 +205,19 @@ func (d *Driver) SetupControllerService() error { return nil } -func (d *Driver) SetupNodeService(metadata metadata.IMetadata) error { +func (d *Driver) SetupNodeService(nodeID, nodeAZ string, md metadata.IMetadata) error { klog.Info("Providing node service") + var effectiveMD metadata.IMetadata + switch { + case nodeID != "" && nodeAZ != "": + effectiveMD = &staticMetadata{nodeID: nodeID, nodeAZ: nodeAZ} + case nodeID != "" || nodeAZ != "": + effectiveMD = &overrideMetadata{nodeID: nodeID, nodeAZ: nodeAZ, fallback: md} + default: + effectiveMD = md + } + var supportsNodeStage bool nodeCapsMap, err := d.initProxiedDriver() @@ -196,7 +237,7 @@ func (d *Driver) SetupNodeService(metadata metadata.IMetadata) error { d.ns = &nodeServer{ d: d, - metadata: metadata, + metadata: effectiveMD, supportsNodeStage: supportsNodeStage, nodeStageCache: make(map[volumeID]stageCacheEntry), } diff --git a/pkg/csi/manila/metadata_test.go b/pkg/csi/manila/metadata_test.go new file mode 100644 index 0000000000..19e2133468 --- /dev/null +++ b/pkg/csi/manila/metadata_test.go @@ -0,0 +1,146 @@ +/* +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 manila + +import ( + "errors" + "testing" +) + +type fakeMetadataProvider struct { + instanceID string + availabilityZone string + err error +} + +func (f *fakeMetadataProvider) GetInstanceID() (string, error) { + if f.err != nil { + return "", f.err + } + return f.instanceID, nil +} + +func (f *fakeMetadataProvider) GetAvailabilityZone() (string, error) { + if f.err != nil { + return "", f.err + } + return f.availabilityZone, nil +} + +func TestStaticMetadata(t *testing.T) { + m := &staticMetadata{nodeID: "my-node", nodeAZ: "az1"} + + id, err := m.GetInstanceID() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if id != "my-node" { + t.Errorf("expected node ID %q, got %q", "my-node", id) + } + + az, err := m.GetAvailabilityZone() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if az != "az1" { + t.Errorf("expected AZ %q, got %q", "az1", az) + } +} + +func TestOverrideMetadataNodeIDSet(t *testing.T) { + fallback := &fakeMetadataProvider{instanceID: "meta-id", availabilityZone: "meta-az"} + m := &overrideMetadata{nodeID: "flag-id", fallback: fallback} + + id, err := m.GetInstanceID() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if id != "flag-id" { + t.Errorf("expected node ID %q, got %q", "flag-id", id) + } + + az, err := m.GetAvailabilityZone() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if az != "meta-az" { + t.Errorf("expected AZ %q, got %q", "meta-az", az) + } +} + +func TestOverrideMetadataNodeAZSet(t *testing.T) { + fallback := &fakeMetadataProvider{instanceID: "meta-id", availabilityZone: "meta-az"} + m := &overrideMetadata{nodeAZ: "flag-az", fallback: fallback} + + id, err := m.GetInstanceID() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if id != "meta-id" { + t.Errorf("expected node ID %q, got %q", "meta-id", id) + } + + az, err := m.GetAvailabilityZone() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if az != "flag-az" { + t.Errorf("expected AZ %q, got %q", "flag-az", az) + } +} + +func TestOverrideMetadataBothSet(t *testing.T) { + fallback := &fakeMetadataProvider{err: errors.New("should not be called")} + m := &overrideMetadata{nodeID: "flag-id", nodeAZ: "flag-az", fallback: fallback} + + id, err := m.GetInstanceID() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if id != "flag-id" { + t.Errorf("expected node ID %q, got %q", "flag-id", id) + } + + az, err := m.GetAvailabilityZone() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if az != "flag-az" { + t.Errorf("expected AZ %q, got %q", "flag-az", az) + } +} + +func TestOverrideMetadataNeitherSet(t *testing.T) { + fallback := &fakeMetadataProvider{instanceID: "meta-id", availabilityZone: "meta-az"} + m := &overrideMetadata{fallback: fallback} + + id, err := m.GetInstanceID() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if id != "meta-id" { + t.Errorf("expected node ID %q, got %q", "meta-id", id) + } + + az, err := m.GetAvailabilityZone() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if az != "meta-az" { + t.Errorf("expected AZ %q, got %q", "meta-az", az) + } +} diff --git a/tests/sanity/manila/sanity_test.go b/tests/sanity/manila/sanity_test.go index 0219e2b894..8218ded280 100644 --- a/tests/sanity/manila/sanity_test.go +++ b/tests/sanity/manila/sanity_test.go @@ -54,7 +54,7 @@ func TestDriver(t *testing.T) { fakemeta := &fakemetadata{} - err = d.SetupNodeService(fakemeta) + err = d.SetupNodeService("", "", fakemeta) if err != nil { t.Fatalf("Failed to initialize CSI Manila node service: %v", err) }