Enable Ingress Status updates #3324

dtomcej · 2018-05-14T20:37:41Z

What does this PR do?

Allows Traefik to update the Status section of Ingress objects either via static IP/Hostname configuration, or via extracting the status section of a fronting service.

Motivation

Fixes #2173

Users requested that the status field be set so that DNS and other discovery tools would work properly.

More

Added/updated tests
Added/updated documentation

Additional Notes

This configuration is entirely optional, and will only apply if one of the 3 configuration settings are enabled.

ldez · 2018-05-14T21:01:35Z

provider/kubernetes/client.go

@@ -166,6 +167,22 @@ func (c *clientImpl) GetIngresses() []*extensionsv1beta1.Ingress {
 	return result
 }

+//UpdateIngressStatus updates an Ingress with a provided status.


Could you add a space between // and the message

ldez · 2018-05-14T21:01:53Z

provider/kubernetes/client.go

@@ -166,6 +167,22 @@ func (c *clientImpl) GetIngresses() []*extensionsv1beta1.Ingress {
 	return result
 }

+//UpdateIngressStatus updates an Ingress with a provided status.
+func (c *clientImpl) UpdateIngressStatus(namespace, name, ip, hostname string) error {
+	item, _, _ := c.factories[c.lookupNamespace(namespace)].Extensions().V1beta1().Ingresses().Informer().GetStore().GetByKey(namespace + "/" + name)


You need to manage the error.

Fun police ;)

ldez · 2018-05-14T21:03:39Z

provider/kubernetes/kubernetes.go

