When a router is reloaded after a batch of route/ingress changes are

committed, haproxy sometimes fail to reload. This can happen if a new
request to delete a route (and so delete the associated certificates)
is processed when the haproxy router is reloading. The router does
recover on subsequent reloads when the config changes are actually
processed. The change here defers the deletes till commit time and
ensures we only delete the certificates for the routes that are being
processed as part of the current batchset.

Author: ramr

Diff for: pkg/router/template/certmanager.go

@@ -12,10 +12,25 @@ import (
 	routeapi "github.com/openshift/origin/pkg/route/apis/route"
 )
 
+// certificateFile represents a certificate file.
+type certificateFile struct {
+	CertDir  string
+	ID       string
+	Contents []byte
+}
+
+// certificateFileTag generates a certificate file tag/name. This is used to
+// index into the map of deleted certificates.
+func (cf certificateFile) Tag() string {
+	return filepath.Join(cf.CertDir, cf.ID+".pem")
+}
+
 // simpleCertificateManager is the default implementation of a certificateManager
 type simpleCertificateManager struct {
 	cfg *certificateManagerConfig
 	w   certificateWriter
+
+	deletedCertificates map[string]certificateFile
 }
 
 // newSimpleCertificateManager should be used to create a new cert manager.  It will return an error
@@ -27,7 +42,7 @@ func newSimpleCertificateManager(cfg *certificateManagerConfig, w certificateWri
 	if w == nil {
 		return nil, fmt.Errorf("certificate manager requires a certificate writer")
 	}
-	return &simpleCertificateManager{cfg, w}, nil
+	return &simpleCertificateManager{cfg, w, make(map[string]certificateFile, 0)}, nil
 }
 
 // validateCertManagerConfig ensures that the key functions and directories are set as well as
@@ -81,6 +96,8 @@ func (cm *simpleCertificateManager) WriteCertificatesForConfig(config *ServiceAl
 					buffer.Write([]byte(caCertObj.Contents))
 				}
 
+				certFile := certificateFile{CertDir: cm.cfg.certDir, ID: certObj.ID}
+				delete(cm.deletedCertificates, certFile.Tag())
 				if err := cm.w.WriteCertificate(cm.cfg.certDir, certObj.ID, buffer.Bytes()); err != nil {
 					return err
 				}
