✨ Add TLS serving option for metrics #993

Danil-Grigorev · 2020-06-11T12:32:53Z

This PR introduces new option to the manager to pass a TLS certificate and key in a separate directory, which will be used to serve metrics endpoint over https://

MetricsCertDir is a location where the certificate files would be located. Those should be named tls.crt and tls.key. Non empty value in the variable enables secure metrics endpoint.

Fixes #979

k8s-ci-robot · 2020-06-11T12:33:01Z

Welcome @Danil-Grigorev!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-06-11T12:33:01Z

Hi @Danil-Grigorev. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2020-06-11T12:33:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Danil-Grigorev
To complete the pull request process, please assign joelanford
You can assign the PR to them by writing /assign @joelanford in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

Danil-Grigorev · 2020-06-11T12:47:17Z

/ok-to-test

k8s-ci-robot · 2020-06-11T12:47:30Z

@Danil-Grigorev: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

JoelSpeed · 2020-06-11T13:36:05Z

/ok-to-test

JoelSpeed · 2020-06-11T13:37:35Z

pkg/manager/manager_suite_test.go

+const testCrt = `-----BEGIN CERTIFICATE-----
+MIIC8zCCAdugAwIBAgIJAOBRXEjnBd8DMA0GCSqGSIb3DQEBBQUAMCQxIjAgBgNV
+BAMTGWh0dHBzbG9jYWxob3N0NDQ0M21ldHJpY3MwHhcNMjAwNjExMTEzMDI3WhcN
+MzAwNjA5MTEzMDI3WjAkMSIwIAYDVQQDExlodHRwc2xvY2FsaG9zdDQ0NDNtZXRy
+aWNzMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAxY6bVSLkfdHlI9kl
+4nAYTXCEdqEjVyrBQpwNeOkQ+E2hX2T28CBUPgWcHfD2wD85rVRh26Cs1ACifpIS
+SJI5nSswBqkA+whzV1uIofcjbxGFlVBfeOEQnbuXpHb60ZzgCVH5/9XIlw5NiR7V
+wfDzm9ZlNZzhCI8dGYFObop3fUfunmublUUyrmkxxdyFAC09w2NYijha+kz0FkGF
+Ao/4Ppqa4lFBYTR41wh5XM3cZFPUTrAw4W3gc64KQIuwXNeUxYYgBA/Xpoz5EQey
+jH/OVKFNKM6IKZNxyx5f9LQ9Lgzl0Vt7jjvjaGsQQ+4c9Ea6eBA9NlaKyzxyILlV
+vAnw1QIDAQABoygwJjAkBgNVHREEHTAbghlodHRwc2xvY2FsaG9zdDQ0NDNtZXRy
+aWNzMA0GCSqGSIb3DQEBBQUAA4IBAQBJBGV7xTrxFGyPNWautrTGo65/fCv7ddra
+1Kkh0Hlf/m9ZXRBkDW7SVlNYV+Uag/jVyT/OfYTe6TO1rxI/0L8gQhXPZ9n59cH
+O8mfT+/bk1CzaUV0pKpf4fO87twLXgp0VSRPkCE3PG3xzhGrbaB36dS8Ploz9nWE
+mG3NAsVc4jXYoVKZBmFCwew5h9IobqXACdWX9ik2eMS5oAOp+U7L1RZQjlXFWxEO
+46TgvNxQP6IPQZlR4ed4j7viY/KSm2FXKKNVjtXLBlGKfdPuKEtCL1axrFWumFi2
+CcOjeGbwwUn602Pd3lNj/GwaKdzC40n9/r7JalgVs3Jc+oiGfZJ9
+-----END CERTIFICATE-----
+`
+
+const testKey = `-----BEGIN RSA PRIVATE KEY-----
+MIIEpQIBAAKCAQEAxY6bVSLkfdHlI9kl4nAYTXCEdqEjVyrBQpwNeOkQ+E2hX2T2
+8CBUPgWcHfD2wD85rVRh26Cs1ACifpISSJI5nSswBqkA+whzV1uIofcjbxGFlVBf
+eOEQnbuXpHb60ZzgCVH5/9XIlw5NiR7VwfDzm9ZlNZzhCI8dGYFObop3fUfunmub
+lUUyrmkxxdyFAC09w2NYijha+kz0FkGFAo/4Ppqa4lFBYTR41wh5XM3cZFPUTrAw
+4W3gc64KQIuwXNeUxYYgBA/Xpoz5EQeyjH/OVKFNKM6IKZNxyx5f9LQ9Lgzl0Vt7
+jjvjaGsQQ+4c9Ea6eBA9NlaKyzxyILlVvAnw1QIDAQABAoIBADFaMsvN767O5KNT
+9/bdcfTGixDnqGB6OdVeDq+J6cdd/VZLbrUGHoVv+VQxgjL8mHgIgHnRZduAXRep
+fg/LF8F/rHu9dJVBwy6rmzJ6/sscYXavoWodL314A6X+YyJCQmWRqRaUXYv+8rey
+kEvm2bSwlpASJNVyix54AxPyW29cPHt/8kWgyrVyr2U+hQwPHntTYrmioRMlSX5v
+qFGt4s4/oo+aXA689Q14Qu1RWldxLog4581VAIjg47FwGRJ1PYyC0ywkgI+b7Kl5
+7YXrBAp8f6xrtpUd4SULbWQ/CKoSembhhZ8eRxfqMXH+Qz4xCgSw5DAaXQoiehkn
+itrsuoECgYEA6XRtZ+tY8dGvypbB+BUdz58QgM9sFKXf5W2DpLbKGFfyaqFTouAs
+mI4YIWrS53CWm1PiG8v3O7duAgtzpr584QjAkw7t3zOqVdtnoE4orxvM6hRhCOV5
+9Wi9axE5iG5Pp66SNlbOvypX8OJUbdi8cFZos4xr54VyXG0gSy3PK30CgYEA2KKz
+Hn+oErsdhpVGC+iOS8QZVL4nfJeAL7AKo6sO66hSvP5tdNTTh06MI5HXz7mUOWVE
+SyampkUD+qk+ptGbD8cF+k3D4/6a5KxL9nnAGxIK9KwMYQsc8/0brieFwy4FWcKH
+huEtX3Bnab7MU6iTmwnnciGG2/5uo5+uvpYd6jkCgYEAh/J4049FmGxXRk5MXj9N
+wN4MKjaf5dZCb8Q6aOzY+xwb2uRfY/XPgnccrjka4BO8YG+UuEMqkefbc+1fR7ad
+2h3SptCGzPe1NZIy4jMhlfdGePmtGBUp1DNOOs8pBb3XPPp3wpUCiGgMFgZ2zBDu
+iyyGhCg9nfEkC5awu5bNkbECgYEAyzcMOW7clf2Ku+WpWKBlYzNn47OgzOI9L/6+
+bDuZenxiaMFuoerHJqULFo7H2CcooRKalriCGXSiP++lQs1a3NkAhYWPXX9Hg30Q
+oPwitgId3tjJn/rRxRrIbXzLoIS6JjIx+defPWjuySZe+5cmJ4iJ4OkMXa/1z22K
+eWPOWhkCgYEA19jUW9NweDMWlkYm92SNzvZIv8brPrWGNthTxUUflaA9NQRe++fZ
+RR1wT84WmV94lDOC4AKHtCQXx4o9m4GVwFcNIzdtexxbdBYPY9FPpZlR/S/VC9Ef
+73rEV46bie8TMDd0CmHBSpiDHDqgPyNwaYpttuoVFGNxw59/8OdeJQM=
+-----END RSA PRIVATE KEY-----
+`
+


