Safely handle CHAP rotation for stale portals during iSCSI self-healing

This commit changes iSCSI self-healing to never logout of stale non-CHAP portals. For stale CHAP portals, logouts are now only performed when the the portal is accessible.

Author: jwebster7

CHANGELOG.md

@@ -12,6 +12,7 @@
 - **Kubernetes:** Fixed backend config credentials to support all available AWS ARN partitions (Issue [#913](https://github.com/NetApp/trident/issues/913)).
 - **Kubernetes:** Added option to disable the auto configurator reconciliation in the Trident operator (Issue [#924](https://github.com/NetApp/trident/issues/924)).
 - **Kubernetes:** Added securityContext for csi-resizer container (Issue [#976](https://github.com/NetApp/trident/issues/976)).
+- **Kubernetes:** Fixed issue where iSCSI self-healing may unsafely log out of stale iSCSI sessions when a portal is inaccessible (Issue [#961](https://github.com/NetApp/trident/issues/961)).
 - Fixed Zonal Flex pools for GCNV driver.
 
 **Enhancements:**

frontend/csi/node_server.go

@@ -50,6 +50,7 @@ const (
 	iSCSINodeUnstageMaxDuration     = 15 * time.Second
 	iSCSILoginTimeout               = 10 * time.Second
 	iSCSISelfHealingLockContext     = "ISCSISelfHealingThread"
+	iSCSISelfHealingTimeout         = 90 * time.Second
 	fcpNodeUnstageMaxDuration       = 15 * time.Second
 	nvmeSelfHealingLockContext      = "NVMeSelfHealingThread"
 	defaultNodeReconciliationPeriod = 1 * time.Minute
@@ -1845,6 +1846,17 @@ func (p *Plugin) ensureAttachISCSIVolume(
 	return mpathSize, nil
 }
 
+// getChapInfoFromController attempts to gather CHAP info from the controller in a timebound manner.
+func (p *Plugin) getChapInfoFromController(
+	ctx context.Context, volumeID, nodeName string,
+) (*models.IscsiChapInfo, error) {
+	chapInfo, err := p.restClient.GetChap(ctx, volumeID, nodeName)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get CHAP info from CSI controller for volume: '%s'; %w", volumeID, err)
+	}
+	return chapInfo, nil
+}
+
 func (p *Plugin) updateChapInfoFromController(
 	ctx context.Context, req *csi.NodeStageVolumeRequest, publishInfo *models.VolumePublishInfo,
 ) error {
@@ -2480,11 +2492,17 @@ func (p *Plugin) selfHealingRectifySession(ctx context.Context, portal string, a
 
 	switch action {
 	case models.LogoutLoginScan:
+		// Do not log out if the portal is unresponsive as subsequent logins are likely to fail.
+		if isAccessible, err := p.iscsi.IsPortalAccessible(ctx, portal); !isAccessible {
+			Logc(ctx).WithError(err).Warnf("Cannot safely log out of unresponsive portal '%s'.", portal)
+			return fmt.Errorf("cannot safely log out of unresponsive portal '%s'", portal)
+		}
+
 		if err = p.iscsi.Logout(ctx, publishInfo.IscsiTargetIQN, portal); err != nil {
 			return fmt.Errorf("error while logging out of target %s", publishInfo.IscsiTargetIQN)
-		} else {
-			Logc(ctx).Debug("Logout is successful.")
 		}
+		Logc(ctx).Debug("Logout is successful.")
+
 		// Logout is successful, fallthrough to perform login.
 		fallthrough
 	case models.LoginScan:
@@ -2508,12 +2526,17 @@ func (p *Plugin) selfHealingRectifySession(ctx context.Context, portal string, a
 		}
 
 		if publishedCHAPCredentials != publishInfo.IscsiChapInfo {
+			fields := LogFields{
+				"portal":    portal,
+				"CHAPInUse": true,
+			}
+
+			// Update the CHAP info in the portal. This should never fail.
 			updateErr := publishedISCSISessions.UpdateCHAPForPortal(portal, publishInfo.IscsiChapInfo)
 			if updateErr != nil {
-				Logc(ctx).Warn("Failed to update published CHAP information.")
+				Logc(ctx).WithFields(fields).Warn("Failed to update published CHAP information.")
 			}
-
-			Logc(ctx).Debug("Updated published CHAP information.")
+			Logc(ctx).WithFields(fields).Debug("Updated published CHAP information after successful login.")
 		}
 
 		Logc(ctx).Debug("Login to target is successful.")
@@ -2566,6 +2589,64 @@ func (p *Plugin) deprecatedIgroupInUse(ctx context.Context) bool {
 	return false
 }
 
+// updateCHAPInfoForSessions provides a best attempt to populate up-to-date CHAP credentials within iSCSI self-healing's
+// published sessions. It tracks CHAP credentials by unique IQN to reduce the number of calls to the controller.
+func (p *Plugin) updateCHAPInfoForSessions(
+	ctx context.Context, publishedSessions, currentSessions *models.ISCSISessions,
+) error {
+	if publishedSessions == nil || currentSessions == nil {
+		return nil
+	}
+
+	// Timebox this operation to prevent it from running indefinitely and blocking everything in self-healing and
+	// other operations in the node server.
+	cancelCtx, cancel := context.WithTimeout(ctx, iSCSISelfHealingTimeout/3)
+	defer cancel()
+
+	// Store the CHAP credentials we've found for certain IQNs.
+	// IQNs should be unique between SVMs and CHAP credentials are scoped at the SVM level.
+	iqnToCHAP := make(map[string]*models.IscsiChapInfo, 0)
+	var errs error
+
+	for portal, publishedData := range publishedSessions.Info {
+		// If the session isn't stale on the host, then we can ignore this portal for now.
+		data, ok := currentSessions.Info[portal]
+		if ok && !p.iscsi.IsSessionStale(cancelCtx, data.PortalInfo.SessionNumber) {
+			continue
+		} else if !publishedData.PortalInfo.CHAPInUse() || !publishedData.PortalInfo.HasTargetIQN() {
+			continue
+		}
+
+		chapInfo, ok := iqnToCHAP[publishedData.PortalInfo.ISCSITargetIQN]
+		if !ok {
+			volumeID, err := publishedSessions.VolumeIDForPortal(portal)
+			if err != nil {
+				errs = errors.Join(errs, fmt.Errorf("failed to get volume ID for portal: '%s'; %w", portal, err))
+				continue
+			}
+
+			chapInfo, err = p.getChapInfoFromController(cancelCtx, volumeID, p.nodeName)
+			if err != nil {
+				errs = errors.Join(errs, fmt.Errorf("failed to get CHAP info for portal: '%s'; %w", portal, err))
+				continue
+			}
+
+			// Store what we've learned for future iterations.
+			iqnToCHAP[publishedData.PortalInfo.ISCSITargetIQN] = chapInfo
+		}
+
+		publishedData.PortalInfo.UpdateCHAPCredentials(*chapInfo)
+		Logc(cancelCtx).WithField("portal", portal).Debug("Updated CHAP info for portal.")
+	}
+
+	if errs != nil {
+		Logc(cancelCtx).WithError(errs).Error("Failed to get updated CHAP info for portal(s).")
+		return errs
+	}
+
+	return nil
+}
+
 // performISCSISelfHealing inspects the desired state of the iSCSI sessions with the current state and accordingly
 // identifies candidate sessions that require remediation. This function is invoked periodically.
 func (p *Plugin) performISCSISelfHealing(ctx context.Context) {
@@ -2580,7 +2661,7 @@ func (p *Plugin) performISCSISelfHealing(ctx context.Context) {
 	}()
 
 	// After this time self-healing may be stopped
-	stopSelfHealingAt := time.Now().Add(60 * time.Second)
+	stopSelfHealingAt := time.Now().Add(iSCSISelfHealingTimeout)
 
 	// If there are not iSCSI volumes expected on the host skip self-healing
 	if publishedISCSISessions.IsEmpty() {
@@ -2610,6 +2691,11 @@ func (p *Plugin) performISCSISelfHealing(ctx context.Context) {
 		Logc(ctx).Debug("No iSCSI sessions LUN mappings found.")
 	}
 
+	// Update CHAP info for published sessions.
+	if err := p.updateCHAPInfoForSessions(ctx, &publishedISCSISessions, &currentISCSISessions); err != nil {
+		Logc(ctx).WithError(err).Error("Failed to update CHAP credentials for published iSCSI sessions.")
+	}
+
 	Logc(ctx).Debugf("Published iSCSI Sessions: %v", publishedISCSISessions)
 	Logc(ctx).Debugf("Current iSCSI Sessions: %v", currentISCSISessions)
 

frontend/csi/node_server_test.go

@@ -606,6 +606,91 @@ func TestNodeStageISCSIVolume(t *testing.T) {
 	}
 }
 
+func TestGetChapInfoFromController(t *testing.T) {
+	type args struct {
+		ctx    context.Context
+		volume string
+		node   string
+	}
+
+	type data struct {
+		chapInfo *models.IscsiChapInfo
+	}
+
+	type assertions struct {
+		Error assert.ErrorAssertionFunc
+		Empty assert.ValueAssertionFunc
+		Equal assert.ComparisonAssertionFunc
+	}
+
+	tt := map[string]struct {
+		args   args
+		data   data
+		assert assertions
+		mocks  func(mockAPI *mockControllerAPI.MockTridentController, args args, data data)
+	}{
+		"Successfully gets CHAP credentials": {
+			args: args{
+				ctx:    context.Background(),
+				volume: "foo",
+				node:   "bar",
+			},
+			data: data{
+				chapInfo: &models.IscsiChapInfo{
+					UseCHAP:              true,
+					IscsiUsername:        "user",
+					IscsiInitiatorSecret: "pass",
+					IscsiTargetUsername:  "user2",
+					IscsiTargetSecret:    "pass2",
+				},
+			},
+			assert: assertions{
+				Error: assert.NoError,
+				Empty: assert.NotEmpty,
+				Equal: assert.Equal,
+			},
+			mocks: func(mockAPI *mockControllerAPI.MockTridentController, args args, data data) {
+				mockAPI.EXPECT().GetChap(args.ctx, args.volume, args.node).Return(data.chapInfo, nil)
+			},
+		},
+		"Fails to get CHAP credentials": {
+			args: args{
+				ctx:    context.Background(),
+				volume: "foo",
+				node:   "bar",
+			},
+			data: data{
+				chapInfo: nil,
+			},
+			assert: assertions{
+				Error: assert.Error,
+				Empty: assert.Empty,
+				Equal: assert.Equal,
+			},
+			mocks: func(mockAPI *mockControllerAPI.MockTridentController, args args, data data) {
+				mockAPI.EXPECT().GetChap(args.ctx, args.volume, args.node).Return(data.chapInfo, errors.New("api error"))
+			},
+		},
+	}
+
+	for name, test := range tt {
+		t.Run(name, func(t *testing.T) {
+			mockCtrl := gomock.NewController(t)
+			mockAPI := mockControllerAPI.NewMockTridentController(mockCtrl)
+			test.mocks(mockAPI, test.args, test.data)
+
+			plugin := &Plugin{
+				restClient: mockAPI,
+			}
+
+			info, err := plugin.getChapInfoFromController(test.args.ctx, test.args.volume, test.args.node)
+			test.assert.Error(t, err)
+			test.assert.Empty(t, info)
+			test.assert.Equal(t, info, test.data.chapInfo)
+		})
+	}
+}
+
 func TestUpdateChapInfoFromController_Success(t *testing.T) {
 	testCtx := context.Background()
 	volumeName := "foo"
@@ -665,6 +750,129 @@ func TestUpdateChapInfoFromController_Error(t *testing.T) {
 	assert.EqualValues(t, models.IscsiChapInfo{}, testPublishInfo.IscsiAccessInfo.IscsiChapInfo)
 }
 
+func TestUpdateChapInfoForSessions(t *testing.T) {
+	// Populate sessions with only the state that matters. The rest of the fields are not relevant for this test.
+	publishedSessions := &models.ISCSISessions{}
+	currentSessions := &models.ISCSISessions{}
+
+	// CHAP portals
+	chapPortals := []string{"0.0.0.0", "4.4.4.4", "5.5.5.5"}
+
+	// Unique CHAP portal
+	uniqueChapPortal := "9.9.9.9"
+	uniqueIQN := "iqn.9999-99.com.netapp:target"
+
+	// non-CHAP portals
+	nonChapPortals := []string{"1.1.1.1", "2.2.2.2", "3.3.3.3"}
+
+	sessionID := "0"
+
+	// Shared CHAP credentials
+	sharedCHAPInfo := models.IscsiChapInfo{
+		UseCHAP:              true,
+		IscsiUsername:        "user",
+		IscsiInitiatorSecret: "pass",
+		IscsiTargetUsername:  "user2",
+		IscsiTargetSecret:    "pass2",
+	}
+
+	// Add CHAP sessions to both maps.
+	for _, portal := range chapPortals {
+		err := publishedSessions.AddPortal(portal, models.PortalInfo{
+			ISCSITargetIQN: "iqn.2020-01.com.netapp:target",
+			Credentials:    sharedCHAPInfo,
+		})
+		assert.NoError(t, err)
+
+		err = currentSessions.AddPortal(portal, models.PortalInfo{
+			ISCSITargetIQN: "iqn.2020-01.com.netapp:target",
+			SessionNumber:  sessionID,
+		})
+		assert.NoError(t, err)
+	}
+
+	// Add a CHAP session with a unique IQN.
+	err := publishedSessions.AddPortal(uniqueChapPortal, models.PortalInfo{
+		// Use a different IQN here to prove we cache returned values from the REST API.
+		ISCSITargetIQN: uniqueIQN,
+		Credentials:    sharedCHAPInfo,
+	})
+	assert.NoError(t, err)
+	err = currentSessions.AddPortal(uniqueChapPortal, models.PortalInfo{
+		ISCSITargetIQN: uniqueIQN,
+		SessionNumber:  sessionID,
+	})
+	assert.NoError(t, err)
+
+	// Add non-CHAP session
+	for _, portal := range nonChapPortals {
+		err := publishedSessions.AddPortal(portal, models.PortalInfo{
+			ISCSITargetIQN: "iqn.2020-01.com.netapp:target",
+			Credentials:    models.IscsiChapInfo{},
+		})
+		assert.NoError(t, err)
+
+		err = currentSessions.AddPortal(portal, models.PortalInfo{
+			ISCSITargetIQN: "iqn.2020-01.com.netapp:target",
+			Credentials:    models.IscsiChapInfo{},
+			SessionNumber:  sessionID,
+		})
+		assert.NoError(t, err)
+	}
+
+	// Populate a single of LUN and volume in each session.
+	volume := "foo"
+	node := "bar"
+	for _, sessionData := range publishedSessions.Info {
+		sessionData.LUNs.Info[0] = volume
+	}
+
+	// Create a mock controller client that will return the expected CHAP info.
+	mockCtrl := gomock.NewController(t)
+	mockISCSI := mock_iscsi.NewMockISCSI(mockCtrl)
+	mockClient := mockControllerAPI.NewMockTridentController(mockCtrl)
+
+	plugin := &Plugin{
+		nodeName:   node,
+		iscsi:      mockISCSI,
+		restClient: mockClient,
+	}
+
+	// Expect calls on the mock client for all sessions that use CHAP.
+	freshCHAPInfo := &models.IscsiChapInfo{
+		UseCHAP:              true,
+		IscsiUsername:        "user2",
+		IscsiInitiatorSecret: "pass2",
+		IscsiTargetUsername:  "user",
+		IscsiTargetSecret:    "pass",
+	}
+
+	// Mock calls to the iSCSI client
+	count := len(currentSessions.Info)
+	mockISCSI.EXPECT().IsSessionStale(gomock.Any(), sessionID).Return(true).Times(count)
+
+	// Mock API calls
+	count = len(chapPortals) - (len(chapPortals) - 1) + 1 // Expect one more call for the unique CHAP portal.
+	mockClient.EXPECT().GetChap(
+		gomock.Any(), volume, node,
+	).Return(freshCHAPInfo, nil).Times(count)
+
+	err = plugin.updateCHAPInfoForSessions(ctx, publishedSessions, currentSessions)
+	assert.NoError(t, err)
+
+	// Verify that the CHAP info was updated in all sessions that use CHAP.
+	for _, portal := range chapPortals {
+		chapInfoInSession := publishedSessions.Info[portal].PortalInfo.Credentials
+		assert.EqualValues(t, *freshCHAPInfo, chapInfoInSession)
+	}
+
+	// Verify that the non-CHAP portals were not updated.
+	for _, portal := range nonChapPortals {
+		chapInfoInSession := publishedSessions.Info[portal].PortalInfo.Credentials
+		assert.EqualValues(t, models.IscsiChapInfo{}, chapInfoInSession)
+	}
+}
+
 type PortalAction struct {
 	Portal string
 	Action models.ISCSIAction