Skip to content

Commit 68f3af9

Browse files
sjbermansjberman
committed
Fix status updates for non-nginx changes
1 parent afe712f commit 68f3af9

File tree

5 files changed

+41
-144
lines changed

5 files changed

+41
-144
lines changed

‎internal/mode/static/handler.go‎

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -218,21 +218,19 @@ func (h *eventHandlerImpl) sendNginxConfig(ctx context.Context, logger logr.Logg
218218
h.setLatestConfiguration(gw, &cfg)
219219

220220
deployment.FileLock.Lock()
221-
configApplied := h.updateNginxConf(deployment, cfg)
221+
h.updateNginxConf(deployment, cfg)
222222
deployment.FileLock.Unlock()
223223

224224
configErr := deployment.GetLatestConfigError()
225225
upstreamErr := deployment.GetLatestUpstreamError()
226226
err := errors.Join(configErr, upstreamErr)
227227

228-
if configApplied || err != nil {
229-
obj := &status.QueueObject{
230-
UpdateType: status.UpdateAll,
231-
Error: err,
232-
Deployment: gw.DeploymentName,
233-
}
234-
h.cfg.statusQueue.Enqueue(obj)
228+
obj := &status.QueueObject{
229+
UpdateType: status.UpdateAll,
230+
Error: err,
231+
Deployment: gw.DeploymentName,
235232
}
233+
h.cfg.statusQueue.Enqueue(obj)
236234
}
237235
}
238236

