Repository: kubernetes/ingress-nginx
Pull request #4500 (2019-08-28 17:50:55)

Fix nginx variable service_port (nginx)
<!--  Thanks for sending a pull request!  Here are some tips for you:
1. If this is your first time, read our contributor guidelines https://git.k8s.io/community/contributors/guide/pull-requests.md#the-pull-request-submit-process and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. If you want *faster* PR reviews, read how: https://git.k8s.io/community/contributors/guide/pull-requests.md#best-practices-for-faster-reviews
-->

**What this PR does / why we need it**:

Right now the generates nginx.conf contains `set $service_port "{ 0 0}";`

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #

**Special notes for your reviewer**:



Patch:
diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go
index 4e7fd1060..f640ba54b 100644
--- a/internal/ingress/controller/nginx.go
+++ b/internal/ingress/controller/nginx.go
@@ -414,7 +414,7 @@ func (n *NGINXController) Stop() error {
 	// wait for the NGINX process to terminate
 	timer := time.NewTicker(time.Second * 1)
 	for range timer.C {
-		if !process.IsNginxRunning() {
+		if !nginx.IsRunning() {
 			klog.Info("NGINX process has stopped")
 			timer.Stop()
 			break
diff --git a/internal/ingress/controller/process/nginx.go b/internal/ingress/controller/process/nginx.go
index 2f31dbc80..a0aa44d90 100644
--- a/internal/ingress/controller/process/nginx.go
+++ b/internal/ingress/controller/process/nginx.go
@@ -24,7 +24,6 @@ import (
 	"syscall"
 	"time"
 
-	ps "github.com/mitchellh/go-ps"
 	"github.com/ncabatoff/process-exporter/proc"
 	"k8s.io/klog"
 )
@@ -82,14 +81,3 @@ func WaitUntilPortIsAvailable(port int) {
 		time.Sleep(100 * time.Millisecond)
 	}
 }
-
-// IsNginxRunning returns true if a process with the name 'nginx' is found
-func IsNginxRunning() bool {
-	processes, _ := ps.Processes()
-	for _, p := range processes {
-		if p.Executable() == "nginx" {
-			return true
-		}
-	}
-	return false
-}
diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go
index 25277537d..7d8593de0 100644
--- a/internal/ingress/controller/template/template.go
+++ b/internal/ingress/controller/template/template.go
@@ -797,6 +797,7 @@ type ingressInformation struct {
 	Namespace   string
 	Rule        string
 	Service     string
+	ServicePort string
 	Annotations map[string]string
 }
 
@@ -810,6 +811,9 @@ func (info *ingressInformation) Equal(other *ingressInformation) bool {
 	if info.Service != other.Service {
 		return false
 	}
+	if info.ServicePort != other.ServicePort {
+		return false
+	}
 	if !reflect.DeepEqual(info.Annotations, other.Annotations) {
 		return false
 	}
@@ -848,6 +852,9 @@ func getIngressInformation(i, h, p interface{}) *ingressInformation {
 
 	if ing.Spec.Backend != nil {
 		info.Service = ing.Spec.Backend.ServiceName
+		if ing.Spec.Backend.ServicePort.String() != "0" {
+			info.ServicePort = ing.Spec.Backend.ServicePort.String()
+		}
 	}
 
 	for _, rule := range ing.Spec.Rules {
@@ -862,6 +869,10 @@ func getIngressInformation(i, h, p interface{}) *ingressInformation {
 		for _, rPath := range rule.HTTP.Paths {
 			if path == rPath.Path {
 				info.Service = rPath.Backend.ServiceName
+				if rPath.Backend.ServicePort.String() != "0" {
+					info.ServicePort = rPath.Backend.ServicePort.String()
+				}
+
 				return info
 			}
 		}
diff --git a/internal/ingress/types_equals.go b/internal/ingress/types_equals.go
index 758590309..d13dbe080 100644
--- a/internal/ingress/types_equals.go
+++ b/internal/ingress/types_equals.go
@@ -347,7 +347,7 @@ func (l1 *Location) Equal(l2 *Location) bool {
 		}
 	}
 
-	if l1.Port.StrVal != l2.Port.StrVal {
+	if l1.Port.String() != l2.Port.String() {
 		return false
 	}
 	if !(&l1.BasicDigestAuth).Equal(&l2.BasicDigestAuth) {
diff --git a/internal/nginx/main.go b/internal/nginx/main.go
index 2e596d101..5a86ea7cf 100644
--- a/internal/nginx/main.go
+++ b/internal/nginx/main.go
@@ -27,6 +27,7 @@ import (
 	"strings"
 	"time"
 
+	ps "github.com/mitchellh/go-ps"
 	"github.com/tv42/httpunix"
 	"k8s.io/klog"
 )
@@ -171,3 +172,15 @@ func Version() string {
 
 	return string(out)
 }
+
+// IsRunning returns true if a process with the name 'nginx' is found
+func IsRunning() bool {
+	processes, _ := ps.Processes()
+	for _, p := range processes {
+		if p.Executable() == "nginx" {
+			return true
+		}
+	}
+
+	return false
+}
diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl
index dd8631325..9b449a7e4 100755
--- a/rootfs/etc/nginx/template/nginx.tmpl
+++ b/rootfs/etc/nginx/template/nginx.tmpl
@@ -955,7 +955,7 @@ stream {
             set $namespace      {{ $ing.Namespace | quote}};
             set $ingress_name   {{ $ing.Rule | quote }};
             set $service_name   {{ $ing.Service | quote }};
-            set $service_port   {{ $location.Port | quote }};
+            set $service_port   {{ $ing.ServicePort | quote }};
             set $location_path  {{ $location.Path | escapeLiteralDollar | quote }};
 
             {{ if $all.Cfg.EnableOpentracing }}


Test patch:
diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go
index 00c909054..aa173facc 100644
--- a/internal/ingress/controller/template/template_test.go
+++ b/internal/ingress/controller/template/template_test.go
@@ -29,7 +29,11 @@ import (
 	"testing"
 
 	jsoniter "github.com/json-iterator/go"
+
+	apiv1 "k8s.io/api/core/v1"
 	networking "k8s.io/api/networking/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
 
 	"k8s.io/ingress-nginx/internal/ingress"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/authreq"
@@ -899,104 +903,108 @@ func TestOpentracingPropagateContext(t *testing.T) {
 }
 
 func TestGetIngressInformation(t *testing.T) {
-	validIngress := &ingress.Ingress{}
-	invalidIngress := "wrongtype"
-	host := "host1"
-	validPath := "/ok"
-	invalidPath := 10
-
-	info := getIngressInformation(invalidIngress, host, validPath)
-	expected := &ingressInformation{}
-	if !info.Equal(expected) {
-		t.Errorf("Expected %v, but got %v", expected, info)
-	}
-
-	info = getIngressInformation(validIngress, host, invalidPath)
-	if !info.Equal(expected) {
-		t.Errorf("Expected %v, but got %v", expected, info)
-	}
 
-	// Setup Ingress Resource
-	validIngress.Namespace = "default"
-	validIngress.Name = "validIng"
-	validIngress.Annotations = map[string]string{
-		"ingress.annotation": "ok",
-	}
-	validIngress.Spec.Backend = &networking.IngressBackend{
-		ServiceName: "a-svc",
-	}
-
-	info = getIngressInformation(validIngress, host, validPath)
-	expected = &ingressInformation{
-		Namespace: "default",
-		Rule:      "validIng",
-		Annotations: map[string]string{
-			"ingress.annotation": "ok",
+	testcases := map[string]struct {
+		Ingress  interface{}
+		Host     string
+		Path     interface{}
+		Expected *ingressInformation
+	}{
+		"wrong ingress type": {
+			"wrongtype",
+			"host1",
+			"/ok",
+			&ingressInformation{},
 		},
-		Service: "a-svc",
-	}
-	if !info.Equal(expected) {
-		t.Errorf("Expected %v, but got %v", expected, info)
-	}
-
-	validIngress.Spec.Backend = nil
-	validIngress.Spec.Rules = []networking.IngressRule{
-		{
-			Host: host,
-			IngressRuleValue: networking.IngressRuleValue{
-				HTTP: &networking.HTTPIngressRuleValue{
-					Paths: []networking.HTTPIngressPath{
-						{
-							Path: "/ok",
-							Backend: networking.IngressBackend{
-								ServiceName: "b-svc",
-							},
+		"wrong path type": {
+			&ingress.Ingress{},
+			"host1",
+			10,
+			&ingressInformation{},
+		},
+		"valid ingress definition with name validIng in namespace default": {
+			&ingress.Ingress{
+				networking.Ingress{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "validIng",
+						Namespace: apiv1.NamespaceDefault,
+						Annotations: map[string]string{
+							"ingress.annotation": "ok",
+						},
+					},
+					Spec: networking.IngressSpec{
+						Backend: &networking.IngressBackend{
+							ServiceName: "a-svc",
 						},
 					},
 				},
+				nil,
+			},
+			"host1",
+			"",
+			&ingressInformation{
+				Namespace: "default",
+				Rule:      "validIng",
+				Annotations: map[string]string{
+					"ingress.annotation": "ok",
+				},
+				Service: "a-svc",
 			},
 		},
-		{},
-	}
-
-	info = getIngressInformation(validIngress, host, validPath)
-	expected = &ingressInformation{
-		Namespace: "default",
-		Rule:      "validIng",
-		Annotations: map[string]string{
-			"ingress.annotation": "ok",
-		},
-		Service: "b-svc",
-	}
-	if !info.Equal(expected) {
-		t.Errorf("Expected %v, but got %v", expected, info)
-	}
-
-	validIngress.Spec.Rules = append(validIngress.Spec.Rules, networking.IngressRule{
-		Host: "host2",
-		IngressRuleValue: networking.IngressRuleValue{
-			HTTP: &networking.HTTPIngressRuleValue{
-				Paths: []networking.HTTPIngressPath{
-					{
-						Path: "/ok",
-						Backend: networking.IngressBackend{
-							ServiceName: "c-svc",
+		"valid ingress definition with name demo in namespace something and path /ok using a service with name b-svc port 80": {
+			&ingress.Ingress{
+				networking.Ingress{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "demo",
+						Namespace: "something",
+						Annotations: map[string]string{
+							"ingress.annotation": "ok",
 						},
 					},
+					Spec: networking.IngressSpec{
+						Rules: []networking.IngressRule{
+							{
+								Host: "foo.bar",
+								IngressRuleValue: networking.IngressRuleValue{
+									HTTP: &networking.HTTPIngressRuleValue{
+										Paths: []networking.HTTPIngressPath{
+											{
+												Path: "/ok",
+												Backend: networking.IngressBackend{
+													ServiceName: "b-svc",
+													ServicePort: intstr.FromInt(80),
+												},
+											},
+										},
+									},
+								},
+							},
+							{},
+						},
+					},
+				},
+				nil,
+			},
+			"foo.bar",
+			"/ok",
+			&ingressInformation{
+				Namespace: "something",
+				Rule:      "demo",
+				Annotations: map[string]string{
+					"ingress.annotation": "ok",
 				},
+				Service:     "b-svc",
+				ServicePort: "80",
 			},
 		},
-	})
-
-	info = getIngressInformation(validIngress, host, validPath)
-	if !info.Equal(expected) {
-		t.Errorf("Expected %v, but got %v", expected, info)
 	}
 
-	info = getIngressInformation(validIngress, "host2", validPath)
-	expected.Service = "c-svc"
-	if !info.Equal(expected) {
-		t.Errorf("Expected %v, but got %v", expected, info)
+	for title, testCase := range testcases {
+		info := getIngressInformation(testCase.Ingress, testCase.Host, testCase.Path)
+
+		if !info.Equal(testCase.Expected) {
+			t.Fatalf("%s: expected '%v' but returned %v", title, testCase.Expected, info)
+		}
 	}
 }