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 2022/08/17 06:28:13 UTC
[GitHub] [apisix] tzssangglass opened a new pull request, #7717: change: drop support for enable_http2 and listen_port in apisix.ssl
tzssangglass opened a new pull request, #7717:
URL: https://github.com/apache/apisix/pull/7717
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes #7482
### 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
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] 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
[GitHub] [apisix] tzssangglass commented on a diff in pull request #7717: change: drop support for enable_http2 and listen_port in apisix.ssl
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7717:
URL: https://github.com/apache/apisix/pull/7717#discussion_r949831718
##########
apisix/cli/schema.lua:
##########
@@ -208,6 +208,25 @@ local config_schema = {
properties = {
ssl_trusted_certificate = {
type = "string",
+ },
+ listen = {
+ type = "array",
+ items = {
+ type = "object",
+ properties = {
Review Comment:
done
--
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 diff in pull request #7717: change: drop support for enable_http2 and listen_port in apisix.ssl
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7717:
URL: https://github.com/apache/apisix/pull/7717#discussion_r949819272
##########
apisix/cli/schema.lua:
##########
@@ -208,6 +208,25 @@ local config_schema = {
properties = {
ssl_trusted_certificate = {
type = "string",
+ },
+ listen = {
+ type = "array",
+ items = {
+ type = "object",
+ properties = {
Review Comment:
Do we need to use `required` for some fields?
--
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] tzssangglass commented on a diff in pull request #7717: change: drop support for enable_http2 and listen_port in apisix.ssl
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7717:
URL: https://github.com/apache/apisix/pull/7717#discussion_r949884379
##########
apisix/cli/schema.lua:
##########
@@ -208,6 +208,25 @@ local config_schema = {
properties = {
ssl_trusted_certificate = {
type = "string",
+ },
+ listen = {
+ type = "array",
+ items = {
+ type = "object",
+ properties = {
Review Comment:
many test cases doesn't have sll config, so ignore `required `
--
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 #7717: change: drop support for enable_http2 and listen_port in apisix.ssl
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #7717:
URL: https://github.com/apache/apisix/pull/7717
--
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 diff in pull request #7717: change: drop support for enable_http2 and listen_port in apisix.ssl
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7717:
URL: https://github.com/apache/apisix/pull/7717#discussion_r948614121
##########
conf/config-default.yaml:
##########
@@ -133,14 +133,13 @@ apisix:
ssl:
enable: true
listen: # APISIX listening port in https.
- - 9443
+ - port: 9443
+ enable_http2: true
# - port: 9444
Review Comment:
We can remove this comment now as it is enabled
##########
apisix/cli/ops.lua:
##########
@@ -428,46 +428,28 @@ Please modify "admin_key" in conf/config.yaml .
local ssl_listen = {}
-- listen in https, support multiple ports, support specific IP
for _, value in ipairs(yaml_conf.apisix.ssl.listen) do
- if type(value) == "number" then
- listen_table_insert(ssl_listen, "https", "0.0.0.0", value,
- yaml_conf.apisix.ssl.enable_http2, yaml_conf.apisix.enable_ipv6)
- elseif type(value) == "table" then
- local ip = value.ip
- local port = value.port
- local enable_ipv6 = false
- local enable_http2 = (value.enable_http2 or yaml_conf.apisix.ssl.enable_http2)
-
- if ip == nil then
- ip = "0.0.0.0"
- if yaml_conf.apisix.enable_ipv6 then
- enable_ipv6 = true
- end
- end
-
- if port == nil then
- port = 9443
- end
-
- if enable_http2 == nil then
- enable_http2 = false
+ local ip = value.ip
Review Comment:
Let's add a check in https://github.com/apache/apisix/blob/86e9a8ea1022c34a3d5550b54a37994f174847a2/apisix/cli/schema.lua#L206 too
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -582,9 +582,6 @@ http {
{% if proxy_protocol and proxy_protocol.listen_http_port then %}
listen {* proxy_protocol.listen_http_port *} default_server proxy_protocol;
{% end %}
- {% if proxy_protocol and proxy_protocol.listen_https_port then %}
Review Comment:
We can't simply delete the listen_https_port feature. Maybe we should provide an implementation without http2?
--
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] tzssangglass commented on a diff in pull request #7717: change: drop support for enable_http2 and listen_port in apisix.ssl
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7717:
URL: https://github.com/apache/apisix/pull/7717#discussion_r948754903
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -582,9 +582,6 @@ http {
{% if proxy_protocol and proxy_protocol.listen_http_port then %}
listen {* proxy_protocol.listen_http_port *} default_server proxy_protocol;
{% end %}
- {% if proxy_protocol and proxy_protocol.listen_https_port then %}
Review Comment:
my mistake, just remove ssl.enable_http2
##########
apisix/cli/ops.lua:
##########
@@ -428,46 +428,28 @@ Please modify "admin_key" in conf/config.yaml .
local ssl_listen = {}
-- listen in https, support multiple ports, support specific IP
for _, value in ipairs(yaml_conf.apisix.ssl.listen) do
- if type(value) == "number" then
- listen_table_insert(ssl_listen, "https", "0.0.0.0", value,
- yaml_conf.apisix.ssl.enable_http2, yaml_conf.apisix.enable_ipv6)
- elseif type(value) == "table" then
- local ip = value.ip
- local port = value.port
- local enable_ipv6 = false
- local enable_http2 = (value.enable_http2 or yaml_conf.apisix.ssl.enable_http2)
-
- if ip == nil then
- ip = "0.0.0.0"
- if yaml_conf.apisix.enable_ipv6 then
- enable_ipv6 = true
- end
- end
-
- if port == nil then
- port = 9443
- end
-
- if enable_http2 == nil then
- enable_http2 = false
+ local ip = value.ip
Review Comment:
added
--
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] tzssangglass commented on a diff in pull request #7717: change: drop support for enable_http2 and listen_port in apisix.ssl
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7717:
URL: https://github.com/apache/apisix/pull/7717#discussion_r948755383
##########
conf/config-default.yaml:
##########
@@ -133,14 +133,13 @@ apisix:
ssl:
enable: true
listen: # APISIX listening port in https.
- - 9443
+ - port: 9443
+ enable_http2: true
# - port: 9444
Review Comment:
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 diff in pull request #7717: change: drop support for enable_http2 and listen_port in apisix.ssl
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7717:
URL: https://github.com/apache/apisix/pull/7717#discussion_r950836222
##########
apisix/cli/schema.lua:
##########
@@ -208,6 +208,25 @@ local config_schema = {
properties = {
ssl_trusted_certificate = {
type = "string",
+ },
+ listen = {
+ type = "array",
+ items = {
+ type = "object",
+ properties = {
Review Comment:
Yes, you are right. The ip and port are not required in the code. I made a mistake.
--
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