Verify after reloading

- Write config version directly in nginx config
- Check config version by querying over socket
- Add config version check to API
- Atomic write (write and rename) for version config

Author: isaac

cmd/nginx-ingress/main.go

@@ -214,6 +214,7 @@ func main() {
 		glog.Fatalf("Error generating NGINX main config: %v", err)
 	}
 	ngxc.UpdateMainConfigFile(content)
+	ngxc.UpdateConfigVersionFile()
 
 	nginxDone := make(chan error, 1)
 	ngxc.Start(nginxDone)

internal/nginx/configurator.go

@@ -1041,7 +1041,7 @@ func (cnf *Configurator) updatePlusEndpoints(ingEx *IngressEx) error {
 		name := getNameForUpstream(ingEx.Ingress, emptyHost, ingEx.Ingress.Spec.Backend)
 		endps, exists := ingEx.Endpoints[ingEx.Ingress.Spec.Backend.ServiceName+ingEx.Ingress.Spec.Backend.ServicePort.String()]
 		if exists {
-			err := cnf.nginxAPI.UpdateServers(name, endps, cfg)
+			err := cnf.nginxAPI.UpdateServers(name, endps, cfg, cnf.nginx.configVersion)
 			if err != nil {
 				return fmt.Errorf("Couldn't update the endpoints for %v: %v", name, err)
 			}
@@ -1055,7 +1055,7 @@ func (cnf *Configurator) updatePlusEndpoints(ingEx *IngressEx) error {
 			name := getNameForUpstream(ingEx.Ingress, rule.Host, &path.Backend)
 			endps, exists := ingEx.Endpoints[path.Backend.ServiceName+path.Backend.ServicePort.String()]
 			if exists {
-				err := cnf.nginxAPI.UpdateServers(name, endps, cfg)
+				err := cnf.nginxAPI.UpdateServers(name, endps, cfg, cnf.nginx.configVersion)
 				if err != nil {
 					return fmt.Errorf("Couldn't update the endpoints for %v: %v", name, err)
 				}
@@ -1117,14 +1117,14 @@ func GenerateNginxMainConfig(config *Config) *MainConfig {
 		SSLCiphers:                config.MainServerSSLCiphers,
 		SSLDHParam:                config.MainServerSSLDHParam,
 		SSLPreferServerCiphers:    config.MainServerSSLPreferServerCiphers,
-		HTTP2:                 config.HTTP2,
-		ServerTokens:          config.ServerTokens,
-		ProxyProtocol:         config.ProxyProtocol,
-		WorkerProcesses:       config.MainWorkerProcesses,
-		WorkerCPUAffinity:     config.MainWorkerCPUAffinity,
-		WorkerShutdownTimeout: config.MainWorkerShutdownTimeout,
-		WorkerConnections:     config.MainWorkerConnections,
-		WorkerRlimitNofile:    config.MainWorkerRlimitNofile,
+		HTTP2:                     config.HTTP2,
+		ServerTokens:              config.ServerTokens,
+		ProxyProtocol:             config.ProxyProtocol,
+		WorkerProcesses:           config.MainWorkerProcesses,
+		WorkerCPUAffinity:         config.MainWorkerCPUAffinity,
+		WorkerShutdownTimeout:     config.MainWorkerShutdownTimeout,
+		WorkerConnections:         config.MainWorkerConnections,
+		WorkerRlimitNofile:        config.MainWorkerRlimitNofile,
 	}
 	return nginxCfg
 }

internal/nginx/nginx.go

@@ -9,6 +9,7 @@ import (
 	"path"
 
 	"github.com/golang/glog"
+	"github.com/nginxinc/kubernetes-ingress/internal/nginx/verify"
 )
 
 const dhparamFilename = "dhparam.pem"
@@ -19,9 +20,12 @@ const jwkSecretFileMode = 0644
 
 // Controller updates NGINX configuration, starts and reloads NGINX
 type Controller struct {
-	nginxConfdPath   string
-	nginxSecretsPath string
-	local            bool
+	nginxConfdPath        string
+	nginxSecretsPath      string
+	local                 bool
+	verifyConfigGenerator *verify.ConfigGenerator
+	verifyClient          *verify.Client
+	configVersion         int
 }
 
 // IngressNginxConfig describes an NGINX configuration
@@ -187,10 +191,18 @@ func NewUpstreamWithDefaultServer(name string) Upstream {
 
 // NewNginxController creates a NGINX controller
 func NewNginxController(nginxConfPath string, local bool) *Controller {
+	verifyConfigGenerator, err := verify.NewConfigGenerator()
+	if err != nil {
+		glog.Fatalf("error instantiating a verify.ConfigGenerator: %v", err)
+	}
+
 	ngxc := Controller{
-		nginxConfdPath:   path.Join(nginxConfPath, "conf.d"),
-		nginxSecretsPath: path.Join(nginxConfPath, "secrets"),
-		local:            local,
+		nginxConfdPath:        path.Join(nginxConfPath, "conf.d"),
+		nginxSecretsPath:      path.Join(nginxConfPath, "secrets"),
+		local:                 local,
+		verifyConfigGenerator: verifyConfigGenerator,
+		configVersion:         0,
+		verifyClient:          verify.NewClient(),
 	}
 
 	return &ngxc
@@ -271,7 +283,6 @@ func (nginx *Controller) DeleteSecretFile(name string) {
 			glog.Warningf("Failed to delete %v: %v", filename, err)
 		}
 	}
-
 }
 
 func (nginx *Controller) getIngressNginxConfigFileName(name string) string {
@@ -284,16 +295,23 @@ func (nginx *Controller) getSecretFileName(name string) string {
 
 // Reload reloads NGINX
 func (nginx *Controller) Reload() error {
-	if !nginx.local {
-		if err := shellOut("nginx -t"); err != nil {
-			return fmt.Errorf("Invalid nginx configuration detected, not reloading: %s", err)
-		}
-		if err := shellOut("nginx -s reload"); err != nil {
-			return fmt.Errorf("Reloading NGINX failed: %s", err)
-		}
-	} else {
-		glog.V(3).Info("Reloading nginx")
+	if nginx.local {
+		glog.V(3).Info("local - skipping nginx reload")
+		return nil
+	}
+	// write a new config version
+	nginx.configVersion++
+	nginx.UpdateConfigVersionFile()
+
+	glog.V(3).Infof("Reloading nginx. configVersion: %v", nginx.configVersion)
+	if err := shellOut("nginx -s reload"); err != nil {
+		return fmt.Errorf("nginx reload failed: %v", err)
+	}
+	err := nginx.verifyClient.WaitForCorrectVersion(nginx.configVersion)
+	if err != nil {
+		return fmt.Errorf("could not get newest config version: %v", err)
 	}
+
 	return nil
 }
 
@@ -399,3 +417,37 @@ func (nginx *Controller) UpdateIngressConfigFile(name string, cfg []byte) {
 	}
 	glog.V(3).Infof("The Ingress config file has been updated")
 }
+
+// UpdateConfigVersionFile writes the config version file.
+func (nginx *Controller) UpdateConfigVersionFile() {
+	cfg, err := nginx.verifyConfigGenerator.GenerateVersionConfig(nginx.configVersion)
+	if err != nil {
+		glog.Fatalf("Error generating config version content: %v", err)
+	}
+
+	filename := "/etc/nginx/config-version.conf"
+	tempname := "/etc/nginx/config-version.conf.tmp"
+	glog.V(3).Infof("Writing config version to %v", filename)
+
+	if bool(glog.V(3)) || nginx.local {
+		glog.Info(string(cfg))
+	}
+
+	if !nginx.local {
+		w, err := os.Create(tempname)
+		if err != nil {
+			glog.Fatalf("Failed to open %v: %v", filename, err)
+		}
+		_, err = w.Write(cfg)
+		if err != nil {
+			glog.Fatalf("Failed to write to %v: %v", filename, err)
+		}
+		w.Close()
+
+		err = os.Rename(tempname, filename)
+		if err != nil {
+			glog.Fatalf("failed to rename version config file: %v", err)
+		}
+	}
+	glog.V(3).Infof("The config version file has been updated.")
+}

internal/nginx/plus/nginx_api.go

@@ -1,6 +1,7 @@
 package plus
 
 import (
+	"fmt"
 	"net/http"
 
 	"github.com/golang/glog"
@@ -29,13 +30,39 @@ func NewNginxAPIController(httpClient *http.Client, endpoint string, local bool)
 	return nginx, nil
 }
 
+// verifyConfigVersion is used to check if the worker process that the API client is connected
+// to is using the latest version of nginx config. This way we avoid making changes on
+// a worker processes that is being shut down.
+func verifyConfigVersion(httpClient *http.Client, configVersion int) error {
+	req, err := http.NewRequest("GET", "http://nginx-plus-api/configVersionCheck", nil)
+	if err != nil {
+		return fmt.Errorf("error creating request: %v", err)
+	}
+	req.Header.Set("x-expected-config-version", fmt.Sprintf("%v", configVersion))
+	resp, err := httpClient.Do(req)
+	if err != nil {
+		return fmt.Errorf("error doing request: %v", err)
+	}
+	defer resp.Body.Close()
+	if resp.StatusCode != 200 {
+		return fmt.Errorf("API returned non-success status: %v", resp.StatusCode)
+	}
+	return nil
+}
+
 // UpdateServers updates upstream servers
-func (nginx *NginxAPIController) UpdateServers(upstream string, servers []string, config ServerConfig) error {
+func (nginx *NginxAPIController) UpdateServers(upstream string, servers []string, config ServerConfig, configVersion int) error {
 	if nginx.local {
 		glog.V(3).Infof("Updating endpoints of %v: %v\n", upstream, servers)
 		return nil
 	}
 
+	err := verifyConfigVersion(nginx.client.httpClient, configVersion)
+	if err != nil {
+		return fmt.Errorf("error verifying config version: %v", err)
+	}
+	glog.V(3).Infof("API has the correct config version: %v.", configVersion)
+
 	var upsServers []UpstreamServer
 	for _, s := range servers {
 		upsServers = append(upsServers, UpstreamServer{
@@ -49,7 +76,7 @@ func (nginx *NginxAPIController) UpdateServers(upstream string, servers []string
 	added, removed, err := nginx.client.UpdateHTTPServers(upstream, upsServers)
 	if err != nil {
 		glog.V(3).Infof("Couldn't update servers of %v upstream: %v", upstream, err)
-		return err
+		return fmt.Errorf("error updating servers of %v upstream: %v", upstream, err)
 	}
 
 	glog.V(3).Infof("Updated servers of %v; Added: %v, Removed: %v", upstream, added, removed)

internal/nginx/templates/nginx-plus.tmpl

@@ -111,11 +111,20 @@ http {
         listen unix:/var/run/nginx-plus-api.sock;
         access_log off;
 
+        # $config_version_mismatch is defined in /etc/nginx/config-version.conf
+        location /configVersionCheck {
+            if ($config_version_mismatch) {
+                return 503;
+            }
+            return 200;
+        }
+
         location /api {
             api write=on;
         }
     }
 
+    include /etc/nginx/config-version.conf;
     include /etc/nginx/conf.d/*.conf;
 }
 

internal/nginx/templates/nginx.tmpl

@@ -96,7 +96,7 @@ http {
     }
     {{- end }}
 
-
+    include /etc/nginx/config-version.conf;
     include /etc/nginx/conf.d/*.conf;
 }
 

internal/nginx/verify/client.go

@@ -0,0 +1,75 @@
+package verify
+
+import (
+	"context"
+	"fmt"
+	"io/ioutil"
+	"net"
+	"net/http"
+	"strconv"
+	"time"
+
+	"github.com/golang/glog"
+)
+
+// Client is a client for verifying the config version.
+type Client struct {
+	client *http.Client
+}
+
+// NewClient returns a new client pointed at the config version socket.
+func NewClient() *Client {
+	return &Client{
+		client: &http.Client{
+			Transport: &http.Transport{
+				DialContext: func(_ context.Context, _, _ string) (net.Conn, error) {
+					return net.Dial("unix", "/var/run/nginx-config-version.sock")
+				},
+			},
+		},
+	}
+}
+
+// GetConfigVersion get version number that we put in the nginx config to verify that we're using
+// the correct config.
+func (c *Client) GetConfigVersion() (int, error) {
+	resp, err := c.client.Get("http://config-version/configVersion")
+	if err != nil {
+		return 0, fmt.Errorf("error getting client: %v", err)
+	}
+	defer resp.Body.Close()
+
+	if resp.StatusCode != http.StatusOK {
+		return 0, fmt.Errorf("non-200 response: %v", resp.StatusCode)
+	}
+
+	body, err := ioutil.ReadAll(resp.Body)
+	if err != nil {
+		return 0, fmt.Errorf("failed to read the response body: %v", err)
+	}
+	v, err := strconv.Atoi(string(body))
+	if err != nil {
+		return 0, fmt.Errorf("error converting string to int: %v", err)
+	}
+	return v, nil
+}
+
+// WaitForCorrectVersion calls the config version endpoint until it gets the expectedVersion,
+// which ensures that a new worker process has been started for that config version.
+func (c *Client) WaitForCorrectVersion(expectedVersion int) error {
+	// This value needs tuning.
+	maxRetries := 160
+	sleep := 25 * time.Millisecond
+	for i := 0; i < maxRetries; i++ {
+		version, err := c.GetConfigVersion()
+		if err != nil {
+			return fmt.Errorf("unable to fetch version: %v", err)
+		}
+		if version == expectedVersion {
+			glog.V(3).Infof("success, version %v ensured. iterations: %v. took: %v", expectedVersion, i, time.Duration(i)*sleep)
+			return nil
+		}
+		time.Sleep(sleep)
+	}
+	return fmt.Errorf("could not get expected version: %v", expectedVersion)
+}

internal/nginx/verify/client_test.go

@@ -0,0 +1,51 @@
+package verify
+
+import (
+	"bytes"
+	"io/ioutil"
+	"net/http"
+	"testing"
+)
+
+type Transport struct {
+}
+
+func (c Transport) RoundTrip(req *http.Request) (*http.Response, error) {
+	return &http.Response{
+		StatusCode: 200,
+		Body:       ioutil.NopCloser(bytes.NewBufferString("42")),
+		Header:     make(http.Header),
+	}, nil
+}
+
+func getTestHTTPClient() *http.Client {
+	ts := Transport{}
+	tClient := &http.Client{
+		Transport: ts,
+	}
+	return tClient
+}
+
+func TestVerifyClient(t *testing.T) {
+
+	c := Client{
+		client: getTestHTTPClient(),
+	}
+
+	configVersion, err := c.GetConfigVersion()
+	if err != nil {
+		t.Errorf("error getting config version: %v", err)
+	}
+	if configVersion != 42 {
+		t.Errorf("got bad config version, expected 42 got %v", configVersion)
+	}
+
+	err = c.WaitForCorrectVersion(43)
+	if err == nil {
+		t.Error("expected error from WaitForCorrectVersion ")
+	}
+	err = c.WaitForCorrectVersion(42)
+	if err != nil {
+		t.Errorf("error waiting for config version: %v", err)
+	}
+}