Add Ingress status management #2386

youngnick · 2020-03-26T03:42:02Z

Updates #403.

This version of Ingress status adds the wiring to manage Ingress status, and allows the configuration of an Envoy service and namespace to watch for addresses to publish into the status.Loadbalancer stanza of Ingress objects.

There are still TODOs, #2387 and #2388 amongst them.

codecov · 2020-03-26T03:51:49Z

Codecov Report

Merging #2386 into master will decrease coverage by 0.73%.
The diff coverage is 31.46%.

@@            Coverage Diff             @@
##           master    #2386      +/-   ##
==========================================
- Coverage   78.05%   77.31%   -0.74%     
==========================================
  Files          62       64       +2     
  Lines        5291     5379      +88     
==========================================
+ Hits         4130     4159      +29     
- Misses       1071     1131      +60     
+ Partials       90       89       -1

Impacted Files	Coverage Δ
cmd/contour/ingressstatus.go	`0.00% <0.00%> (ø)`
cmd/contour/serve.go	`0.00% <0.00%> (ø)`
internal/k8s/ingressstatus.go	`69.44% <69.44%> (ø)`
cmd/contour/servecontext.go	`93.04% <100.00%> (+0.12%)`	⬆️
internal/contour/handler.go	`74.52% <100.00%> (ø)`
internal/dag/cache.go	`96.12% <0.00%> (+0.70%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa02842...630b411. Read the comment docs.

jpeach · 2020-03-26T03:54:40Z

I'll review tomorrow.

Updates #403. Change StatusLoadBalancer name to IngressStatusUpdater, makes the purpose clearer. Minor fix to RBAC - "ingress/status" resource requires "update" for updating status. Signed-off-by: Nick Young <ynick@vmware.com>

Updates #403 Add informer to watch envoy's service document and pass the status.loadbalancer stanza to the ingress status updater. Signed-off-by: Dave Cheney <dave@cheney.net> cmd/contour: add IngressStatusWriter Updates #403 This PR adds a worker to doServe's group which writes back load balancer status to ingress.v1beta1 objects. In this PR, the population of the isw.lbStatus channel is not present but has been manually verified with the following code: g.Add(func(stop <-chan struct{}) error { tick := time.Tick(2 * time.Second) for { select { case <-tick: isw.lbStatus <- v1.LoadBalancerStatus{ Ingress: []v1.LoadBalancerIngress{{ Hostname: "gke.davecheney.com", }}, } case <-stop: return nil } } }) Future PRs will add workers to populate the channel. Signed-off-by: Dave Cheney <dave@cheney.net> hack: hide prepare release script from glob builds Use // +build to hide prepare-release.go from drive by go build/test ... invocations Signed-off-by: Dave Cheney <dave@cheney.net> cmd/contour: wire up envoy service name/namespace flags Updates #403 Wire up envoy service name and namespace to flags, defaults match the example deployment. Signed-off-by: Dave Cheney <dave@cheney.net> examples/contour: add ingress status subresource to Contour's role Updates #403 Signed-off-by: Dave Cheney <dave@cheney.net> internal/k8s: final cleanups Fixes #403 Signed-off-by: Dave Cheney <dave@cheney.net>

Updates #403. Change StatusLoadBalancer name to IngressStatusUpdater, makes the purpose clearer. Minor fix to RBAC - "ingress/status" resource requires "update" for updating status. Signed-off-by: Nick Young <ynick@vmware.com>

jpeach

Looks pretty good. I had a few suggestions, but the main change is dropping the envoy deployment changes.

The squashed commit message also looks a bit messy; what do you think about doing some redrafting on it?

cmd/contour/ingressstatus.go

examples/contour/03-envoy.yaml

cmd/contour/ingressstatus.go

jpeach · 2020-03-26T06:23:02Z

internal/k8s/ingressstatus.go

+	if svc.Name != s.ServiceName {
+		return
+	}
+	s.notify(svc.Status.LoadBalancer)


Nit: Is a method useful here? We could just

s.LBStatus <- svc.Status.LoadBalancer

Maybe this could boil down to:

if svc, ok := obj.(*v1.Service); ok && svc.Name == s.ServiceName { s.LBStatus <- svc.Status.LoadBalancer }

Agreed about the method, it's not really necessary for now, unless we end up wanting to do something more clever than just send down the channel.

For the code though, I feel like having individual clauses is more verbose, but far easier to read. For lack of a better word, I'd call it more idiomatic.

internal/k8s/ingressstatus_test.go

jpeach · 2020-03-26T06:42:31Z

cmd/contour/ingressstatus.go

+//    OnUpdate and OnDelete events are ignored.If a new v1.LoadBalancerStatus value
+//    is been received, operation restarts at step 3.
+// 5. If the worker is stopped, any existing informer is stopped before the worker stops.
+type ingressStatusWriter struct {


Nit: I think that calling this loadBalancerStatusWriter as an actor that writes LoadBalancerStatuses makes it clearer to my reading.

I went back and forth on this a bit, and I can see the perspective this name is coming from. Maybe we keep this name and think about renaming if it updates status on HTTPProxy too.

Yes, in my initial reading of this file it was called StatusLoadBalanceWriter or something, and it was hard to tell what it was actually acting on. The reason to move to ingressStatusWriter was to make it clear what objects were being written onto in the end.

loadBalancerStatusWriter is fine though, that covers us for HTTPProxy as well.

cmd/contour/ingressstatus.go

internal/k8s/ingressstatus.go

Updates #403 Add informer to watch envoy's service document and pass the status.loadbalancer stanza to the ingress status updater. Signed-off-by: Dave Cheney <dave@cheney.net> cmd/contour: add IngressStatusWriter Updates #403 This PR adds a worker to doServe's group which writes back load balancer status to ingress.v1beta1 objects. In this PR, the population of the isw.lbStatus channel is not present but has been manually verified with the following code: g.Add(func(stop <-chan struct{}) error { tick := time.Tick(2 * time.Second) for { select { case <-tick: isw.lbStatus <- v1.LoadBalancerStatus{ Ingress: []v1.LoadBalancerIngress{{ Hostname: "gke.davecheney.com", }}, } case <-stop: return nil } } }) Future PRs will add workers to populate the channel. Signed-off-by: Dave Cheney <dave@cheney.net> hack: hide prepare release script from glob builds Use // +build to hide prepare-release.go from drive by go build/test ... invocations Signed-off-by: Dave Cheney <dave@cheney.net> cmd/contour: wire up envoy service name/namespace flags Updates #403 Wire up envoy service name and namespace to flags, defaults match the example deployment. Signed-off-by: Dave Cheney <dave@cheney.net> examples/contour: add ingress status subresource to Contour's role Updates #403 Signed-off-by: Dave Cheney <dave@cheney.net> internal/k8s: final cleanups Fixes #403 Signed-off-by: Dave Cheney <dave@cheney.net>

Updates #403. Change StatusLoadBalancer name to IngressStatusUpdater, makes the purpose clearer. Minor fix to RBAC - "ingress/status" resource requires "update" for updating status. Signed-off-by: Nick Young <ynick@vmware.com>

Updates #403 Add informer to watch envoy's service document and pass the status.loadbalancer stanza to the ingress status updater. cmd/contour: add IngressStatusWriter Wire up envoy service name and namespace to flags, defaults match the example deployment. examples/contour: add ingress status subresource to Contour's role Signed-off-by: Dave Cheney <dave@cheney.net>

Updates #403. Change StatusLoadBalancer name to IngressStatusUpdater, makes the purpose clearer. Minor fix to RBAC - "ingress/status" resource requires "update" for updating status. Signed-off-by: Nick Young <ynick@vmware.com>

youngnick · 2020-03-31T05:04:24Z

Okay, ready for rereview.

jpeach

Looks good.

stevesloka · 2020-03-31T16:38:25Z

cmd/contour/ingressstatus.go

+			isw.log.Info("Received a new address for status.loadBalancer")
+
+			// Create new informer for the new LoadBalancerStatus
+			factory := isw.clients.NewInformerFactory()


Should we reuse the informers created earlier? What if the informer would fail to create?

The reason to throw away the old informer is to ensure that the new informer does a full resync, so that all the existing Ingress objects that are in scope will get updated (via OnAdd calls).

I think that if the informer failed to create, there would be a panic somewhere, because this method doesn't return an error for us to check.

cmd/contour/servecontext.go

Updates #403 Add informer to watch envoy's service document and pass the status.loadbalancer stanza to the ingress status updater. cmd/contour: add IngressStatusWriter Wire up envoy service name and namespace to flags, defaults match the example deployment. examples/contour: add ingress status subresource to Contour's role Signed-off-by: Dave Cheney <dave@cheney.net>

Updates #403. Change StatusLoadBalancer name to IngressStatusUpdater, makes the purpose clearer. Minor fix to RBAC - "ingress/status" resource requires "update" for updating status. Signed-off-by: Nick Young <ynick@vmware.com>

stevesloka

/lgtm

Updates #403 Add informer to watch envoy's service document and pass the status.loadbalancer stanza to the ingress status updater. cmd/contour: add IngressStatusWriter Wire up envoy service name and namespace to flags, defaults match the example deployment. examples/contour: add ingress status subresource to Contour's role Signed-off-by: Dave Cheney <dave@cheney.net>

youngnick requested a review from jpeach March 26, 2020 03:42

This was referenced Mar 26, 2020

Add command line flag to allow specification of Ingress.status.Loadbalancer value #2387

Closed

Ingress status updating should respect ingress class #2388

Closed

youngnick requested a review from stevesloka March 26, 2020 03:48

youngnick self-assigned this Mar 26, 2020

youngnick added this to the 1.4.0 milestone Mar 26, 2020

youngnick added this to In progress in Contour Project Board via automation Mar 26, 2020

youngnick added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 26, 2020

youngnick force-pushed the ingress-status-loadbalancer branch from 46beae1 to 4bb5f70 Compare March 26, 2020 03:54

youngnick force-pushed the ingress-status-loadbalancer branch from 4bb5f70 to 92ab2ba Compare March 26, 2020 03:55

youngnick force-pushed the ingress-status-loadbalancer branch from 92ab2ba to 9636b4a Compare March 26, 2020 03:57

jpeach suggested changes Mar 26, 2020

View reviewed changes

youngnick force-pushed the ingress-status-loadbalancer branch from 9636b4a to 888059d Compare March 31, 2020 05:00

youngnick force-pushed the ingress-status-loadbalancer branch from 888059d to f1b2a4e Compare March 31, 2020 05:02

youngnick requested a review from jpeach March 31, 2020 05:04

jpeach approved these changes Mar 31, 2020

View reviewed changes

stevesloka suggested changes Mar 31, 2020

View reviewed changes

davecheney and others added 2 commits April 1, 2020 09:18

youngnick force-pushed the ingress-status-loadbalancer branch from f1b2a4e to 630b411 Compare March 31, 2020 22:24

youngnick requested a review from stevesloka April 1, 2020 01:01

stevesloka approved these changes Apr 1, 2020

View reviewed changes

youngnick merged commit eeda348 into master Apr 1, 2020

Contour Project Board automation moved this from In progress to 1.4 release Apr 1, 2020

youngnick deleted the ingress-status-loadbalancer branch April 2, 2020 03:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Ingress status management #2386

Add Ingress status management #2386

youngnick commented Mar 26, 2020 •

edited

codecov bot commented Mar 26, 2020 •

edited

jpeach commented Mar 26, 2020

jpeach left a comment

jpeach Mar 26, 2020

jpeach Mar 26, 2020

youngnick Mar 31, 2020

jpeach Mar 26, 2020

youngnick Mar 31, 2020

youngnick commented Mar 31, 2020

jpeach left a comment

stevesloka Mar 31, 2020

youngnick Mar 31, 2020

stevesloka left a comment

Add Ingress status management #2386

Add Ingress status management #2386

Conversation

youngnick commented Mar 26, 2020 • edited

codecov bot commented Mar 26, 2020 • edited

Codecov Report

jpeach commented Mar 26, 2020

jpeach left a comment

Choose a reason for hiding this comment

jpeach Mar 26, 2020

Choose a reason for hiding this comment

jpeach Mar 26, 2020

Choose a reason for hiding this comment

youngnick Mar 31, 2020

Choose a reason for hiding this comment

jpeach Mar 26, 2020

Choose a reason for hiding this comment

youngnick Mar 31, 2020

Choose a reason for hiding this comment

youngnick commented Mar 31, 2020

jpeach left a comment

Choose a reason for hiding this comment

stevesloka Mar 31, 2020

Choose a reason for hiding this comment

youngnick Mar 31, 2020

Choose a reason for hiding this comment

stevesloka left a comment

Choose a reason for hiding this comment

youngnick commented Mar 26, 2020 •

edited

codecov bot commented Mar 26, 2020 •

edited