Create Global Backend Ingress #3404

dtomcej · 2018-05-28T22:06:26Z

What does this PR do?

Allows for an ingress with a Global Parameter Backend to create a default backend route.

Motivation

Fixes #1051 , and also allows for traefik to support web frameworks that use global defaults.

More

Added/updated tests
Added/updated documentation

Additional Notes

Priority is set based on the ingress priority. Enabling this without setting priority on other ingresses may run into issues with overlapping rules...May want to discuss moving the default priority for kubernetes ingresses to 10, to allow for the default route to be 5, and still give some flexibility.

nmengin

Hello @dtomcej .

Many thanks for this PR.
Few questions/suggestions... 😉

nmengin · 2018-05-29T07:26:56Z

provider/kubernetes/kubernetes.go

+
+					service, exists, err := k8sClient.GetService(i.Namespace, i.Spec.Backend.ServiceName)
+					if err != nil {
+						log.Errorf("Error while retrieving service information from k8s API %s/%s: %v", i.Namespace, i.Spec.Backend.ServiceName, err)


Because the error seems to be logged in a higher level function, WDYT to replace the log.Errorf by a fmt.Errorf and return this new error without logging it?

Yes. On second thought, that makes sense

nmengin · 2018-05-29T07:29:27Z

provider/kubernetes/kubernetes.go

+					}
+					if !exists {
+						log.Warnf("Endpoints not found for %s/%s", service.Namespace, service.Name)
+						break


If the service doest not exist, an error is logged and the continue instruction is called.
Can you explain to me why is there a Warn log and a break instruction when the entrypoint does not exist please?

Service not existing is usually due to someone mistyping something into an ingress hence the Error being logged, and the continue skips processing the rest of that ingress, and continues processing the next ingress.

The warn and break are due to endpoints not being found. This is a very common occurrence, as k8s pods are created and killed. These are not user-error prone, and the rest of the ingress can still be processed if there are no endpoints found.

nmengin · 2018-05-29T07:30:22Z

provider/kubernetes/kubernetes.go

+					}
+
+					for _, subset := range endpoints.Subsets {
+						endpointPort := i.Spec.Backend.ServicePort


Are we sure the i.Spec.Backend.ServicePort is never nil or empty?

dtomcej · 2018-06-04T19:44:08Z

@timoreimann thoughts?

create example ingress add default route update continues add tests fix tests update docs final cleanup revert frontend name change add new builder

nmengin

Hello @dtomcej .

More suggestions 😉

nmengin · 2018-06-11T07:06:01Z

docs/configuration/backends/kubernetes.md

+This will allow users to create a "default backend" that will match all unmatched requests.
+
+!!! note
+    Due to traefik's use of priorities, you may have to set this ingress priority lower than other ingresses in your environment, to avoid this global ingress from satisfying requests that _could_ match other ingresses.


s/traefik/Træfik please 😉

We should add this as a linter, so they don't get missed.

nmengin · 2018-06-11T07:34:43Z

docs/configuration/backends/kubernetes.md

+### Global Default Backend Ingresses
+
+Ingresses can be created that look like the following:
+```yaml


Can you add an empty line before and after the yaml example please?

nmengin · 2018-06-11T07:38:30Z

provider/kubernetes/kubernetes.go

+
+					service, exists, err := k8sClient.GetService(i.Namespace, i.Spec.Backend.ServiceName)
+					if err != nil {
+						return nil, fmt.Errorf("Error while retrieving service information from k8s API %s/%s: %v", i.Namespace, i.Spec.Backend.ServiceName, err)


Please downcase the Error word in the error message.

nmengin · 2018-06-11T07:39:05Z

provider/kubernetes/kubernetes.go

+
+					endpoints, exists, err := k8sClient.GetEndpoints(service.Namespace, service.Name)
+					if err != nil {
+						return nil, fmt.Errorf("Error retrieving endpoint information from k8s API %s/%s: %v", service.Namespace, service.Name, err)


Please downcase the Error word in the error message.

nmengin · 2018-06-11T07:41:35Z

provider/kubernetes/kubernetes.go

@@ -264,8 +363,7 @@ func (p *Provider) loadIngresses(k8sClient Client) (*types.Configuration, error)

 				service, exists, err := k8sClient.GetService(i.Namespace, pa.Backend.ServiceName)
 				if err != nil {
-					log.Errorf("Error while retrieving service information from k8s API %s/%s: %v", i.Namespace, pa.Backend.ServiceName, err)
-					return nil, err
+					return nil, fmt.Errorf("Error while retrieving service information from k8s API %s/%s: %v", i.Namespace, pa.Backend.ServiceName, err)


Please downcase the Error word in the error message.

nmengin · 2018-06-11T07:41:45Z

provider/kubernetes/kubernetes.go

@@ -300,8 +398,7 @@ func (p *Provider) loadIngresses(k8sClient Client) (*types.Configuration, error)
 						} else {
 							endpoints, exists, err := k8sClient.GetEndpoints(service.Namespace, service.Name)
 							if err != nil {
-								log.Errorf("Error retrieving endpoints %s/%s: %v", service.Namespace, service.Name, err)
-								return nil, err
+								return nil, fmt.Errorf("Error retrieving endpoint information from k8s API %s/%s: %v", service.Namespace, service.Name, err)


Please downcase the Error word in the error message.

nmengin · 2018-06-11T07:42:04Z

provider/kubernetes/kubernetes_test.go

+			if _, err := provider.loadIngresses(client); err != nil {
+				switch {
+				case client.apiServiceError != nil:
+					expected := errors.New("Error while retrieving service information from k8s API testing/service1: failed kube api call")


Please downcase the Error word in the error message.

nmengin · 2018-06-11T07:42:17Z

provider/kubernetes/kubernetes_test.go

+					}
+
+				case client.apiEndpointsError != nil:
+					expected := errors.New("Error retrieving endpoint information from k8s API testing/service1: failed kube api call")


Please downcase the Error word in the error message.

nmengin

Change my review state.

m3co-code

That was a tough one for me to review but it's a nice feature and a nice PR :-) I left a couple of comments.

Regarding the priority: it's a tricky area. I have to think about it more before I can say something qualified :D

m3co-code · 2018-06-11T19:53:28Z

provider/kubernetes/kubernetes.go

+			continue
+		}
+
+		err = p.updateIngressStatus(i, k8sClient)


This could be combined with the next line.

m3co-code · 2018-06-11T20:18:05Z

docs/configuration/backends/kubernetes.md

+This will allow users to create a "default backend" that will match all unmatched requests.
+
+!!! note
+    Due to Træfik's use of priorities, you may have to set this ingress priority lower than other ingresses in your environment, to avoid this global ingress from satisfying requests that _could_ match other ingresses.


Can we add an example on how you would set this priority? I think it would be nice to just show it shortly or link to the point where it's documented.

Could we just add this to example ingress above?

m3co-code · 2018-06-11T21:47:41Z

provider/kubernetes/kubernetes.go

+
+func (p *Provider) buildTemplateFromIngress(i *extensionsv1beta1.Ingress, templateObjects types.Configuration, k8sClient Client) (types.Configuration, error) {
+	if i.Spec.Backend != nil {
+		// If a global backend is configured, add this as a "default route" by adding a wildcard frontend, with the priority of the ingress.


Can we extract the whole part inside the if to a method called like: AddGlobalBackendAsDefaultRoute(...)? Comments tend to become out-dated or forgotten and it would make it a bit better from the readability.

m3co-code · 2018-06-11T22:06:56Z

provider/kubernetes/kubernetes.go

+		}
+
+		for _, subset := range endpoints.Subsets {
+			endpointPort := endpointPortNumber(corev1.ServicePort{Protocol: "TCP", Port: int32(i.Spec.Backend.ServicePort.IntValue())}, subset.Ports)


It seems to be still valid to specify the service port as string, referencing the actual service port. Therefore, I think we have to write a second function that does that logic for the ingress service port. WDYT?

You are correct, but its even simpler for this spec! 😄

Tests added!

m3co-code · 2018-06-11T22:08:41Z

provider/kubernetes/kubernetes.go

-					}
+			protocol := "http"
+			for _, address := range subset.Addresses {
+				if endpointPort == 443 {


Do we also have to extend this check for the https prefix in the port name? Goes together with the comment above.

m3co-code · 2018-06-11T22:10:59Z

provider/kubernetes/kubernetes.go

@@ -736,3 +791,58 @@ func getRateLimit(i *extensionsv1beta1.Ingress) *types.RateLimit {

 	return rateLimit
 }
+
+func createEmptyBackend(name string, templateObjects types.Configuration) (types.Configuration, error) {
+	if _, exists := templateObjects.Backends[name]; !exists {


If you turn around the logic we could save one level of indent :) The comment below would also become obsolete as the error message documents the if then. The same holds true for the functions below.

This reverts commit 98fae73.

This reverts commit 4a752d5.

ldez · 2018-06-12T18:19:40Z

provider/kubernetes/kubernetes_test.go

-			if _, err := provider.loadIngresses(client); err != apiErr {
-				t.Errorf("Got error %v, wanted error %v", err, apiErr)
+			if _, err := provider.loadIngresses(client); err != nil {
+				switch {


could you use a if instead of switch

ldez · 2018-06-12T18:24:58Z

provider/kubernetes/kubernetes_test.go

+					expected := errors.New("error while retrieving service information from k8s API testing/service1: failed kube api call")
+					if err.Error() != expected.Error() {
+						t.Errorf("Got error: %+v, wanted error: %+v, provided: %v", err, expected, client.apiServiceError)
+					}


assert.EqualError(t, err, "error while retrieving service information from k8s API testing/service1: failed kube api call")

ldez · 2018-06-12T18:25:19Z

provider/kubernetes/kubernetes_test.go

+					expected := errors.New("error retrieving endpoint information from k8s API testing/service1: failed kube api call")
+					if err.Error() != expected.Error() {
+						t.Errorf("Got error: %+v, wanted error: %+v, provided: %v", err, expected, client.apiServiceError)
+					}


assert.EqualError(t, err, "error retrieving endpoint information from k8s API testing/service1: failed kube api call")

provider/kubernetes/kubernetes_test.go

+	)
+	assert.Equal(t, expected, actual)
+}
+func TestLoadGlobalIngressWithHttpsPortNames(t *testing.T) {


nmengin

Hey @dtomcej .

One last little note before a LGTM 😉

nmengin · 2018-06-14T07:09:33Z

docs/configuration/backends/kubernetes.md

+This will allow users to create a "default backend" that will match all unmatched requests.
+
+!!! note
+    Due to Træfik's use of priorities, you may have to set this ingress priority lower than other ingresses in your environment, to avoid this global ingress from satisfying requests that _could_ match other ingresses. To do this, use the `traefik.ingress.kubernetes.io/priority` annotation (as seen in the General Annotations table above) on your ingresses accordingly.


One sentence per line please.

ldez

LGTM 🍰

nmengin

LGTM 👏 👏

m3co-code

LGTM, nice job :-)

timoreimann · 2018-07-03T14:53:56Z

docs/configuration/backends/kubernetes.md

+
+!!! note
+    Due to Træfik's use of priorities, you may have to set this ingress priority lower than other ingresses in your environment, to avoid this global ingress from satisfying requests that _could_ match other ingresses.
+    To do this, use the `traefik.ingress.kubernetes.io/priority` annotation (as seen in the General Annotations table above) on your ingresses accordingly.


Can we create a link out of "General annotations table" and drop "above" then?

timoreimann · 2018-07-03T15:12:36Z

provider/kubernetes/kubernetes.go

@@ -270,8 +280,7 @@ func (p *Provider) loadIngresses(k8sClient Client) (*types.Configuration, error)

 				service, exists, err := k8sClient.GetService(i.Namespace, pa.Backend.ServiceName)
 				if err != nil {
-					log.Errorf("Error while retrieving service information from k8s API %s/%s: %v", i.Namespace, pa.Backend.ServiceName, err)
-					return nil, err
+					return nil, fmt.Errorf("error while retrieving service information from k8s API %s/%s: %v", i.Namespace, pa.Backend.ServiceName, err)


Is this change related to what the PR essentially tries to achieve?

I don't mind a good drive-by improvement. What I'm wondering here though is if we had a reason to log the error first. From what I can tell, this line will eventually log the error. I'm not sure if this reads well though as it would show up as

Cannot connect to Provider: error while retrieving service information from k8s API [...]

We are beyond the connection phase at this point already, aren't we?
How about annotating the error returned here like return fmt.Errorf("failed to load Ingress: %s", err) and log the error from line 159 as-in?

Similar for line 318.

Looking at it in that perspective, I like the original logging, then return pattern.

timoreimann · 2018-07-03T15:13:08Z

provider/kubernetes/kubernetes.go

@@ -401,6 +409,100 @@ func (p *Provider) loadConfig(templateObjects types.Configuration) *types.Config
 	return configuration
 }

+func (p *Provider) addGlobalBackend(k8sClient Client, i *extensionsv1beta1.Ingress, templateObjects *types.Configuration) error {


I'd rename k8sClient to just cl.

timoreimann · 2018-07-03T15:13:58Z

provider/kubernetes/kubernetes.go

+func (p *Provider) addGlobalBackend(k8sClient Client, i *extensionsv1beta1.Ingress, templateObjects *types.Configuration) error {
+	// Ensure that we are not duplicating the frontend
+	if _, exists := templateObjects.Frontends[defaultFrontendName]; exists {
+		return errors.New("duplicate frontend: global-default-backend")


You probably mean global-default-frontend. Why not use the constant here?

timoreimann · 2018-07-03T15:14:04Z

provider/kubernetes/kubernetes.go

+
+	// Ensure we are not duplicating the backend
+	if _, exists := templateObjects.Backends[defaultBackendName]; exists {
+		return errors.New("duplicate backend: global-default-backend")


Constant usage here too?

timoreimann · 2018-07-03T15:18:12Z

provider/kubernetes/kubernetes.go

@@ -401,6 +409,100 @@ func (p *Provider) loadConfig(templateObjects types.Configuration) *types.Config
 	return configuration
 }

+func (p *Provider) addGlobalBackend(k8sClient Client, i *extensionsv1beta1.Ingress, templateObjects *types.Configuration) error {


The method name seems incorrect. To the least, the method also populates the frontend, right?

What is the scope of the method, what's it supposed to do? (Its large size may hint at a smell.)

EDIT: I did not mean to imply that we should refactor this method now -- this could well be done by another PR. We should just think about the method name maybe.

The reason I chose this name is in reference to the backend section of the spec, which k8s named global default backend. If the name bothers us, we can change it in an upcoming refactor :)

Ah okay, missed that -- makes sense to me now, so feel free to keep it. :)

timoreimann

Changing state just to be sure, because we already have three LGTMs.

timoreimann · 2018-07-03T16:20:42Z

provider/kubernetes/kubernetes.go

@@ -306,8 +316,7 @@ func (p *Provider) loadIngresses(k8sClient Client) (*types.Configuration, error)
 						} else {
 							endpoints, exists, err := k8sClient.GetEndpoints(service.Namespace, service.Name)
 							if err != nil {
-								log.Errorf("Error retrieving endpoints %s/%s: %v", service.Namespace, service.Name, err)
-								return nil, err
+								return nil, fmt.Errorf("error retrieving endpoint information from k8s API %s/%s: %v", service.Namespace, service.Name, err)


Do you want to undo this one as well?

timoreimann

Happy!

dtomcej requested a review from a team as a code owner May 28, 2018 22:06

traefiker added area/provider/k8s/ingress size/M status/0-needs-triage labels May 28, 2018

dtomcej added this to the 1.7 milestone May 28, 2018

ldez added kind/enhancement a new or improved feature. status/2-needs-review and removed status/0-needs-triage labels May 29, 2018

nmengin requested changes May 29, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label May 29, 2018

dtomcej force-pushed the issue-1051 branch from ef5f188 to 1ccc135 Compare June 4, 2018 19:15

dtomcej removed the contributor/waiting-for-corrections label Jun 5, 2018

dtomcej force-pushed the issue-1051 branch 2 times, most recently from 4d5e575 to 128ce73 Compare June 6, 2018 14:43

initial commit

cde784b

create example ingress add default route update continues add tests fix tests update docs final cleanup revert frontend name change add new builder

dtomcej force-pushed the issue-1051 branch from 128ce73 to cde784b Compare June 6, 2018 14:47

nmengin reviewed Jun 11, 2018

View reviewed changes

nmengin requested changes Jun 11, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jun 11, 2018

dtomcej added 3 commits June 11, 2018 08:39

error casing

629c327

extract code to function

4a752d5

skip setting status on error

98fae73

dtomcej removed the contributor/waiting-for-corrections label Jun 11, 2018

m3co-code suggested changes Jun 11, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jun 11, 2018

ldez and others added 4 commits June 12, 2018 11:19

Revert "skip setting status on error"

4bdd1c1

This reverts commit 98fae73.

Revert "extract code to function"

8e4cc52

This reverts commit 4a752d5.

review: isolate code.

a203bc6

fix review, add more tests

f7686ad

ldez suggested changes Jun 12, 2018

View reviewed changes

all the reviews yo

8815c11

ldez removed the contributor/waiting-for-corrections label Jun 13, 2018

review: minor.

0f26fd7

nmengin requested changes Jun 14, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jun 14, 2018

new line

33b7120

ldez removed the contributor/waiting-for-corrections label Jun 14, 2018

ldez approved these changes Jun 15, 2018

View reviewed changes

nmengin approved these changes Jun 15, 2018

View reviewed changes

m3co-code approved these changes Jun 18, 2018

View reviewed changes

Merge branch 'master'

2248a79

timoreimann reviewed Jul 3, 2018

View reviewed changes

timoreimann suggested changes Jul 3, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jul 3, 2018

fix review

014d776

dtomcej removed the contributor/waiting-for-corrections label Jul 3, 2018

timoreimann reviewed Jul 3, 2018

View reviewed changes

undo drive-by

94a9bdf

timoreimann approved these changes Jul 3, 2018

View reviewed changes

timoreimann added status/3-needs-merge and removed status/2-needs-review labels Jul 3, 2018

traefiker added the status/4-merge-in-progress label Jul 3, 2018

Merge remote-tracking branch 'upstream/master' into issue-1051

67e0b0c

traefiker removed the status/4-merge-in-progress label Jul 3, 2018

traefiker merged commit 461ebf6 into traefik:master Jul 3, 2018

traefiker removed the status/3-needs-merge label Jul 3, 2018

Create Global Backend Ingress #3404

Create Global Backend Ingress #3404

Conversation

dtomcej commented May 28, 2018 • edited Loading

What does this PR do?

Motivation

More

Additional Notes

nmengin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtomcej commented Jun 4, 2018

nmengin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmengin left a comment

Choose a reason for hiding this comment

m3co-code left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

nmengin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez left a comment

Choose a reason for hiding this comment

nmengin left a comment

Choose a reason for hiding this comment

m3co-code left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

dtomcej commented May 28, 2018 •

edited

Loading

timoreimann Jul 3, 2018 •

edited

Loading