lib: only use gRPC to connect to SaaS #354
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (34)
WalkthroughAdds SaaS testing mode to GrpcAgent with token-based endpoint derivation and an is_testing flag. Updates SaaSInfo to include a testing boolean. Adjusts config parsing for insecure mode in SaaS testing. Modifies Node updateConfig to avoid assigning command from SaaS URL. Broadly parameterizes gRPC tests to run with GRPC and SaaS envs; simplifies ZMQ tests to local-only. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Env as Environment
participant Node as updateConfig (lib/nsolid.js)
participant Cfg as nsolidConfig
Env->>Node: incoming config + NSOLID_* envs
Node->>Node: parse SaaS URL (validate)
alt saas provided and no incoming command
Node->>Cfg: command = null
else no change to command
Node->>Cfg: preserve incoming command
end
opt grpc present and SaaS URL valid
Node->>Cfg: update grpc (no command override)
end
sequenceDiagram
autonumber
participant Env as Env vars / Tokens
participant Agent as GrpcAgent::config
participant Token as parse_saas_token
participant Info as SaaSInfo
Env->>Agent: NSOLID_GRPC[_INSECURE], NSOLID_SAAS
alt SaaS token provided
Agent->>Token: parse_saas_token(token)
Token->>Token: detect staging/testing
alt token contains "testing" and not staging
Token->>Token: derive endpoint from token suffix<br/>(fallback localhost:50051)
Token-->>Agent: {token, endpoint, is_testing=true}
else staging or production
Token-->>Agent: {token, endpoint, is_testing=false}
end
Agent->>Info: construct SaaSInfo(token, endpoint, testing)
end
Note over Agent: insecure flag parsing now allowed<br/>if insecure_str set and (no SaaS or testing)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip You can customize the tone of the review comments and chat replies.Set the ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (5)
test/agents/test-grpc-packages.mjs (1)
403-404: Bug: Extra argument passed to checkPackagesDatacheckPackagesData(msg, metadata, requestId, agentId) accepts 4 args, but a 5th (nsolidConfig) is passed and ignored. Clean this up to avoid confusion.
Apply this diff:
- checkPackagesData(data.msg, data.metadata, requestId, agentId, nsolidConfig); + checkPackagesData(data.msg, data.metadata, requestId, agentId);test/agents/test-zmq-metrics.mjs (1)
314-324: Ensure server cleanup on failure with try/finallyIf any test throws, the server may remain running. Wrap start/loop/stopClient in try/finally to guarantee stopServer().
-const playground = new TestPlayground(config); -await playground.startServer(); - -for (const { name, test } of tests) { - console.log(`[local] metrics command ${name}`); - await test(playground); - await playground.stopClient(); -} - -await playground.stopServer(); +const playground = new TestPlayground(config); +try { + await playground.startServer(); + for (const { name, test } of tests) { + console.log(`[local] metrics command ${name}`); + await test(playground); + await playground.stopClient(); + } +} finally { + await playground.stopServer(); +}test/agents/test-zmq-snapshot.mjs (1)
246-256: Protect server lifecycle with try/finallyEnsure the server is stopped even if a test throws.
-const playground = new TestPlayground(config); -await playground.startServer(); - -for (const { name, test } of tests) { - console.log(`[local] snapshot command ${name}`); - await test(playground); - await playground.stopClient(); -} - -await playground.stopServer(); +const playground = new TestPlayground(config); +try { + await playground.startServer(); + for (const { name, test } of tests) { + console.log(`[local] snapshot command ${name}`); + await test(playground); + await playground.stopClient(); + } +} finally { + await playground.stopServer(); +}test/agents/test-zmq-heap-profile.mjs (1)
423-433: Harden server teardown with try/finallyGuarantee stopServer() on failures.
-const playground = new TestPlayground(config); -await playground.startServer(); - -for (const { name, test } of tests) { - console.log(`[local] profile command ${name}`); - await test(playground); - await playground.stopClient(); -} - -await playground.stopServer(); +const playground = new TestPlayground(config); +try { + await playground.startServer(); + for (const { name, test } of tests) { + console.log(`[local] profile command ${name}`); + await test(playground); + await playground.stopClient(); + } +} finally { + await playground.stopServer(); +}agents/grpc/src/grpc_agent.cc (1)
1659-1663: Token validation may fail for testing tokens with non-standard console ID length.The validation at line 1660 checks if
console_id.size() != CONSOLE_ID_SIZE(36 characters), but the test tokens use 32 'b' characters. This will cause validation failure and early return at line 1662.Either:
- Update test tokens to use 36 characters for console ID, or
- Relax validation for testing tokens:
- if (console_id.size() != CONSOLE_ID_SIZE) { + bool is_testing = token.find("testing") != std::string::npos; + if (!is_testing && console_id.size() != CONSOLE_ID_SIZE) { Debug("Invalid SaaS token: %s\n", token.c_str()); return; }
♻️ Duplicate comments (2)
test/agents/test-grpc-heap-profile.mjs (1)
350-350: Same SAAS token format issue as in test-grpc-blocked-loop.mjs.The console ID portion should be 36 characters instead of 32.
test/agents/test-grpc-heap-sampling.mjs (1)
328-328: Same SAAS token format issue.
🧹 Nitpick comments (9)
test/agents/test-zmq-handshake.mjs (2)
52-53:saas: falseappears unused for ZMQ playground.If
TestPlaygroundno longer reads this flag in the ZMQ path, remove it to avoid confusion.Apply:
- saas: false,
58-62: Avoid double client teardown (shutdown vs stopClient).Inside the test you call
client.shutdown(0); the harness then callsstopClient(). EnsurestopClient()is idempotent and doesn’t race withshutdown. Optionally, rely on one to reduce flakiness.Example:
- await playground.stopClient(); + // Optional: rely on shutdown inside the test; ensure stopClient is idempotent if kept + // await playground.stopClient();test/agents/test-zmq-ping.mjs (1)
72-73: Verify fixed-port usage across all ZMQ tests to avoid collisionsWe found 14 ZMQ test files binding to the same TCP ports (9001–9003). If your test runner executes files in parallel, you’ll hit EADDRINUSE. Please ensure one of the following before merging:
- Run these tests sequentially (disable file-level parallelism in your test runner).
- Bind explicitly to loopback (
tcp://127.0.0.1:<port>) instead of*to limit exposure.- Use dynamic port assignment (e.g. pick an ephemeral port per test via an environment variable or a port-finder library).
Affected files:
- test/agents/test-zmq-basic.mjs
- test/agents/test-zmq-blocked-loop.mjs
- test/agents/test-zmq-startup-times.mjs
- test/agents/test-zmq-tracing.mjs
- test/agents/test-zmq-snapshot.mjs
- test/agents/test-zmq-reconfigure.mjs
- test/agents/test-zmq-packages.mjs
- test/agents/test-zmq-metrics.mjs
- test/agents/test-zmq-heap-profile.mjs
- test/agents/test-zmq-ping.mjs
- test/agents/test-zmq-handshake.mjs
- test/agents/test-zmq-profile.mjs
- test/agents/test-zmq-info.mjs
- test/agents/test-zmq-heap-sampling.mjs
No code changes are strictly required if execution is guaranteed to be serial; please verify your test-runner configuration accordingly.
test/parallel/test-nsolid-config-saas.js (1)
108-109: Nit: trailing comma is fineTrailing comma is stylistic; harmless in this object literal.
test/agents/test-grpc-metrics.mjs (1)
254-275: DRY suggestion: centralize env buildersMultiple gRPC test files embed the same two getEnv variants. Consider factoring an env factory into test/common to reduce duplication and keep tokens consistent.
I can extract a shared getEnv factory/token helper under test/common if you want.
test/agents/test-grpc-info.mjs (1)
100-138: Optional: dedupe test env definitionsSame two getEnv variants appear across files. A shared builder (e.g., in test/common) would reduce duplication and keep the testing token uniform.
test/agents/test-grpc-reconfigure.mjs (1)
125-125: Minor: Remove unnecessary blank lineThere's an extra blank line here that can be removed for consistency.
- }; - + }; const client = new TestClient([], opts);test/agents/test-grpc-source-code.mjs (1)
350-350: Maintain consistent console log format across test filesThe console log format here differs from other test files which use brackets (e.g.,
[basic],[reconfigure]). Consider maintaining consistency.- console.log(`source code ${name}`); + console.log(`[source code] ${name}`);agents/grpc/src/grpc_agent.cc (1)
1672-1679: Consider making the fallback endpoint configurable.The hardcoded fallback to
localhost:50051at line 1678 might not work in all testing environments. Consider using an environment variable or making this configurable.- endpoint = "localhost:50051"; // fallback if no dot is found + // Fallback to environment variable or default + auto fallback = per_process::system_environment->Get("NSOLID_TEST_ENDPOINT"); + endpoint = fallback.value_or("localhost:50051");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
agents/grpc/src/grpc_agent.cc(2 hunks)agents/grpc/src/grpc_agent.h(1 hunks)lib/nsolid.js(1 hunks)test/agents/test-grpc-basic.mjs(9 hunks)test/agents/test-grpc-blocked-loop.mjs(5 hunks)test/agents/test-grpc-continuous-profile.mjs(10 hunks)test/agents/test-grpc-heap-profile.mjs(9 hunks)test/agents/test-grpc-heap-sampling.mjs(9 hunks)test/agents/test-grpc-info.mjs(1 hunks)test/agents/test-grpc-logs.mjs(5 hunks)test/agents/test-grpc-metrics.mjs(1 hunks)test/agents/test-grpc-packages.mjs(1 hunks)test/agents/test-grpc-profile.mjs(11 hunks)test/agents/test-grpc-reconfigure.mjs(3 hunks)test/agents/test-grpc-snapshot.mjs(7 hunks)test/agents/test-grpc-source-code.mjs(7 hunks)test/agents/test-grpc-startup-times.mjs(3 hunks)test/agents/test-grpc-tracing.mjs(5 hunks)test/agents/test-zmq-basic.mjs(1 hunks)test/agents/test-zmq-blocked-loop.mjs(1 hunks)test/agents/test-zmq-handshake.mjs(2 hunks)test/agents/test-zmq-heap-profile.mjs(1 hunks)test/agents/test-zmq-heap-sampling.mjs(1 hunks)test/agents/test-zmq-info.mjs(1 hunks)test/agents/test-zmq-metrics.mjs(1 hunks)test/agents/test-zmq-packages.mjs(1 hunks)test/agents/test-zmq-ping.mjs(1 hunks)test/agents/test-zmq-profile.mjs(1 hunks)test/agents/test-zmq-reconfigure.mjs(1 hunks)test/agents/test-zmq-snapshot.mjs(1 hunks)test/agents/test-zmq-startup-times.mjs(1 hunks)test/agents/test-zmq-tracing.mjs(1 hunks)test/common/nsolid-grpc-agent/index.js(2 hunks)test/parallel/test-nsolid-config-saas.js(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-08T14:46:47.806Z
Learnt from: santigimeno
PR: nodesource/nsolid#339
File: test/addons/nsolid-elf-utils/nsolid-elf-utils.js:13-14
Timestamp: 2025-07-08T14:46:47.806Z
Learning: In the nsolid test suite, native addon bindings are expected to be built beforehand; tests do not add fs.existsSync guards to skip when bindings are missing.
Applied to files:
test/agents/test-grpc-profile.mjstest/agents/test-grpc-heap-sampling.mjstest/agents/test-grpc-source-code.mjstest/agents/test-grpc-heap-profile.mjstest/agents/test-grpc-snapshot.mjs
📚 Learning: 2025-07-08T14:47:34.724Z
Learnt from: santigimeno
PR: nodesource/nsolid#339
File: test/addons/nsolid-elf-utils/nsolid-elf-utils.js:16-18
Timestamp: 2025-07-08T14:47:34.724Z
Learning: In the nsolid test suite, external tool invocations (e.g., execSync to run `readelf`) are intentionally left uncaught so that any failure causes the test to fail rather than being skipped.
Applied to files:
test/agents/test-grpc-snapshot.mjs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test-tarball-linux
- GitHub Check: test-macOS
- GitHub Check: coverage-windows
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-linux (ubuntu-24.04)
🔇 Additional comments (47)
test/agents/test-zmq-handshake.mjs (2)
12-12: Single-path (local) test: confirm SAAS/GRPC coverage exists elsewhere.Test now only exercises local ZMQ handshake. Please confirm the SAAS/GRPC handshake path is covered in the new/updated GRPC test suite to avoid regressions.
16-18: State defaults: confirmdetectInitialSequenceexpectations.Initialize to
events: 0andauthCount: 0is reasonable. Please confirmdetectInitialSequencestill expects this shape/initialization in the single-path (local) flow.test/agents/test-zmq-packages.mjs (1)
223-223: Explicitly disabling SaaS for this ZMQ-only test is correctThis suite exercises the ZMQ agent path; making saas: false explicit avoids mixing SaaS (gRPC) paths here. LGTM.
test/agents/test-grpc-packages.mjs (1)
421-429: Add SAAS test config (good coverage)Running the same test under GRPC and SAAS envs is great. The token includes "testing", matching the new testing mode behavior.
test/agents/test-zmq-metrics.mjs (1)
311-311: Explicitly pin saas: false (clarity)Explicitly setting saas: false clarifies the local-only ZMQ path for this suite. LGTM.
test/common/nsolid-grpc-agent/index.js (1)
321-325: Env injection for SAAS (sensible default)Conditionally injecting NSOLID_SAAS when options.saas is set is reasonable, and later options.env can still override it due to the subsequent merge. LGTM.
test/agents/test-zmq-snapshot.mjs (1)
243-243: Explicit saas: false (clarity)Keeps this ZMQ suite explicitly local. Looks good.
test/parallel/test-nsolid-config-saas.js (1)
76-78: Behavior change: command now undefined after start({ saas })This aligns with “only use gRPC to connect to SaaS” (i.e., don’t set the ZMQ command). Good assertion update.
test/agents/test-grpc-metrics.mjs (1)
265-274: Add SAAS run (good coverage)Nice to exercise metrics under SAAS with the testing token. Keeps NSOLID_INTERVAL parity across configs.
test/agents/test-zmq-heap-profile.mjs (1)
419-419: Explicit saas: false (clarity)Matches the single local ZMQ harness used elsewhere. LGTM.
test/agents/test-grpc-info.mjs (1)
111-120: Add SAAS env variant (good)Covers info path under SAAS using a token with “testing”. Consistent with the rest of the suite.
test/agents/test-zmq-heap-sampling.mjs (1)
440-453: Summary inconsistency detectedThe AI summary incorrectly states that this file introduces a two-configuration test harness with
testConfigsandgetEnvpatterns similar to grpc tests. However, the actual code changes show this file follows the same local-only pattern as the other zmq tests (single playground withsaas: false).The changes correctly implement the single local configuration pattern consistent with other zmq test files in this PR.
test/agents/test-zmq-startup-times.mjs (1)
104-116: LGTM!The changes correctly implement the single local configuration pattern, consistent with other zmq test files in this PR.
test/agents/test-zmq-profile.mjs (1)
357-369: LGTM!The changes correctly implement the single local configuration pattern, consistent with other zmq test files in this PR.
test/agents/test-zmq-reconfigure.mjs (1)
166-178: LGTM!The changes correctly implement the single local configuration pattern, consistent with other zmq test files in this PR.
test/agents/test-zmq-info.mjs (1)
147-159: LGTM!The changes correctly implement the single local configuration pattern, consistent with other zmq test files in this PR.
test/agents/test-grpc-logs.mjs (3)
146-176: LGTM!The test parameterization with
getEnvcallback is clean and allows for flexible environment configuration.
214-235: LGTM!The
testConfigsstructure properly supports both GRPC and SAAS testing modes. The SAAS token format follows the expected pattern with the 'testing' identifier.
237-242: LGTM!The nested loop execution pattern ensures each test runs against all configurations systematically.
test/agents/test-zmq-tracing.mjs (2)
412-413: LGTM!The addition of
saas: falseexplicitly configures the test for local mode, which is appropriate for ZMQ tests.
415-425: LGTM!The simplified lifecycle with a single playground instance that starts once and stops once is cleaner and more efficient than the previous per-test instantiation.
test/agents/test-zmq-blocked-loop.mjs (1)
216-228: LGTM!The ZMQ test refactoring follows a consistent pattern with other ZMQ tests, using a single local playground instance with simplified lifecycle management.
test/agents/test-zmq-basic.mjs (1)
100-112: LGTM!The refactoring maintains consistency with other ZMQ tests using the single local playground pattern.
lib/nsolid.js (2)
718-718: Consistent use ofnullfor unset commandGood change from
undefinedtonullfor consistency in representing an unset command field.
716-731: No SAAS command references foundVerification via searching for any usage of
nsolidConfig.commandin conjunction withsaas, as well as ZMQ-specific SAAS handling, returned no results. The assignment ofcommand = urlwhen SAAS is configured has no remaining consumers elsewhere in the codebase. Exclusively using gRPC for SaaS connections is therefore intentional and safe.test/agents/test-grpc-tracing.mjs (2)
186-220: LGTM!The test parameterization for HTTP tracing is clean and consistent with other GRPC tests.
262-285: LGTM!The
testConfigssetup properly supports both GRPC and SAAS testing modes with the correct token format including the 'testing' identifier.test/agents/test-grpc-profile.mjs (2)
76-108: LGTM!The test parameterization for CPU profiling maintains the existing test logic while adding environment flexibility.
356-382: LGTM!The test configuration structure is consistent with other GRPC tests in the PR.
test/agents/test-grpc-snapshot.mjs (3)
74-108: LGTM!The test parameterization for heap snapshots follows the established pattern.
295-295: Good fix for the log prefixThe correction from "[heap profile]" to "[heap snapshot]" accurately reflects what this test file is testing.
272-291: LGTM!The test configuration setup is consistent with the rest of the GRPC test suite refactoring.
test/agents/test-grpc-basic.mjs (2)
16-38: Test refactoring looks good!The parameterization of test functions to accept
getEnvcallback is implemented correctly. Each test properly uses the environment factory to create the test environment while preserving the original test logic.Also applies to: 42-67, 71-96, 100-126
128-154: Test configuration matrix implementation is correct!The dual configuration approach properly tests both NSOLID_GRPC and NSOLID_SAAS paths. The SAAS token format with the "testing" string aligns with the PR objective of allowing test SaaS tokens.
test/agents/test-grpc-reconfigure.mjs (2)
69-96: Test parameterization implemented correctly!Both test functions properly accept and use the
getEnvcallback to create test environments while maintaining the original test logic.Also applies to: 116-168
170-196: Test execution harness correctly implements dual configuration testing!The testConfigs and execution loop follow the same pattern as other test files, ensuring consistency across the test suite.
test/agents/test-grpc-source-code.mjs (2)
79-182: Test parameterization correctly applied to all three test functions!The refactoring properly converts all tests to use the
getEnvcallback pattern while preserving the original test logic for main thread, worker thread, and data URL import scenarios.Also applies to: 186-274, 278-325
327-353: Test configuration matrix properly implements dual environment testing!The testConfigs setup and execution loop correctly follow the established pattern for running tests under both GRPC and SAAS configurations.
test/agents/test-grpc-startup-times.mjs (2)
75-94: Test functions correctly refactored for parameterized environment setup!Both tests properly accept and utilize the
getEnvcallback to create test environments while maintaining their original startup times validation logic.Also applies to: 98-118
120-148: Test configuration includes appropriate blocked loop threshold!The testConfigs correctly include
NSOLID_BLOCKED_LOOP_THRESHOLD: 100in both configurations, which is appropriate for startup times testing. The execution loop properly runs each test under both configurations.test/agents/test-grpc-continuous-profile.mjs (2)
83-117: All five test functions correctly refactored for parameterized testing!Each test properly accepts the
getEnvcallback and uses it to create test environments. The original continuous profiling test logic is preserved while enabling dual configuration testing.Also applies to: 121-165, 169-207, 211-262, 266-326
328-358: Test configurations properly include continuous profiling settings!The testConfigs correctly include
NSOLID_CONT_CPU_PROFILEandNSOLID_CONT_CPU_PROFILE_INTERVALsettings in both GRPC and SAAS configurations, enabling comprehensive testing of continuous CPU profiling functionality under both connection modes.test/agents/test-grpc-blocked-loop.mjs (1)
147-147: LGTM! Clean implementation of parameterized test pattern.The conversion to accept
getEnvparameter and dynamically generate environment variables enables proper testing of both GRPC and SAAS connection modes.Also applies to: 162-163, 178-178, 193-194
test/agents/test-grpc-heap-profile.mjs (1)
86-86: LGTM! Consistent test parameterization pattern.The implementation follows the same clean pattern for parameterized testing across all test cases in this file.
Also applies to: 90-90, 125-125, 129-129, 167-167, 171-171, 195-195, 199-199, 227-227, 231-231, 261-261, 276-276, 310-310, 314-314
test/agents/test-grpc-heap-sampling.mjs (1)
75-75: LGTM! Consistent parameterization across all test cases.The test parameterization is uniformly applied.
Also applies to: 79-79, 114-114, 118-118, 155-155, 159-159, 183-183, 187-187, 215-215, 219-219, 250-250, 260-260, 288-288, 292-292
agents/grpc/src/grpc_agent.h (1)
145-149: LGTM! Clean addition of testing flag to SaaSInfo.The
testingfield properly tracks whether the SAAS token is for testing purposes.agents/grpc/src/grpc_agent.cc (1)
1037-1040: LGTM! Appropriate relaxation of security for testing mode.Allowing insecure connections when
saas_->testingis true enables proper local testing while maintaining security in production SAAS mode.
santigimeno
force-pushed
the
santi/prepare_for_6
branch
from
dabf705 to
e76ce39
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
agents/grpc/src/grpc_agent.cc (1)
1669-1691: Testing endpoint extraction via last '.' breaks IPv4/multi-label hosts; derive from baseUrl instead.Using token.rfind('.') and taking the suffix drops most of an IP like 127.0.0.1:50051 (you’d get "1:50051"). You already parsed baseUrl and basePort—use them to strip the console_id prefix and preserve the full host, whether it’s localhost, IPv4, or a multi-label domain.
Apply this diff to make testing endpoint derivation resilient:
- is_testing = token.find("testing") != std::string::npos; - if (is_testing) { - // For testing, set endpoint to the string after the last dot in the token - size_t last_dot = token.rfind('.'); - if (last_dot != std::string::npos && last_dot + 1 < token.size()) { - endpoint = token.substr(last_dot + 1); - } else { - endpoint = "localhost:50051"; // fallback if no dot is found - } - } else { + is_testing = token.find("testing") != std::string::npos; + if (is_testing) { + // For testing, strip the console_id prefix from baseUrl and keep full host + size_t first_dot = baseUrl.find('.'); + if (first_dot != std::string::npos && first_dot + 1 < baseUrl.size() && !basePort.empty()) { + std::string host = baseUrl.substr(first_dot + 1); + endpoint = host + ":" + basePort; + } else { + endpoint = "localhost:50051"; // conservative fallback + } + } else { endpoint = console_id + ".grpc.nodesource.io:443"; }Note: This keeps behavior identical for the canonical "…testing.localhost:port" tokens while fixing IP and multi-label host cases.
🧹 Nitpick comments (2)
agents/grpc/src/grpc_agent.cc (2)
1033-1041: Guard against invalid NSOLID_GRPC_INSECURE values (std::stoi can throw).If the env var contains a non-integer (e.g., "true"), std::stoi throws and will bring the agent down during config. For a test-only flag, fail-safe parsing is preferable.
Apply this diff to make parsing robust and default to secure on malformed values:
- if (insecure_str.has_value() && (!saas_ || saas_->testing)) { - // insecure = std::stoull(insecure_str.value()); - insecure = std::stoi(insecure_str.value()); - } + if (insecure_str.has_value() && (!saas_ || saas_->testing)) { + try { + insecure = std::stoi(insecure_str.value()) != 0; + } catch (const std::exception&) { + Debug("Invalid NSOLID_GRPC_INSECURE value: %s; defaulting to secure\n", + insecure_str.value().c_str()); + insecure = false; + } + }
1648-1649: Avoid logging full SaaS tokens in debug logs.This prints the entire token, which is sensitive. Even in debug builds, scrubbing secrets avoids accidental leakage in CI or support bundles.
[security]
Consider logging only derived fields (console_id length, staging/testing flags, endpoint), e.g.:
- "Parsing SaaS token (console_id_len=36, testing=1, endpoint=localhost:50051)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
agents/grpc/src/grpc_agent.cc(2 hunks)agents/grpc/src/grpc_agent.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- agents/grpc/src/grpc_agent.h
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-20T11:17:56.482Z
Learnt from: santigimeno
PR: nodesource/nsolid#354
File: test/agents/test-grpc-blocked-loop.mjs:224-224
Timestamp: 2025-08-20T11:17:56.482Z
Learning: In SAAS token validation for gRPC agents, the console_id must be exactly CONSOLE_ID_SIZE (36) characters long. Test tokens like "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbbtesting.localhost:port" work correctly because after extracting the pubKey (first 40 chars), the console_id portion ("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbtesting") is exactly 36 characters, satisfying the validation requirement.
Applied to files:
agents/grpc/src/grpc_agent.cc
📚 Learning: 2025-08-20T11:17:56.482Z
Learnt from: santigimeno
PR: nodesource/nsolid#354
File: test/agents/test-grpc-blocked-loop.mjs:224-224
Timestamp: 2025-08-20T11:17:56.482Z
Learning: In the gRPC agent token parsing (agents/grpc/src/grpc_agent.cc), CONSOLE_ID_SIZE = 36 is used for generating console IDs, not for validating console ID length during SAAS token parsing. Token parsing does not enforce console ID length constraints, so test tokens with console ID portions of different lengths (like 32 characters) work correctly.
Applied to files:
agents/grpc/src/grpc_agent.cc
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test-macOS
- GitHub Check: lint-js-and-md
- GitHub Check: build-docs
- GitHub Check: build-tarball
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: coverage-windows
🔇 Additional comments (2)
agents/grpc/src/grpc_agent.cc (2)
1689-1691: All SaaSInfo initializations include the newtestingflagVerified via repository-wide search—only one instantiation remains:
- agents/grpc/src/grpc_agent.cc:1689–1691
saas_ = std::make_unique<SaaSInfo>(SaaSInfo{token, std::move(endpoint), is_testing});No other call sites found.
1663-1667: Ignore strict console_id length concern—tests use a 36-char ID.Verified the SAAS token in test/agents/test-grpc-blocked-loop.mjs:
- PubKey: first 40 chars
- Console ID: “bbbbbbbbbbbbbbbbbbbbbbbbbbbbbtesting” (length 36)
No test will be rejected by the
console_id.size() != 36check. Feel free to disregard the prior length-check warning.Likely an incorrect or invalid review comment.
For testing, allow saas tokens with `testing` string.
santigimeno
force-pushed
the
santi/prepare_for_6
branch
from
e76ce39 to
d1db06a
Compare
For testing, allow saas tokens with `testing` string. PR-URL: #354 Reviewed-By: Juan José Arboleda <[email protected]>
|
Landed in 5846312 |
For testing, allow saas tokens with `testing` string. PR-URL: #354 Reviewed-By: Juan José Arboleda <[email protected]>
For testing, allow saas tokens with `testing` string. PR-URL: #354 Reviewed-By: Juan José Arboleda <[email protected]> PR-URL: #359 Reviewed-By: Rafael Gonzaga <[email protected]>
For testing, allow saas tokens with
testingstring.Summary by CodeRabbit
New Features
Bug Fixes
Tests