Add metrics #158

JoshVanL · 2020-06-18T23:35:33Z

This PR metrics adds a number of metrics to the proxy.

The PR should be split up fairly well per commit.

It would be good to have a bit of insight into whether these metrics look sane... It is tricky not to make the number of time series explode here.

/assign @munnerz
/assign @simonswine
fixes #156

james-w · 2020-06-29T13:09:45Z

docs/tasks/metrics.md

+
+### kube_oidc_proxy_http_client_requests
+counter - {http status code, path, remote address}
+The number of requests for incoming requests.


Suggested change

The number of requests for incoming requests.

The number of incoming requests.

james-w · 2020-06-29T13:10:24Z

docs/tasks/metrics.md

+
+### kube_oidc_proxy_http_server_requests
+counter - {http status code, path, remote address}
+The requests for outgoing server requests.


Suggested change

The requests for outgoing server requests.

The number of outgoing server requests.

james-w · 2020-06-29T13:11:56Z

cmd/app/options/app.go

+		"Adress to serving metrics on at the /metrics path. An empty address will "+
+			"disable serving metrics.")


Worth mentioning that this can't conflict with the other addresses here?

Also the readiness probe is exposed as a port, but this is a full address. I think that makes sense despite the inconsistency.

james-w · 2020-06-29T13:14:13Z

cmd/app/run.go

+					return err
+				}
+			} else {
+				klog.Info("metrics listen address empty, disabling serving")


Suggested change

klog.Info("metrics listen address empty, disabling serving")

klog.Info("metrics listen address empty, disabling serving metrics")

deploy/charts/kube-oidc-proxy/values.yaml

james-w · 2020-06-29T13:40:09Z

pkg/proxy/handlers.go

 			http.Error(rw, "Impersonation requests are disabled when using kube-oidc-proxy", http.StatusForbidden)
 			return

 			// No name given or available in oidc request
 		case errNoName:
-			klog.V(2).Infof("no name available in oidc info %s", r.RemoteAddr)
+			statusCode = http.StatusForbidden
+			klog.V(2).Infof("no name available in oidc info %s", remoteAddr)
 			http.Error(rw, "Username claim not available in OIDC Issuer response", http.StatusForbidden)


Suggested change

http.Error(rw, "Username claim not available in OIDC Issuer response", http.StatusForbidden)

http.Error(rw, "Username claim not available in OIDC Issuer response", statusCode)

james-w · 2020-06-29T13:40:28Z

pkg/proxy/handlers.go

 			http.Error(rw, "Username claim not available in OIDC Issuer response", http.StatusForbidden)
 			return

 			// No impersonation configuration found in context
 		case errNoImpersonationConfig:
-			klog.Errorf("if you are seeing this, there is likely a bug in the proxy (%s): %s", r.RemoteAddr, err)
+			statusCode = http.StatusInternalServerError
+			klog.Errorf("if you are seeing this, there is likely a bug in the proxy (%s): %s", remoteAddr, err)
 			http.Error(rw, "", http.StatusInternalServerError)


Suggested change

http.Error(rw, "", http.StatusInternalServerError)

http.Error(rw, "", statusCode)

james-w · 2020-06-29T13:40:44Z

pkg/proxy/handlers.go

 			http.Error(rw, "", http.StatusInternalServerError)
 			return

 			// Server or unknown error
 		default:
-			klog.Errorf("unknown error (%s): %s", r.RemoteAddr, err)
+			statusCode = http.StatusInternalServerError
+			klog.Errorf("unknown error (%s): %s", remoteAddr, err)
 			http.Error(rw, "", http.StatusInternalServerError)


Suggested change

http.Error(rw, "", http.StatusInternalServerError)

http.Error(rw, "", statusCode)

james-w · 2020-06-29T13:42:56Z

pkg/proxy/proxy.go

+	var statusCode int
+	if resp != nil {
+		statusCode = resp.StatusCode
+	}


So we will report statusCode as nil if there was an error in round-tripping? What will the client see in that case?

The client will see the error code that comes from the error handler if the there is an error.

If the response is nil then we have to report the status code as 0.

I actually made a mistake here that if there is an error, then we will be observing a client call twice (once here, and again in the error handler). I have now wrapped the client observation in an err != nil. The server still need to be observed here regardless.

Is there some wrapper we could make the observation for both the error case (with the real error code) and the success case? I guess we lose access to some of the labels if we do that?

Yeah exactly, the error handler code path isn't in most requests so we can't invoke it there, and indeed we loose some info.

james-w · 2020-06-29T13:44:20Z

pkg/metrics/metrics.go

@@ -0,0 +1,218 @@
+// Copyright Jetstack Ltd. See LICENSE for details.
+package metrics


How is the prom client testing support? Is it possible to write tests that check that samples were recorded with certain labels set?

It is, but it is a bit of a string matching exercise. Happy to stick something in if you are keen...

https://github.com/jetstack/cert-manager/blob/master/pkg/metrics/certificates_test.go

I don't think it's critical, but I always prefer tests if they aren't too onerous.

Signed-off-by: JoshVanL <[email protected]>

buckets for request duration Signed-off-by: JoshVanL <[email protected]>

Signed-off-by: JoshVanL <[email protected]>

jetstack-bot · 2020-06-29T16:41:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoshVanL

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoshVanL]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

