diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go index fd4bc78a82..1a7fe28098 100644 --- 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 index b7357da313..56ad14f779 100644 --- 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 index 02c546ec8c..7609117a27 100644 --- 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 index add5c90987..3bfe60db6f 100644 --- 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))