Merge pull request #139 from openshift-cherrypick-robot/cherry-pick-137-to-release-4.2

[release-4.2] Bug 1762748: Fix high memory usage

Author: openshift-merge-robot

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

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")
+	}
+}

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
+}

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)
 	}