Ensure NGINX reload occurs (#1033)

* Ensure reload occurs

Author: ciarams87

docs/architecture.md

@@ -106,7 +106,7 @@ API to update the handled resources' statuses and emit events.
    - Read: *NKG* reads the PID file `nginx.pid` from the `nginx-run` volume, located at `/var/run/nginx`. *NKG*
      extracts the PID of the nginx process from this file in order to send reload signals to *NGINX master*.
 4. (File I/O) *NKG* writes logs to its *stdout* and *stderr*, which are collected by the container runtime.
-5. (HTTP) *NKG* fetches the NGINX metrics via the unix:/var/lib/nginx/nginx-status.sock UNIX socket and converts it to
+5. (HTTP) *NKG* fetches the NGINX metrics via the unix:/var/run/nginx/nginx-status.sock UNIX socket and converts it to
    *Prometheus* format used in #2.
 6. (Signal) To reload NGINX, *NKG* sends the [reload signal][reload] to the **NGINX master**.
 7. (File I/O)
@@ -124,9 +124,12 @@ API to update the handled resources' statuses and emit events.
 9. (File I/O) The *NGINX master* sends logs to its *stdout* and *stderr*, which are collected by the container runtime.
 10. (File I/O) An *NGINX worker* writes logs to its *stdout* and *stderr*, which are collected by the container runtime.
 11. (Signal) The *NGINX master* controls the [lifecycle of *NGINX workers*][lifecycle] it creates workers with the new
-configuration and shutdowns workers with the old configuration.
-12. (HTTP,HTTPS) A *client* sends traffic to and receives traffic from any of the *NGINX workers* on ports 80 and 443.
-13. (HTTP,HTTPS) An *NGINX worker* sends traffic to and receives traffic from the *backends*.
+    configuration and shutdowns workers with the old configuration.
+12. (HTTP) To consider a configuration reload a success, *NKG* ensures that at least one NGINX worker has the new
+    configuration. To do that, *NKG* checks a particular endpoint via the unix:/var/run/nginx/nginx-config-version.sock
+    UNIX socket.
+13. (HTTP,HTTPS) A *client* sends traffic to and receives traffic from any of the *NGINX workers* on ports 80 and 443.
+14. (HTTP,HTTPS) An *NGINX worker* sends traffic to and receives traffic from the *backends*.
 
 [controller]: https://kubernetes.io/docs/concepts/architecture/controller/
 

docs/images/nkg-pod.png

@@ -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 {
@@ -107,7 +113,7 @@ func (h *eventHandlerImpl) updateNginx(ctx context.Context, conf dataplane.Confi
 		return fmt.Errorf("failed to replace NGINX configuration files: %w", err)
 	}
 
-	if err := h.cfg.nginxRuntimeMgr.Reload(ctx); err != nil {
+	if err := h.cfg.nginxRuntimeMgr.Reload(ctx, conf.Version); err != nil {
 		return fmt.Errorf("failed to reload NGINX: %w", err)
 	}
 

internal/mode/static/handler.go

@@ -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)
 			})
 		})
 

internal/mode/static/handler_test.go

@@ -20,6 +20,9 @@ const (
 
 	// httpConfigFile is the path to the configuration file with HTTP configuration.
 	httpConfigFile = httpFolder + "/http.conf"
+
+	// configVersionFile is the path to the config version configuration file.
+	configVersionFile = httpFolder + "/config-version.conf"
 )
 
 // ConfigFolders is a list of folders where NGINX configuration files are stored.
@@ -63,6 +66,8 @@ func (g GeneratorImpl) Generate(conf dataplane.Configuration) []file.File {
 
 	files = append(files, generateHTTPConfig(conf))
 
+	files = append(files, generateConfigVersion(conf.Version))
+
 	return files
 }
 
@@ -104,3 +109,14 @@ func getExecuteFuncs() []executeFunc {
 		executeMaps,
 	}
 }
+
+// generateConfigVersion writes the config version file.
+func generateConfigVersion(configVersion int) file.File {
+	c := executeVersion(configVersion)
+
+	return file.File{
+		Content: c,
+		Path:    configVersionFile,
+		Type:    file.TypeRegular,
+	}
+}

internal/mode/static/nginx/config/generator.go

@@ -1,6 +1,7 @@
 package config_test
 
 import (
+	"fmt"
 	"testing"
 
 	. "github.com/onsi/gomega"
@@ -65,7 +66,7 @@ func TestGenerate(t *testing.T) {
 
 	files := generator.Generate(conf)
 
-	g.Expect(files).To(HaveLen(2))
+	g.Expect(files).To(HaveLen(3))
 
 	g.Expect(files[0]).To(Equal(file.File{
 		Type:    file.TypeSecret,
@@ -82,4 +83,9 @@ func TestGenerate(t *testing.T) {
 	g.Expect(httpCfg).To(ContainSubstring("listen 443"))
 	g.Expect(httpCfg).To(ContainSubstring("upstream"))
 	g.Expect(httpCfg).To(ContainSubstring("split_clients"))
+
+	g.Expect(files[2].Type).To(Equal(file.TypeRegular))
+	g.Expect(files[2].Path).To(Equal("/etc/nginx/conf.d/config-version.conf"))
+	configVersion := string(files[2].Content)
+	g.Expect(configVersion).To(ContainSubstring(fmt.Sprintf("return 200 %d", conf.Version)))
 }

internal/mode/static/nginx/config/generator_test.go

@@ -79,9 +79,9 @@ func TestExecuteServers(t *testing.T) {
 func TestExecuteForDefaultServers(t *testing.T) {
 	testcases := []struct {
 		msg       string
-		conf      dataplane.Configuration
 		httpPorts []int
 		sslPorts  []int
+		conf      dataplane.Configuration
 	}{
 		{
 			conf: dataplane.Configuration{},

internal/mode/static/nginx/config/servers_test.go

@@ -0,0 +1,11 @@
+package config
+
+import (
+	gotemplate "text/template"
+)
+
+var versionTemplate = gotemplate.Must(gotemplate.New("version").Parse(versionTemplateText))
+
+func executeVersion(version int) []byte {
+	return execute(versionTemplate, version)
+}

internal/mode/static/nginx/config/version.go

@@ -0,0 +1,12 @@
+package config
+
+var versionTemplateText = `
+server {
+    listen unix:/var/run/nginx/nginx-config-version.sock;
+    access_log off;
+
+    location /version {
+        return 200 {{.}};
+    }
+}
+`

internal/mode/static/nginx/config/version_template.go

@@ -0,0 +1,20 @@
+package config
+
+import (
+	"strings"
+	"testing"
+
+	. "github.com/onsi/gomega"
+)
+
+func TestExecuteVersion(t *testing.T) {
+	g := NewWithT(t)
+	expSubStrings := map[string]int{
+		"return 200 42;": 1,
+	}
+
+	maps := string(executeVersion(42))
+	for expSubStr, expCount := range expSubStrings {
+		g.Expect(expCount).To(Equal(strings.Count(maps, expSubStr)))
+	}
+}