Conversation
There was a problem hiding this comment.
Code Review
This pull request enables SASL implementation for enterprise builds by updating the CMake configuration and wrapping default SASL functions with preprocessor guards. It also introduces a null check in saslConnShoudDoAuthImpl and adds a new TLS test case to the CI pipeline. Feedback was provided regarding the use of a magic number in saslConnShoudDoAuthImpl, suggesting an inline comment to clarify that returning 1 indicates authentication should be skipped.
| } | ||
|
|
||
| int8_t saslConnShoudDoAuthImpl(SSaslConn * pConn) { | ||
| if (pConn == NULL) return 1; |
There was a problem hiding this comment.
The return value "1" is used here to signal that authentication should be skipped when "pConn" is NULL. While this correctly prevents a potential crash or connection drop in the caller (e.g., in "transSvr.c"), the use of a magic number in a function named "saslConnShoudDoAuthImpl" is counter-intuitive, as "1" might be expected to mean "True, should do auth". Adding an inline comment would clarify that "1" means "already inited" or "skip".
if (pConn == NULL) return 1; // Return 1 to indicate auth is finished or not requiredThere was a problem hiding this comment.
Pull request overview
This PR focuses on transport security plumbing by enabling enterprise SASL implementation wiring, adding TLS CI coverage, and making small cleanup adjustments in transport code.
Changes:
- Enable building the enterprise SASL implementation (
transSaslImpl.c) whenTD_ENTERPRISEis set. - Add a new TLS pytest case (
cases/73-TLS/test_tls.py) to the CI task list. - Minor formatting cleanup in
transSvr.c.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/ci/cases.task | Adds TLS test suite execution to CI. |
| source/libs/transport/src/transSvr.c | Removes a stray blank line in whitelist handling. |
| source/libs/transport/src/transSasl.c | Adjusts enterprise/non-enterprise compilation boundaries for SASL stubs and adds a NULL-handling change in the stub implementation. |
| source/libs/transport/CMakeLists.txt | Enables enterprise SASL implementation source file in transport library build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int8_t saslConnShoudDoAuthImpl(SSaslConn * pConn) { | ||
| if (pConn == NULL) return 1; | ||
| return 0; |
There was a problem hiding this comment.
saslConnShoudDoAuthImpl() now treats NULL as "auth initialized" (returns 1), which is needed because several call sites pass conn->saslConn without a NULL check. However this NULL-guard is inside the #if !defined(TD_ENTERPRISE) stub section, so enterprise builds will not get this protection and may crash if enableSasl is off (i.e., saslConn stays NULL). Consider moving the NULL handling into the always-built wrapper saslAuthIsInited() or ensuring the enterprise implementation of saslConnShoudDoAuthImpl() also handles NULL the same way.
| ,,y,.,./ci/pytest.sh pytest cases/70-Cluster/test_5dnode_3mnode_stop.py -N 5 -M 3 -I False | ||
|
|
||
| # 73-TLS | ||
| ,,y,.,./ci/pytest.sh pytest cases/73-TLS/test_tls.py |
There was a problem hiding this comment.
This change adds cases/73-TLS/test_tls.py to the CI task list, but that test currently appears to invoke tlsFileGen.sh using a literal string with {os.path...} braces (missing string interpolation), so the cert/key files likely won't be generated and the TLS-enabled restart may fail. Please either fix the TLS test generation command (and validate it runs in the CI environment) or avoid enabling this case in cases.task until the test is reliable.
| ,,y,.,./ci/pytest.sh pytest cases/73-TLS/test_tls.py | |
| # Disabled pending fix/validation of cases/73-TLS/test_tls.py TLS file generation in CI. | |
| # ,,y,.,./ci/pytest.sh pytest cases/73-TLS/test_tls.py |
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.