Pr feedback

internal/model/config.go

@@ -78,16 +78,20 @@ type ConfigApplySuccess struct {
 // Complexity is 11, allowed is 10
 // nolint: revive, cyclop
 func (ncc *NginxConfigContext) Equal(otherNginxConfigContext *NginxConfigContext) bool {
-	if ncc.StubStatus.URL != otherNginxConfigContext.StubStatus.URL || ncc.StubStatus.Listen !=
-		otherNginxConfigContext.StubStatus.Listen || ncc.StubStatus.Location !=
-		otherNginxConfigContext.StubStatus.Location {
-		return false
+	if ncc.StubStatus != nil && otherNginxConfigContext.StubStatus != nil {
+		if ncc.StubStatus.URL != otherNginxConfigContext.StubStatus.URL || ncc.StubStatus.Listen !=
+			otherNginxConfigContext.StubStatus.Listen || ncc.StubStatus.Location !=
+			otherNginxConfigContext.StubStatus.Location {
+			return false
+		}
 	}
 
-	if ncc.PlusAPI.URL != otherNginxConfigContext.PlusAPI.URL || ncc.PlusAPI.Listen !=
-		otherNginxConfigContext.PlusAPI.Listen || ncc.PlusAPI.Location !=
-		otherNginxConfigContext.PlusAPI.Location {
-		return false
+	if ncc.PlusAPI != nil && otherNginxConfigContext.PlusAPI != nil {
+		if ncc.PlusAPI.URL != otherNginxConfigContext.PlusAPI.URL || ncc.PlusAPI.Listen !=
+			otherNginxConfigContext.PlusAPI.Listen || ncc.PlusAPI.Location !=
+			otherNginxConfigContext.PlusAPI.Location {
+			return false
+		}
 	}
 
 	if ncc.InstanceID != otherNginxConfigContext.InstanceID {

internal/model/config_test.go

@@ -81,6 +81,10 @@ func TestNginxConfigContext_Equal(t *testing.T) {
 	nginxConfigContextWithDifferentErrorLogs := *nginxConfigContext
 	nginxConfigContextWithDifferentErrorLogs.ErrorLogs = []*ErrorLog{}
 
+	nginxConfigContextWithNilValues := *nginxConfigContext
+	nginxConfigContextWithNilValues.StubStatus = nil
+	nginxConfigContextWithNilValues.PlusAPI = nil
+
 	assert.True(t, nginxConfigContext.Equal(&nginxConfigContextWithSameValues))
 	assert.False(t, nginxConfigContext.Equal(&nginxConfigContextWithDifferentStubStatus))
 	assert.False(t, nginxConfigContext.Equal(&nginxConfigContextWithDifferentPlusAPI))
@@ -89,4 +93,5 @@ func TestNginxConfigContext_Equal(t *testing.T) {
 	assert.False(t, nginxConfigContext.Equal(&nginxConfigContextWithDifferentFileHashes))
 	assert.False(t, nginxConfigContext.Equal(&nginxConfigContextWithDifferentAccessLogs))
 	assert.False(t, nginxConfigContext.Equal(&nginxConfigContextWithDifferentErrorLogs))
+	assert.True(t, nginxConfigContext.Equal(&nginxConfigContextWithNilValues))
 }

internal/watcher/instance/instance_watcher_service.go

@@ -126,15 +126,33 @@ func (iw *InstanceWatcherService) Watch(
 func (iw *InstanceWatcherService) ReparseConfigs(ctx context.Context) {
 	slog.DebugContext(ctx, "Reparsing all instance configurations")
 	for _, instance := range iw.instanceCache {
-		iw.ReparseConfig(ctx, instance.GetInstanceMeta().GetInstanceId(), &model.NginxConfigContext{})
+		slog.DebugContext(
+			ctx,
+			"Reparsing NGINX instance config",
+			"instance_id", instance.GetInstanceMeta().GetInstanceId(),
+		)
+
+		nginxConfigContext, parseErr := iw.nginxConfigParser.Parse(ctx, instance)
+		if parseErr != nil {
+			slog.ErrorContext(
+				ctx,
+				"Failed to parse NGINX instance config",
+				"config_path", instance.GetInstanceRuntime().GetConfigPath(),
+				"instance_id", instance.GetInstanceMeta().GetInstanceId(),
+				"error", parseErr,
+			)
+
+			return
+		}
+
+		iw.HandleNginxConfigContextUpdate(ctx, instance.GetInstanceMeta().GetInstanceId(), nginxConfigContext)
 	}
 	slog.DebugContext(ctx, "Finished reparsing all instance configurations")
 }
 
-func (iw *InstanceWatcherService) ReparseConfig(ctx context.Context, instanceID string,
+func (iw *InstanceWatcherService) HandleNginxConfigContextUpdate(ctx context.Context, instanceID string,
 	nginxConfigContext *model.NginxConfigContext,
 ) {
-	var parseErr error
 	iw.cacheMutex.Lock()
 	defer iw.cacheMutex.Unlock()
 
@@ -145,29 +163,6 @@ func (iw *InstanceWatcherService) ReparseConfig(ctx context.Context, instanceID
 
 	if instanceType == mpi.InstanceMeta_INSTANCE_TYPE_NGINX ||
 		instanceType == mpi.InstanceMeta_INSTANCE_TYPE_NGINX_PLUS {
-		// If the ReparseConfig was not triggered by a config apply that had changes there is no NginxConfigContext
-		// passed to this function. So we need to parse the config and create one.
-		if nginxConfigContext.InstanceID == "" {
-			slog.DebugContext(
-				ctx,
-				"Reparsing NGINX instance config",
-				"instance_id", instanceID,
-			)
-
-			nginxConfigContext, parseErr = iw.nginxConfigParser.Parse(ctx, instance)
-			if parseErr != nil {
-				slog.WarnContext(
-					ctx,
-					"Unable to parse NGINX instance config",
-					"config_path", instance.GetInstanceRuntime().GetConfigPath(),
-					"instance_id", instanceID,
-					"error", parseErr,
-				)
-
-				return
-			}
-		}
-
 		iw.sendNginxConfigContextUpdate(ctx, nginxConfigContext)
 		iw.nginxConfigCache[nginxConfigContext.InstanceID] = nginxConfigContext
 		updatesRequired = proto.UpdateNginxInstanceRuntime(instance, nginxConfigContext)

internal/watcher/instance/instance_watcher_service_test.go

@@ -263,6 +263,8 @@ func TestInstanceWatcherService_ReparseConfig(t *testing.T) {
 	updatedInstance.GetInstanceRuntime().GetNginxRuntimeInfo().AccessLogs = []string{"access2.log"}
 	updatedInstance.GetInstanceRuntime().GetNginxRuntimeInfo().ErrorLogs = []string{"error.log"}
 
+	updateNginxConfigContext.InstanceID = updatedInstance.GetInstanceMeta().GetInstanceId()
+
 	tests := []struct {
 		parseReturns *model.NginxConfigContext
 		name         string
@@ -275,13 +277,10 @@ func TestInstanceWatcherService_ReparseConfig(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(tt *testing.T) {
-			fakeNginxConfigParser := &instancefakes.FakeNginxConfigParser{}
-			fakeNginxConfigParser.ParseReturns(test.parseReturns, nil)
 			instanceUpdatesChannel := make(chan InstanceUpdatesMessage, 1)
 			nginxConfigContextChannel := make(chan NginxConfigContextMessage, 1)
 
 			instanceWatcherService := NewInstanceWatcherService(types.AgentConfig())
-			instanceWatcherService.nginxConfigParser = fakeNginxConfigParser
 			instanceWatcherService.instancesChannel = instanceUpdatesChannel
 			instanceWatcherService.nginxConfigContextChannel = nginxConfigContextChannel
 
@@ -293,12 +292,16 @@ func TestInstanceWatcherService_ReparseConfig(t *testing.T) {
 				instance.GetInstanceMeta().GetInstanceId(): instance,
 			}
 
-			instanceWatcherService.ReparseConfig(ctx, updatedInstance.GetInstanceMeta().GetInstanceId(),
-				&model.NginxConfigContext{})
+			instanceWatcherService.HandleNginxConfigContextUpdate(ctx, updatedInstance.
+				GetInstanceMeta().GetInstanceId(),
+				updateNginxConfigContext)
 
 			nginxConfigContextMessage := <-nginxConfigContextChannel
 			assert.Equal(t, updateNginxConfigContext, nginxConfigContextMessage.NginxConfigContext)
 
+			assert.Equal(tt, updateNginxConfigContext, instanceWatcherService.
+				nginxConfigCache[updatedInstance.GetInstanceMeta().GetInstanceId()])
+
 			instanceUpdatesMessage := <-instanceUpdatesChannel
 			assert.Len(t, instanceUpdatesMessage.InstanceUpdates.UpdatedInstances, 1)
 			assert.Equal(tt, updatedInstance, instanceUpdatesMessage.InstanceUpdates.UpdatedInstances[0])

internal/watcher/watcher_plugin.go

@@ -57,7 +57,7 @@ type (
 			instancesChannel chan<- instance.InstanceUpdatesMessage,
 			nginxConfigContextChannel chan<- instance.NginxConfigContextMessage,
 		)
-		ReparseConfig(ctx context.Context, instanceID string, configContext *model.NginxConfigContext)
+		HandleNginxConfigContextUpdate(ctx context.Context, instanceID string, configContext *model.NginxConfigContext)
 		ReparseConfigs(ctx context.Context)
 		SetEnabled(enabled bool)
 	}
@@ -202,7 +202,7 @@ func (w *Watcher) handleConfigApplySuccess(ctx context.Context, msg *bus.Message
 		slog.DebugContext(ctx, "NginxConfigContext is empty, no need to reparse config")
 		return
 	}
-	w.instanceWatcherService.ReparseConfig(ctx, instanceID, successMessage.ConfigContext)
+	w.instanceWatcherService.HandleNginxConfigContextUpdate(ctx, instanceID, successMessage.ConfigContext)
 
 	w.watcherMutex.Lock()
 	w.instancesWithConfigApplyInProgress = slices.DeleteFunc(

internal/watcher/watcher_plugin_test.go

@@ -196,7 +196,7 @@ func TestWatcher_Process_ConfigApplySuccessfulTopic(t *testing.T) {
 
 	watcherPlugin.Process(ctx, message)
 
-	assert.Equal(t, 1, fakeWatcherService.ReparseConfigCallCount())
+	assert.Equal(t, 1, fakeWatcherService.HandleNginxConfigContextUpdateCallCount())
 	assert.Empty(t, watcherPlugin.instancesWithConfigApplyInProgress)
 }
 

test/integration/grpc_management_plane_api_test.go

@@ -310,7 +310,6 @@ func TestGrpc_ConfigApply(t *testing.T) {
 
 		performConfigApply(t, nginxInstanceID)
 
-		time.Sleep(5 * time.Second)
 		responses = getManagementPlaneResponses(t, 2)
 		t.Logf("Config apply responses: %v", responses)