Fix Dell OME authentication token handling - #136
Conversation
…ions kubebuilder edit overwrites dist/chart/templates/manager/manager.yaml on every run. Add patches to the helm target to re-apply the POD_NAMESPACE env fieldRef (required for telemetry/leader-election namespace resolution) and the ports values template after regeneration. Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
Instead of patching manager.yaml after kubebuilder regenerates it, set POD_NAMESPACE as a default in values.yaml so make helm produces a stable, committable result without any post-processing. Also remove the ports customization from values.yaml since container port declarations are informational only — the Service handles routing. Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
… header OME returns the session token in the X-Auth-Token response header, not the JSON body. Subsequent requests must also send the token via X-Auth-Token rather than Authorization: Bearer. Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
|
Warning Review limit reached
Next review available in: 47 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe hardware manager clients now use updated token handling, paginated inventory lookup, and revised discovery and removal requests. The vendor console controller now matches hostnames in more than one form and skips active-profile removal errors. ChangesHardware manager updates
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DellClient
participant HttpClient
participant OME
participant MockServer
DellClient->>HttpClient: authenticate session request
HttpClient->>OME: POST SessionService/Sessions
OME-->>HttpClient: 201 + X-Auth-Token
HttpClient->>DellClient: store token
DellClient->>OME: follow paginated device queries
OME-->>DellClient: Dell device pages
MockServer-->>HttpClient: X-Auth-Token for session tests
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Required to keep controller tests passing after the Dell OME auth fix. Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
LXCA does not issue session tokens — it requires HTTP Basic Auth on every request. Also fixes JoinPath calls that incorrectly URL-encoded query string separators, causing 404s on /nodes and /manageRequest endpoints. Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
xkonni
left a comment
xkonni
left a comment
There was a problem hiding this comment.
just one thing, lgtm otherwise.
| return "", fmt.Errorf("error executing auth request: %w", err) | ||
| } | ||
| defer res.Body.Close() //nolint:errcheck | ||
| if res.StatusCode != http.StatusCreated { |
There was a problem hiding this comment.
add _, _ = io.ReadAll(res.Body) after defer ...
The body must be read before the deferred close to drain the connection back to the pool — same pattern as DoRequest in httpclient.go
There was a problem hiding this comment.
ok, for the first reconcile or token refresh that is :)
- Dell OME: extract session token from X-Auth-Token response header;
use DELETE /Devices({id}) for removal; fix DeviceName field mapping;
add pagination for full device list; fix WSMAN/REDFISH discovery payload
- HPE OneView: use Hostname field for iLO IP; paginate ListServers;
use Name field as Hostname; skip removal of servers with active profiles
- Lenovo LXCA: switch to Basic Auth per-request (no session tokens);
fix JSON array wrapping for manage/unmanage requests; fix unmanage
payload structure (endpoints wrapper); fix FQDN field mapping;
fix JoinPath query string encoding
Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
LXCA sometimes returns only the short hostname in the FQDN field. Construct the full FQDN from hostname + domainName when needed, so ListServers returns FQDNs that match what the controller passes from bmc.Spec.Hostname. Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
Some Lenovo servers in LXCA have empty domainName and no FQDN — LXCA discovers these from the BMC and some BMCs are not configured with a domain. Index managedMap by both FQDN and short name so servers are correctly identified as managed regardless of what LXCA returns. Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
The session POST and subsequent API calls share the same httpClient transport pool (keyed by host:port). Draining the body before close returns the TCP connection to the pool, avoiding a new handshake on the first API call after authentication. Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
…ncile Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/hwmgr/dell.go (1)
278-302: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the paginated lookup in
ListServers.
ListServersmakes one request and returns only the first page.listAllServersis called only byRemoveServer. The controller usesListServers, so devices after the first Dell page are reported as absent and can be imported again.Proposed fix
func (c *DellClient) ListServers() ([]Device, error) { - serversURL := c.client.parsedURL.JoinPath("/api/DeviceService/Devices") - // one-page request and mapping + return c.listAllServers() }🤖 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 `@internal/hwmgr/dell.go` around lines 278 - 302, Update ListServers to use the existing listAllServers pagination helper instead of issuing a single Devices request and unmarshalling only its first page. Preserve the current Device mapping and error behavior while ensuring all Dell pages are returned to controller callers.internal/hwmgr/lenovo.go (1)
191-212: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSend the async discovery payload as an array.
ImportServerserializes the discovery request as[]ServerManageRequest, butImportServerAsyncserializes oneServerManageRequest. The controller usesImportServerAsync, so this path sends a payload that does not match the new array-based Lenovo discovery contract.Proposed fix
- payloadBytes, err := json.Marshal(discoveryPayload) + payloadBytes, err := json.Marshal([]ServerManageRequest{discoveryPayload})🤖 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 `@internal/hwmgr/lenovo.go` around lines 191 - 212, Update ImportServerAsync to serialize the discovery payload as an array of ServerManageRequest, matching the payload shape used by ImportServer and the Lenovo discovery contract; preserve the existing request creation and error handling.
🧹 Nitpick comments (1)
internal/controller/vendorconsole/console_controller.go (1)
321-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required Kubernetes logging style.
Initialize a local context logger once in
deleteand use it here. Change the message to past tense and name the object type, for example:Controller skipped server removal because the server had an active profile. Keep theserverkey-value pair.[details]
Suggested logging change
+ log := log.FromContext(ctx) ... - log.FromContext(ctx).Info("Skipping server removal: has active profile", "server", server.Name) + log.Info("Controller skipped server removal because the server had an active profile", "server", server.Name)Update the other
log.FromContext(ctx)calls indeleteto use the locallogvariable.As per coding guidelines, Kubernetes logging messages must start with a capital letter, do not end with a period, use active voice and past tense, name the object type, and keep key-value pairs balanced.
🤖 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 `@internal/controller/vendorconsole/console_controller.go` around lines 321 - 324, Update the delete method to initialize one local context logger and replace all log.FromContext(ctx) calls with it. In the ErrServerHasActiveProfile branch, use a capitalized, active, past-tense message naming the server object, preserve the server key-value pair, and keep the message without a trailing period.Source: Coding guidelines
🤖 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 `@internal/hwmgr/dell.go`:
- Around line 269-270: Update the pagination handling around page.NextLink to
resolve the next link against the current request URL using
req.URL.ResolveReference, rather than manually concatenating the configured
scheme and host. Validate the resolved URL against the configured Console
endpoint and reject it when its scheme or host differs before following the
link.
In `@internal/hwmgr/lenovo.go`:
- Around line 166-183: Introduce and reuse a single safe hostname resolver in
the Lenovo manager for both inventory device construction and job polling
matching. Update the logic around device creation to avoid generating a hostname
beginning with "." when HostName is empty, falling back to the node name as
appropriate, and update the polling comparison near the job-status handling to
resolve the hostname instead of using raw node.FQDN. Keep hostname matching
consistent across inventory and asynchronous imports.
- Around line 69-70: Update the Lenovo client initialization and GetAuthToken
flow to use Lenovo session-token authentication instead of forcing Basic Auth.
Ensure GetAuthToken validates an existing token, creates a new session token
when absent or invalid, and returns the resulting token for Secret persistence;
remove the unconditional empty-token return while preserving the client setup in
the surrounding constructor.
- Around line 69-70: Update NewClient after parsing Endpoint to validate that
the URL scheme is HTTPS before enabling console credentials or returning the
client; reject non-HTTPS endpoints, including HTTP, while preserving valid HTTPS
initialization.
---
Outside diff comments:
In `@internal/hwmgr/dell.go`:
- Around line 278-302: Update ListServers to use the existing listAllServers
pagination helper instead of issuing a single Devices request and unmarshalling
only its first page. Preserve the current Device mapping and error behavior
while ensuring all Dell pages are returned to controller callers.
In `@internal/hwmgr/lenovo.go`:
- Around line 191-212: Update ImportServerAsync to serialize the discovery
payload as an array of ServerManageRequest, matching the payload shape used by
ImportServer and the Lenovo discovery contract; preserve the existing request
creation and error handling.
---
Nitpick comments:
In `@internal/controller/vendorconsole/console_controller.go`:
- Around line 321-324: Update the delete method to initialize one local context
logger and replace all log.FromContext(ctx) calls with it. In the
ErrServerHasActiveProfile branch, use a capitalized, active, past-tense message
naming the server object, preserve the server key-value pair, and keep the
message without a trailing period.
🪄 Autofix
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 Plus
Run ID: c179d311-67ca-4e69-ad99-c69d6b8eb1c3
📒 Files selected for processing (6)
internal/controller/vendorconsole/console_controller.gointernal/hwmgr/dell.gointernal/hwmgr/hpe.gointernal/hwmgr/httpclient.gointernal/hwmgr/lenovo.gointernal/hwmgr/types.go
| if page.NextLink != "" { | ||
| nextURL = c.client.parsedURL.Scheme + "://" + c.client.parsedURL.Host + page.NextLink |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)dell\.go$|internal/hwmgr' | head -100
echo "== outline dell.go =="
ast-grep outline internal/hwmgr/dell.go --view expanded || true
echo "== relevant dell.go lines =="
sed -n '1,360p' internal/hwmgr/dell.go | cat -n
echo "== package imports/url usage =="
rg -n "url|DoRequest|NextLink|nextLink|RemoveServer|ImportServer|GetAuthToken" internal/hwmgr/dell.goRepository: ironcore-dev/metal-maintenance-operator
Length of output: 19198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== httpclient.go outline and relevant sections =="
ast-grep outline internal/hwmgr/httpclient.go --view expanded || true
sed -n '1,260p' internal/hwmgr/httpclient.go | cat -n
echo "== DoRequest callers and req.URL usage =="
rg -n "DoRequest\\(|req\\.URL|parsedURL" internal/hwmgr -S
echo "== URL construction probes =="
cat > /tmp/url_probe.go <<'GO'
package main
import (
"fmt"
"net/url"
)
func main() {
base := "https://192.168.0.10:1337/base/api/DeviceService/Devices"
baseURL, _ := url.Parse(base)
cases := []string{"?$skip=1", "/?$skip=1", "http://evil.example/next", "http://192.168.0.10:1337/base/api/DeviceService/Devices?$skip=1"}
for _, nextLink := range cases {
badPrefix := baseURL.Scheme + "://" + baseURL.Host + nextLink
nextRef, _ := url.Parse(nextLink)
resolved := baseURL.ResolveReference(nextRef).String()
offOrigin := false
if resolvedScheme, err := url.Parse(nextRef.Scheme + "://example.invalid" + nextRef.Path); err == nil {
offOrigin = resolvedScheme.Scheme == "http" && resolvedScheme.Host == "example.invalid"
}
fmt.Printf("nextLink=%s\nbadPrefix=%s\nresolved=%s\noffOrigin=%s\n", nextLink, badPrefix, resolved, nextRef.Host)
}
}
ГО
go run /tmp/url_probe.go
echo "== tests around Dell list/remove pagination =="
rg -n "listAllServers|RemoveServer|NextLink|`@odata`\\.(count|context)|page\\.NextLink" internal/hwmgr -SRepository: ironcore-dev/metal-maintenance-operator
Length of output: 9725
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Go package modules/imports =="
git ls-files | rg '(^go\.mod$|go\.sum$)' && cat go.mod | head -20
echo "== URL construction probes =="
python3 - <<'PY'
from urllib.parse import urljoin
base = "https://192.168.0.10:1337/base/api/DeviceService/Devices"
case_prefix = lambda s: "https://192.168.0.10:1337" + s
cases = {
"query-relative": "?$skip=1",
"path-relative": "/?$skip=1",
"absolute-origin": "http://evil.example/next",
"absolute-same-origin": "http://192.168.0.10:1337/base/api/DeviceService/Devices?$skip=1",
}
for name, next_link in cases.items():
print(f"{name}:")
print(f" current_prefix: {case_prefix(next_link)}")
print(f" urljoin(base, next_link): {urljoin(base, next_link)}")
PYRepository: ironcore-dev/metal-maintenance-operator
Length of output: 1590
Resolve @odata.nextLink against the current request URL.
Replace the scheme/host prefix at internal/hwmgr/dell.go:270 with req.URL.ResolveReference(page.NextLink), then reject resolved URLs whose scheme or host differs from the configured Console endpoint. This avoids invalid pagination targets for query-relative and path-relative next links and prevents following off-origin next links.
🤖 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 `@internal/hwmgr/dell.go` around lines 269 - 270, Update the pagination
handling around page.NextLink to resolve the next link against the current
request URL using req.URL.ResolveReference, rather than manually concatenating
the configured scheme and host. Validate the resolved URL against the configured
Console endpoint and reject it when its scheme or host differs before following
the link.
| c.client.basicAuth = true | ||
| return c, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Restore the required Lenovo session-token flow.
Line 69 forces Basic Auth for every request. Lines 186-188 always return an empty token. This client cannot validate or create the required Lenovo session token, and it cannot return a token for Secret persistence.
As per coding guidelines, “Dell and Lenovo use session tokens” and each vendor GetAuthToken() must validate or create a token when needed.
Also applies to: 186-188
🤖 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 `@internal/hwmgr/lenovo.go` around lines 69 - 70, Update the Lenovo client
initialization and GetAuthToken flow to use Lenovo session-token authentication
instead of forcing Basic Auth. Ensure GetAuthToken validates an existing token,
creates a new session token when absent or invalid, and returns the resulting
token for Secret persistence; remove the unconditional empty-token return while
preserving the client setup in the surrounding constructor.
Source: Coding guidelines
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline internal/hwmgr/httpclient.go --items all
rg -n -C 5 'func NewClient|parsedURL|\.Scheme|https|basicAuth|SetBasicAuth' internal/hwmgr/httpclient.goRepository: ironcore-dev/metal-maintenance-operator
Length of output: 2703
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '49,72p' internal/hwmgr/httpclient.go
rg -n -C 4 'NewClient\(\)|NewLenovoClient|&LenovoClient|Username:|Password:|Endpoint:' internal/hwmgr internal/controller internal -g '*.go'Repository: ironcore-dev/metal-maintenance-operator
Length of output: 13128
Sensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: Internal
Reachability path
● Entry
internal/hwmgr/dell.go:118
ImportServer
│
▼
● Sink
internal/hwmgr/lenovo.go
Reject non-HTTPS endpoints before using console credentials.
NewClient parses Endpoint but does not enforce https, while Lenovo Basic requests send c.username and c.password via SetBasicAuth. If a Console URL uses http, credentials are sent over an unencrypted connection. Add an HTTPS-only validation check after parsing the URL.
🤖 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 `@internal/hwmgr/lenovo.go` around lines 69 - 70, Update NewClient after
parsing Endpoint to validate that the URL scheme is HTTPS before enabling
console credentials or returning the client; reject non-HTTPS endpoints,
including HTTP, while preserving valid HTTPS initialization.
| fqdn := node.FQDN | ||
| if !strings.Contains(fqdn, ".") { | ||
| if node.DomainName != "" { | ||
| fqdn = node.HostName + "." + node.DomainName | ||
| } else { | ||
| fqdn = node.Name | ||
| } | ||
| } | ||
| device := Device{ | ||
| UUID: node.UUID, | ||
| Name: node.Name, | ||
| Hostname: node.HostName, | ||
| Hostname: fqdn, | ||
| Model: node.Type, | ||
| // HealthStatus mapping can be added based on HealthState | ||
| } | ||
| devices = append(devices, device) | ||
| } | ||
|
|
||
| return devices, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use one safe hostname resolver for inventory and job polling.
Lines 166-173 can produce .example.com when HostName is empty and DomainName is set. Line 263 bypasses this fallback logic and compares only raw FQDN. If LXCA returns empty or inconsistent FQDN data, inventory matching can use an invalid hostname and async imports can remain in running state.
Proposed fix
+func nodeHostname(node ServerNode) string {
+ fqdn := strings.TrimSpace(node.FQDN)
+ hostname := strings.TrimSpace(node.HostName)
+ domain := strings.Trim(strings.TrimSpace(node.DomainName), ".")
+
+ if strings.Contains(fqdn, ".") {
+ return fqdn
+ }
+ if hostname != "" && domain != "" {
+ return hostname + "." + domain
+ }
+ if fqdn != "" {
+ return fqdn
+ }
+ if hostname != "" {
+ return hostname
+ }
+ return node.Name
+}
+
- fqdn := node.FQDN
- if !strings.Contains(fqdn, ".") {
- if node.DomainName != "" {
- fqdn = node.HostName + "." + node.DomainName
- } else {
- fqdn = node.Name
- }
- }
+ fqdn := nodeHostname(node)
...
- if node.FQDN == jobID {
+ if nodeHostname(node) == jobID {Also applies to: 263-263
🤖 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 `@internal/hwmgr/lenovo.go` around lines 166 - 183, Introduce and reuse a
single safe hostname resolver in the Lenovo manager for both inventory device
construction and job polling matching. Update the logic around device creation
to avoid generating a hostname beginning with "." when HostName is empty,
falling back to the node name as appropriate, and update the polling comparison
near the job-status handling to resolve the hostname instead of using raw
node.FQDN. Keep hostname matching consistent across inventory and asynchronous
imports.
3 participants
OME returns the session token in the
X-Auth-Tokenresponse header — not the JSON body. Subsequent requests must also carry it viaX-Auth-Token, notAuthorization: Bearer.Signed-off-by: Stefan Hipfel stefan.hipfel@sap.com
Summary by CodeRabbit
New Features
Bug Fixes