From 3af5f5c31196cc1fb38d9640af713a609cf32bd4 Mon Sep 17 00:00:00 2001 From: Nick Sardo Date: Wed, 26 Apr 2017 09:20:21 -0700 Subject: [PATCH] Address review comments --- .../gce/loadbalancers/loadbalancers.go | 165 +++++++++--------- controllers/gce/utils/utils.go | 12 +- 2 files changed, 95 insertions(+), 82 deletions(-) diff --git a/controllers/gce/loadbalancers/loadbalancers.go b/controllers/gce/loadbalancers/loadbalancers.go index e2d788a2f..df32b7ba9 100644 --- a/controllers/gce/loadbalancers/loadbalancers.go +++ b/controllers/gce/loadbalancers/loadbalancers.go @@ -344,10 +344,8 @@ func (l *L7) deleteOldSSLCert() (err error) { 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 @@ -362,38 +360,71 @@ func getResourceNameFromLink(link string) string { return s[len(s)-1] } -func (l *L7) checkSSLCert() (err error) { - // Handle Pre-Shared cert - preSharedCertName := l.runtimeInfo.TLSName +func (l *L7) usePreSharedCert() (bool, error) { // Use the named GCE cert when it is specified by the annotation. - if preSharedCertName != "" { - // Ask GCE for the cert, checking for problems and existence. - cert, err := l.cloud.GetSslCertificate(preSharedCertName) - if err != nil { - return err - } - if cert == nil { - return fmt.Errorf("cannot find existing sslCertificate %v for %v", preSharedCertName, l.Name) - } - - glog.V(2).Infof("Using existing sslCertificate %v for %v", preSharedCertName, l.Name) - l.sslCert = cert - return nil + preSharedCertName := l.runtimeInfo.TLSName + if preSharedCertName == "" { + return false, nil } - // Handle secret-created cert + // 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) + } + + 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.getSslCertInUse()) + expectedCertName = getResourceNameFromLink(l.getSslCertLinkInUse()) } - // Retrieve known ssl certificate + var err error if expectedCertName != "" { - l.sslCert, _ = l.cloud.GetSslCertificate(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)) + + 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 + } + + // 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 @@ -402,32 +433,20 @@ func (l *L7) checkSSLCert() (err error) { ingCert := l.runtimeInfo.TLS.Cert ingKey := l.runtimeInfo.TLS.Key - // 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)) - // 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 l.sslCert == nil || ingCert != l.sslCert.Certificate { - newCertName := primaryCertName - if l.sslCert != nil && (ingCert != l.sslCert.Certificate) { - if l.sslCert.Name == primaryCertName { - newCertName = secondaryCertName - } - } + 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 = l.cloud.DeleteSslCertificate(newCertName); err != nil && !utils.IsHTTPErrorCode(err, http.StatusNotFound) { + 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.Infof("Creating new sslCertificates %v for %v", newCertName, l.Name) + glog.V(2).Infof("Creating new sslCertificate %v for %v", newCertName, l.Name) cert, err := l.cloud.CreateSslCertificate(&compute.SslCertificate{ Name: newCertName, Certificate: ingCert, @@ -444,7 +463,7 @@ func (l *L7) checkSSLCert() (err error) { return nil } -func (l *L7) getSslCertInUse() string { +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 { @@ -497,10 +516,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 } @@ -862,66 +879,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/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