MM-67050: Add TLS config support to client library#190
Conversation
📝 WalkthroughWalkthroughAdded support for supplying a caller-provided TLS configuration to the main client and propagated it to both the HTTP transport used by the API client and the WebSocket dialer via new Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/client.go (1)
141-141: Clone and modify the HTTP client rather than replacing it entirely.Line 141 replaces the entire
HTTPClientwith a new client. Whilst the current initialization bymodel.NewAPIv4Client()only creates a zero-value client, cloning the existing client and overriding onlyTransportis a more maintainable defensive pattern that protects against future configuration changes.Suggested fix
- c.apiClient.HTTPClient = &http.Client{Transport: transport} + httpClient := c.apiClient.HTTPClient + if httpClient == nil { + httpClient = &http.Client{} + } + clonedClient := *httpClient + clonedClient.Transport = transport + c.apiClient.HTTPClient = &clonedClient🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/client.go` at line 141, The code replaces c.apiClient.HTTPClient outright which loses any future configuration; instead clone the existing http.Client and set only the Transport: read the current c.apiClient.HTTPClient (created by model.NewAPIv4Client()), create a shallow copy of that client, assign the new Transport on the copy, and then set c.apiClient.HTTPClient to the copy so other client fields (timeouts, headers, etc.) are preserved; update the assignment that currently sets &http.Client{Transport: transport} to this clone-and-modify approach targeting c.apiClient.HTTPClient.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/client.go`:
- Line 138: The code replaces c.apiClient.HTTPClient with a brand new
http.Client and loses any prior settings; instead, preserve the existing client
and only swap its Transport: obtain the current client (c.apiClient.HTTPClient),
clone its Transport if present (e.g., existingTransport :=
c.apiClient.HTTPClient.Transport; if t, use t.(*http.Transport).Clone() else
fallback to http.DefaultTransport.(*http.Transport).Clone()), then create a
shallow copy of the existing client (newClient := *c.apiClient.HTTPClient), set
newClient.Transport = transport and assign c.apiClient.HTTPClient = &newClient
so timeouts, redirect policies and other fields remain intact while replacing
only the transport.
---
Nitpick comments:
In `@client/client.go`:
- Line 141: The code replaces c.apiClient.HTTPClient outright which loses any
future configuration; instead clone the existing http.Client and set only the
Transport: read the current c.apiClient.HTTPClient (created by
model.NewAPIv4Client()), create a shallow copy of that client, assign the new
Transport on the copy, and then set c.apiClient.HTTPClient to the copy so other
client fields (timeouts, headers, etc.) are preserved; update the assignment
that currently sets &http.Client{Transport: transport} to this clone-and-modify
approach targeting c.apiClient.HTTPClient.
🪄 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: 7ae81550-e09b-4096-89c1-28a9c4b18ae1
📒 Files selected for processing (4)
client/client.goclient/websocket.goservice/ws/client.goservice/ws/option.go
…port instead of localhost:8080
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/websocket_test.go`:
- Around line 123-127: The test currently ignores the return value of
ln.Close(), which can leave the port bound and make reconnect behavior flaky;
update the teardown to check and fail the test if Close() returns an error
(e.g., replace ln.Close() with an assertion like require.NoError(t, ln.Close())
or otherwise handle the error) before using unusedAddr to set
th.userClient.cfg.wsURL so the test reliably triggers ECONNREFUSED when
reconnecting.
🪄 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: a3858727-4c8b-4562-9f97-06be0a042b79
📒 Files selected for processing (1)
client/websocket_test.go
Summary
WithTLSConfigclient option for configuring custom TLS settings (self-signed / private CA certificates) on both the HTTP API client and WebSocket connectionhttp.DefaultTransportto preserve default timeouts and connection pooling, and force HTTP/2WithTLSConfigoption to the WebSocket client, applied to the gorilla/websocket dialerThis enables consumers (e.g. calls-transcriber) to connect to Mattermost servers using self-signed or private CA certificates.
This change does not affect RTCD server.
Ticket Link
https://mattermost.atlassian.net/browse/MM-67050