@@ -408,16 +406,14 @@ func (h *eventHandlerImpl) parseAndCaptureEvent(ctx context.Context, logger logr
408406
func (h *eventHandlerImpl) updateNginxConf(
409407
deployment *agent.Deployment,
410408
conf dataplane.Configuration,
411-
) bool {
409+
) {
412410
files := h.cfg.generator.Generate(conf)
413-
applied := h.cfg.nginxUpdater.UpdateConfig(deployment, files)
411+
h.cfg.nginxUpdater.UpdateConfig(deployment, files)
414412

415413
// If using NGINX Plus, update upstream servers using the API.
416414
if h.cfg.plus {
417-
applied = h.cfg.nginxUpdater.UpdateUpstreamServers(deployment, conf) || applied
415+
h.cfg.nginxUpdater.UpdateUpstreamServers(deployment, conf)
418416
}
419-
420-
return applied
421417
}
422418

423419
// updateControlPlaneAndSetStatus updates the control plane configuration and then sets the status

‎internal/mode/static/handler_test.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,6 @@ var _ = Describe("eventHandler", func() {
102102
fakeProcessor.GetLatestGraphReturns(baseGraph)
103103
fakeGenerator = &configfakes.FakeGenerator{}
104104
fakeNginxUpdater = &agentfakes.FakeNginxUpdater{}
105-
fakeNginxUpdater.UpdateConfigReturns(true)
106105
fakeProvisioner = &provisionerfakes.FakeProvisioner{}
107106
fakeProvisioner.RegisterGatewayReturns(nil)
108107
fakeStatusUpdater = &statusfakes.FakeGroupUpdater{}

‎internal/mode/static/nginx/agent/agent.go‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,8 @@ const retryUpstreamTimeout = 5 * time.Second
2828

2929
// NginxUpdater is an interface for updating NGINX using the NGINX agent.
3030
type NginxUpdater interface {
31-
UpdateConfig(deployment *Deployment, files []File) bool
32-
UpdateUpstreamServers(deployment *Deployment, conf dataplane.Configuration) bool
31+
UpdateConfig(deployment *Deployment, files []File)
32+
UpdateUpstreamServers(deployment *Deployment, conf dataplane.Configuration)
3333
}
3434

3535
// NginxUpdaterImpl implements the NginxUpdater interface.
@@ -74,7 +74,6 @@ func NewNginxUpdater(
7474
}
7575

7676
// UpdateConfig sends the nginx configuration to the agent.
77-
// Returns whether the configuration was sent to any agents.
7877
//
7978
// The flow of events is as follows:
8079
// - Set the configuration files on the deployment.
@@ -87,10 +86,10 @@ func NewNginxUpdater(
8786
func (n *NginxUpdaterImpl) UpdateConfig(
8887
deployment *Deployment,
8988
files []File,
90-
) bool {
89+
) {
9190
msg := deployment.SetFiles(files)
9291
if msg == nil {
93-
return false
92+
return
9493
}
9594

9695
applied := deployment.GetBroadcaster().Send(*msg)
@@ -99,19 +98,16 @@ func (n *NginxUpdaterImpl) UpdateConfig(
9998
}
10099

101100
deployment.SetLatestConfigError(deployment.GetConfigurationStatus())
102-
103-
return applied
104101
}
105102

106103
// UpdateUpstreamServers sends an APIRequest to the agent to update upstream servers using the NGINX Plus API.
107104
// Only applicable when using NGINX Plus.
108-
// Returns whether the configuration was sent to any agents.
109105
func (n *NginxUpdaterImpl) UpdateUpstreamServers(
110106
deployment *Deployment,
111107
conf dataplane.Configuration,
112-
) bool {
108+
) {
113109
if !n.plus {
114-
return false
110+
return
115111
}
116112

117113
broadcaster := deployment.GetBroadcaster()
@@ -141,7 +137,7 @@ func (n *NginxUpdaterImpl) UpdateUpstreamServers(
141137
}
142138

143139
if actionsEqual(deployment.GetNGINXPlusActions(), actions) {
144-
return false
140+
return
145141
}
146142

147143
for _, action := range actions {
@@ -166,8 +162,6 @@ func (n *NginxUpdaterImpl) UpdateUpstreamServers(
166162

167163
// Store the most recent actions on the deployment so any new subscribers can apply them when first connecting.
168164
deployment.SetNGINXPlusActions(actions)
169-
170-
return applied
171165
}
172166

173167
func buildHTTPUpstreamServers(upstream dataplane.Upstream) *pb.UpdateHTTPUpstreamServers {

‎internal/mode/static/nginx/agent/agent_test.go‎

Lines changed: 17 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -21,24 +21,16 @@ func TestUpdateConfig(t *testing.T) {
2121
t.Parallel()
2222

2323
tests := []struct {
24-
name string
25-
configApplied bool
26-
expErr bool
24+
name string
25+
expErr bool
2726
}{
2827
{
29-
name: "success",
30-
configApplied: true,
31-
expErr: false,
28+
name: "success",
29+
expErr: false,
3230
},
3331
{
34-
name: "error returned from agent",
35-
configApplied: true,
36-
expErr: true,
37-
},
38-
{
39-
name: "configuration not applied",
40-
configApplied: false,
41-
expErr: false,
32+
name: "error returned from agent",
33+
expErr: true,
4234
},
4335
}
4436

@@ -48,7 +40,7 @@ func TestUpdateConfig(t *testing.T) {
4840
g := NewWithT(t)
4941

5042
fakeBroadcaster := &broadcastfakes.FakeBroadcaster{}
51-
fakeBroadcaster.SendReturns(test.configApplied)
43+
fakeBroadcaster.SendReturns(true)
5244

5345
plus := false
5446
updater := NewNginxUpdater(logr.Discard(), fake.NewFakeClient(), &status.Queue{}, nil, plus)
@@ -70,9 +62,9 @@ func TestUpdateConfig(t *testing.T) {
7062
deployment.SetPodErrorStatus("pod1", testErr)
7163
}
7264

73-
applied := updater.UpdateConfig(deployment, []File{file})
65+
updater.UpdateConfig(deployment, []File{file})
7466

75-
g.Expect(applied).To(Equal(test.configApplied))
67+
g.Expect(fakeBroadcaster.SendCallCount()).To(Equal(1))
7668
g.Expect(deployment.GetFile(file.Meta.Name, file.Meta.Hash)).To(Equal(file.Contents))
7769

7870
if test.expErr {
@@ -114,10 +106,9 @@ func TestUpdateConfig_NoChange(t *testing.T) {
114106
deployment.SetFiles([]File{file})
115107

116108
// Call UpdateConfig with the same files
117-
applied := updater.UpdateConfig(deployment, []File{file})
109+
updater.UpdateConfig(deployment, []File{file})
118110

119111
// Verify that no new configuration was sent
120-
g.Expect(applied).To(BeFalse())
121112
g.Expect(fakeBroadcaster.SendCallCount()).To(Equal(0))
122113
}
123114

@@ -128,43 +119,31 @@ func TestUpdateUpstreamServers(t *testing.T) {
128119
name string
129120
buildUpstreams bool
130121
plus bool
131-
configApplied bool
132122
expErr bool
133123
}{
134124
{
135125
name: "success",
136126
plus: true,
137127
buildUpstreams: true,
138-
configApplied: true,
139128
expErr: false,
140129
},
141130
{
142131
name: "no upstreams to apply",
143132
plus: true,
144133
buildUpstreams: false,
145-
configApplied: false,
146134
expErr: false,
147135
},
148136
{
149-
name: "not running nginx plus",
150-
plus: false,
151-
configApplied: false,
152-
expErr: false,
137+
name: "not running nginx plus",
138+
plus: false,
139+
expErr: false,
153140
},
154141
{
155142
name: "error returned from agent",
156143
plus: true,
157144
buildUpstreams: true,
158-
configApplied: true,
159145
expErr: true,
160146
},
161-
{
162-
name: "configuration not applied",
163-
plus: true,
164-
buildUpstreams: true,
165-
configApplied: false,
166-
expErr: false,
167-
},
168147
}
169148

170149
for _, test := range tests {
@@ -173,7 +152,6 @@ func TestUpdateUpstreamServers(t *testing.T) {
173152
g := NewWithT(t)
174153

175154
fakeBroadcaster := &broadcastfakes.FakeBroadcaster{}
176-
fakeBroadcaster.SendReturns(test.configApplied)
177155

178156
updater := NewNginxUpdater(logr.Discard(), fake.NewFakeClient(), &status.Queue{}, nil, test.plus)
179157
updater.retryTimeout = 0
@@ -215,8 +193,7 @@ func TestUpdateUpstreamServers(t *testing.T) {
215193
}
216194
}
217195

218-
applied := updater.UpdateUpstreamServers(deployment, conf)
219-
g.Expect(applied).To(Equal(test.configApplied))
196+
updater.UpdateUpstreamServers(deployment, conf)
220197

221198
expActions := make([]*pb.NGINXPlusAction, 0)
222199
if test.buildUpstreams {
@@ -254,8 +231,10 @@ func TestUpdateUpstreamServers(t *testing.T) {
254231

255232
if !test.plus {
256233
g.Expect(deployment.GetNGINXPlusActions()).To(BeNil())
234+
g.Expect(fakeBroadcaster.SendCallCount()).To(Equal(0))
257235
} else if test.buildUpstreams {
258236
g.Expect(deployment.GetNGINXPlusActions()).To(Equal(expActions))
237+
g.Expect(fakeBroadcaster.SendCallCount()).To(Equal(2))
259238
}
260239

261240
if test.expErr {
@@ -347,10 +326,9 @@ func TestUpdateUpstreamServers_NoChange(t *testing.T) {
347326
deployment.SetNGINXPlusActions(initialActions)
348327

349328
// Call UpdateUpstreamServers with the same configuration
350-
applied := updater.UpdateUpstreamServers(deployment, conf)
329+
updater.UpdateUpstreamServers(deployment, conf)
351330

352331
// Verify that no new actions were sent
353-
g.Expect(applied).To(BeFalse())
354332
g.Expect(fakeBroadcaster.SendCallCount()).To(Equal(0))
355333
}
356334

0 commit comments

Comments
 (0)