@@ -129,6 +137,9 @@ func (p *Provider) Provide(configurationChan chan<- types.ConfigMessage, pool *s
 						if err != nil {
 							return err
 						}
+						if err != nil {


Seems not needed

Missed on move

ldez · 2018-05-14T21:03:43Z

provider/kubernetes/kubernetes.go

@@ -155,6 +167,50 @@ func (p *Provider) Provide(configurationChan chan<- types.ConfigMessage, pool *s
 	return nil
 }

+func (p *Provider) updateIngressStatuses(k8sClient Client) error {
+	//Only run these ingress updates if config is provided. This will reduce overhead


Could you add a space between // and the message

ldez · 2018-05-14T21:04:26Z

provider/kubernetes/kubernetes.go

@@ -155,6 +167,50 @@ func (p *Provider) Provide(configurationChan chan<- types.ConfigMessage, pool *s
 	return nil
 }

+func (p *Provider) updateIngressStatuses(k8sClient Client) error {
+	//Only run these ingress updates if config is provided. This will reduce overhead
+	if p.IngressEndpoint.Hostname != "" || p.IngressEndpoint.IP != "" || p.IngressEndpoint.PublishedService != "" {


I think you need to check if p.IngressEndpoint is not nil

Ah yes. It breaks spectacularly if p.IngressEndpoint is nil!

ldez · 2018-05-14T21:11:28Z

provider/kubernetes/kubernetes.go

+			} else {
+				err := k8sClient.UpdateIngressStatus(i.Namespace, i.Name, p.IngressEndpoint.IP, p.IngressEndpoint.Hostname)
+				if err != nil {
+					log.Errorf("Cannot update Ingress %s/%s due to error: %s", i.Namespace, i.Name, err)


I think the log can be transform to an error.

ldez · 2018-05-14T21:12:17Z

docs/configuration/backends/kubernetes.md

+
+```
+
+You can configure a static hostname or IP address that traefik will add to the status section of ingress objects that it manages. If you prefer, you can provide a service, which traefik will copy the status spec from. This will give more flexibility in cloud/dynamic environments.


One sentence, one line

ldez · 2018-05-14T21:14:37Z

docs/configuration/backends/kubernetes.md

+# ip = 127.0.0.1
+#
+# Optional
+# publishedservice = default/cheddar


all these values don't seem optional.

All three are optional!

ldez · 2018-05-14T21:15:15Z

provider/kubernetes/kubernetes.go

+				s := strings.Split(p.IngressEndpoint.PublishedService, "/")
+				serviceNamespace, serviceName := s[0], s[1]
+				svc, exists, _ := k8sClient.GetService(serviceNamespace, serviceName)
+				//Set the status to the status of the fronting LB service


Could you add a space between // and the message

ldez · 2018-05-14T21:15:21Z

provider/kubernetes/kubernetes.go

+				svc, exists, _ := k8sClient.GetService(serviceNamespace, serviceName)
+				//Set the status to the status of the fronting LB service
+				if exists && svc.Status.LoadBalancer.Ingress == nil {
+					//service exists, but has no LoadBalancer status


Could you add a space between // and the message

ldez · 2018-05-14T23:37:39Z

docs/configuration/backends/kubernetes.md

+# Enable IngressEndpoint configuration. This will allow Traefik to update the status section of ingress objects.
+#
+# Optional
+# Default: false


You could remove this line, because kubernetes.ingressendpoint is not a boolean.
I know that related to CLI option.

ldez

LGTM

dtomcej · 2018-05-17T17:33:46Z

provider/kubernetes/kubernetes.go

+
+		err = p.updateIngressStatus(i, k8sClient)
+		if err != nil {
+			log.Printf("cannot update Ingress %s/%s due to error: %s", i.Namespace, i.Name, err)


should be errorf

Not here, because we don't want to stop to browse all other ingress

But why would log.Errorf stop processing? If it's an error we should mark it that way in the logs, shouldn't we?

You mix %s and %v for errors. I don't really mind as there is no logical difference in the output, but consistence is a thing and %s should be faster as it doesn't have to do type assertions and therefore would stick with it.

Logs should start with upper case.

oh yes sorry I was thinking about fmt.Errorf

mmatur

LGTM

m3co-code

@dtomcej I really like the code clarity and structure, nice job! Most of my comments are nit-picky as hell and they are basically only about consistency in how we write things.

I think that we should write some unit tests for the updateIngressStatus and UpdateIngressStatus method respectively, WDYT?

m3co-code · 2018-05-18T07:50:51Z

docs/configuration/backends/kubernetes.md

+# [kubernetes.ingressEndpoint]
+#
+# One must be configured.
+# Publishedservice will override the hostname and ip settings if configured.


Shouldn't PublishedService be "camel cased" here?

I think ip should be written IP as it's usual in go.

m3co-code · 2018-05-18T07:54:56Z

docs/configuration/backends/kubernetes.md

@@ -105,14 +119,20 @@ A label selector can be defined to filter on specific Ingress objects only.

 See [label-selectors](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors) for details.

+### `ingressEndpoint`
+
+You can configure a static hostname or IP address that traefik will add to the status section of ingress objects that it manages.


I think the name traefik should be written as Traefik. At least we usually do this.

m3co-code · 2018-05-18T07:56:38Z

docs/configuration/backends/kubernetes.md

@@ -105,14 +119,20 @@ A label selector can be defined to filter on specific Ingress objects only.

 See [label-selectors](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors) for details.

+### `ingressEndpoint`
+
+You can configure a static hostname or IP address that traefik will add to the status section of ingress objects that it manages.


You mix ingress and Ingress. I think for Kubernetes objects it would make sense to always write them in uppercase. WDYT?

m3co-code · 2018-05-18T07:59:15Z

provider/kubernetes/client.go

@@ -166,6 +167,33 @@ func (c *clientImpl) GetIngresses() []*extensionsv1beta1.Ingress {
 	return result
 }

+// UpdateIngressStatus updates an Ingress with a provided status.
+func (c *clientImpl) UpdateIngressStatus(namespace, name, ip, hostname string) error {
+	item, exists, err := c.factories[c.lookupNamespace(namespace)].Extensions().V1beta1().Ingresses().Informer().GetStore().GetByKey(namespace + "/" + name)


Should we extract the Ingress identifier (namespace + "/" + name) into a variable? You're rebuilding the same string multiple times in the method.

m3co-code · 2018-05-18T08:02:29Z

docs/configuration/backends/kubernetes.md

+#
+# [kubernetes.ingressEndpoint]
+#
+# One must be configured.


I think we have to emphasize more that there are two different configuration possibilities. So either: hostname + IP OR the service name. Or do I understand this wrong?

I think the explanation is on the next line.

Can we change it to At least one must be configured? This + the next sentence describes everything well then.

m3co-code · 2018-05-18T08:05:36Z

provider/kubernetes/kubernetes.go

+
+		err = p.updateIngressStatus(i, k8sClient)
+		if err != nil {
+			log.Printf("cannot update Ingress %s/%s due to error: %s", i.Namespace, i.Name, err)


But why would log.Errorf stop processing? If it's an error we should mark it that way in the logs, shouldn't we?

m3co-code · 2018-05-18T08:06:52Z

provider/kubernetes/kubernetes.go

+
+		err = p.updateIngressStatus(i, k8sClient)
+		if err != nil {
+			log.Printf("cannot update Ingress %s/%s due to error: %s", i.Namespace, i.Name, err)


You mix %s and %v for errors. I don't really mind as there is no logical difference in the output, but consistence is a thing and %s should be faster as it doesn't have to do type assertions and therefore would stick with it.

m3co-code · 2018-05-18T08:08:32Z

provider/kubernetes/kubernetes.go

+	}
+
+	if p.IngressEndpoint.PublishedService == "" {
+		return k8sClient.UpdateIngressStatus(i.Namespace, i.Name, p.IngressEndpoint.IP, p.IngressEndpoint.Hostname)


Should we maybe emit a warning when one of the fields is an empty string? Or is that expected?

I think is is expected that there would only be one of IP or Hostname ... perhaps we should check that they are not both empty?

I added a check if the both are empty.

m3co-code · 2018-05-18T08:10:23Z

docs/configuration/backends/kubernetes.md

+#
+# hostname = "localhost"
+# ip = "127.0.0.1"
+# publishedService = "default/cheddar"


Can we document that it's namespace/servicename? Or do you think this is clear to Kubernetes users? I didn't get it in the first moments. Maybe simply a different example would help like publishedService = namespaceName/serviceName

m3co-code · 2018-05-18T08:10:58Z

provider/kubernetes/kubernetes.go

+
+		err = p.updateIngressStatus(i, k8sClient)
+		if err != nil {
+			log.Printf("cannot update Ingress %s/%s due to error: %s", i.Namespace, i.Name, err)


Logs should start with upper case.

errm

Design is very good, code is Clean

LGTM

m3co-code

Thanks for walking with me through the nit-pick marshes! Great job guys 👏

add mock function test factory clean up kube go fix client usage create endpoint type add service status copy update docs gofmt golint code review fixes wtf spelling update docs review: simplify. fix test panic fix spacing

piclemx · 2018-05-24T19:00:49Z

when this will be merged?

ldez · 2018-05-24T19:12:58Z

It's already merged.

anandsinghkunwar · 2018-09-18T07:35:36Z

Will this be cherry-picked for the older versions?

timoreimann · 2018-09-18T08:59:02Z

@anandsinghkunwar we cherry-pick bug fixes only to minimize the risk of affecting otherwise stable releases adversely. Especially in this case, the code delta is quite big.

Traefik 1.7 is on the horizon though, it shouldn't take much longer anymore until we ship.

dtomcej requested a review from a team as a code owner May 14, 2018 20:37

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

dtomcej force-pushed the issue-2173 branch from 57e2472 to bcfba92 Compare May 14, 2018 20:48

ldez added status/2-needs-review and removed status/0-needs-triage labels May 14, 2018

ldez requested changes May 14, 2018

View reviewed changes

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

dtomcej force-pushed the issue-2173 branch from 7d59e8f to f49b55c Compare May 14, 2018 22:31

ldez requested changes May 14, 2018

View reviewed changes

dtomcej force-pushed the issue-2173 branch from f49b55c to 5a62727 Compare May 15, 2018 13:51

ldez removed the contributor/waiting-for-corrections label May 16, 2018

ldez added this to the 1.7 milestone May 16, 2018

ldez force-pushed the issue-2173 branch from 94db77b to 50f1f88 Compare May 16, 2018 18:25

dtomcej force-pushed the issue-2173 branch from c118777 to 11cd6fb Compare May 16, 2018 19:59

ldez force-pushed the issue-2173 branch from eb179f9 to e19b2af Compare May 16, 2018 20:17

ldez approved these changes May 16, 2018

View reviewed changes

ldez added the kind/enhancement a new or improved feature. label May 16, 2018

dtomcej commented May 17, 2018

View reviewed changes

mmatur approved these changes May 17, 2018

View reviewed changes

m3co-code suggested changes May 18, 2018

View reviewed changes

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

errm approved these changes May 18, 2018

View reviewed changes

traefiker removed the contributor/waiting-for-corrections label May 18, 2018

ldez force-pushed the issue-2173 branch 2 times, most recently from f6ab2b2 to ff83b6a Compare May 18, 2018 11:09

m3co-code approved these changes May 18, 2018

View reviewed changes

ldez added status/3-needs-merge and removed status/2-needs-review labels May 18, 2018

traefiker added the status/4-merge-in-progress label May 18, 2018

dtomcej and others added 7 commits May 18, 2018 11:48

create loadbalancer status

550982b

add mock function test factory clean up kube go fix client usage create endpoint type add service status copy update docs gofmt golint code review fixes wtf spelling update docs review: simplify. fix test panic fix spacing

review: simplify.

19b3ec6

review: some changes.

4462da4

review: some changes.

53b35ec

review: some changes.

ad98c91

review: add tests.

a9697fe

review: doc.

296d296

traefiker force-pushed the issue-2173 branch from 15570f7 to 296d296 Compare May 18, 2018 11:48

traefiker removed the status/4-merge-in-progress label May 18, 2018

traefiker merged commit 9227d32 into traefik:master May 18, 2018

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

dtomcej mentioned this pull request May 24, 2018

Traefik does not update the kubernetes ingress status or address #3377

Closed

alapidas mentioned this pull request Jun 13, 2018

Panic after setting ingress endpoint #3490

Closed

dtomcej deleted the issue-2173 branch June 13, 2018 21:28

liyanliang1994 mentioned this pull request Jun 15, 2018

Terminal disconnect when another workspace is starting or stopping on kubernetes eclipse-che/che#9943

Closed

lblackstone mentioned this pull request Nov 21, 2018

Ingress creation error pulumi/pulumi-kubernetes#277

Closed

seslattery mentioned this pull request Jun 21, 2019

Add support (might just be docs) for external DNS emissary-ingress/emissary#1057

Closed

wi1dcard mentioned this pull request Oct 25, 2019

helm (v2.15.) upgrade with --wait is waiting till timeout and not showing status even though all kubernetes resources are in ready state. helm/helm#6751

Closed


		```

		You can configure a static hostname or IP address that traefik will add to the status section of ingress objects that it manages. If you prefer, you can provide a service, which traefik will copy the status spec from. This will give more flexibility in cloud/dynamic environments.

Enable Ingress Status updates #3324

Enable Ingress Status updates #3324

Conversation

dtomcej commented May 14, 2018 • edited by ldez

What does this PR do?

Motivation

More

Additional Notes

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

ldez 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

ldez May 18, 2018 • edited

Choose a reason for hiding this comment

mmatur 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

m3co-code May 18, 2018 • edited

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

errm left a comment

Choose a reason for hiding this comment

m3co-code left a comment

Choose a reason for hiding this comment

piclemx commented May 24, 2018

ldez commented May 24, 2018

anandsinghkunwar commented Sep 18, 2018

timoreimann commented Sep 18, 2018

dtomcej commented May 14, 2018 •

edited by ldez

ldez May 18, 2018 •

edited

m3co-code May 18, 2018 •

edited