chore(router): refactor TLS configuration#2863
Merged
dkorittki merged 24 commits intoMay 19, 2026
Merged
Conversation
Contributor
|
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:
WalkthroughRefactors TLS handling: nested router TLS types, serverHTTPTLSConfig() compiles *tls.Config at router init, server construction accepts *tls.Config and checks http.Server.TLSConfig for TLS, and tests/testenv updated to the new config shape. ChangesTLS Configuration Extraction and Server Refactoring
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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. Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
router/core/router.go (1)
832-836: ⚡ Quick winSet an explicit TLS minimum version in server config.
Go's default for
tls.Config.MinVersionis already TLS 1.2 for servers, which aligns with current recommendations. However, explicitly settingMinVersionmakes the intent clear in code and protects against unintended behavior if defaults change in future Go releases.🔐 Proposed change
return &tls.Config{ + MinVersion: tls.VersionTLS12, ClientCAs: caCertPool, Certificates: []tls.Certificate{cer}, ClientAuth: clientAuthMode, }, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/core/router.go` around lines 832 - 836, The TLS server config returned from the function constructs a tls.Config without an explicit MinVersion; update the returned tls.Config (the struct created where you set ClientCAs, Certificates and ClientAuth) to also set MinVersion to tls.VersionTLS12 to make the minimum TLS version explicit. Modify the tls.Config literal in router.go (where the function returns &tls.Config{...}) to include MinVersion: tls.VersionTLS12 so the intent is clear and future Go default changes won’t affect your server.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router/core/router.go`:
- Around line 804-821: Currently if r.tlsConfig.ClientAuth.Required is true but
ClientAuth.CertFile is empty the code falls back to NoClientCert; change the
logic in the TLS setup (the block using r.tlsConfig.ClientAuth.CertFile,
caCertPool and clientAuthMode) to validate that when
r.tlsConfig.ClientAuth.Required is true there is a non-empty CertFile and a
successfully loaded/parsed CA (caCertPool), otherwise return a clear error; only
set clientAuthMode to tls.RequireAndVerifyClientCert or
tls.VerifyClientCertIfGiven after confirming the CA was loaded, and never
silently use tls.NoClientCert when Required is true.
---
Nitpick comments:
In `@router/core/router.go`:
- Around line 832-836: The TLS server config returned from the function
constructs a tls.Config without an explicit MinVersion; update the returned
tls.Config (the struct created where you set ClientCAs, Certificates and
ClientAuth) to also set MinVersion to tls.VersionTLS12 to make the minimum TLS
version explicit. Modify the tls.Config literal in router.go (where the function
returns &tls.Config{...}) to include MinVersion: tls.VersionTLS12 so the intent
is clear and future Go default changes won’t affect your server.
🪄 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: 65a530b5-f421-4d4e-9da1-43d7ff8a1ebe
📒 Files selected for processing (3)
router/core/http_server.gorouter/core/router.gorouter/core/router_config.go
💤 Files with no reviewable changes (1)
- router/core/router_config.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2863 +/- ##
===========================================
+ Coverage 33.36% 66.00% +32.64%
===========================================
Files 633 256 -377
Lines 89449 26971 -62478
Branches 5359 0 -5359
===========================================
- Hits 29847 17803 -12044
+ Misses 59273 7747 -51526
- Partials 329 1421 +1092
🚀 New features to boost your workflow:
|
dkorittki
force-pushed
the
dominik/eng-9572-improve-tls-server-configuration
branch
from
05fab0d to
dda275c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router/core/router_config.go`:
- Around line 260-261: The usage map currently sets "tls_client" based only on
c.tls != nil which overreports client TLS; change it to detect actual client TLS
settings by checking the real client TLS sub-structure and its meaningful fields
(e.g. replace c.tls != nil with a check like c.tls.Client != nil &&
(c.tls.Client.CA != "" || c.tls.Client.Cert != "" || c.tls.Client.Key != "" ||
c.tls.Client.InsecureSkipVerify || c.tls.Client.ServerName != "" ) or call an
existing helper like HasClientTLS() if present) so that usage["tls_client"]
reflects whether client TLS is actually configured rather than just the wrapper
being allocated.
In `@router/core/router.go`:
- Around line 2378-2381: WithTLSConfig currently replaces r.tls wholesale and
can drop configured client branches (e.g., r.tls.Client.Subgraphs.HTTP) set
earlier by WithSubgraphTLSConfiguration in newRouter; change WithTLSConfig to
merge the incoming cfg into the existing r.tls (preserving non-nil subtrees like
r.tls.Client and r.tls.Client.Subgraphs.HTTP) instead of assigning it, or
alternatively consolidate TLS option handling so server/client TLS aren't split
across separate options; update the Option function returned by WithTLSConfig
(the closure that mutates *Router) to perform a field-wise merge/overwrite only
for non-nil fields, referencing Router.r.tls and the TlsConfig.Client/Subgraphs
structures to ensure existing client mTLS entries remain intact.
🪄 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: 3c05c1d7-72e9-4aa2-a8bc-c2da4b4beb6b
📒 Files selected for processing (5)
router/core/graph_server.gorouter/core/http_server.gorouter/core/router.gorouter/core/router_config.gorouter/core/supervisor_instance.go
🚧 Files skipped from review as they are similar to previous changes (1)
- router/core/http_server.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router-tests/testenv/testenv.go`:
- Around line 725-728: The helper currently calls log.Fatal after os.ReadFile
when loading cfg.TLSConfig.Server.HTTP.Settings.CertFile (caCert, err :=
os.ReadFile(...)); replace the process-killing log.Fatal(err) with a test
assertion like require.NoError(t, err) so the current test fails instead of
terminating the whole suite, and ensure the helper has access to *testing.T (add
a t parameter if needed) and the testify/require import; make the same change
for the other occurrence around the caCert read at lines 1156-1159.
🪄 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: 88194c05-ea5d-4d4c-bc1d-467ab53b416e
📒 Files selected for processing (5)
router-tests/events/nats_events_test.gorouter-tests/security/tls_test.gorouter-tests/testenv/testenv.gorouter/core/router.gorouter/core/supervisor_instance.go
🚧 Files skipped from review as they are similar to previous changes (1)
- router/core/supervisor_instance.go
dkorittki
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
router-tests/testenv/testenv.go (1)
720-749: ⚡ Quick winExtract the router TLS client bootstrap into one helper.
These two branches are meant to stay in lockstep, but they have already drifted (
require.NoErrorinCreateTestSupervisorEnvversuslog.FatalinCreateTestEnv). Pulling the cert/key/CA loading and retryable-client wiring into a shared helper will keep both test envs aligned through future TLS refactors.Also applies to: 1147-1179
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router-tests/testenv/testenv.go` around lines 720 - 749, Extract the duplicated TLS client bootstrap into a single helper, e.g. newTLSHTTPClientFromConfig(cfg *Config) (*http.Client, error), that performs tls.LoadX509KeyPair, os.ReadFile, builds the x509.CertPool, creates cleanhttp.DefaultPooledClient(), sets TLSClientConfig with RootCAs and Certificates, and wraps it with retryablehttp (respecting cfg.NoRetryClient and Retry settings) returning the final *http.Client or an error; then replace the duplicated blocks in CreateTestEnv and CreateTestSupervisorEnv with a call to this helper and handle the returned error consistently (either return it up the stack or assert/require in the same manner in both functions) so both branches remain in lockstep.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router/core/router.go`:
- Around line 159-165: The struct HTTPServerTLSConfig exposes a derived
*tls.Config as Config which should be unexported to prevent callers from
constructing/mutating inconsistent state; change the exported field to an
unexported config *tls.Config (or remove it from the public struct entirely) and
update internal consumers (Start, NewServer and any functions that currently
read HTTPServerTLSConfig.Config) to access the package-internal compiled config
instead or use a package-level accessor that returns a safe copy; ensure any
public constructors/validators accept Settings and produce the internal compiled
config so callers never directly hold the mutable *tls.Config.
---
Nitpick comments:
In `@router-tests/testenv/testenv.go`:
- Around line 720-749: Extract the duplicated TLS client bootstrap into a single
helper, e.g. newTLSHTTPClientFromConfig(cfg *Config) (*http.Client, error), that
performs tls.LoadX509KeyPair, os.ReadFile, builds the x509.CertPool, creates
cleanhttp.DefaultPooledClient(), sets TLSClientConfig with RootCAs and
Certificates, and wraps it with retryablehttp (respecting cfg.NoRetryClient and
Retry settings) returning the final *http.Client or an error; then replace the
duplicated blocks in CreateTestEnv and CreateTestSupervisorEnv with a call to
this helper and handle the returned error consistently (either return it up the
stack or assert/require in the same manner in both functions) so both branches
remain in lockstep.
🪄 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: 6a2bf225-68f2-4573-9985-59f78ea9db1b
📒 Files selected for processing (5)
router-tests/security/subgraph_mtls_test.gorouter-tests/testenv/testenv.gorouter/core/router.gorouter/core/router_config.gorouter/core/supervisor_instance.go
🚧 Files skipped from review as they are similar to previous changes (2)
- router/core/supervisor_instance.go
- router/core/router_config.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
router-tests/security/tls_test.go (1)
189-203: ⚡ Quick winExtract an mTLS test config helper to remove repeated nested literals.
These blocks repeat the same nested TLS structure with tiny differences. A helper (e.g.,
newServerMTLSConfig(required bool, certFile string)) would reduce noise and make future TLS shape changes safer in tests.Also applies to: 224-237, 252-263, 278-290, 349-361, 400-412
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router-tests/security/tls_test.go` around lines 189 - 203, Extract a test helper that builds the nested TLS config to avoid repeating the long literal: add a function like newServerMTLSConfig(required bool, certFile string) that returns *core.TlsConfig (or the nested Server->HTTP->Settings structure) and internally constructs core.TlsConfig -> core.ServerTLSConfig -> core.HTTPServerTLSConfig -> core.HTTPServerTLSConfigSettings with ClientAuth set to &core.HTTPServerMTLSConfigSettings{Required: required, CertFile: certFile} and common CertFile/KeyFile defaults; then replace the repeated literal blocks passed to testenv.Run (the TLSConfig argument) with calls to newServerMTLSConfig(true, "../testdata/tls/cert.pem") or newServerMTLSConfig(false, "...") as appropriate for the occurrences around the test cases (the blocks creating core.TlsConfig / core.ServerTLSConfig / core.HTTPServerTLSConfig / core.HTTPServerTLSConfigSettings / core.HTTPServerMTLSConfigSettings).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router-tests/security/tls_test.go`:
- Around line 23-33: The shared variable testdataCertsServerTLSConfig is being
reused by parallel subtests causing concurrent mutation of
r.tls.Server.HTTP.Config; add a factory function
newTestdataCertsServerTLSConfig() that returns a fresh *core.TlsConfig for each
test (allocate/copy the testdataCertsServerTLSConfig data into a new instance)
and replace all usages of &testdataCertsServerTLSConfig with calls to
newTestdataCertsServerTLSConfig(); ensure the router initialization that mutates
r.tls.Server.HTTP.Config (via r.serverHTTPTLSConfig()) operates on the new
instance so each subtest gets an independent TLS config.
---
Nitpick comments:
In `@router-tests/security/tls_test.go`:
- Around line 189-203: Extract a test helper that builds the nested TLS config
to avoid repeating the long literal: add a function like
newServerMTLSConfig(required bool, certFile string) that returns *core.TlsConfig
(or the nested Server->HTTP->Settings structure) and internally constructs
core.TlsConfig -> core.ServerTLSConfig -> core.HTTPServerTLSConfig ->
core.HTTPServerTLSConfigSettings with ClientAuth set to
&core.HTTPServerMTLSConfigSettings{Required: required, CertFile: certFile} and
common CertFile/KeyFile defaults; then replace the repeated literal blocks
passed to testenv.Run (the TLSConfig argument) with calls to
newServerMTLSConfig(true, "../testdata/tls/cert.pem") or
newServerMTLSConfig(false, "...") as appropriate for the occurrences around the
test cases (the blocks creating core.TlsConfig / core.ServerTLSConfig /
core.HTTPServerTLSConfig / core.HTTPServerTLSConfigSettings /
core.HTTPServerMTLSConfigSettings).
🪄 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: 53f2fcbf-f067-41a9-b035-850fd0dde316
📒 Files selected for processing (2)
router-tests/security/subgraph_mtls_test.gorouter-tests/security/tls_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- router-tests/security/subgraph_mtls_test.go
dkorittki
requested review from
SkArchon,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
There are two places where TLS settings for the router live. One is user-facing in pkg/config. The other is used internally by the Router object. They hold nearly the same data. When the Router object is build it translates the user-facing config into the internal config. The internal type is now practically gone. The router uses the user-facing config type directly, which aligns nicely with other configs sections, where this is already the case. This allowed me to remove the translation code as well. fix: router-tests chore: fix router-tests
Noroth
approved these changes
May 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactors the routers TLS configuration structure.
What doesn't change
Motivation
Eventually I want to add gRPC subgraph TLS config options (#2861) and without this refactor that config would need to live alongside other TLS configs on the router:
Thats already four different fields for something TLS related. Can you tell which one is used for what?
It's not very clear. The router has serverside and clientside TLS configs and it should be unmistakingly clear from the first look which field represents what.
The Changes
I got rid of the
core.TlsConfigtype. It's basically the same asTLSConfigurationfrom the user-facing config packagepkg/config. TheRouternow directly uses that type. During bootstrap we now directly hand over the user-facing tls settings to the router viaWithTLSConfig(cfg config.TLSConfiguration), instead of doing boilerplate mapping from one config type to another. I also removed unneccessary pointers.ℹ️ One note on https://github.com/wundergraph/cosmo/pull/2863/changes#diff-0b1a65770e1b2a3a8d841725fdbae4e436d118088b796cb819fe9be0bf6a9ad7R261:
This is a behaviour change because it was a bug. Previously this was always true. Now it's only true when subgraphs TLS is configured. It affects Posthog telemetry only. I hope this is okay. If not I'll have to revert to original behaviour.
Summary by CodeRabbit
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.