From d7b261dbda403488ce4b6e98ddf8a03263d138ef Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Mon, 11 Sep 2023 11:31:09 +0100 Subject: [PATCH] Move version state to event handler --- internal/mode/static/handler.go | 10 ++++++++-- internal/mode/static/handler_test.go | 6 +++--- internal/mode/static/manager.go | 2 +- .../nginx/config/configfakes/fake_generator.go | 12 ++++++------ internal/mode/static/nginx/config/generator.go | 18 ++++++------------ .../mode/static/nginx/config/generator_test.go | 2 +- .../static/state/dataplane/configuration.go | 14 ++++++++++---- .../state/dataplane/configuration_test.go | 3 ++- 8 files changed, 37 insertions(+), 30 deletions(-) diff --git a/internal/mode/static/handler.go b/internal/mode/static/handler.go index 126adeba49..144cc0adc2 100644 --- a/internal/mode/static/handler.go +++ b/internal/mode/static/handler.go @@ -45,6 +45,8 @@ type eventHandlerConfig struct { logger logr.Logger // controlConfigNSName is the NamespacedName of the NginxGateway config for this controller. controlConfigNSName types.NamespacedName + // version is the current version number of the nginx config. + version int } // eventHandlerImpl implements EventHandler. @@ -90,7 +92,11 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, batch events.Ev } var nginxReloadRes nginxReloadResult - if err := h.updateNginx(ctx, dataplane.BuildConfiguration(ctx, graph, h.cfg.serviceResolver)); err != nil { + h.cfg.version++ + if err := h.updateNginx( + ctx, + dataplane.BuildConfiguration(ctx, graph, h.cfg.serviceResolver, h.cfg.version), + ); err != nil { h.cfg.logger.Error(err, "Failed to update NGINX configuration") nginxReloadRes.error = err } else { @@ -100,7 +106,7 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, batch events.Ev h.cfg.statusUpdater.Update(ctx, buildStatuses(graph, nginxReloadRes)) } -func (h *eventHandlerImpl) updateNginx(ctx context.Context, conf *dataplane.Configuration) error { +func (h *eventHandlerImpl) updateNginx(ctx context.Context, conf dataplane.Configuration) error { files := h.cfg.generator.Generate(conf) if err := h.cfg.nginxFileMgr.ReplaceFiles(files); err != nil { diff --git a/internal/mode/static/handler_test.go b/internal/mode/static/handler_test.go index db0a88c39b..a6b0fa4a42 100644 --- a/internal/mode/static/handler_test.go +++ b/internal/mode/static/handler_test.go @@ -45,7 +45,7 @@ var _ = Describe("eventHandler", func() { Expect(fakeProcessor.ProcessCallCount()).Should(Equal(1)) Expect(fakeGenerator.GenerateCallCount()).Should(Equal(1)) - Expect(fakeGenerator.GenerateArgsForCall(0)).Should(Equal(&expectedConf)) + Expect(fakeGenerator.GenerateArgsForCall(0)).Should(Equal(expectedConf)) Expect(fakeNginxFileMgr.ReplaceFilesCallCount()).Should(Equal(1)) files := fakeNginxFileMgr.ReplaceFilesArgsForCall(0) @@ -111,7 +111,7 @@ var _ = Describe("eventHandler", func() { handler.HandleEventBatch(context.Background(), batch) checkUpsertEventExpectations(e) - expectReconfig(dataplane.Configuration{}, fakeCfgFiles) + expectReconfig(dataplane.Configuration{Version: 1}, fakeCfgFiles) }) It("should process Delete", func() { @@ -124,7 +124,7 @@ var _ = Describe("eventHandler", func() { handler.HandleEventBatch(context.Background(), batch) checkDeleteEventExpectations(e) - expectReconfig(dataplane.Configuration{}, fakeCfgFiles) + expectReconfig(dataplane.Configuration{Version: 1}, fakeCfgFiles) }) }) diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index be8599738d..ac5a239775 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -212,7 +212,7 @@ func StartManager(cfg config.Config) error { eventHandler := newEventHandlerImpl(eventHandlerConfig{ processor: processor, serviceResolver: resolver.NewServiceResolverImpl(mgr.GetClient()), - generator: &configGenerator, + generator: configGenerator, logger: cfg.Logger.WithName("eventHandler"), logLevelSetter: logLevelSetter, nginxFileMgr: nginxFileMgr, diff --git a/internal/mode/static/nginx/config/configfakes/fake_generator.go b/internal/mode/static/nginx/config/configfakes/fake_generator.go index 0c14817a48..4b058a3553 100644 --- a/internal/mode/static/nginx/config/configfakes/fake_generator.go +++ b/internal/mode/static/nginx/config/configfakes/fake_generator.go @@ -10,10 +10,10 @@ import ( ) type FakeGenerator struct { - GenerateStub func(*dataplane.Configuration) []file.File + GenerateStub func(dataplane.Configuration) []file.File generateMutex sync.RWMutex generateArgsForCall []struct { - arg1 *dataplane.Configuration + arg1 dataplane.Configuration } generateReturns struct { result1 []file.File @@ -25,11 +25,11 @@ type FakeGenerator struct { invocationsMutex sync.RWMutex } -func (fake *FakeGenerator) Generate(arg1 *dataplane.Configuration) []file.File { +func (fake *FakeGenerator) Generate(arg1 dataplane.Configuration) []file.File { fake.generateMutex.Lock() ret, specificReturn := fake.generateReturnsOnCall[len(fake.generateArgsForCall)] fake.generateArgsForCall = append(fake.generateArgsForCall, struct { - arg1 *dataplane.Configuration + arg1 dataplane.Configuration }{arg1}) stub := fake.GenerateStub fakeReturns := fake.generateReturns @@ -50,13 +50,13 @@ func (fake *FakeGenerator) GenerateCallCount() int { return len(fake.generateArgsForCall) } -func (fake *FakeGenerator) GenerateCalls(stub func(*dataplane.Configuration) []file.File) { +func (fake *FakeGenerator) GenerateCalls(stub func(dataplane.Configuration) []file.File) { fake.generateMutex.Lock() defer fake.generateMutex.Unlock() fake.GenerateStub = stub } -func (fake *FakeGenerator) GenerateArgsForCall(i int) *dataplane.Configuration { +func (fake *FakeGenerator) GenerateArgsForCall(i int) dataplane.Configuration { fake.generateMutex.RLock() defer fake.generateMutex.RUnlock() argsForCall := fake.generateArgsForCall[i] diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index b9fd10b508..851d8147f8 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/internal/mode/static/nginx/config/generator.go @@ -32,7 +32,7 @@ var ConfigFolders = []string{httpFolder, secretsFolder} // This interface is used for testing purposes only. type Generator interface { // Generate generates NGINX configuration files from internal representation. - Generate(configuration *dataplane.Configuration) []file.File + Generate(configuration dataplane.Configuration) []file.File } // GeneratorImpl is an implementation of Generator. @@ -43,15 +43,11 @@ type Generator interface { // // It also expects that the main NGINX configuration file nginx.conf is located in configFolder and nginx.conf // includes (https://nginx.org/en/docs/ngx_core_module.html#include) the files from httpFolder. -type GeneratorImpl struct { - configVersion int -} +type GeneratorImpl struct{} // NewGeneratorImpl creates a new GeneratorImpl. func NewGeneratorImpl() GeneratorImpl { - return GeneratorImpl{ - configVersion: 0, - } + return GeneratorImpl{} } // executeFunc is a function that generates NGINX configuration from internal representation. @@ -61,18 +57,16 @@ type executeFunc func(configuration dataplane.Configuration) []byte // It is the responsibility of the caller to validate the configuration before calling this function. // In case of invalid configuration, NGINX will fail to reload or could be configured with malicious configuration. // To validate, use the validators from the validation package. -func (g *GeneratorImpl) Generate(conf *dataplane.Configuration) []file.File { - g.configVersion++ - conf.Version = g.configVersion +func (g GeneratorImpl) Generate(conf dataplane.Configuration) []file.File { files := make([]file.File, 0, len(conf.SSLKeyPairs)+1 /* http config */) for id, pair := range conf.SSLKeyPairs { files = append(files, generatePEM(id, pair.Cert, pair.Key)) } - files = append(files, generateHTTPConfig(*conf)) + files = append(files, generateHTTPConfig(conf)) - files = append(files, generateConfigVersion(g.configVersion)) + files = append(files, generateConfigVersion(conf.Version)) return files } diff --git a/internal/mode/static/nginx/config/generator_test.go b/internal/mode/static/nginx/config/generator_test.go index 774f498e40..983185d2c0 100644 --- a/internal/mode/static/nginx/config/generator_test.go +++ b/internal/mode/static/nginx/config/generator_test.go @@ -64,7 +64,7 @@ func TestGenerate(t *testing.T) { generator := config.NewGeneratorImpl() - files := generator.Generate(&conf) + files := generator.Generate(conf) g.Expect(files).To(HaveLen(3)) diff --git a/internal/mode/static/state/dataplane/configuration.go b/internal/mode/static/state/dataplane/configuration.go index eab1fd21ed..d438c1ec66 100644 --- a/internal/mode/static/state/dataplane/configuration.go +++ b/internal/mode/static/state/dataplane/configuration.go @@ -16,13 +16,18 @@ import ( const wildcardHostname = "~^" // BuildConfiguration builds the Configuration from the Graph. -func BuildConfiguration(ctx context.Context, g *graph.Graph, resolver resolver.ServiceResolver) *Configuration { +func BuildConfiguration( + ctx context.Context, + g *graph.Graph, + resolver resolver.ServiceResolver, + configVersion int, +) Configuration { if g.GatewayClass == nil || !g.GatewayClass.Valid { - return &Configuration{} + return Configuration{Version: configVersion} } if g.Gateway == nil { - return &Configuration{} + return Configuration{Version: configVersion} } upstreams := buildUpstreams(ctx, g.Gateway.Listeners, resolver) @@ -36,9 +41,10 @@ func BuildConfiguration(ctx context.Context, g *graph.Graph, resolver resolver.S Upstreams: upstreams, BackendGroups: backendGroups, SSLKeyPairs: keyPairs, + Version: configVersion, } - return &config + return config } // buildSSLKeyPairs builds the SSLKeyPairs from the Secrets. It will only include Secrets that are referenced by diff --git a/internal/mode/static/state/dataplane/configuration_test.go b/internal/mode/static/state/dataplane/configuration_test.go index 9e722d0e01..cff5bc8592 100644 --- a/internal/mode/static/state/dataplane/configuration_test.go +++ b/internal/mode/static/state/dataplane/configuration_test.go @@ -1488,13 +1488,14 @@ func TestBuildConfiguration(t *testing.T) { t.Run(test.msg, func(t *testing.T) { g := NewGomegaWithT(t) - result := BuildConfiguration(context.TODO(), test.graph, fakeResolver) + result := BuildConfiguration(context.TODO(), test.graph, fakeResolver, 1) g.Expect(result.BackendGroups).To(ConsistOf(test.expConf.BackendGroups)) g.Expect(result.Upstreams).To(ConsistOf(test.expConf.Upstreams)) g.Expect(result.HTTPServers).To(ConsistOf(test.expConf.HTTPServers)) g.Expect(result.SSLServers).To(ConsistOf(test.expConf.SSLServers)) g.Expect(result.SSLKeyPairs).To(Equal(test.expConf.SSLKeyPairs)) + g.Expect(result.Version).To(Equal(1)) }) } }