Repository: kubernetes/ingress-nginx
Pull request #7469 (2021-08-09 14:54:04)

Allow the usage of Services as Upstream on a global level
It is possible to change this behavior on an ingress level, which works
well when you only have a few of them. When running several dozen
ingress and with a high change rate of running pods it makes it easier
to define this configuration on a global level.

This change is completely backwards compatible, only adding the
possibility of defining a new key in the configmap.

## Types of changes

- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## How Has This Been Tested?

I've written and run the tests locally. Additionally, I am currently building this image locally and testing on a cluster.

## Checklist:

- [x] My change requires a change to the documentation.
- [x] I have updated the documentation accordingly.
- [x] I've read the [CONTRIBUTION](https://github.com/kubernetes/ingress-nginx/blob/main/CONTRIBUTING.md) guide
- [x] I have added tests to cover my changes.
- [x] All new and existing tests passed.


k8s-ci-robot: Welcome @renan! <br><br>It looks like this is your first PR to <a href='https://github.com/kubernetes/ingress-nginx'>kubernetes/ingress-nginx</a> 🎉. Please refer to our [pull request process documentation](https://git.k8s.io/community/contributors/guide/pull-requests.md) to help your PR have a smooth ride to approval. <br><br>You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. [Here is the bot commands documentation](https://go.k8s.io/bot-commands). <br><br>You can also check if kubernetes/ingress-nginx has [its own contribution guidelines](https://github.com/kubernetes/ingress-nginx/tree/master/CONTRIBUTING.md). <br><br>You may want to refer to our [testing guide](https://git.k8s.io/community/contributors/devel/sig-testing/testing.md) if you run into trouble with your tests not passing. <br><br>If you are having difficulty getting your pull request seen, please follow the [recommended escalation practices](https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#why-is-my-pull-request-not-getting-reviewed). Also, for tips and tricks in the contribution process you may want to read the [Kubernetes contributor cheat sheet](https://git.k8s.io/community/contributors/guide/contributor-cheatsheet/README.md). We want to make sure your contribution gets all the attention it needs! <br><br>Thank you, and welcome to Kubernetes. :smiley:
k8s-ci-robot: @renan: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
k8s-ci-robot: @renan: This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the `triage/accepted` label and provide further guidance.

The `triage/accepted` label can be added by org members by writing `/triage accepted` in a comment.


<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
k8s-ci-robot: Hi @renan. Thanks for your PR.

I'm waiting for a [kubernetes](https://github.com/orgs/kubernetes/people) member to verify that this patch is reasonable to test. If it is, they should reply with `/ok-to-test` on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should [join the org](https://git.k8s.io/community/community-membership.md#member) to skip this step.

Once the patch is verified, the new status will be reflected by the `ok-to-test` label.

I understand the commands that are listed [here](https://go.k8s.io/bot-commands?repo=kubernetes%2Fingress-nginx).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes/ingress-nginx/pull/7469#" title="Author self-approved">renan</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **thockin** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @thockin` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes%2Fingress-nginx).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes/ingress-nginx/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["thockin"]} -->
renan: @rikatz I think I covered all the cases in the new e2e test. Would you be able to verify it, please?
rikatz: /ok-to-test
k8s-ci-robot: @renan: The following tests **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-ingress-nginx-gofmt | 6e86bb757bca0a164324dd403c9d6be85ac87fbb | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/ingress-nginx/7469/pull-ingress-nginx-gofmt/1429745989278765056) | `/test pull-ingress-nginx-gofmt`
pull-ingress-nginx-codegen | 6e86bb757bca0a164324dd403c9d6be85ac87fbb | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/ingress-nginx/7469/pull-ingress-nginx-codegen/1429745989245210624) | `/test pull-ingress-nginx-codegen`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes&repo=ingress-nginx&pr=7469). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Arenan). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes/ingress-nginx/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
rikatz: /approve
@renan overall lgtm, please just fix the errors in gofmt and we are good to merge
rikatz: /retest
Maybe rebasing should fix this

