Skip to content

Commit 88678b4

Browse files
sjbermansjberman
committed
Fix channels and blocking; apply config on connection
1 parent fac256c commit 88678b4

File tree

10 files changed

+306
-174
lines changed

10 files changed

+306
-174
lines changed

‎internal/mode/static/handler.go‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,7 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
169169
// If scaled down, we should remove the pod from the ConnectionsTracker
170170
// If fully deleted, then delete the deployment from the Store
171171
var err error
172+
var configApplied bool
172173
switch changeType {
173174
case state.NoChange:
174175
logger.Info("Handling events didn't result into NGINX configuration changes")
@@ -190,9 +191,9 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
190191
Name: "tmp-nginx-deployment",
191192
Namespace: h.cfg.gatewayPodConfig.Namespace,
192193
}
193-
err = h.cfg.nginxUpdater.UpdateUpstreamServers(ctx, deployment, cfg)
194+
configApplied, err = h.cfg.nginxUpdater.UpdateUpstreamServers(ctx, deployment, cfg)
194195
} else {
195-
err = h.updateNginxConf(ctx, cfg)
196+
configApplied, err = h.updateNginxConf(ctx, cfg)
196197
}
197198
case state.ClusterStateChange:
198199
h.version++
@@ -205,20 +206,25 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
205206

206207
h.setLatestConfiguration(&cfg)
207208

208-
err = h.updateNginxConf(ctx, cfg)
209+
configApplied, err = h.updateNginxConf(ctx, cfg)
209210
}
210211

211212
var nginxReloadRes status.NginxReloadResult
212-
if err != nil {
213+
switch {
214+
case err != nil:
213215
logger.Error(err, "Failed to update NGINX configuration")
214216
nginxReloadRes.Error = err
215-
} else {
217+
case configApplied:
216218
logger.Info("NGINX configuration was successfully updated")
219+
default:
220+
logger.Info("No NGINX instances to configure")
217221
}
218222

219223
h.latestReloadResult = nginxReloadRes
220224

221-
h.updateStatuses(ctx, logger, gr)
225+
if configApplied || err != nil {
226+
h.updateStatuses(ctx, logger, gr)
227+
}
222228
}
223229

