diff --git a/controllers/gce/loadbalancers/loadbalancers.go b/controllers/gce/loadbalancers/loadbalancers.go index 9972ccc01..24c7bd07f 100644 --- a/controllers/gce/loadbalancers/loadbalancers.go +++ b/controllers/gce/loadbalancers/loadbalancers.go @@ -339,91 +339,143 @@ func (l *L7) checkProxy() (err error) { } func (l *L7) deleteOldSSLCert() (err error) { - if l.oldSSLCert == nil || l.sslCert == nil || l.oldSSLCert.Name == l.sslCert.Name { + if l.oldSSLCert == nil || l.sslCert == nil || + l.oldSSLCert.Name == l.sslCert.Name || !strings.HasPrefix(l.oldSSLCert.Name, sslCertPrefix) { return nil } glog.Infof("Cleaning up old SSL Certificate %v, current name %v", l.oldSSLCert.Name, l.sslCert.Name) - if err := l.cloud.DeleteSslCertificate(l.oldSSLCert.Name); err != nil { - if !utils.IsHTTPErrorCode(err, http.StatusNotFound) { - return err - } + if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteSslCertificate(l.oldSSLCert.Name)); err != nil { + return err } l.oldSSLCert = nil return nil } -func (l *L7) checkSSLCert() (err error) { - certName := l.runtimeInfo.TLSName +// Returns the name portion of a link - which is the last section +func getResourceNameFromLink(link string) string { + s := strings.Split(link, "/") + if len(s) == 0 { + return "" + } + return s[len(s)-1] +} +func (l *L7) usePreSharedCert() (bool, error) { // Use the named GCE cert when it is specified by the annotation. - if certName != "" { - // Ask GCE for the cert, checking for problems and existence. - cert, err := l.cloud.GetSslCertificate(certName) - if err != nil { - return err - } - if cert == nil { - return fmt.Errorf("cannot find existing sslCertificate %v for %v", certName, l.Name) - } - - glog.Infof("Using existing sslCertificate %v for %v", certName, l.Name) - l.sslCert = cert - return nil + preSharedCertName := l.runtimeInfo.TLSName + if preSharedCertName == "" { + return false, nil } - // TODO: Currently, GCE only supports a single certificate per static IP - // so we don't need to bother with disambiguation. Naming the cert after - // the loadbalancer is a simplification. + // Ask GCE for the cert, checking for problems and existence. + cert, err := l.cloud.GetSslCertificate(preSharedCertName) + if err != nil { + return true, err + } + if cert == nil { + return true, fmt.Errorf("cannot find existing sslCertificate %v for %v", preSharedCertName, l.Name) + } - ingCert := l.runtimeInfo.TLS.Cert - ingKey := l.runtimeInfo.TLS.Key + glog.V(2).Infof("Using existing sslCertificate %v for %v", preSharedCertName, l.Name) + l.sslCert = cert + return true, nil +} +func (l *L7) populateSSLCert() error { + // Determine what certificate name is being used + var expectedCertName string + if l.sslCert != nil { + expectedCertName = l.sslCert.Name + } else { + // Retrieve the ssl certificate in use by the expected target proxy (if exists) + expectedCertName = getResourceNameFromLink(l.getSslCertLinkInUse()) + } + + var err error + if expectedCertName != "" { + // Retrieve the certificate and ignore error if certificate wasn't found + l.sslCert, err = l.cloud.GetSslCertificate(expectedCertName) + if err != nil { + return utils.IgnoreHTTPNotFound(err) + } + } + return nil +} + +func (l *L7) nextCertificateName() string { // The name of the cert for this lb flip-flops between these 2 on // every certificate update. We don't append the index at the end so we're // sure it isn't truncated. // TODO: Clean this code up into a ring buffer. primaryCertName := l.namer.Truncate(fmt.Sprintf("%v-%v", sslCertPrefix, l.Name)) secondaryCertName := l.namer.Truncate(fmt.Sprintf("%v-%d-%v", sslCertPrefix, 1, l.Name)) - certName = primaryCertName - if l.sslCert != nil { - certName = l.sslCert.Name + + if l.sslCert != nil && l.sslCert.Name == primaryCertName { + return secondaryCertName + } + return primaryCertName +} + +func (l *L7) checkSSLCert() error { + // Handle Pre-Shared cert and early return if used + if used, err := l.usePreSharedCert(); used { + return err } - // Skip error checking because error-ing out will retry and loop, when we - // should create/update the cert if there is an error or does not exist. - cert, _ := l.cloud.GetSslCertificate(certName) + // Get updated value of certificate for comparison + if err := l.populateSSLCert(); err != nil { + return err + } + + // TODO: Currently, GCE only supports a single certificate per static IP + // so we don't need to bother with disambiguation. Naming the cert after + // the loadbalancer is a simplification. + ingCert := l.runtimeInfo.TLS.Cert + ingKey := l.runtimeInfo.TLS.Key // PrivateKey is write only, so compare certs alone. We're assuming that // no one will change just the key. We can remember the key and compare, // but a bug could end up leaking it, which feels worse. - if cert == nil || ingCert != cert.Certificate { - - certChanged := cert != nil && (ingCert != cert.Certificate) - if certChanged { - if certName == primaryCertName { - certName = secondaryCertName - } else { - certName = primaryCertName - } - } - - glog.Infof("Creating new sslCertificates %v for %v", certName, l.Name) - cert, err = l.cloud.CreateSslCertificate(&compute.SslCertificate{ - Name: certName, - Certificate: ingCert, - PrivateKey: ingKey, - }) - if err != nil { - return err - } - // Save the current cert for cleanup after we update the target proxy. - l.oldSSLCert = l.sslCert + if l.sslCert != nil && ingCert == l.sslCert.Certificate { + return nil } + // Controller needs to create or update the certificate. + // Generate the next certificate name to use. + newCertName := l.nextCertificateName() + + // Perform a delete in case a certificate exists with the exact name + // This certificate should be unused since we check the target proxy's certificate prior + // to this point. Although, it's possible an actor pointed a target proxy to this certificate. + if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteSslCertificate(newCertName)); err != nil { + return fmt.Errorf("unable to delete ssl certificate with name %q, expected it to be unused. err: %v", newCertName, err) + } + + glog.V(2).Infof("Creating new sslCertificate %v for %v", newCertName, l.Name) + cert, err := l.cloud.CreateSslCertificate(&compute.SslCertificate{ + Name: newCertName, + Certificate: ingCert, + PrivateKey: ingKey, + }) + if err != nil { + return err + } + // Save the current cert for cleanup after we update the target proxy. + l.oldSSLCert = l.sslCert l.sslCert = cert + return nil } +func (l *L7) getSslCertLinkInUse() string { + proxyName := l.namer.Truncate(fmt.Sprintf("%v-%v", targetHTTPSProxyPrefix, l.Name)) + proxy, _ := l.cloud.GetTargetHttpsProxy(proxyName) + if proxy != nil && len(proxy.SslCertificates) > 0 { + return proxy.SslCertificates[0] + } + return "" +} + func (l *L7) checkHttpsProxy() (err error) { if l.sslCert == nil { glog.V(3).Infof("No SSL certificates for %v, will not create HTTPS proxy.", l.Name) @@ -468,10 +520,8 @@ func (l *L7) checkForwardingRule(name, proxyLink, ip, portRange string) (fw *com if fw != nil && (ip != "" && fw.IPAddress != ip || fw.PortRange != portRange) { glog.Warningf("Recreating forwarding rule %v(%v), so it has %v(%v)", fw.IPAddress, fw.PortRange, ip, portRange) - if err := l.cloud.DeleteGlobalForwardingRule(name); err != nil { - if !utils.IsHTTPErrorCode(err, http.StatusNotFound) { - return nil, err - } + if err = utils.IgnoreHTTPNotFound(l.cloud.DeleteGlobalForwardingRule(name)); err != nil { + return nil, err } fw = nil } @@ -833,66 +883,52 @@ func mapsEqual(a, b *compute.UrlMap) bool { // This leaves backends and health checks, which are shared across loadbalancers. func (l *L7) Cleanup() error { if l.fw != nil { - glog.Infof("Deleting global forwarding rule %v", l.fw.Name) - if err := l.cloud.DeleteGlobalForwardingRule(l.fw.Name); err != nil { - if !utils.IsHTTPErrorCode(err, http.StatusNotFound) { - return err - } + glog.V(2).Infof("Deleting global forwarding rule %v", l.fw.Name) + if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteGlobalForwardingRule(l.fw.Name)); err != nil { + return err } l.fw = nil } if l.fws != nil { - glog.Infof("Deleting global forwarding rule %v", l.fws.Name) - if err := l.cloud.DeleteGlobalForwardingRule(l.fws.Name); err != nil { - if !utils.IsHTTPErrorCode(err, http.StatusNotFound) { - return err - } - l.fws = nil + glog.V(2).Infof("Deleting global forwarding rule %v", l.fws.Name) + if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteGlobalForwardingRule(l.fws.Name)); err != nil { + return err } + l.fws = nil } if l.ip != nil { - glog.Infof("Deleting static IP %v(%v)", l.ip.Name, l.ip.Address) - if err := l.cloud.DeleteGlobalStaticIP(l.ip.Name); err != nil { - if !utils.IsHTTPErrorCode(err, http.StatusNotFound) { - return err - } - l.ip = nil + glog.V(2).Infof("Deleting static IP %v(%v)", l.ip.Name, l.ip.Address) + if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteGlobalStaticIP(l.ip.Name)); err != nil { + return err } + l.ip = nil } if l.tps != nil { - glog.Infof("Deleting target https proxy %v", l.tps.Name) - if err := l.cloud.DeleteTargetHttpsProxy(l.tps.Name); err != nil { - if !utils.IsHTTPErrorCode(err, http.StatusNotFound) { - return err - } + glog.V(2).Infof("Deleting target https proxy %v", l.tps.Name) + if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteTargetHttpsProxy(l.tps.Name)); err != nil { + return err } l.tps = nil } // Delete the SSL cert if it is from a secret, not referencing a pre-created GCE cert. if l.sslCert != nil && l.runtimeInfo.TLSName == "" { - glog.Infof("Deleting sslcert %v", l.sslCert.Name) - if err := l.cloud.DeleteSslCertificate(l.sslCert.Name); err != nil { - if !utils.IsHTTPErrorCode(err, http.StatusNotFound) { - return err - } + glog.V(2).Infof("Deleting sslcert %v", l.sslCert.Name) + if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteSslCertificate(l.sslCert.Name)); err != nil { + return err } l.sslCert = nil } if l.tp != nil { - glog.Infof("Deleting target http proxy %v", l.tp.Name) - if err := l.cloud.DeleteTargetHttpProxy(l.tp.Name); err != nil { - if !utils.IsHTTPErrorCode(err, http.StatusNotFound) { - return err - } + glog.V(2).Infof("Deleting target http proxy %v", l.tp.Name) + if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteTargetHttpProxy(l.tp.Name)); err != nil { + return err } l.tp = nil } if l.um != nil { - glog.Infof("Deleting url map %v", l.um.Name) - if err := l.cloud.DeleteUrlMap(l.um.Name); err != nil { - if !utils.IsHTTPErrorCode(err, http.StatusNotFound) { - return err - } + glog.V(2).Infof("Deleting url map %v", l.um.Name) + if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteUrlMap(l.um.Name)); err != nil { + return err } l.um = nil } diff --git a/controllers/gce/loadbalancers/loadbalancers_test.go b/controllers/gce/loadbalancers/loadbalancers_test.go index 7d7c6925d..7f8a0635b 100644 --- a/controllers/gce/loadbalancers/loadbalancers_test.go +++ b/controllers/gce/loadbalancers/loadbalancers_test.go @@ -106,6 +106,118 @@ func TestCreateHTTPSLoadBalancer(t *testing.T) { } } +// Tests that a certificate is created from the provided Key/Cert combo +// and the proxy is updated to another cert when the provided cert changes +func TestCertUpdate(t *testing.T) { + primaryCertName := "k8s-ssl-test" + secondaryCertName := "k8s-ssl-1-test" + lbInfo := &L7RuntimeInfo{ + Name: "test", + AllowHTTP: false, + TLS: &TLSCerts{Key: "key", Cert: "cert"}, + } + + f := NewFakeLoadBalancers(lbInfo.Name) + pool := newFakeLoadBalancerPool(f, t) + + // Sync first cert + pool.Sync([]*L7RuntimeInfo{lbInfo}) + verifyCertAndProxyLink(primaryCertName, lbInfo.TLS.Cert, f, t) + + // Sync with different cert + lbInfo.TLS = &TLSCerts{Key: "key2", Cert: "cert2"} + pool.Sync([]*L7RuntimeInfo{lbInfo}) + verifyCertAndProxyLink(secondaryCertName, lbInfo.TLS.Cert, f, t) +} + +// Tests that controller can overwrite existing, unused certificates +func TestCertCreationWithCollision(t *testing.T) { + primaryCertName := "k8s-ssl-test" + secondaryCertName := "k8s-ssl-1-test" + lbInfo := &L7RuntimeInfo{ + Name: "test", + AllowHTTP: false, + TLS: &TLSCerts{Key: "key", Cert: "cert"}, + } + + f := NewFakeLoadBalancers(lbInfo.Name) + pool := newFakeLoadBalancerPool(f, t) + + // Have both names already used by orphaned certs + f.CreateSslCertificate(&compute.SslCertificate{ + Name: primaryCertName, + Certificate: "abc", + SelfLink: "existing", + }) + f.CreateSslCertificate(&compute.SslCertificate{ + Name: secondaryCertName, + Certificate: "xyz", + SelfLink: "existing", + }) + + // Sync first cert + pool.Sync([]*L7RuntimeInfo{lbInfo}) + verifyCertAndProxyLink(primaryCertName, lbInfo.TLS.Cert, f, t) + + // Sync with different cert + lbInfo.TLS = &TLSCerts{Key: "key2", Cert: "cert2"} + pool.Sync([]*L7RuntimeInfo{lbInfo}) + verifyCertAndProxyLink(secondaryCertName, lbInfo.TLS.Cert, f, t) +} + +func TestCertRetentionAfterRestart(t *testing.T) { + primaryCertName := "k8s-ssl-test" + secondaryCertName := "k8s-ssl-1-test" + lbInfo := &L7RuntimeInfo{ + Name: "test", + AllowHTTP: false, + TLS: &TLSCerts{Key: "key", Cert: "cert"}, + } + + f := NewFakeLoadBalancers(lbInfo.Name) + firstPool := newFakeLoadBalancerPool(f, t) + + // Sync twice so the expected certificate uses the secondary name + firstPool.Sync([]*L7RuntimeInfo{lbInfo}) + verifyCertAndProxyLink(primaryCertName, lbInfo.TLS.Cert, f, t) + lbInfo.TLS = &TLSCerts{Key: "key2", Cert: "cert2"} + firstPool.Sync([]*L7RuntimeInfo{lbInfo}) + verifyCertAndProxyLink(secondaryCertName, lbInfo.TLS.Cert, f, t) + + // Restart of controller represented by a new pool + secondPool := newFakeLoadBalancerPool(f, t) + secondPool.Sync([]*L7RuntimeInfo{lbInfo}) + + // Verify second name is still used + verifyCertAndProxyLink(secondaryCertName, lbInfo.TLS.Cert, f, t) + + // Update cert one more time to verify loop + lbInfo.TLS = &TLSCerts{Key: "key3", Cert: "cert3"} + secondPool.Sync([]*L7RuntimeInfo{lbInfo}) + verifyCertAndProxyLink(primaryCertName, lbInfo.TLS.Cert, f, t) + +} + +func verifyCertAndProxyLink(certName, certValue string, f *FakeLoadBalancers, t *testing.T) { + cert, err := f.GetSslCertificate(certName) + if err != nil { + t.Fatalf("expected ssl certificate to exist: %v, err: %v", certName, err) + } + + if cert.Certificate != certValue { + t.Fatalf("unexpected certificate value; expected %v, actual %v", certValue, cert.Certificate) + } + + tps, err := f.GetTargetHttpsProxy(f.tpName(true)) + if err != nil { + t.Fatalf("expected https proxy to exist: %v, err: %v", certName, err) + } + + if len(tps.SslCertificates) == 0 || tps.SslCertificates[0] != cert.SelfLink { + t.Fatalf("expected ssl certificate to be linked in target proxy; Cert Link: %q; Target Proxy Certs: %v", cert.SelfLink, tps.SslCertificates) + } +} + func TestCreateHTTPSLoadBalancerAnnotationCert(t *testing.T) { // This should NOT create the forwarding rule and target proxy // associated with the HTTP branch of this loadbalancer. diff --git a/controllers/gce/utils/utils.go b/controllers/gce/utils/utils.go index 1db1541e8..3b628ef6c 100644 --- a/controllers/gce/utils/utils.go +++ b/controllers/gce/utils/utils.go @@ -18,6 +18,7 @@ package utils import ( "fmt" + "net/http" "regexp" "strconv" "strings" @@ -310,7 +311,7 @@ func (g GCEURLMap) PutDefaultBackend(d *compute.BackendService) { } } -// FakeNotFoundErr creates a NotFound error with type googleapi.Error +// FakeGoogleAPINotFoundErr creates a NotFound error with type googleapi.Error func FakeGoogleAPINotFoundErr() *googleapi.Error { return &googleapi.Error{Code: 404} } @@ -322,6 +323,15 @@ func IsHTTPErrorCode(err error, code int) bool { return ok && apiErr.Code == code } +// IgnoreHTTPNotFound returns the passed err if it's not a GoogleAPI error +// with a NotFound status code. +func IgnoreHTTPNotFound(err error) error { + if err != nil && IsHTTPErrorCode(err, http.StatusNotFound) { + return nil + } + return err +} + // CompareLinks returns true if the 2 self links are equal. func CompareLinks(l1, l2 string) bool { // TODO: These can be partial links