Fix router reload errors for deleted certificates

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{}
 

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)
 		}

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 {

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
 }