Skip to content

Commit

Permalink
Fixing a bug with multipart requests, and closing #202
Browse files Browse the repository at this point in the history
  • Loading branch information
subnetmarco committed May 29, 2015
1 parent d612f65 commit 148580e
Show file tree
Hide file tree
Showing 7 changed files with 62 additions and 28 deletions.
6 changes: 6 additions & 0 deletions kong/plugins/basicauth/access.lua
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ local constants = require "kong.constants"

local _M = {}

local function skip_authentication(headers)
-- Skip upload request that expect a 100 Continue response
return headers["expect"] and stringy.startswith(headers["expect"], "100")
end

-- Fast lookup for credential retrieval depending on the type of the authentication
--
-- All methods must respect:
Expand Down Expand Up @@ -61,6 +66,7 @@ local function validate_credentials(credential, username, password)
end

function _M.execute(conf)
if not conf or skip_authentication(ngx.req.get_headers()) then return end
if not conf then return end

local username, password = retrieve_credentials(ngx.req, conf)
Expand Down
17 changes: 15 additions & 2 deletions kong/plugins/filelog/log.lua
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,21 @@ local cjson = require "cjson"

local _M = {}

function _M.execute()
ngx.log(ngx.INFO, cjson.encode(ngx.ctx.log_message))
-- Log to a file
-- @param `premature`
-- @param `conf` Configuration table, holds http endpoint details
-- @param `message` Message to be logged
local function log(premature, conf, message)
local f = io.open(conf.path, "a+")

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha May 29, 2015

Member

Are you opening and closing the file on every request? I think that is extremely bad. I don't know exactly how nginx logs to the files on disk but very probably not by doing so

This comment has been minimized.

Copy link
@subnetmarco

subnetmarco May 29, 2015

Author Member

That's what I though too, but we can figure it out. In one of my first implementations I was keeping a global file handler, but that's unable to detect file failures. So when it fails, nothing will be logged. I will check the nginx source code.

f:write(cjson.encode(message).."\n")
f:close()
end

function _M.execute(conf)
local ok, err = ngx.timer.at(0, log, conf, ngx.ctx.log_message)
if not ok then
ngx.log(ngx.ERR, "[filelog] failed to create timer: ", err)
end
end

return _M
19 changes: 18 additions & 1 deletion kong/plugins/filelog/schema.lua
Original file line number Diff line number Diff line change
@@ -1 +1,18 @@
return {} -- No schema
local IO = require "kong.tools.io"

local function validate_file(value)
local exists = IO.file_exists(value)
if not os.execute("touch "..value) == 0 then
return false, "Cannot create a file in the path specified. Make sure the path is valid, and Kong has the right permissions"
end

if not exists then
os.remove(value) -- Remove the created file if it didn't exist before
end

return true
end

return {
path = { required = true, type = "string", func = validate_file }
}
7 changes: 6 additions & 1 deletion kong/plugins/keyauth/access.lua
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ local MULTIPART_DATA = "multipart/form-data"

local _M = {}

local function skip_authentication(headers)
-- Skip upload request that expect a 100 Continue response
return headers["expect"] and stringy.startswith(headers["expect"], "100")
end

local function get_key_from_query(key_name, request, conf)
local key, parameters
local found_in = {}
Expand Down Expand Up @@ -111,7 +116,7 @@ local retrieve_credentials = {
}

function _M.execute(conf)
if not conf then return end
if not conf or skip_authentication(ngx.req.get_headers()) then return end

local credential
for _, v in ipairs({ constants.AUTHENTICATION.QUERY, constants.AUTHENTICATION.HEADER }) do
Expand Down
7 changes: 1 addition & 6 deletions kong/resolver/access.lua
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,9 @@ function _M.execute(conf)

-- Setting the backend URL for the proxy_pass directive
ngx.var.backend_url = get_backend_url(api)..ngx.var.request_uri

ngx.req.set_header("Host", get_host_from_url(ngx.var.backend_url))

-- There are some requests whose authentication needs to be skipped
if not skip_authentication(ngx.req.get_headers()) then
-- Saving these properties for the other plugins handlers
ngx.ctx.api = api
end
ngx.ctx.api = api
end

return _M
7 changes: 7 additions & 0 deletions kong/tools/io.lua
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,13 @@ function _M.write_to_file(path, value)
return true
end

function _M.file_size(path)
local file = io.open(path, "rb")
local size = file:seek("end")
file:close()
return size
end

function _M.retrieve_files(dir, options)
local fs = require "luarocks.fs"
local pattern = options.file_pattern
Expand Down
27 changes: 9 additions & 18 deletions spec/plugins/logging_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ local TCP_PORT = 20777
local UDP_PORT = 20778
local HTTP_PORT = 20779

local FILE_LOG_PATH = "/tmp/file_log_spec_output.log"

describe("Logging Plugins", function()

setup(function()
Expand All @@ -31,7 +33,7 @@ describe("Logging Plugins", function()
{ name = "tcplog", value = { host = "127.0.0.1", port = TCP_PORT }, __api = 1 },
{ name = "udplog", value = { host = "127.0.0.1", port = UDP_PORT }, __api = 2 },
{ name = "httplog", value = { http_endpoint = "http://localhost:"..HTTP_PORT }, __api = 3 },
{ name = "filelog", value = {}, __api = 4 }
{ name = "filelog", value = { path = FILE_LOG_PATH }, __api = 4 }
}
}

Expand Down Expand Up @@ -97,6 +99,8 @@ describe("Logging Plugins", function()
end)

it("should log to file", function()
os.remove(FILE_LOG_PATH)

local uuid = string.gsub(uuid(), "-", "")

-- Making the request
Expand All @@ -105,25 +109,12 @@ describe("Logging Plugins", function()
)
assert.are.equal(200, status)

-- Reading the log file and finding the line with the entry
local configuration = yaml.load(IO.read_file(TEST_CONF))
assert.truthy(configuration)
local error_log = IO.read_file(configuration.nginx_working_dir.."/logs/error.log")
local line
local lines = stringy.split(error_log, "\n")
for _, v in ipairs(lines) do
if string.find(v, uuid, nil, true) then
line = v
break
end
while not (IO.file_exists(FILE_LOG_PATH) and IO.file_size(FILE_LOG_PATH) > 0) do
-- Wait for the file to be created, and for the log to be appended
end
assert.truthy(line)

-- Retrieve the JSON part of the line
local json_str = line:match("(%{.*%})")
assert.truthy(json_str)

local log_message = cjson.decode(json_str)
local file_log = IO.read_file(FILE_LOG_PATH)
local log_message = cjson.decode(stringy.strip(file_log))
assert.are.same("127.0.0.1", log_message.client_ip)
assert.are.same(uuid, log_message.request.headers.file_log_uuid)
end)
Expand Down

2 comments on commit 148580e

@thibaultcha
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please describe what the bug was in commit messages... Easier to write the Changelog after

@subnetmarco
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On multipart requests when a client sends a Expect: 100-continue header, Kong stopped executing any subsequent phase.

Please sign in to comment.