ROX-33557: fix node resolution from hostname by rhacs-bot · Pull Request #19461 · stackrox/stackrox
Please address the comments from this code review: ## Overall Comments - In `getNodeByPrefix`, the warning log for additional matches uses `match.Name`, which refers to the previously selected node instead of the current conflicting node (`k`); this makes the log misleading and likely not what you intended to surface for debugging. - The selection logic in `getNodeByPrefix` compares keys lexicographically (`k < match.Name`), but the comment says it should favor the 'closest match' and 'most likely requested node'; consider aligning the implementation with that intent (e.g., preferring the shortest matching name or longest common prefix) or adjusting the comment to reflect the actual behavior. ## Individual Comments ### Comment 1 <location path="sensor/kubernetes/listener/resources/node_store.go" line_range="109-110" /> <code_context> + + var match *nodeWrap + for k, v := range s.nodes { + if strings.HasPrefix(k, prefix) { + if match != nil { + log.Warnf("Additional nodes match prefix %q: %s", prefix, match.Name) + } </code_context> <issue_to_address> **issue:** Warn log for additional matches logs the previous match name instead of the newly found conflicting key, which can be misleading. Inside the `if match != nil` block, the warning logs `match.Name` (the already selected node) instead of the newly found key `k`. This makes the warning misleading about which node is the “additional” match. Please log `k`, or both `k` and `match.Name`, so the conflicting entries are clearly identified. </issue_to_address> ### Comment 2 <location path="sensor/kubernetes/listener/resources/node_store.go" line_range="113-120" /> <code_context> + if match != nil { + log.Warnf("Additional nodes match prefix %q: %s", prefix, match.Name) + } + if match == nil || k < match.Name { + // favor the closest match, whether the prefix matches more + // of the string. This serves two purposes: + // 1. it is deterministic + // 2. it returns the most likely requested node of the multiple + // matches + match = v </code_context> <issue_to_address> **suggestion:** Lexicographical comparison does not really implement the "closest match" heuristic described in the comment. The logic here returns the lexicographically smallest `k`, which is deterministic but not necessarily the “closest” match (e.g., `ip-10-0-24-9a` vs `ip-10-0-24-9z`). If the intent is a true notion of closeness (e.g., minimal suffix length or another distance metric), the comparison should be updated. Otherwise, consider rephrasing the comment to emphasize determinism and tie-breaking rather than semantic closeness or “most likely requested node.” ```suggestion if match == nil || k < match.Name { // favor a deterministic match by choosing the lexicographically // smallest node name among matches. This serves two purposes: // 1. it is deterministic // 2. it provides a stable tie-breaker when multiple nodes // match the same prefix, but does not guarantee semantic // "closeness" to the requested node name match = v } ``` </issue_to_address>