properly handle default and custom default certs in dynamic ssl mode

cmd/nginx/main.go

@@ -55,7 +55,7 @@ const (
 	// client code is overriding it.
 	defaultBurst = 1e6
 
-	fakeCertificate = "default-fake-certificate"
+	fakeCertificateName = "default-fake-certificate"
 )
 
 func main() {
@@ -116,12 +116,12 @@ func main() {
 	if err != nil {
 		klog.Fatalf("unexpected error creating fake SSL Cert: %v", err)
 	}
-	err = ssl.StoreSSLCertOnDisk(fs, fakeCertificate, sslCert)
+	err = ssl.StoreSSLCertOnDisk(fs, fakeCertificateName, sslCert)
 	if err != nil {
 		klog.Fatalf("unexpected error storing fake SSL Cert: %v", err)
 	}
-	conf.FakeCertificatePath = sslCert.PemFileName
+	conf.FakeCertificate = sslCert
-	conf.FakeCertificateSHA = sslCert.PemSHA
+	klog.Infof("Created fake certificate with PemFileName: %v", conf.FakeCertificate.PemFileName)
 	// end create default fake SSL certificates
 
 	conf.Client = kubeClient

internal/ingress/controller/controller.go

@@ -88,8 +88,7 @@ type Configuration struct {
 
 	EnableSSLChainCompletion bool
 
-	FakeCertificatePath string
+	FakeCertificate *ingress.SSLCert
-	FakeCertificateSHA  string
 
 	SyncRateLimit float32
 
@@ -829,6 +828,21 @@ func (n *NGINXController) serviceEndpoints(svcKey, backendPort string) ([]ingres
 	return upstreams, nil
 }
 
+// overridePemFileNameAndPemSHA should only be called when DynamicCertificatesEnabled
+// ideally this function should not exist, the only reason why we use it is that
+// we rely on PemFileName in nginx.tmpl to configure SSL directives
+// and PemSHA to force reload
+func (n *NGINXController) overridePemFileNameAndPemSHA(cert *ingress.SSLCert) {
+	// TODO(elvinefendi): It is not great but we currently use PemFileName to decide whether SSL needs to be configured
+	// in nginx configuration or not. The whole thing needs to be refactored, we should rely on a proper
+	// signal to configure SSL, not PemFileName.
+	cert.PemFileName = n.cfg.FakeCertificate.PemFileName
+
+	// TODO(elvinefendi): This is again another hacky way of avoiding Nginx reload when certificate
+	// changes in dynamic SSL mode since FakeCertificate never changes.
+	cert.PemSHA = n.cfg.FakeCertificate.PemSHA
+}
+
 // createServers builds a map of host name to Server structs from a map of
 // already computed Upstream structs. Each Server is configured with at least
 // one root location, which uses a default backend if left unspecified.
@@ -856,16 +870,16 @@ func (n *NGINXController) createServers(data []*ingress.Ingress,
 		ProxyBuffering:    bdef.ProxyBuffering,
 	}
 
-	// generated on Start() with createDefaultSSLCertificate()
+	defaultCertificate := n.cfg.FakeCertificate
-	defaultPemFileName := n.cfg.FakeCertificatePath
-	defaultPemSHA := n.cfg.FakeCertificateSHA
 
 	// read custom default SSL certificate, fall back to generated default certificate
 	if n.cfg.DefaultSSLCertificate != "" {
-		defaultCertificate, err := n.store.GetLocalSSLCert(n.cfg.DefaultSSLCertificate)
+		certificate, err := n.store.GetLocalSSLCert(n.cfg.DefaultSSLCertificate)
 		if err == nil {
-			defaultPemFileName = defaultCertificate.PemFileName
+			defaultCertificate = certificate
-			defaultPemSHA = defaultCertificate.PemSHA
+			if n.cfg.DynamicCertificatesEnabled {
+				n.overridePemFileNameAndPemSHA(defaultCertificate)
+			}
 		} else {
 			klog.Warningf("Error loading custom default certificate, falling back to generated default:\n%v", err)
 		}
@@ -874,10 +888,7 @@ func (n *NGINXController) createServers(data []*ingress.Ingress,
 	// initialize default server and root location
 	servers[defServerName] = &ingress.Server{
 		Hostname: defServerName,
-		SSLCert: ingress.SSLCert{
+		SSLCert:  *defaultCertificate,
-			PemFileName: defaultPemFileName,
-			PemSHA:      defaultPemSHA,
-		},
 		Locations: []*ingress.Location{
 			{
 				Path:         rootLocation,
@@ -1021,8 +1032,7 @@ func (n *NGINXController) createServers(data []*ingress.Ingress,
 
 			if tlsSecretName == "" {
 				klog.V(3).Infof("Host %q is listed in the TLS section but secretName is empty. Using default certificate.", host)
-				servers[host].SSLCert.PemFileName = defaultPemFileName
+				servers[host].SSLCert = *defaultCertificate
-				servers[host].SSLCert.PemSHA = defaultPemSHA
 				continue
 			}
 
@@ -1030,8 +1040,7 @@ func (n *NGINXController) createServers(data []*ingress.Ingress,
 			cert, err := n.store.GetLocalSSLCert(secrKey)
 			if err != nil {
 				klog.Warningf("Error getting SSL certificate %q: %v. Using default certificate", secrKey, err)
-				servers[host].SSLCert.PemFileName = defaultPemFileName
+				servers[host].SSLCert = *defaultCertificate
-				servers[host].SSLCert.PemSHA = defaultPemSHA
 				continue
 			}
 
@@ -1046,16 +1055,13 @@ func (n *NGINXController) createServers(data []*ingress.Ingress,
 					klog.Warningf("SSL certificate %q does not contain a Common Name or Subject Alternative Name for server %q: %v",
 						secrKey, host, err)
 					klog.Warningf("Using default certificate")
-					servers[host].SSLCert.PemFileName = defaultPemFileName
+					servers[host].SSLCert = *defaultCertificate
-					servers[host].SSLCert.PemSHA = defaultPemSHA
 					continue
 				}
 			}
 
 			if n.cfg.DynamicCertificatesEnabled {
-				// useless placeholders: just to shut up NGINX configuration loader errors:
+				n.overridePemFileNameAndPemSHA(cert)
-				cert.PemFileName = defaultPemFileName
-				cert.PemSHA = defaultPemSHA
 			}
 
 			servers[host].SSLCert = *cert

internal/ingress/controller/store/backend_ssl.go

@@ -103,7 +103,7 @@ func (s *k8sStore) getPemCertificate(secretName string) (*ingress.SSLCert, error
 			return nil, fmt.Errorf("unexpected error creating SSL Cert: %v", err)
 		}
 
-		if !s.isDynamicCertificatesEnabled || len(ca) > 0 || s.defaultSSLCertificate == secretName {
+		if !s.isDynamicCertificatesEnabled || len(ca) > 0 {
 			err = ssl.StoreSSLCertOnDisk(s.filesystem, nsSecName, sslCert)
 			if err != nil {
 				return nil, fmt.Errorf("error while storing certificate and key: %v", err)