From f7a72276f2069e6fbad5e265284851021f94c367 Mon Sep 17 00:00:00 2001 From: ramr Date: Thu, 8 Feb 2018 21:39:59 -0800 Subject: [PATCH 1/2] 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. --- pkg/router/template/certmanager.go | 62 +++++++++++++++++++++---- pkg/router/template/certmanager_test.go | 19 ++++++++ pkg/router/template/router.go | 7 +++ pkg/router/template/types.go | 2 + 4 files changed, 80 insertions(+), 10 deletions(-) diff --git a/pkg/router/template/certmanager.go b/pkg/router/template/certmanager.go index 2b0bd44c6049..a92862ab9567 100644 --- a/pkg/router/template/certmanager.go +++ b/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 --git a/pkg/router/template/certmanager_test.go b/pkg/router/template/certmanager_test.go index 7bfcb4034ac9..e662ec20610b 100644 --- a/pkg/router/template/certmanager_test.go +++ b/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 --git a/pkg/router/template/router.go b/pkg/router/template/router.go index 29f191c9c776..fea48638971a 100644 --- a/pkg/router/template/router.go +++ b/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 --git a/pkg/router/template/types.go b/pkg/router/template/types.go index dd06561313ab..f6764684ebbb 100644 --- a/pkg/router/template/types.go +++ b/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 } From 456bb4bfc9db3b56ceb7b2290de127b347e89cf6 Mon Sep 17 00:00:00 2001 From: ramr Date: Tue, 13 Feb 2018 13:40:03 -0800 Subject: [PATCH 2/2] Cleanup comments, log delete failed message as a warning and fix exported names as per @{pravisankar,Miciah,knobunc} review comments. --- pkg/router/template/certmanager.go | 34 +++++++++++++----------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/pkg/router/template/certmanager.go b/pkg/router/template/certmanager.go index a92862ab9567..cffdd2054e4f 100644 --- a/pkg/router/template/certmanager.go +++ b/pkg/router/template/certmanager.go @@ -14,15 +14,14 @@ import ( // certificateFile represents a certificate file. type certificateFile struct { - CertDir string - ID string - Contents []byte + certDir string + id string } -// certificateFileTag generates a certificate file tag/name. This is used to -// index into the map of deleted certificates. +// Tag generates a certificate file tag/name. This is used to index into the +// the map of deleted certificates. func (cf certificateFile) Tag() string { - return filepath.Join(cf.CertDir, cf.ID+".pem") + return filepath.Join(cf.certDir, cf.id+".pem") } // simpleCertificateManager is the default implementation of a certificateManager @@ -96,7 +95,7 @@ func (cm *simpleCertificateManager) WriteCertificatesForConfig(config *ServiceAl buffer.Write([]byte(caCertObj.Contents)) } - certFile := certificateFile{CertDir: cm.cfg.certDir, ID: certObj.ID} + 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 @@ -109,7 +108,7 @@ func (cm *simpleCertificateManager) WriteCertificatesForConfig(config *ServiceAl destCert, ok := config.Certificates[destCertKey] if ok { - destCertFile := certificateFile{CertDir: cm.cfg.caCertDir, ID: destCert.ID} + 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 @@ -131,7 +130,7 @@ func (cm *simpleCertificateManager) DeleteCertificatesForConfig(config *ServiceA certObj, ok := config.Certificates[certKey] if ok { - certFile := certificateFile{CertDir: cm.cfg.certDir, ID: certObj.ID} + certFile := certificateFile{certDir: cm.cfg.certDir, id: certObj.ID} cm.deletedCertificates[certFile.Tag()] = certFile } } @@ -141,8 +140,7 @@ func (cm *simpleCertificateManager) DeleteCertificatesForConfig(config *ServiceA destCert, ok := config.Certificates[destCertKey] if ok { - - destCertFile := certificateFile{CertDir: cm.cfg.caCertDir, ID: destCert.ID} + destCertFile := certificateFile{certDir: cm.cfg.caCertDir, id: destCert.ID} cm.deletedCertificates[destCertFile.Tag()] = destCertFile } } @@ -157,21 +155,19 @@ func (cm *simpleCertificateManager) Commit() error { // 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) + 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 + // Log a warning if the delete fails but proceed on. + glog.Warningf("Ignoring error deleting certificate file %v: %v", certFile.Tag(), 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. + // write to the disk here. Today, the certificate writes are done + // just before this function is called. The tradeoff is storing a + // copy in memory until we commit. return nil }