Improve BMC connection reliability and error logging#758
Draft
Conversation
Use explicit HTTP/2 transport for Redfish connections instead of relying
on gofish defaults, and add structured logging on connection failures with
error type and URL error op fields to aid debugging.
fixes issues with one bmc
```
ERROR Reconciler error {"controller": "bmc", "controllerGroup": "metal.ironcore.dev", "controllerKind": "BMC", "BMC": {"name":"nodexx"}, "namespace": "", "name": "nodexx", "reconcileID": "", "error": "Get \"https://1.2.3.4:443/redfish/v1/\": net/http: HTTP/1.x transport connection broken: malformed HTTP status code \"bad\""}
```
Contributor
WalkthroughModified Redfish BMC client initialization to use explicit HTTP transport configuration with TLS insecure skip verification and HTTP/2 support. Updated ClientConfig to include explicit HTTPClient and NoModifyTransport flag. Added low-verbosity error logging for connection failures. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bmc/redfish.go (1)
89-94: Consider settingMinVersionin TLS configuration.The static analysis flagged that
MinVersionis not set. WhileInsecureSkipVerifyis intentionally used for BMC connections with self-signed certificates, setting a minimum TLS version improves security posture. TLS 1.2 is a reasonable minimum for BMC compatibility.🔒 Proposed fix
transport := &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, //nolint:gosec + MinVersion: tls.VersionTLS12, + }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/redfish.go` around lines 89 - 94, The TLS config for the HTTP transport lacks a minimum protocol version; update the TLSClientConfig used when creating transport in bmc (the transport variable/TLSClientConfig) to set MinVersion to tls.VersionTLS12 while keeping InsecureSkipVerify for self-signed BMC certs, so replace the current &tls.Config{InsecureSkipVerify: true} with &tls.Config{InsecureSkipVerify: true, MinVersion: tls.VersionTLS12}; leave the http2.ConfigureTransport(transport) call unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bmc/redfish.go`:
- Around line 89-94: The TLS config for the HTTP transport lacks a minimum
protocol version; update the TLSClientConfig used when creating transport in bmc
(the transport variable/TLSClientConfig) to set MinVersion to tls.VersionTLS12
while keeping InsecureSkipVerify for self-signed BMC certs, so replace the
current &tls.Config{InsecureSkipVerify: true} with
&tls.Config{InsecureSkipVerify: true, MinVersion: tls.VersionTLS12}; leave the
http2.ConfigureTransport(transport) call unchanged.
afritzler
requested changes
Mar 27, 2026
| // newRedfishBaseBMCClient creates a new RedfishBaseBMC with the given connection details (internal use only). | ||
| func newRedfishBaseBMCClient(ctx context.Context, options Options) (*RedfishBaseBMC, error) { | ||
| transport := &http.Transport{ | ||
| TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec |
Member
There was a problem hiding this comment.
I think we should make this configurable.
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
4 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.
Proposed Changes
Fixes #759
EDIT: opened an upstream PR instead: stmcginnis/gofish#518