You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "YapWC (via GitHub)" <gi...@apache.org> on 2025/12/22 11:05:19 UTC
[PR] Fix: Make protocol_name optional and default to 'MQTT' for mqtt plugin [apisix]
YapWC opened a new pull request, #12831:
URL: https://github.com/apache/apisix/pull/12831
### Description
The mqtt protocol_name default to 'MQTT' as it is the standard protocol. And the protocol_name is now optional as it is not strictly required, reducing user configuration overhead.
Changes:
- The mqtt protocol_name default to 'MQTT' and it is an optional attribute now.
- Updated the preread validation logic to only perform protocol_name validation when protocol_name is set.
- Implemented sanity test for mqtt schema check (for protocol_name & protocol_level).
- Updated both the english and mandarin documentation for mqtt plugin.
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
#### Which issue(s) this PR fixes:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
-->
Fixes #11589
### Checklist
- [ x ] I have explained the need for this PR and the problem it solves
- [ x ] I have explained the changes or the new features added to this PR
- [ x ] I have added tests corresponding to this change
- [ x ] I have updated the documentation to reflect this change
- [ x ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
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
Re: [PR] fix: Make protocol_name optional and default to 'MQTT' for mqtt plugin [apisix]
Posted by "Baoyuantop (via GitHub)" <gi...@apache.org>.
Baoyuantop commented on PR #12831:
URL: https://github.com/apache/apisix/pull/12831#issuecomment-3691036899
Hi @YapWC, please fix lint error
--
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
Re: [PR] fix: Make protocol_name optional and default to 'MQTT' for mqtt plugin [apisix]
Posted by "Baoyuantop (via GitHub)" <gi...@apache.org>.
Baoyuantop commented on PR #12831:
URL: https://github.com/apache/apisix/pull/12831#issuecomment-3688470047
Hi @YapWC, please fix the failed lint CI
--
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
Re: [PR] fix: Make protocol_name optional and default to 'MQTT' for mqtt plugin [apisix]
Posted by "YapWC (via GitHub)" <gi...@apache.org>.
YapWC commented on PR #12831:
URL: https://github.com/apache/apisix/pull/12831#issuecomment-3690873235
Hi @Baoyuantop here is the latest fixes, feel free to let me know if there is any else.
--
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
Re: [PR] fix: Make protocol_name optional and default to 'MQTT' for mqtt plugin [apisix]
Posted by "YapWC (via GitHub)" <gi...@apache.org>.
YapWC commented on PR #12831:
URL: https://github.com/apache/apisix/pull/12831#issuecomment-3688669442
> Hi @YapWC, please fix the failed lint CI
It is fixed ya.
--
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
Re: [PR] fix: Make protocol_name optional and default to 'MQTT' for mqtt plugin [apisix]
Posted by "YapWC (via GitHub)" <gi...@apache.org>.
YapWC commented on code in PR #12831:
URL: https://github.com/apache/apisix/pull/12831#discussion_r2646537317
##########
apisix/stream/plugins/mqtt-proxy.lua:
##########
@@ -156,7 +156,7 @@ function _M.preread(conf, ctx)
return 503
end
- if res.protocol and res.protocol ~= conf.protocol_name then
+ if conf.protocol_name and res.protocol and res.protocol ~= conf.protocol_name then
Review Comment:
Oh righttt...lol I was thinking how it is going to validate user input if user configure the input, but in fact we already have a default value anyways. This is funny 🤣
--
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
Re: [PR] fix: Make protocol_name optional and default to 'MQTT' for mqtt plugin [apisix]
Posted by "Baoyuantop (via GitHub)" <gi...@apache.org>.
Baoyuantop commented on PR #12831:
URL: https://github.com/apache/apisix/pull/12831#issuecomment-3690789796
Hi @YapWC, following up on the previous review comments. Please let us know if you have any updates. 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
Re: [PR] fix: Make protocol_name optional and default to 'MQTT' for mqtt plugin [apisix]
Posted by "ronething (via GitHub)" <gi...@apache.org>.
ronething commented on code in PR #12831:
URL: https://github.com/apache/apisix/pull/12831#discussion_r2646456849
##########
t/stream-plugin/mqtt-proxy2.t:
##########
@@ -182,3 +182,30 @@ qr/(mqtt client id: \w+|proxy request to \S+)/
--- grep_error_log_out
mqtt client id: g
proxy request to 127.0.0.1:1995
+
+
+
+=== TEST 8: sanity
+--- config
+ location /t {
+ content_by_lua_block {
+ local test_cases = {
+ {protocol_name = "MQTT", protocol_level = 4},
+ {protocol_name = "MQTT"},
+ {protocol_level = 4},
+
Review Comment:
Redundant blank lines.
--
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
Re: [PR] fix: Make protocol_name optional and default to 'MQTT' for mqtt plugin [apisix]
Posted by "YapWC (via GitHub)" <gi...@apache.org>.
YapWC commented on code in PR #12831:
URL: https://github.com/apache/apisix/pull/12831#discussion_r2646538710
##########
t/stream-plugin/mqtt-proxy2.t:
##########
@@ -182,3 +182,30 @@ qr/(mqtt client id: \w+|proxy request to \S+)/
--- grep_error_log_out
mqtt client id: g
proxy request to 127.0.0.1:1995
+
+
+
+=== TEST 8: sanity
+--- config
+ location /t {
+ content_by_lua_block {
+ local test_cases = {
+ {protocol_name = "MQTT", protocol_level = 4},
+ {protocol_name = "MQTT"},
+ {protocol_level = 4},
+
Review Comment:
Alright will delete the blank line
--
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
Re: [PR] fix: Make protocol_name optional and default to 'MQTT' for mqtt plugin [apisix]
Posted by "YapWC (via GitHub)" <gi...@apache.org>.
YapWC commented on PR #12831:
URL: https://github.com/apache/apisix/pull/12831#issuecomment-3691135222
Okay I think it is fixed this time.
--
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
Re: [PR] fix: Make protocol_name optional and default to 'MQTT' for mqtt plugin [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on code in PR #12831:
URL: https://github.com/apache/apisix/pull/12831#discussion_r2646475710
##########
apisix/stream/plugins/mqtt-proxy.lua:
##########
@@ -156,7 +156,7 @@ function _M.preread(conf, ctx)
return 503
end
- if res.protocol and res.protocol ~= conf.protocol_name then
+ if conf.protocol_name and res.protocol and res.protocol ~= conf.protocol_name then
Review Comment:
This check is meaningless. After declaring the default value, `conf.protocol_name` will definitely have a value.
--
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
Re: [PR] fix: Make protocol_name optional and default to 'MQTT' for mqtt plugin [apisix]
Posted by "Baoyuantop (via GitHub)" <gi...@apache.org>.
Baoyuantop merged PR #12831:
URL: https://github.com/apache/apisix/pull/12831
--
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