Patch:
diff --git a/docs/user-guide/nginx-configuration/configmap.md b/docs/user-guide/nginx-configuration/configmap.md
index 6e6152c5f..c8aa27373 100755
--- a/docs/user-guide/nginx-configuration/configmap.md
+++ b/docs/user-guide/nginx-configuration/configmap.md
@@ -203,6 +203,7 @@ The following table shows a configuration option's name, type, and the default v
 |[global-rate-limit-memcached-max-idle-timeout](#global-rate-limit)|int|10000|
 |[global-rate-limit-memcached-pool-size](#global-rate-limit)|int|50|
 |[global-rate-limit-status-code](#global-rate-limit)|int|429|
+|[service-upstream](#service-upstream)|bool|"false"|
 
 ## add-headers
 
@@ -1224,3 +1225,8 @@ Configure `memcached` client for [Global Rate Limiting](https://github.com/kuber
 
 These settings get used by [lua-resty-global-throttle](https://github.com/ElvinEfendi/lua-resty-global-throttle)
 that ingress-nginx includes. Refer to the link to learn more about `lua-resty-global-throttle`.
+
+## service-upstream
+
+Set if the service's Cluster IP and port should be used instead of a list of all endpoints. This can be overwritten by an annotation on an Ingress rule.
+_**default:**_ "false"
diff --git a/internal/ingress/annotations/serviceupstream/main.go b/internal/ingress/annotations/serviceupstream/main.go
index 4c44356d6..4a4879682 100644
--- a/internal/ingress/annotations/serviceupstream/main.go
+++ b/internal/ingress/annotations/serviceupstream/main.go
@@ -20,6 +20,7 @@ import (
 	networking "k8s.io/api/networking/v1"
 
 	"k8s.io/ingress-nginx/internal/ingress/annotations/parser"
+	"k8s.io/ingress-nginx/internal/ingress/errors"
 	"k8s.io/ingress-nginx/internal/ingress/resolver"
 )
 
@@ -33,5 +34,13 @@ func NewParser(r resolver.Resolver) parser.IngressAnnotation {
 }
 
 func (s serviceUpstream) Parse(ing *networking.Ingress) (interface{}, error) {
-	return parser.GetBoolAnnotation("service-upstream", ing)
+	defBackend := s.r.GetDefaultBackend()
+
+	val, err := parser.GetBoolAnnotation("service-upstream", ing)
+	// A missing annotation is not a problem, just use the default
+	if err == errors.ErrMissingAnnotations {
+		return defBackend.ServiceUpstream, nil
+	}
+
+	return val, nil
 }
diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go
index b96e26772..0546e9618 100644
--- a/internal/ingress/controller/config/config.go
+++ b/internal/ingress/controller/config/config.go
@@ -860,6 +860,7 @@ func NewDefault() Configuration {
 			ProxyBuffering:           "off",
 			ProxyHTTPVersion:         "1.1",
 			ProxyMaxTempFileSize:     "1024m",
+			ServiceUpstream:          false,
 		},
 		UpstreamKeepaliveConnections:           320,
 		UpstreamKeepaliveTimeout:               60,
diff --git a/internal/ingress/controller/endpoints.go b/internal/ingress/controller/endpoints.go
index bdddcb0a0..26d7f298e 100644
--- a/internal/ingress/controller/endpoints.go
+++ b/internal/ingress/controller/endpoints.go
@@ -23,6 +23,7 @@ import (
 	"strconv"
 	"strings"
 
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/klog/v2"
 
@@ -81,7 +82,8 @@ func getEndpoints(s *corev1.Service, port *corev1.ServicePort, proto corev1.Prot
 	}
 
 	for _, ss := range ep.Subsets {
-		for _, epPort := range ss.Ports {
+		matchedPortNameFound := false
+		for i, epPort := range ss.Ports {
 
 			if !reflect.DeepEqual(epPort.Protocol, proto) {
 				continue
@@ -92,8 +94,16 @@ func getEndpoints(s *corev1.Service, port *corev1.ServicePort, proto corev1.Prot
 			if port.Name == "" {
 				// port.Name is optional if there is only one port
 				targetPort = epPort.Port
+				matchedPortNameFound = true
 			} else if port.Name == epPort.Name {
 				targetPort = epPort.Port
+				matchedPortNameFound = true
+			}
+
+			if i == len(ss.Ports)-1 && !matchedPortNameFound && port.TargetPort.Type == intstr.Int {
+				// use service target port if it's a number and no port name matched
+				// https://github.com/kubernetes/ingress-nginx/issues/7390
+				targetPort = port.TargetPort.IntVal
 			}
 
 			if targetPort <= 0 {
diff --git a/internal/ingress/defaults/main.go b/internal/ingress/defaults/main.go
index 03926baa0..bc9734257 100644
--- a/internal/ingress/defaults/main.go
+++ b/internal/ingress/defaults/main.go
@@ -161,4 +161,8 @@ type Backend struct {
 	// Sets the maximum temp file size when proxy-buffers capacity is exceeded.
 	// http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_max_temp_file_size
 	ProxyMaxTempFileSize string `json:"proxy-max-temp-file-size"`
+
+	// By default, the NGINX ingress controller uses a list of all endpoints (Pod IP/port) in the NGINX upstream configuration.
+	// It disables that behavior and instead uses a single upstream in NGINX, the service's Cluster IP and port.
+	ServiceUpstream bool `json:"service-upstream"`
 }


Test patch:
diff --git a/internal/ingress/annotations/serviceupstream/main_test.go b/internal/ingress/annotations/serviceupstream/main_test.go
index 3fbdb5785..b773e9723 100644
--- a/internal/ingress/annotations/serviceupstream/main_test.go
+++ b/internal/ingress/annotations/serviceupstream/main_test.go
@@ -23,6 +23,7 @@ import (
 	networking "k8s.io/api/networking/v1"
 	meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/parser"
+	"k8s.io/ingress-nginx/internal/ingress/defaults"
 	"k8s.io/ingress-nginx/internal/ingress/resolver"
 )
 
@@ -119,3 +120,52 @@ func TestIngressAnnotationServiceUpstreamSetFalse(t *testing.T) {
 		t.Errorf("expected annotation value to be false, got true")
 	}
 }
+
+type mockBackend struct {
+	resolver.Mock
+}
+
+// GetDefaultBackend returns the backend that must be used as default
+func (m mockBackend) GetDefaultBackend() defaults.Backend {
+	return defaults.Backend{
+		ServiceUpstream: true,
+	}
+}
+
+// Test that when we have a default configuration set on the Backend that is used
+// when we don't have the annotation
+func TestParseAnnotationsWithDefaultConfig(t *testing.T) {
+	ing := buildIngress()
+
+	val, _ := NewParser(mockBackend{}).Parse(ing)
+	enabled, ok := val.(bool)
+
+	if !ok {
+		t.Errorf("expected a bool type")
+	}
+
+	if !enabled {
+		t.Errorf("expected annotation value to be true, got false")
+	}
+}
+
+// Test that the annotation will disable the service upstream when enabled
+// in the default configuration
+func TestParseAnnotationsOverridesDefaultConfig(t *testing.T) {
+	ing := buildIngress()
+
+	data := map[string]string{}
+	data[parser.GetAnnotationWithPrefix("service-upstream")] = "false"
+	ing.SetAnnotations(data)
+
+	val, _ := NewParser(mockBackend{}).Parse(ing)
+	enabled, ok := val.(bool)
+
+	if !ok {
+		t.Errorf("expected a bool type")
+	}
+
+	if enabled {
+		t.Errorf("expected annotation value to be false, got true")
+	}
+}
diff --git a/internal/ingress/controller/endpoints_test.go b/internal/ingress/controller/endpoints_test.go
index 20d53c526..83c8e1837 100644
--- a/internal/ingress/controller/endpoints_test.go
+++ b/internal/ingress/controller/endpoints_test.go
@@ -315,7 +315,50 @@ func TestGetEndpoints(t *testing.T) {
 			[]ingress.Endpoint{},
 		},
 		{
-			"should return no endpoint when the name of the port name do not match any port in the endpoint Subsets",
+			"should return no endpoint when the name of the port name do not match any port in the endpoint Subsets and TargetPort is string",
+			&corev1.Service{
+				Spec: corev1.ServiceSpec{
+					Type:      corev1.ServiceTypeClusterIP,
+					ClusterIP: "1.1.1.1",
+					Ports: []corev1.ServicePort{
+						{
+							Name:       "default",
+							TargetPort: intstr.FromString("port-1"),
+						},
+					},
+				},
+			},
+			&corev1.ServicePort{
+				Name:       "default",
+				TargetPort: intstr.FromString("port-1"),
+			},
+			corev1.ProtocolTCP,
+			func(string) (*corev1.Endpoints, error) {
+				nodeName := "dummy"
+				return &corev1.Endpoints{
+					Subsets: []corev1.EndpointSubset{
+						{
+							Addresses: []corev1.EndpointAddress{
+								{
+									IP:       "1.1.1.1",
+									NodeName: &nodeName,
+								},
+							},
+							Ports: []corev1.EndpointPort{
+								{
+									Protocol: corev1.ProtocolTCP,
+									Port:     int32(80),
+									Name:     "another-name",
+								},
+							},
+						},
+					},
+				}, nil
+			},
+			[]ingress.Endpoint{},
+		},
+		{
+			"should return one endpoint when the name of the port name do not match any port in the endpoint Subsets and TargetPort is int",
 			&corev1.Service{
 				Spec: corev1.ServiceSpec{
 					Type:      corev1.ServiceTypeClusterIP,
@@ -355,7 +398,12 @@ func TestGetEndpoints(t *testing.T) {
 					},
 				}, nil
 			},
-			[]ingress.Endpoint{},
+			[]ingress.Endpoint{
+				{
+					Address: "1.1.1.1",
+					Port:    "80",
+				},
+			},
 		},
 		{
 			"should return one endpoint when the name of the port name match a port in the endpoint Subsets",
diff --git a/test/e2e/annotations/serviceupstream.go b/test/e2e/annotations/serviceupstream.go
new file mode 100644
index 000000000..c0bf37603
--- /dev/null
+++ b/test/e2e/annotations/serviceupstream.go
@@ -0,0 +1,128 @@
+/*
+Copyright 2021 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package annotations
+
+import (
+	"fmt"
+	"net/http"
+	"strings"
+
+	"github.com/onsi/ginkgo"
+	"github.com/stretchr/testify/assert"
+
+	"k8s.io/ingress-nginx/test/e2e/framework"
+
+	"k8s.io/ingress-nginx/internal/nginx"
+)
+
+var _ = framework.DescribeAnnotation("service-upstream", func() {
+	f := framework.NewDefaultFramework("serviceupstream")
+	host := "serviceupstream"
+
+	ginkgo.BeforeEach(func() {
+		f.NewEchoDeployment()
+	})
+
+	ginkgo.Context("when using the default value (false) and enabling in the annotations", func() {
+		ginkgo.It("should use the Service Cluster IP and Port ", func() {
+			annotations := map[string]string{
+				"nginx.ingress.kubernetes.io/service-upstream": "true",
+			}
+
+			ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations)
+			f.EnsureIngress(ing)
+
+			f.WaitForNginxServer(host, func(server string) bool {
+				return strings.Contains(server, fmt.Sprintf("server_name %s", host))
+			})
+
+			ginkgo.By("checking if the service is reached")
+			f.HTTPTestClient().
+				GET("/").
+				WithHeader("Host", host).
+				Expect().
+				Status(http.StatusOK)
+
+			ginkgo.By("checking if the Service Cluster IP and Port are used")
+			s := f.GetService(f.Namespace, framework.EchoService)
+			curlCmd := fmt.Sprintf("curl --fail --silent http://localhost:%v/configuration/backends", nginx.StatusPort)
+			output, err := f.ExecIngressPod(curlCmd)
+			assert.Nil(ginkgo.GinkgoT(), err)
+			assert.Contains(ginkgo.GinkgoT(), output, fmt.Sprintf(`{"address":"%s"`, s.Spec.ClusterIP))
+		})
+	})
+
+	ginkgo.Context("when enabling in the configmap", func() {
+		ginkgo.It("should use the Service Cluster IP and Port ", func() {
+			annotations := map[string]string{}
+
+			f.UpdateNginxConfigMapData("service-upstream", "true")
+
+			ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations)
+			f.EnsureIngress(ing)
+
+			f.WaitForNginxServer(host, func(server string) bool {
+				return strings.Contains(server, fmt.Sprintf("server_name %s", host))
+			})
+
+			ginkgo.By("checking if the service is reached")
+			f.HTTPTestClient().
+				GET("/").
+				WithHeader("Host", host).
+				Expect().
+				Status(http.StatusOK)
+
+			ginkgo.By("checking if the Service Cluster IP and Port are used")
+			s := f.GetService(f.Namespace, framework.EchoService)
+			curlCmd := fmt.Sprintf("curl --fail --silent http://localhost:%v/configuration/backends", nginx.StatusPort)
+			output, err := f.ExecIngressPod(curlCmd)
+			assert.Nil(ginkgo.GinkgoT(), err)
+			assert.Contains(ginkgo.GinkgoT(), output, fmt.Sprintf(`{"address":"%s"`, s.Spec.ClusterIP))
+		})
+	})
+
+	ginkgo.Context("when enabling in the configmap and disabling in the annotations", func() {
+		ginkgo.It("should not use the Service Cluster IP and Port", func() {
+			annotations := map[string]string{
+				"nginx.ingress.kubernetes.io/service-upstream": "false",
+			}
+
+			f.UpdateNginxConfigMapData("service-upstream", "true")
+
+			ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations)
+			f.EnsureIngress(ing)
+
+			f.WaitForNginxServer(host, func(server string) bool {
+				return strings.Contains(server, fmt.Sprintf("server_name %s", host))
+			})
+
+			ginkgo.By("checking if the service is reached")
+			f.HTTPTestClient().
+				GET("/").
+				WithHeader("Host", host).
+				Expect().
+				Status(http.StatusOK)
+
+			ginkgo.By("checking if the Service Cluster IP and Port are not used")
+			s := f.GetService(f.Namespace, framework.EchoService)
+			curlCmd := fmt.Sprintf("curl --fail --silent http://localhost:%v/configuration/backends", nginx.StatusPort)
+			output, err := f.ExecIngressPod(curlCmd)
+			assert.Nil(ginkgo.GinkgoT(), err)
+			assert.NotContains(ginkgo.GinkgoT(), output, fmt.Sprintf(`{"address":"%s"`, s.Spec.ClusterIP))
+		})
+	})
+})
diff --git a/test/e2e/framework/k8s.go b/test/e2e/framework/k8s.go
index 14937e6bd..ea34960b6 100644
--- a/test/e2e/framework/k8s.go
+++ b/test/e2e/framework/k8s.go
@@ -102,16 +102,21 @@ func (f *Framework) UpdateIngress(ingress *networking.Ingress) *networking.Ingre
 	return ing
 }
 
+// GetService gets a Service object from the given namespace, name and returns it, throws error if it does not exist.
+func (f *Framework) GetService(namespace string, name string) *core.Service {
+	s, err := f.KubeClientSet.CoreV1().Services(namespace).Get(context.TODO(), name, metav1.GetOptions{})
+	assert.Nil(ginkgo.GinkgoT(), err, "getting service")
+	assert.NotNil(ginkgo.GinkgoT(), s, "expected a service but none returned")
+
+	return s
+}
+
 // EnsureService creates a Service object and returns it, throws error if it already exists.
 func (f *Framework) EnsureService(service *core.Service) *core.Service {
 	err := createServiceWithRetries(f.KubeClientSet, f.Namespace, service)
 	assert.Nil(ginkgo.GinkgoT(), err, "creating service")
 
-	s, err := f.KubeClientSet.CoreV1().Services(f.Namespace).Get(context.TODO(), service.Name, metav1.GetOptions{})
-	assert.Nil(ginkgo.GinkgoT(), err, "getting service")
-	assert.NotNil(ginkgo.GinkgoT(), s, "expected a service but none returned")
-
-	return s
+	return f.GetService(f.Namespace, service.Name)
 }
 
 // EnsureDeployment creates a Deployment object and returns it, throws error if it already exists.