Merge pull request #4594 from Shopify/cleanup-certs

cleanup unused certificates
kubernetes · Sep 24, 2019 · 73bc2cf · 73bc2cf
2 parents 1dc4d18 + e392c8a
commit 73bc2cf
Show file tree

Hide file tree

Showing 4 changed files with 92 additions and 52 deletions.
diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go
@@ -67,6 +67,7 @@ import (
 
 const (
 	tempNginxPattern = "nginx-cfg"
+	emptyUID         = "-1"
 )
 
 // NewNGINXController creates a new NGINX Ingress controller.
@@ -1004,39 +1005,35 @@ func configureCertificates(rawServers []*ingress.Server) error {
 		Servers:      map[string]string{},
 	}
 
-	for _, rawServer := range rawServers {
-		if rawServer.SSLCert == nil {
-			continue
-		}
+	configure := func(hostname string, sslCert *ingress.SSLCert) {
+		uid := emptyUID
 
-		uid := rawServer.SSLCert.UID
+		if sslCert != nil {
+			uid = sslCert.UID
 
-		if _, ok := configuration.Certificates[uid]; !ok {
-			configuration.Certificates[uid] = rawServer.SSLCert.PemCertKey
+			if _, ok := configuration.Certificates[uid]; !ok {
+				configuration.Certificates[uid] = sslCert.PemCertKey
+			}
 		}
 
-		configuration.Servers[rawServer.Hostname] = uid
+		configuration.Servers[hostname] = uid
+	}
+
+	for _, rawServer := range rawServers {
+		configure(rawServer.Hostname, rawServer.SSLCert)
 
 		for _, alias := range rawServer.Aliases {
-			if !ssl.IsValidHostname(alias, rawServer.SSLCert.CN) {
-				continue
+			if rawServer.SSLCert != nil && ssl.IsValidHostname(alias, rawServer.SSLCert.CN) {
+				configuration.Servers[alias] = rawServer.SSLCert.UID
+			} else {
+				configuration.Servers[alias] = emptyUID
 			}
-
-			configuration.Servers[alias] = uid
 		}
 	}
 
 	redirects := buildRedirects(rawServers)
 	for _, redirect := range redirects {
-		if redirect.SSLCert == nil {
-			continue
-		}
-
-		configuration.Servers[redirect.From] = redirect.SSLCert.UID
-
-		if _, ok := configuration.Certificates[redirect.SSLCert.UID]; !ok {
-			configuration.Certificates[redirect.SSLCert.UID] = redirect.SSLCert.PemCertKey
-		}
+		configure(redirect.From, redirect.SSLCert)
 	}
 
 	statusCode, _, err := nginx.NewPostStatusRequest("/configuration/servers", "application/json", configuration)

diff --git a/internal/ingress/controller/nginx_test.go b/internal/ingress/controller/nginx_test.go
@@ -205,7 +205,7 @@ func TestConfigureDynamically(t *testing.T) {
 					}
 				case "/configuration/servers":
 					{
-						if !strings.Contains(body, `{"certificates":{},"servers":{}}`) {
+						if !strings.Contains(body, `{"certificates":{},"servers":{"myapp.fake":"-1"}}`) {
 							t.Errorf("controllerPodsCount should be present in JSON content: %v", body)
 						}
 					}
@@ -330,13 +330,18 @@ func TestConfigureCertificates(t *testing.T) {
 	}
 	defer streamListener.Close()
 
-	servers := []*ingress.Server{{
-		Hostname: "myapp.fake",
-		SSLCert: &ingress.SSLCert{
-			PemCertKey: "fake-cert",
-			UID:        "c89a5111-b2e9-4af8-be19-c2a4a924c256",
+	servers := []*ingress.Server{
+		{
+			Hostname: "myapp.fake",
+			SSLCert: &ingress.SSLCert{
+				PemCertKey: "fake-cert",
+				UID:        "c89a5111-b2e9-4af8-be19-c2a4a924c256",
+			},
 		},
-	}}
+		{
+			Hostname: "myapp.nossl",
+		},
+	}
 
 	server := &httptest.Server{
 		Listener: listener,
@@ -363,8 +368,14 @@ func TestConfigureCertificates(t *testing.T) {
 				}
 
 				for _, server := range servers {
-					if server.SSLCert.UID != conf.Servers[server.Hostname] {
-						t.Errorf("Expected servers and posted servers to be equal")
+					if server.SSLCert == nil {
+						if conf.Servers[server.Hostname] != emptyUID {
+							t.Errorf("Expected server %s to have UID of %s but got %s", server.Hostname, emptyUID, conf.Servers[server.Hostname])
+						}
+					} else {
+						if server.SSLCert.UID != conf.Servers[server.Hostname] {
+							t.Errorf("Expected server %s to have UID of %s but got %s", server.Hostname, server.SSLCert.UID, conf.Servers[server.Hostname])
+						}
 					}
 				}
 			}),

diff --git a/rootfs/etc/nginx/lua/configuration.lua b/rootfs/etc/nginx/lua/configuration.lua
@@ -5,6 +5,8 @@ local configuration_data = ngx.shared.configuration_data
 local certificate_data = ngx.shared.certificate_data
 local certificate_servers = ngx.shared.certificate_servers
 
+local EMPTY_UID = "-1"
+
 local _M = {}
 
 function _M.get_backends_data()
@@ -63,15 +65,21 @@ local function handle_servers()
   local err_buf = {}
 
   for server, uid in pairs(configuration.servers) do
-    local success, set_err, forcible = certificate_servers:set(server, uid)
-    if not success then
-      local err_msg = string.format("error setting certificate for %s: %s\n", server, tostring(set_err))
-      table.insert(err_buf, err_msg)
-    end
-    if forcible then
-      local msg = string.format("certificate_servers dictionary is full, LRU entry has been removed to store %s",
-        server)
-      ngx.log(ngx.WARN, msg)
+    if uid == EMPTY_UID then
+      -- notice that we do not delete certificate corresponding to this server
+      -- this is becase a certificate can be used by multiple servers/hostnames
+      certificate_servers:delete(server)
+    else
+      local success, set_err, forcible = certificate_servers:set(server, uid)
+      if not success then
+        local err_msg = string.format("error setting certificate for %s: %s\n", server, tostring(set_err))
+        table.insert(err_buf, err_msg)
+      end
+      if forcible then
+        local msg = string.format("certificate_servers dictionary is full, LRU entry has been removed to store %s",
+          server)
+        ngx.log(ngx.WARN, msg)
+      end
     end
   end
 

diff --git a/rootfs/etc/nginx/lua/test/configuration_test.lua b/rootfs/etc/nginx/lua/test/configuration_test.lua
@@ -166,6 +166,16 @@ describe("Configuration", function()
 
   describe("handle_servers()", function()
     local UUID = "2ea8adb5-8ebb-4b14-a79b-0cdcd892e884"
+
+    local function mock_ssl_configuration(configuration)
+      local json = cjson.encode(configuration)
+      ngx.req.get_body_data = function() return json end
+    end
+
+    before_each(function()
+      ngx.var.request_method = "POST"
+    end)
+
     it("should not accept non POST methods", function()
       ngx.var.request_method = "GET"
 
@@ -175,32 +185,49 @@ describe("Configuration", function()
       assert.same(ngx.status, ngx.HTTP_BAD_REQUEST)
     end)
 
+    it("deletes server with empty UID without touching the corresponding certificate", function()
+      mock_ssl_configuration({
+        servers = { ["hostname"] = UUID },
+        certificates = { [UUID] = "pemCertKey" }
+      })
+      assert.has_no.errors(configuration.handle_servers)
+      assert.same("pemCertKey", certificate_data:get(UUID))
+      assert.same(UUID, certificate_servers:get("hostname"))
+      assert.same(ngx.HTTP_CREATED, ngx.status)
+
+      local EMPTY_UID = "-1"
+      mock_ssl_configuration({
+        servers = { ["hostname"] = EMPTY_UID },
+        certificates = { [UUID] = "pemCertKey" }
+      })
+      assert.has_no.errors(configuration.handle_servers)
+      assert.same("pemCertKey", certificate_data:get(UUID))
+      assert.same(nil, certificate_servers:get("hostname"))
+      assert.same(ngx.HTTP_CREATED, ngx.status)
+    end)
+
     it("should successfully update certificates and keys for each host", function()
-      ngx.var.request_method = "POST"
-      local mock_ssl_configuration = cjson.encode({
+      mock_ssl_configuration({
         servers = { ["hostname"] = UUID },
         certificates = { [UUID] = "pemCertKey" }
       })
-      ngx.req.get_body_data = function() return mock_ssl_configuration end
 
       assert.has_no.errors(configuration.handle_servers)
-      assert.same(certificate_data:get(UUID), "pemCertKey")
-      assert.same(certificate_servers:get("hostname"), UUID)
-      assert.same(ngx.status, ngx.HTTP_CREATED)
+      assert.same("pemCertKey", certificate_data:get(UUID))
+      assert.same(UUID, certificate_servers:get("hostname"))
+      assert.same(ngx.HTTP_CREATED, ngx.status)
     end)
 
     it("should log an err and set status to Internal Server Error when a certificate cannot be set", function()
       local uuid2 = "8ea8adb5-8ebb-4b14-a79b-0cdcd892e999"
-      ngx.var.request_method = "POST"
       ngx.shared.certificate_data.set = function(self, uuid, certificate)
         return false, "error", nil
       end
 
-      local mock_ssl_configuration = cjson.encode({
+      mock_ssl_configuration({
         servers = { ["hostname"] = UUID, ["hostname2"] = uuid2 },
         certificates = { [UUID] = "pemCertKey", [uuid2] = "pemCertKey2" }
       })
-      ngx.req.get_body_data = function() return mock_ssl_configuration end
 
       local s = spy.on(ngx, "log")
       assert.has_no.errors(configuration.handle_servers)
@@ -213,18 +240,15 @@ describe("Configuration", function()
       local uuid2 = "8ea8adb5-8ebb-4b14-a79b-0cdcd892e999"
       local stored_entries = {}
 
-      ngx.var.request_method = "POST"
       ngx.shared.certificate_data.set = function(self, uuid, certificate)
         stored_entries[uuid] = certificate
         return true, nil, true
       end
-      local mock_ssl_configuration = cjson.encode({
+      mock_ssl_configuration({
         servers = { ["hostname"] = UUID, ["hostname2"] = uuid2 },
         certificates = { [UUID] = "pemCertKey", [uuid2] = "pemCertKey2" }
       })
 
-      ngx.req.get_body_data = function() return mock_ssl_configuration end
-
       local s1 = spy.on(ngx, "log")
       assert.has_no.errors(configuration.handle_servers)
       assert.spy(s1).was_called_with(ngx.WARN, string.format("certificate_data dictionary is full, LRU entry has been removed to store %s", UUID))