You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2021/12/16 10:25:23 UTC
[GitHub] [apisix] guoqqqi opened a new pull request #5831: feat: add `file-logger` plugin
guoqqqi opened a new pull request #5831:
URL: https://github.com/apache/apisix/pull/5831
### What this PR does / why we need it:
1. add file-logger plugin
Related: Discuss on the [mailing list](https://mail.google.com/mail/u/0/?zx=ynbavjb165m7#inbox/QgrcJHsHnPNSqGvMKclFwpZBtkfXPgGBjTB)
### Pre-submission checklist:
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [ ] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [ ] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bisakhmondal commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
bisakhmondal commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r788432285
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,102 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message)
+ local file, err = io_open(conf.path, 'a+')
+
+ if not file then
+ core.log.error("failed to open file: ", conf.path, ", error info: ", err)
+ else
+ local ok, err = file:write(msg, '\n')
+ if not ok then
+ core.log.error("failed to write file: ", conf.path, ", error info: ", err)
+ else
+ file:flush()
+ end
+ file:close()
+ end
+end
Review comment:
Instead of opening a file handle for each time we dump a log, can we optimize it a bit by keeping the fd open and caching it so that it can be reused (maybe with a timeout, so that it doesn't remain open indefinitely [do we really care about it?])? Because you know, writing something is cheap but opening a file handler for every logs and moving the fd to the EOF takes some resources.
Maybe it's a premature optimization. @spacewander WDYT?
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,102 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message)
+ local file, err = io_open(conf.path, 'a+')
+
+ if not file then
+ core.log.error("failed to open file: ", conf.path, ", error info: ", err)
+ else
+ local ok, err = file:write(msg, '\n')
+ if not ok then
+ core.log.error("failed to write file: ", conf.path, ", error info: ", err)
+ else
+ file:flush()
+ end
+ file:close()
+ end
+end
+
+
+function _M.log(conf, ctx)
+ local metadata = plugin.plugin_metadata(plugin_name)
Review comment:
How about a gentle info log of the json serialized metadata after this line?
##########
File path: t/plugin/file-logger.t
##########
@@ -0,0 +1,204 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+no_long_string();
+no_root_location();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (! $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+
+ if (! $block->no_error_log && ! $block->error_log) {
+ $block->set_value("no_error_log", "[error]");
+ }
+});
+
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: sanity
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema({
+ path = "file.log"
+ })
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- response_body
+done
+
+
+
+=== TEST 2: path is missing check
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema()
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- no_error_log
+property "path" is required
Review comment:
We can go with table-driven checks?
https://github.com/apache/apisix/blob/d1d16f403c3ded1dc7a9390d0252200afc2591cc/t/plugin/splunk-hec-logging.t#L46-L99
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,102 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
Review comment:
we don't need this validation as the current file-logger schema doesn't contains any expression like `include_resp_body_expr` or `include_req_body_expr`
##########
File path: t/plugin/file-logger.t
##########
@@ -0,0 +1,204 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+no_long_string();
+no_root_location();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (! $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+
+ if (! $block->no_error_log && ! $block->error_log) {
+ $block->set_value("no_error_log", "[error]");
+ }
+});
+
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: sanity
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema({
+ path = "file.log"
+ })
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- response_body
+done
+
+
+
+=== TEST 2: path is missing check
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema()
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- no_error_log
+property "path" is required
+
+
+
+=== TEST 3: add plugin metadata
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/plugin_metadata/file-logger',
+ ngx.HTTP_PUT,
+ [[{
+ "log_format": {
+ "host": "$host",
+ "client_ip": "$remote_addr"
+ }
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(code..body)
+ }
+ }
+--- response_body
+201passed
+
+
+
+=== TEST 4: add plugin
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "file-logger": {
+ "path": "file.log"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1982": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 5: verify plugin
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local t = require("lib.test_admin").test
+ local code = t("/hello", ngx.HTTP_GET)
+ local fd, err = io.open("file.log", 'r')
+ local msg
+
+ if fd then
+ msg = fd:read()
+ else
+ core.log.error("failed to open file: file.log, error info: ", err)
+ end
+ fd:close()
+
+ local new_msg = core.json.decode(msg)
+ if new_msg.client_ip == '127.0.0.1' and new_msg.route_id == '1'
+ and new_msg.host == '127.0.0.1'
+ then
+ msg = "write file log success"
+ ngx.status = code
+ ngx.say(msg)
+ end
+ }
+ }
+--- response_body
+write file log success
+
+
+
+=== TEST 6: failed to open the path
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "file-logger": {
+ "path": "/log/file.log"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1982": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ end
+
+ local code, messages = t("/hello", GET)
+ core.log.warn("messages: ", messages)
+ if code >= 300 then
+ ngx.status = code
+ end
+ }
+ }
+--- error_log
+failed to open file: /log/file.log, error info: /log/file.log: No such file or directory
Review comment:
In such a case, how about we try to create a file with the corresponding name. If that returns an error [maybe insufficient permission], we throw it away. Else it would not be a good user experience to create a file each time they create a log configuration. What do you think about it? Thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] guoqqqi commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
guoqqqi commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r776560490
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,212 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+
+local C = ffi.C
+local ngx = ngx
+local pairs = pairs
+local load_string = loadstring
+local io_open = io.open
+
+local plugin_name = "file-logger"
+local O_CREAT = 00000040 -- create and open
Review comment:
The reason for using `ffi` to call `write` is that I do not want to use the cached `io.write`. For more information on this, see the:
1. https://m.yisu.com/zixun/194893.html
2. https://github.com/Kong/kong/blob/master/kong/plugins/file-log/handler.lua
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] guoqqqi commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
guoqqqi commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r785443788
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,99 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+local file
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string",
+ pattern = [[^[^*&%%\`]+$]],
+ err = "not a valid filename"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message) .. "\n"
+
+ if not file then
+ file = io_open(conf.path, 'a+')
Review comment:
Oops, I misunderstood here before, I thought this way of writing would cause the file to be reopened on every request
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r788318405
##########
File path: t/plugin/file-logger.t
##########
@@ -0,0 +1,137 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+no_long_string();
+no_root_location();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (! $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+
+ if (! $block->no_error_log && ! $block->error_log) {
+ $block->set_value("no_error_log", "[error]");
+ }
+});
+
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: sanity
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema({
+ path = "file.log"
+ })
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- response_body
+done
+
+
+
+=== TEST 2: path is missing check
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema()
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- no_error_log
+property "path" is required
+
+
+
+=== TEST 3: add plugin
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "file-logger": {
+ "path": "file.log"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1982": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 4: verify plugin
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local t = require("lib.test_admin").test
+ local code, message = t("/hello", ngx.HTTP_GET)
+ local fd, err = io.open("file.log", 'r')
+ local msg
+
+ if fd then
+ msg = fd:read()
+ fd:close()
+ else
+ fd:close()
+ core.log.error("failed to open file: logs/file.log, error info: ", err)
+ end
+ core.log.warn('msg: ', msg)
Review comment:
Better to check them all, we can custom the format to avoid timestamps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r776955967
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,186 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+local C = ffi.C
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+local O_CREAT = 00000040 -- create and open
+local O_APPEND = 00000400 -- add content to the end of
+local O_WRONLY = 00000001 -- write only open
+local S_IRUSR = 00400 -- user has read permission
+local S_IWUSR = 00200 -- user has write permission
+local S_IRGRP = 00040 -- group has read permission
+local S_IROTH = 00004 -- others have read permission
+local oflags = bit.bor(O_WRONLY, O_CREAT, O_APPEND)
+local mode = bit.bor(S_IRUSR, S_IWUSR, S_IRGRP, S_IROTH)
+local file_descriptors = {}
+
+
+ffi.cdef [[
+ int open(const char * filename, int flags, int mode);
+ int write(int fd, const void * ptr, int numbytes);
+ int close(int fd);
+]]
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string",
+ match = [[^[^*&%%\`]+$]],
+ err = "not a valid filename"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function reopen()
+ local target_pointer, err = io_open('logs/file_pointer', 'r')
Review comment:
There is no need to track the status with a file in disk? We can just iterate all cache fds and reopen them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] guoqqqi commented on pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
guoqqqi commented on pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#issuecomment-997520694
TODO:
- [ ] add control API
- [ ] test
- [ ] docs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r785599890
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,97 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message) .. "\n"
+ local file, err = io_open(conf.path, 'a+')
+
+ if not file then
+ core.log.error("failed to open file: " .. conf.path .. ", error info: " .. err)
Review comment:
```suggestion
core.log.error("failed to open file: ", conf.path, ", error info: ", err)
```
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,97 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message) .. "\n"
+ local file, err = io_open(conf.path, 'a+')
+
+ if not file then
+ core.log.error("failed to open file: " .. conf.path .. ", error info: " .. err)
+ else
+ file:write(msg)
Review comment:
Need to handle the err.
And don't forget to close the file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] juzhiyuan commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r776537373
##########
File path: docs/zh/latest/plugins/file-logger.md
##########
@@ -0,0 +1,121 @@
+---
+title: file-logger
+---
+
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+## 目录
+
+- [**定义**](#定义)
+- [**属性列表**](#属性)
+- [**如何开启**](#如何启用)
+- [**测试插件**](#测试插件)
+- [**插件元数据设置**](#插件元数据设置)
+- [**禁用插件**](#禁用插件)
+
+## 定义
+
+`file-logger` 是一个插件，可将 Log 数据流推送到指定位置，例如，可以自定义输入路径：`logs/file.log`。
+
+## 属性列表
+
+| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
+| ---------------- | ------- | ------ | ------------- | ------- | ------------------------------------------------ |
+| path | string | 必须 | | | 自定义输入文件路径 |
+| custom_fields_by_lua | object | 可选 | | | 键值对列表，其中键是日志字段的名称，值为一段 `lua` 代码，其返回值设置或者替换日志字段值 |
+
+## 如何开启
+
+这是有关如何为特定路由启用 `file-logger` 插件的示例。
+
+```shell
+curl http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+ "plugins": {
+ "file-logger": {
+ "path": "logs/file.log",
+ "custom_fields_by_lua": {"route_id": "return nil"}
+ }
Review comment:
Hi, could we use 2 spaces here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #5831:
URL: https://github.com/apache/apisix/pull/5831
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] guoqqqi commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
guoqqqi commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r787585226
##########
File path: t/plugin/file-logger.t
##########
@@ -0,0 +1,137 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+no_long_string();
+no_root_location();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (! $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+
+ if (! $block->no_error_log && ! $block->error_log) {
+ $block->set_value("no_error_log", "[error]");
+ }
+});
+
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: sanity
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema({
+ path = "file.log"
+ })
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- response_body
+done
+
+
+
+=== TEST 2: path is missing check
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema()
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- no_error_log
+property "path" is required
+
+
+
+=== TEST 3: add plugin
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "file-logger": {
+ "path": "file.log"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1982": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 4: verify plugin
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local t = require("lib.test_admin").test
+ local code, message = t("/hello", ngx.HTTP_GET)
+ local fd, err = io.open("file.log", 'r')
+ local msg
+
+ if fd then
+ msg = fd:read()
+ fd:close()
+ else
+ fd:close()
+ core.log.error("failed to open file: logs/file.log, error info: ", err)
+ end
+ core.log.warn('msg: ', msg)
Review comment:
Do I need to check all the fields and values in the msg, or do I just need to check a few specific values?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r785684333
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -70,10 +70,15 @@ local function write_file_data(conf, log_message)
local file, err = io_open(conf.path, 'a+')
if not file then
- core.log.error("failed to open file: " .. conf.path .. ", error info: " .. err)
+ core.log.error("failed to open file: ", conf.path, ", error info: ", err)
else
- file:write(msg)
- file:flush()
+ local ok, error = file:write(msg)
Review comment:
```suggestion
local ok, err = file:write(msg)
```
and we need to close the file even failed to write.
BTW, we can use `file:write(msg, '\n')` to avoid `local msg = core.json.encode(log_message) .. "\n"`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r785441792
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,99 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+local file
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string",
+ pattern = [[^[^*&%%\`]+$]],
+ err = "not a valid filename"
Review comment:
We don't have `err` in the jsonschema. BTW, we don't need to verify the path with regex? Different filesystems will have different requirements.
##########
File path: docs/zh/latest/plugins/file-logger.md
##########
@@ -0,0 +1,119 @@
+---
+title: file-logger
+---
+
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+## 目录
+
+- [**定义**](#定义)
+- [**属性列表**](#属性)
+- [**如何开启**](#如何启用)
+- [**测试插件**](#测试插件)
+- [**插件元数据设置**](#插件元数据设置)
+- [**禁用插件**](#禁用插件)
+
+## 定义
+
+`file-logger` 是一个插件，可将 Log 数据流推送到指定位置，例如，可以自定义输出路径：`logs/file.log`。
+
+## 属性列表
+
+| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
+| ---------------- | ------- | ------ | ------------- | ------- | ------------------------------------------------ |
+| path | string | 必须 | | | 自定义输出文件路径 |
+
+## 如何开启
+
+这是有关如何为特定路由启用 `file-logger` 插件的示例。
+
+```shell
+curl http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+ "plugins": {
+ "file-logger": {
+ "path": "logs/file.log"
+ }
+ },
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:9001": 1
+ }
+ },
+ "uri": "/hello"
+}'
+```
+
+## 测试插件
+
+> 成功:
+
+```shell
+$ curl -i http://127.0.0.1:9080/hello
+HTTP/1.1 200 OK
+...
+hello, world
+```
+
+然后你可以在对应的 `logs` 目录下找到 `file.log` 文件，且该日志中现有字段的 `route_id` 被删除。初次接受请求时会额外生成一个 `file_pointer` 文件，它用来保存打开目标输出文件的指针。
+
+> 通过 control API 重新打开该日志文件
+
+```shell
+$ curl -i http://localhost:9090/v1/plugin/file-logger/reopen -X PUT
+```
+
+然后下次请求时，会将之前缓冲区所有的日志数据刷新，到我们刚刚自定义的输出日志文件中。
+
+## 插件元数据设置
+
+| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
+| ---------------- | ------- | ------ | ------------- | ------- | ------------------------------------------------ |
+| log_format | object | 可选 | {"host": "$host", "@timestamp": "$time_iso8601", "client_ip": "$remote_addr"} | | 以 JSON 格式的键值对来声明日志格式。对于值部分，仅支持字符串。如果是以 `$` 开头，则表明是要获取 __APISIX__ 变量或 [Nginx 内置变量](http://nginx.org/en/docs/varindex.html)。特别的，**该设置是全局生效的**，意味着指定 log_format 后，将对所有绑定 file-logger 的 Route 或 Service 生效。 |
+
+**APISIX 变量**
Review comment:
Please update the doc according to the latest http-logger.
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,99 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+local file
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string",
+ pattern = [[^[^*&%%\`]+$]],
+ err = "not a valid filename"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message) .. "\n"
+
+ if not file then
+ file = io_open(conf.path, 'a+')
Review comment:
```suggestion
local file, err = io_open(conf.path, 'a+')
```
is enough? And remember to check the err.
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,99 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
Review comment:
```suggestion
local plugin_name = "file-logger"
```
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,99 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+local file
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string",
+ pattern = [[^[^*&%%\`]+$]],
+ err = "not a valid filename"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message) .. "\n"
+
+ if not file then
+ file = io_open(conf.path, 'a+')
+ end
+
+ file:write(msg)
Review comment:
Ditto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] guoqqqi commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
guoqqqi commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r788602883
##########
File path: t/plugin/file-logger.t
##########
@@ -0,0 +1,204 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+no_long_string();
+no_root_location();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (! $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+
+ if (! $block->no_error_log && ! $block->error_log) {
+ $block->set_value("no_error_log", "[error]");
+ }
+});
+
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: sanity
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema({
+ path = "file.log"
+ })
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- response_body
+done
+
+
+
+=== TEST 2: path is missing check
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema()
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- no_error_log
+property "path" is required
+
+
+
+=== TEST 3: add plugin metadata
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/plugin_metadata/file-logger',
+ ngx.HTTP_PUT,
+ [[{
+ "log_format": {
+ "host": "$host",
+ "client_ip": "$remote_addr"
+ }
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(code..body)
+ }
+ }
+--- response_body
+201passed
+
+
+
+=== TEST 4: add plugin
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "file-logger": {
+ "path": "file.log"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1982": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 5: verify plugin
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local t = require("lib.test_admin").test
+ local code = t("/hello", ngx.HTTP_GET)
+ local fd, err = io.open("file.log", 'r')
+ local msg
+
+ if fd then
+ msg = fd:read()
+ else
+ core.log.error("failed to open file: file.log, error info: ", err)
+ end
+ fd:close()
+
+ local new_msg = core.json.decode(msg)
+ if new_msg.client_ip == '127.0.0.1' and new_msg.route_id == '1'
+ and new_msg.host == '127.0.0.1'
+ then
+ msg = "write file log success"
+ ngx.status = code
+ ngx.say(msg)
+ end
+ }
+ }
+--- response_body
+write file log success
+
+
+
+=== TEST 6: failed to open the path
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "file-logger": {
+ "path": "/log/file.log"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1982": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ end
+
+ local code, messages = t("/hello", GET)
+ core.log.warn("messages: ", messages)
+ if code >= 300 then
+ ngx.status = code
+ end
+ }
+ }
+--- error_log
+failed to open file: /log/file.log, error info: /log/file.log: No such file or directory
Review comment:
Good idea, as I'm not particularly familiar with it, can I finish it with reopen function later?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r787389352
##########
File path: docs/en/latest/plugins/file-logger.md
##########
@@ -0,0 +1,122 @@
+---
+title: file-logger
+---
+
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+## Summary
+
+- [**Name**](#name)
+- [**Attributes**](#attributes)
+- [**How To Enable**](#how-to-enable)
+- [**Test Plugin**](#test-plugin)
+- [**Metadata**](#metadata)
+- [**Disable Plugin**](#disable-plugin)
+
+## Name
+
+`file-logger` is a plugin which push a stream of Log data to a specified location. For example, the output path can be customized: `logs/file.log`.
Review comment:
```suggestion
`file-logger` is a plugin that pushes a stream of log data to a specified file. For example, the output path can be customized: `logs/file.log`.
```
##########
File path: t/plugin/file-logger.t
##########
@@ -0,0 +1,137 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+no_long_string();
+no_root_location();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (! $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+
+ if (! $block->no_error_log && ! $block->error_log) {
+ $block->set_value("no_error_log", "[error]");
+ }
+});
+
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: sanity
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema({
+ path = "file.log"
+ })
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- response_body
+done
+
+
+
+=== TEST 2: path is missing check
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema()
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- no_error_log
+property "path" is required
+
+
+
+=== TEST 3: add plugin
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "file-logger": {
+ "path": "file.log"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1982": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 4: verify plugin
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local t = require("lib.test_admin").test
+ local code, message = t("/hello", ngx.HTTP_GET)
+ local fd, err = io.open("file.log", 'r')
+ local msg
+
+ if fd then
+ msg = fd:read()
+ fd:close()
+ else
+ fd:close()
Review comment:
Ditto
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,103 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message)
+ local file, err = io_open(conf.path, 'a+')
+
+ if not file then
+ core.log.error("failed to open file: ", conf.path, ", error info: ", err)
+ else
+ local ok, err = file:write(msg, '\n')
+ if not ok then
+ file:close()
+ core.log.error("failed to write file: ", conf.path, ", error info: ", err)
+ else
+ file:flush()
+ file:close()
Review comment:
We can merge the two close into one
##########
File path: t/plugin/file-logger.t
##########
@@ -0,0 +1,137 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+no_long_string();
+no_root_location();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (! $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+
+ if (! $block->no_error_log && ! $block->error_log) {
+ $block->set_value("no_error_log", "[error]");
+ }
+});
+
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: sanity
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema({
+ path = "file.log"
+ })
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- response_body
+done
+
+
+
+=== TEST 2: path is missing check
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema()
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- no_error_log
+property "path" is required
+
+
+
+=== TEST 3: add plugin
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "file-logger": {
+ "path": "file.log"
Review comment:
Let's add a test that the path can't be opened.
##########
File path: t/plugin/file-logger.t
##########
@@ -0,0 +1,137 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+no_long_string();
+no_root_location();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (! $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+
+ if (! $block->no_error_log && ! $block->error_log) {
+ $block->set_value("no_error_log", "[error]");
+ }
+});
+
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: sanity
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema({
+ path = "file.log"
+ })
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- response_body
+done
+
+
+
+=== TEST 2: path is missing check
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema()
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- no_error_log
+property "path" is required
+
+
+
+=== TEST 3: add plugin
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "file-logger": {
+ "path": "file.log"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1982": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 4: verify plugin
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local t = require("lib.test_admin").test
+ local code, message = t("/hello", ngx.HTTP_GET)
+ local fd, err = io.open("file.log", 'r')
+ local msg
+
+ if fd then
+ msg = fd:read()
+ fd:close()
+ else
+ fd:close()
+ core.log.error("failed to open file: logs/file.log, error info: ", err)
+ end
+ core.log.warn('msg: ', msg)
Review comment:
Let's check the msg too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] guoqqqi commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
guoqqqi commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r788453348
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,102 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message)
+ local file, err = io_open(conf.path, 'a+')
+
+ if not file then
+ core.log.error("failed to open file: ", conf.path, ", error info: ", err)
+ else
+ local ok, err = file:write(msg, '\n')
+ if not ok then
+ core.log.error("failed to write file: ", conf.path, ", error info: ", err)
+ else
+ file:flush()
+ end
+ file:close()
+ end
+end
Review comment:
hai, @bisakhmondal I tried to add a reopen function to it to cache the `fd`, but I had some problems, so I wanted to finish it later. https://github.com/apache/apisix/pull/5831#discussion_r776956698
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r776956077
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,186 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+local C = ffi.C
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+local O_CREAT = 00000040 -- create and open
+local O_APPEND = 00000400 -- add content to the end of
+local O_WRONLY = 00000001 -- write only open
+local S_IRUSR = 00400 -- user has read permission
+local S_IWUSR = 00200 -- user has write permission
+local S_IRGRP = 00040 -- group has read permission
+local S_IROTH = 00004 -- others have read permission
+local oflags = bit.bor(O_WRONLY, O_CREAT, O_APPEND)
+local mode = bit.bor(S_IRUSR, S_IWUSR, S_IRGRP, S_IROTH)
+local file_descriptors = {}
+
+
+ffi.cdef [[
+ int open(const char * filename, int flags, int mode);
+ int write(int fd, const void * ptr, int numbytes);
+ int close(int fd);
+]]
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string",
+ match = [[^[^*&%%\`]+$]],
+ err = "not a valid filename"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function reopen()
+ local target_pointer, err = io_open('logs/file_pointer', 'r')
+ if not target_pointer then
+ core.log.error("failed to open file, error info: " .. err)
+ core.response.exit(400, {error_msg = "failed to open file, error info: " .. err})
Review comment:
```suggestion
return 400, {error_msg = "failed to open file, error info: " .. err}
```
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,186 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+local C = ffi.C
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+local O_CREAT = 00000040 -- create and open
+local O_APPEND = 00000400 -- add content to the end of
+local O_WRONLY = 00000001 -- write only open
+local S_IRUSR = 00400 -- user has read permission
+local S_IWUSR = 00200 -- user has write permission
+local S_IRGRP = 00040 -- group has read permission
+local S_IROTH = 00004 -- others have read permission
+local oflags = bit.bor(O_WRONLY, O_CREAT, O_APPEND)
+local mode = bit.bor(S_IRUSR, S_IWUSR, S_IRGRP, S_IROTH)
+local file_descriptors = {}
+
+
+ffi.cdef [[
+ int open(const char * filename, int flags, int mode);
+ int write(int fd, const void * ptr, int numbytes);
+ int close(int fd);
+]]
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string",
+ match = [[^[^*&%%\`]+$]],
+ err = "not a valid filename"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function reopen()
+ local target_pointer, err = io_open('logs/file_pointer', 'r')
+ if not target_pointer then
+ core.log.error("failed to open file, error info: " .. err)
+ core.response.exit(400, {error_msg = "failed to open file, error info: " .. err})
+ else
+ local pointer = target_pointer:read('*n')
+ target_pointer:close()
+ C.close(pointer)
+
+ local reset_pointer = io_open('logs/file_pointer', 'w+')
+ reset_pointer:write(-1)
+ reset_pointer:close()
+ end
+
+ core.response.exit(200, 'reopen file-logger successfully')
Review comment:
Better to return a JSON response
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r776954699
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,212 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+
+local C = ffi.C
+local ngx = ngx
+local pairs = pairs
+local load_string = loadstring
+local io_open = io.open
+
+local plugin_name = "file-logger"
+local O_CREAT = 00000040 -- create and open
Review comment:
To bypass the user-space cache, there is already a `flush` method for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r784478071
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,212 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+
+local C = ffi.C
+local ngx = ngx
+local pairs = pairs
+local load_string = loadstring
+local io_open = io.open
+
+local plugin_name = "file-logger"
+local O_CREAT = 00000040 -- create and open
Review comment:
The error msg indicates that you can't log a userdata.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r788346680
##########
File path: t/plugin/file-logger.t
##########
@@ -74,7 +74,46 @@ property "path" is required
-=== TEST 3: add plugin
+=== TEST 3: add plugin metadata
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/plugin_metadata/file-logger',
+ ngx.HTTP_PUT,
+ [[{
+ "log_format": {
+ "host": "$host",
+ "client_ip": "$remote_addr"
+ }
+ }]],
+ [[{
+ "node": {
+ "value": {
+ "log_format": {
+ "host": "$host",
+ "client_ip": "$remote_addr"
+ }
+ }
+ },
+ "action": "set"
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+--- request
Review comment:
Don't add redundant section
##########
File path: docs/en/latest/plugins/file-logger.md
##########
@@ -84,7 +84,7 @@ You can then find the `file.log` file in the corresponding `logs` directory.
### Example
```shell
-curl http://127.0.0.1:9080/apisix/admin/plugin_metadata/http-logger -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+curl http://127.0.0.1:9080/apisix/admin/plugin_metadata/file-logger -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
Review comment:
Please sync to Chinese doc
##########
File path: t/plugin/file-logger.t
##########
@@ -74,7 +74,46 @@ property "path" is required
-=== TEST 3: add plugin
+=== TEST 3: add plugin metadata
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/plugin_metadata/file-logger',
+ ngx.HTTP_PUT,
+ [[{
+ "log_format": {
+ "host": "$host",
+ "client_ip": "$remote_addr"
+ }
+ }]],
+ [[{
Review comment:
We don't need to check the response, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] guoqqqi commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
guoqqqi commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r788453348
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,102 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message)
+ local file, err = io_open(conf.path, 'a+')
+
+ if not file then
+ core.log.error("failed to open file: ", conf.path, ", error info: ", err)
+ else
+ local ok, err = file:write(msg, '\n')
+ if not ok then
+ core.log.error("failed to write file: ", conf.path, ", error info: ", err)
+ else
+ file:flush()
+ end
+ file:close()
+ end
+end
Review comment:
Hai, @bisakhmondal I tried to add a reopen function to it to cache the `fd`, but I had some problems, so I wanted to finish it later. https://github.com/apache/apisix/pull/5831#discussion_r776956698
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r786361041
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -66,18 +66,18 @@ end
local function write_file_data(conf, log_message)
- local msg = core.json.encode(log_message) .. "\n"
+ local msg = core.json.encode(log_message)
local file, err = io_open(conf.path, 'a+')
if not file then
core.log.error("failed to open file: ", conf.path, ", error info: ", err)
else
- local ok, error = file:write(msg)
+ local ok, err = file:write(msg, '\n')
if not ok then
- core.log.error("failed to write file: ", conf.path, ", error info: ", error)
+ file:close()
Review comment:
We should close the file whether the writing success or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r776956698
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,186 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+local C = ffi.C
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+local O_CREAT = 00000040 -- create and open
+local O_APPEND = 00000400 -- add content to the end of
+local O_WRONLY = 00000001 -- write only open
+local S_IRUSR = 00400 -- user has read permission
+local S_IWUSR = 00200 -- user has write permission
+local S_IRGRP = 00040 -- group has read permission
+local S_IROTH = 00004 -- others have read permission
+local oflags = bit.bor(O_WRONLY, O_CREAT, O_APPEND)
+local mode = bit.bor(S_IRUSR, S_IWUSR, S_IRGRP, S_IROTH)
+local file_descriptors = {}
+
+
+ffi.cdef [[
+ int open(const char * filename, int flags, int mode);
+ int write(int fd, const void * ptr, int numbytes);
+ int close(int fd);
+]]
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string",
+ match = [[^[^*&%%\`]+$]],
+ err = "not a valid filename"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function reopen()
+ local target_pointer, err = io_open('logs/file_pointer', 'r')
Review comment:
We also need to remove the stale cached data when a route used file-logger is deleted. It can be done with a timer: https://github.com/apache/apisix/blob/cc09ef3b69089be62718455d9f919879bf28318b/apisix/plugins/server-info.lua#L240
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r776957552
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,186 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+local C = ffi.C
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+local O_CREAT = 00000040 -- create and open
+local O_APPEND = 00000400 -- add content to the end of
+local O_WRONLY = 00000001 -- write only open
+local S_IRUSR = 00400 -- user has read permission
+local S_IWUSR = 00200 -- user has write permission
+local S_IRGRP = 00040 -- group has read permission
+local S_IROTH = 00004 -- others have read permission
+local oflags = bit.bor(O_WRONLY, O_CREAT, O_APPEND)
+local mode = bit.bor(S_IRUSR, S_IWUSR, S_IRGRP, S_IROTH)
+local file_descriptors = {}
+
+
+ffi.cdef [[
+ int open(const char * filename, int flags, int mode);
+ int write(int fd, const void * ptr, int numbytes);
+ int close(int fd);
+]]
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string",
+ match = [[^[^*&%%\`]+$]],
+ err = "not a valid filename"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function reopen()
+ local target_pointer, err = io_open('logs/file_pointer', 'r')
Review comment:
Since only one worker can receive the reopen request, we can use worker event to trigger fd reopen in the other workers, like: https://github.com/apache/apisix/blob/cc09ef3b69089be62718455d9f919879bf28318b/apisix/discovery/consul_kv/init.lua#L376
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] juzhiyuan commented on pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#issuecomment-1018389004
Hi @guoqqqi, please add this to README refer to @Baoyuantop's PR :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bisakhmondal commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
bisakhmondal commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r789376199
##########
File path: t/plugin/file-logger.t
##########
@@ -0,0 +1,204 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+no_long_string();
+no_root_location();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (! $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+
+ if (! $block->no_error_log && ! $block->error_log) {
+ $block->set_value("no_error_log", "[error]");
+ }
+});
+
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: sanity
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema({
+ path = "file.log"
+ })
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- response_body
+done
+
+
+
+=== TEST 2: path is missing check
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema()
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- no_error_log
+property "path" is required
+
+
+
+=== TEST 3: add plugin metadata
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/plugin_metadata/file-logger',
+ ngx.HTTP_PUT,
+ [[{
+ "log_format": {
+ "host": "$host",
+ "client_ip": "$remote_addr"
+ }
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(code..body)
+ }
+ }
+--- response_body
+201passed
+
+
+
+=== TEST 4: add plugin
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "file-logger": {
+ "path": "file.log"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1982": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 5: verify plugin
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local t = require("lib.test_admin").test
+ local code = t("/hello", ngx.HTTP_GET)
+ local fd, err = io.open("file.log", 'r')
+ local msg
+
+ if fd then
+ msg = fd:read()
+ else
+ core.log.error("failed to open file: file.log, error info: ", err)
+ end
+ fd:close()
+
+ local new_msg = core.json.decode(msg)
+ if new_msg.client_ip == '127.0.0.1' and new_msg.route_id == '1'
+ and new_msg.host == '127.0.0.1'
+ then
+ msg = "write file log success"
+ ngx.status = code
+ ngx.say(msg)
+ end
+ }
+ }
+--- response_body
+write file log success
+
+
+
+=== TEST 6: failed to open the path
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "file-logger": {
+ "path": "/log/file.log"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1982": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ end
+
+ local code, messages = t("/hello", GET)
+ core.log.warn("messages: ", messages)
+ if code >= 300 then
+ ngx.status = code
+ end
+ }
+ }
+--- error_log
+failed to open file: /log/file.log, error info: /log/file.log: No such file or directory
Review comment:
Yes sure. Totally up to you. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#issuecomment-1018304159
Merged. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r776958094
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,212 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+
+local C = ffi.C
+local ngx = ngx
+local pairs = pairs
+local load_string = loadstring
+local io_open = io.open
+
+local plugin_name = "file-logger"
+local O_CREAT = 00000040 -- create and open
Review comment:
`file:flush()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] guoqqqi edited a comment on pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
guoqqqi edited a comment on pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#issuecomment-997520694
TODO:
- [x] add control API
- [ ] test
- [ ] docs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r785434787
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,102 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+local file_descriptors = {}
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string",
+ match = [[^[^*&%%\`]+$]],
Review comment:
The syntax is still incorrect. See https://github.com/apache/apisix/pull/5831#discussion_r776539246
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,102 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+local file_descriptors = {}
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string",
+ match = [[^[^*&%%\`]+$]],
+ err = "not a valid filename"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message) .. "\n"
+ local fd = file_descriptors[conf.path]
+
+ if not fd then
+ local file = io_open(conf.path, 'a+')
Review comment:
Please don't cache fd in this version. We don't have a way to remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r776956698
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,186 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+local C = ffi.C
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+local O_CREAT = 00000040 -- create and open
+local O_APPEND = 00000400 -- add content to the end of
+local O_WRONLY = 00000001 -- write only open
+local S_IRUSR = 00400 -- user has read permission
+local S_IWUSR = 00200 -- user has write permission
+local S_IRGRP = 00040 -- group has read permission
+local S_IROTH = 00004 -- others have read permission
+local oflags = bit.bor(O_WRONLY, O_CREAT, O_APPEND)
+local mode = bit.bor(S_IRUSR, S_IWUSR, S_IRGRP, S_IROTH)
+local file_descriptors = {}
+
+
+ffi.cdef [[
+ int open(const char * filename, int flags, int mode);
+ int write(int fd, const void * ptr, int numbytes);
+ int close(int fd);
+]]
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string",
+ match = [[^[^*&%%\`]+$]],
+ err = "not a valid filename"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function reopen()
+ local target_pointer, err = io_open('logs/file_pointer', 'r')
Review comment:
We also need to remove the stale cached data when a route used file-logger is deleted. It can be done with a timer: https://github.com/apache/apisix/blob/cc09ef3b69089be62718455d9f919879bf28318b/apisix/plugins/server-info.lua#L240
https://github.com/apache/apisix/blob/cc09ef3b69089be62718455d9f919879bf28318b/apisix/plugins/server-info.lua#L248
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#issuecomment-1012707677
It's OK.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] guoqqqi commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
guoqqqi commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r788453348
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,102 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message)
+ local file, err = io_open(conf.path, 'a+')
+
+ if not file then
+ core.log.error("failed to open file: ", conf.path, ", error info: ", err)
+ else
+ local ok, err = file:write(msg, '\n')
+ if not ok then
+ core.log.error("failed to write file: ", conf.path, ", error info: ", err)
+ else
+ file:flush()
+ end
+ file:close()
+ end
+end
Review comment:
Hi, @bisakhmondal I tried to add a reopen function to it to cache the `fd`, but I had some problems, so I wanted to finish it later. https://github.com/apache/apisix/pull/5831#discussion_r776956698
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bisakhmondal commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
bisakhmondal commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r789377081
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,102 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message)
+ local file, err = io_open(conf.path, 'a+')
+
+ if not file then
+ core.log.error("failed to open file: ", conf.path, ", error info: ", err)
+ else
+ local ok, err = file:write(msg, '\n')
+ if not ok then
+ core.log.error("failed to write file: ", conf.path, ", error info: ", err)
+ else
+ file:flush()
+ end
+ file:close()
+ end
+end
Review comment:
Okay, @guoqqqi. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] guoqqqi commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
guoqqqi commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r782832033
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,212 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+
+local C = ffi.C
+local ngx = ngx
+local pairs = pairs
+local load_string = loadstring
+local io_open = io.open
+
+local plugin_name = "file-logger"
+local O_CREAT = 00000040 -- create and open
Review comment:
Hi, @spacewander I am trying to use `io.write` and `file:flush` to write logs, but I find that `io.open` returns a `userdata` and my attempts to store and read it fail, how do I go about it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r776539101
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,212 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+
+local C = ffi.C
+local ngx = ngx
+local pairs = pairs
+local load_string = loadstring
+local io_open = io.open
+
+local plugin_name = "file-logger"
+local O_CREAT = 00000040 -- create and open
Review comment:
Why not use Lua's std file library? That one is much simpler. See https://www.tutorialspoint.com/lua/lua_file_io.htm
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,212 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+
+local C = ffi.C
+local ngx = ngx
+local pairs = pairs
+local load_string = loadstring
+local io_open = io.open
+
+local plugin_name = "file-logger"
+local O_CREAT = 00000040 -- create and open
+local O_APPEND = 00000400 -- add content to the end of
+local O_WRONLY = 00000001 -- write only open
+local S_IRUSR = 00400 -- user has read permission
+local S_IWUSR = 00200 -- user has write permission
+local S_IRGRP = 00040 -- group has read permission
+local S_IROTH = 00004 -- others have read permission
+
+local oflags = bit.bor(O_WRONLY, O_CREAT, O_APPEND)
+local mode = bit.bor(S_IRUSR, S_IWUSR, S_IRGRP, S_IROTH)
+local file_descriptors = {}
+
+ffi.cdef [[
+ int open(const char * filename, int flags, int mode);
+ int write(int fd, const void * ptr, int numbytes);
+ int close(int fd);
+]]
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string",
+ require = true,
+ match = [[^[^*&%%\`]+$]],
+ err = "not a valid filename"
+ },
+ custom_fields_by_lua = {
+ type = "object",
+ keys = {
+ type = "string",
+ len_min = 1
+ },
+ values = {
+ type = "string",
+ len_min = 1
+ }
+ }
+ },
+ required = {"path"}
+}
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function reopen()
+ local args = ngx.req.get_uri_args()
+ if args["reopen"] then
+ local target_pointer, err = io_open('logs/file_pointer', 'r')
+ if not target_pointer then
+ core.log.error("failed to open file, error info: " .. err)
+ else
+ local pointer = target_pointer:read('*n')
+ target_pointer:close()
+ core.log.warn('pointer: ', pointer)
+ C.close(pointer)
+
+ local reset_pointer = io_open('logs/file_pointer', 'w+')
+ reset_pointer:write(-1)
+ reset_pointer:close()
+ end
+ end
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message) .. "\n"
+ local fd = file_descriptors[conf.path]
+ local file = io_open(conf.path, 'r')
+ local file_pointer = io_open('logs/file_pointer', 'r')
+
+ if file_pointer then
+ local read_pointer = file_pointer:read('*n')
+ core.log.warn('read_pointer: ', read_pointer)
+ file_pointer:close()
+ if read_pointer < 0 then
+ C.close(fd)
+ fd = nil
+ file_descriptors[conf.path] = nil
+ end
+ else
+ fd = file_descriptors[conf.path]
+ end
+
+ if not file then
+ file = io_open(conf.path, 'a+')
+ file:close()
+ if fd then
+ C.close(fd)
+ fd = nil
+ file_descriptors[conf.path] = nil
+ end
+ else
+ file:close()
+ end
+
+
+ if not fd then
+ fd = C.open(conf.path, oflags, mode)
+ core.log.warn('fd: ', fd)
+ local write_pointer = io_open('logs/file_pointer', 'w+')
+ write_pointer:write(fd)
+ write_pointer:close()
+
+ if fd < 0 then
+ local err = ffi.errno()
+ core.log.error("failed to open file: " .. conf.path .. ", error info: " .. err)
+
+ else
+ file_descriptors[conf.path] = fd
+ end
+ end
+
+ C.write(fd, msg, #msg)
+end
+
+
+function _M.body_filter(conf, ctx)
+ log_util.collect_body(conf, ctx)
+end
+
+
+function _M.log(conf, ctx)
+ local metadata = plugin.plugin_metadata(plugin_name)
+ local entry
+
+ if metadata and metadata.value.log_format
+ and core.table.nkeys(metadata.value.log_format) > 0
+ then
+ entry = log_util.get_custom_format_log(ctx, metadata.value.log_format)
+ else
+ entry = log_util.get_full_log(ngx, conf)
+ end
+
+ if conf.custom_fields_by_lua
Review comment:
Look like we don't need to provide `custom_fields_by_lua`, as we already have "log_util.get_custom_format_log".
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,212 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+
Review comment:
Please don't use a blank line between localized functions.
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,212 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+
+local C = ffi.C
+local ngx = ngx
+local pairs = pairs
+local load_string = loadstring
+local io_open = io.open
+
+local plugin_name = "file-logger"
+local O_CREAT = 00000040 -- create and open
+local O_APPEND = 00000400 -- add content to the end of
+local O_WRONLY = 00000001 -- write only open
+local S_IRUSR = 00400 -- user has read permission
+local S_IWUSR = 00200 -- user has write permission
+local S_IRGRP = 00040 -- group has read permission
+local S_IROTH = 00004 -- others have read permission
+
+local oflags = bit.bor(O_WRONLY, O_CREAT, O_APPEND)
+local mode = bit.bor(S_IRUSR, S_IWUSR, S_IRGRP, S_IROTH)
+local file_descriptors = {}
+
+ffi.cdef [[
+ int open(const char * filename, int flags, int mode);
+ int write(int fd, const void * ptr, int numbytes);
+ int close(int fd);
+]]
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string",
+ require = true,
+ match = [[^[^*&%%\`]+$]],
+ err = "not a valid filename"
+ },
+ custom_fields_by_lua = {
+ type = "object",
+ keys = {
+ type = "string",
+ len_min = 1
+ },
+ values = {
+ type = "string",
+ len_min = 1
+ }
+ }
+ },
+ required = {"path"}
+}
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function reopen()
+ local args = ngx.req.get_uri_args()
+ if args["reopen"] then
+ local target_pointer, err = io_open('logs/file_pointer', 'r')
+ if not target_pointer then
+ core.log.error("failed to open file, error info: " .. err)
+ else
+ local pointer = target_pointer:read('*n')
+ target_pointer:close()
+ core.log.warn('pointer: ', pointer)
+ C.close(pointer)
+
+ local reset_pointer = io_open('logs/file_pointer', 'w+')
+ reset_pointer:write(-1)
+ reset_pointer:close()
+ end
+ end
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message) .. "\n"
+ local fd = file_descriptors[conf.path]
+ local file = io_open(conf.path, 'r')
+ local file_pointer = io_open('logs/file_pointer', 'r')
+
+ if file_pointer then
+ local read_pointer = file_pointer:read('*n')
+ core.log.warn('read_pointer: ', read_pointer)
+ file_pointer:close()
+ if read_pointer < 0 then
+ C.close(fd)
+ fd = nil
+ file_descriptors[conf.path] = nil
+ end
+ else
+ fd = file_descriptors[conf.path]
+ end
+
+ if not file then
+ file = io_open(conf.path, 'a+')
+ file:close()
+ if fd then
+ C.close(fd)
+ fd = nil
+ file_descriptors[conf.path] = nil
+ end
+ else
+ file:close()
+ end
+
+
+ if not fd then
+ fd = C.open(conf.path, oflags, mode)
+ core.log.warn('fd: ', fd)
+ local write_pointer = io_open('logs/file_pointer', 'w+')
+ write_pointer:write(fd)
+ write_pointer:close()
+
+ if fd < 0 then
+ local err = ffi.errno()
+ core.log.error("failed to open file: " .. conf.path .. ", error info: " .. err)
+
+ else
+ file_descriptors[conf.path] = fd
+ end
+ end
+
+ C.write(fd, msg, #msg)
+end
+
+
+function _M.body_filter(conf, ctx)
+ log_util.collect_body(conf, ctx)
Review comment:
Since the first version of file-logger doesn't support logging response body, we don't need to collect it.
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,212 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+
+local C = ffi.C
+local ngx = ngx
+local pairs = pairs
+local load_string = loadstring
+local io_open = io.open
+
+local plugin_name = "file-logger"
+local O_CREAT = 00000040 -- create and open
+local O_APPEND = 00000400 -- add content to the end of
+local O_WRONLY = 00000001 -- write only open
+local S_IRUSR = 00400 -- user has read permission
+local S_IWUSR = 00200 -- user has write permission
+local S_IRGRP = 00040 -- group has read permission
+local S_IROTH = 00004 -- others have read permission
+
+local oflags = bit.bor(O_WRONLY, O_CREAT, O_APPEND)
+local mode = bit.bor(S_IRUSR, S_IWUSR, S_IRGRP, S_IROTH)
+local file_descriptors = {}
+
+ffi.cdef [[
+ int open(const char * filename, int flags, int mode);
+ int write(int fd, const void * ptr, int numbytes);
+ int close(int fd);
+]]
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string",
+ require = true,
+ match = [[^[^*&%%\`]+$]],
+ err = "not a valid filename"
+ },
+ custom_fields_by_lua = {
+ type = "object",
+ keys = {
+ type = "string",
+ len_min = 1
+ },
+ values = {
+ type = "string",
+ len_min = 1
+ }
+ }
+ },
+ required = {"path"}
+}
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
Review comment:
Please use two blank lines to separate each section (localized functions, define module-level variables, define functions, return _M)
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,212 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+
+local C = ffi.C
+local ngx = ngx
+local pairs = pairs
+local load_string = loadstring
+local io_open = io.open
+
+local plugin_name = "file-logger"
+local O_CREAT = 00000040 -- create and open
+local O_APPEND = 00000400 -- add content to the end of
+local O_WRONLY = 00000001 -- write only open
+local S_IRUSR = 00400 -- user has read permission
+local S_IWUSR = 00200 -- user has write permission
+local S_IRGRP = 00040 -- group has read permission
+local S_IROTH = 00004 -- others have read permission
+
+local oflags = bit.bor(O_WRONLY, O_CREAT, O_APPEND)
+local mode = bit.bor(S_IRUSR, S_IWUSR, S_IRGRP, S_IROTH)
+local file_descriptors = {}
+
+ffi.cdef [[
+ int open(const char * filename, int flags, int mode);
+ int write(int fd, const void * ptr, int numbytes);
+ int close(int fd);
+]]
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string",
+ require = true,
+ match = [[^[^*&%%\`]+$]],
+ err = "not a valid filename"
+ },
+ custom_fields_by_lua = {
+ type = "object",
+ keys = {
+ type = "string",
+ len_min = 1
+ },
+ values = {
+ type = "string",
+ len_min = 1
+ }
+ }
+ },
+ required = {"path"}
+}
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function reopen()
+ local args = ngx.req.get_uri_args()
+ if args["reopen"] then
+ local target_pointer, err = io_open('logs/file_pointer', 'r')
+ if not target_pointer then
+ core.log.error("failed to open file, error info: " .. err)
+ else
+ local pointer = target_pointer:read('*n')
+ target_pointer:close()
+ core.log.warn('pointer: ', pointer)
+ C.close(pointer)
+
+ local reset_pointer = io_open('logs/file_pointer', 'w+')
+ reset_pointer:write(-1)
+ reset_pointer:close()
+ end
+ end
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message) .. "\n"
+ local fd = file_descriptors[conf.path]
+ local file = io_open(conf.path, 'r')
+ local file_pointer = io_open('logs/file_pointer', 'r')
+
+ if file_pointer then
+ local read_pointer = file_pointer:read('*n')
+ core.log.warn('read_pointer: ', read_pointer)
+ file_pointer:close()
+ if read_pointer < 0 then
+ C.close(fd)
+ fd = nil
+ file_descriptors[conf.path] = nil
+ end
+ else
+ fd = file_descriptors[conf.path]
+ end
+
+ if not file then
+ file = io_open(conf.path, 'a+')
+ file:close()
+ if fd then
+ C.close(fd)
+ fd = nil
+ file_descriptors[conf.path] = nil
+ end
+ else
+ file:close()
+ end
+
+
+ if not fd then
+ fd = C.open(conf.path, oflags, mode)
+ core.log.warn('fd: ', fd)
+ local write_pointer = io_open('logs/file_pointer', 'w+')
+ write_pointer:write(fd)
+ write_pointer:close()
+
+ if fd < 0 then
+ local err = ffi.errno()
+ core.log.error("failed to open file: " .. conf.path .. ", error info: " .. err)
+
+ else
+ file_descriptors[conf.path] = fd
+ end
+ end
+
+ C.write(fd, msg, #msg)
+end
+
+
+function _M.body_filter(conf, ctx)
+ log_util.collect_body(conf, ctx)
+end
+
+
+function _M.log(conf, ctx)
+ local metadata = plugin.plugin_metadata(plugin_name)
+ local entry
+
+ if metadata and metadata.value.log_format
+ and core.table.nkeys(metadata.value.log_format) > 0
+ then
+ entry = log_util.get_custom_format_log(ctx, metadata.value.log_format)
+ else
+ entry = log_util.get_full_log(ngx, conf)
+ end
+
+ if conf.custom_fields_by_lua
+ and core.table.nkeys(conf.custom_fields_by_lua) > 0
+ then
+ local set_log_fields_value = entry
+ for key, expression in pairs(conf.custom_fields_by_lua) do
+ set_log_fields_value[key] = load_string(expression)()
+ end
+ write_file_data(conf, set_log_fields_value)
+ else
+ write_file_data(conf, entry)
+ end
+end
+
+
+function _M.control_api()
+ return {
+ {
+ methods = {"GET"},
+ uris = {"/plugin/file-logger/reopen"},
Review comment:
```suggestion
uris = {"/v1/plugin/file-logger/reopen"},
```
would be better
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,212 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+
+local C = ffi.C
+local ngx = ngx
+local pairs = pairs
+local load_string = loadstring
+local io_open = io.open
+
Review comment:
Better to use two blank lines after localized functions.
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,212 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+
+local C = ffi.C
+local ngx = ngx
+local pairs = pairs
+local load_string = loadstring
+local io_open = io.open
+
+local plugin_name = "file-logger"
+local O_CREAT = 00000040 -- create and open
+local O_APPEND = 00000400 -- add content to the end of
+local O_WRONLY = 00000001 -- write only open
+local S_IRUSR = 00400 -- user has read permission
+local S_IWUSR = 00200 -- user has write permission
+local S_IRGRP = 00040 -- group has read permission
+local S_IROTH = 00004 -- others have read permission
+
+local oflags = bit.bor(O_WRONLY, O_CREAT, O_APPEND)
+local mode = bit.bor(S_IRUSR, S_IWUSR, S_IRGRP, S_IROTH)
+local file_descriptors = {}
+
+ffi.cdef [[
+ int open(const char * filename, int flags, int mode);
+ int write(int fd, const void * ptr, int numbytes);
+ int close(int fd);
+]]
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string",
+ require = true,
Review comment:
This is not a valid JSON schema syntax. Please take a look at https://github.com/apache/apisix/blob/master/apisix/schema_def.lua
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,212 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ffi = require("ffi")
+local bit = require("bit")
+
+local C = ffi.C
+local ngx = ngx
+local pairs = pairs
+local load_string = loadstring
+local io_open = io.open
+
+local plugin_name = "file-logger"
+local O_CREAT = 00000040 -- create and open
+local O_APPEND = 00000400 -- add content to the end of
+local O_WRONLY = 00000001 -- write only open
+local S_IRUSR = 00400 -- user has read permission
+local S_IWUSR = 00200 -- user has write permission
+local S_IRGRP = 00040 -- group has read permission
+local S_IROTH = 00004 -- others have read permission
+
+local oflags = bit.bor(O_WRONLY, O_CREAT, O_APPEND)
+local mode = bit.bor(S_IRUSR, S_IWUSR, S_IRGRP, S_IROTH)
+local file_descriptors = {}
+
+ffi.cdef [[
+ int open(const char * filename, int flags, int mode);
+ int write(int fd, const void * ptr, int numbytes);
+ int close(int fd);
+]]
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string",
+ require = true,
+ match = [[^[^*&%%\`]+$]],
+ err = "not a valid filename"
+ },
+ custom_fields_by_lua = {
+ type = "object",
+ keys = {
+ type = "string",
+ len_min = 1
+ },
+ values = {
+ type = "string",
+ len_min = 1
+ }
+ }
+ },
+ required = {"path"}
+}
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function reopen()
+ local args = ngx.req.get_uri_args()
+ if args["reopen"] then
+ local target_pointer, err = io_open('logs/file_pointer', 'r')
+ if not target_pointer then
+ core.log.error("failed to open file, error info: " .. err)
+ else
+ local pointer = target_pointer:read('*n')
+ target_pointer:close()
+ core.log.warn('pointer: ', pointer)
+ C.close(pointer)
+
+ local reset_pointer = io_open('logs/file_pointer', 'w+')
+ reset_pointer:write(-1)
+ reset_pointer:close()
+ end
+ end
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message) .. "\n"
+ local fd = file_descriptors[conf.path]
+ local file = io_open(conf.path, 'r')
+ local file_pointer = io_open('logs/file_pointer', 'r')
+
+ if file_pointer then
+ local read_pointer = file_pointer:read('*n')
+ core.log.warn('read_pointer: ', read_pointer)
+ file_pointer:close()
+ if read_pointer < 0 then
+ C.close(fd)
+ fd = nil
+ file_descriptors[conf.path] = nil
+ end
+ else
+ fd = file_descriptors[conf.path]
+ end
+
+ if not file then
+ file = io_open(conf.path, 'a+')
+ file:close()
+ if fd then
+ C.close(fd)
+ fd = nil
+ file_descriptors[conf.path] = nil
+ end
+ else
+ file:close()
+ end
+
+
+ if not fd then
+ fd = C.open(conf.path, oflags, mode)
+ core.log.warn('fd: ', fd)
+ local write_pointer = io_open('logs/file_pointer', 'w+')
+ write_pointer:write(fd)
+ write_pointer:close()
+
+ if fd < 0 then
+ local err = ffi.errno()
+ core.log.error("failed to open file: " .. conf.path .. ", error info: " .. err)
+
+ else
+ file_descriptors[conf.path] = fd
+ end
+ end
+
+ C.write(fd, msg, #msg)
+end
+
+
+function _M.body_filter(conf, ctx)
+ log_util.collect_body(conf, ctx)
+end
+
+
+function _M.log(conf, ctx)
+ local metadata = plugin.plugin_metadata(plugin_name)
+ local entry
+
+ if metadata and metadata.value.log_format
+ and core.table.nkeys(metadata.value.log_format) > 0
+ then
+ entry = log_util.get_custom_format_log(ctx, metadata.value.log_format)
+ else
+ entry = log_util.get_full_log(ngx, conf)
+ end
+
+ if conf.custom_fields_by_lua
+ and core.table.nkeys(conf.custom_fields_by_lua) > 0
+ then
+ local set_log_fields_value = entry
+ for key, expression in pairs(conf.custom_fields_by_lua) do
+ set_log_fields_value[key] = load_string(expression)()
+ end
+ write_file_data(conf, set_log_fields_value)
+ else
+ write_file_data(conf, entry)
+ end
+end
+
+
+function _M.control_api()
+ return {
+ {
+ methods = {"GET"},
Review comment:
What about using `PUT`?
We already use it in the reload: https://github.com/apache/apisix/blob/master/docs/en/latest/architecture-design/plugin.md#hot-reload
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] juzhiyuan commented on pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#issuecomment-995739305
Hi, maybe you'd better add a TODO checklist in this PR's description 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] guoqqqi edited a comment on pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
guoqqqi edited a comment on pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#issuecomment-997520694
TODO:
- [x] add control API
- [ ] test
- [x] docs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] guoqqqi edited a comment on pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
guoqqqi edited a comment on pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#issuecomment-997520694
TODO:
- [x] add control API
- [ ] test
- [x] docs
Wait for the review to be completed and add the English document
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] guoqqqi commented on pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
guoqqqi commented on pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#issuecomment-1011889536
Hi, can I finish the reopen function in another PR later? I may need to spend some time learning `worker.events` and `timers`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r789348773
##########
File path: conf/config-default.yaml
##########
@@ -375,6 +375,7 @@ plugins: # plugin list (sorted by priority)
- rocketmq-logger # priority: 402
- syslog # priority: 401
- udp-logger # priority: 400
+ - file-logger # priority: 399
Review comment:
> The same type of plugin, the first number should be the same. `4xx` is for logger plugins
Ignore this, it looks like it's not the rule anymore
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #5831: feat: add `file-logger` plugin
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r789347915
##########
File path: conf/config-default.yaml
##########
@@ -375,6 +375,7 @@ plugins: # plugin list (sorted by priority)
- rocketmq-logger # priority: 402
- syslog # priority: 401
- udp-logger # priority: 400
+ - file-logger # priority: 399
Review comment:
The same type of plugin, the first number should be the same. `4xx` is for logger plugins
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org