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 2020/09/03 12:17:59 UTC
[GitHub] [apisix] tokers opened a new pull request #2163: improve: cache parsed certs and pkeys to LRU cache
tokers opened a new pull request #2163:
URL: https://github.com/apache/apisix/pull/2163
### What this PR does / why we need it:
The processes of `ngx_ssl.parse_pem_cert` and `ngx_ssl.parse_pem_priv_key` are relatively expensive, and keypair is inclined to be constant (for a long while), so it's worth to cache the parsed result into LRU cache, which can reduce a part of overheads during `ssl_certificate_by_lua*` phase.
### 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?
* [x] Is this PR backward compatible?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2163: improve: cache parsed SSL certs and pkeys
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2163:
URL: https://github.com/apache/apisix/pull/2163#discussion_r484460351
##########
File path: apisix/http/router/radixtree_sni.lua
##########
@@ -95,6 +134,9 @@ local function create_router(ssl_items)
end
end
+ parsed_certs = core.table.new(0, idx)
+ parsed_pkeys = core.table.new(0, idx)
Review comment:
Maybe we can store the parsed data in `ssl.value` directly, so the data can be reused even after router recreated.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers edited a comment on pull request #2163: improve: cache parsed certs and pkeys to LRU cache
Posted by GitBox <gi...@apache.org>.
tokers edited a comment on pull request #2163:
URL: https://github.com/apache/apisix/pull/2163#issuecomment-688270284
@spacewander The size of LRU cache queue is 512, which is relatively large and is uncommon the eviction will happen (in most cases), but indeed the way you recommended is better since we can avoid the duplicate parsing totally.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on pull request #2163: improve: cache parsed certs and pkeys to LRU cache
Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #2163:
URL: https://github.com/apache/apisix/pull/2163#issuecomment-686863695
Related issue: https://github.com/apache/apisix/issues/2144
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2163: improve: cache parsed SSL certs and pkeys
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2163:
URL: https://github.com/apache/apisix/pull/2163#discussion_r486031725
##########
File path: apisix/http/router/radixtree_sni.lua
##########
@@ -30,14 +30,53 @@ local ngx_decode_base64 = ngx.decode_base64
local ssl_certificates
local radixtree_router
local radixtree_router_ver
-
+local parsed_certs
+local parsed_pkeys
local _M = {
version = 0.1,
server_name = ngx_ssl.server_name,
}
+local function parse_pem_cert(sni, cert)
+ local parsed = parsed_certs[cert]
+ if parsed then
+ return parsed
+ end
+
+ core.log.debug("parsing cert for sni: ", sni)
+
+ local parsed, err = ngx_ssl.parse_pem_cert(cert)
+ if not parsed then
+ return nil, err
+ end
+
+ parsed_certs[cert] = parsed
+
+ return parsed
+end
+
+
+local function parse_pem_priv_key(sni, pkey)
+ local parsed = parsed_pkeys[pkey]
+ if parsed then
+ return parsed
+ end
+
+ core.log.debug("parsing priv key for sni: ", sni)
+
+ local parsed, err = ngx_ssl.parse_pem_priv_key(pkey)
+ if not parsed then
+ return nil, err
+ end
+
+ parsed_pkeys[pkey] = parsed
Review comment:
ditto
##########
File path: apisix/http/router/radixtree_sni.lua
##########
@@ -95,6 +134,9 @@ local function create_router(ssl_items)
end
end
+ parsed_certs = core.table.new(0, idx)
+ parsed_pkeys = core.table.new(0, idx)
Review comment:
The current way is wrong. We cannot store them directly in the hash table. This method will cause memory overflow.
We need to cache the parsed data in lrucache, and we need to control the number of cached certificates.
##########
File path: apisix/http/router/radixtree_sni.lua
##########
@@ -30,14 +30,53 @@ local ngx_decode_base64 = ngx.decode_base64
local ssl_certificates
local radixtree_router
local radixtree_router_ver
-
+local parsed_certs
+local parsed_pkeys
local _M = {
version = 0.1,
server_name = ngx_ssl.server_name,
}
+local function parse_pem_cert(sni, cert)
+ local parsed = parsed_certs[cert]
+ if parsed then
+ return parsed
+ end
+
+ core.log.debug("parsing cert for sni: ", sni)
+
+ local parsed, err = ngx_ssl.parse_pem_cert(cert)
+ if not parsed then
+ return nil, err
+ end
+
+ parsed_certs[cert] = parsed
Review comment:
bad way, need to use `lrucache`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2163: improve: cache parsed SSL certs and pkeys
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2163:
URL: https://github.com/apache/apisix/pull/2163#discussion_r484471733
##########
File path: apisix/http/router/radixtree_sni.lua
##########
@@ -95,6 +134,9 @@ local function create_router(ssl_items)
end
end
+ parsed_certs = core.table.new(0, idx)
+ parsed_pkeys = core.table.new(0, idx)
Review comment:
@membphis
Need your confirmation whether this way is safe 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on pull request #2163: improve: cache parsed certs and pkeys to LRU cache
Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #2163:
URL: https://github.com/apache/apisix/pull/2163#issuecomment-688013180
Ping @membphis @moonming
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on pull request #2163: improve: cache parsed SSL certs and pkeys
Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #2163:
URL: https://github.com/apache/apisix/pull/2163#issuecomment-689945812
@membphis It's really a problem if the number of certs is large, I will refactor it with the LRU cache.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers edited a comment on pull request #2163: improve: cache parsed SSL certs and pkeys
Posted by GitBox <gi...@apache.org>.
tokers edited a comment on pull request #2163:
URL: https://github.com/apache/apisix/pull/2163#issuecomment-690011030
@membphis changed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on pull request #2163: improve: cache parsed SSL certs and pkeys
Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #2163:
URL: https://github.com/apache/apisix/pull/2163#issuecomment-690011030
@membphis way changed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming merged pull request #2163: improve: cache parsed SSL certs and pkeys
Posted by GitBox <gi...@apache.org>.
moonming merged pull request #2163:
URL: https://github.com/apache/apisix/pull/2163
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2163: improve: cache parsed SSL certs and pkeys
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2163:
URL: https://github.com/apache/apisix/pull/2163#issuecomment-690043885
> @membphis changed.
approved, need one more `approved`, then your PR can be merged ^_^
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #2163: improve: cache parsed certs and pkeys to LRU cache
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #2163:
URL: https://github.com/apache/apisix/pull/2163#issuecomment-688200088
Would it be better to store the parsed data in `ssl.value` like https://github.com/apache/apisix/blob/f6228b180946a0c15dd314cdc0c53a9024d04823/apisix/http/router/radixtree_sni.lua#L104?
Therefore we can ensure the parsed data will be reused. If we use LRU cache and cache is not large enough, the newly parsed data will evict an old one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on pull request #2163: improve: cache parsed certs and pkeys to LRU cache
Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #2163:
URL: https://github.com/apache/apisix/pull/2163#issuecomment-688270284
@spacewander The size of LRU cache queue is 512, which is relatively large and is uncommon in most cases, but indeed the way you recommended is better since we can avoid the duplicate parsing totally.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org