@@ -92,6 +109,8 @@ func (cm *simpleCertificateManager) WriteCertificatesForConfig(config *ServiceAl
 			destCert, ok := config.Certificates[destCertKey]
 
 			if ok {
+				destCertFile := certificateFile{CertDir: cm.cfg.caCertDir, ID: destCert.ID}
+				delete(cm.deletedCertificates, destCertFile.Tag())
 				if err := cm.w.WriteCertificate(cm.cfg.caCertDir, destCert.ID, []byte(destCert.Contents)); err != nil {
 					return err
 				}
@@ -101,7 +120,7 @@ func (cm *simpleCertificateManager) WriteCertificatesForConfig(config *ServiceAl
 	return nil
 }
 
-// deleteCertificatesForConfig will delete all certificates for the ServiceAliasConfig
+// DeleteCertificatesForConfig will delete all certificates for the ServiceAliasConfig
 func (cm *simpleCertificateManager) DeleteCertificatesForConfig(config *ServiceAliasConfig) error {
 	if config == nil {
 		return nil
@@ -112,10 +131,8 @@ func (cm *simpleCertificateManager) DeleteCertificatesForConfig(config *ServiceA
 			certObj, ok := config.Certificates[certKey]
 
 			if ok {
-				err := cm.w.DeleteCertificate(cm.cfg.certDir, certObj.ID)
-				if err != nil {
-					return err
-				}
+				certFile := certificateFile{CertDir: cm.cfg.certDir, ID: certObj.ID}
+				cm.deletedCertificates[certFile.Tag()] = certFile
 			}
 		}
 
@@ -124,16 +141,41 @@ func (cm *simpleCertificateManager) DeleteCertificatesForConfig(config *ServiceA
 			destCert, ok := config.Certificates[destCertKey]
 
 			if ok {
-				err := cm.w.DeleteCertificate(cm.cfg.caCertDir, destCert.ID)
-				if err != nil {
-					return err
-				}
+
+				destCertFile := certificateFile{CertDir: cm.cfg.caCertDir, ID: destCert.ID}
+				cm.deletedCertificates[destCertFile.Tag()] = destCertFile
 			}
 		}
 	}
 	return nil
 }
 
+// Commit applies any pending changes made to the certificateManager.
+func (cm *simpleCertificateManager) Commit() error {
+	// Deletion of certificates that are being referenced in backends or
+	// config is problematic in that the template router will not
+	// reload because the config is invalid, so we _do_ need to "stage"
+	// or commit the removals. Remove all the deleted certificates.
+	for _, certFile := range cm.deletedCertificates {
+		err := cm.w.DeleteCertificate(certFile.CertDir, certFile.ID)
+		if err != nil {
+			// TODO: Do we care if a file deletion failed?
+			// Otherwise we will keep hitting this error on
+			// every commit. Should we just ignore errors here?
+			// Clean this up based on review comments.
+			// FIXME: return err
+		}
+	}
+
+	cm.deletedCertificates = make(map[string]certificateFile, 0)
+
+	// If we decide to stage the certificate writes, we can flush the
+	// write to the disk here. The tradeoff is storing a copy in memory
+	// until we commit.
+
+	return nil
+}
+
 // simpleCertificateWriter is the default implementation of a certificateWriter
 type simpleCertificateWriter struct{}
 

Diff for: pkg/router/template/certmanager_test.go

@@ -2,6 +2,7 @@ package templaterouter
 
 import (
 	"reflect"
+	"sort"
 	"testing"
 
 	routeapi "github.com/openshift/origin/pkg/route/apis/route"
@@ -119,6 +120,7 @@ func TestCertManager(t *testing.T) {
 	}
 
 	for k, tc := range testCases {
+		certManager.Commit()
 		fakeCertWriter.clear()
 		err := certManager.WriteCertificatesForConfig(tc.cfg)
 		if err != nil {
@@ -133,14 +135,31 @@ func TestCertManager(t *testing.T) {
 			t.Errorf("Unexpected number of adds for %s occurred. Expected: %d Got: %d", k, len(tc.expectedAdds), len(fakeCertWriter.addedCerts))
 		}
 
+		if 0 != len(fakeCertWriter.deletedCerts) {
+			t.Errorf("Unexpected number of deletes prior to certManager.Commit() for %s occurred. Expected: 0 Got: %d", k, len(fakeCertWriter.deletedCerts))
+		}
+
+		err = certManager.Commit()
+		if err != nil {
+			t.Fatalf("Unexpected error committing certs for service alias config for %s.  Config: %v, err: %v", k, tc.cfg, err)
+		}
+
+		if len(tc.expectedAdds) != len(fakeCertWriter.addedCerts) {
+			t.Errorf("Unexpected number of adds for %s occurred. Expected: %d Got: %d", k, len(tc.expectedAdds), len(fakeCertWriter.addedCerts))
+		}
+
 		if len(tc.expectedDeletes) != len(fakeCertWriter.deletedCerts) {
 			t.Errorf("Unexpected number of deletes for %s occurred. Expected: %d Got: %d", k, len(tc.expectedDeletes), len(fakeCertWriter.deletedCerts))
 		}
 
+		sort.Strings(tc.expectedAdds)
+		sort.Strings(fakeCertWriter.addedCerts)
 		if !reflect.DeepEqual(tc.expectedAdds, fakeCertWriter.addedCerts) {
 			t.Errorf("Unexpected adds for %s, wanted: %v, got %v", k, tc.expectedAdds, fakeCertWriter.addedCerts)
 		}
 
+		sort.Strings(tc.expectedDeletes)
+		sort.Strings(fakeCertWriter.deletedCerts)
 		if !reflect.DeepEqual(tc.expectedDeletes, fakeCertWriter.deletedCerts) {
 			t.Errorf("Unexpected deletes for %s, wanted: %v, got %v", k, tc.expectedDeletes, fakeCertWriter.deletedCerts)
 		}

Diff for: pkg/router/template/router.go

@@ -394,6 +394,13 @@ func (r *templateRouter) writeConfig() error {
 		r.state[k] = cfg
 	}
 
+	glog.V(4).Infof("Committing router certificate manager changes...")
+	if err := r.certManager.Commit(); err != nil {
+		return fmt.Errorf("error committing certificate changes: %v", err)
+	}
+
+	glog.V(4).Infof("Router certificate manager config committed")
+
 	for path, template := range r.templates {
 		file, err := os.Create(path)
 		if err != nil {

Diff for: pkg/router/template/types.go

@@ -103,6 +103,8 @@ type certificateManager interface {
 	WriteCertificatesForConfig(config *ServiceAliasConfig) error
 	// DeleteCertificatesForConfig deletes all certificates for all ServiceAliasConfigs in config
 	DeleteCertificatesForConfig(config *ServiceAliasConfig) error
+	// Commit commits all the changes made to the certificateManager.
+	Commit() error
 	// CertificateWriter provides direct access to the underlying writer if required
 	CertificateWriter() certificateWriter
 }