Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion api/v1alpha1/server_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ type ServerSpec struct {
UUID string `json:"uuid,omitempty"`

// SystemUUID is the unique identifier for the server.
// +required
// If not provided, it will be derived from the serial
// +optional
SystemUUID string `json:"systemUUID"`

// SystemURI is the unique URI for the server resource in REDFISH API.
Expand Down
4 changes: 3 additions & 1 deletion bmc/redfish.go
Original file line number Diff line number Diff line change
Expand Up @@ -848,7 +848,9 @@ func (r *RedfishBMC) getSystemFromUri(ctx context.Context, systemURI string) (*s
}); err != nil {
return nil, fmt.Errorf("failed to wait for for server systems to be ready: %w", err)
}
if system.UUID != "" {
// System is considered ready even if UUID is empty - allow graceful handling of systems
// that don't expose System.UUID in their Redfish payload
if system != nil {
return system, nil
}
return nil, fmt.Errorf("no system found for %v", systemURI)
Expand Down
6 changes: 3 additions & 3 deletions config/crd/bases/metal.ironcore.dev_servers.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -292,15 +292,15 @@ spec:
REDFISH API.
type: string
systemUUID:
description: SystemUUID is the unique identifier for the server.
description: |-
SystemUUID is the unique identifier for the server.
If not provided, it will be derived from the serial
type: string
Comment on lines +295 to 298
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Minor: description text is incomplete.

The description says "derived from the serial" — should say "serial number" for clarity, and the sentence is missing a trailing period.

📝 Suggested fix
               systemUUID:
                 description: |-
                   SystemUUID is the unique identifier for the server.
-                  If not provided, it will be derived from the serial
+                  If not provided, it will be derived from the serial number.
                 type: string
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
description: |-
SystemUUID is the unique identifier for the server.
If not provided, it will be derived from the serial
type: string
description: |-
SystemUUID is the unique identifier for the server.
If not provided, it will be derived from the serial number.
type: string
🤖 Prompt for AI Agents
In `@config/crd/bases/metal.ironcore.dev_servers.yaml` around lines 302 - 305,
Update the description for the SystemUUID field so the sentence is complete and
clear: replace "derived from the serial" with "derived from the serial number."
Ensure the description for SystemUUID ends with a trailing period; locate the
SystemUUID field's description in the YAML (the block under "description" for
SystemUUID) and make this text change.

