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/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/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/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) + } + }) + } +} 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) }