do not require --default-backend-service #3126

ElvinEfendi · 2018-09-25T03:38:59Z

What this PR does / why we need it:
~~At this time the PR is mainly to foster discussion around the necessity of default-backend-service.~~
The idea is not everyone requires custom default backend and it should not be ingress-nginx's concern to provide a sophisticated default backend. What I'm suggesting in this PR is to make default-backend-service optional. When it is not given then ingress-nginx returns 404 directly from Nginx. When configured then the existing behaviour is held.

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:

aledbf · 2018-09-25T12:01:07Z

What I'm suggesting in this PR is to make default-backend-service optional.

This approach lgtm

aledbf · 2018-09-25T12:02:24Z

The original reason for the default backend was related to the limitation of providing a default route when no valid servers were found.

jonpulsifer · 2018-09-25T16:23:55Z

I understand this is [WIP], but what else needs to be done so we can ? The reason for #3125 was an inadvertent information disclosure of the /metrics endpoint -- which resulted in that (now closed) PR

aledbf · 2018-09-25T17:00:52Z

@jonpulsifer pass the tests

ElvinEfendi · 2018-09-26T01:15:45Z

rootfs/etc/nginx/template/nginx.tmpl

@@ -584,6 +584,17 @@ http {

    {{ end }}

+    # backend for when default-backend-service is not configured or it does not have endpoints
+    server {
+        listen {{ $all.ListenPorts.Default }} default_server {{ if $all.Cfg.ReusePort }}reuseport{{ end }} backlog={{ $all.BacklogSize }};


I kind of repurposed this $all.ListenPorts.Default, I don't see it being used anywhere else.

ElvinEfendi · 2018-09-26T02:10:53Z

The PR is ready now.

aledbf · 2018-09-26T09:51:48Z

/lgtm

k8s-ci-robot · 2018-09-26T09:51:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ElvinEfendi,aledbf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Rude-Monkey · 2018-09-26T12:59:18Z

Hello!

I'm not very familiar with Ingress-nginx yet, but shouldn't you build a new Docker image for Ingress-nginx to use with the mandatory.yaml file?

My Ingress-nginx pod won't launch because it's missing a default backend now that the deployment file has been changed. Since the Ingress-nginx code has been changed, I would expect that same deployment file not to be using the same Docker image as before.

Please tell me if I'm missing anything here =)

aledbf · 2018-09-26T13:16:53Z

@Rude-Monkey apologies for this. The deployment files should be updated after the release.
I just merged a PR to fix this issue.

Rude-Monkey · 2018-09-27T09:15:40Z

Thanks @aledbf !

No problem at all. =D
I was starting to panick a little as I was already investigating an issue with the /deploy/provider/aws/service-nlb.yaml file when applied to a cluster created on AWS using kops with the ingress-nginx service not being correctly created.

In fact, default-backend-service is not required. See kubernetes/ingress-nginx#3126 Signed-off-by: zhanglei <[email protected]>

It's not needed since nginx 0.20.0. See the following pull-requests for context: * kubernetes/ingress-nginx#3126 * kubernetes/ingress-nginx#3156

In fact, default-backend-service is not required. See kubernetes/ingress-nginx#3126 Signed-off-by: zhanglei <[email protected]>

In fact, default-backend-service is not required. See kubernetes/ingress-nginx#3126 Signed-off-by: zhanglei <[email protected]> Signed-off-by: Roland Gritzer <[email protected]>

In fact, default-backend-service is not required. See kubernetes/ingress-nginx#3126 Signed-off-by: zhanglei <[email protected]>

k8s-ci-robot requested review from aledbf and antoineco September 25, 2018 03:39

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 26, 2018

do not require --default-backend-service

b3a22f7

ElvinEfendi commented Sep 26, 2018

View reviewed changes

update docs

14815c5

ElvinEfendi changed the title ~~[WIP] do not require --default-backend-service~~ do not require --default-backend-service Sep 26, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 26, 2018

ElvinEfendi mentioned this pull request Sep 26, 2018

[404-server] Conceal /metrics and /healthz behind port 10254 #3125

Merged

k8s-ci-robot assigned aledbf Sep 26, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2018

k8s-ci-robot merged commit c3ce6b8 into kubernetes:master Sep 26, 2018

ElvinEfendi deleted the optional-default-backend-service branch September 26, 2018 13:34

jonpulsifer mentioned this pull request Sep 27, 2018

A new home for 404-server (defaultbackend) kubernetes/ingress-gce#498

Closed

aledbf mentioned this pull request Nov 14, 2018

[404-server] Removes 404 server #3156

Merged

antoineco mentioned this pull request Jan 8, 2019

Remove default backend requirement #3196

Merged

reegnz mentioned this pull request Jul 17, 2019

Remove ingress-nginx default backend kubernetes/minikube#4786

Merged

scuzhanglei mentioned this pull request Jul 25, 2019

Fix default-backend-service doc helm/charts#15868

Merged

4 tasks

k8s-ci-robot pushed a commit to helm/charts that referenced this pull request Jul 25, 2019

Update README.md (#15868)

05db816

In fact, default-backend-service is not required. See kubernetes/ingress-nginx#3126 Signed-off-by: zhanglei <[email protected]>

replicated-ship bot mentioned this pull request Jul 25, 2019

Update nginx-ingress with edits made in Replicated Ship Cloud resouer/array#82

Open

gypsydiver pushed a commit to gypsydiver/charts that referenced this pull request Jul 25, 2019

Update README.md (helm#15868)

c70eae5

In fact, default-backend-service is not required. See kubernetes/ingress-nginx#3126 Signed-off-by: zhanglei <[email protected]>

medyagh pushed a commit to medyagh/minikube that referenced this pull request Aug 1, 2019

Remove ingress-nginx default backend

6b23003

It's not needed since nginx 0.20.0. See the following pull-requests for context: * kubernetes/ingress-nginx#3126 * kubernetes/ingress-nginx#3156

medyagh pushed a commit to medyagh/minikube that referenced this pull request Aug 1, 2019

Remove ingress-nginx default backend

de27472

It's not needed since nginx 0.20.0. See the following pull-requests for context: * kubernetes/ingress-nginx#3126 * kubernetes/ingress-nginx#3156

ThoTischner pushed a commit to bitsbeats/charts that referenced this pull request Aug 13, 2019

Update README.md (helm#15868)

7039fd2

In fact, default-backend-service is not required. See kubernetes/ingress-nginx#3126 Signed-off-by: zhanglei <[email protected]>

ralgozino mentioned this pull request Sep 2, 2019

Updated to 0.25.1 and dropped default-backend sighupio/fury-kubernetes-ingress#5

Merged

kengou pushed a commit to kengou/charts that referenced this pull request Sep 18, 2019

Update README.md (helm#15868)

b79359b

In fact, default-backend-service is not required. See kubernetes/ingress-nginx#3126 Signed-off-by: zhanglei <[email protected]>

ramkumarvs pushed a commit to yugabyte/charts-helm-fork that referenced this pull request Sep 30, 2019

Update README.md (helm#15868)

9cef133

In fact, default-backend-service is not required. See kubernetes/ingress-nginx#3126 Signed-off-by: zhanglei <[email protected]>

gaida pushed a commit to gaida/charts that referenced this pull request Oct 3, 2019

Update README.md (helm#15868)

1fe112b

In fact, default-backend-service is not required. See kubernetes/ingress-nginx#3126 Signed-off-by: zhanglei <[email protected]>

dmitry-j-mikhin pushed a commit to dmitry-j-mikhin/ingress-chart that referenced this pull request Feb 27, 2020

Update README.md (#15868)

ebdc7c1

In fact, default-backend-service is not required. See kubernetes/ingress-nginx#3126 Signed-off-by: zhanglei <[email protected]>

sozercan mentioned this pull request Jan 13, 2022

Defaultbackend image in Helm chart is deprecated #8141

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

do not require --default-backend-service #3126

do not require --default-backend-service #3126

ElvinEfendi commented Sep 25, 2018 •

edited

Loading

aledbf commented Sep 25, 2018

aledbf commented Sep 25, 2018

jonpulsifer commented Sep 25, 2018

aledbf commented Sep 25, 2018

ElvinEfendi Sep 26, 2018

ElvinEfendi commented Sep 26, 2018

aledbf commented Sep 26, 2018

k8s-ci-robot commented Sep 26, 2018

Rude-Monkey commented Sep 26, 2018

aledbf commented Sep 26, 2018

Rude-Monkey commented Sep 27, 2018

do not require --default-backend-service #3126

do not require --default-backend-service #3126

Conversation

ElvinEfendi commented Sep 25, 2018 • edited Loading

aledbf commented Sep 25, 2018

aledbf commented Sep 25, 2018

jonpulsifer commented Sep 25, 2018

aledbf commented Sep 25, 2018

ElvinEfendi Sep 26, 2018

Choose a reason for hiding this comment

ElvinEfendi commented Sep 26, 2018

aledbf commented Sep 26, 2018

k8s-ci-robot commented Sep 26, 2018

Rude-Monkey commented Sep 26, 2018

aledbf commented Sep 26, 2018

Rude-Monkey commented Sep 27, 2018

ElvinEfendi commented Sep 25, 2018 •

edited

Loading