From 86c9401492e9d49d48d45062d6f8543215d23de4 Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Wed, 15 Aug 2018 16:28:38 -0700 Subject: [PATCH] Do not truncate tls certs list based on tp limit The target proxy has a limit of 10, so we limit ingress certs to this value. Hardcoding limit means more changes in ingress when targetproxy limit is increased. --- pkg/loadbalancers/certificates.go | 6 ---- pkg/loadbalancers/fakes.go | 9 ++++-- pkg/loadbalancers/loadbalancers_test.go | 39 +++++++++++++++++++++---- pkg/tls/tls.go | 8 ----- 4 files changed, 40 insertions(+), 22 deletions(-) diff --git a/pkg/loadbalancers/certificates.go b/pkg/loadbalancers/certificates.go index 2ba2dd8682..1593b253a3 100644 --- a/pkg/loadbalancers/certificates.go +++ b/pkg/loadbalancers/certificates.go @@ -99,12 +99,6 @@ func (l *L7) usePreSharedCert() (bool, error) { return false, nil } preSharedCerts := strings.Split(preSharedCertName, ",") - if len(preSharedCerts) > TargetProxyCertLimit { - glog.Warningf("Specified %d preshared certs, limit is %d, rest will be ignored", - len(preSharedCerts), TargetProxyCertLimit) - preSharedCerts = preSharedCerts[:TargetProxyCertLimit] - } - l.sslCerts = make([]*compute.SslCertificate, 0, len(preSharedCerts)) var failedCerts []string diff --git a/pkg/loadbalancers/fakes.go b/pkg/loadbalancers/fakes.go index a406f33c1a..2d43d12d25 100644 --- a/pkg/loadbalancers/fakes.go +++ b/pkg/loadbalancers/fakes.go @@ -29,6 +29,8 @@ import ( "k8s.io/ingress-gce/pkg/utils" ) +const FakeCertLimit = 15 + var testIPManager = testIP{} type testIP struct { @@ -342,6 +344,9 @@ func (f *FakeLoadBalancers) SetSslCertificateForTargetHttpsProxy(proxy *compute. found := false for i := range f.Tps { if f.Tps[i].Name == proxy.Name { + if len(sslCertURLs) > TargetProxyCertLimit { + return utils.FakeGoogleAPIForbiddenErr() + } f.Tps[i].SslCertificates = sslCertURLs found = true break @@ -412,9 +417,9 @@ func (f *FakeLoadBalancers) ListSslCertificates() ([]*compute.SslCertificate, er func (f *FakeLoadBalancers) CreateSslCertificate(cert *compute.SslCertificate) (*compute.SslCertificate, error) { f.calls = append(f.calls, "CreateSslCertificate") cert.SelfLink = cloud.NewSslCertificatesResourceID("mock-project", cert.Name).SelfLink(meta.VersionGA) - if len(f.Certs) == TargetProxyCertLimit { + if len(f.Certs) == FakeCertLimit { // Simulate cert creation failure - return nil, fmt.Errorf("Unable to create cert, Exceeded cert limit of %d.", TargetProxyCertLimit) + return nil, fmt.Errorf("Unable to create cert, Exceeded cert limit of %d.", FakeCertLimit) } f.Certs = append(f.Certs, cert) return cert, nil diff --git a/pkg/loadbalancers/loadbalancers_test.go b/pkg/loadbalancers/loadbalancers_test.go index 32b2a468b5..0e624768c4 100644 --- a/pkg/loadbalancers/loadbalancers_test.go +++ b/pkg/loadbalancers/loadbalancers_test.go @@ -360,17 +360,19 @@ func TestUpgradeToNewCertNames(t *testing.T) { verifyCertAndProxyLink(expectCerts, expectCerts, f, t) } -// Tests uploading 10 certs which is the global limit today. Ensures that creation of the 11th cert fails. +// Tests uploading 15 certs which is the limit for the fake loadbalancer. Ensures that creation of the 16th cert fails. +// Tests uploading 10 certs which is the target proxy limit. Uploading 11th cert should fail. func TestMaxCertsUpload(t *testing.T) { gceUrlMap := utils.NewGCEURLMap() gceUrlMap.DefaultBackend = &utils.ServicePort{NodePort: 31234} gceUrlMap.PutPathRulesForHost("bar.example.com", []utils.PathRule{utils.PathRule{Path: "/bar", Backend: utils.ServicePort{NodePort: 30000}}}) var tlsCerts []*TLSCerts expectCerts := make(map[string]string) + expectCertsExtra := make(map[string]string) namer := utils.NewNamer("uid1", "fw1") lbName := namer.LoadBalancer("test") - for ix := 0; ix < TargetProxyCertLimit; ix++ { + for ix := 0; ix < FakeCertLimit; ix++ { str := strconv.Itoa(ix) tlsCerts = append(tlsCerts, createCert("key-"+str, "cert-"+str, "name-"+str)) certName := namer.SSLCertName(lbName, GetCertHash("cert-"+str)) @@ -392,7 +394,32 @@ func TestMaxCertsUpload(t *testing.T) { verifyCertAndProxyLink(expectCerts, expectCerts, f, t) failCert := createCert("key100", "cert100", "name100") lbInfo.TLS = append(lbInfo.TLS, failCert) - pool.Sync(lbInfo) + err := pool.Sync(lbInfo) + if err == nil { + t.Fatalf("Creating more than %d certs should have errored out", FakeCertLimit) + } + verifyCertAndProxyLink(expectCerts, expectCerts, f, t) + // Set cert count less than cert creation limit but more than target proxy limit + lbInfo.TLS = lbInfo.TLS[:TargetProxyCertLimit+1] + for _, cert := range lbInfo.TLS { + expectCertsExtra[namer.SSLCertName(lbName, cert.CertHash)] = cert.Cert + } + err = pool.Sync(lbInfo) + if err == nil { + t.Fatalf("Assigning more than %d certs should have errored out", TargetProxyCertLimit) + } + // load balancer will contain the extra cert, but target proxy will not + verifyCertAndProxyLink(expectCertsExtra, expectCerts, f, t) + // Removing the extra cert from ingress spec should delete it + lbInfo.TLS = lbInfo.TLS[:TargetProxyCertLimit] + err = pool.Sync(lbInfo) + if err != nil { + t.Fatalf("Unexpected error %s", err) + } + expectCerts = make(map[string]string) + for _, cert := range lbInfo.TLS { + expectCerts[namer.SSLCertName(lbName, cert.CertHash)] = cert.Cert + } verifyCertAndProxyLink(expectCerts, expectCerts, f, t) } @@ -618,9 +645,9 @@ func verifyCertAndProxyLink(expectCerts map[string]string, expectCertsProxy map[ } for _, link := range tps.SslCertificates { certName, _ := utils.KeyName(link) - if _, ok := expectCerts[certName]; !ok { - t.Fatalf("unexpected ssl certificate linked in target proxy; Expected : %v; Target Proxy Certs: %v", - expectCertsProxy, tps.SslCertificates) + if _, ok := expectCertsProxy[certName]; !ok { + t.Fatalf("unexpected ssl certificate '%s' linked in target proxy; Expected : %v; Target Proxy Certs: %v", + certName, expectCertsProxy, tps.SslCertificates) } } } diff --git a/pkg/tls/tls.go b/pkg/tls/tls.go index 8a34d7ca4d..26d76edfc0 100644 --- a/pkg/tls/tls.go +++ b/pkg/tls/tls.go @@ -58,11 +58,6 @@ func (t *TLSCertsFromSecretsLoader) Load(ing *extensions.Ingress) ([]*loadbalanc } var certs []*loadbalancers.TLSCerts - if len(ing.Spec.TLS) > loadbalancers.TargetProxyCertLimit { - glog.Warningf("Specified %d tls secrets, limit is %d, rest will be ignored", - len(ing.Spec.TLS), loadbalancers.TargetProxyCertLimit) - } - for _, tlsSecret := range ing.Spec.TLS { // TODO: Replace this for a secret watcher. glog.V(3).Infof("Retrieving secret for ing %v with name %v", ing.Name, tlsSecret.SecretName) @@ -84,9 +79,6 @@ func (t *TLSCertsFromSecretsLoader) Load(ing *extensions.Ingress) ([]*loadbalanc return nil, err } certs = append(certs, newCert) - if len(certs) == loadbalancers.TargetProxyCertLimit { - break - } } return certs, nil }