Change naming of SSL certs

pkg/loadbalancers/l7.go

@@ -30,6 +30,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 
 	"crypto/sha256"
+
 	"k8s.io/ingress-gce/pkg/annotations"
 	"k8s.io/ingress-gce/pkg/backends"
 	"k8s.io/ingress-gce/pkg/utils"
@@ -109,8 +110,6 @@ type L7 struct {
 	fws *compute.ForwardingRule
 	// ip is the static-ip associated with both GlobalForwardingRules.
 	ip *compute.Address
-	// prefix to use in ssl cert names
-	sslCertPrefix string
 	// sslCerts is the list of ssl certs associated with the targetHTTPSProxy.
 	sslCerts []*compute.SslCertificate
 	// oldSSLCerts is the list of certs that used to be hooked up to the
@@ -197,7 +196,7 @@ func (l *L7) deleteOldSSLCerts() (err error) {
 	}
 	certsMap := getMapfromCertList(l.sslCerts)
 	for _, cert := range l.oldSSLCerts {
-		if !l.isSSLCert(cert.Name) && !l.namer.IsLegacySSLCert(l.Name, cert.Name) {
+		if !l.namer.IsCertUsedForLB(l.Name, cert.Name) && !l.namer.IsLegacySSLCert(l.Name, cert.Name) {
 			// retain cert if it is managed by GCE(non-ingress)
 			continue
 		}
@@ -261,11 +260,6 @@ func (l *L7) usePreSharedCert() (bool, error) {
 	return true, nil
 }
 
-// isSSLCert returns true if name is ingress managed, specifically by this loadbalancer instance
-func (l *L7) isSSLCert(name string) bool {
-	return strings.HasPrefix(name, l.sslCertPrefix)
-}
-
 func getMapfromCertList(certs []*compute.SslCertificate) map[string]*compute.SslCertificate {
 	if len(certs) == 0 {
 		return nil
@@ -287,7 +281,7 @@ func (l *L7) populateSSLCert() error {
 		return utils.IgnoreHTTPNotFound(err)
 	}
 	for _, c := range certs {
-		if l.isSSLCert(c.Name) {
+		if l.namer.IsCertUsedForLB(l.Name, c.Name) {
 			glog.Infof("Populating ssl cert %s for l7 %s", c.Name, l.Name)
 			l.sslCerts = append(l.sslCerts, c)
 		}
@@ -331,7 +325,7 @@ func (l *L7) checkSSLCert() error {
 	for _, tlsCert := range l.runtimeInfo.TLS {
 		ingCert := tlsCert.Cert
 		ingKey := tlsCert.Key
-		newCertName := l.namer.SSLCertName(l.sslCertPrefix, tlsCert.CertHash)
+		newCertName := l.namer.SSLCertName(l.Name, tlsCert.CertHash)
 
 		// 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,
@@ -423,7 +417,7 @@ func (l *L7) checkHttpsProxy() (err error) {
 
 	if !l.compareCerts(proxy.SslCertificates) {
 		glog.Infof("Https proxy %v has the wrong ssl certs, setting %v overwriting %v",
-			proxy.Name, l.sslCerts, proxy.SslCertificates)
+			proxy.Name, toCertNames(l.sslCerts), proxy.SslCertificates)
 		if err := l.cloud.SetSslCertificateForTargetHttpsProxy(proxy, l.sslCerts); err != nil {
 			return err
 		}
@@ -989,3 +983,10 @@ func GCEResourceName(ingAnnotations map[string]string, resourceName string) stri
 func GetCertHash(contents string) string {
 	return fmt.Sprintf("%x", sha256.Sum256([]byte(contents)))[:16]
 }
+
+func toCertNames(certs []*compute.SslCertificate) (names []string) {
+	for _, v := range certs {
+		names = append(names, v.Name)
+	}
+	return names
+}

pkg/loadbalancers/l7s.go

@@ -76,7 +76,6 @@ func (l *L7s) create(ri *L7RuntimeInfo) (*L7, error) {
 		cloud:              l.cloud,
 		glbcDefaultBackend: l.glbcDefaultBackend,
 		namer:              l.namer,
-		sslCertPrefix:      l.namer.SSLCertPrefix(ri.Name),
 	}, nil
 }
 

pkg/loadbalancers/loadbalancers_test.go

@@ -23,14 +23,15 @@ import (
 	compute "google.golang.org/api/compute/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 
+	"strconv"
+	"strings"
+
 	"k8s.io/ingress-gce/pkg/annotations"
 	"k8s.io/ingress-gce/pkg/backends"
 	"k8s.io/ingress-gce/pkg/healthchecks"
 	"k8s.io/ingress-gce/pkg/instances"
 	"k8s.io/ingress-gce/pkg/neg"
 	"k8s.io/ingress-gce/pkg/utils"
-	"strconv"
-	"strings"
 )
 
 const (
@@ -65,11 +66,13 @@ func TestCreateHTTPLoadBalancer(t *testing.T) {
 	}
 	f := NewFakeLoadBalancers(lbInfo.Name, namer)
 	pool := newFakeLoadBalancerPool(f, t, namer)
+
+	// Run Sync
 	pool.Sync([]*L7RuntimeInfo{lbInfo})
-	l7, err := pool.Get(lbInfo.Name)
 
+	l7, err := pool.Get(lbInfo.Name)
 	if err != nil || l7 == nil {
-		t.Fatalf("Expected l7 not created")
+		t.Fatalf("Expected l7 not created, err: %v", err)
 	}
 	um, err := f.GetUrlMap(f.UMName())
 	if err != nil {
@@ -129,12 +132,12 @@ func TestCreateHTTPSLoadBalancer(t *testing.T) {
 // and the proxy is updated to another cert when the provided cert changes
 func TestCertUpdate(t *testing.T) {
 	namer := utils.NewNamer("uid1", "fw1")
-	sslCertPrefix := namer.SSLCertPrefix("test")
-	certName1 := namer.SSLCertName(sslCertPrefix, GetCertHash("cert"))
-	certName2 := namer.SSLCertName(sslCertPrefix, GetCertHash("cert2"))
+	lbName := namer.LoadBalancer("test")
+	certName1 := namer.SSLCertName(lbName, GetCertHash("cert"))
+	certName2 := namer.SSLCertName(lbName, GetCertHash("cert2"))
 
 	lbInfo := &L7RuntimeInfo{
-		Name:      namer.LoadBalancer("test"),
+		Name:      lbName,
 		AllowHTTP: false,
 		TLS:       []*TLSCerts{createCert("key", "cert", "name")},
 	}
@@ -144,7 +147,9 @@ func TestCertUpdate(t *testing.T) {
 
 	// Sync first cert
 	pool.Sync([]*L7RuntimeInfo{lbInfo})
-	t.Logf("name=%q", certName1)
+
+	// Verify certs
+	t.Logf("lbName=%q, name=%q", lbName, certName1)
 	expectCerts := map[string]string{certName1: lbInfo.TLS[0].Cert}
 	verifyCertAndProxyLink(expectCerts, expectCerts, f, t)
 
@@ -158,12 +163,12 @@ func TestCertUpdate(t *testing.T) {
 // Tests that controller can overwrite existing, unused certificates
 func TestCertCreationWithCollision(t *testing.T) {
 	namer := utils.NewNamer("uid1", "fw1")
-	sslCertPrefix := namer.SSLCertPrefix("test")
-	certName1 := namer.SSLCertName(sslCertPrefix, GetCertHash("cert"))
-	certName2 := namer.SSLCertName(sslCertPrefix, GetCertHash("cert2"))
+	lbName := namer.LoadBalancer("test")
+	certName1 := namer.SSLCertName(lbName, GetCertHash("cert"))
+	certName2 := namer.SSLCertName(lbName, GetCertHash("cert2"))
 
 	lbInfo := &L7RuntimeInfo{
-		Name:      namer.LoadBalancer("test"),
+		Name:      lbName,
 		AllowHTTP: false,
 		TLS:       []*TLSCerts{createCert("key", "cert", "name")},
 	}
@@ -202,18 +207,18 @@ func TestCertCreationWithCollision(t *testing.T) {
 
 func TestMultipleCertRetentionAfterRestart(t *testing.T) {
 	namer := utils.NewNamer("uid1", "fw1")
-	sslCertPrefix := namer.SSLCertPrefix("test")
 	cert1 := createCert("key", "cert", "name")
 	cert2 := createCert("key2", "cert2", "name2")
 	cert3 := createCert("key3", "cert3", "name3")
-	certName1 := namer.SSLCertName(sslCertPrefix, cert1.CertHash)
-	certName2 := namer.SSLCertName(sslCertPrefix, cert2.CertHash)
-	certName3 := namer.SSLCertName(sslCertPrefix, cert3.CertHash)
+	lbName := namer.LoadBalancer("test")
+	certName1 := namer.SSLCertName(lbName, cert1.CertHash)
+	certName2 := namer.SSLCertName(lbName, cert2.CertHash)
+	certName3 := namer.SSLCertName(lbName, cert3.CertHash)
 
 	expectCerts := map[string]string{}
 
 	lbInfo := &L7RuntimeInfo{
-		Name:      namer.LoadBalancer("test"),
+		Name:      lbName,
 		AllowHTTP: false,
 		TLS:       []*TLSCerts{cert1},
 	}
@@ -249,14 +254,15 @@ func TestMultipleCertRetentionAfterRestart(t *testing.T) {
 // are picked up and deleted when upgrading to the new scheme.
 func TestUpgradeToNewCertNames(t *testing.T) {
 	namer := utils.NewNamer("uid1", "fw1")
+	lbName := namer.LoadBalancer("test")
 	lbInfo := &L7RuntimeInfo{
-		Name:      namer.LoadBalancer("test"),
+		Name:      lbName,
 		AllowHTTP: false,
 	}
 	oldCertName := "k8s-ssl-" + lbInfo.Name
 	tlsCert := createCert("key", "cert", "name")
 	lbInfo.TLS = []*TLSCerts{tlsCert}
-	newCertName := namer.SSLCertName(namer.SSLCertPrefix("test"), tlsCert.CertHash)
+	newCertName := namer.SSLCertName(lbName, tlsCert.CertHash)
 	f := NewFakeLoadBalancers(lbInfo.Name, namer)
 	pool := newFakeLoadBalancerPool(f, t, namer)
 
@@ -292,16 +298,16 @@ func TestMaxCertsUpload(t *testing.T) {
 	var tlsCerts []*TLSCerts
 	expectCerts := make(map[string]string)
 	namer := utils.NewNamer("uid1", "fw1")
-	certPrefix := namer.SSLCertPrefix("test")
+	lbName := namer.LoadBalancer("test")
 
 	for ix := 0; ix < TargetProxyCertLimit; ix++ {
 		str := strconv.Itoa(ix)
 		tlsCerts = append(tlsCerts, createCert("key-"+str, "cert-"+str, "name-"+str))
-		certName := namer.SSLCertName(certPrefix, GetCertHash("cert-"+str))
+		certName := namer.SSLCertName(lbName, GetCertHash("cert-"+str))
 		expectCerts[certName] = "cert-" + str
 	}
 	lbInfo := &L7RuntimeInfo{
-		Name:      namer.LoadBalancer("test"),
+		Name:      lbName,
 		AllowHTTP: false,
 		TLS:       tlsCerts,
 	}
@@ -322,18 +328,18 @@ func TestIdenticalHostnameCerts(t *testing.T) {
 	var tlsCerts []*TLSCerts
 	expectCerts := make(map[string]string)
 	namer := utils.NewNamer("uid1", "fw1")
-	certPrefix := namer.SSLCertPrefix("test")
+	lbName := namer.LoadBalancer("test")
 	contents := ""
 
 	for ix := 0; ix < 3; ix++ {
 		str := strconv.Itoa(ix)
 		contents = "cert-" + str + " foo.com"
 		tlsCerts = append(tlsCerts, createCert("key-"+str, contents, "name-"+str))
-		certName := namer.SSLCertName(certPrefix, GetCertHash(contents))
+		certName := namer.SSLCertName(lbName, GetCertHash(contents))
 		expectCerts[certName] = contents
 	}
 	lbInfo := &L7RuntimeInfo{
-		Name:      namer.LoadBalancer("test"),
+		Name:      lbName,
 		AllowHTTP: false,
 		TLS:       tlsCerts,
 	}
@@ -391,12 +397,12 @@ func TestIdenticalHostnameCertsPreShared(t *testing.T) {
 // to secret based cert and verifies the pre-shared cert is retained.
 func TestPreSharedToSecretBasedCertUpdate(t *testing.T) {
 	namer := utils.NewNamer("uid1", "fw1")
-	sslCertPrefix := namer.SSLCertPrefix("test")
-	certName1 := namer.SSLCertName(sslCertPrefix, GetCertHash("cert"))
-	certName2 := namer.SSLCertName(sslCertPrefix, GetCertHash("cert2"))
+	lbName := namer.LoadBalancer("test")
+	certName1 := namer.SSLCertName(lbName, GetCertHash("cert"))
+	certName2 := namer.SSLCertName(lbName, GetCertHash("cert2"))
 
 	lbInfo := &L7RuntimeInfo{
-		Name:      namer.LoadBalancer("test"),
+		Name:      lbName,
 		AllowHTTP: false,
 	}
 
@@ -500,7 +506,7 @@ func verifyCertAndProxyLink(expectCerts map[string]string, expectCertsProxy map[
 	for certName, certValue := range expectCerts {
 		cert, err := f.GetSslCertificate(certName)
 		if err != nil {
-			t.Fatalf("expected ssl certificate to exist: %v, err: %v", certName, err)
+			t.Fatalf("expected ssl certificate to exist: %v, err: %v, all certs: %v", certName, err, toCertNames(allCerts))
 		}
 
 		if cert.Certificate != certValue {

pkg/utils/namer.go

@@ -25,6 +25,7 @@ import (
 	"sync"
 
 	"crypto/sha256"
+
 	"github.com/golang/glog"
 )
 
@@ -302,35 +303,33 @@ func (n *Namer) TargetProxy(lbName string, protocol NamerProtocol) string {
 	return "invalid"
 }
 
-// IsLegacySSLCert returns true if certName is an Ingress managed name following the older naming convention. The check
-// also ensures that the cert is managed by the specific ingress instance - lbName
-func (n *Namer) IsLegacySSLCert(lbName string, name string) bool {
-	// old style name is of the form k8s-ssl-<lbname> or k8s-ssl-1-<lbName>.
-	legacyPrefixPrimary := truncate(strings.Join([]string{n.prefix, sslCertPrefix, lbName}, "-"))
-	legacyPrefixSec := truncate(strings.Join([]string{n.prefix, sslCertPrefix, "1", lbName}, "-"))
-	return strings.HasPrefix(name, legacyPrefixPrimary) || strings.HasPrefix(name, legacyPrefixSec)
+// IsCertUsedForLB returns true if the resourceName belongs to this cluster's ingress.
+// It checks that the hashed lbName exists and
+func (n *Namer) IsCertUsedForLB(lbName, resourceName string) bool {
+	lbNameHash := n.lbNameToHash(lbName)
+	prefix := fmt.Sprintf("%s-%s-%s", n.prefix, sslCertPrefix, lbNameHash)
+	return strings.HasPrefix(resourceName, prefix) && strings.HasSuffix(resourceName, n.UID())
 }
 
-func (n *Namer) SSLCertPrefix(lbKey string) string {
-	// lbKey is of the form namespace/ingressname. Cert prefix will use the 8 byte(16 chars) hash of namespace-ingressname
-	// followed by the cluster id(also 16 char). We use hash instead of the actual name so that the cert prefix is of a
-	// fixed length and at the same time unique for a given loadbalancer instance.
+func (n *Namer) lbNameToHash(lbName string) string {
+	ingHash := fmt.Sprintf("%x", sha256.Sum256([]byte(lbName)))
+	return ingHash[:16]
+}
 
-	parts := strings.Split(lbKey, clusterNameDelimiter)
-	scrubbedName := strings.Replace(lbKey, "/", "-", -1)
-	clusterName := n.UID()
-	if parts[len(parts)-1] == clusterName {
-		scrubbedName = strings.TrimSuffix(scrubbedName, clusterNameDelimiter+clusterName)
-	}
-	// sha256 is 32 bytes(64 chars) long, truncating to first 8 bytes still results in low probability of collision
-	namespaceHash := fmt.Sprintf("%x", sha256.Sum256([]byte(scrubbedName)))
-	clusterPrefix := namespaceHash[:16] + clusterNameDelimiter + clusterName
-	return fmt.Sprintf("%s-%s-%s", n.prefix, sslCertPrefix, clusterPrefix)
+// IsLegacySSLCert returns true if certName is an Ingress managed name following the older naming convention. The check
+// also ensures that the cert is managed by the specific ingress instance - lbName
+func (n *Namer) IsLegacySSLCert(lbName string, resourceName string) bool {
+	// old style name is of the form k8s-ssl-<lbname> or k8s-ssl-1-<lbName>.
+	legacyPrefixPrimary := truncate(fmt.Sprintf("%s-%s-%s", n.prefix, sslCertPrefix, lbName))
+	legacyPrefixSec := truncate(fmt.Sprintf("%s-%s-1-%s", n.prefix, sslCertPrefix, lbName))
+	return strings.HasPrefix(resourceName, legacyPrefixPrimary) || strings.HasPrefix(resourceName, legacyPrefixSec)
 }
 
 // SSLCertName returns the name of the certificate.
-func (n *Namer) SSLCertName(prefix string, secretHash string) string {
-	return truncate(prefix + "-" + secretHash)
+func (n *Namer) SSLCertName(lbName string, secretHash string) string {
+	lbNameHash := n.lbNameToHash(lbName)
+	// k8s-ssl-[lbNameHash]-[certhash]--[clusterUID]
+	return truncate(fmt.Sprintf("%s-%s-%s-%s%s%s", n.prefix, sslCertPrefix, lbNameHash, secretHash, clusterNameDelimiter, n.UID()))
 }
 
 // ForwardingRule returns the name of the forwarding rule prefix.