diff --git a/csireverseproxy/pkg/config/config.go b/csireverseproxy/pkg/config/config.go index 56096332..65238eeb 100644 --- a/csireverseproxy/pkg/config/config.go +++ b/csireverseproxy/pkg/config/config.go @@ -576,9 +576,22 @@ func (pc *ProxyConfig) ParseConfig(proxyConfigMap ProxyConfigMap, k8sUtils k8sut storageArrayIdentifiers := make(map[url.URL][]string) ipAddresses := make([]string, 0) - for _, mgmtServer := range config.ManagementServerConfig { - ipAddresses = append(ipAddresses, mgmtServer.Endpoint) + for i := range config.ManagementServerConfig { + sanitizedPath := utils.SanitizeURLPath(config.ManagementServerConfig[i].Endpoint) + config.ManagementServerConfig[i].Endpoint = sanitizedPath + ipAddresses = append(ipAddresses, sanitizedPath) } + + for i := range config.StorageArrayConfig { + // sanitize primary endpoint + sanitizedPrimary := utils.SanitizeURLPath(config.StorageArrayConfig[i].PrimaryEndpoint) + config.StorageArrayConfig[i].PrimaryEndpoint = sanitizedPrimary + + // sanitize backup endpoint + sanitizedBackup := utils.SanitizeURLPath(config.StorageArrayConfig[i].BackupEndpoint) + config.StorageArrayConfig[i].BackupEndpoint = sanitizedBackup + } + for _, array := range config.StorageArrayConfig { if array.PrimaryEndpoint == "" { return fmt.Errorf("primary endpoint not configured for array: %s", array.StorageArrayID) diff --git a/csireverseproxy/pkg/utils/utils.go b/csireverseproxy/pkg/utils/utils.go index 49278edf..f49bebcf 100644 --- a/csireverseproxy/pkg/utils/utils.go +++ b/csireverseproxy/pkg/utils/utils.go @@ -199,3 +199,12 @@ func RemoveTempFiles() error { } return nil } + +// SanitizeURLPath - removes trailing slash from URL and trim the whitespaces +func SanitizeURLPath(path string) string { + path = strings.TrimSpace(path) + if strings.HasSuffix(path, "/") { + return strings.TrimRight(path, "/") + } + return path +} diff --git a/csireverseproxy/pkg/utils/utils_test.go b/csireverseproxy/pkg/utils/utils_test.go index 98b9bfb2..1af48f03 100644 --- a/csireverseproxy/pkg/utils/utils_test.go +++ b/csireverseproxy/pkg/utils/utils_test.go @@ -348,3 +348,26 @@ func TestGetListenAddress(t *testing.T) { t.Errorf("Expected %s, but got %s", expectedListenAddress, listenAddress) } } + +func TestSanitizeURLPath(t *testing.T) { + // Test case for good https://example.com/path + goodURL := "https://example.com/path" + sanitizedURl := SanitizeURLPath(goodURL) + if sanitizedURl != goodURL { + t.Errorf("Expected %s, but got %s", goodURL, sanitizedURl) + } + + // Test case for bad URL with spaces ' https://example.com/path ' + badURL := " https://example.com/path " + sanitizedURl = SanitizeURLPath(badURL) + if sanitizedURl != goodURL { + t.Errorf("Expected %s, but got %s", goodURL, sanitizedURl) + } + + // Test case for bad URL with trailing /// https://example.com/path/// + badURL = "https://example.com/path////" + sanitizedURl = SanitizeURLPath(badURL) + if sanitizedURl != goodURL { + t.Errorf("Expected %s, but got %s", goodURL, sanitizedURl) + } +} diff --git a/csireverseproxy/test-config/config-params.yaml b/csireverseproxy/test-config/config-params.yaml index 159992f6..362aff30 100644 --- a/csireverseproxy/test-config/config-params.yaml +++ b/csireverseproxy/test-config/config-params.yaml @@ -1,16 +1,3 @@ -# Copyright © 2020-2025 Dell Inc. or its subsidiaries. All Rights Reserved. -# -# 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. -# - csi_powermax_reverse_proxy_port: "2222" csi_log_level: debug csi_log_format: json diff --git a/csireverseproxy/test-config/config.yaml b/csireverseproxy/test-config/config.yaml index 6ae3b43a..2ebe0a95 100644 --- a/csireverseproxy/test-config/config.yaml +++ b/csireverseproxy/test-config/config.yaml @@ -15,8 +15,8 @@ port: 2222 config: storageArrays: - storageArrayId: "000000000001" - primaryURL: https://primary-1.unisphe.re:8443 - backupURL: https://backup-1.unisphe.re:8443 + primaryURL: https://primary-1.unisphe.re:8443/ + backupURL: https://backup-1.unisphe.re:8443/ proxyCredentialSecrets: - primary-unisphere-secret-1 - backup-unisphere-secret-1 diff --git a/csireverseproxy/test-config/configB.yaml b/csireverseproxy/test-config/configB.yaml index b1108727..812ded0e 100644 --- a/csireverseproxy/test-config/configB.yaml +++ b/csireverseproxy/test-config/configB.yaml @@ -15,7 +15,7 @@ port: 2222 config: storageArrays: - storageArrayId: "000000000001" - primaryURL: https://primary-3.unisphe.re:8443 + primaryURL: https://primary-3.unisphe.re:8443/ backupURL: https://backup-3.unisphe.re:8443 proxyCredentialSecrets: - primary-unisphere-secret-1 diff --git a/service/node.go b/service/node.go index 957cbd0e..8f1bb7b9 100644 --- a/service/node.go +++ b/service/node.go @@ -82,6 +82,9 @@ type maskingViewNVMeTargetInfo struct { target gonvme.NVMeTarget } +// Mockable function variable for unit testing +var getIPInterfaces func(ctx context.Context, symID string, portGroups []string, pmaxClient pmax.Pmax) (map[string]int32, error) = getIPInterfacesImpl + // Mapping between symid and all remote targets on the sym // key - string, value - []NVMETCPTarget var symToAllNVMeTCPTargets sync.Map @@ -1113,7 +1116,7 @@ func (s *service) NodeGetCapabilities( }, nil } -func (s *service) getIPInterfaces(ctx context.Context, symID string, portGroups []string, pmaxClient pmax.Pmax) (map[string]int32, error) { +func getIPInterfacesImpl(ctx context.Context, symID string, portGroups []string, pmaxClient pmax.Pmax) (map[string]int32, error) { ipInterfaces := make(map[string]int32) for _, pg := range portGroups { portGroup, err := pmaxClient.GetPortGroupByID(ctx, symID, pg) @@ -1192,7 +1195,7 @@ func (s *service) createTopologyMap(ctx context.Context, nodeName string) map[st } } - ipInterfaces, err := s.getIPInterfaces(ctx, id, s.opts.PortGroups, pmaxClient) + ipInterfaces, err := getIPInterfaces(ctx, id, s.opts.PortGroups, pmaxClient) if err != nil { log.Errorf("unable to fetch ip interfaces for %s: %s", id, err.Error()) continue @@ -1984,7 +1987,7 @@ func (s *service) updateNQNWithHostID(ctx context.Context, symID string, NQNs [] if strings.Contains(hostInitiator, nqn) { initiator, err := pmaxClient.GetInitiatorByID(ctx, symID, hostInitiator) if err != nil { - log.Errorf("Failed to fetch InitiatorID details for the initiator %s", initiator.InitiatorID) + log.Errorf("Failed to fetch InitiatorID details for the initiator %s", hostInitiator) } else { hostID := initiator.HostID nqn = nqn + ":" + hostID @@ -2066,15 +2069,17 @@ func (s *service) getAndConfigureMaskingViewTargetsNVMeTCP(ctx context.Context, // setupNVMeTCPTargetDiscovery is called to discover NVMe targets from the host/node func (s *service) setupNVMeTCPTargetDiscovery(ctx context.Context, array string, pmaxClient pmax.Pmax) error { - loggedInAll := true - ipInterfaces, err := s.getIPInterfaces(ctx, array, s.opts.PortGroups, pmaxClient) + var combinedErrors []string + atLeastOneLoggedIn := false + var totalTargetsDiscovered int + ipInterfaces, err := getIPInterfaces(ctx, array, s.opts.PortGroups, pmaxClient) if err != nil { - log.Errorf("unable to fetch ip interfaces for %s: %s", array, err.Error()) + log.Errorf("unable to fetch IP interfaces for %s: %s", array, err.Error()) return err } if len(ipInterfaces) == 0 { - return fmt.Errorf("couldn't find any ip interfaces on any of the port-groups %s", s.opts.PortGroups) + return fmt.Errorf("couldn't find any IP interfaces on any of the port-groups %s", s.opts.PortGroups) } for ip := range ipInterfaces { // Attempt target discovery from host @@ -2083,62 +2088,61 @@ func (s *service) setupNVMeTCPTargetDiscovery(ctx context.Context, array string, if discoveryError != nil { log.Errorf("Failed to discover the NVMe target: %s. Error: %s", ip, discoveryError.Error()) - err = discoveryError - loggedInAll = false + combinedErrors = append(combinedErrors, fmt.Sprintf("target IP: %s, Error: %s", ip, discoveryError.Error())) } else { - log.Infof("Successfully logged into target IP: %s ", ip) + log.Infof("Successfully logged into target IP: %s", ip) + atLeastOneLoggedIn = true + totalTargetsDiscovered++ } } - if loggedInAll { - return nil + if !atLeastOneLoggedIn { + return fmt.Errorf("failed to discover NVMe targets on any of the port-groups %s. Errors: %s", + s.opts.PortGroups, strings.Join(combinedErrors, "; ")) } - return err + log.Infof("Discovered %d NVMe targets out of %d from port-groups %s", + totalTargetsDiscovered, len(ipInterfaces), strings.Join(s.opts.PortGroups, ", ")) + return nil } // loginIntoISCSITargets - for a given array id and list of masking view targets // attempt login. The login method is different if CHAP is enabled // also update the logged in arrays cache func (s *service) loginIntoISCSITargets(array string, targets []maskingViewTargetInfo) error { + var combinedErrors []string + atLeastOneLoggedIn := false + var totalTargetsDiscovered int var err error - loggedInAll := true - if s.opts.EnableCHAP { - // CHAP is already enabled on the array, discovery will not work, - // so we need to do a login (as we have already setup the database(s) successfully - for _, tgt := range targets { - loginError := s.iscsiClient.PerformLogin(tgt.target) - if loginError != nil { - log.Errorf("Failed to perform ISCSI login for target: %s. Error: %s", - tgt.target.Target, loginError.Error()) - err = loginError - loggedInAll = false - } else { - s.iscsiTargets[array] = append(s.iscsiTargets[array], tgt.target.Target) - log.Infof("Successfully logged into target: %s", tgt.target.Target) - } + + for _, tgt := range targets { + if s.opts.EnableCHAP { + err = s.iscsiClient.PerformLogin(tgt.target) + } else { + _, err = s.iscsiClient.DiscoverTargets(tgt.target.Portal, true) } - } else { - for _, tgt := range targets { - // If CHAP is not enabled, attempt a discovery login - log.Debugf("Discovering iSCSI targets on %s", tgt.target.Portal) - _, discoveryError := s.iscsiClient.DiscoverTargets(tgt.target.Portal, true) - if discoveryError != nil { - log.Errorf("Failed to discover the ISCSI target: %s. Error: %s", - tgt.target.Target, discoveryError.Error()) - err = discoveryError - loggedInAll = false - } else { - s.iscsiTargets[array] = append(s.iscsiTargets[array], tgt.target.Target) - log.Infof("Successfully logged into target: %s on portal :%s", - tgt.target.Target, tgt.target.Portal) - } + if err != nil { + log.Errorf("Failed to login to target %s: %v", + tgt.target.Target, err) + combinedErrors = append(combinedErrors, fmt.Sprintf("target: %s, Error: %v", + tgt.target.Target, err)) + } else { + s.iscsiTargets[array] = append(s.iscsiTargets[array], tgt.target.Target) + log.Infof("Successfully logged to target: %s", tgt.target.Target) + atLeastOneLoggedIn = true + totalTargetsDiscovered++ } } - // If we successfully logged into all targets, then marked the array as logged in - if loggedInAll { + + // If we successfully logged into at least one target, then mark the array as logged in + if atLeastOneLoggedIn { s.UpdateLoggedInArrays(array, true) + } else { + return fmt.Errorf("failed to login to ISCSI targets on any of the portals. Errors: %s", + strings.Join(combinedErrors, "; ")) } - return err + log.Infof("Logged into %d ISCSI targets out of %d on array %s", + totalTargetsDiscovered, len(targets), array) + return nil } func (s *service) UpdateLoggedInArrays(array string, value bool) { diff --git a/service/node_test.go b/service/node_test.go index e0cd0174..8020c409 100644 --- a/service/node_test.go +++ b/service/node_test.go @@ -2486,18 +2486,8 @@ func TestGetIPIntefaces(t *testing.T) { for _, tc := range testCases { tc.pmaxClient = tc.getClient() t.Run(tc.name, func(t *testing.T) { - s := &service{ - opts: Opts{ - UseProxy: true, - TransportProtocol: tc.transportProtocol, - }, - nvmetcpClient: gonvme.NewMockNVMe(map[string]string{}), - nvmeTargets: &sync.Map{}, - loggedInNVMeArrays: map[string]bool{}, - } - tc.init() - got, _ := s.getIPInterfaces(context.Background(), tc.symID, tc.portGroups, tc.pmaxClient) + got, _ := getIPInterfaces(context.Background(), tc.symID, tc.portGroups, tc.pmaxClient) if len(got) != len(tc.want) { t.Errorf("Expected: %v, but got: %v", len(tc.want), len(got)) } @@ -2551,3 +2541,207 @@ func TestReachableEndPoint(t *testing.T) { }) } } + +func TestSetupNVMeTCPTargetDiscovery(t *testing.T) { + twoInterfaces := func() (map[string]int32, error) { + return map[string]int32{ + "ip1": 100, + "ip2": 200, + }, nil + } + + tests := []struct { + name string + getIPInterfaces func() (map[string]int32, error) + getDiscoverError func(address string) error + wantErr bool + }{ + { + name: "Successful discovery", + getIPInterfaces: twoInterfaces, + getDiscoverError: func(_ string) error { return nil }, + wantErr: false, + }, + { + name: "Error fetching IP interfaces", + getIPInterfaces: func() (map[string]int32, error) { + return nil, errors.New("unable to fetch IP interfaces") + }, + getDiscoverError: nil, // should not be called + wantErr: true, + }, + { + name: "All targets failed to discover", + getIPInterfaces: twoInterfaces, + getDiscoverError: func(_ string) error { + return errors.New("unable to discover NVMe targets") + }, + wantErr: true, + }, + { + name: "First of two targets failed to discover", + getIPInterfaces: twoInterfaces, + getDiscoverError: func(address string) error { + if address == "ip1" { + return errors.New("unable to discover NVMe target") + } + return nil + }, + wantErr: false, + }, + } + + origGetIPInterfaces := getIPInterfaces + defer func() { + getIPInterfaces = origGetIPInterfaces + }() + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl := gmock.NewController(t) + c := mocks.NewMockPmaxClient(ctrl) + + s := &service{ + nvmetcpClient: &nmveClientMock{ + getDiscoverError: tt.getDiscoverError, + }, + } + + getIPInterfaces = func(_ context.Context, _ string, _ []string, _ pmax.Pmax) (map[string]int32, error) { + return tt.getIPInterfaces() + } + + err := s.setupNVMeTCPTargetDiscovery(context.Background(), "sym1", c) + if (err != nil) && !tt.wantErr { + t.Errorf("Got unexpected setupNVMeTCPTargetDiscovery() error = %v", err) + } else if (err == nil) && tt.wantErr { + t.Errorf("Expected setupNVMeTCPTargetDiscovery() error, but got nil") + } + }) + } +} + +type nmveClientMock struct { + gonvme.NVMEinterface + getDiscoverError func(address string) error +} + +func (c *nmveClientMock) DiscoverNVMeTCPTargets(address string, _ bool) ([]gonvme.NVMeTarget, error) { + if err := c.getDiscoverError(address); err != nil { + return nil, err + } + return []gonvme.NVMeTarget{}, nil +} + +func TestLoginIntoISCSITargets(t *testing.T) { + twoTargets := []maskingViewTargetInfo{ + { + target: goiscsi.ISCSITarget{ + Target: "target1", + Portal: "portal1", + }, + }, + { + target: goiscsi.ISCSITarget{ + Target: "target2", + Portal: "portal2", + }, + }, + } + + tests := []struct { + name string + enableCHAP bool + getLoginError func(portal string) error + wantLoggedIn []string + wantErr bool + }{ + { + name: "Successful login", + enableCHAP: false, + getLoginError: func(_ string) error { return nil }, + wantLoggedIn: []string{ + "portal1", + "portal2", + }, + wantErr: false, + }, + { + name: "Successful login with CHAP", + enableCHAP: true, + getLoginError: func(_ string) error { return nil }, + wantLoggedIn: []string{ + "portal1", + "portal2", + }, + wantErr: false, + }, + { + name: "All targets failed to login", + getLoginError: func(_ string) error { + return errors.New("unable to login to ISCSI target") + }, + wantLoggedIn: nil, + wantErr: true, + }, + { + name: "First of two targets failed to login", + getLoginError: func(portal string) error { + if portal == "portal1" { + return errors.New("unable to login to ISCSI target") + } + return nil + }, + wantLoggedIn: []string{ + "portal2", // only the second target is expected to log in + }, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := &service{ + opts: Opts{ + EnableCHAP: tt.enableCHAP, + }, + iscsiClient: &iscsiClientMock{getLoginError: tt.getLoginError}, + iscsiTargets: map[string][]string{}, + loggedInArrays: map[string]bool{}, + } + + err := s.loginIntoISCSITargets("sym1", twoTargets) + if (err != nil) && !tt.wantErr { + t.Errorf("Got unexpected loginIntoISCSITargets() error = %v", err) + } else if (err == nil) && tt.wantErr { + t.Errorf("Expected loginIntoISCSITargets() error, but got nil") + } + + if !tt.wantErr { + if len(s.loggedInArrays) != 1 || s.loggedInArrays["sym1"] != true { + t.Errorf("Unexpected logged in arrays: %v", s.loggedInArrays) + } + } else { + if len(s.loggedInArrays) > 0 { + t.Errorf("Unexpected logged in arrays: %v", s.loggedInArrays) + } + } + }) + } +} + +type iscsiClientMock struct { + goiscsi.ISCSIinterface + getLoginError func(portal string) error +} + +func (c *iscsiClientMock) PerformLogin(target goiscsi.ISCSITarget) error { + return c.getLoginError(target.Portal) +} + +func (c *iscsiClientMock) DiscoverTargets(portal string, _ bool) ([]goiscsi.ISCSITarget, error) { + if err := c.getLoginError(portal); err != nil { + return nil, err + } + return []goiscsi.ISCSITarget{}, nil +} diff --git a/service/service.go b/service/service.go index b0022788..1d660680 100644 --- a/service/service.go +++ b/service/service.go @@ -991,11 +991,9 @@ func setArrayConfigEnvs(ctx context.Context) error { if !ok { return errors.New("unable to read X_CSI_POWERMAX_ARRAY_CONFIG_PATH from env") } - paramsViper := viper.New() paramsViper.SetConfigFile(configFilePath) paramsViper.SetConfigType("yaml") - err := paramsViper.ReadInConfig() // if unable to read configuration file, set defaults if err != nil { @@ -1017,6 +1015,11 @@ func setArrayConfigEnvs(ctx context.Context) error { } endpoint := paramsViper.GetString(EnvEndpoint) if endpoint != "" { + // Clean up endpoint by removing spaces or trailing slash + endpoint = strings.TrimSpace(endpoint) + if strings.HasSuffix(endpoint, "/") { + endpoint = strings.TrimRight(endpoint, "/") + } log.Info("Read endpoint from config file:", endpoint) _ = os.Setenv(EnvEndpoint, endpoint) }