From fd6b4dddd47de78fe36b09efbbde8fde2aa02111 Mon Sep 17 00:00:00 2001
From: Manuel de Brito Fontes <aledbf@gmail.com>
Date: Thu, 22 Sep 2016 14:08:35 -0300
Subject: [PATCH] Avoid replacing nginx.conf file if the new configuration is
 invalid

---
 ingress/controllers/nginx/nginx/command.go    | 19 +++++++++++++-
 .../nginx/nginx/template/template.go          | 26 ++++++++++++++-----
 2 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/ingress/controllers/nginx/nginx/command.go b/ingress/controllers/nginx/nginx/command.go
index 2ac830934a..8142019d3c 100644
--- a/ingress/controllers/nginx/nginx/command.go
+++ b/ingress/controllers/nginx/nginx/command.go
@@ -18,6 +18,7 @@ package nginx
 
 import (
 	"fmt"
+	"io/ioutil"
 	"net/http"
 	"os"
 	"os/exec"
@@ -63,7 +64,7 @@ func (ngx *Manager) CheckAndReload(cfg config.Configuration, ingressCfg ingress.
 	ngx.reloadLock.Lock()
 	defer ngx.reloadLock.Unlock()
 
-	newCfg, err := ngx.template.Write(cfg, ingressCfg)
+	newCfg, err := ngx.template.Write(cfg, ingressCfg, ngx.testTemplate)
 	if err != nil {
 		return fmt.Errorf("failed to write new nginx configuration. Avoiding reload: %v", err)
 	}
@@ -118,3 +119,19 @@ func (ngx Manager) Check(_ *http.Request) error {
 
 	return nil
 }
+
+// testTemplate checks if the NGINX configuration inside the byte array is valid
+// running the command "nginx -t" using a temporal file.
+func (ngx Manager) testTemplate(cfg []byte) error {
+	tmpfile, err := ioutil.TempFile("", "nginx-cfg")
+	if err != nil {
+		return err
+	}
+	defer tmpfile.Close()
+	defer os.Remove(tmpfile.Name())
+	ioutil.WriteFile(tmpfile.Name(), cfg, 0644)
+	if err := ngx.shellOut(fmt.Sprintf("nginx -t -c %v", tmpfile.Name())); err != nil {
+		return fmt.Errorf("invalid nginx configuration: %v", err)
+	}
+	return nil
+}
diff --git a/ingress/controllers/nginx/nginx/template/template.go b/ingress/controllers/nginx/nginx/template/template.go
index a8380d0faf..d4f22a371f 100644
--- a/ingress/controllers/nginx/nginx/template/template.go
+++ b/ingress/controllers/nginx/nginx/template/template.go
@@ -91,7 +91,10 @@ func (t *Template) Close() {
 
 // Write populates a buffer using a template with NGINX configuration
 // and the servers and upstreams created by Ingress rules
-func (t *Template) Write(cfg config.Configuration, ingressCfg ingress.Configuration) ([]byte, error) {
+func (t *Template) Write(
+	cfg config.Configuration,
+	ingressCfg ingress.Configuration,
+	isValidTemplate func([]byte) error) ([]byte, error) {
 	var longestName int
 	var serverNames int
 	for _, srv := range ingressCfg.Servers {
@@ -109,12 +112,14 @@ func (t *Template) Write(cfg config.Configuration, ingressCfg ingress.Configurat
 	// https://trac.nginx.org/nginx/ticket/631
 	nameHashBucketSize := nextPowerOf2(longestName)
 	if nameHashBucketSize > cfg.ServerNameHashBucketSize {
-		glog.V(3).Infof("adjusting ServerNameHashBucketSize variable from %v to %v", cfg.ServerNameHashBucketSize, nameHashBucketSize)
+		glog.V(3).Infof("adjusting ServerNameHashBucketSize variable from %v to %v",
+			cfg.ServerNameHashBucketSize, nameHashBucketSize)
 		cfg.ServerNameHashBucketSize = nameHashBucketSize
 	}
 	serverNameHashMaxSize := nextPowerOf2(serverNames)
 	if serverNameHashMaxSize > cfg.ServerNameHashMaxSize {
-		glog.V(3).Infof("adjusting ServerNameHashMaxSize variable from %v to %v", cfg.ServerNameHashMaxSize, serverNameHashMaxSize)
+		glog.V(3).Infof("adjusting ServerNameHashMaxSize variable from %v to %v",
+			cfg.ServerNameHashMaxSize, serverNameHashMaxSize)
 		cfg.ServerNameHashMaxSize = serverNameHashMaxSize
 	}
 
@@ -141,9 +146,15 @@ func (t *Template) Write(cfg config.Configuration, ingressCfg ingress.Configurat
 	err := t.tmpl.Execute(buffer, conf)
 	if err != nil {
 		glog.V(3).Infof("%v", string(buffer.Bytes()))
+		return nil, err
+	}
+
+	err = isValidTemplate(buffer.Bytes())
+	if err != nil {
+		return nil, err
 	}
 
-	return buffer.Bytes(), err
+	return buffer.Bytes(), nil
 }
 
 func fixKeyNames(data map[string]interface{}) map[string]interface{} {
@@ -258,13 +269,16 @@ func buildRateLimitZones(input interface{}) []string {
 
 			if loc.RateLimit.Connections.Limit > 0 {
 				zone := fmt.Sprintf("limit_conn_zone $binary_remote_addr zone=%v:%vm;",
-					loc.RateLimit.Connections.Name, loc.RateLimit.Connections.SharedSize)
+					loc.RateLimit.Connections.Name,
+					loc.RateLimit.Connections.SharedSize)
 				zones = append(zones, zone)
 			}
 
 			if loc.RateLimit.RPS.Limit > 0 {
 				zone := fmt.Sprintf("limit_conn_zone $binary_remote_addr zone=%v:%vm rate=%vr/s;",
-					loc.RateLimit.Connections.Name, loc.RateLimit.Connections.SharedSize, loc.RateLimit.Connections.Limit)
+					loc.RateLimit.Connections.Name,
+					loc.RateLimit.Connections.SharedSize,
+					loc.RateLimit.Connections.Limit)
 				zones = append(zones, zone)
 			}
 		}