Bug: BMCSettings POST-based settings can never be verified via GET — permanent diff loop

[atd9876]

Which component is affected?

BMCSettings controller (internal/controller/bmcsettings_controller.go) and the OEM helpers verification logic (bmc/oem_helpers.go — httpBasedGetBMCSettingAttribute()).

Describe the bug

When a BMCSettings resource contains a key with the POST HTTP method prefix (e.g., "POST /redfish/v1/Managers/1/SecurityService/HttpsCert/Certificates/"), the controller can never verify that the setting was applied successfully. After applying the POST, verification does a GET on the same URI and compares the response to the original POST body using isSubMap(). Per the Redfish specification (DSP0266 v1.20.1 §7.9), POST creates a new collection member with a service-assigned URI, and the response structure is fundamentally different from the request body. This causes a permanent diff, and the controller re-applies the POST on every reconcile cycle (~2 minutes), creating duplicate resources on the BMC indefinitely.

To reproduce

1. Apply a BMCSettingsSet targeting an HPE iLO BMC with a POST-based key
2. Wait for the BMCSettings child to enter InProgress and apply settings
3. Observe that the BMCSettings never transitions to Applied — it stays InProgress forever
4. Check the BMC's certificate collection — a new duplicate certificate is created every ~2 minutes

Resource definitions used

apiVersion: metal.ironcore.dev/v1alpha1
kind: BMCSettingsSet
metadata:
  name: ad-ldap-hpe-ilo
spec:
  clusterScope: true
  bmcSelector:
    matchLabels:
      manufacturer: hpe
  template:
    version: "1.68"
    serverMaintenancePolicy: Enforced
    settingsMap:
      # PATCH key — works correctly (idempotent)
      "PATCH /redfish/v1/AccountService": |
        {
          "ActiveDirectory": {
            "ServiceEnabled": true,
            "ServiceAddresses": ["ldaps.example.com"]
          }
        }
      # POST key — triggers infinite loop
      "POST /redfish/v1/Managers/1/SecurityService/HttpsCert/Certificates/": |
        {
          "CertificateType": "PEM",
          "CertificateString": "-----BEGIN CERTIFICATE-----\nMIID...snip...\n-----END CERTIFICATE-----"
        }

Expected behavior

After successfully POSTing the certificate (HTTP 201 Created), the controller should consider the POST key as applied and not attempt to re-POST on subsequent reconciles. The BMCSettings should transition to Applied state.

Actual behavior

The controller enters an infinite loop:

1. POST certificate → succeeds (201)
2. Verification: GET /redfish/v1/Managers/1/SecurityService/HttpsCert/Certificates/ returns a collection of member links ({"Members": [{"@odata.id": "/redfish/v1/.../1"}]}) — structurally different from the POST body
3. isSubMap(specValue, getResponse) returns false → diff detected
4. Next reconcile: re-enters apply branch, re-POSTs the certificate
5. A new duplicate certificate resource is created on the BMC each cycle (or overwrites the existing one depending on vendor behavior)

Over time this either exhausts BMC resources (if certificates accumulate) or causes repeated HTTPS service restarts (if the certificate is replaced), both of which can destabilize the management controller.

Controller logs

# Every ~2 minutes, settings are re-applied:
{"level":"debug","msg":"BMC settings issued successfully","SettingKeys":["PATCH /redfish/v1/AccountService","POST /redfish/v1/Managers/1/SecurityService/HttpsCert/Certificates/"]}

# Verification always finds a diff:
{"level":"debug","msg":"Current BMC settings fetched","SettingKeys":["PATCH /redfish/v1/AccountService","POST /redfish/v1/Managers/1/SecurityService/HttpsCert/Certificates/"]}

# BMCSettings never reaches Applied — stays InProgress indefinitely

Environment

• metal-operator version/commit: main (pre-PR-812)
• Kubernetes version: v1.30
• Deployment method: Helm
• Hardware / BMC type: HPE iLO 6 firmware v1.68

Additional context

The root cause is in bmc/oem_helpers.go httpBasedGetBMCSettingAttribute() (~line 140). For ALL keys (including POST), it does:

resp, err := c.Get(parts[1])  // GETs the URI from the key
// ... then compares with isSubMap(specValue, getResponse)

Per the Redfish standard:

• POST is not idempotent — it creates a new resource each time
• The POST body (e.g., a PEM certificate string) is decomposed by the BMC into structured fields (Subject, Issuer, SerialNumber, etc.)
• GET on the collection returns member links, not the original POST body
• Password/key fields return null on GET (security requirement §13.2)

