Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Add TLS serving option for metrics #993

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 27 additions & 6 deletions pkg/manager/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,12 @@ type Options struct {
// It can be set to "0" to disable the metrics serving.
MetricsBindAddress string

// MetricsCertDir is the directory that contains the secure metrics server key and certificate.
// When is set, server would be served over TLS and look up the server key and certificate in
// {MetricsCertDir}/tls.key and {MetricsCertDir}/tls.crt. The server key and certificate
// must be named tls.key and tls.crt, respectively.
MetricsCertDir string

// HealthProbeBindAddress is the TCP address that the controller should bind to
// for serving health probes
HealthProbeBindAddress string
Expand Down Expand Up @@ -206,9 +212,11 @@ type Options struct {
EventBroadcaster record.EventBroadcaster

// Dependency injection for testing
newRecorderProvider func(config *rest.Config, scheme *runtime.Scheme, logger logr.Logger, broadcaster record.EventBroadcaster) (recorder.Provider, error)
newResourceLock func(config *rest.Config, recorderProvider recorder.Provider, options leaderelection.Options) (resourcelock.Interface, error)
newMetricsListener func(addr string) (net.Listener, error)
newRecorderProvider func(config *rest.Config, scheme *runtime.Scheme, logger logr.Logger, broadcaster record.EventBroadcaster) (recorder.Provider, error)
newResourceLock func(config *rest.Config, recorderProvider recorder.Provider, options leaderelection.Options) (resourcelock.Interface, error)
newMetricsListener func(addr string) (net.Listener, error)
newSecureMetricsListener func(addr, certDir string) (net.Listener, error)

newHealthProbeListener func(addr string) (net.Listener, error)
}

Expand Down Expand Up @@ -299,9 +307,17 @@ func New(config *rest.Config, options Options) (Manager, error) {

// Create the metrics listener. This will throw an error if the metrics bind
// address is invalid or already in use.
metricsListener, err := options.newMetricsListener(options.MetricsBindAddress)
if err != nil {
return nil, err
var metricsListener net.Listener
if options.MetricsCertDir == "" {
metricsListener, err = options.newMetricsListener(options.MetricsBindAddress)
if err != nil {
return nil, err
}
} else {
metricsListener, err = options.newSecureMetricsListener(options.MetricsBindAddress, options.MetricsCertDir)
if err != nil {
return nil, err
}
}

// By default we have no extra endpoints to expose on metrics http server.
Expand Down Expand Up @@ -410,6 +426,11 @@ func setOptionsDefaults(options Options) Options {
if options.newMetricsListener == nil {
options.newMetricsListener = metrics.NewListener
}

if options.newSecureMetricsListener == nil {
options.newSecureMetricsListener = metrics.NewSecureListener
}

leaseDuration, renewDeadline, retryPeriod := defaultLeaseDuration, defaultRenewDeadline, defaultRetryPeriod
if options.LeaseDuration == nil {
options.LeaseDuration = &leaseDuration
Expand Down
90 changes: 90 additions & 0 deletions pkg/manager/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@ limitations under the License.
package manager

import (
"crypto/tls"
"fmt"
"io/ioutil"
"net"
"net/http"
"path/filepath"
rt "runtime"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -233,6 +235,24 @@ var _ = Describe("manger.Manager", func() {
Expect(listener.Close()).ToNot(HaveOccurred())
})

It("should create a TLS listener for the metrics if certificates are provided and the address is correct", func() {
var listener net.Listener
certDir := filepath.Join(".", "testdata", "certs")
m, err := New(cfg, Options{
MetricsBindAddress: ":0",
MetricsCertDir: certDir,
newSecureMetricsListener: func(addr, certDir string) (net.Listener, error) {
var err error
listener, err = metrics.NewSecureListener(addr, certDir)
return listener, err
},
})
Expect(m).ToNot(BeNil())
Expect(err).ToNot(HaveOccurred())
Expect(listener).ToNot(BeNil())
Expect(listener.Close()).ToNot(HaveOccurred())
})

It("should return an error if the metrics bind address is already in use", func() {
ln, err := metrics.NewListener(":0")
Expect(err).ShouldNot(HaveOccurred())
Expand Down Expand Up @@ -559,6 +579,76 @@ var _ = Describe("manger.Manager", func() {
})
})

Context("should start serving metrics over TLS", func() {
var listener net.Listener
var opts Options

BeforeEach(func() {
listener = nil
opts = Options{
MetricsCertDir: filepath.Join(".", "testdata", "certs"),
newSecureMetricsListener: func(addr, certDir string) (net.Listener, error) {
var err error
listener, err = metrics.NewSecureListener(addr, certDir)
return listener, err
},
}
})

AfterEach(func() {
if listener != nil {
listener.Close()
}
})

It("should serve secure metrics endpoint", func(done Done) {
opts.MetricsBindAddress = ":0"
m, err := New(cfg, opts)
Expect(err).NotTo(HaveOccurred())

s := make(chan struct{})
defer close(s)
go func() {
defer GinkgoRecover()
Expect(m.Start(s)).NotTo(HaveOccurred())
close(done)
}()

metricsEndpoint := fmt.Sprintf("https://%s/metrics", listener.Addr().String())
tr := &http.Transport{
TLSClientConfig: &tls.Config{
InsecureSkipVerify: true,
},
}
client := &http.Client{Transport: tr}
resp, err := client.Get(metricsEndpoint)
Expect(err).NotTo(HaveOccurred())
Expect(resp.StatusCode).To(Equal(200))
})

It("should not serve both endpoints", func(done Done) {
var insecureListener net.Listener
opts.MetricsBindAddress = ":0"
opts.newMetricsListener = func(addr string) (net.Listener, error) {
var err error
insecureListener, err = metrics.NewListener(addr)
return listener, err
}

m, err := New(cfg, opts)
Expect(err).NotTo(HaveOccurred())

s := make(chan struct{})
defer close(s)
go func() {
defer GinkgoRecover()
Expect(m.Start(s)).NotTo(HaveOccurred())
close(done)
}()
Expect(insecureListener).To(BeNil())
})
})

Context("should start serving health probes", func() {
var listener net.Listener
var opts Options
Expand Down
18 changes: 18 additions & 0 deletions pkg/manager/testdata/certs/tls.crt
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
-----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-----
27 changes: 27 additions & 0 deletions pkg/manager/testdata/certs/tls.key
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
-----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-----
46 changes: 43 additions & 3 deletions pkg/metrics/listener.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,13 @@ limitations under the License.
package metrics

import (
"context"
"crypto/tls"
"fmt"
"net"
"path/filepath"

"sigs.k8s.io/controller-runtime/pkg/internal/certwatcher"
logf "sigs.k8s.io/controller-runtime/pkg/internal/log"
)

Expand All @@ -29,6 +33,12 @@ var log = logf.RuntimeLog.WithName("metrics")
// The metrics is on by default.
var DefaultBindAddress = ":8080"

// DefaultKeyPath is prefix path for TLS key
var DefaultKeyPath = "tls.key"

// DefualtCertPath is prefix path for TLS certificate
var DefualtCertPath = "tls.crt"

// NewListener creates a new TCP listener bound to the given address.
func NewListener(addr string) (net.Listener, error) {
if addr == "" {
Expand All @@ -44,9 +54,39 @@ func NewListener(addr string) (net.Listener, error) {
log.Info("metrics server is starting to listen", "addr", addr)
ln, err := net.Listen("tcp", addr)
if err != nil {
er := fmt.Errorf("error listening on %s: %w", addr, err)
log.Error(er, "metrics server failed to listen. You may want to disable the metrics server or use another port if it is due to conflicts")
return nil, er
err = fmt.Errorf("error listening on %s: %w", addr, err)
log.Error(err, "metrics server failed to listen. You may want to disable the metrics server or use another port if it is due to conflicts")
return nil, err
}

return ln, nil
}

// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error is unchecked

Suggested change
ln, err := NewListener(addr)
ln, err := NewListener(addr)
if err != nil {
return nil, err
}

if err != nil {
return nil, err
}

certPath := filepath.Join(certDir, DefualtCertPath)
certKey := filepath.Join(certDir, DefaultKeyPath)
certWatcher, err := certwatcher.New(certPath, certKey)
if err != nil {
return nil, err
}

go func() {
if err := certWatcher.Start(context.Background().Done()); err != nil {
log.Error(err, "certificate watcher error")
return
}
}()

cfg := &tls.Config{
NextProtos: []string{"h2"},
GetCertificate: certWatcher.GetCertificate,
}

return tls.NewListener(ln, cfg), nil
}
3 changes: 2 additions & 1 deletion pkg/webhook/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,9 @@ import (

"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promhttp"

"sigs.k8s.io/controller-runtime/pkg/internal/certwatcher"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
"sigs.k8s.io/controller-runtime/pkg/webhook/internal/certwatcher"
"sigs.k8s.io/controller-runtime/pkg/webhook/internal/metrics"
)

Expand Down