-
Notifications
You must be signed in to change notification settings - Fork 170
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
Logging policy #856
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
f7230af
nginx.conf.liquid: add param to enable/disable access logs
davidor 4329f50
Add Logging policy
davidor 7e9de95
spec/policy: add specs for the logging policy
davidor 89a8dc5
t: add tests for the logging policy
davidor 74b263d
CHANGELOG: add logging policy
davidor File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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" | ||
} | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
return require('logging') |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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] |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'
, not1
.It is a bit confusing (even though it will still work).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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):
I thought we had a PR to strip
''
from some of the values because it was generating quotes. My bad I guess.