From 6ba7171f19ab7218deac0958a4fe51a27659a956 Mon Sep 17 00:00:00 2001 From: Stanislav Laznicka Date: Mon, 7 Oct 2019 16:09:26 +0200 Subject: [PATCH 1/2] re-use openshift http client to prevent high memory usage Creating a new http.client object with every request, re-reading the CA files, can cause the oauth-proxy container to eventually get killed with OOM errors as the clients are not actually getting released anywhere. Re-use the clients based on checking whether their CA files actually changed or not - the check for change is based on last-modified timestamp and size metadata of each CA cert file. --- providers/openshift/provider.go | 23 ++++++- providers/openshift/provider_test.go | 89 +++++++++++++++++++++++++++- util/util.go | 26 ++++++++ 3 files changed, 134 insertions(+), 4 deletions(-) diff --git a/providers/openshift/provider.go b/providers/openshift/provider.go index a73bcced1..469a04d03 100644 --- a/providers/openshift/provider.go +++ b/providers/openshift/provider.go @@ -14,6 +14,7 @@ import ( "os" "sort" "strings" + "sync" "time" "github.com/bitly/go-simplejson" @@ -29,6 +30,13 @@ import ( authorizationv1beta1 "k8s.io/client-go/pkg/apis/authorization/v1beta1" ) +// httpClientCache stores httpClient objects so that new client does not have to +// be created on each request just to prevent CAs content changed +// key is bytes of the hashed metadata of the files for CAs the client was +// created with +// NOTE: the entries of the map are currently not being cleaned up +var httpClientCache sync.Map + func emptyURL(u *url.URL) bool { return u == nil || u.String() == "" } @@ -131,18 +139,29 @@ func (p *OpenShiftProvider) newOpenShiftClient() (*http.Client, error) { capaths = paths system_roots = false } + + // try to retrieve a cached client + metadataHash, err := util.GetFilesMetadataHash(capaths) + if httpClient, ok := httpClientCache.Load(metadataHash); ok { + return httpClient.(*http.Client), nil + } + + // client not in cache, create new pool, err := util.GetCertPool(capaths, system_roots) if err != nil { return nil, err } - return &http.Client{ + httpClient := &http.Client{ Jar: http.DefaultClient.Jar, Transport: &http.Transport{ Proxy: http.ProxyFromEnvironment, TLSClientConfig: oscrypto.SecureTLSConfig(&tls.Config{RootCAs: pool}), }, - }, nil + } + httpClientCache.Store(metadataHash, httpClient) + + return httpClient, nil } // encodeSARWithScope adds a "scopes" array to the SAR if it does not have one already, and outputs diff --git a/providers/openshift/provider_test.go b/providers/openshift/provider_test.go index 9f9ae73d2..2eadbaf00 100644 --- a/providers/openshift/provider_test.go +++ b/providers/openshift/provider_test.go @@ -1,11 +1,14 @@ package openshift import ( - "k8s.io/apiserver/pkg/authentication/user" - "k8s.io/apiserver/pkg/authorization/authorizer" + "io/ioutil" "net/http" + "os" "reflect" "testing" + + "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/apiserver/pkg/authorization/authorizer" ) type mockAuthRequestHandler struct { @@ -14,6 +17,42 @@ type mockAuthRequestHandler struct { type mockAuthorizer struct { } +// if you're seeing cert expiration errors on 'Nov 3 11:57:34 2119 GMT', I am sorry +const longLivedCACert = ` +-----BEGIN CERTIFICATE----- +MIIFjjCCA3agAwIBAgIUYICrP1shKbhgEbQsmHdf64W7hGwwDQYJKoZIhvcNAQEN +BQAwTzELMAkGA1UEBhMCQ1oxEDAOBgNVBAgMB01vcmF2aWExHDAaBgNVBAoME015 +IFByaXZhdGUgT3JnIEx0ZC4xEDAOBgNVBAMMB1Rlc3QgQ0EwIBcNMTkxMDA4MTE1 +NzMzWhgPMjExOTExMDMxMTU3MzNaME8xCzAJBgNVBAYTAkNaMRAwDgYDVQQIDAdN +b3JhdmlhMRwwGgYDVQQKDBNNeSBQcml2YXRlIE9yZyBMdGQuMRAwDgYDVQQDDAdU +ZXN0IENBMIICIjANBgkqhkiG9w0BAQEFAAOCAg8AMIICCgKCAgEAqGrcAHxo2Iiu +jNABMdasP0lHRiV3m6DGmDFGEWI9A5s4hSL+2Nh9Hnu1bmCqmm88EB8wQBxgte08 +hhxtamFHhqTvsr2zvZIinPI+ntgHuKWH2fKVNmHUA0/DfA51yPppRZXws2J2OhwG +VBfmztV6StSWP5HuCbujGnuMG37+CEiOqqR8nfvwtXhebEYCEGcRJmPQLWZuhohh +7Ie/M6auSQS29Xnezy/6To1V7kMuBwKq+ywTftfNiWRTRRAtx5+cd5EeZf8svO5z +WSYWQK+OzyjqCTwYDmm5WhHid112jsjhNMHVM8mL9za4E7zgZBYBRSkKiM5UVWTs +Lb6kO3FkIlQzqt9eSYzZfcQxUfuSOKviubtNghGI2TmoElcbgIIZ0zVBxa5k4DMY +Hr36B+PggXPbzF+pxAMpmR0qYKth6mGW6SJZTXdjwEbFSRE+zrpcttCGJgQsseTl +hV2BCyVq8aDvmMKh63sGAkalK1TmqNRplFuohSFW523Ilm2I93EF0/L4pRQ7+KZ3 +8+tFvrv1XswX0wWMNnsrUVIkvmsX2olZgvlN/taqovgTvC0zcO7EopDDveXMMLRY +C3wPP222sJ5wOGpT+m8HmddNaVWuW/9MzOgAEr4kuFlQUcvGdP/Z3IUgp8cVrjM7 +g6wVyVvguWE0a2q8xLw6Y3CKp5bLHh8CAwEAAaNgMF4wHQYDVR0OBBYEFNb1bu9A +OeRUWyN15uG/aIBtIgyTMB8GA1UdIwQYMBaAFNb1bu9AOeRUWyN15uG/aIBtIgyT +MA8GA1UdEwEB/wQFMAMBAf8wCwYDVR0PBAQDAgEGMA0GCSqGSIb3DQEBDQUAA4IC +AQACR2hSEMqlkFZ7RX/csZgpMt4E5z0TJZ7Uny+yKV/ibIFcy7sfU2bXnZX63Sdl +do89DkVTqI4T48byvF8KQ+pHr4ow5nvA2rigmQEySrSBT9GseZm9XIFy/Sb4vUml +dXYcmeJYNVgGAOspwrFg8mJ8a+afkBArSJyNLIemv+P2Bb4fChUhpoVt3XngJJJZ +5SxvF9g++0ZaDEse80wHCaHlgeh48Yo0SczNHv5lJ5uQzNIjxBEad/4P02Uj7wXf +J8TX3NK15P+Iwvf+UY8odtjIsLMd2KltaJ7P4MqTAS+b7Xb9i0CZgEtnCG3Fup8b +xM5S9S55qLUNUQtolNs2jxSnMGOciG3G/sdcl/qbiQZchvKvYZp8Q8NnavBIcRkQ +mZ0P2BPrg6rfofaNvOpTz+NeaWDFfQzC7+2QnfiiIOL8le7b4lOjmLyCfZaNW8WN +PlYMGYA460xdn/IWPJcLCdt3rNw+CKZCw4pxZvUWqzRnCrNkM4zA7JgLn7M9Vx1l +3q4sUFMZuUjWIxACwk9u/U4sc2rLYelwHhg/2j0hUoqbDhyHRYUVruptwRSebE2U +KvcuxUCTIws0kHzgUX6qT6gDFKDl9A+EgIcusosjUNIjLUsgUPs6THNvQadMEEV7 +w9aR8p+EwE+/BERIzwURZmyINWafvMjVGNHCKC1w7AhFEA== +-----END CERTIFICATE----- +` + func TestParseSubjectAccessReviews(t *testing.T) { tests := []struct { @@ -93,3 +132,49 @@ func TestDontPassBasicAuthentication(t *testing.T) { t.Errorf("access token should be empty string for basic authentication: %v", session) } } + +func TestNewOpenShiftClient(t *testing.T) { + tmpfile, err := ioutil.TempFile("", "osclienttest-") + if err != nil { + t.Fatalf("failed to create tempfile: %v", err) + } + defer os.Remove(tmpfile.Name()) + + _, err = tmpfile.WriteString(longLivedCACert) + if err != nil { + t.Fatalf("failed to write CA cert to tmpfile: %v", err) + } + + p := &OpenShiftProvider{} + p.paths = recordsByPath{pathRecord{"/someurl", authorizer.AttributesRecord{}}} + p.authenticator = &mockAuthRequestHandler{} + p.authorizer = &mockAuthorizer{} + p.SetReviewCAs([]string{tmpfile.Name()}) + + client, err := p.newOpenShiftClient() + if err != nil { + t.Fatalf("failed to create an OpenShift Client") + } + + newClient, err := p.newOpenShiftClient() + if err != nil { + t.Fatalf("failed to create a new OpenShift Client") + } + + // caching should make sure the clients are the same + if client != newClient { + t.Errorf("repeated call of newOpenShiftClient() returned different client pointers") + } + + // useless change but should change the metadata enough to get us a new client + tmpfile.WriteString("\n") + + newClient, err = p.newOpenShiftClient() + if err != nil { + t.Fatalf("failed to create a new OpenShift Client") + } + + if client == newClient { + t.Errorf("repeated call of newOpenShiftClient() after one of the CA changed returned the same client pointer") + } +} diff --git a/util/util.go b/util/util.go index 093c2c3fb..209696a8a 100644 --- a/util/util.go +++ b/util/util.go @@ -1,10 +1,16 @@ package util import ( + "crypto/sha256" "crypto/x509" + "encoding/base64" "fmt" "io/ioutil" "log" + "os" + "sort" + "strconv" + "strings" ) func GetCertPool(paths []string, system_roots bool) (*x509.CertPool, error) { @@ -33,3 +39,23 @@ func GetCertPool(paths []string, system_roots bool) (*x509.CertPool, error) { } return pool, nil } + +// GetFilesMetadataHash returns base64-encoded hash of (size + name + modtime) of the file at path +func GetFilesMetadataHash(paths []string) (string, error) { + hashData := make([]string, 0, len(paths)) + + for _, path := range paths { + meta, err := os.Stat(path) + if err != nil { + return "", fmt.Errorf("couldn't stat '%s': %v", path, err) + } + + hashData = append(hashData, meta.Name()+strconv.FormatInt(meta.Size(), 10)+meta.ModTime().String()) + } + + // sort the strings so that the same paths in different order still generate the same hash + sort.Strings(hashData) + + h := sha256.Sum256([]byte(strings.Join(hashData, ""))) + return base64.StdEncoding.EncodeToString(h[:]), nil +} From 03aa0c993b91adfc9ef736119c59f52464244686 Mon Sep 17 00:00:00 2001 From: Stanislav Laznicka Date: Mon, 7 Oct 2019 16:13:51 +0200 Subject: [PATCH 2/2] fix util package unit tests --- util/util_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/util/util_test.go b/util/util_test.go index 933808547..746ab6fee 100644 --- a/util/util_test.go +++ b/util/util_test.go @@ -1,18 +1,18 @@ package util import ( - "testing" - "os" + "encoding/hex" "io/ioutil" + "os" "reflect" - "encoding/hex" + "testing" "github.com/bmizerany/assert" ) var ( testCA1Subj = "301e311c301a060355040313136f617574682d70726f78792074657374206361" - testCA1 = `-----BEGIN CERTIFICATE----- + testCA1 = `-----BEGIN CERTIFICATE----- MIICuTCCAaGgAwIBAgIFAKuKEWowDQYJKoZIhvcNAQELBQAwHjEcMBoGA1UEAxMT b2F1dGgtcHJveHkgdGVzdCBjYTAeFw0xNzEwMjQyMDExMzJaFw0xOTEwMjQyMDEx MzJaMB4xHDAaBgNVBAMTE29hdXRoLXByb3h5IHRlc3QgY2EwggEiMA0GCSqGSIb3 @@ -31,7 +31,7 @@ pP5YlVqdRCVrxgT80PIMsvQhfcuIrbbeiRDEUdEX7FqebuGCEa2757MTdW7UYQiB -----END CERTIFICATE----- ` testCA2Subj = "3025312330210603550403131a6f617574682d70726f7879207365636f6e642074657374206361" - testCA2 = `-----BEGIN CERTIFICATE----- + testCA2 = `-----BEGIN CERTIFICATE----- MIICxzCCAa+gAwIBAgIFAKuMKewwDQYJKoZIhvcNAQELBQAwJTEjMCEGA1UEAxMa b2F1dGgtcHJveHkgc2Vjb25kIHRlc3QgY2EwHhcNMTcxMDI1MTYxMTQxWhcNMTkx MDI1MTYxMTQxWjAlMSMwIQYDVQQDExpvYXV0aC1wcm94eSBzZWNvbmQgdGVzdCBj @@ -64,18 +64,18 @@ func makeTestCertFile(t *testing.T, pem, dir string) *os.File { } func TestGetCertPool(t *testing.T) { - _, err := GetCertPool([]string(nil)) + _, err := GetCertPool([]string(nil), false) if err == nil { t.Errorf("expected an error") } - assert.Equal(t, "Invalid empty list of Root CAs file paths" ,err.Error()) + assert.Equal(t, "Invalid empty list of Root CAs file paths", err.Error()) tempDir := os.TempDir() defer os.RemoveAll(tempDir) certFile1 := makeTestCertFile(t, testCA1, tempDir) certFile2 := makeTestCertFile(t, testCA2, tempDir) - certPool, err := GetCertPool([]string{certFile1.Name(), certFile2.Name()}) + certPool, err := GetCertPool([]string{certFile1.Name(), certFile2.Name()}, false) if err != nil { t.Errorf("unexpected error %v", err) }