Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logging policy #856

Merged
merged 5 commits into from
Aug 28, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- Conditions for the limits of the rate-limit policy [PR #839](https://github.com/3scale/apicast/pull/839)
- `bin/apicast console` to start Lua REPL with APIcast code loaded [PR #853](https://github.com/3scale/apicast/pull/853)
- Liquid Context Debugging policy. It's a policy only meant for debugging purposes, returns the context available when evaluating liquid [PR #849](https://github.com/3scale/apicast/pull/849)
- Logging policy. It allows to enable/disable access logs per service [PR #856](https://github.com/3scale/apicast/pull/856), [THREESCALE-1148](https://issues.jboss.org/browse/THREESCALE-1148)

### Changed

Expand Down
3 changes: 2 additions & 1 deletion gateway/conf/nginx.conf.liquid
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,8 @@ http {

server {

access_log {{ access_log_file | default: "/dev/stdout" }} time;
set $access_logs_enabled '1';
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure about the quotes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's correct. From the docs:
The if parameter (1.7.0) enables conditional logging. A request will not be logged if the condition evaluates to “0” or an empty string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. But what I meant is that set value should not be quoted. Now the value is '1', not 1.
It is a bit confusing (even though it will still work).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I wanted it to be a string because the docs mention "0" as a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So just to be clear, I inspected the value of the ngx.var and it's the string "1", not the string "'1'". Not sure if that's what you meant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. There is some magic going on.

Both these are the same value (string):

    set $access_log_enabled '1';
    set $access_log_enabled_num 1;

I thought we had a PR to strip '' from some of the values because it was generating quotes. My bad I guess.

access_log {{ access_log_file | default: "/dev/stdout" }} time if=$access_logs_enabled;

{%- assign http_port = port.apicast | default: 8080 %}
{%- assign https_port = env.APICAST_HTTPS_PORT %}
Expand Down
19 changes: 19 additions & 0 deletions gateway/src/apicast/policy/logging/apicast-policy.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"$schema": "http://apicast.io/policy-v1/schema#manifest#",
"name": "Logging",
"summary": "Controls logging",
"description": [
"Controls logging. It allows to enable and disable access logs per ",
"service."
],
"version": "builtin",
"configuration": {
"type": "object",
"properties": {
"enable_access_logs": {
"description": "Whether to enable access logs for the service",
"type": "boolean"
}
}
}
}
1 change: 1 addition & 0 deletions gateway/src/apicast/policy/logging/init.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
return require('logging')
40 changes: 40 additions & 0 deletions gateway/src/apicast/policy/logging/logging.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
--- Logging policy

local _M = require('apicast.policy').new('Logging Policy')

local new = _M.new

local default_enable_access_logs = true

-- Defined in ngx.conf.liquid and used in the 'access_logs' directive.
local ngx_var_access_logs_enabled = 'access_logs_enabled'

-- Returns the value for the ngx var above from a boolean that indicates
-- whether access logs are enabled or not.
local val_for_ngx_var ={
[true] = '1',
[false] = '0'
}

function _M.new(config)
local self = new(config)

local enable_access_logs = config.enable_access_logs
if enable_access_logs == nil then -- Avoid overriding when it's false.
enable_access_logs = default_enable_access_logs
end

if not enable_access_logs then
ngx.log(ngx.DEBUG, 'Disabling access logs')
end

self.enable_access_logs_val = val_for_ngx_var[enable_access_logs]

return self
end

function _M:log()
ngx.var[ngx_var_access_logs_enabled] = self.enable_access_logs_val
end

return _M
39 changes: 39 additions & 0 deletions spec/policy/logging/logging_spec.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
local LoggingPolicy = require('apicast.policy.logging')

describe('Logging policy', function()
describe('.log', function()
before_each(function()
ngx.var = {}
end)

context('when access logs are enabled', function()
it('sets ngx.var.access_logs_enabled to "1"', function()
local logging = LoggingPolicy.new({ enable_access_logs = true })

logging:log()

assert.equals('1', ngx.var.access_logs_enabled)
end)
end)

context('when access logs are disabled', function()
it('sets ngx.var.enable_access_logs to "0"', function()
local logging = LoggingPolicy.new({ enable_access_logs = false })

logging:log()

assert.equals('0', ngx.var.access_logs_enabled)
end)
end)

context('when access logs are not configured', function()
it('enables them by default by setting ngx.var.enable_access_logs to "1"', function()
local logging = LoggingPolicy.new({})

logging:log()

assert.equals('1', ngx.var.access_logs_enabled)
end)
end)
end)
end)
133 changes: 133 additions & 0 deletions t/apicast-policy-logging.t
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
use lib 't';
use Test::APIcast::Blackbox 'no_plan';

# Test::Nginx does not allow to grep access logs, so we redirect them to
# stderr to be able to use "grep_error_log" by setting APICAST_ACCESS_LOG_FILE
$ENV{APICAST_ACCESS_LOG_FILE} = "$Test::Nginx::Util::ErrLogFile";

run_tests();

__DATA__

=== TEST 1: Enables access logs when configured to do so
--- configuration
{
"services": [
{
"id": 42,
"proxy": {
"policy_chain": [
{
"name": "apicast.policy.logging",
"configuration": {
"enable_access_logs": true
}
},
{
"name": "apicast.policy.upstream",
"configuration":
{
"rules": [ { "regex": "/", "url": "http://echo" } ]
}
}
]
}
}
]
}
--- upstream
location / {
content_by_lua_block {
ngx.say('yay, api backend');
}
}
--- request
GET /
--- error_code: 200
--- grep_error_log eval
qr/"GET \W+ HTTP\/1.1" 200/
--- grep_error_log_out
"GET / HTTP/1.1" 200
--- no_error_log
[error]

=== TEST 2: Disables access logs when configured to do so
--- configuration
{
"services": [
{
"id": 42,
"proxy": {
"policy_chain": [
{
"name": "apicast.policy.logging",
"configuration": {
"enable_access_logs": false
}
},
{
"name": "apicast.policy.upstream",
"configuration":
{
"rules": [ { "regex": "/", "url": "http://echo" } ]
}
}
]
}
}
]
}
--- upstream
location / {
content_by_lua_block {
ngx.say('yay, api backend');
}
}
--- request
GET /
--- error_code: 200
--- grep_error_log eval
qr/"GET \W+ HTTP\/1.1" 200/
--- grep_error_log_out
--- no_error_log
[error]

=== TEST 3: Enables access logs by default when the policy is included
--- configuration
{
"services": [
{
"id": 42,
"proxy": {
"policy_chain": [
{
"name": "apicast.policy.logging",
"configuration": { }
},
{
"name": "apicast.policy.upstream",
"configuration":
{
"rules": [ { "regex": "/", "url": "http://echo" } ]
}
}
]
}
}
]
}
--- upstream
location / {
content_by_lua_block {
ngx.say('yay, api backend');
}
}
--- request
GET /
--- error_code: 200
--- grep_error_log eval
qr/"GET \W+ HTTP\/1.1" 200/
--- grep_error_log_out
"GET / HTTP/1.1" 200
--- no_error_log
[error]