The correct verification for POST keys is the HTTP response code at apply time (201 Created = success). No GET-based comparison will ever match.

This issue interacts with the missing ConditionBMCResetPostSettingApply for HPE (Issue #857) — together they cause the controller to re-enter the apply branch every reconcile and re-POST indefinitely.

[coderabbitai]

🔗 Related PRs

#581 - Transition Server to Maintenance in case of pending BIOS settings exist [merged]
#723 - Simplify BMCSettings reconciler and fix bugs [merged]
#739 - Improve Redfish protocol and TLS certificate verification handling [merged]
#754 - Fix BMC event subscription errors on production hardware [merged]
#849 - Improved handling of BMC attribute values by consulting registry type definitions [merged]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

• Create Plan

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[atd9876]

Proposal: ETag-Based Settings Verification

ETag-Based Drift Detection

After a successful apply, capture the resource's ETag. On verification, GET the resource and compare its current ETag to the stored one. If they match, nothing has changed (including write-only fields). If they differ, something drifted — re-apply.

Why ETags

• ETags change whenever any field on the resource changes, including write-only ones
• ETags are the ONLY spec-compliant way to detect drift on non-readable state
• No special-casing needed for write-only fields or POST-created resources — ETag covers all fields uniformly
• Already used in two existing code paths for If-Match concurrency control:
  • Dell BMC settings (redfish_dell.go): GET → extract ETag → PatchWithHeaders with If-Match
  • gofish BIOS settings (UpdateBiosAttributesApplyAt): GET → extract ETag → PatchWithHeaders with If-Match
• Recommended by the Redfish specification (DMTF DSP0266 §6.5) for PATCH/PUT operations to prevent lost updates

Validation Against Real Hardware

Scanned mock BMC fixtures captured from production hardware:

Vendor	Fixture	Total Resources	With @odata.etag	Coverage
HPE iLO 6 v1.68	ProLiant DL380 Gen11	1,084	1,072	99%
Lenovo XCC 4.12	ThinkSystem SR850P	1,109	1,106	99.7%
Dell iDRAC 7.20	PowerEdge R660	2,889	64*	100% of settings resources

*Dell uses individual .json files vs index.json; all settings-relevant resources have ETags.

Key resources confirmed to have ETags on all three vendors:

• AccountService ✓
• AccountService/Accounts/{id} ✓
• AccountService/ExternalAccountProviders/LDAP/Certificates/{id} ✓

Resources without ETags (12 on HPE, 3 on Lenovo): Only non-settings resources — NetworkAdapter port metrics, hpsut utilities, session objects, schema registries. None are targets of BMCSettings operations.

---

Design

Two Distinct Uses of ETags

This proposal uses ETags for two complementary but distinct purposes:

1. Concurrency control on apply (If-Match header) — Before a PATCH, GET the resource to retrieve its current ETag, then include it as If-Match in the PATCH request. If another client modified the resource between the GET and PATCH, the BMC returns HTTP 412 Precondition Failed instead of silently overwriting. This is a defensive best-practice already implemented in the Dell and gofish BIOS paths.

2. Drift detection on verify (stored ETag comparison) — After a successful apply, store the response ETag. On subsequent reconciles, GET the resource and compare its current ETag to the stored one. If they match, nothing has changed. If they differ, something drifted externally — re-apply. This is the core innovation of this proposal.

Prerequisites: gofish Library Support

The gofish schemas.Client interface (v0.21.6) already exposes all required methods:

type Client interface {
    Get(url string) (*http.Response, error)
    GetWithHeaders(url string, customHeaders map[string]string) (*http.Response, error)
    Patch(url string, payload any) (*http.Response, error)
    PatchWithHeaders(url string, payload any, customHeaders map[string]string) (*http.Response, error)
    Post(url string, payload any) (*http.Response, error)
    PostWithHeaders(url string, payload any, customHeaders map[string]string) (*http.Response, error)
    // ...
}

No upstream gofish changes are required. The HPE/Lenovo path currently calls c.Patch() / c.Post() — switching to c.PatchWithHeaders() with an If-Match header is a drop-in change.

1. Annotation for ETag Storage

const AnnotationAppliedETags = "metal.ironcore.dev/applied-etags"

Value is a JSON object mapping each settings key to its apply result:

{
  "PATCH /redfish/v1/AccountService": {
    "uri": "/redfish/v1/AccountService",
    "etag": "W/\"20B77DA6\"",
    "valueHash": "sha256:a1b2c3d4..."
  },
  "POST /redfish/v1/AccountService/ExternalAccountProviders/LDAP/Certificates/": {
    "uri": "/redfish/v1/AccountService/ExternalAccountProviders/LDAP/Certificates/1",
    "etag": "W/\"3AA79E2E\"",
    "valueHash": "sha256:e5f6a7b8..."
  }
}

• For PATCH keys: uri = the request URI
• For POST keys: uri = the created resource's URI from the Location response header
• etag = the ETag response header (or @odata.etag from a follow-up GET)
• valueHash = SHA-256 of the effective (resolved) value that was applied

Why annotations over status?

• Annotations survive Cluster API migrations (status does not)
• Consistent with existing annotation patterns (metal.ironcore.dev/operation, etc.)
• Size: ~100 bytes per key, well within the 256KB annotation limit

2. Modified Apply Function

Current signature:

func httpBasedUpdateBMCAttributes(c schemas.Client, attrs schemas.SettingsAttributes, applyTime schemas.SettingsApplyTime) error

Proposed signature:

// ApplyResult holds the outcome of a single settings apply operation.
type ApplyResult struct {
    URI  string // For POST: Location header (created resource); for PATCH: request URI
    ETag string // ETag from response header or follow-up GET
}

func httpBasedUpdateBMCAttributes(c schemas.Client, attrs schemas.SettingsAttributes, applyTime schemas.SettingsApplyTime) (map[string]ApplyResult, error)

Implementation changes per operation:

POST:

resp, err := c.Post(url, valueMap)
// ... error handling ...
result[attr] = ApplyResult{
    URI:  resp.Header.Get("Location"),
    ETag: resp.Header.Get("ETag"),
}

PATCH (with If-Match concurrency control):

// GET current ETag for optimistic concurrency control
getResp, err := c.Get(url)
if err != nil {
    errs = append(errs, fmt.Errorf("failed to GET %s for ETag: %w", url, err))
    continue
}
var headers map[string]string
if etag := getResp.Header.Get("ETag"); etag != "" {
    headers = map[string]string{"If-Match": etag}
}
getResp.Body.Close()

// PATCH with If-Match to prevent lost updates
resp, err := c.PatchWithHeaders(url, valueMap, headers)
// ... error handling ...
result[attr] = ApplyResult{
    URI:  url,
    ETag: resp.Header.Get("ETag"),
}

This mirrors the existing patterns in redfish_dell.go and gofish's UpdateBiosAttributesApplyAt.

Fallback: If the response doesn't include an ETag header (unlikely per fixture analysis):

if result[attr].ETag == "" {
    getResp, _ := c.Get(result[attr].URI)
    result[attr].ETag = getResp.Header.Get("ETag")
}

3. Modified Verify Function

Replace isSubMap comparison with ETag comparison for all keys:

func httpBasedGetBMCSettingAttribute(c schemas.Client, attributes map[string]string, storedETags map[string]ApplyResult) (schemas.SettingsAttributes, error) {
    result := schemas.SettingsAttributes{}
    var errs []error

    for key, data := range attributes {
        parts := strings.Fields(key)
        if len(parts) != 2 {
            errs = append(errs, fmt.Errorf("invalid attribute format: %s", key))
            continue
        }

        stored, hasStoredETag := storedETags[key]
        if !hasStoredETag {
            // Never applied — report as diff (use raw spec value)
            result[key] = data
            continue
        }

        // GET the resource (use stored URI for POST keys, request URI for PATCH)
        targetURI := stored.URI
        resp, err := c.Get(targetURI)
        if err != nil {
            errs = append(errs, fmt.Errorf("failed to GET %s: %w", targetURI, err))
            continue
        }
        defer resp.Body.Close()

        currentETag := resp.Header.Get("ETag")
        if currentETag == "" {
            // Try @odata.etag from body
            var body map[string]any
            json.NewDecoder(resp.Body).Decode(&body)
            if etag, ok := body["@odata.etag"].(string); ok {
                currentETag = etag
            }
        }

        if currentETag == stored.ETag {
            // No drift — report spec value (matches)
            result[key] = data
        } else {
            // Drift detected — report current state (differs from spec)
            result[key] = currentETag // signals diff to caller
        }
    }
    return result, errors.Join(errs...)
}

4. Detecting Desired-State Changes

ETags detect BMC-side drift, but we also need to detect when the desired state changes (spec edit, ConfigMap certificate rotation, Secret password rotation). metadata.generation only increments on spec edits — it does not change when a referenced ConfigMap or Secret is updated.

Solution: store a valueHash (SHA-256 of the effective resolved value) per key alongside the ETag. On each reconcile:

1. Resolve variables from Secrets/ConfigMaps → compute effective value per key
2. Compare hash(currentEffectiveValue) to storedValueHash
3. If hashes differ → desired state changed → re-apply (regardless of ETag)
4. If hashes match → check ETag for BMC-side drift

The controller already watches ConfigMaps and Secrets (enqueueBMCSettingsByConfigMap, enqueueBMCSettingsBySecret) and re-resolves variables every reconcile, so the valueHash comparison naturally catches all sources of change.

5. Controller Integration

In updateSettingsAndVerify(), after successful apply:

// Apply settings
applyResults, err := bmcClient.SetBMCAttributesImmediately(ctx, bmcObj.Spec.BMCUUID, settingsDiff)
if err != nil {
    return ctrl.Result{}, fmt.Errorf("failed to set BMC settings: %w", err)
}

// Store ETags in annotation
if err := r.storeAppliedETags(ctx, settings, applyResults); err != nil {
    return ctrl.Result{}, fmt.Errorf("failed to store applied ETags: %w", err)
}

In getBMCSettingsDifference(), pass stored ETags to the verify function:

storedETags := r.loadAppliedETags(settings)
currentSettings, err := bmcClient.GetBMCAttributeValues(ctx, bmcObj.Spec.BMCUUID, effectiveSettingsMap, storedETags)

6. Interface Changes

The BMC interface method needs updating:

// Current
GetBMCAttributeValues(ctx context.Context, UUID string, attributes map[string]string) (schemas.SettingsAttributes, error)
SetBMCAttributesImmediately(ctx context.Context, UUID string, attributes schemas.SettingsAttributes) error

// Proposed
GetBMCAttributeValues(ctx context.Context, UUID string, attributes map[string]string, storedETags map[string]ApplyResult) (schemas.SettingsAttributes, error)
SetBMCAttributesImmediately(ctx context.Context, UUID string, attributes schemas.SettingsAttributes) (map[string]ApplyResult, error)

---

What This Fixes

Scenario	Before	After
POST certificate	isSubMap always fails → infinite re-POST	ETag matches → no diff
PATCH with password field	GET returns null → always differs	ETag matches → no diff
Actual external drift (admin changes setting)	isSubMap may miss structured changes	ETag differs → re-apply
Concurrent modification	No detection	If-Match on apply prevents lost updates

---

Scope

In Scope

• bmc/oem_helpers.go: httpBasedUpdateBMCAttributes, httpBasedGetBMCSettingAttribute
• bmc/redfish_hpe.go: GetBMCAttributeValues, SetBMCAttributesImmediately
• bmc/redfish_lenovo.go: GetBMCAttributeValues, SetBMCAttributesImmediately
• bmc/redfish_dell.go: SetBMCAttributesImmediately (add POST support + ETag capture)
• bmc/bmc.go: Interface definition
• internal/controller/bmcsettings_controller.go: Annotation helpers, getBMCSettingsDifference wiring

Dell-Specific Notes

Dell currently uses registry-based PATCH-only attribute comparison. POST-based settings (e.g. certificate upload) need to be added for Dell. Dell already uses ETags for If-Match on PATCH — extending to capture and store ETags after POST/PATCH responses aligns with the existing Dell pattern. Dell's GetBMCAttributeValues will need an ETag-based verification path for POST keys alongside the existing registry-based comparison for PATCH keys.

Retained

• isSubMap() function: Can remain as utility or be removed — not used after this change for HPE/Lenovo verification.

---

Implementation Order

Step	Description	Prerequisite
1	Add ApplyResult type in bmc/oem_helpers.go	None
2	Change httpBasedUpdateBMCAttributes to return map[string]ApplyResult	Step 1
3	Add If-Match concurrency control to PATCH path (GET → PatchWithHeaders)	Step 2
4	Capture ETag + Location from POST/PATCH responses	Step 3
5	Add annotation helpers (storeAppliedETags, loadAppliedETags)	Step 1
6	Replace httpBasedGetBMCSettingAttribute with ETag comparison	Steps 4, 5
7	Update BMC interface and all implementations	Steps 2, 6
8	Wire into controller: store ETags after apply, load before verify	Steps 5, 7

---

Open Questions

1. What if ETag is missing from both response header AND body? Per fixture analysis this shouldn't happen for settings resources. Defensive fallback: do a GET immediately after apply to capture the ETag. If GET also has no ETag (truly broken BMC): fall back to "trust HTTP 2xx = success" and skip verification for that key (log a warning).