Copilot commented on code in PR #13402:
URL: https://github.com/apache/apisix/pull/13402#discussion_r3278141645
##########
t/stream-plugin/syslog.t:
##########
@@ -349,7 +349,123 @@ sending a batch logs to 127.0.0.1:5045
-=== TEST 8: log format in plugin
+=== TEST 8: check service plugin configuration updating
+--- stream_conf_enable
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body1 = t('/apisix/admin/services/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "syslog": {
+ "host" : "127.0.0.1",
+ "port" : 5044,
+ "batch_max_size": 1
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1995": 1
+ },
+ "type": "roundrobin"
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ ngx.say("fail")
+ return
+ end
+
+ local code, body2 = t('/apisix/admin/stream_routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "service_id": "1"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ ngx.say("fail")
+ return
+ end
+
+ ngx.sleep(0.5)
+
+ local sock = ngx.socket.tcp()
+ local ok, err = sock:connect("127.0.0.1", 1985)
+ if not ok then
+ ngx.status = code
+ ngx.say("fail")
Review Comment:
In the socket connect failure path, this sets `ngx.status = code`, but
`code` here is the HTTP status from the previous Admin API call (often 200),
which will hide the real failure. Consider returning a fixed 500 (or similar)
and include `err` in the response/log to make failures diagnosable.
##########
t/stream-plugin/syslog.t:
##########
@@ -349,7 +349,123 @@ sending a batch logs to 127.0.0.1:5045
-=== TEST 8: log format in plugin
+=== TEST 8: check service plugin configuration updating
+--- stream_conf_enable
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body1 = t('/apisix/admin/services/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "syslog": {
+ "host" : "127.0.0.1",
+ "port" : 5044,
+ "batch_max_size": 1
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1995": 1
+ },
+ "type": "roundrobin"
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ ngx.say("fail")
+ return
+ end
+
+ local code, body2 = t('/apisix/admin/stream_routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "service_id": "1"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ ngx.say("fail")
+ return
+ end
+
+ ngx.sleep(0.5)
+
+ local sock = ngx.socket.tcp()
+ local ok, err = sock:connect("127.0.0.1", 1985)
+ if not ok then
+ ngx.status = code
+ ngx.say("fail")
+ return
+ end
+
+ assert(sock:send("mmm"))
+ local body3 = assert(sock:receive("*a"))
+
+ local code, body4 = t('/apisix/admin/services/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "syslog": {
+ "host" : "127.0.0.1",
+ "port" : 5045,
+ "batch_max_size": 1
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1995": 1
+ },
+ "type": "roundrobin"
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ ngx.say("fail")
+ return
+ end
+
+ ngx.sleep(0.5)
+
+ local sock = ngx.socket.tcp()
+ local ok, err = sock:connect("127.0.0.1", 1985)
+ if not ok then
+ ngx.status = code
+ ngx.say("fail")
+ return
+ end
+
+ assert(sock:send("mmm"))
+ local body5 = assert(sock:receive("*a"))
+
Review Comment:
This second socket is also never closed after `receive("*a")`, which can
accumulate across tests. Consider closing the cosocket after use and ensuring
the close happens on failure paths too.
##########
t/stream-plugin/syslog.t:
##########
@@ -349,7 +349,123 @@ sending a batch logs to 127.0.0.1:5045
-=== TEST 8: log format in plugin
+=== TEST 8: check service plugin configuration updating
+--- stream_conf_enable
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body1 = t('/apisix/admin/services/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "syslog": {
+ "host" : "127.0.0.1",
+ "port" : 5044,
+ "batch_max_size": 1
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1995": 1
+ },
+ "type": "roundrobin"
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ ngx.say("fail")
+ return
+ end
+
+ local code, body2 = t('/apisix/admin/stream_routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "service_id": "1"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ ngx.say("fail")
+ return
+ end
+
+ ngx.sleep(0.5)
+
+ local sock = ngx.socket.tcp()
+ local ok, err = sock:connect("127.0.0.1", 1985)
+ if not ok then
+ ngx.status = code
+ ngx.say("fail")
+ return
+ end
+
+ assert(sock:send("mmm"))
+ local body3 = assert(sock:receive("*a"))
+
Review Comment:
The TCP cosocket is never closed after `receive("*a")`. This test adds
multiple new connections, so leaving them open can leak file descriptors and
make the suite flaky under load. Consider calling `sock:close()` after the read
(including on error paths).
##########
t/stream-plugin/syslog.t:
##########
@@ -349,7 +349,123 @@ sending a batch logs to 127.0.0.1:5045
-=== TEST 8: log format in plugin
+=== TEST 8: check service plugin configuration updating
+--- stream_conf_enable
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body1 = t('/apisix/admin/services/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "syslog": {
+ "host" : "127.0.0.1",
+ "port" : 5044,
+ "batch_max_size": 1
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1995": 1
+ },
+ "type": "roundrobin"
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ ngx.say("fail")
+ return
+ end
+
+ local code, body2 = t('/apisix/admin/stream_routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "service_id": "1"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ ngx.say("fail")
+ return
+ end
+
+ ngx.sleep(0.5)
+
+ local sock = ngx.socket.tcp()
+ local ok, err = sock:connect("127.0.0.1", 1985)
+ if not ok then
+ ngx.status = code
+ ngx.say("fail")
+ return
+ end
+
+ assert(sock:send("mmm"))
+ local body3 = assert(sock:receive("*a"))
+
+ local code, body4 = t('/apisix/admin/services/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "syslog": {
+ "host" : "127.0.0.1",
+ "port" : 5045,
+ "batch_max_size": 1
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1995": 1
+ },
+ "type": "roundrobin"
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ ngx.say("fail")
+ return
+ end
+
+ ngx.sleep(0.5)
+
+ local sock = ngx.socket.tcp()
+ local ok, err = sock:connect("127.0.0.1", 1985)
+ if not ok then
+ ngx.status = code
+ ngx.say("fail")
+ return
+ end
Review Comment:
Same as above: on connect failure this uses `ngx.status = code`, where
`code` is from an earlier Admin API call rather than the socket failure. Using
a fixed 500 (and reporting `err`) would avoid masking stream connection
problems.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
