Re: [PR] fix(stream): initialize env and secret for TLS cert resolution [apisix]
suryaparua-official commented on PR #12935: URL: https://github.com/apache/apisix/pull/12935#issuecomment-4175203330 Hi @Baoyuantop, I’ve tried multiple approaches for the Vault test (using @file and inline content), but CI still fails with PEM parsing and TLS handshake errors. It seems the issue might be related to when secrets are initialized in the stream lifecycle rather than the test itself. Could you please guide if any additional initialization or ordering is required for stream TLS secret resolution? 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix(stream): initialize env and secret for TLS cert resolution [apisix]
suryaparua-official commented on PR #12935: URL: https://github.com/apache/apisix/pull/12935#issuecomment-4071582086 Hi @moonming, thanks for the suggestion. I set up a local environment (WSL + OpenResty + etcd + Vault dev server) and ran the stream TLS tests to validate the behavior. From my testing: * `apisix_secret.init_worker()` only initializes the providers and does not perform any external network calls during worker startup. * Secret fetching is triggered lazily when referenced (observed during TLS handling rather than startup), so this change should not introduce startup latency. The `$ENV://` cases work as expected. For `$secret://`, the integration is triggered, but some test cases fail locally due to Vault access differences (403 on internal endpoints), so I could not fully validate the end-to-end secret fetch in this setup. Based on the above, the initialization itself remains lightweight and should not impact startup latency. Please let me know if you’d like me to align the Vault setup more closely with the CI environment or make further adjustments. -- 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]
Re: [PR] fix(stream): initialize env and secret for TLS cert resolution [apisix]
Baoyuantop commented on PR #12935: URL: https://github.com/apache/apisix/pull/12935#issuecomment-4044670357 Hi @suryaparua-official, please fix review comments + 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix(stream): initialize env and secret for TLS cert resolution [apisix]
suryaparua-official commented on PR #12935: URL: https://github.com/apache/apisix/pull/12935#issuecomment-3903208558 I’ve fixed the code lint related issues. Please let me know if anything else needs adjustment. -- 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]
Re: [PR] fix(stream): initialize env and secret for TLS cert resolution [apisix]
Copilot commented on code in PR #12935:
URL: https://github.com/apache/apisix/pull/12935#discussion_r2802875534
##
t/stream-node/tls.t:
##
@@ -133,3 +150,161 @@ fetch table plugins
release table ctx_var
release table plugins
release table api_ctx
+
+=== TEST 6: stream tls supports $ENV certificate reference
+--- config
+location /t-env {
+content_by_lua_block {
+local core = require("apisix.core")
+local t = require("lib.test_admin")
+
+local data = {
+cert = "$ENV://APISIX_STREAM_ENV_CERT",
+key = "$ENV://APISIX_STREAM_ENV_KEY",
+sni = "env.test.com",
+}
+
+local code, body = t.test('/apisix/admin/ssls/2',
+ngx.HTTP_PUT,
+core.json.encode(data)
+)
+
+if code >= 300 then
+ngx.status = code
+ngx.say(body)
+return
+end
+
+local code, body = t.test('/apisix/admin/stream_routes/2',
+ngx.HTTP_PUT,
+[[{
+"upstream": {
+"nodes": {
+"127.0.0.1:1995": 1
+},
+"type": "roundrobin"
+}
+}]]
+)
+
+if code >= 300 then
+ngx.status = code
+end
+
+ngx.say("passed")
+}
Review Comment:
In this test block, if creating the stream_route fails (code >= 300) you
still respond with body "passed". Even though the status is set to the error
code, this hides the actual error response and makes failures harder to debug.
Consider returning early on error and outputting the admin API response body
instead of always printing "passed".
##
t/stream-node/tls.t:
##
@@ -133,3 +150,161 @@ fetch table plugins
release table ctx_var
release table plugins
release table api_ctx
+
+=== TEST 6: stream tls supports $ENV certificate reference
+--- config
+location /t-env {
+content_by_lua_block {
+local core = require("apisix.core")
+local t = require("lib.test_admin")
+
+local data = {
+cert = "$ENV://APISIX_STREAM_ENV_CERT",
+key = "$ENV://APISIX_STREAM_ENV_KEY",
+sni = "env.test.com",
+}
+
+local code, body = t.test('/apisix/admin/ssls/2',
+ngx.HTTP_PUT,
+core.json.encode(data)
+)
+
+if code >= 300 then
+ngx.status = code
+ngx.say(body)
+return
+end
+
+local code, body = t.test('/apisix/admin/stream_routes/2',
+ngx.HTTP_PUT,
+[[{
+"upstream": {
+"nodes": {
+"127.0.0.1:1995": 1
+},
+"type": "roundrobin"
+}
+}]]
+)
+
+if code >= 300 then
+ngx.status = code
+end
+
+ngx.say("passed")
+}
+}
+--- request
+GET /t-env
+--- response_body
+passed
+
+
+
+=== TEST 7: hit stream route with env cert
+--- stream_tls_request
+hello
+--- stream_sni: env.test.com
+--- response_body
+hello world
+
+=== TEST 8: store cert and key in vault for stream tls
+--- exec
+VAULT_TOKEN='root' VAULT_ADDR='http://0.0.0.0:8200' vault kv put kv/apisix/ssl
\
+test.com.crt=@t/certs/apisix.crt \
+test.com.key=@t/certs/apisix.key
+--- response_body
+Success!
+
+
+=== TEST 9: set secret provider (vault) for stream tls
+--- config
+location /t-secret {
+content_by_lua_block {
+local t = require("lib.test_admin").test
+
+local code, body = t('/apisix/admin/secrets/vault/stream-test',
+ngx.HTTP_PUT,
+[[{
+"uri": "http://0.0.0.0:8200";,
+"prefix": "kv/apisix",
+"token": "root"
+}]],
+[[{
+"key": "/apisix/secrets/vault/stream-test",
+"value": {
+"uri": "http://0.0.0.0:8200";,
+"prefix": "kv/apisix",
+"token": "root"
+}
+}]]
+)
+
+ngx.status = code
+ngx.say(body)
+}
+}
+--- request
+GET /t-secret
+--- response_body
+passed
+
+
+
+=== TEST 10: stream tls supports $secret certificate reference
+--- config
+location /t-secret {
+content_by_lua_block {
+local core = require("apisix.core")
+local t = require("lib.test_admin")
+
+local data = {
+cert = "$secret://vault/stream-test/ssl/test.com.crt",
+key =
Re: [PR] fix(stream): initialize env and secret for TLS cert resolution [apisix]
Baoyuantop commented on PR #12935: URL: https://github.com/apache/apisix/pull/12935#issuecomment-3895513482 Hi @suryaparua-official, please fix code 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix(stream): initialize env and secret for TLS cert resolution [apisix]
suryaparua-official commented on PR #12935: URL: https://github.com/apache/apisix/pull/12935#issuecomment-3798522876 I’ve fixed the remaining issues. Please let me know if anything else is needed. -- 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]
Re: [PR] fix(stream): initialize env and secret for TLS cert resolution [apisix]
Baoyuantop commented on code in PR #12935:
URL: https://github.com/apache/apisix/pull/12935#discussion_r2726680834
##
t/stream-node/tls.t:
##
@@ -25,6 +25,14 @@ add_block_preprocessor(sub {
my ($block) = @_;
});
+BEGIN {
+use t::APISIX;
+
+$ENV{APISIX_STREAM_ENV_CERT} = t::APISIX::read_file("t/certs/apisix.crt");
+$ENV{APISIX_STREAM_ENV_KEY} = t::APISIX::read_file("t/certs/apisix.key");
+}
Review Comment:
Hi @suryaparua-official, this still needs some tweaking.
--
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]
Re: [PR] fix(stream): initialize env and secret for TLS cert resolution [apisix]
suryaparua-official commented on PR #12935:
URL: https://github.com/apache/apisix/pull/12935#issuecomment-3797911328
I’ve added test coverage for `$secret://` certificate resolution as
suggested, followed the pattern from `t/node/ssl.t`, and removed the `END {}`
block per your recommendation. Please let me know if any further adjustments
are needed.
--
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]
Re: [PR] fix(stream): initialize env and secret for TLS cert resolution [apisix]
Baoyuantop commented on code in PR #12935:
URL: https://github.com/apache/apisix/pull/12935#discussion_r2726151006
##
t/stream-node/tls.t:
##
@@ -133,3 +141,65 @@ fetch table plugins
release table ctx_var
release table plugins
release table api_ctx
+
+=== TEST 6: stream tls supports $ENV certificate reference
+--- config
+location /t-env {
+content_by_lua_block {
+local core = require("apisix.core")
+local t = require("lib.test_admin")
+
+local data = {
+cert = "$ENV://APISIX_STREAM_ENV_CERT",
+key = "$ENV://APISIX_STREAM_ENV_KEY",
+sni = "env.test.com",
+}
+
+local code, body = t.test('/apisix/admin/ssls/2',
+ngx.HTTP_PUT,
+core.json.encode(data)
+)
+
+if code >= 300 then
+ngx.status = code
+ngx.say(body)
+return
+end
+
+local code, body = t.test('/apisix/admin/stream_routes/2',
+ngx.HTTP_PUT,
+[[{
+"upstream": {
+"nodes": {
+"127.0.0.1:1995": 1
+},
+"type": "roundrobin"
+}
+}]]
+)
+
+if code >= 300 then
+ngx.status = code
+end
+
+ngx.say("passed")
+}
+}
+--- request
+GET /t-env
+--- response_body
+passed
+
+
+
+=== TEST 7: hit stream route with env cert
+--- stream_tls_request
+hello
+--- stream_sni: env.test.com
+--- response_body
+hello world
+
+END {
+delete $ENV{APISIX_STREAM_ENV_CERT};
+delete $ENV{APISIX_STREAM_ENV_KEY};
+}
Review Comment:
It is recommended to remove the END {} block; manual cleanup of these
environment variables is not required.
##
t/stream-node/tls.t:
##
@@ -25,6 +25,14 @@ add_block_preprocessor(sub {
my ($block) = @_;
});
+BEGIN {
+use t::APISIX;
+
+$ENV{APISIX_STREAM_ENV_CERT} = t::APISIX::read_file("t/certs/apisix.crt");
+$ENV{APISIX_STREAM_ENV_KEY} = t::APISIX::read_file("t/certs/apisix.key");
+}
Review Comment:
You can refer to
https://github.com/apache/apisix/blob/master/t/node/ssl.t#L18-L33
##
t/stream-node/tls.t:
##
@@ -133,3 +141,65 @@ fetch table plugins
release table ctx_var
release table plugins
release table api_ctx
+
+=== TEST 6: stream tls supports $ENV certificate reference
Review Comment:
Test cases for $secret:// should also be added to ensure complete coverage.
--
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]
Re: [PR] fix(stream): initialize env and secret for TLS cert resolution [apisix]
suryaparua-official commented on PR #12935: URL: https://github.com/apache/apisix/pull/12935#issuecomment-3790164118 I’ve added a Stream TLS test covering `$ENV://` certificate resolution in `t/stream-node/tls.t`. Please let me know if any adjustments are needed. -- 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]
Re: [PR] fix(stream): initialize env and secret for TLS cert resolution [apisix]
Baoyuantop commented on PR #12935: URL: https://github.com/apache/apisix/pull/12935#issuecomment-3789254832 Hi @suryaparua-official, we need add test case for this fix -- 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]
