[GLBC] Better certificate handling (#639)

* Harden ssl cert logic to handle unknown state

* Do not delete non-controller-created certificates (pre-shared certs)

* Remove unnecessary variable

* Added three tests to check ssl certificate naming

* Address review comments

* Early return instead of large code block

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 err := utils.IgnoreHTTPNotFound(l.cloud.DeleteSslCertificate(l.oldSSLCert.Name)); err != nil {
-		if !utils.IsHTTPErrorCode(err, http.StatusNotFound) {
+		return err
-			return err
-		}
 	}
 	l.oldSSLCert = nil
 	return nil
 }
 
-func (l *L7) checkSSLCert() (err error) {
+// Returns the name portion of a link - which is the last section
-	certName := l.runtimeInfo.TLSName
+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 != "" {
+	preSharedCertName := l.runtimeInfo.TLSName
-		// Ask GCE for the cert, checking for problems and existence.
+	if preSharedCertName == "" {
-		cert, err := l.cloud.GetSslCertificate(certName)
+		return false, nil
-		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
 	}
 
-	// TODO: Currently, GCE only supports a single certificate per static IP
+	// Ask GCE for the cert, checking for problems and existence.
-	// so we don't need to bother with disambiguation. Naming the cert after
+	cert, err := l.cloud.GetSslCertificate(preSharedCertName)
-	// the loadbalancer is a simplification.
+	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
+	glog.V(2).Infof("Using existing sslCertificate %v for %v", preSharedCertName, l.Name)
-	ingKey := l.runtimeInfo.TLS.Key
+	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 {
+	if l.sslCert != nil && l.sslCert.Name == primaryCertName {
-		certName = l.sslCert.Name
+		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
+	// Get updated value of certificate for comparison
-	// should create/update the cert if there is an error or does not exist.
+	if err := l.populateSSLCert(); err != nil {
-	cert, _ := l.cloud.GetSslCertificate(certName)
+		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 {
+	if l.sslCert != nil && ingCert == l.sslCert.Certificate {
-
+		return nil
-		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
 	}
 
+	// 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 err = utils.IgnoreHTTPNotFound(l.cloud.DeleteGlobalForwardingRule(name)); err != nil {
-			if !utils.IsHTTPErrorCode(err, http.StatusNotFound) {
+			return nil, err
-				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)
+		glog.V(2).Infof("Deleting global forwarding rule %v", l.fw.Name)
-		if err := l.cloud.DeleteGlobalForwardingRule(l.fw.Name); err != nil {
+		if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteGlobalForwardingRule(l.fw.Name)); err != nil {
-			if !utils.IsHTTPErrorCode(err, http.StatusNotFound) {
+			return err
-				return err
-			}
 		}
 		l.fw = nil
 	}
 	if l.fws != nil {
-		glog.Infof("Deleting global forwarding rule %v", l.fws.Name)
+		glog.V(2).Infof("Deleting global forwarding rule %v", l.fws.Name)
-		if err := l.cloud.DeleteGlobalForwardingRule(l.fws.Name); err != nil {
+		if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteGlobalForwardingRule(l.fws.Name)); err != nil {
-			if !utils.IsHTTPErrorCode(err, http.StatusNotFound) {
+			return err
-				return err
-			}
-			l.fws = nil
 		}
+		l.fws = nil
 	}
 	if l.ip != nil {
-		glog.Infof("Deleting static IP %v(%v)", l.ip.Name, l.ip.Address)
+		glog.V(2).Infof("Deleting static IP %v(%v)", l.ip.Name, l.ip.Address)
-		if err := l.cloud.DeleteGlobalStaticIP(l.ip.Name); err != nil {
+		if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteGlobalStaticIP(l.ip.Name)); err != nil {
-			if !utils.IsHTTPErrorCode(err, http.StatusNotFound) {
+			return err
-				return err
-			}
-			l.ip = nil
 		}
+		l.ip = nil
 	}
 	if l.tps != nil {
-		glog.Infof("Deleting target https proxy %v", l.tps.Name)
+		glog.V(2).Infof("Deleting target https proxy %v", l.tps.Name)
-		if err := l.cloud.DeleteTargetHttpsProxy(l.tps.Name); err != nil {
+		if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteTargetHttpsProxy(l.tps.Name)); err != nil {
-			if !utils.IsHTTPErrorCode(err, http.StatusNotFound) {
+			return err
-				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)
+		glog.V(2).Infof("Deleting sslcert %v", l.sslCert.Name)
-		if err := l.cloud.DeleteSslCertificate(l.sslCert.Name); err != nil {
+		if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteSslCertificate(l.sslCert.Name)); err != nil {
-			if !utils.IsHTTPErrorCode(err, http.StatusNotFound) {
+			return err
-				return err
-			}
 		}
 		l.sslCert = nil
 	}
 	if l.tp != nil {
-		glog.Infof("Deleting target http proxy %v", l.tp.Name)
+		glog.V(2).Infof("Deleting target http proxy %v", l.tp.Name)
-		if err := l.cloud.DeleteTargetHttpProxy(l.tp.Name); err != nil {
+		if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteTargetHttpProxy(l.tp.Name)); err != nil {
-			if !utils.IsHTTPErrorCode(err, http.StatusNotFound) {
+			return err
-				return err
-			}
 		}
 		l.tp = nil
 	}
 	if l.um != nil {
-		glog.Infof("Deleting url map %v", l.um.Name)
+		glog.V(2).Infof("Deleting url map %v", l.um.Name)
-		if err := l.cloud.DeleteUrlMap(l.um.Name); err != nil {
+		if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteUrlMap(l.um.Name)); err != nil {
-			if !utils.IsHTTPErrorCode(err, http.StatusNotFound) {
+			return err
-				return err
-			}
 		}
 		l.um = nil
 	}

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.

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