OCPEDGE-2115: types: support MAC address as alternative identifier for fencing credential#10513
OCPEDGE-2115: types: support MAC address as alternative identifier for fencing credential#10513fracappa wants to merge 2 commits intoopenshift:mainfrom
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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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
| // getValidOptionalInstallConfigWithMACAddress returns an install config | ||
| // with valid MAC addresess in fencing credentials. | ||
| func getValidOptionalInstallConfigWithMACAddress() *agent.OptionalInstallConfig { | ||
| installConfig := getValidOptionalInstallConfig() | ||
| installConfig.Config.ControlPlane.Replicas = ptr.To(int64(2)) | ||
| installConfig.Config.ControlPlane.Fencing = &types.Fencing{ | ||
| Credentials: []*types.Credential{ | ||
| { | ||
| MACAddress: "AA:BB:CC:DD:EE:01", | ||
| Address: "redfish+https://192.168.111.1:8000/redfish/v1/Systems/abc", | ||
| Username: "admin", | ||
| Password: "password123", | ||
| }, | ||
| { | ||
| MACAddress: "AA:BB:CC:DD:EE:02", | ||
| Address: "redfish+https://192.168.111.1:8000/redfish/v1/Systems/def", | ||
| Username: "admin", | ||
| Password: "password456", | ||
| }, | ||
| }, | ||
| } | ||
| return installConfig | ||
| } | ||
|
|
||
| // getOptionalInstallConfigWithDuplicateMACs returns an install config | ||
| // with duplicate MAC addresess in fencing credentials. | ||
| func getOptionalInstallConfigWithDuplicateMACs() *agent.OptionalInstallConfig { | ||
| installConfig := getValidOptionalInstallConfig() | ||
| installConfig.Config.ControlPlane.Replicas = ptr.To(int64(2)) | ||
| installConfig.Config.ControlPlane.Fencing = &types.Fencing{ | ||
| Credentials: []*types.Credential{ | ||
| { | ||
| MACAddress: "AA:BB:CC:DD:EE:01", | ||
| Address: "redfish+https://192.168.111.1:8000/redfish/v1/Systems/abc", | ||
| Username: "admin", | ||
| Password: "password123", | ||
| }, | ||
| { | ||
| MACAddress: "aa:bb:cc:dd:ee:01", // duplicate (case-insensitive) | ||
| Address: "redfish+https://192.168.111.1:8000/redfish/v1/Systems/def", | ||
| Username: "admin", | ||
| Password: "password456", | ||
| }, | ||
| }, | ||
| } | ||
| return installConfig | ||
| } |
There was a problem hiding this comment.
Nit: typo "addresess" in helper comments.
Both getValidOptionalInstallConfigWithMACAddress and getOptionalInstallConfigWithDuplicateMACs comments read "MAC addresess" — should be "MAC addresses".
✏️ Proposed fix
-// with valid MAC addresess in fencing credentials.
+// with valid MAC addresses in fencing credentials.-// with duplicate MAC addresess in fencing credentials.
+// with duplicate MAC addresses in fencing credentials.📝 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.
| // getValidOptionalInstallConfigWithMACAddress returns an install config | |
| // with valid MAC addresess in fencing credentials. | |
| func getValidOptionalInstallConfigWithMACAddress() *agent.OptionalInstallConfig { | |
| installConfig := getValidOptionalInstallConfig() | |
| installConfig.Config.ControlPlane.Replicas = ptr.To(int64(2)) | |
| installConfig.Config.ControlPlane.Fencing = &types.Fencing{ | |
| Credentials: []*types.Credential{ | |
| { | |
| MACAddress: "AA:BB:CC:DD:EE:01", | |
| Address: "redfish+https://192.168.111.1:8000/redfish/v1/Systems/abc", | |
| Username: "admin", | |
| Password: "password123", | |
| }, | |
| { | |
| MACAddress: "AA:BB:CC:DD:EE:02", | |
| Address: "redfish+https://192.168.111.1:8000/redfish/v1/Systems/def", | |
| Username: "admin", | |
| Password: "password456", | |
| }, | |
| }, | |
| } | |
| return installConfig | |
| } | |
| // getOptionalInstallConfigWithDuplicateMACs returns an install config | |
| // with duplicate MAC addresess in fencing credentials. | |
| func getOptionalInstallConfigWithDuplicateMACs() *agent.OptionalInstallConfig { | |
| installConfig := getValidOptionalInstallConfig() | |
| installConfig.Config.ControlPlane.Replicas = ptr.To(int64(2)) | |
| installConfig.Config.ControlPlane.Fencing = &types.Fencing{ | |
| Credentials: []*types.Credential{ | |
| { | |
| MACAddress: "AA:BB:CC:DD:EE:01", | |
| Address: "redfish+https://192.168.111.1:8000/redfish/v1/Systems/abc", | |
| Username: "admin", | |
| Password: "password123", | |
| }, | |
| { | |
| MACAddress: "aa:bb:cc:dd:ee:01", // duplicate (case-insensitive) | |
| Address: "redfish+https://192.168.111.1:8000/redfish/v1/Systems/def", | |
| Username: "admin", | |
| Password: "password456", | |
| }, | |
| }, | |
| } | |
| return installConfig | |
| } | |
| // getValidOptionalInstallConfigWithMACAddress returns an install config | |
| // with valid MAC addresses in fencing credentials. | |
| func getValidOptionalInstallConfigWithMACAddress() *agent.OptionalInstallConfig { | |
| installConfig := getValidOptionalInstallConfig() | |
| installConfig.Config.ControlPlane.Replicas = ptr.To(int64(2)) | |
| installConfig.Config.ControlPlane.Fencing = &types.Fencing{ | |
| Credentials: []*types.Credential{ | |
| { | |
| MACAddress: "AA:BB:CC:DD:EE:01", | |
| Address: "redfish+https://192.168.111.1:8000/redfish/v1/Systems/abc", | |
| Username: "admin", | |
| Password: "password123", | |
| }, | |
| { | |
| MACAddress: "AA:BB:CC:DD:EE:02", | |
| Address: "redfish+https://192.168.111.1:8000/redfish/v1/Systems/def", | |
| Username: "admin", | |
| Password: "password456", | |
| }, | |
| }, | |
| } | |
| return installConfig | |
| } | |
| // getOptionalInstallConfigWithDuplicateMACs returns an install config | |
| // with duplicate MAC addresses in fencing credentials. | |
| func getOptionalInstallConfigWithDuplicateMACs() *agent.OptionalInstallConfig { | |
| installConfig := getValidOptionalInstallConfig() | |
| installConfig.Config.ControlPlane.Replicas = ptr.To(int64(2)) | |
| installConfig.Config.ControlPlane.Fencing = &types.Fencing{ | |
| Credentials: []*types.Credential{ | |
| { | |
| MACAddress: "AA:BB:CC:DD:EE:01", | |
| Address: "redfish+https://192.168.111.1:8000/redfish/v1/Systems/abc", | |
| Username: "admin", | |
| Password: "password123", | |
| }, | |
| { | |
| MACAddress: "aa:bb:cc:dd:ee:01", // duplicate (case-insensitive) | |
| Address: "redfish+https://192.168.111.1:8000/redfish/v1/Systems/def", | |
| Username: "admin", | |
| Password: "password456", | |
| }, | |
| }, | |
| } | |
| return installConfig | |
| } |
🤖 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 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.
| { | ||
| name: "validation error - invalid MAC address", | ||
| dependencies: []asset.Asset{ | ||
| getOptionalInstallConfigWithInvalidMAC(), | ||
| &workflow.AgentWorkflow{Workflow: workflow.AgentWorkflowTypeInstall}, | ||
| }, | ||
| expectedError: "macAddress", | ||
| }, |
There was a problem hiding this comment.
Weak assertion for invalid MAC test.
expectedError: "macAddress" will match virtually any validation error that references the field name (including unrelated ones like "macAddress is required"). Consider asserting on a more specific substring from the actual parse error (e.g., "invalid MAC address" or whatever net.ParseMAC / the validator emits) so the test actually proves the invalid-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 -
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.
| 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))) | ||
| } | ||
| seenMACs[normalizedMAC] = i |
There was a problem hiding this comment.
Use canonical MACs for duplicate detection.
strings.ToLower catches case-only duplicates, but not equivalent valid formats like AA-BB-CC-DD-EE-01 vs aa:bb:cc:dd:ee:01. Normalize from the parsed MAC before inserting into seenMACs.
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)))
- }
- seenMACs[normalizedMAC] = i
+ } 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
+ }
}📝 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.
| 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))) | |
| } | |
| seenMACs[normalizedMAC] = i | |
| 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 | |
| } | |
| } |
🤖 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 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.
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
| HostName string `json:"hostName,omitempty" yaml:"hostname,omitempty" validate:"uniqueField"` | ||
| MACAddress string `json:"macAddress,omitempty" yaml:"macAddress,omitempty" validate:"uniqueField"` |
There was a problem hiding this comment.
YAML tag casing is inconsistent between HostName and MACAddress.
HostName uses yaml:"hostname,omitempty" (all lowercase) while the new MACAddress uses yaml:"macAddress,omitempty" (camelCase). For a user-facing config schema this inconsistency is easy to get wrong when hand-writing YAML. Consider picking one convention — most likely yaml:"macAddress,omitempty" matches the JSON form, but if parity with the existing hostname convention is intended, use yaml:"macaddress,omitempty" (or, preferably, align both to camelCase and keep hostname as an accepted alias for backward compatibility).
Worth confirming which form the docs/examples added in this PR actually use so users aren't surprised.
🤖 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 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.
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 |
2 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/test e2e-agent-two-node-fencing-ipv4 |
|
/retest |
| allErrs := field.ErrorList{} | ||
| if fencingCredentials != nil { | ||
| for _, credential := range fencingCredentials.Credentials { | ||
| credential.MACAddress = strings.ToLower(credential.MACAddress) |
There was a problem hiding this comment.
Validation should not change the values here.
| name = credential.HostName | ||
| case credential.MACAddress != "": | ||
| // mac := strings.ReplaceAll(strings.ToLower(credential.MACAddress), ":", "") | ||
| if err := validate.MAC(credential.MACAddress); err != nil { |
There was a problem hiding this comment.
Is this redundant with the check below? I think that they both validate a mac address.
| } | ||
| mac := strings.ReplaceAll(strings.ToLower(parsedMAC.String()), ":", "") | ||
| hash := sha256.Sum256([]byte(mac)) | ||
| name = hex.EncodeToString(hash[:8]) |
There was a problem hiding this comment.
Is there a need to use 8 characters? This increases the chance of collision unless this is required.
There was a problem hiding this comment.
There's no specific need I'd say. It was just to give hashing a bit of meaning because MAC addresses are only 12 chars long at the end. Could be more meaningful to use normalized MACs directly
| case credential.HostName != "": | ||
| name = credential.HostName | ||
| case credential.MACAddress != "": | ||
| // mac := strings.ReplaceAll(strings.ToLower(credential.MACAddress), ":", "") |
There was a problem hiding this comment.
Remove the commented string.
| hash := sha256.Sum256([]byte(mac)) | ||
| name = hex.EncodeToString(hash[:8]) | ||
| default: | ||
| return nil, fmt.Errorf("credential at index %d has neither macAddress nor hostName", i) |
There was a problem hiding this comment.
nit: could we change the error message to be slightly different so that it isn't occurring in two places?
| fieldValue := fl.Field().Interface() | ||
|
|
||
| // Skip uniqueness check for zero values (empty strings, zero ints, etc.) | ||
| if fl.Field().IsZero() { |
There was a problem hiding this comment.
this is editing a common place. We need to test and make sure this isn't changing behavior that already exists. Please add unit tests and prove output.
There was a problem hiding this comment.
New tests appear to cover this.
…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
b9586f0 to
efbb418
Compare
|
/retest-required |
1 similar comment
|
/retest-required |
|
@fracappa what is the preference when both |
|
Hi @sadasu! The preferences is leaded by this switch-case statement, where hostname comes first, so it has the priority when both provided. However, I did not include a unit test where both are provided. I'm doing it right now |
Also add a unit test verifying that hostName takes precedence over nacAddress when both are provided in fencing credentials
|
/retest-required |
|
@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. |
|
/retest-required |
4 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