chore(router-tests): bump nats client lib + add router-tests to verify nats tls#2788
chore(router-tests): bump nats client lib + add router-tests to verify nats tls#2788dkorittki wants to merge 14 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an end-to-end NATS TLS/mTLS router test (with subscribe→publish→assert flows), TLS certificate generation helpers for tests, and bumps NATS and multiple golang.org/x/* dependency versions in two go.mod files. ChangesNATS TLS tests and helpers
Dependency updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2788 +/- ##
==========================================
+ Coverage 66.16% 66.22% +0.06%
==========================================
Files 256 256
Lines 26956 26968 +12
==========================================
+ Hits 17835 17860 +25
+ Misses 7712 7702 -10
+ Partials 1409 1406 -3 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router-tests/go.mod (1)
35-36:⚠️ Potential issue | 🟠 MajorPin
go.opentelemetry.io/otel/sdktov1.40.0or later in this module.Lines 35 and 209 currently pin vulnerable versions (
v1.39.0andv1.28.0replacement) ofgo.opentelemetry.io/otel/sdk. The module includesgoogle.golang.org/grpc v1.79.3at line 43, which transitively requires the patched version to address the PATH hijacking vulnerability (GHSA-9h8m-3fm2-qjrq / CVE-2026-24051).🔧 Suggested change
- go.opentelemetry.io/otel/sdk v1.39.0 + go.opentelemetry.io/otel/sdk v1.40.0 ... - go.opentelemetry.io/otel/sdk => go.opentelemetry.io/otel/sdk v1.28.0 + go.opentelemetry.io/otel/sdk => go.opentelemetry.io/otel/sdk v1.40.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/go.mod` around lines 35 - 36, Update the pinned opentelemetry SDK to a patched version: change all occurrences that pin go.opentelemetry.io/otel/sdk (including the direct require at the top-level and any replace entries) from v1.39.0 or v1.28.0 to v1.40.0 or later; specifically update the require line(s) and any replace directive referencing go.opentelemetry.io/otel/sdk to use v1.40.0+ and run `go mod tidy` to ensure the module graph is consistent.
🧹 Nitpick comments (1)
router/pkg/pubsub/nats/provider_builder_test.go (1)
276-312: Consider adding KeyUsage to client certificate for consistency.The unit test certificate helper omits
KeyUsageandExtKeyUsageon the client certificate, while the integration test (nats_tls_test.go) correctly setsKeyUsage: x509.KeyUsageDigitalSignatureandExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}. While this doesn't affect these unit tests (they only inspect options, not actual TLS handshakes), aligning the helpers would improve consistency.♻️ Optional: Add KeyUsage to client certificate
clientTemplate := &x509.Certificate{ SerialNumber: big.NewInt(2), Subject: pkix.Name{CommonName: "test-client"}, NotBefore: time.Now().Add(-time.Hour), NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/pubsub/nats/provider_builder_test.go` around lines 276 - 312, The client cert created in generateTestCerts lacks KeyUsage/ExtKeyUsage; update the clientTemplate in generateTestCerts to set KeyUsage: x509.KeyUsageDigitalSignature and ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth} (matching the nats_tls_test.go integration helper) so the generated client certificate includes the proper usages for client auth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@router-tests/go.mod`:
- Around line 35-36: Update the pinned opentelemetry SDK to a patched version:
change all occurrences that pin go.opentelemetry.io/otel/sdk (including the
direct require at the top-level and any replace entries) from v1.39.0 or v1.28.0
to v1.40.0 or later; specifically update the require line(s) and any replace
directive referencing go.opentelemetry.io/otel/sdk to use v1.40.0+ and run `go
mod tidy` to ensure the module graph is consistent.
---
Nitpick comments:
In `@router/pkg/pubsub/nats/provider_builder_test.go`:
- Around line 276-312: The client cert created in generateTestCerts lacks
KeyUsage/ExtKeyUsage; update the clientTemplate in generateTestCerts to set
KeyUsage: x509.KeyUsageDigitalSignature and ExtKeyUsage:
[]x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth} (matching the nats_tls_test.go
integration helper) so the generated client certificate includes the proper
usages for client auth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecf47630-b184-49c4-9242-4cfb21e795c2
⛔ Files ignored due to path filters (1)
router-tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
docs-website/router/configuration.mdxrouter-tests/events/nats_tls_test.gorouter-tests/go.modrouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/config_events_nats_test.gorouter/pkg/config/testdata/config_full.jsonrouter/pkg/pubsub/nats/provider_builder.gorouter/pkg/pubsub/nats/provider_builder_test.go
dkorittki
force-pushed
the
dominik/eng-9395-add-nats-tlsmtls-router-tests
branch
from
2a69264 to
011df23
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router-tests/events/nats_tls_test.go`:
- Around line 223-244: Start the client.Run() goroutine and immediately register
cleanup to ensure the websocket/goroutine is always stopped: after creating
clientErrCh and launching go func() { clientErrCh <- client.Run() }(), call
t.Cleanup (or defer in scope) to call client.Close() and wait for clientErrCh so
the goroutine is closed even if earlier require.* assertions fail; reference
client.Run(), client.Close(), clientErrCh and resultCh when adding the cleanup
to guarantee proper teardown in all test failure paths.
- Around line 345-370: The test currently sets both a tls:// URL and an explicit
TLS config (routerTLS), so it cannot prove scheme-only behavior; update the test
in the "router uses TLS when URL scheme is tls://" block to pass a nil TLS
config to tlsNATSEventSources (remove or set routerTLS to nil) so the router
must rely on the tls:// scheme alone to enable TLS; keep using tlsSchemeURL
(returned by srv.ClientURL()) and the insecure client connect helper
(connectInsecureTLSNATSClient) to allow the self-signed server cert while
ensuring provider_builder.go's scheme handling is actually exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07c5ff43-3898-4c1a-942e-8d179e0921e6
⛔ Files ignored due to path filters (1)
router-tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
router-tests/events/nats_tls_test.gorouter-tests/go.mod
✅ Files skipped from review due to trivial changes (1)
- router-tests/go.mod
|
fyi I force pushed to rebase the actual change onto main after the original pull request got merged to main. |
To have version parity between router-tests and router. And because it doesn't hurt to update deps once in a while. It's declared as a non-breaking change.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/go.mod (1)
46-46:⚠️ Potential issue | 🔴 CriticalCritical: Upgrade
go.opentelemetry.io/otel/sdkto v1.40.0+ to fix GHSA-9h8m-3fm2-qjrq.The current version v1.39.0 (and the replace directive at line 191 pinning to v1.28.0) is vulnerable to GHSA-9h8m-3fm2-qjrq (CVE-2026-24051), a PATH hijacking vulnerability on macOS/Darwin. Explicitly pin this dependency to v1.40.0 or later in both the require section and the replace directive. Based on learnings, PR
#2668was meant to address this for the router module, but the vulnerability remains.🔒 Proposed fix to upgrade OTel SDK
Update the require section:
- go.opentelemetry.io/otel/sdk v1.39.0 + go.opentelemetry.io/otel/sdk v1.40.0Update the replace directive:
- go.opentelemetry.io/otel/sdk => go.opentelemetry.io/otel/sdk v1.28.0 + go.opentelemetry.io/otel/sdk => go.opentelemetry.io/otel/sdk v1.40.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/go.mod` at line 46, The go.opentelemetry.io/otel/sdk dependency is pinned to v1.39.0 in the require block and additionally replaced/pinned to v1.28.0 (via a replace directive), leaving the module vulnerable to GHSA-9h8m-3fm2-qjrq; update the require line for go.opentelemetry.io/otel/sdk to v1.40.0 or later and change/remove the replace directive that pins it to v1.28.0 so the module resolves to >= v1.40.0 (ensure the same version is used in both the require and any replace entries referencing go.opentelemetry.io/otel/sdk).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@router/go.mod`:
- Line 46: The go.opentelemetry.io/otel/sdk dependency is pinned to v1.39.0 in
the require block and additionally replaced/pinned to v1.28.0 (via a replace
directive), leaving the module vulnerable to GHSA-9h8m-3fm2-qjrq; update the
require line for go.opentelemetry.io/otel/sdk to v1.40.0 or later and
change/remove the replace directive that pins it to v1.28.0 so the module
resolves to >= v1.40.0 (ensure the same version is used in both the require and
any replace entries referencing go.opentelemetry.io/otel/sdk).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42c49a85-9e94-405c-a856-f43545650b61
⛔ Files ignored due to path filters (1)
router/go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
router/go.mod
There was a problem hiding this comment.
♻️ Duplicate comments (1)
router-tests/events/nats_tls_test.go (1)
277-283:⚠️ Potential issue | 🟡 MinorThis case still doesn't isolate
tls://handling.Because this subtest passes a non-nil
NatsTLSConfiguration, it can still succeed even if the router ignores thetls://scheme and enables TLS only from the explicit config. Either make the scheme the only TLS signal here, or rename the case so it doesn't claim scheme-only coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/events/nats_tls_test.go` around lines 277 - 283, The test currently supplies a non-nil NatsTLSConfiguration (routerTLS) which lets TLS succeed via explicit config, so change the test to truly exercise scheme-only TLS: remove or set nil the routerTLS passed into ModifyEventsConfiguration (stop calling tlsNATSEventSourceConfig with a non-nil NatsTLSConfiguration) and rely solely on the tls:// scheme in tlsSchemeURL, or alternatively rename the subtest to indicate it verifies explicit-config+scheme rather than scheme-only; update references to routerTLS, tlsNATSEventSourceConfig, and ModifyEventsConfiguration/EventsConfiguration accordingly so the test either uses only the scheme as the TLS signal or the name reflects the actual behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@router-tests/events/nats_tls_test.go`:
- Around line 277-283: The test currently supplies a non-nil
NatsTLSConfiguration (routerTLS) which lets TLS succeed via explicit config, so
change the test to truly exercise scheme-only TLS: remove or set nil the
routerTLS passed into ModifyEventsConfiguration (stop calling
tlsNATSEventSourceConfig with a non-nil NatsTLSConfiguration) and rely solely on
the tls:// scheme in tlsSchemeURL, or alternatively rename the subtest to
indicate it verifies explicit-config+scheme rather than scheme-only; update
references to routerTLS, tlsNATSEventSourceConfig, and
ModifyEventsConfiguration/EventsConfiguration accordingly so the test either
uses only the scheme as the TLS signal or the name reflects the actual behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23b47638-6f2b-4589-8f4e-37fbcba51f63
📒 Files selected for processing (2)
router-tests/events/helpers_test.gorouter-tests/events/nats_tls_test.go
dkorittki
requested review from
Noroth,
SkArchon,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
dkorittki
changed the title
SkArchon
left a comment
SkArchon
left a comment
There was a problem hiding this comment.
Changes look ok in general, few questions
2 participants
This adds integration tests to the PR #2749 from @vatsalpatel .
I made a noteworthy design decision for these new tests: Currently nats tests use the nats instance from docker-compose or as defined in CI. In order to test TLS I would have to configure the instance to allow for TLS and worse: require mTLS. To avoid this instead I spawn an in-process nats server configured to require TLS, instead of using the one from docker-compose or CI. This allows developers to keep their plain text connection. Also other nats test focusing on other things don't need to bother with TLS. The newly added tests are the only ones who need to bother with TLS/mTLS.
In order to use a nats in-process server the router-tests go.mod needs a new dependency
github.com/nats-io/nats-server/v2(and nats.go upgraded from v1.35.0 → v1.50.0 as a transitive requirement — this is a backwards-compatible minor version bump). To keep version parity between router and router-tests I updated the router nats dependencies, too.Summary by CodeRabbit
Tests
Chores
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.