OCPEDGE-2115: types: support MAC address as alternative identifier for fencing credential#10513
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@fracappa: This pull request references OCPEDGE-2115 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an optional macAddress to baremetal fencing credentials across schema, types, validation, agent/machine logic and tests; requires at least one identifier (hostName or macAddress); when hostName is absent, secret name is derived from a normalized MAC hashed with SHA‑256 (first 8 bytes hex). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/asset/machines/master_test.go (1)
471-473:⚠️ Potential issue | 🟠 MajorAssert the actual generated error, not the expected error.
assert.Error(t, tc.err, master.Generate(...))checks whethertc.erris non-nil—not whethermaster.Generate()returns an error. The actual error is incorrectly passed as a message argument, allowing the test to pass even ifGenerate()returns a different error or no error at all.Fix
if tc.err != nil { - assert.Error(t, tc.err, master.Generate(context.Background(), parents)) + err := master.Generate(context.Background(), parents) + assert.EqualError(t, err, tc.err.Error()) assert.Len(t, master.FencingCredentialsSecretFiles, len(tc.fencingCredentialsSecretFiles))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/machines/master_test.go` around lines 471 - 473, The test is asserting the wrong value: it passes tc.err as the message to assert.Error instead of checking the error returned by master.Generate; change the call to capture the returned error (e.g., err := master.Generate(context.Background(), parents)), then assert against that err — use assert.Error(t, err) or assert.ErrorIs(t, err, tc.err) / assert.EqualError(t, err, tc.err.Error()) depending on whether you expect error identity or string equality; keep the subsequent assertion on master.FencingCredentialsSecretFiles (len check) after verifying err as appropriate.pkg/asset/agent/agentconfig/fencingcredentials_test.go (1)
107-124:⚠️ Potential issue | 🟡 MinorNit: typo in comment.
"MACAdress" → "MACAddress".
✏️ Proposed fix
- MACAddress: "", // Missing MACAdress + MACAddress: "", // Missing MACAddress🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/agent/agentconfig/fencingcredentials_test.go` around lines 107 - 124, Update the inline comment inside getOptionalInstallConfigWithMissingIdentifier to correct the typo "MACAdress" to "MACAddress" so the comment accurately reflects the MACAddress field name on the Credential struct; no code logic changes required—only fix the comment text next to the MACAddress field in the fencing credential literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/asset/agent/agentconfig/fencingcredentials_test.go`:
- Around line 339-346: The test case "validation error - invalid MAC address"
currently uses a weak expectation (expectedError: "macAddress"); update the
assertion to match the actual parse/validator message produced when
getOptionalInstallConfigWithInvalidMAC() is used so the invalid-format branch is
exercised (e.g., change expectedError to a more specific substring such as
"invalid MAC address" or the exact message returned by net.ParseMAC/your
validator), and adjust the test assertion to use substring matching (e.g.,
assert.Contains on the error string) against the error produced by the code
paths in the agent config validation logic.
- Around line 168-214: Fix the typo in the comments above the helper functions
getValidOptionalInstallConfigWithMACAddress and
getOptionalInstallConfigWithDuplicateMACs: replace "MAC addresess" with "MAC
addresses" in both comment blocks so the wording is correct and consistent.
- Around line 589-611: The test is brittle because it asserts exact-case MAC
strings and a loose "hostname:" substring; update the assertions in the
t.Run("with macAddress") block to be case-insensitive for MAC checks and to
match hostname as a full YAML key. Specifically, when examining yamlContent
produced by FencingCredentials.Generate()/Files(), compare using a
case-normalized string (e.g., lowercased yamlContent and lowercased MAC literal)
for the macAddress assertions, and replace the hostname negative check with a
boundary-aware check that ensures a newline or start-of-string precedes
"hostname:" (so it only matches the YAML key, not substrings like
"hostname_override").
In `@pkg/asset/agent/agentconfig/fencingcredentials.go`:
- Around line 178-188: The duplicate detection currently uses strings.ToLower on
cred.MACAddress which misses equivalent formats; after
validate.MAC(cred.MACAddress) succeeds, parse the address (e.g., net.ParseMAC or
equivalent) and derive a canonical form (use the parsed hardware address's
String() which yields lower-case colon-separated bytes) and use that canonical
value as the key into seenMACs instead of strings.ToLower; if parsing
unexpectedly fails, append a field.Invalid to credPath.Child("macAddress") and
skip duplicate logic. Ensure you update references to normalizedMAC (the key)
and leave the user-facing message using the original cred.MACAddress or include
the canonical form if desired.
In `@pkg/asset/machines/master.go`:
- Around line 752-760: The MAC hashing path in the credentials loop (where
credential.MACAddress is used to derive name via sha256.Sum256 and
hex.EncodeToString) only strips colons and therefore treats dash- or
dot-separated MACs as different values; update the logic to parse and validate
the MAC using net.ParseMAC (or equivalent) to produce a canonical byte form
(lowercase, no separators) before hashing, handle parse errors by
skipping/returning an error as appropriate, and then feed the canonical byte
slice into sha256.Sum256 to derive the hex-encoded name; ensure this change
touches the credential.MACAddress handling in the for i, credential := range
credentials loop and preserves the existing HostName-first switch branch.
In `@pkg/types/validation/installconfig.go`:
- Around line 1772-1776: The loop validating fencingCredentials.Credentials
currently only checks presence of MACAddress but not its format; update the
validation inside the for i, credential := range fencingCredentials.Credentials
loop to check that when credential.MACAddress is non-empty it is a valid MAC
(e.g., use net.ParseMAC or a strict regexp) and if invalid append a
field.Invalid to credPath.Child("macAddress") with a clear message; keep the
existing hostname check and ensure you reference fencingCredentials.Credentials,
credential.MACAddress, credPath and fldPath.Child("macAddress") and use
field.Invalid for the error.
---
Outside diff comments:
In `@pkg/asset/agent/agentconfig/fencingcredentials_test.go`:
- Around line 107-124: Update the inline comment inside
getOptionalInstallConfigWithMissingIdentifier to correct the typo "MACAdress" to
"MACAddress" so the comment accurately reflects the MACAddress field name on the
Credential struct; no code logic changes required—only fix the comment text next
to the MACAddress field in the fencing credential literal.
In `@pkg/asset/machines/master_test.go`:
- Around line 471-473: The test is asserting the wrong value: it passes tc.err
as the message to assert.Error instead of checking the error returned by
master.Generate; change the call to capture the returned error (e.g., err :=
master.Generate(context.Background(), parents)), then assert against that err —
use assert.Error(t, err) or assert.ErrorIs(t, err, tc.err) /
assert.EqualError(t, err, tc.err.Error()) depending on whether you expect error
identity or string equality; keep the subsequent assertion on
master.FencingCredentialsSecretFiles (len check) after verifying err as
appropriate.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 240ce38e-3922-44d0-9996-14f0846b0ac7
📒 Files selected for processing (9)
data/data/install.openshift.io_installconfigs.yamlpkg/asset/agent/agentconfig/fencingcredentials.gopkg/asset/agent/agentconfig/fencingcredentials_test.gopkg/asset/machines/master.gopkg/asset/machines/master_test.gopkg/types/common/common.gopkg/types/machinepools.gopkg/types/validation/installconfig.gopkg/types/validation/installconfig_test.go
fracappa
force-pushed
the
fca/mac-fencing-credentials
branch
2 times, most recently
from
a28ff05 to
b190d66
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/asset/agent/agentconfig/fencingcredentials_test.go (1)
339-345:⚠️ Potential issue | 🟡 MinorStrengthen the invalid MAC assertion.
expectedError: "macAddress"is too broad and can match unrelated field errors. Prefer the concrete invalid-format message emitted by the validator so this test proves the parse/format branch is exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/agent/agentconfig/fencingcredentials_test.go` around lines 339 - 345, The test's expectedError "macAddress" is too generic; update the test case (the struct using getOptionalInstallConfigWithInvalidMAC()) to assert the exact validator error message emitted for malformed MACs (replace expectedError with the concrete invalid-format string returned by your validator, e.g., the message produced by ValidateFencingCredentials/Validate on the macAddress field such as "macAddress: invalid MAC format" or the exact string you observe when running the validator) so the test verifies the parse/format branch rather than any field-name match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/asset/machines/master.go`:
- Around line 754-770: In the credentials loop fix three issues: remove the dead
commented line containing mac := strings.ReplaceAll(...) ; correct the typo
credential.MACAdress to credential.MACAddress; and replace the incorrect
strings.ToLower(parsedMAC) usage by capturing the net.ParseMAC error (parsedMAC,
err := net.ParseMAC(credential.MACAddress)) and returning the error if non-nil,
then call parsedMAC.String() to get the canonical MAC string, strip colons with
strings.ReplaceAll(parsedMAC.String(), ":", "") before hashing with
sha256.Sum256 and encoding via hex.EncodeToString(hash[:8]); keep
validate.MAC(credential.MACAddress) as the first check.
---
Duplicate comments:
In `@pkg/asset/agent/agentconfig/fencingcredentials_test.go`:
- Around line 339-345: The test's expectedError "macAddress" is too generic;
update the test case (the struct using getOptionalInstallConfigWithInvalidMAC())
to assert the exact validator error message emitted for malformed MACs (replace
expectedError with the concrete invalid-format string returned by your
validator, e.g., the message produced by ValidateFencingCredentials/Validate on
the macAddress field such as "macAddress: invalid MAC format" or the exact
string you observe when running the validator) so the test verifies the
parse/format branch rather than any field-name match.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 32989428-f2ff-4269-8e3c-797016cc666f
📒 Files selected for processing (10)
data/data/install.openshift.io_installconfigs.yamlpkg/asset/agent/agentconfig/fencingcredentials.gopkg/asset/agent/agentconfig/fencingcredentials_test.gopkg/asset/machines/master.gopkg/asset/machines/master_test.gopkg/asset/tls/iriregistryauth_test.gopkg/types/common/common.gopkg/types/machinepools.gopkg/types/validation/installconfig.gopkg/types/validation/installconfig_test.go
✅ Files skipped from review due to trivial changes (3)
- pkg/types/validation/installconfig.go
- pkg/types/common/common.go
- pkg/asset/tls/iriregistryauth_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/asset/machines/master_test.go
- pkg/asset/agent/agentconfig/fencingcredentials.go
- pkg/types/validation/installconfig_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/asset/machines/master.go (1)
754-770:⚠️ Potential issue | 🔴 CriticalThis block will not compile — typo and type mismatch still present.
Three blockers remain in the MAC branch:
- Line 764 — typo:
credential.MACAdress→ should becredential.MACAddress(the field defined inpkg/types/machinepools.goline 235 isMACAddress). This is a compile error.- Line 765 — type error:
net.ParseMACreturnsnet.HardwareAddr([]byte), notstring.strings.ToLower(parsedMAC)does not compile. UseparsedMAC.String()— which already returns canonical lowercase hex with:separators.- Line 760 — dead code: stale commented-out
mac := strings.ReplaceAll(...)line should be removed.Additionally, line 764 discards
net.ParseMAC's error via_. Even thoughvalidate.MACis called just above, capture and return the error defensively so the two calls cannot silently diverge.Proposed fix
case credential.MACAddress != "": - // mac := strings.ReplaceAll(strings.ToLower(credential.MACAddress), ":", "") if err := validate.MAC(credential.MACAddress); err != nil { return nil, fmt.Errorf("credential at index %d has invalid MAC address: %w", i, err) } - parsedMAC, _ := net.ParseMAC(credential.MACAdress) - mac := strings.ReplaceAll(strings.ToLower(parsedMAC), ":", "") + parsedMAC, err := net.ParseMAC(credential.MACAddress) + if err != nil { + return nil, fmt.Errorf("credential at index %d has invalid MAC address: %w", i, err) + } + mac := strings.ReplaceAll(parsedMAC.String(), ":", "") hash := sha256.Sum256([]byte(mac)) name = hex.EncodeToString(hash[:8])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/machines/master.go` around lines 754 - 770, In the credentials loop in master.go fix the MAC branch: remove the stale commented-out mac line, correct the typo credential.MACAdress → credential.MACAddress, call parsedMAC, err := net.ParseMAC(credential.MACAddress) and return the error if non-nil (don’t discard it), then convert parsedMAC to a string with parsedMAC.String(), normalize it by removing ":" and lowercasing (e.g., strings.ReplaceAll(parsedMAC.String(), ":", "") and strings.ToLower(...)), then feed that normalized mac into sha256.Sum256 and hex.EncodeToString for name; keep the existing validate.MAC call but handle ParseMAC defensively to avoid silent divergence.
🧹 Nitpick comments (1)
pkg/types/machinepools.go (1)
234-235: YAML tag casing is inconsistent betweenHostNameandMACAddress.
HostNameusesyaml:"hostname,omitempty"(all lowercase, pre-existing), while the newMACAddressusesyaml:"macAddress,omitempty"(camelCase). Downstream YAML consumers will need to use different casing conventions for neighboring fields of the same struct. Sincehostnameis pre-existing and likely part of a public surface, the safer fix is to alignMACAddresstoyaml:"macaddress,omitempty"to match the existing convention — or document the intentional divergence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/machinepools.go` around lines 234 - 235, The YAML tag for MACAddress is using camelCase while HostName uses all-lowercase, causing inconsistent YAML field names; update the struct tag on MACAddress (the field named MACAddress in the same struct as HostName) to use yaml:"macaddress,omitempty" (all lowercase) to match the existing hostname convention while leaving the json and validate tags unchanged so downstream YAML consumers see consistent casing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/types/machinepools.go`:
- Around line 234-238: The MAC case-sensitivity bug: replace the generic
uniqueField check on the MACAddress field with a case-insensitive uniqueness
validator or normalize MACs during decode so all codepaths see the same
canonical form. Concretely, add a new validator (e.g., uniqueMAC) in
pkg/types/common/common.go that lowercases (and optionally canonicalizes) the
MAC string before using it as the map key, register that validator with the same
validation registry used by ValidateUniqueAndRequiredFields, and change the
struct tag on MACAddress in pkg/types/machinepools.go from
validate:"uniqueField" to validate:"uniqueMAC"; ensure this new validator
behavior matches the lowercase normalization used in
pkg/asset/agent/agentconfig/fencingcredentials.go and the hashing logic used by
gatherFencingCredentials so duplicate detection is consistent across
ValidateUniqueAndRequiredFields, validateFencingCredentialsAndPlatform and
gatherFencingCredentials.
---
Duplicate comments:
In `@pkg/asset/machines/master.go`:
- Around line 754-770: In the credentials loop in master.go fix the MAC branch:
remove the stale commented-out mac line, correct the typo credential.MACAdress →
credential.MACAddress, call parsedMAC, err :=
net.ParseMAC(credential.MACAddress) and return the error if non-nil (don’t
discard it), then convert parsedMAC to a string with parsedMAC.String(),
normalize it by removing ":" and lowercasing (e.g.,
strings.ReplaceAll(parsedMAC.String(), ":", "") and strings.ToLower(...)), then
feed that normalized mac into sha256.Sum256 and hex.EncodeToString for name;
keep the existing validate.MAC call but handle ParseMAC defensively to avoid
silent divergence.
---
Nitpick comments:
In `@pkg/types/machinepools.go`:
- Around line 234-235: The YAML tag for MACAddress is using camelCase while
HostName uses all-lowercase, causing inconsistent YAML field names; update the
struct tag on MACAddress (the field named MACAddress in the same struct as
HostName) to use yaml:"macaddress,omitempty" (all lowercase) to match the
existing hostname convention while leaving the json and validate tags unchanged
so downstream YAML consumers see consistent casing.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 72c03c0a-c2ee-408a-b1c4-1bbea8c42255
📒 Files selected for processing (10)
data/data/install.openshift.io_installconfigs.yamlpkg/asset/agent/agentconfig/fencingcredentials.gopkg/asset/agent/agentconfig/fencingcredentials_test.gopkg/asset/machines/master.gopkg/asset/machines/master_test.gopkg/asset/tls/iriregistryauth_test.gopkg/types/common/common.gopkg/types/machinepools.gopkg/types/validation/installconfig.gopkg/types/validation/installconfig_test.go
✅ Files skipped from review due to trivial changes (3)
- pkg/asset/tls/iriregistryauth_test.go
- pkg/asset/agent/agentconfig/fencingcredentials_test.go
- pkg/types/validation/installconfig.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/asset/agent/agentconfig/fencingcredentials.go
- pkg/types/validation/installconfig_test.go
- data/data/install.openshift.io_installconfigs.yaml
- pkg/types/common/common.go
- pkg/asset/machines/master_test.go
fracappa
force-pushed
the
fca/mac-fencing-credentials
branch
from
3e220c2 to
1472d4f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
pkg/types/validation/installconfig.go (1)
1777-1781:⚠️ Potential issue | 🟠 MajorCanonicalize MACs before duplicate validation.
validate.MACaccepts equivalent formats such asAA-BB-CC-DD-EE-01,aa:bb:cc:dd:ee:01, and dotted MACs, but the existinguniqueFieldcheck compares raw strings. Those can pass install-config validation and later collide whenpkg/asset/machines/master.gohashes the parsed canonical MAC into the same Secret name.Consider tracking
net.ParseMAC(credential.MACAddress).String()in this loop after validation succeeds and reportingfield.DuplicateoncredPath.Child("macAddress").🔎 Suggested read-only verification
#!/bin/bash # Verify that MAC uniqueness still relies on raw string comparison while secret naming canonicalizes parsed MACs. rg -nP --type=go -C4 'validate\.MAC\(credential\.MACAddress\)|ValidateUniqueAndRequiredFields\(fencingCredentials\.Credentials' rg -nP --type=go -C4 'net\.ParseMAC\(credential\.MACAddress\)|sha256\.Sum256' rg -nP --type=go -C6 'RegisterValidation\("uniqueField"'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/validation/installconfig.go` around lines 1777 - 1781, The loop that validates credential.MACAddress must canonicalize the MAC before checking uniqueness: after validate.MAC(credential.MACAddress) returns nil, call net.ParseMAC(credential.MACAddress) and use its String() form as the canonical key, maintain a local seen map[string]bool of these canonical MACs, and when a canonical MAC is already present append a field.Duplicate error on credPath.Child("macAddress") (instead of relying on raw string comparison); continue to use field.Invalid for parsing/validation errors and preserve existing credPath/credential.MACAddress references.pkg/asset/agent/agentconfig/fencingcredentials.go (1)
178-188:⚠️ Potential issue | 🟠 MajorUse the parsed canonical MAC as the duplicate key.
strings.ToLoweronly catches case differences. Valid equivalent inputs likeAA-BB-CC-DD-EE-01andaa:bb:cc:dd:ee:01still produce differentseenMACskeys, while the machine asset canonicalizes them before hashing into the same Secret name.🐛 Proposed fix
import ( "context" "fmt" + "net" "os" - "strings" @@ if cred.MACAddress != "" { if err := validate.MAC(cred.MACAddress); err != nil { allErrs = append(allErrs, field.Invalid(credPath.Child("macAddress"), cred.MACAddress, err.Error())) - } - normalizedMAC := strings.ToLower(cred.MACAddress) - if prevIdx, exists := seenMACs[normalizedMAC]; exists { - allErrs = append(allErrs, field.Duplicate(credPath.Child("macAddress"), - fmt.Sprintf("macAddress %q already defined at index %d", cred.MACAddress, prevIdx))) + } else { + parsedMAC, err := net.ParseMAC(cred.MACAddress) + if err != nil { + allErrs = append(allErrs, field.Invalid(credPath.Child("macAddress"), + cred.MACAddress, err.Error())) + continue + } + normalizedMAC := parsedMAC.String() + if prevIdx, exists := seenMACs[normalizedMAC]; exists { + allErrs = append(allErrs, field.Duplicate(credPath.Child("macAddress"), + fmt.Sprintf("macAddress %q already defined at index %d", cred.MACAddress, prevIdx))) + } + seenMACs[normalizedMAC] = i } - seenMACs[normalizedMAC] = i }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/agent/agentconfig/fencingcredentials.go` around lines 178 - 188, The code currently uses strings.ToLower(cred.MACAddress) as the duplicate key which misses equivalent but differently formatted MACs; instead, after validate.MAC(cred.MACAddress) succeeds, parse/canonicalize the MAC into a normalized canonical form (for example parse with net.ParseMAC and format to a consistent lowercase colon-separated hex string) and use that canonical value as the duplicate key (replace normalizedMAC with the parsed canonical MAC) when checking/setting seenMACs; keep references to cred.MACAddress, validate.MAC, normalizedMAC (or its replacement), and seenMACs to locate the change.pkg/types/machinepools.go (1)
234-238:⚠️ Potential issue | 🟠 MajorDon’t use raw
uniqueFieldfor MAC identity.
MACAddressis not a raw-string identity: equivalent valid forms can differ by case or separator format. Withvalidate:"uniqueField", install-config validation may allow values that later canonicalize to the same fencing Secret name.Prefer a MAC-specific validator that parses with
net.ParseMACand comparesparsedMAC.String(), or move canonicalization earlier so all validation paths see the same value.🔎 Suggested read-only verification
#!/bin/bash # Confirm MACAddress still uses raw uniqueField and uniqueField compares raw values. rg -nP --type=go -C3 'MACAddress\s+string.*validate:"uniqueField"' rg -nP --type=go -C8 'RegisterValidation\("uniqueField"' rg -nP --type=go -C4 'net\.ParseMAC\(.*MACAddress|strings\.ToLower\(.*MACAddress'
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/asset/agent/agentconfig/fencingcredentials.go`:
- Around line 178-188: The code currently uses strings.ToLower(cred.MACAddress)
as the duplicate key which misses equivalent but differently formatted MACs;
instead, after validate.MAC(cred.MACAddress) succeeds, parse/canonicalize the
MAC into a normalized canonical form (for example parse with net.ParseMAC and
format to a consistent lowercase colon-separated hex string) and use that
canonical value as the duplicate key (replace normalizedMAC with the parsed
canonical MAC) when checking/setting seenMACs; keep references to
cred.MACAddress, validate.MAC, normalizedMAC (or its replacement), and seenMACs
to locate the change.
In `@pkg/types/validation/installconfig.go`:
- Around line 1777-1781: The loop that validates credential.MACAddress must
canonicalize the MAC before checking uniqueness: after
validate.MAC(credential.MACAddress) returns nil, call
net.ParseMAC(credential.MACAddress) and use its String() form as the canonical
key, maintain a local seen map[string]bool of these canonical MACs, and when a
canonical MAC is already present append a field.Duplicate error on
credPath.Child("macAddress") (instead of relying on raw string comparison);
continue to use field.Invalid for parsing/validation errors and preserve
existing credPath/credential.MACAddress references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7fa9938b-464b-4ee8-896f-62ac8ba349fa
📒 Files selected for processing (10)
data/data/install.openshift.io_installconfigs.yamlpkg/asset/agent/agentconfig/fencingcredentials.gopkg/asset/agent/agentconfig/fencingcredentials_test.gopkg/asset/machines/master.gopkg/asset/machines/master_test.gopkg/asset/tls/iriregistryauth_test.gopkg/types/common/common.gopkg/types/machinepools.gopkg/types/validation/installconfig.gopkg/types/validation/installconfig_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/asset/tls/iriregistryauth_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/types/common/common.go
- pkg/asset/machines/master_test.go
- pkg/types/validation/installconfig_test.go
- pkg/asset/agent/agentconfig/fencingcredentials_test.go
- data/data/install.openshift.io_installconfigs.yaml
fracappa
force-pushed
the
fca/mac-fencing-credentials
branch
from
8b2e838 to
33f97ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
data/data/install.openshift.io_installconfigs.yaml (1)
122-134:⚠️ Potential issue | 🟡 MinorAdd object-level validation requiring at least one identifier in fencing credential schema.
Both
hostNameandmacAddressare optional in the CRD schema, allowing credentials with neither identifier to pass OpenAPI validation. The Go source validates this invariant at runtime (seefencingcredentials.golines 164-167), but the CRD lacks the corresponding schema rule.Add a kubebuilder validation marker to the
Credentialstruct inpkg/types/machinepools.go, then regenerate this file:Proposed kubebuilder marker
// +kubebuilder:validation:XValidationRule="(has(self.hostName) && self.hostName != '') || (has(self.macAddress) && self.macAddress != '')" // +kubebuilder:validation:XValidationRule="message=either hostName or macAddress must be set"Then the generated CRD will include the
x-kubernetes-validationsrule across all three credential blocks (arbiter, compute, controlPlane).Also applies to: 1746–1758, 3310–3322
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/data/install.openshift.io_installconfigs.yaml` around lines 122 - 134, The CRD lacks an object-level OpenAPI validation ensuring at least one identifier is present for fencing credentials; add the kubebuilder XValidation markers shown in the comment to the Credential struct in pkg/types/machinepools.go (the struct referenced by the arbiter/compute/controlPlane credential blocks) so the generated CRD includes the x-kubernetes-validations rule, then regenerate the CRD; reference the runtime check in fencingcredentials.go (around the validation at lines ~164-167) to confirm the rule matches the existing Go invariant and ensure the same validation is added for all three credential occurrences.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/types/machinepools.go`:
- Around line 234-235: The YAML tag for MACAddress is inconsistent with
HostName; update the struct tags on HostName and MACAddress in
pkg/types/machinepools.go so both use the same YAML naming convention
(preferably camelCase to match JSON: yaml:"hostName,omitempty" and
yaml:"macAddress,omitempty"), or if you need to preserve the existing lowercase
hostname for backward compatibility, add a second yaml alias for HostName
(accepting "hostname") and ensure MACAddress uses the chosen convention; edit
the HostName and MACAddress field tags accordingly.
---
Outside diff comments:
In `@data/data/install.openshift.io_installconfigs.yaml`:
- Around line 122-134: The CRD lacks an object-level OpenAPI validation ensuring
at least one identifier is present for fencing credentials; add the kubebuilder
XValidation markers shown in the comment to the Credential struct in
pkg/types/machinepools.go (the struct referenced by the
arbiter/compute/controlPlane credential blocks) so the generated CRD includes
the x-kubernetes-validations rule, then regenerate the CRD; reference the
runtime check in fencingcredentials.go (around the validation at lines ~164-167)
to confirm the rule matches the existing Go invariant and ensure the same
validation is added for all three credential occurrences.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ae260dc-1649-45bb-9189-6ed49200ec85
📒 Files selected for processing (10)
data/data/install.openshift.io_installconfigs.yamlpkg/asset/agent/agentconfig/fencingcredentials.gopkg/asset/agent/agentconfig/fencingcredentials_test.gopkg/asset/machines/master.gopkg/asset/machines/master_test.gopkg/asset/tls/iriregistryauth_test.gopkg/types/common/common.gopkg/types/machinepools.gopkg/types/validation/installconfig.gopkg/types/validation/installconfig_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/asset/tls/iriregistryauth_test.go
- pkg/types/validation/installconfig.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/asset/agent/agentconfig/fencingcredentials.go
- pkg/asset/agent/agentconfig/fencingcredentials_test.go
- pkg/asset/machines/master_test.go
- pkg/asset/machines/master.go
- pkg/types/common/common.go
fracappa
force-pushed
the
fca/mac-fencing-credentials
branch
4 times, most recently
from
e037242 to
1f2a5b1
Compare
|
/test e2e-agent-two-node-fencing-ipv4 |
|
/test ? |
|
/test e2e-agent-two-node-fencing-ipv4 |
2 similar comments
|
/test e2e-agent-two-node-fencing-ipv4 |
|
/test e2e-agent-two-node-fencing-ipv4 |
openshift-ci
Bot
added
the
needs-rebase
fracappa
force-pushed
the
fca/mac-fencing-credentials
branch
from
1f2a5b1 to
4c7904b
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
fracappa
force-pushed
the
fca/mac-fencing-credentials
branch
2 times, most recently
from
1041515 to
780cd53
Compare
fonta-rh
left a comment
fonta-rh
left a comment
There was a problem hiding this comment.
Review: OCPEDGE-2115 — MAC address as alternative fencing credential identifier
The feature design is sound — MAC is the right choice over BMC IP or BaremetalHost CR, and the SHA256-based secret naming is a clean solution. The zero-value skip in common.go is well-implemented and the test coverage for the new functionality is thorough.
However, there are structural issues that should be addressed before merge.
1. Validation duplication violates installer architecture
Files: pkg/asset/agent/agentconfig/fencingcredentials.go vs pkg/types/validation/installconfig.go
The installer follows a layered convention: pkg/types/validation/ owns config-level constraints, and assets in pkg/asset/ trust that validation has already run — they don't re-validate. For precedent, see agenthosts.go: it validates only agent-specific concerns (interfaces, roles, root device hints) without duplicating host-level checks from pkg/types/baremetal/validation/.
This PR breaks that pattern. fencingcredentials.go duplicates the hostname-or-MAC required check, MAC format validation, and duplicate detection — all of which already exist in installconfig.go. The duplication has already drifted:
- Error messages differ:
"hostname or macaddress"(lowercase) vs"hostname or macAddress"(camelCase) - Duplicate detection semantics differ:
fencingcredentials.gonormalizes vianet.ParseMAC(case-insensitive), whileinstallconfig.gorelies onuniqueFieldraw string comparison (case-sensitive, depends on defaults having already lowercased)
Suggestion: Move case-insensitive MAC duplicate detection into installconfig.go (the canonical validation site). Strip the duplicated validation from fencingcredentials.go and let it trust config validation, consistent with how other assets work in this codebase.
2. Broken test assertion in master_test.go
File: pkg/asset/machines/master_test.go:497
assert.Error(t, tc.err, master.Generate(context.Background(), parents))assert.Error(t, err, msgAndArgs...) checks that its second argument (tc.err) is non-nil — which is always true since it's fmt.Errorf(...). The Generate() return value is consumed as a format string argument, never compared. This test passes even if Generate returns nil.
Additionally, the expected error "has neither macAddress nor hostName" doesn't match the actual code "no hostName or macAddress provided" — but this mismatch is invisible because the comparison never happens.
Fix:
err := master.Generate(context.Background(), parents)
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "no hostName or macAddress provided")
}3. Dead continue in fencingcredentials.go MAC validation
File: pkg/asset/agent/agentconfig/fencingcredentials.go:183-189
validate.MAC() internally calls net.ParseMAC(). In the else branch (where validate.MAC succeeded), the second net.ParseMAC() call can never fail — making the error branch + continue dead code. The continue would skip validation of Address, Username, and Password if the branch ever became reachable.
Fix: Remove the redundant net.ParseMAC() call. Since validate.MAC already confirmed the MAC parses successfully:
if cred.MACAddress != "" {
if err := validate.MAC(cred.MACAddress); err != nil {
allErrs = append(allErrs, field.Invalid(credPath.Child("macAddress"),
cred.MACAddress, err.Error()))
} else {
parsedMAC, _ := net.ParseMAC(cred.MACAddress)
normalizedMAC := parsedMAC.String()
if prevIdx, exists := seenMACs[normalizedMAC]; exists {
allErrs = append(allErrs, field.Duplicate(credPath.Child("macAddress"),
fmt.Sprintf("macAddress %q already defined at index %d", cred.MACAddress, prevIdx)))
}
seenMACs[normalizedMAC] = i
}
}(Note: if finding #1 is addressed by moving validation to installconfig.go, this block would be removed entirely from fencingcredentials.go.)
What's good
- Zero-value skip in
common.gois correct and well-tested — the interaction withrequiredtag short-circuiting is sound - SHA256 secret naming with the
openshift.io/fencing-credentialsannotation is a clean design - Backward compatibility is preserved — hostname-only credentials work identically
- YAML omitempty behavior is properly tested for both identifier fields
- The
common_test.gocoverage of the zero-value uniqueness behavior is thorough
fracappa
force-pushed
the
fca/mac-fencing-credentials
branch
3 times, most recently
from
f95e192 to
d81b850
Compare
|
@fonta-rh thanks for the feedback! I should have solved the issues you requested to solve. Just a quick note: for the first point, the redundancy was already present before this PR. I completely remove the method |
fonta-rh
left a comment
fonta-rh
left a comment
There was a problem hiding this comment.
All three issues from the previous review are resolved — nice cleanup.
Two nits (non-blocking):
-
Missing test for invalid MAC format —
installconfig.govalidates MAC viavalidate.MAC(), but there's no test case with a malformed MAC (e.g.,"not-a-mac"). The happy paths and duplicate detection are covered, just no negative format test. -
containsStringincommon_test.go— reimplementsstrings.Contains()from stdlib. Minor, but no reason not to use the standard library version.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: barbacbd, fonta-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
openshift-ci
Bot
added
lgtm
fracappa
force-pushed
the
fca/mac-fencing-credentials
branch
2 times, most recently
from
53a15ea to
034c03c
Compare
|
/lgtm |
|
Minor note for future reference: if this merges as-is, the comment at More importantly: the ABI flow will need a companion PR in openshift/assisted-service before MAC-only fencing credentials work end-to-end. The installer correctly generates Hostname-based fencing credentials (the existing flow) are unaffected. |
…entials Allow fencing credentials to use either hostname or MAC addreess to identify baremetal hosts, rather than requiring hostname. When only MAC address is progvided, the secret name is derived from SHA256 hash of the normalized MAC. The uniqueField validator now skips empty values to avoid false duplicate errors on optional fields.
fracappa
force-pushed
the
fca/mac-fencing-credentials
branch
from
de3aa7b to
f391dbd
Compare
|
/lgtm |
|
@fracappa: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/verified by @fracappa |
openshift-ci-robot
added
the
verified
|
@fracappa: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-merge-bot
Bot
merged commit c5e380f
into
openshift:main
6 participants
Allow fencing credentials to use either hostname or MAC addreess to identify baremetal hosts, rather than requiring hostname. When only MAC address is progvided, the secret name is derived from SHA256 hash of the normalized MAC. The uniqueField validator now skips empty values to avoid false duplicate errors on optional fields.
Summary by CodeRabbit
New Features
Bug Fixes
Tests