Skip to content

Commit a662ad7

Browse files
aphralGaphralG
committed
Pr feedback
1 parent 3d6db7a commit a662ad7

File tree

8 files changed

+73
-67
lines changed

8 files changed

+73
-67
lines changed

‎internal/model/config.go‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -78,16 +78,20 @@ type ConfigApplySuccess struct {
7878
// Complexity is 11, allowed is 10
7979
// nolint: revive, cyclop
8080
func (ncc *NginxConfigContext) Equal(otherNginxConfigContext *NginxConfigContext) bool {
81-
if ncc.StubStatus.URL != otherNginxConfigContext.StubStatus.URL || ncc.StubStatus.Listen !=
82-
otherNginxConfigContext.StubStatus.Listen || ncc.StubStatus.Location !=
83-
otherNginxConfigContext.StubStatus.Location {
84-
return false
81+
if ncc.StubStatus != nil && otherNginxConfigContext.StubStatus != nil {
82+
if ncc.StubStatus.URL != otherNginxConfigContext.StubStatus.URL || ncc.StubStatus.Listen !=
83+
otherNginxConfigContext.StubStatus.Listen || ncc.StubStatus.Location !=
84+
otherNginxConfigContext.StubStatus.Location {
85+
return false
86+
}
8587
}
8688

87-
if ncc.PlusAPI.URL != otherNginxConfigContext.PlusAPI.URL || ncc.PlusAPI.Listen !=
88-
otherNginxConfigContext.PlusAPI.Listen || ncc.PlusAPI.Location !=
89-
otherNginxConfigContext.PlusAPI.Location {
90-
return false
89+
if ncc.PlusAPI != nil && otherNginxConfigContext.PlusAPI != nil {
90+
if ncc.PlusAPI.URL != otherNginxConfigContext.PlusAPI.URL || ncc.PlusAPI.Listen !=
91+
otherNginxConfigContext.PlusAPI.Listen || ncc.PlusAPI.Location !=
92+
otherNginxConfigContext.PlusAPI.Location {
93+
return false
94+
}
9195
}
9296

9397
if ncc.InstanceID != otherNginxConfigContext.InstanceID {

‎internal/model/config_test.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,10 @@ func TestNginxConfigContext_Equal(t *testing.T) {
8181
nginxConfigContextWithDifferentErrorLogs := *nginxConfigContext
8282
nginxConfigContextWithDifferentErrorLogs.ErrorLogs = []*ErrorLog{}
8383

84+
nginxConfigContextWithNilValues := *nginxConfigContext
85+
nginxConfigContextWithNilValues.StubStatus = nil
86+
nginxConfigContextWithNilValues.PlusAPI = nil
87+
8488
assert.True(t, nginxConfigContext.Equal(&nginxConfigContextWithSameValues))
8589
assert.False(t, nginxConfigContext.Equal(&nginxConfigContextWithDifferentStubStatus))
8690
assert.False(t, nginxConfigContext.Equal(&nginxConfigContextWithDifferentPlusAPI))
@@ -89,4 +93,5 @@ func TestNginxConfigContext_Equal(t *testing.T) {
8993
assert.False(t, nginxConfigContext.Equal(&nginxConfigContextWithDifferentFileHashes))
9094
assert.False(t, nginxConfigContext.Equal(&nginxConfigContextWithDifferentAccessLogs))
9195
assert.False(t, nginxConfigContext.Equal(&nginxConfigContextWithDifferentErrorLogs))
96+
assert.True(t, nginxConfigContext.Equal(&nginxConfigContextWithNilValues))
9297
}

‎internal/watcher/instance/instance_watcher_service.go‎

Lines changed: 21 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -126,15 +126,33 @@ func (iw *InstanceWatcherService) Watch(
126126
func (iw *InstanceWatcherService) ReparseConfigs(ctx context.Context) {
127127
slog.DebugContext(ctx, "Reparsing all instance configurations")
128128
for _, instance := range iw.instanceCache {
129-
iw.ReparseConfig(ctx, instance.GetInstanceMeta().GetInstanceId(), &model.NginxConfigContext{})
129+
slog.DebugContext(
130+
ctx,
131+
"Reparsing NGINX instance config",
132+
"instance_id", instance.GetInstanceMeta().GetInstanceId(),
133+
)
134+
135+
nginxConfigContext, parseErr := iw.nginxConfigParser.Parse(ctx, instance)
136+
if parseErr != nil {
137+
slog.ErrorContext(
138+
ctx,
139+
"Failed to parse NGINX instance config",
140+
"config_path", instance.GetInstanceRuntime().GetConfigPath(),
141+
"instance_id", instance.GetInstanceMeta().GetInstanceId(),
142+
"error", parseErr,
143+
)
144+
145+
return
146+
}
147+
148+
iw.HandleNginxConfigContextUpdate(ctx, instance.GetInstanceMeta().GetInstanceId(), nginxConfigContext)
130149
}
131150
slog.DebugContext(ctx, "Finished reparsing all instance configurations")
132151
}
133152

134-
func (iw *InstanceWatcherService) ReparseConfig(ctx context.Context, instanceID string,
153+
func (iw *InstanceWatcherService) HandleNginxConfigContextUpdate(ctx context.Context, instanceID string,
135154
nginxConfigContext *model.NginxConfigContext,
136155
) {
137-
var parseErr error
138156
iw.cacheMutex.Lock()
139157
defer iw.cacheMutex.Unlock()
140158

@@ -145,29 +163,6 @@ func (iw *InstanceWatcherService) ReparseConfig(ctx context.Context, instanceID
145163

146164
if instanceType == mpi.InstanceMeta_INSTANCE_TYPE_NGINX ||
147165
instanceType == mpi.InstanceMeta_INSTANCE_TYPE_NGINX_PLUS {
148-
// If the ReparseConfig was not triggered by a config apply that had changes there is no NginxConfigContext
149-
// passed to this function. So we need to parse the config and create one.
150-
if nginxConfigContext.InstanceID == "" {
151-
slog.DebugContext(
152-
ctx,
153-
"Reparsing NGINX instance config",
154-
"instance_id", instanceID,
155-
)
156-
157-
nginxConfigContext, parseErr = iw.nginxConfigParser.Parse(ctx, instance)
158-
if parseErr != nil {
159-
slog.WarnContext(
160-
ctx,
161-
"Unable to parse NGINX instance config",
162-
"config_path", instance.GetInstanceRuntime().GetConfigPath(),
163-
"instance_id", instanceID,
164-
"error", parseErr,
165-
)
166-
167-
return
168-
}
169-
}
170-
171166
iw.sendNginxConfigContextUpdate(ctx, nginxConfigContext)
172167
iw.nginxConfigCache[nginxConfigContext.InstanceID] = nginxConfigContext
173168
updatesRequired = proto.UpdateNginxInstanceRuntime(instance, nginxConfigContext)

‎internal/watcher/instance/instance_watcher_service_test.go‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,8 @@ func TestInstanceWatcherService_ReparseConfig(t *testing.T) {
263263
updatedInstance.GetInstanceRuntime().GetNginxRuntimeInfo().AccessLogs = []string{"access2.log"}
264264
updatedInstance.GetInstanceRuntime().GetNginxRuntimeInfo().ErrorLogs = []string{"error.log"}
265265

266+
updateNginxConfigContext.InstanceID = updatedInstance.GetInstanceMeta().GetInstanceId()
267+
266268
tests := []struct {
267269
parseReturns *model.NginxConfigContext
268270
name string
@@ -275,13 +277,10 @@ func TestInstanceWatcherService_ReparseConfig(t *testing.T) {
275277

276278
for _, test := range tests {
277279
t.Run(test.name, func(tt *testing.T) {
278-
fakeNginxConfigParser := &instancefakes.FakeNginxConfigParser{}
279-
fakeNginxConfigParser.ParseReturns(test.parseReturns, nil)
280280
instanceUpdatesChannel := make(chan InstanceUpdatesMessage, 1)
281281
nginxConfigContextChannel := make(chan NginxConfigContextMessage, 1)
282282

283283
instanceWatcherService := NewInstanceWatcherService(types.AgentConfig())
284-
instanceWatcherService.nginxConfigParser = fakeNginxConfigParser
285284
instanceWatcherService.instancesChannel = instanceUpdatesChannel
286285
instanceWatcherService.nginxConfigContextChannel = nginxConfigContextChannel
287286

@@ -293,12 +292,16 @@ func TestInstanceWatcherService_ReparseConfig(t *testing.T) {
293292
instance.GetInstanceMeta().GetInstanceId(): instance,
294293
}
295294

296-
instanceWatcherService.ReparseConfig(ctx, updatedInstance.GetInstanceMeta().GetInstanceId(),
297-
&model.NginxConfigContext{})
295+
instanceWatcherService.HandleNginxConfigContextUpdate(ctx, updatedInstance.
296+
GetInstanceMeta().GetInstanceId(),
297+
updateNginxConfigContext)
298298

299299
nginxConfigContextMessage := <-nginxConfigContextChannel
300300
assert.Equal(t, updateNginxConfigContext, nginxConfigContextMessage.NginxConfigContext)
301301

302+
assert.Equal(tt, updateNginxConfigContext, instanceWatcherService.
303+
nginxConfigCache[updatedInstance.GetInstanceMeta().GetInstanceId()])
304+
302305
instanceUpdatesMessage := <-instanceUpdatesChannel
303306
assert.Len(t, instanceUpdatesMessage.InstanceUpdates.UpdatedInstances, 1)
304307
assert.Equal(tt, updatedInstance, instanceUpdatesMessage.InstanceUpdates.UpdatedInstances[0])

‎internal/watcher/watcher_plugin.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ type (
5757
instancesChannel chan<- instance.InstanceUpdatesMessage,
5858
nginxConfigContextChannel chan<- instance.NginxConfigContextMessage,
5959
)
60-
ReparseConfig(ctx context.Context, instanceID string, configContext *model.NginxConfigContext)
60+
HandleNginxConfigContextUpdate(ctx context.Context, instanceID string, configContext *model.NginxConfigContext)
6161
ReparseConfigs(ctx context.Context)
6262
SetEnabled(enabled bool)
6363
}
@@ -202,7 +202,7 @@ func (w *Watcher) handleConfigApplySuccess(ctx context.Context, msg *bus.Message
202202
slog.DebugContext(ctx, "NginxConfigContext is empty, no need to reparse config")
203203
return
204204
}
205-
w.instanceWatcherService.ReparseConfig(ctx, instanceID, successMessage.ConfigContext)
205+
w.instanceWatcherService.HandleNginxConfigContextUpdate(ctx, instanceID, successMessage.ConfigContext)
206206

207207
w.watcherMutex.Lock()
208208
w.instancesWithConfigApplyInProgress = slices.DeleteFunc(

‎internal/watcher/watcher_plugin_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,7 @@ func TestWatcher_Process_ConfigApplySuccessfulTopic(t *testing.T) {
196196

197197
watcherPlugin.Process(ctx, message)
198198

199-
assert.Equal(t, 1, fakeWatcherService.ReparseConfigCallCount())
199+
assert.Equal(t, 1, fakeWatcherService.HandleNginxConfigContextUpdateCallCount())
200200
assert.Empty(t, watcherPlugin.instancesWithConfigApplyInProgress)
201201
}
202202

‎internal/watcher/watcherfakes/fake_instance_watcher_service_interface.go‎

Lines changed: 24 additions & 24 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎test/integration/grpc_management_plane_api_test.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -310,7 +310,6 @@ func TestGrpc_ConfigApply(t *testing.T) {
310310

311311
performConfigApply(t, nginxInstanceID)
312312

313-
time.Sleep(5 * time.Second)
314313
responses = getManagementPlaneResponses(t, 2)
315314
t.Logf("Config apply responses: %v", responses)
316315

0 commit comments

Comments
 (0)