xds-server updates #437

iffyio · 2021-11-12T13:58:05Z

Adds health probes and configurable logging
Fixes a bug where we pass each endpoint as its own locality
rather than all in the same locality
Add support for routing tokens - if a routing token annotation
is set, then the server configures the proxy with a
CaptureBytes=>TokenRouter in the filter chain.
Adds a simple dockerfile that can be used to build the server

- Adds health probes and configurable logging - Fixes a bug where we pass each endpoint as its own locality rather than all in the same locality - Add support for routing tokens - if a routing token annotation is set, then the server configures the proxy with a `CaptureBytes=>TokenRouter` in the filter chain. - Adds a simple dockerfile that can be used to build the server

XAMPPRocky

LGTM, though I'll obviously defer to @markmandel on the Go code.

xds/ci/Dockerfile

xds/pkg/server/server.go

markmandel · 2021-11-16T00:40:32Z

xds/pkg/server/server.go

 	}
 }

+func (s *Server) startAdminServer(ctx context.Context, address string) {


Suggested change

func (s *Server) startAdminServer(ctx context.Context, address string) {

func (s *Server) Run(ctx context.Context) {

Move address into New, and then this object matches the New/Run pattern as everywhere else.

updated to create address within the function, this is a helper function that is already being called from Server.Run

markmandel · 2021-11-16T00:41:03Z

xds/pkg/server/server.go

 }

 // NewServer returns a new Server
 func New(
 	logger *log.Logger,
-	port int16,
+	xdsPort int16,


This should fail linting 🤔 , since the function name doesn't match the comment (and it should be NewServer)

not sure how to configure it, golint fails as expected but golangci-lint doesn't do this check

Here's the config we use for Agones:
https://github.com/googleforgames/agones/blob/97dbf4677ac1c5223e434bca8ffc84654f6ae851/.golangci.yml#L47-L65

There's lots more linters 😃 (want to do it here, or in aseparatee PR?)

TIL (source)

When a function returns a value of type pkg.T, where T is not Pkg, the function name may include T to make client code easier to understand. A common situation is a package with multiple New-like functions:

So since this returns a Server, New is perfectly acceptable as a constructor name!

Updated to use the same linters, (though none of those still catches the functionname/comment matching)

Weird. 🤔 Seems fine now though.

xds/cmd/controller.go

markmandel · 2021-11-17T23:04:22Z

xds/pkg/server/server.go

 }

 // NewServer returns a new Server
 func New(
 	logger *log.Logger,
-	port int16,
+	xdsPort int16,


Here's the config we use for Agones:
https://github.com/googleforgames/agones/blob/97dbf4677ac1c5223e434bca8ffc84654f6ae851/.golangci.yml#L47-L65

There's lots more linters 😃 (want to do it here, or in aseparatee PR?)

markmandel · 2021-11-17T23:09:53Z

xds/pkg/server/server.go

 }

 // NewServer returns a new Server
 func New(
 	logger *log.Logger,
-	port int16,
+	xdsPort int16,


TIL (source)

When a function returns a value of type pkg.T, where T is not Pkg, the function name may include T to make client code easier to understand. A common situation is a package with multiple New-like functions:

So since this returns a Server, New is perfectly acceptable as a constructor name!

markmandel · 2021-11-17T23:27:18Z

xds/ci/Dockerfile

+
+RUN go build -o /server cmd/controller.go
+
+FROM alpine:latest


This should run as a non-root user.

I would also recommend pinning an alpine version, I think major.minor is fine though, since alphine

Actually, if we are going down this path, our other image is distroless -- I would suggest keeping one base image rather than two. distroless has a very low surface area, and also makes it very easy to run as non root in an already setup best-practice environment.

Go example:
https://github.com/GoogleContainerTools/distroless/blob/main/examples/go/Dockerfile
Although we would want to run gcr.io/distroless/static:nonroot

Local example:

quilkin/image/Dockerfile

Line 17 in 6a0762f

FROM gcr.io/distroless/cc:nonroot as base

markmandel · 2021-11-17T23:56:57Z

xds/pkg/server/server.go

+
+	http.Handle("/metrics", promhttp.Handler())
+
+	http.HandleFunc("/healthz", func(w http.ResponseWriter, r *http.Request) {


Is this meant to be a liveness probe or a readiness probe?

It would be good if this was /live or /ready so that it was clear.

I'm assuming that it's a readiness probe, in that if the k8s control plane goes down, we don't want to keep restarting this process because the health check is failing.

In that vein, it's probably useful to have both, as if the process can't respond to a simple http response, it should probably be restarted.

But if the k8s api is down we should keep logging this via liveness, but not restart the process.

Updated to /ready, I assume if folks want to use it for liveness they can point to the same endpoint as well?

Yeah they could. But I don't think people will want to restart the container if the K8s control plane goes down (but it's a choice 😁)

Doesn't have to be this PR, but I'd be tempted to have a /live endpoint as well that was just a Http Handler that returns 200.

Reason being that if the process does freeze (and can't respond to just a simple http request) for whatever reason, then it will restart and hopefully recover again.

markmandel

Love it!

markmandel · 2021-11-22T23:02:27Z

xds/pkg/server/server.go

 }

 // NewServer returns a new Server
 func New(
 	logger *log.Logger,
-	port int16,
+	xdsPort int16,


Weird. 🤔 Seems fine now though.

markmandel · 2021-11-22T23:32:36Z

Doesn't have to be this PR, but I'd be tempted to have a /live endpoint as well that was just a Http Handler that returns 200.

Only a minor thought for me - so if you want to merge as is, please feel free. Just leaving it open if it resonated and you wanted to do it with this PR 👍🏻

google-cla bot added the cla: yes label Nov 12, 2021

github-actions bot added the size/l label Nov 12, 2021

iffyio marked this pull request as ready for review November 12, 2021 14:28

iffyio requested review from markmandel and XAMPPRocky as code owners November 12, 2021 14:28

XAMPPRocky approved these changes Nov 15, 2021

View reviewed changes

xds/ci/Dockerfile Show resolved Hide resolved

markmandel reviewed Nov 16, 2021

View reviewed changes

updates

29e40b0

iffyio requested a review from markmandel November 16, 2021 07:11

markmandel reviewed Nov 17, 2021

View reviewed changes

update docker,golangci-lint

a744504

iffyio requested a review from markmandel November 19, 2021 13:45

markmandel approved these changes Nov 22, 2021

View reviewed changes

markmandel mentioned this pull request Nov 24, 2021

Proposal: Cut 0.3.0 Release. #444

Closed

iffyio added 2 commits November 29, 2021 07:40

Merge remote-tracking branch 'origin/main' into iu/xds-server-updates

0e6c0e3

add /live

7c650bb

iffyio force-pushed the iu/xds-server-updates branch from c37d1d4 to 7c650bb Compare November 29, 2021 06:50

iffyio merged commit 58ac663 into main Nov 29, 2021

iffyio deleted the iu/xds-server-updates branch November 29, 2021 08:23

markmandel added kind/bug Something isn't working kind/feature New feature or request labels Feb 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds-server updates #437

xds-server updates #437

iffyio commented Nov 12, 2021

XAMPPRocky left a comment

markmandel Nov 16, 2021

iffyio Nov 16, 2021

markmandel Nov 16, 2021

iffyio Nov 16, 2021

markmandel Nov 17, 2021

markmandel Nov 17, 2021

iffyio Nov 19, 2021

markmandel Nov 22, 2021

markmandel Nov 17, 2021

markmandel Nov 17, 2021

markmandel Nov 17, 2021

iffyio Nov 19, 2021

markmandel Nov 17, 2021

iffyio Nov 19, 2021

markmandel Nov 22, 2021

markmandel left a comment

markmandel Nov 22, 2021

markmandel commented Nov 22, 2021

	func (s *Server) startAdminServer(ctx context.Context, address string) {
	func (s *Server) Run(ctx context.Context) {


		http.Handle("/metrics", promhttp.Handler())

		http.HandleFunc("/healthz", func(w http.ResponseWriter, r *http.Request) {

xds-server updates #437

xds-server updates #437

Conversation

iffyio commented Nov 12, 2021

XAMPPRocky 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

Choose a reason for hiding this comment

markmandel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markmandel commented Nov 22, 2021