Merge pull request #1450 from aledbf/improve-template-errors

[nginx-ingress-controller]: Check for errors in nginx template

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

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

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
 }

controllers/nginx/nginx/template.go

@@ -54,9 +54,13 @@ var (
 	}
 )
 
-func (ngx *Manager) loadTemplate() {
+func (ngx *Manager) loadTemplate() error {
-	tmpl, _ := template.New("nginx.tmpl").Funcs(funcMap).ParseFiles(tmplPath)
+	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
 	}
 

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)