From 0886c7e58a3975d4d9b6f32f5934a1299d5ce447 Mon Sep 17 00:00:00 2001
From: Manuel de Brito Fontes <aledbf@gmail.com>
Date: Thu, 28 Jul 2016 17:35:36 -0400
Subject: [PATCH] Check for errors in nginx template

---
 controllers/nginx/nginx/ipwhitelist/main.go   |  6 +--
 .../nginx/nginx/ipwhitelist/main_test.go      | 49 +++++++++++++++++++
 controllers/nginx/nginx/main.go               |  4 +-
 controllers/nginx/nginx/template.go           | 12 +++--
 controllers/nginx/utils.go                    |  2 +-
 5 files changed, 64 insertions(+), 9 deletions(-)

diff --git a/controllers/nginx/nginx/ipwhitelist/main.go b/controllers/nginx/nginx/ipwhitelist/main.go
index d533d9c083..75bca5e5f0 100644
--- a/controllers/nginx/nginx/ipwhitelist/main.go
+++ b/controllers/nginx/nginx/ipwhitelist/main.go
@@ -46,18 +46,18 @@ type SourceRange struct {
 type ingAnnotations map[string]string
 
 func (a ingAnnotations) whitelist() ([]string, error) {
+	cidrs := make([]string, 0)
 	val, ok := a[whitelist]
 	if !ok {
-		return nil, ErrMissingWhitelist
+		return cidrs, ErrMissingWhitelist
 	}
 
 	values := strings.Split(val, ",")
 	ipnets, err := sets.ParseIPNets(values...)
 	if err != nil {
-		return nil, ErrInvalidCIDR
+		return cidrs, ErrInvalidCIDR
 	}
 
-	cidrs := make([]string, 0)
 	for k := range ipnets {
 		cidrs = append(cidrs, k)
 	}
diff --git a/controllers/nginx/nginx/ipwhitelist/main_test.go b/controllers/nginx/nginx/ipwhitelist/main_test.go
index 9a13c9da7c..ea657ee9c7 100644
--- a/controllers/nginx/nginx/ipwhitelist/main_test.go
+++ b/controllers/nginx/nginx/ipwhitelist/main_test.go
@@ -96,3 +96,52 @@ func TestAnnotations(t *testing.T) {
 		t.Errorf("Expected 2 netwotks but %v was returned", len(wl))
 	}
 }
+
+func TestParseAnnotations(t *testing.T) {
+	ing := buildIngress()
+
+	_, err := ingAnnotations(ing.GetAnnotations()).whitelist()
+	if err == nil {
+		t.Error("Expected a validation error")
+	}
+
+	testNet := "10.0.0.0/24"
+	enet := []string{testNet}
+
+	data := map[string]string{}
+	data[whitelist] = testNet
+	ing.SetAnnotations(data)
+
+	expected := &SourceRange{
+		CIDR: enet,
+	}
+
+	sr, err := ParseAnnotations([]string{}, ing)
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
+	}
+
+	if !reflect.DeepEqual(sr, expected) {
+		t.Errorf("Expected %v but returned %s", sr, expected)
+	}
+
+	data[whitelist] = "www"
+	ing.SetAnnotations(data)
+	_, err = ParseAnnotations([]string{}, ing)
+	if err == nil {
+		t.Errorf("Expected error parsing an invalid cidr")
+	}
+
+	delete(data, "whitelist")
+	ing.SetAnnotations(data)
+	sr, _ = ParseAnnotations([]string{}, ing)
+	if !reflect.DeepEqual(sr.CIDR, []string{}) {
+		t.Errorf("Expected empty CIDR but %v returned", sr.CIDR)
+	}
+
+	sr, _ = ParseAnnotations([]string{}, &extensions.Ingress{})
+	if !reflect.DeepEqual(sr.CIDR, []string{}) {
+		t.Errorf("Expected empty CIDR but %v returned", sr.CIDR)
+	}
+
+}
diff --git a/controllers/nginx/nginx/main.go b/controllers/nginx/nginx/main.go
index f7fc27dff2..f60ad2dad6 100644
--- a/controllers/nginx/nginx/main.go
+++ b/controllers/nginx/nginx/main.go
@@ -67,7 +67,9 @@ func NewManager(kubeClient *client.Client) *Manager {
 
 	ngx.sslDHParam = ngx.SearchDHParamFile(config.SSLDirectory)
 
-	ngx.loadTemplate()
+	if err := ngx.loadTemplate(); err != nil {
+		glog.Fatalf("invalid NGINX template: %v", err)
+	}
 
 	return ngx
 }
diff --git a/controllers/nginx/nginx/template.go b/controllers/nginx/nginx/template.go
index 1b131c7fbb..f5cd24f5c2 100644
--- a/controllers/nginx/nginx/template.go
+++ b/controllers/nginx/nginx/template.go
@@ -54,9 +54,13 @@ var (
 	}
 )
 
-func (ngx *Manager) loadTemplate() {
-	tmpl, _ := template.New("nginx.tmpl").Funcs(funcMap).ParseFiles(tmplPath)
+func (ngx *Manager) loadTemplate() error {
+	tmpl, err := template.New("nginx.tmpl").Funcs(funcMap).ParseFiles(tmplPath)
+	if err != nil {
+		return err
+	}
 	ngx.template = tmpl
+	return nil
 }
 
 func (ngx *Manager) writeCfg(cfg config.Configuration, ingressCfg IngressConfig) (bool, error) {
@@ -74,7 +78,7 @@ func (ngx *Manager) writeCfg(cfg config.Configuration, ingressCfg IngressConfig)
 	if glog.V(3) {
 		b, err := json.Marshal(conf)
 		if err != nil {
-			fmt.Println("error:", err)
+			glog.Errorf("unexpected error:", err)
 		}
 		glog.Infof("NGINX configuration: %v", string(b))
 	}
@@ -82,7 +86,7 @@ func (ngx *Manager) writeCfg(cfg config.Configuration, ingressCfg IngressConfig)
 	buffer := new(bytes.Buffer)
 	err := ngx.template.Execute(buffer, conf)
 	if err != nil {
-		glog.Infof("NGINX error: %v", err)
+		glog.V(3).Infof("%v", string(buffer.Bytes()))
 		return false, err
 	}
 
diff --git a/controllers/nginx/utils.go b/controllers/nginx/utils.go
index bf696e7212..11ac82c2aa 100644
--- a/controllers/nginx/utils.go
+++ b/controllers/nginx/utils.go
@@ -88,7 +88,7 @@ func (t *taskQueue) worker() {
 		}
 		glog.V(3).Infof("syncing %v", key)
 		if err := t.sync(key.(string)); err != nil {
-			glog.V(3).Infof("requeuing %v, err %v", key, err)
+			glog.Warningf("requeuing %v, err %v", key, err)
 			t.requeue(key.(string))
 		} else {
 			t.queue.Forget(key)