uuid:
description: |-
UUID is the unique identifier for the server.
Deprecated in favor of systemUUID.
type: string
required:
- systemUUID
type: object
status:
description: ServerStatus defines the observed state of Server.
Expand Down
6 changes: 3 additions & 3 deletions dist/chart/templates/crd/metal.ironcore.dev_servers.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -298,15 +298,15 @@ spec:
REDFISH API.
type: string
systemUUID:
description: SystemUUID is the unique identifier for the server.
description: |-
SystemUUID is the unique identifier for the server.
If not provided, it will be derived from the serial
type: string
uuid:
description: |-
UUID is the unique identifier for the server.
Deprecated in favor of systemUUID.
type: string
required:
- systemUUID
type: object
status:
description: ServerStatus defines the observed state of Server.
Expand Down
2 changes: 1 addition & 1 deletion docs/api-reference/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1557,7 +1557,7 @@ _Appears in:_
| Field | Description | Default | Validation |
| --- | --- | --- | --- |
| `uuid` _string_ | UUID is the unique identifier for the server.<br />Deprecated in favor of systemUUID. | | |
| `systemUUID` _string_ | SystemUUID is the unique identifier for the server. | | |
| `systemUUID` _string_ | SystemUUID is the unique identifier for the server.<br />If not provided, it will be derived from the serial | | |
| `systemURI` _string_ | SystemURI is the unique URI for the server resource in REDFISH API. | | |
Comment thread
coderabbitai[bot] marked this conversation as resolved.
| `power` _[Power](#power)_ | Power specifies the desired power state of the server. | | |
| `indicatorLED` _[IndicatorLED](#indicatorled)_ | IndicatorLED specifies the desired state of the server's indicator LED. | | |
Expand Down
84 changes: 75 additions & 9 deletions internal/controller/server_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package controller

import (
"context"
"crypto/md5"
"crypto/rand"
"crypto/rsa"
"encoding/json"
Expand Down Expand Up @@ -829,6 +830,7 @@ func (r *ServerReconciler) extractServerDetailsFromRegistry(ctx context.Context,
}

serverBase := server.DeepCopy()

// update network interfaces
nics := make([]metalv1alpha1.NetworkInterface, 0, len(serverDetails.NetworkInterfaces))
for _, s := range serverDetails.NetworkInterfaces {
Expand Down Expand Up @@ -905,6 +907,23 @@ func (r *ServerReconciler) patchServerState(ctx context.Context, server *metalv1
return true, nil
}

// generatePseudoUUID generates a deterministic UUID from an input string.
// Format: 99999999-xxxx-3xxx-8xxx-xxxxxxxxxxxx (prefix identifies generated UUIDs)
func generatePseudoUUID(input string) string {
hash := md5.Sum([]byte(input))
hashHex := fmt.Sprintf("%x", hash[:])

// Build UUID with version (3) and variant (8) bits embedded in groups
// Format: 99999999 - 4 - 3+3 - 8+3 - 12
uuid := fmt.Sprintf("99999999-%s-3%s-8%s-%s",
hashHex[0:4], // 4 hex chars
hashHex[4:7], // 3 hex chars (becomes 3XXX)
hashHex[7:10], // 3 hex chars (becomes 8XXX)
hashHex[10:22], // 12 hex chars
)
return uuid
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

func (r *ServerReconciler) patchServerURI(ctx context.Context, bmcClient bmc.BMC, server *metalv1alpha1.Server) (bool, error) {
log := ctrl.LoggerFrom(ctx)
if len(server.Spec.SystemURI) != 0 {
Expand All @@ -917,21 +936,68 @@ func (r *ServerReconciler) patchServerURI(ctx context.Context, bmcClient bmc.BMC
return false, err
}

for _, system := range systems {
if strings.EqualFold(system.UUID, server.Spec.SystemUUID) {
serverBase := server.DeepCopy()
server.Spec.SystemURI = system.URI
if err := r.Patch(ctx, server, client.MergeFrom(serverBase)); err != nil {
return false, fmt.Errorf("failed to patch server URI: %w", err)
// Try to find system by UUID if one is provided
uuidProvided := len(server.Spec.SystemUUID) > 0
uuidMatched := false
if uuidProvided {
for _, system := range systems {
if strings.EqualFold(system.UUID, server.Spec.SystemUUID) {
serverBase := server.DeepCopy()
server.Spec.SystemURI = system.URI
if err := r.Patch(ctx, server, client.MergeFrom(serverBase)); err != nil {
return false, fmt.Errorf("failed to patch server URI: %w", err)
}
return true, nil
}
}
// UUID was provided but didn't match any system
uuidMatched = false
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is already false here... why assign again?

}

// If no system found by UUID or UUID is empty, and we only have one system, use it
// This handles cases where the Redfish implementation doesn't provide System.UUID
if len(systems) == 1 {
// If a UUID was provided but didn't match the only available system, report an error
if uuidProvided && !uuidMatched {
system := systems[0]
log.V(1).Info("Provided SystemUUID does not match the only available system",
"requestedUUID", server.Spec.SystemUUID, "systemUUID", system.UUID, "systemSerialNumber", system.SerialNumber)
return false, fmt.Errorf("provided SystemUUID %q does not match the only available system (UUID: %q, SerialNumber: %q); check your configuration",
server.Spec.SystemUUID, system.UUID, system.SerialNumber)
}

system := systems[0]
serverBase := server.DeepCopy()
server.Spec.SystemURI = system.URI

// If SystemUUID is empty, use system UUID if available, otherwise generate from SerialNumber
if len(server.Spec.SystemUUID) == 0 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is always true?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it? if there is some uuid reported the len should be greater 0.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if UUID has something.. it either returns at line 950 or at line 965.
implying that the length here is always 0?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I think this logic can be optimized, may be give AI a go see what it comes up with?

if len(system.UUID) > 0 {
server.Spec.SystemUUID = system.UUID
} else if len(system.SerialNumber) > 0 {
server.Spec.SystemUUID = generatePseudoUUID(system.SerialNumber)
log.V(1).Info("Generated pseudo-UUID from system serial number",
"serialNumber", system.SerialNumber, "pseudoUUID", server.Spec.SystemUUID)
} else {
return false, fmt.Errorf("system does not provide UUID or SerialNumber; cannot generate a unique identifier")
}
}

if err := r.Patch(ctx, server, client.MergeFrom(serverBase)); err != nil {
Comment thread
xkonni marked this conversation as resolved.
return false, fmt.Errorf("failed to patch server URI: %w", err)
}
return true, nil
}
Comment on lines +939 to +990
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Silent fall-through when a provided SystemUUID doesn't match any system on a single-system BMC.

If server.Spec.SystemUUID is set but doesn't match the system's UUID (lines 924–935), the code falls through to the single-system branch (line 939). There, since SystemUUID is already non-empty, the derivation block on line 945 is skipped, and the server is silently bound to the unmatched system.

This could mask a misconfiguration (e.g., wrong UUID supplied). Consider either:

  • Logging a warning that the provided UUID didn't match the only available system, or
  • Returning an error when a UUID was explicitly provided but doesn't match.
🔧 Suggested: add a warning log
 	if len(systems) == 1 {
 		system := systems[0]
+		if len(server.Spec.SystemUUID) > 0 && !strings.EqualFold(system.UUID, server.Spec.SystemUUID) {
+			log.V(1).Info("Provided SystemUUID does not match the only available system, using it anyway",
+				"providedUUID", server.Spec.SystemUUID, "systemUUID", system.UUID)
+		}
 		serverBase := server.DeepCopy()
 		server.Spec.SystemURI = system.URI
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Try to find system by UUID if one is provided
if len(server.Spec.SystemUUID) > 0 {
for _, system := range systems {
if strings.EqualFold(system.UUID, server.Spec.SystemUUID) {
serverBase := server.DeepCopy()
server.Spec.SystemURI = system.URI
if err := r.Patch(ctx, server, client.MergeFrom(serverBase)); err != nil {
return false, fmt.Errorf("failed to patch server URI: %w", err)
}
return true, nil
}
}
}
if len(server.Spec.SystemURI) == 0 {
log.V(1).Info("Patching systemURI failed", "no system found for UUID", server.Spec.SystemUUID)
// If no system found by UUID or UUID is empty, and we only have one system, use it
// This handles cases where the Redfish implementation doesn't provide System.UUID
if len(systems) == 1 {
system := systems[0]
serverBase := server.DeepCopy()
server.Spec.SystemURI = system.URI
// If SystemUUID is empty, use system UUID if available, otherwise generate from SerialNumber
if len(server.Spec.SystemUUID) == 0 {
if len(system.UUID) > 0 {
server.Spec.SystemUUID = system.UUID
} else if len(system.SerialNumber) > 0 {
server.Spec.SystemUUID = generatePseudoUUID(system.SerialNumber)
log.V(1).Info("Generated pseudo-UUID from system serial number",
"serialNumber", system.SerialNumber, "pseudoUUID", server.Spec.SystemUUID)
} else {
return false, fmt.Errorf("system does not provide UUID or SerialNumber; cannot generate a unique identifier")
}
}
if err := r.Patch(ctx, server, client.MergeFrom(serverBase)); err != nil {
return false, fmt.Errorf("failed to patch server URI: %w", err)
}
return true, nil
}
// Try to find system by UUID if one is provided
if len(server.Spec.SystemUUID) > 0 {
for _, system := range systems {
if strings.EqualFold(system.UUID, server.Spec.SystemUUID) {
serverBase := server.DeepCopy()
server.Spec.SystemURI = system.URI
if err := r.Patch(ctx, server, client.MergeFrom(serverBase)); err != nil {
return false, fmt.Errorf("failed to patch server URI: %w", err)
}
return true, nil
}
}
}
// If no system found by UUID or UUID is empty, and we only have one system, use it
// This handles cases where the Redfish implementation doesn't provide System.UUID
if len(systems) == 1 {
system := systems[0]
if len(server.Spec.SystemUUID) > 0 && !strings.EqualFold(system.UUID, server.Spec.SystemUUID) {
log.V(1).Info("Provided SystemUUID does not match the only available system, using it anyway",
"providedUUID", server.Spec.SystemUUID, "systemUUID", system.UUID)
}
serverBase := server.DeepCopy()
server.Spec.SystemURI = system.URI
// If SystemUUID is empty, use system UUID if available, otherwise generate from SerialNumber
if len(server.Spec.SystemUUID) == 0 {
if len(system.UUID) > 0 {
server.Spec.SystemUUID = system.UUID
} else if len(system.SerialNumber) > 0 {
server.Spec.SystemUUID = generatePseudoUUID(system.SerialNumber)
log.V(1).Info("Generated pseudo-UUID from system serial number",
"serialNumber", system.SerialNumber, "pseudoUUID", server.Spec.SystemUUID)
} else {
return false, fmt.Errorf("system does not provide UUID or SerialNumber; cannot generate a unique identifier")
}
}
if err := r.Patch(ctx, server, client.MergeFrom(serverBase)); err != nil {
return false, fmt.Errorf("failed to patch server URI: %w", err)
}
return true, nil
}
🤖 Prompt for AI Agents
In `@internal/controller/server_controller.go` around lines 923 - 961, When a
non-empty server.Spec.SystemUUID is provided but the loop over systems finds no
match and len(systems)==1, log a warning before binding to that sole system so
misconfigurations aren't silent: detect whether the initial UUID scan matched
any system (use the existing loop over systems that checks strings.EqualFold on
system.UUID), and if it didn't and you're about to enter the single-system
branch, emit a warning via the controller logger (include provided
server.Spec.SystemUUID and the discovered system.UUID/URI), then continue with
setting server.Spec.SystemURI and deriving server.Spec.SystemUUID (using
generatePseudoUUID or system.UUID) and call r.Patch(serverBase) as before;
reference server.DeepCopy, r.Patch, generatePseudoUUID and
server.Spec.SystemUUID to locate the changes.

if len(server.Spec.SystemURI) == 0 {
log.V(1).Info("Patching systemURI failed", "no system found for UUID", server.Spec.SystemUUID)

// Multiple systems available but couldn't match by UUID
if len(server.Spec.SystemUUID) > 0 {
log.V(1).Info("No system found for UUID, and multiple systems available", "requestedUUID", server.Spec.SystemUUID)
return false, fmt.Errorf("unable to find system URI for UUID: %v", server.Spec.SystemUUID)
}

return true, nil
// No SystemUUID provided and multiple systems available - cannot determine which to use
log.V(1).Info("No SystemUUID provided and multiple systems available, cannot determine target system")
return false, fmt.Errorf("SystemUUID must be provided when multiple systems are available")
Comment thread
xkonni marked this conversation as resolved.
}

func (r *ServerReconciler) ensureServerPowerState(ctx context.Context, bmcClient bmc.BMC, server *metalv1alpha1.Server) error {
Expand Down
81 changes: 81 additions & 0 deletions internal/controller/server_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -937,3 +937,84 @@ func deleteRegistrySystemIfExists(systemUUID string) {
defer resp.Body.Close() //nolint:errcheck
}
}

var _ = Describe("generatePseudoUUID", func() {
It("Should generate UUIDs matching RFC 4122-like format (8-4-4-4-12)", func() {
testCases := []struct {
name string
serial string
}{
{"short_numeric", "123"},
{"short_alphanumeric", "ABC"},
{"medium_serial", "CZ2D1Y0BB3"},
{"long_serial", "LENOVO-SR850P-00012345-ABCDEF"},
{"uuid_like_serial", "550e8400-e29b-41d4-a716-446655440000"},
{"special_chars", "SN-2024_001+TEST"},
{"long_complex", "HPE-DL360-Gen10-Plus-SN123456789ABCDEFGHIJKLMNOP"},
{"numeric_heavy", "999999999999999999999999"},
{"mixed_case", "LenovoThinkSystem-SR850P-SN001"},
{"minimal", "X"},
}

for _, tc := range testCases {
uuid := generatePseudoUUID(tc.serial)

// Verify 8-4-4-4-12 hex format
Expect(uuid).To(MatchRegexp(`^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$`),
fmt.Sprintf("Invalid UUID format for %s: %s", tc.name, uuid))

// Verify 99999999 prefix
Expect(uuid).To(HavePrefix("99999999-"),
fmt.Sprintf("Missing 99999999 prefix for %s: %s", tc.name, uuid))

// Verify version 3 bit (3rd group)
Expect(uuid).To(MatchRegexp(`-3[0-9a-f]{3}-`),
fmt.Sprintf("Version 3 bit not set for %s: %s", tc.name, uuid))

// Verify variant 8 bit (4th group starts with 8, 9, a, or b)
Expect(uuid).To(MatchRegexp(`-[89ab][0-9a-f]{3}-`),
fmt.Sprintf("Variant bit not set for %s: %s", tc.name, uuid))
}
})

It("Should generate UUIDs that are deterministic", func() {
serial := "TEST-SERIAL-12345"
uuid1 := generatePseudoUUID(serial)
uuid2 := generatePseudoUUID(serial)

Expect(uuid1).To(Equal(uuid2),
"Same serial should always produce same UUID")
})

It("Should generate UUIDs that are unique for different inputs", func() {
serials := []string{"SN001", "SN002", "SN003", "SN004", "SN005"}
uuids := make(map[string]bool)

for _, serial := range serials {
uuid := generatePseudoUUID(serial)
Expect(uuids[uuid]).To(BeFalse(),
fmt.Sprintf("Duplicate UUID generated for serial %s: %s", serial, uuid))
uuids[uuid] = true
}

Expect(uuids).To(HaveLen(len(serials)))
})

It("Should generate UUIDs that handle serials of varying length and complexity", func() {
testCases := []string{
"X", // minimal
"SHORT", // short
"CZ2D1Y0BB3", // medium
"LENOVO-SR850P-00012345-ABCDEF-GHIJKL-MNOPQR", // long
"!@#$%^&*()-_=+[]{}|;:',.<>?/~`", // special
"混合テスト", // unicode
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", // very long
}

for _, serial := range testCases {
uuid := generatePseudoUUID(serial)
Expect(uuid).To(MatchRegexp(`^99999999-[0-9a-f]{4}-3[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$`),
fmt.Sprintf("Invalid UUID for complex input: %s", uuid))
}
})
})
Loading