OCPBUGS-75009: sort list of map entries on DCM#738
Merged
openshift-merge-bot[bot] merged 2 commits intoopenshift:masterfrom Mar 11, 2026
Merged
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-75009, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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-ci-robot
added
the
jira/valid-bug
Member
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
Member
Author
|
This is a rework on #726, that should be closed. |
Contributor
|
/assign @bentito |
jcmoraisjr
force-pushed
the
OCPBUGS-75009-sync-map-entries
branch
from
5cd2903 to
1a3428b
Compare
Member
Author
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
jcmoraisjr
commented
Feb 25, 2026
bentito
reviewed
Feb 25, 2026
|
|
||
| // addEntry adds a new haproxy map entry. | ||
| func (m *HAProxyMap) addEntry(k string, v templaterouter.ServiceAliasConfigKey) error { | ||
| keyExpr := escapeKeyExpr(string(k)) |
Contributor
|
/lgtm |
|
tested it with 4.22.0-0-2026-02-26-064510-test-ci-ln-1m7iqkb-latest |
|
/label qe-approved |
openshift-ci-robot
added
the
verified
Contributor
|
@ShudiLi: 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-ci
Bot
added
the
qe-approved
Contributor
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-75009, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
Member
Author
|
/retest |
jcmoraisjr
force-pushed
the
OCPBUGS-75009-sync-map-entries
branch
from
1a3428b to
61ddc88
Compare
openshift-ci-robot
removed
the
verified
|
Tested it with 4.22.0-0-2026-02-28-012008-test-ci-ln-i3ckfyk-latest |
openshift-ci-robot
removed
the
jira/invalid-bug
Contributor
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-75009, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/router/template/configmanager/haproxy/testing/haproxy.go (1)
148-159: Consider adding lock protection for thread safety.
SetCustomMapmodifiesp.mapswithout acquiringp.lock, while other methods likeshowMapandaddMapPayloaddo lock. Although the test usage pattern (setup before test operations) makes races unlikely, adding lock protection would be more robust.♻️ Proposed fix
func (p *fakeHAProxy) SetCustomMap(filename string, lines []string) { + p.lock.Lock() + defer p.lock.Unlock() var entries []haproxyMapEntry for _, line := range lines { params := strings.SplitN(line, " ", 3) entries = append(entries, haproxyMapEntry{ id: params[0], key: params[1], value: params[2], }) } p.maps[filename] = entries }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go` around lines 148 - 159, SetCustomMap on fakeHAProxy writes to p.maps without using the mutex p.lock, which is inconsistent with other methods like showMap and addMapPayload; modify SetCustomMap to acquire the lock (e.g., p.lock.Lock() / defer p.lock.Unlock()) around the mutation of p.maps to ensure thread safety and consistency with the fakeHAProxy's other map-manipulating methods.
🤖 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/router/template/configmanager/haproxy/map.go`:
- Around line 146-148: The call to Refresh() in the dirty-check path ignores its
error and may let sync proceed with stale/empty m.entries; change the call in
the if m.dirty block to capture the error returned by m.Refresh(), handle it by
aborting the sync (returning or propagating the error) or logging and preserving
current entries instead of continuing, and ensure subsequent merge logic (which
reads m.entries) only runs when Refresh() succeeds; update any callers of this
method to handle the propagated error as needed.
In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go`:
- Around line 321-337: The function fakeHAProxy.commitMap populates a local
response slice (using p.pendingMaps, p.maps, delete) but ignores it and always
returns the constant "\n"; change the return to emit the computed response
(e.g., join the response slice with newlines and ensure a trailing newline) so
errors/messages built into response are actually returned; update the code
around commitMap (and add a strings import if necessary) so the function returns
the joined response instead of the hard-coded "\n".
---
Nitpick comments:
In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go`:
- Around line 148-159: SetCustomMap on fakeHAProxy writes to p.maps without
using the mutex p.lock, which is inconsistent with other methods like showMap
and addMapPayload; modify SetCustomMap to acquire the lock (e.g., p.lock.Lock()
/ defer p.lock.Unlock()) around the mutation of p.maps to ensure thread safety
and consistency with the fakeHAProxy's other map-manipulating methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ffda88f-2ea7-4516-a341-32988380b24c
⛔ Files ignored due to path filters (23)
vendor/github.com/stretchr/testify/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_compare.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_format.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_format.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_forward.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_forward.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_order.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/forward_assertions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/http_assertions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/yaml/yaml_custom.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/yaml/yaml_default.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/yaml/yaml_fail.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/forward_requirements.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require_forward.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require_forward.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/requirements.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (6)
go.modpkg/router/template/configmanager/haproxy/client_test.gopkg/router/template/configmanager/haproxy/manager.gopkg/router/template/configmanager/haproxy/map.gopkg/router/template/configmanager/haproxy/map_test.gopkg/router/template/configmanager/haproxy/testing/haproxy.go
jcmoraisjr
force-pushed
the
OCPBUGS-75009-sync-map-entries
branch
from
d541766 to
e400c89
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/router/template/configmanager/haproxy/client_test.go`:
- Around line 395-397: The loop calling m.SyncEntries(configEntryMap{"key":
"value"}, true) must check its returned error and fail the test if non-nil to
ensure the fake server was actually seeded; update the setup loop in
client_test.go to capture the error from SyncEntries on each map (the maps
variable and configEntryMap usage) and call t.Fatalf or t.Fatalf-like assertion
with the error (and context identifying which map) so a failed SyncEntries
cannot leave commands on the fake server and falsely pass the test.
In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go`:
- Around line 301-317: The fake HAProxy server's addMapPayload is incorrectly
appending newlines onto an existing p.pendingMaps[name], causing staged map
state to accumulate across prepare operations; instead, replace the pending map
entries for that name with only the new lines. Modify addMapPayload (and any use
of p.pendingMaps[name]) to initialize m as an empty slice before iterating
newlines (i.e., do not start from the existing p.pendingMaps[name]) so
p.pendingMaps[name] becomes exactly the parsed entries from newlines; keep the
rest of the locking and response logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f68f1b82-f091-409b-af3d-a9821d4a38f8
⛔ Files ignored due to path filters (23)
vendor/github.com/stretchr/testify/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_compare.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_format.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_format.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_forward.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_forward.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_order.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/forward_assertions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/http_assertions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/yaml/yaml_custom.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/yaml/yaml_default.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/yaml/yaml_fail.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/forward_requirements.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require_forward.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require_forward.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/requirements.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (6)
go.modpkg/router/template/configmanager/haproxy/client_test.gopkg/router/template/configmanager/haproxy/manager.gopkg/router/template/configmanager/haproxy/map.gopkg/router/template/configmanager/haproxy/map_test.gopkg/router/template/configmanager/haproxy/testing/haproxy.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
jcmoraisjr
force-pushed
the
OCPBUGS-75009-sync-map-entries
branch
from
e400c89 to
95c896b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/router/template/configmanager/haproxy/map_test.go (1)
494-507: Add a same-host/different-path regression case here too.OCPBUGS-75009 was about overlapping routes on the same hostname, but this reorder case only changes hostnames. A fixture with the same host and a more specific path would pin the exact
SortMapPathsbehavior this PR is protecting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/router/template/configmanager/haproxy/map_test.go` around lines 494 - 507, Add a regression test that mirrors the "add and reorder" case but uses the same hostname with a more specific path to exercise SortMapPaths behavior: in map_test.go create a new test entry in the table (similar structure to the existing "add and reorder") where currentEntries contains two entries sharing the same host but different path specificity, newEntries (a configEntryMap) adds a route for the same host with a specific path, and expectedEntries asserts the precise order after calling the code under test (the same expectation logic used for the existing case). Ensure you reference the same fields (currentEntries, newEntries, add, expectedEntries) and the SortMapPaths ordering semantics so the test pins the overlapping-host/different-path scenario described by OCPBUGS-75009.
🤖 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/router/template/configmanager/haproxy/map.go`:
- Around line 156-180: The current merge collapses duplicate HAProxyMap.entries
by key (m.entries / entry.Name) causing unrelated duplicate rows to be rewritten
or removed; fix by replacing the boolean set "added" with a per-key counter and
treat duplicates one-by-one: when iterating m.entries, for each entry check
newEntries[entry.Name] and if present either replace that single occurrence (in
add mode) or remove that single occurrence (in remove mode) and increment a
counter for entry.Name; after the loop, when appending remaining newEntries,
only append keys for which the counter shows fewer replacements than intended
(so you don't silently collapse or delete extra duplicate rows). Use the
existing symbols m.entries, entry.Name, newEntries, add, and lines and change
added from a set to a map[string]int (or similar counter) to implement this
one-to-one merge behavior.
In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go`:
- Around line 454-470: The fake HAProxy server's process routine reads only the
first ~1024 bytes of the incoming command so multi-line "add map" payloads get
truncated and don't reach addMapPayload; change process to read the full request
(e.g., loop until the connection is closed or until a defined command
terminator) instead of a single fixed-size Read, so that multi-line payloads are
preserved and passed intact to addMapPayload/prepareMap/commitMap; ensure the
parsing logic still splits into lines and trims prefixes after you switch to a
full-read approach.
- Around line 163-168: ReadMapContent reads p.maps without synchronization
causing data races; acquire the same mutex used by writers (p.lock) at the start
of ReadMapContent, defer its unlock, then iterate p.maps to build the response.
Ensure you use the existing lock field on the fakeHAProxy struct so the read is
protected the same way writers are (matching other methods that take p.lock).
---
Nitpick comments:
In `@pkg/router/template/configmanager/haproxy/map_test.go`:
- Around line 494-507: Add a regression test that mirrors the "add and reorder"
case but uses the same hostname with a more specific path to exercise
SortMapPaths behavior: in map_test.go create a new test entry in the table
(similar structure to the existing "add and reorder") where currentEntries
contains two entries sharing the same host but different path specificity,
newEntries (a configEntryMap) adds a route for the same host with a specific
path, and expectedEntries asserts the precise order after calling the code under
test (the same expectation logic used for the existing case). Ensure you
reference the same fields (currentEntries, newEntries, add, expectedEntries) and
the SortMapPaths ordering semantics so the test pins the
overlapping-host/different-path scenario described by OCPBUGS-75009.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df0cb794-0014-4e09-8627-3acbf01438e1
⛔ Files ignored due to path filters (23)
vendor/github.com/stretchr/testify/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_compare.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_format.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_format.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_forward.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_forward.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_order.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/forward_assertions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/http_assertions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/yaml/yaml_custom.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/yaml/yaml_default.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/yaml/yaml_fail.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/forward_requirements.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require_forward.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require_forward.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/requirements.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (6)
go.modpkg/router/template/configmanager/haproxy/client_test.gopkg/router/template/configmanager/haproxy/manager.gopkg/router/template/configmanager/haproxy/map.gopkg/router/template/configmanager/haproxy/map_test.gopkg/router/template/configmanager/haproxy/testing/haproxy.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/router/template/configmanager/haproxy/client_test.go
- pkg/router/template/configmanager/haproxy/manager.go
jcmoraisjr
force-pushed
the
OCPBUGS-75009-sync-map-entries
branch
from
95c896b to
6caa636
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)
pkg/router/template/configmanager/haproxy/testing/haproxy.go (1)
202-215:⚠️ Potential issue | 🟡 Minor
Reset()should rebuild fake map state, not just reload defaults.Line 210 repopulates the built-in maps, but
initialize()never clearsp.mapsorp.pendingMapsfirst. A custom map or a leftover staged transaction can surviveReset()and bleed into the next test case.🧹 Suggested fix
func (p *fakeHAProxy) initialize() { redirectMap := []haproxyMapEntry{ {id: "0x559a137bb720", key: `^route\.edge\.test(:[0-9]+)?(/.*)?$`, value: `be_edge_http:ns1:edge-redirect-to-https`}, {id: "0x559a137bb7e0", key: `^redirect\.blueprints\.test(:[0-9]+)?(/.*)?$`, value: `be_edge_http:blueprints:blueprint-redirect-to-https`}, } @@ p.lock.Lock() defer p.lock.Unlock() + p.maps = make(map[string][]haproxyMapEntry, len(mapNames)) + p.pendingMaps = make(map[string][]haproxyMapEntry) for k, v := range mapNames { name := path.Join(haproxyConfigDir, k) p.maps[name] = v } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go` around lines 202 - 215, Reset() / initialize() currently repopulates built-in maps but does not clear existing state, so custom maps or staged transactions persist; modify the initialization logic in the testing haproxy fake (the function containing initialize()/Reset() which builds mapNames and writes to p.maps) to acquire p.lock, clear p.maps and p.pendingMaps (e.g. set them to empty maps or nil then reassign) before populating mapNames into p.maps, ensuring any leftover staged state is removed and the fake starts clean; keep the existing population of mapNames and use haproxyConfigDir/name as before.
♻️ Duplicate comments (1)
pkg/router/template/configmanager/haproxy/testing/haproxy.go (1)
429-437:⚠️ Potential issue | 🟠 MajorThe fake server still hard-limits batched map updates to 4 KiB.
Line 434 performs a single
Read, and Lines 435-437 reject any request that fills the buffer.SyncEntriesnow sends whole-map payloads throughadd map @... <<, so larger maps are still truncated or rejected and the fake diverges from HAProxy behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go` around lines 429 - 437, The fakeHAProxy.process currently reads once into a fixed 4KiB buffer and errors if the buffer fills, which truncates or rejects larger batched map updates; update process (function fakeHAProxy.process) to read the full request stream instead of a single Read: accumulate data by looping reads until EOF/connection close or use io.ReadAll / bufio.Reader to consume the entire request, remove the "response larger than buffer" check and handle arbitrarily-sized payloads before parsing the add map @... << payload so the fake matches real HAProxy behavior.
🤖 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/router/template/configmanager/haproxy/map.go`:
- Around line 194-198: The code currently trims the "New version created: "
prefix from prepareResponse (derived from prepareResponseRaw) and ignores
strconv.Atoi errors, which can accept invalid responses; update the logic in the
prepare map handling to first verify that prepareResponse actually has the
expected prefix (using strings.HasPrefix or checking the result of TrimPrefix),
then call strconv.Atoi on the suffix and check the error, and finally ensure the
parsed version is a positive non-zero integer; if the prefix is missing, Atoi
fails, or version <= 0, return a descriptive fmt.Errorf including the raw
prepareResponse so invalid responses (e.g., "1" or "-1") are rejected.
---
Outside diff comments:
In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go`:
- Around line 202-215: Reset() / initialize() currently repopulates built-in
maps but does not clear existing state, so custom maps or staged transactions
persist; modify the initialization logic in the testing haproxy fake (the
function containing initialize()/Reset() which builds mapNames and writes to
p.maps) to acquire p.lock, clear p.maps and p.pendingMaps (e.g. set them to
empty maps or nil then reassign) before populating mapNames into p.maps,
ensuring any leftover staged state is removed and the fake starts clean; keep
the existing population of mapNames and use haproxyConfigDir/name as before.
---
Duplicate comments:
In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go`:
- Around line 429-437: The fakeHAProxy.process currently reads once into a fixed
4KiB buffer and errors if the buffer fills, which truncates or rejects larger
batched map updates; update process (function fakeHAProxy.process) to read the
full request stream instead of a single Read: accumulate data by looping reads
until EOF/connection close or use io.ReadAll / bufio.Reader to consume the
entire request, remove the "response larger than buffer" check and handle
arbitrarily-sized payloads before parsing the add map @... << payload so the
fake matches real HAProxy behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1cbdc037-2fcc-483c-862d-02e48f22f953
⛔ Files ignored due to path filters (23)
vendor/github.com/stretchr/testify/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_compare.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_format.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_format.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_forward.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_forward.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_order.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/forward_assertions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/http_assertions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/yaml/yaml_custom.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/yaml/yaml_default.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/yaml/yaml_fail.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/forward_requirements.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require_forward.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require_forward.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/requirements.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (6)
go.modpkg/router/template/configmanager/haproxy/client_test.gopkg/router/template/configmanager/haproxy/manager.gopkg/router/template/configmanager/haproxy/map.gopkg/router/template/configmanager/haproxy/map_test.gopkg/router/template/configmanager/haproxy/testing/haproxy.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/router/template/configmanager/haproxy/client_test.go
When DCM is enabled, creating or modifying routes trigger HAProxy map updates via API. These maps are used to select the right backend according to the incoming request. In case of creation, new map entries are added in the end of the map file. In case of a modification, DCM reuses part of the template workflow and performs a remove+add calls, leading to add the changed route to the end of the map list. This approach can potentially link a request to the wrong backend, in case there is an overlap, and the more specific one is created or changed: the less specific one will be used, which does not give the specific one to match. This update removes the individual delete and add map entry API calls, so the following workflow takes place whenever a creation or modification is needed: * merge current and new desired content, either adding, replacing or removing; * the resulting list is sorted, using the same algorithm used by the non DCM counterpart; * the map is fully replaced on an atomic operation, via prepare map + add map + commit map operations.
Adding and removing map was changed to a single operation that adds and removes all the maps on a single step.
jcmoraisjr
force-pushed
the
OCPBUGS-75009-sync-map-entries
branch
from
6caa636 to
f13a5c9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/router/template/configmanager/haproxy/testing/haproxy.go (2)
32-35: UnexportCustomHAProxyMapor align entry type visibility.
CustomHAProxyMap.Entriesis public buthaproxyMapEntryand its fields are private, making the exported field unusable to callers outside this package. SinceCustomHAProxyMapis test-internal (no usage outside its file), consider unexporting it to match its supporting types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go` around lines 32 - 35, CustomHAProxyMap is exported while its element type haproxyMapEntry (and its fields) are unexported, making Entries unusable externally; rename CustomHAProxyMap to unexported customHAProxyMap to match haproxyMapEntry (or alternatively export haproxyMapEntry and its fields) so visibility aligns—update all references in this test file to use customHAProxyMap (or export haproxyMapEntry if you prefer external use).
301-303: Enhance the fake to track and validate@versionacross prepare, add, and commit operations.The fake currently hardcodes
prepareMap()to return version1and bothaddMapPayload()andcommitMap()ignore the version parameter parsed from commands. The real HAProxy client extracts a version fromprepare map(lines 190-196 in map.go) and uses the same version in bothadd map @<version>(line 203) andcommit map @<version>(line 221) commands. Since the fake doesn't validate versions, tests won't detect if the client reuses stale versions, mismatches versions between add and commit, or skips the prepare step entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go` around lines 301 - 303, The fakeHAProxy currently returns a hardcoded "New version created: 1" in prepareMap() and ignores the version in addMapPayload() and commitMap(); update fakeHAProxy to track the current `@version` as a field (e.g., in the fakeHAProxy struct), make prepareMap() parse/increment and store the new version and return "New version created: <version>\n", and make addMapPayload(command string) and commitMap(command string) parse the @<version> token from the incoming command and validate it matches the stored version—if it doesn't, return an error (or record a failure) so tests can detect reused/stale/missing prepare calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go`:
- Around line 32-35: CustomHAProxyMap is exported while its element type
haproxyMapEntry (and its fields) are unexported, making Entries unusable
externally; rename CustomHAProxyMap to unexported customHAProxyMap to match
haproxyMapEntry (or alternatively export haproxyMapEntry and its fields) so
visibility aligns—update all references in this test file to use
customHAProxyMap (or export haproxyMapEntry if you prefer external use).
- Around line 301-303: The fakeHAProxy currently returns a hardcoded "New
version created: 1" in prepareMap() and ignores the version in addMapPayload()
and commitMap(); update fakeHAProxy to track the current `@version` as a field
(e.g., in the fakeHAProxy struct), make prepareMap() parse/increment and store
the new version and return "New version created: <version>\n", and make
addMapPayload(command string) and commitMap(command string) parse the @<version>
token from the incoming command and validate it matches the stored version—if it
doesn't, return an error (or record a failure) so tests can detect
reused/stale/missing prepare calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 88c5474a-e674-4e5e-ae98-ca10fc81c8f4
⛔ Files ignored due to path filters (23)
vendor/github.com/stretchr/testify/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_compare.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_format.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_format.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_forward.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_forward.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertion_order.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/assertions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/forward_assertions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/http_assertions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/yaml/yaml_custom.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/yaml/yaml_default.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/assert/yaml/yaml_fail.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/forward_requirements.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require_forward.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/require_forward.go.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/require/requirements.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (6)
go.modpkg/router/template/configmanager/haproxy/client_test.gopkg/router/template/configmanager/haproxy/manager.gopkg/router/template/configmanager/haproxy/map.gopkg/router/template/configmanager/haproxy/map_test.gopkg/router/template/configmanager/haproxy/testing/haproxy.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/router/template/configmanager/haproxy/client_test.go
- go.mod
- pkg/router/template/configmanager/haproxy/manager.go
Member
Author
|
/retest |
Contributor
|
/lgtm |
Contributor
|
/approve |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bentito 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 |
openshift-ci
Bot
added
the
approved
|
tested it with 4.22.0-0-2026-03-11-044554-test-ci-ln-0lkwtkb-latest |
|
/verified by @ShudiLi |
openshift-ci-robot
added
the
verified
Contributor
|
@ShudiLi: 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. |
Member
Author
|
/test unit |
Contributor
|
@jcmoraisjr: all tests passed! 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. |
Contributor
|
@jcmoraisjr: Jira Issue Verification Checks: Jira Issue OCPBUGS-75009 Jira Issue OCPBUGS-75009 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
Contributor
|
Fix included in accepted release 4.22.0-0.nightly-2026-03-13-065313 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When DCM is enabled, creating or modifying routes trigger HAProxy map updates via API. These maps are used to select the right backend according to the incoming request. In case of creation, new map entries are added in the end of the map file. In case of a modification, DCM reuses part of the template workflow and performs a remove+add calls, leading to add the changed route to the end of the map list.
This approach can potentially link a request to the wrong backend, in case there is an overlap, and the more specific one is created or changed: the less specific one will be used, which does not give the specific one to match.
This update removes the individual delete and add map entry API calls, so the following workflow takes place whenever a creation or modification is needed:
Summary by CodeRabbit
Improvements
Tests
New Features