JoshVanL · 2020-06-29T16:42:13Z

Thanks for the review @james-w :)
I've made the suggested changes or responded to you comments, and should be ready for another look
/unassign
/assign @james-w

Signed-off-by: JoshVanL <[email protected]>

munnerz · 2020-07-03T10:15:13Z

cmd/app/options/app.go

@@ -43,6 +44,10 @@ func (k *KubeOIDCProxyOptions) AddFlags(fs *pflag.FlagSet) *KubeOIDCProxyOptions
 	fs.IntVarP(&k.ReadinessProbePort, "readiness-probe-port", "P", 8080,
 		"Port to expose readiness probe.")

+	fs.StringVar(&k.MetricsListenAddress, "metrics-serving-address", "0.0.0.0:80",
+		"Address to serve metrics on at the /metrics path. An empty address will "+
+			"disable serving metrics. Cannot use the same address as proxy or probe.")


Is it possible to explicitly set --metrics-serving-address="" without it being defaulted to 0.0.0.0:80?

munnerz · 2020-07-03T10:16:22Z

cmd/app/run.go

 				return err
 			}
+			hooks.AddPreShutdownHook("Readiness Probe", readinessHandler.Shutdown)


nit: are spaces in hook names okay/normal?

munnerz · 2020-07-03T10:17:50Z

deploy/charts/kube-oidc-proxy/templates/deployment.yaml

@@ -74,6 +82,9 @@ spec:
          {{- range $key, $value := .Values.extraArgs -}}
          - "--{{ $key }}={{ $value -}}"
          {{ end }}
+          {{- if and .Values.metrics.enabled }}


What this and for? Should it be if .Values.metrics.enabled?

munnerz · 2020-07-03T10:19:08Z

deploy/charts/kube-oidc-proxy/templates/deployment.yaml

@@ -74,6 +82,9 @@ spec:
          {{- range $key, $value := .Values.extraArgs -}}
          - "--{{ $key }}={{ $value -}}"
          {{ end }}
+          {{- if and .Values.metrics.enabled }}
+          - "--metrics-serving-address={{ .Values.metrics.address }}:{{ .Values.metrics.port }}"
+          {{ end }}


nit: I think we want this to be {{- end }} to avoid a trailing whitespace, but I see that others don't use it and it isn't really essential at all to this PR :)

munnerz · 2020-07-03T10:19:49Z

deploy/yaml/kube-oidc-proxy.yaml

+      annotations:
+        prometheus.io/path: /metrics
+        prometheus.io/port: "80"
+        prometheus.io/scrape: "true"