224230
func (h *eventHandlerImpl) updateStatuses(ctx context.Context, logger logr.Logger, gr *graph.Graph) {
@@ -304,7 +310,7 @@ func (h *eventHandlerImpl) parseAndCaptureEvent(ctx context.Context, logger logr
304310
}
305311

306312
// updateNginxConf updates nginx conf files and reloads nginx.
307-
func (h *eventHandlerImpl) updateNginxConf(ctx context.Context, conf dataplane.Configuration) error {
313+
func (h *eventHandlerImpl) updateNginxConf(ctx context.Context, conf dataplane.Configuration) (bool, error) {
308314
files := h.cfg.generator.Generate(conf)
309315

310316
// TODO(sberman): hardcode this deployment name until we support provisioning data planes
@@ -313,18 +319,21 @@ func (h *eventHandlerImpl) updateNginxConf(ctx context.Context, conf dataplane.C
313319
Namespace: h.cfg.gatewayPodConfig.Namespace,
314320
}
315321

316-
if err := h.cfg.nginxUpdater.UpdateConfig(ctx, deployment, files); err != nil {
317-
return err
322+
applied, err := h.cfg.nginxUpdater.UpdateConfig(ctx, deployment, files)
323+
if err != nil {
324+
return false, err
318325
}
319326

320327
// If using NGINX Plus, update upstream servers using the API.
328+
var plusApplied bool
321329
if h.cfg.plus {
322-
if err := h.cfg.nginxUpdater.UpdateUpstreamServers(ctx, deployment, conf); err != nil {
323-
return err
330+
plusApplied, err = h.cfg.nginxUpdater.UpdateUpstreamServers(ctx, deployment, conf)
331+
if err != nil {
332+
return false, err
324333
}
325334
}
326335

327-
return nil
336+
return applied || plusApplied, nil
328337
}
329338

330339
// updateControlPlaneAndSetStatus updates the control plane configuration and then sets the status
@@ -440,6 +449,8 @@ func (h *eventHandlerImpl) GetLatestConfiguration() *dataplane.Configuration {
440449
}
441450

442451
// setLatestConfiguration sets the latest configuration.
452+
// TODO(sberman): once we support multiple Gateways, this will likely have to be a map
453+
// of all configurations.
443454
func (h *eventHandlerImpl) setLatestConfiguration(cfg *dataplane.Configuration) {
444455
h.lock.Lock()
445456
defer h.lock.Unlock()

‎internal/mode/static/handler_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ var _ = Describe("eventHandler", func() {
8484
fakeProcessor.ProcessReturns(state.NoChange, &graph.Graph{})
8585
fakeGenerator = &configfakes.FakeGenerator{}
8686
fakeNginxUpdater = &agentfakes.FakeNginxUpdater{}
87+
fakeNginxUpdater.UpdateConfigReturns(true, nil)
8788
fakeStatusUpdater = &statusfakes.FakeGroupUpdater{}
8889
fakeEventRecorder = record.NewFakeRecorder(1)
8990
zapLogLevelSetter = newZapLogLevelSetter(zap.NewAtomicLevel())

‎internal/mode/static/manager.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -505,6 +505,7 @@ func registerControllers(
505505
objectType: &ngfAPI.NginxGateway{},
506506
options: []controller.Option{
507507
controller.WithNamespacedNameFilter(filter.CreateSingleResourceFilter(controlConfigNSName)),
508+
controller.WithK8sPredicate(k8spredicate.GenerationChangedPredicate{}),
508509
},
509510
})
510511
if err := setInitialConfig(

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

Lines changed: 30 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -27,12 +27,12 @@ type NginxUpdater interface {
2727
ctx context.Context,
2828
deploymentNsName types.NamespacedName,
2929
files []File,
30-
) error
30+
) (bool, error)
3131
UpdateUpstreamServers(
3232
ctx context.Context,
3333
deploymentNsName types.NamespacedName,
3434
conf dataplane.Configuration,
35-
) error
35+
) (bool, error)
3636
}
3737

3838
// NginxUpdaterImpl implements the NginxUpdater interface.
@@ -66,12 +66,13 @@ func NewNginxUpdater(
6666
}
6767

6868
// UpdateConfig sends the nginx configuration to the agent.
69-
func (n *NginxUpdaterImpl) UpdateConfig(ctx context.Context, nsName types.NamespacedName, files []File) error {
69+
// Returns whether configuration was applied or not, and any error that occurred.
70+
func (n *NginxUpdaterImpl) UpdateConfig(ctx context.Context, nsName types.NamespacedName, files []File) (bool, error) {
7071
n.logger.Info("Sending nginx configuration to agent")
7172

7273
deployment := n.nginxDeployments.GetOrStore(ctx, nsName)
7374
if deployment == nil {
74-
return fmt.Errorf("failed to register nginx deployment %q", nsName.Name)
75+
return false, fmt.Errorf("failed to register nginx deployment %q", nsName.Name)
7576
}
7677

7778
// TODO(sberman): wait to send config until Deployment pods have all connected.
@@ -81,50 +82,61 @@ func (n *NginxUpdaterImpl) UpdateConfig(ctx context.Context, nsName types.Namesp
8182

8283
msg := deployment.SetFiles(files)
8384

84-
if err := deployment.GetBroadcaster().Send(msg); err != nil {
85-
return fmt.Errorf("could not set nginx files: %w", err)
85+
applied, err := deployment.GetBroadcaster().Send(msg)
86+
if err != nil {
87+
return false, fmt.Errorf("could not set nginx files: %w", err)
8688
}
8789

88-
return nil
90+
return applied, nil
8991
}
9092

9193
// UpdateUpstreamServers sends an APIRequest to the agent to update upstream servers using the NGINX Plus API.
9294
// Only applicable when using NGINX Plus.
95+
// Returns whether configuration was applied or not, and any error that occurred.
9396
func (n *NginxUpdaterImpl) UpdateUpstreamServers(
9497
ctx context.Context,
9598
nsName types.NamespacedName,
9699
conf dataplane.Configuration,
97-
) error {
100+
) (bool, error) {
98101
if !n.plus {
99-
return nil
102+
return false, nil
100103
}
101104

102105
n.logger.Info("Updating upstream servers using NGINX Plus API")
103106

104107
deployment := n.nginxDeployments.GetOrStore(ctx, nsName)
105108
if deployment == nil {
106-
return fmt.Errorf("failed to register nginx deployment %q", nsName.Name)
109+
return false, fmt.Errorf("failed to register nginx deployment %q", nsName.Name)
107110
}
108111
broadcaster := deployment.GetBroadcaster()
109112

110113
var updateErr error
114+
var applied bool
115+
actions := make([]*pb.NGINXPlusAction, 0, len(conf.Upstreams))
111116
for _, upstream := range conf.Upstreams {
112-
msg := broadcast.NginxAgentMessage{
113-
Type: broadcast.APIRequest,
114-
NGINXPlusAction: &pb.NGINXPlusAction{
115-
Action: &pb.NGINXPlusAction_UpdateHttpUpstreamServers{
116-
UpdateHttpUpstreamServers: buildUpstreamServers(upstream),
117-
},
117+
action := &pb.NGINXPlusAction{
118+
Action: &pb.NGINXPlusAction_UpdateHttpUpstreamServers{
119+
UpdateHttpUpstreamServers: buildUpstreamServers(upstream),
118120
},
119121
}
122+
actions = append(actions, action)
123+
124+
msg := broadcast.NginxAgentMessage{
125+
Type: broadcast.APIRequest,
126+
NGINXPlusAction: action,
127+
}
120128

121-
if err := broadcaster.Send(msg); err != nil {
129+
var err error
130+
applied, err = broadcaster.Send(msg)
131+
if err != nil {
122132
updateErr = errors.Join(updateErr, fmt.Errorf(
123133
"couldn't update upstream %q via the API: %w", upstream.Name, err))
124134
}
125135
}
136+
// Store the most recent actions on the deployment so any new subscribers can apply them when first connecting.
137+
deployment.SetNGINXPlusActions(actions)
126138

127-
return updateErr
139+
return applied, updateErr
128140
}
129141

130142
func buildUpstreamServers(upstream dataplane.Upstream) *pb.UpdateHTTPUpstreamServers {

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

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

0 commit comments

Comments
 (0)