Merge pull request #18587 from ramr/fix-reload-cert-deleted

Automatic merge from submit-queue (batch tested with PRs 18587, 18296, 18667, 18665, 18532).

Fix router reload errors for deleted certificates

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. 
See associated error log: 
https://gist.github.com/ramr/122d70591d1fb8f97820869f7ca5550f

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.

To recreate:  
Just create and delete a number of routes in a loop ala:
```
function create_routes() {
  local n=${1:-$NROUTES}
  for i in `seq $n`; do
    echo "  - Creating route header-test-route-$i ... "
    sed "s/%1/$i/g" route.json.template | oc create -f -
  done
}

function delete_routes() {
  local n=${1:-$NROUTES}
  for i in `seq $n`; do
    echo "  - Deleting route header-test-route-$i ... "
    oc delete route header-test-route-$i
    # [ $((i%50)) == 0 ] && sleep 2
  done
}
```

where `route.json.template` is just a route with the `metadata.name`, `id` and `spec.host` fields containing some text (`%1`) that gets substituted.

@knobunc @rajatchopra  PTAL Thx

Author: openshift-merge-robot

pkg/router/template/certmanager.go

@@ -12,10 +12,24 @@ import (
 	routeapi "github.com/openshift/origin/pkg/route/apis/route"
 )
 
+// certificateFile represents a certificate file.
+type certificateFile struct {
+	certDir string
+	id      string
+}
+
+// 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")
+}
+
 // 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 +41,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 +95,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 +108,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 +119,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 +130,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 +140,38 @@ 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 {
+			// 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. 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
+}
+
 // 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
 }