automatically expose pprof+prometheus endpoint. #677

raulk · 2020-03-12T13:21:06Z

With this change, test plans using runtime.Invoke() will automatically benefit from pprof and prometheus HTTP exports. Fixes #673.

Works in all runneres (local:exec, local:docker, cluster:k8s (via k8s port forwarding).

Fixes #698.

raulk · 2020-03-12T18:06:31Z

TODO:

publish ports on local:docker.
make sure sidecar doesn't annul the routes.

pkg/build/golang/Dockerfile.template

manifests/placebo.toml

pkg/docker/container.go

pkg/runner/local_docker.go

raulk · 2020-03-17T13:52:37Z

pkg/sidecar/docker_reactor.go

+	// Allow traffic from the host machine (acting as the gateway).
+	var (
+		gwIPs    = make(map[string]struct{})
+		gwRoutes []netlink.Route
+	)
+	for _, route := range controlRoutes {
+		if _, ok := gwIPs[route.Gw.String()]; ok {
+			continue
+		}
+		nlroutes, err := netlinkHandle.RouteGet(route.Gw)
+		if err != nil {
+			return nil, fmt.Errorf("failed to resolve route: %w", err)
+		}
+		gwIPs[route.Gw.String()] = struct{}{}
+		gwRoutes = append(gwRoutes, nlroutes...)
+	}
+
+	controlRoutes = append(controlRoutes, gwRoutes...)


This is the key. In kubernetes we don't need this, because kubernetes has native port-forwarding out of the box. In our case, we connect our test containers to the control and data networks. The former is tightly restricted, and the latter is internal and connects only the containers participating in the test. For containers to successfully expose ports to the host, besides publishing those ports, we need to ALLOW traffic from the gateway (host).

What is actually added here?
I think this would be just a route to the single host, the route to the host, right?

by the way, I see this route table on plan containers:

docker exec tg-example-prometheus2-f3de125372b3-single-1 ip route default via 192.18.0.1 dev eth0 16.0.0.0/16 dev eth1 proto kernel scope link src 16.0.0.2 192.18.0.0/16 dev eth0 proto kernel scope link src 192.18.0.6

they can ping google

docker exec tg-example-prometheus2-f3de125372b3-single-1 ping google.com PING google.com (172.217.1.142) 56(84) bytes of data. 64 bytes from dfw28s23-in-f14.1e100.net (172.217.1.142): icmp_seq=1 ttl=52 time=97.6 ms 64 bytes from dfw28s23-in-f14.1e100.net (172.217.1.142): icmp_seq=2 ttl=52 time=104 ms

Just for comparison... Without this change, from the current master,
the route table looks like this:

docker exec tg-example-prometheus2-19979292b320-single-1 ip route 16.1.0.0/16 dev eth1 proto kernel scope link src 16.1.0.3 192.18.0.3 dev eth0 src 192.18.0.7 192.18.0.4 dev eth0 src 192.18.0.7

and there is no default route, so no place to send packets outside of known networks.

docker exec tg-example-prometheus2-19979292b320-single-1 ping -c 2 google.com ping: google.com: Temporary failure in name resolution

Thanks for spotting this. Definitely unintended. I've fixed this logic in eaa06cd. Can you take it out for a spin again? Works for me.

The host can access ports exposed by the container.

The container cannot access public IPs.

Sometimes the flush could end up sending a nil bytes slice to TgWriter, and this would make it as a nil progress payload on the wire.

manifests/placebo.toml

pkg/docker/container.go

coryschwartz · 2020-03-17T22:39:21Z

pkg/sidecar/docker_reactor.go

+	// Allow traffic from the host machine (acting as the gateway).
+	var (
+		gwIPs    = make(map[string]struct{})
+		gwRoutes []netlink.Route
+	)
+	for _, route := range controlRoutes {
+		if _, ok := gwIPs[route.Gw.String()]; ok {
+			continue
+		}
+		nlroutes, err := netlinkHandle.RouteGet(route.Gw)
+		if err != nil {
+			return nil, fmt.Errorf("failed to resolve route: %w", err)
+		}
+		gwIPs[route.Gw.String()] = struct{}{}
+		gwRoutes = append(gwRoutes, nlroutes...)
+	}
+
+	controlRoutes = append(controlRoutes, gwRoutes...)


What is actually added here?
I think this would be just a route to the single host, the route to the host, right?

pkg/docker/container.go

manifests/placebo.toml

coryschwartz · 2020-03-17T22:46:43Z

pkg/sidecar/docker_reactor.go

+	// Allow traffic from the host machine (acting as the gateway).
+	var (
+		gwIPs    = make(map[string]struct{})
+		gwRoutes []netlink.Route
+	)
+	for _, route := range controlRoutes {
+		if _, ok := gwIPs[route.Gw.String()]; ok {
+			continue
+		}
+		nlroutes, err := netlinkHandle.RouteGet(route.Gw)
+		if err != nil {
+			return nil, fmt.Errorf("failed to resolve route: %w", err)
+		}
+		gwIPs[route.Gw.String()] = struct{}{}
+		gwRoutes = append(gwRoutes, nlroutes...)
+	}
+
+	controlRoutes = append(controlRoutes, gwRoutes...)


by the way, I see this route table on plan containers:

docker exec tg-example-prometheus2-f3de125372b3-single-1 ip route default via 192.18.0.1 dev eth0 16.0.0.0/16 dev eth1 proto kernel scope link src 16.0.0.2 192.18.0.0/16 dev eth0 proto kernel scope link src 192.18.0.6

they can ping google

docker exec tg-example-prometheus2-f3de125372b3-single-1 ping google.com PING google.com (172.217.1.142) 56(84) bytes of data. 64 bytes from dfw28s23-in-f14.1e100.net (172.217.1.142): icmp_seq=1 ttl=52 time=97.6 ms 64 bytes from dfw28s23-in-f14.1e100.net (172.217.1.142): icmp_seq=2 ttl=52 time=104 ms

coryschwartz · 2020-03-17T23:05:29Z

pkg/sidecar/docker_reactor.go

+	// Allow traffic from the host machine (acting as the gateway).
+	var (
+		gwIPs    = make(map[string]struct{})
+		gwRoutes []netlink.Route
+	)
+	for _, route := range controlRoutes {
+		if _, ok := gwIPs[route.Gw.String()]; ok {
+			continue
+		}
+		nlroutes, err := netlinkHandle.RouteGet(route.Gw)
+		if err != nil {
+			return nil, fmt.Errorf("failed to resolve route: %w", err)
+		}
+		gwIPs[route.Gw.String()] = struct{}{}
+		gwRoutes = append(gwRoutes, nlroutes...)
+	}
+
+	controlRoutes = append(controlRoutes, gwRoutes...)


Just for comparison... Without this change, from the current master,
the route table looks like this:

docker exec tg-example-prometheus2-19979292b320-single-1 ip route 16.1.0.0/16 dev eth1 proto kernel scope link src 16.1.0.3 192.18.0.3 dev eth0 src 192.18.0.7 192.18.0.4 dev eth0 src 192.18.0.7

and there is no default route, so no place to send packets outside of known networks.

docker exec tg-example-prometheus2-19979292b320-single-1 ping -c 2 google.com ping: google.com: Temporary failure in name resolution

raulk · 2020-03-18T11:39:18Z

Re-requested review from @coryschwartz. I've rectified the logic that whitelists the host's IP addr.

coryschwartz

Appears to work, I can hit http://<plan_ip>:6060/metrics and it responds with output,

The plan can't ping outside the network. LGTM

coryschwartz · 2020-03-18T17:59:17Z

pkg/sidecar/docker_reactor.go

+	switch {
+	case len(pub) == 0:
+		logging.S().Warnw("failed to discover gateway/host address; no routes to public IPs", "container_id", container.ID)
+	case pub[0].Gw == nil:


With this change, I have the following routing table in the plans:

docker exec tg-example-prometheus2-a0bf3767448d-single-9 ip r 16.2.0.0/16 dev eth1 proto kernel scope link src 16.2.0.3 192.18.0.1 dev eth0 src 192.18.0.6 192.18.0.3 dev eth0 src 192.18.0.6 192.18.0.4 dev eth0 src 192.18.0.6

Very nice, no default gateway.

and the outside is un-pingable, also very nice.

docker exec tg-example-prometheus2-a0bf3767448d-single-9 ping 1.1.1.1 connect: Network is unreachable

coryschwartz · 2020-03-18T18:00:21Z

pkg/sidecar/docker_reactor.go

+// Sidecar doesn't whitelist traffic to public addresses, but it special-cases
+// traffic between the container and the host, so that pprof, metrics and other
+// ports can be exposed to the Docker host.
+var PublicAddr = net.ParseIP("1.1.1.1")


This is the IP address of Cloudfliar DNS. I don't have a problem with that, just pointing that out.

Yep, that's ok. This will not perform any lookup of any kind, it just matches against the kernel routing tables.

coryschwartz · 2020-03-18T18:01:39Z

pkg/tgwriter/tgwriter.go

@@ -44,6 +44,10 @@ type Error struct {
 }

 func (tgw *TgWriter) Write(p []byte) (n int, err error) {
+	if p == nil {


Does this prevent errors from popping up?

It suppresses empty writes from making it onto the wire, which I found can happen when Flush is called. This was leading to the intermittent bug with the describe command.

raulk added 2 commits March 12, 2020 13:08

automatically expose pprof+prometheus endpoint.

1398f7c

local:docker runner: publish all ports.

536c743

raulk changed the title ~~automatically expose pprof+prometheus endpoint.~~ [DO NOT MERGE] automatically expose pprof+prometheus endpoint. Mar 12, 2020

wip

a2e6243

raulk added this to the Testground v0.3 milestone Mar 12, 2020

Robmat05 modified the milestones: Testground v0.3, Testground v0.4 Mar 16, 2020

raulk added 2 commits March 17, 2020 11:55

Merge branch 'master' into feat/automatic-pprof

59ad5ce

sidecar/docker: whitelist traffic from host to test plan containers.

a34d7f5

raulk modified the milestones: Testground v0.4, Testground v0.3 Mar 17, 2020

remove stray file.

5bed0dd

raulk commented Mar 17, 2020

View reviewed changes

raulk changed the title ~~[DO NOT MERGE] automatically expose pprof+prometheus endpoint.~~ automatically expose pprof+prometheus endpoint. Mar 17, 2020

raulk requested a review from coryschwartz March 17, 2020 13:53

fix flakiness in describe command.

b96a996

Sometimes the flush could end up sending a nil bytes slice to TgWriter, and this would make it as a nil progress payload on the wire.

Robmat05 modified the milestones: Testground v0.3, Testground v0.4 Mar 17, 2020

raulk modified the milestones: Testground v0.4, Testground v0.3 Mar 17, 2020

Robmat05 assigned raulk Mar 17, 2020

Robmat05 mentioned this pull request Mar 17, 2020

uncomment the panic test in manifests/placebo.toml #698

Closed

coryschwartz suggested changes Mar 17, 2020

View reviewed changes

sidecar: docker reactor: fix logic that whitelists traffic from host.

eaa06cd

raulk requested a review from coryschwartz March 18, 2020 11:38

coryschwartz approved these changes Mar 18, 2020

View reviewed changes

Robmat05 added the status/waiting label Mar 18, 2020

raulk merged commit e16f9ca into master Mar 18, 2020

raulk deleted the feat/automatic-pprof branch March 18, 2020 18:52

raulk added status/done and removed status/waiting labels Mar 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

automatically expose pprof+prometheus endpoint. #677

automatically expose pprof+prometheus endpoint. #677

raulk commented Mar 12, 2020 •

edited

raulk commented Mar 12, 2020 •

edited

raulk Mar 17, 2020

coryschwartz Mar 17, 2020

coryschwartz Mar 17, 2020

coryschwartz Mar 17, 2020

raulk Mar 18, 2020

coryschwartz Mar 17, 2020

coryschwartz Mar 17, 2020

coryschwartz Mar 17, 2020

raulk commented Mar 18, 2020

coryschwartz left a comment

coryschwartz Mar 18, 2020

coryschwartz Mar 18, 2020

raulk Mar 18, 2020

coryschwartz Mar 18, 2020

raulk Mar 18, 2020

automatically expose pprof+prometheus endpoint. #677

automatically expose pprof+prometheus endpoint. #677

Conversation

raulk commented Mar 12, 2020 • edited

raulk commented Mar 12, 2020 • 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

raulk commented Mar 18, 2020

coryschwartz 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

raulk commented Mar 12, 2020 •

edited

raulk commented Mar 12, 2020 •

edited