Re: [PR] fix(stream): initialize env and secret for TLS cert resolution [apisix]

["AlinsRan (via GitHub)" <gi...@apache.org>]

AlinsRan commented on PR #12935:
URL: https://github.com/apache/apisix/pull/12935#issuecomment-4922235893

   Took a close look at this.
   
   The secret part is a real gap. On master `apisix_secret.init_worker()` only runs in `http_init_worker()`, and `/secrets` isn't in `STREAM_ETCD_DIRECTORY`, so `$secret://` certs in stream TLS genuinely don't work today. The chain checks out too: the stream handshake goes through `ssl/router/radixtree_sni.lua` -> `secret.fetch_secrets()` -> `core.config.fetch_created_obj("/secrets")`, and that object only exists if `secret.init_worker()` ran. Adding `/secrets` to `STREAM_ETCD_DIRECTORY` is only consumed by the stream prefetch in `config_etcd.lua`, so no side effects there.
   
   One thing worth calling out: master picked up #13595 after you branched, which already adds `core.env.init()` to `stream_init_worker()`. Yours is in `stream_init()`. These aren't duplicates — the init-phase call captures env vars inherited from the OS, and the worker-phase call picks up `env NAME=VALUE;` directives, since nginx replaces `environ` in the worker with only the declared ones. HTTP calls it in both places. So after a rebase stream ends up symmetric with HTTP, which is what we want. Please rebase on master so this is obvious to whoever reviews next.
   
   CI: lint is failing, you just need to run `make reindex` and commit the result for `t/stream-node/tls.t`. The openid-connect and openwhisk failures in the other job are flaky external-network tests, not yours — a rebase should get you a clean run.
   
   Some test nits:
   
   - The `add_block_preprocessor` that injects `GET /t` into every block isn't really needed. TEST 7 and 11 are pure stream requests. Cleaner to just put `--- request GET /t` on TEST 6 and 10.
   - `set_env_from_file` is defined twice — the file-scope copy is never called.
   - ssl and route ids skip 2, and TEST 9 uses `t(...)` while 6 and 10 use `t.test(...)`.
   
   Would also be good to mention stream support in `docs/en/latest/terminology/secret.md`.
   


-- 
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