This manifest does not have - "--metrics-serving-address={{ .Values.metrics.address }}:{{ .Values.metrics.port }}" in it, but does enable scraping of the endpoint. Is it being generated from the Helm chart?

munnerz · 2020-07-03T10:52:55Z

pkg/probe/probe.go

+		klog.Infof("serving readiness probe on %s/ready", ln.Addr())
+
+		if err := h.Serve(ln); err != nil {
+			klog.Errorf("failed to serve readiness probe: %s", err)


Again, we don't have any way to pass 'up' the stack if this is failing from what I can tell? Meaning if the readiness probe listener fails, the pod will never be restarted (assuming the liveness probe handler doesn't fail).

This isn't a change from what happened before from what I can see, but may be worth looking into as a follow up

munnerz · 2020-07-03T10:55:43Z

pkg/proxy/handlers.go

 		// Auth request and handle unauthed
 		info, ok, err := p.oidcRequestAuther.AuthenticateRequest(req)
 		if err != nil {
 			// Since we have failed OIDC auth, we will try a token review, if enabled.
+			p.metrics.IncrementOIDCAuthCount(false, remoteAddr, "")


If a user specifically isn't connecting with OIDC, is there any way to avoid that one user flooding the metric with failure counts?

If I wanted to use token passthrough for one client, I'd end out with something that looks like an error but actually isn't really (from what I can tell).

Maybe a distinct 'number of passthroughs' metric? 🤷‍♂️

munnerz · 2020-07-03T10:57:33Z

pkg/proxy/handlers.go

+
+	// Setup unauthed handler so that it is passed through the audit
+	unauthedHandler := audit.NewUnauthenticatedHandler(p.auditor, func(rw http.ResponseWriter, req *http.Request) {
+		_, remoteAddr := context.RemoteAddr(req)


bit of a nit, but why does RemoteAddr return the request as well? Does it modify it in any way? 🤔

Ah, I see:

clientAddress, ok := ctx.Value(clientAddressKey).(string) if !ok { clientAddress = xff.GetRemoteAddr(req) req = req.WithContext(request.WithValue(ctx, clientAddressKey, clientAddress)) }

Not one for this PR, but that seems like non-obvious behaviour 😬 is xff.GetRemoteAddr(req) particularly expensive? What about just making this only accept the req, and then having a separate function WithRemoteAddress(ctx context.Context, remoteAddr string) context.Context or something? I don't quite see why we need to add that context back into the request, but perhaps I am missing something 😄 is this to allow passing through a header containing the connecting client's address?

munnerz · 2020-07-03T11:02:57Z

pkg/proxy/proxy.go

@@ -199,8 +186,26 @@ func (p *Proxy) RoundTrip(req *http.Request) (*http.Response, error) {
 	// Set up impersonation request.
 	rt := transport.NewImpersonatingRoundTripper(*conf, p.clientTransport)

+	req, remoteAddr := context.RemoteAddr(req)
+	serverDuration := time.Now()
+	clientDuration := context.ClientRequestTimestamp(req)


nit: you might want to consider adopting the k8s.io/utils 'clock' package sometime soon - this will be tough to test otherwise 😄

munnerz · 2020-07-03T11:04:15Z

pkg/proxy/tokenreview/tokenreview.go

+
+	// Start clock on metrics
+	tokenReviewDuration := time.Now()
+	req, remoteAddr := proxycontext.RemoteAddr(req)


Just to confirm.. do we want to be overwriting req here to include this remote address?

SimplySeth · 2020-08-26T13:57:40Z

Pardon my ignorance, is this complete ?
How do I get access to the metrics if it is?
Thanks.

jetstack-bot assigned munnerz and simonswine Jun 18, 2020

jetstack-bot added the dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. label Jun 18, 2020

jetstack-bot requested a review from simonswine June 18, 2020 23:35

jetstack-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 18, 2020

james-w reviewed Jun 29, 2020

View reviewed changes

JoshVanL added 11 commits June 29, 2020 16:42

Adds metrics package to expose proxy metrics

5c2aa3c

Signed-off-by: JoshVanL <[email protected]>

Adds metrics to tokenreview

5661a3e

Signed-off-by: JoshVanL <[email protected]>

Adds proxy to main proxy and it's handlers

92a2c19

Signed-off-by: JoshVanL <[email protected]>

Fix probe to allow proper shutdowns

25742fb

Signed-off-by: JoshVanL <[email protected]>

Expose metrics listen address

5fd47bd

Signed-off-by: JoshVanL <[email protected]>

Adds Prometheus go vendor to mod

ed444df

Signed-off-by: JoshVanL <[email protected]>

Adds metrics task doc

8a9acc5

Signed-off-by: JoshVanL <[email protected]>

Adds metrics deployment options to yaml + helm chart

539a941

Signed-off-by: JoshVanL <[email protected]>

Cleans up status code variables in handlers and use exponential metrics

da5e757

buckets for request duration Signed-off-by: JoshVanL <[email protected]>

Fixes some spelling and grammar in docs and flags

38d2ace

Signed-off-by: JoshVanL <[email protected]>

Adds the Prometheus scrape annotations if enabled on chart

cf0add4

Signed-off-by: JoshVanL <[email protected]>

JoshVanL force-pushed the metrics branch from e385cd9 to cf0add4 Compare June 29, 2020 16:41

jetstack-bot assigned james-w Jun 29, 2020

JoshVanL added 2 commits June 29, 2020 17:53

Fix metrics exponential output

f20288d

Signed-off-by: JoshVanL <[email protected]>

Fix metrics exponential buckets

94f3579

Signed-off-by: JoshVanL <[email protected]>

munnerz reviewed Jul 3, 2020

View reviewed changes

SimplySeth unassigned munnerz and simonswine Aug 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metrics #158

Add metrics #158

JoshVanL commented Jun 18, 2020

james-w Jun 29, 2020

james-w Jun 29, 2020

james-w Jun 29, 2020

james-w Jun 29, 2020

james-w Jun 29, 2020

james-w Jun 29, 2020

james-w Jun 29, 2020

james-w Jun 29, 2020

james-w Jun 29, 2020

JoshVanL Jun 29, 2020

james-w Jun 30, 2020

JoshVanL Jun 30, 2020

james-w Jun 29, 2020

JoshVanL Jun 29, 2020

james-w Jun 30, 2020

jetstack-bot commented Jun 29, 2020

JoshVanL commented Jun 29, 2020

munnerz Jul 3, 2020

munnerz Jul 3, 2020

munnerz Jul 3, 2020

munnerz Jul 3, 2020

munnerz Jul 3, 2020

munnerz Jul 3, 2020

munnerz Jul 3, 2020

munnerz Jul 3, 2020

munnerz Jul 3, 2020

munnerz Jul 3, 2020

munnerz Jul 3, 2020

munnerz Jul 3, 2020

SimplySeth commented Aug 26, 2020

	The number of requests for incoming requests.
	The number of incoming requests.

	The requests for outgoing server requests.
	The number of outgoing server requests.

		"Adress to serving metrics on at the /metrics path. An empty address will "+
		"disable serving metrics.")

	klog.Info("metrics listen address empty, disabling serving")
	klog.Info("metrics listen address empty, disabling serving metrics")

	http.Error(rw, "Username claim not available in OIDC Issuer response", http.StatusForbidden)
	http.Error(rw, "Username claim not available in OIDC Issuer response", statusCode)

	http.Error(rw, "", http.StatusInternalServerError)
	http.Error(rw, "", statusCode)

		@@ -0,0 +1,218 @@
		// Copyright Jetstack Ltd. See LICENSE for details.
		package metrics

Add metrics #158

Are you sure you want to change the base?

Add metrics #158

Conversation

JoshVanL commented Jun 18, 2020

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

jetstack-bot commented Jun 29, 2020

JoshVanL commented Jun 29, 2020

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

SimplySeth commented Aug 26, 2020