Perhaps we could generate a new key and certificate on the fly rather than storing one? I don't know how complex that would be, but I suspect this will flag up those tools that scan repositories for secrets as it is know

Moved it to testdata directory, should be little more sane. Can't really see a good solution to generate it on the go, and judging from the examples, it should be ok: https://golang.org/src/crypto/tls/example_test.go

pkg/metrics/listener.go

pkg/manager/manager.go

pkg/manager/manager_test.go

djzager

One super nitpick. Other than that this looks good to me.

djzager · 2020-06-15T13:39:37Z

pkg/webhook/server.go

@@ -31,8 +31,8 @@ import (

 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
+	"sigs.k8s.io/controller-runtime/pkg/internal/certwatcher"


I believe that imports are intentionally grouped together such that all sigs.k8s.io/controller-runtime would be separate.

JoelSpeed

Had another look, added a few more minor things

JoelSpeed · 2020-06-16T08:58:59Z

pkg/manager/manager_test.go

@@ -233,6 +235,24 @@ var _ = Describe("manger.Manager", func() {
 			Expect(listener.Close()).ToNot(HaveOccurred())
 		})

+		It("should create a TLS listener for the metrics if a valid address is provided", func() {


Perhaps rename this one, as it creates a normal listener if a valid address is provided, we need certs to create the TLS listener, WDYT?

JoelSpeed · 2020-06-16T09:04:31Z

pkg/metrics/listener.go

+
+// NewSecureListener creates a new TCP listener over TLS bound to the given address.
+func NewSecureListener(addr, certDir string) (net.Listener, error) {
+	ln, err := NewListener(addr)


This error is unchecked

Suggested change

ln, err := NewListener(addr)

ln, err := NewListener(addr)

if err != nil {

return nil, err

}

JoelSpeed · 2020-06-16T09:06:28Z

pkg/metrics/listener.go

+	}
+
+	go func() {
+		if err = certWatcher.Start(context.Background().Done()); err != nil {


I'm normally not a fan of shadowing, but in this situation I'd be tempted to use a different variable since it's within a go routine, WDYT?

Suggested change

if err = certWatcher.Start(context.Background().Done()); err != nil {

if err := certWatcher.Start(context.Background().Done()); err != nil {

JoelSpeed

/lgtm

JoelSpeed · 2020-06-16T09:38:56Z

pkg/manager/manager_test.go

@@ -233,6 +235,24 @@ var _ = Describe("manger.Manager", func() {
 			Expect(listener.Close()).ToNot(HaveOccurred())
 		})

+		It("should create a TLS listener for the metrics if a certificates are provided and the address is correct", func() {


Nit

Suggested change

It("should create a TLS listener for the metrics if a certificates are provided and the address is correct", func() {

It("should create a TLS listener for the metrics if certificates are provided and the address is correct", func() {

JoelSpeed

/lgtm

djzager

/lgtm

joelanford · 2020-06-16T20:32:20Z

@JoelSpeed @Danil-Grigorev Instead of forcing callers into always having to get certs on disk in their image (albeit not that hard), WDYT of instead exposing a MetricsTLSConfig *tls.Config field to the manager Options struct?

The default nil value would result in the current behavior, but if it was non-nil, we would create a listener using tls.NewListener.

joelanford · 2020-06-16T20:32:53Z

/hold

For above discussion.

DirectXMan12 · 2020-06-16T22:32:40Z

Hey, so we generally wanted to sidestep this by using kube-rbac-proxy. I'm open to rediscussion, but the historical decision we made was largely to just let sidecar containers (rbac proxy, service mesh, etc) handle that.

Danil-Grigorev · 2020-06-19T10:00:54Z

Closing in favour of #1007 to be exposed for the user modification. This PR introduces sample implementation for that on https://github.com/kubernetes-sigs/controller-runtime/pull/993/files#diff-652ee3c14d4f011345992fadfb31853eR66-R92

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 11, 2020

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 11, 2020

k8s-ci-robot requested review from alexeldeib and droot June 11, 2020 12:33

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 11, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 11, 2020

JoelSpeed reviewed Jun 11, 2020

View reviewed changes

Danil-Grigorev force-pushed the secure-metrics branch 3 times, most recently from e8b2eaf to be3e6dd Compare June 11, 2020 20:07

Danil-Grigorev requested a review from JoelSpeed June 11, 2020 20:19

djzager suggested changes Jun 15, 2020

View reviewed changes

Danil-Grigorev force-pushed the secure-metrics branch 2 times, most recently from e6c854e to 12f3eb3 Compare June 16, 2020 08:58

Danil-Grigorev requested a review from djzager June 16, 2020 08:58

JoelSpeed reviewed Jun 16, 2020

View reviewed changes

Danil-Grigorev force-pushed the secure-metrics branch 2 times, most recently from a0366e0 to 5c909a0 Compare June 16, 2020 09:24

Danil-Grigorev requested a review from JoelSpeed June 16, 2020 09:27

JoelSpeed reviewed Jun 16, 2020

View reviewed changes

k8s-ci-robot assigned JoelSpeed Jun 16, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 16, 2020

Add metrics TLS serving option

0a62376

Danil-Grigorev force-pushed the secure-metrics branch from 5c909a0 to 0a62376 Compare June 16, 2020 09:40

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 16, 2020

JoelSpeed reviewed Jun 16, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 16, 2020

djzager approved these changes Jun 16, 2020

View reviewed changes

k8s-ci-robot assigned djzager Jun 16, 2020

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 16, 2020

Danil-Grigorev closed this Jun 19, 2020

timebertt mentioned this pull request Oct 21, 2021

Support HTTPS for /healthz, /readyz, /metrics and /debug endpoints gardener/gardener#2829

Closed

varshaprasad96 mentioned this pull request Jun 27, 2022

expose metrics of the operator with tls (https) certificates operator-framework/operator-sdk#5899

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add TLS serving option for metrics #993

✨ Add TLS serving option for metrics #993

Danil-Grigorev commented Jun 11, 2020 •

edited

Loading

k8s-ci-robot commented Jun 11, 2020

k8s-ci-robot commented Jun 11, 2020

k8s-ci-robot commented Jun 11, 2020

Danil-Grigorev commented Jun 11, 2020

k8s-ci-robot commented Jun 11, 2020

JoelSpeed commented Jun 11, 2020

JoelSpeed Jun 11, 2020

Danil-Grigorev Jun 11, 2020

djzager left a comment

djzager Jun 15, 2020

JoelSpeed left a comment

JoelSpeed Jun 16, 2020

JoelSpeed Jun 16, 2020

JoelSpeed Jun 16, 2020

JoelSpeed left a comment

JoelSpeed Jun 16, 2020

JoelSpeed left a comment

djzager left a comment

joelanford commented Jun 16, 2020

joelanford commented Jun 16, 2020

DirectXMan12 commented Jun 16, 2020

Danil-Grigorev commented Jun 19, 2020

	if err = certWatcher.Start(context.Background().Done()); err != nil {
	if err := certWatcher.Start(context.Background().Done()); err != nil {

	It("should create a TLS listener for the metrics if a certificates are provided and the address is correct", func() {
	It("should create a TLS listener for the metrics if certificates are provided and the address is correct", func() {

✨ Add TLS serving option for metrics #993

✨ Add TLS serving option for metrics #993

Conversation

Danil-Grigorev commented Jun 11, 2020 • edited Loading

k8s-ci-robot commented Jun 11, 2020

k8s-ci-robot commented Jun 11, 2020

k8s-ci-robot commented Jun 11, 2020

Danil-Grigorev commented Jun 11, 2020

k8s-ci-robot commented Jun 11, 2020

JoelSpeed commented Jun 11, 2020

JoelSpeed Jun 11, 2020

Choose a reason for hiding this comment

Danil-Grigorev Jun 11, 2020

Choose a reason for hiding this comment

djzager left a comment

Choose a reason for hiding this comment

djzager Jun 15, 2020

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

JoelSpeed Jun 16, 2020

Choose a reason for hiding this comment

JoelSpeed Jun 16, 2020

Choose a reason for hiding this comment

JoelSpeed Jun 16, 2020

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

JoelSpeed Jun 16, 2020

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

djzager left a comment

Choose a reason for hiding this comment

joelanford commented Jun 16, 2020

joelanford commented Jun 16, 2020

DirectXMan12 commented Jun 16, 2020

Danil-Grigorev commented Jun 19, 2020

Danil-Grigorev commented Jun 11, 2020 •

edited

Loading