fix(docker): avoid panic on colliding nested crowdsec labels#4555
fix(docker): avoid panic on colliding nested crowdsec labels#4555arpitjain099 wants to merge 3 commits into
Conversation
parseKeyToMap descended into nested label maps with an unchecked type
assertion:
m = m[parts[i]].(map[string]interface{})
When a container carries both a leaf label and a branch label under the
same key (for example crowdsec.enable=true and crowdsec.enable.foo=bar),
the intermediate key already holds a string, so the assertion panics
with "interface conversion: interface {} is string, not
map[string]interface {}". Container labels are set by whoever launches
the container, so on a shared host this can crash the acquisition
goroutine.
Use the comma-ok form and replace a non-map (or absent) value with a
fresh nested map. Added a regression test.
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
|
@arpitjain099: There are no 'kind' label on this PR. You need a 'kind' label to generate the release automatically.
DetailsI am a bot created to help the crowdsecurity developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the BirthdayResearch/oss-governance-bot repository. |
|
@arpitjain099: There are no area labels on this PR. You can add as many areas as you see fit.
DetailsI am a bot created to help the crowdsecurity developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the BirthdayResearch/oss-governance-bot repository. |
| // The key is absent, or a leaf value (e.g. a sibling label like | ||
| // crowdsec.enable=true) is already stored here. Replace it with a | ||
| // nested map instead of panicking on the type assertion. | ||
| next = make(map[string]interface{}) |
There was a problem hiding this comment.
I think there's a chance of skipping some values depending on the order of the elements in the map (which is random) ?
We probably want iterate over a sorted slice of all labels to avoid potential overwrite.
There was a problem hiding this comment.
Good catch, you're right that the random map order made this non-deterministic. I pushed a change to parseLabels that sorts the keys before building the tree, so the shorter key (crowdsec.enable) is always applied before the more specific one (crowdsec.enable.foo).
With genuinely colliding labels only one can survive, since enable can't be both a string and a nested map at once, but now the branch wins on every run instead of it being a coin flip on iteration order. I strengthened the test to assert the branch wins and to run parseLabels repeatedly so any remaining order dependence would fail it.
Colliding nested labels (a leaf and a branch under the same key, e.g. crowdsec.enable and crowdsec.enable.foo) can only resolve one way, but Go map iteration order is random, so which value won was decided at random from run to run. Iterate over sorted keys so the shorter, less specific key is always applied first and then overwritten by the more specific one. Strengthen the test to assert the branch wins deterministically and unrelated labels survive. Signed-off-by: arpitjain099 <arpitjain099@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4555 +/- ##
==========================================
- Coverage 63.81% 63.81% -0.01%
==========================================
Files 507 507
Lines 38584 38590 +6
==========================================
+ Hits 24624 24625 +1
- Misses 11613 11617 +4
- Partials 2347 2348 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
parseKeyToMapwalks into nested label maps with an unchecked type assertion:If a container carries both a leaf label and a branch label under the same key, the intermediate key already holds a string, so the assertion panics:
Container labels are set by whoever launches the container, so on a shared host this can crash the acquisition goroutine (an unrecovered goroutine panic takes down the process). The trigger is also order-dependent because
parseLabelsranges over a Go map, so today it is a flaky panic vs. a silent clobber depending on iteration order.Fix: use the comma-ok form and replace a non-map (or absent) value with a fresh nested map, so a deeper key takes precedence instead of panicking.
Added
TestParseLabelsNestedCollisionDoesNotPanic(asserts no panic and that unrelated labels survive; the exact winner is map-order-dependent, which is pre-existing). Rango test ./pkg/acquisition/modules/docker/locally (passes; the test panics without the fix).