Skip to content

fix(routeros): address RouterOS per-family address forms and make duplicate inserts idempotent - #382

Merged
svenshi merged 3 commits into
svenshi:fix/ros_address_listfrom
kirileec:fix/ros-address-list-routeros-address-forms
Oct 12, 2026
Merged

svenshi merged 3 commits into
svenshi:fix/ros_address_listfrom
kirileec:fix/ros-address-list-routeros-address-forms

Conversation

@kirileec

Copy link
Copy Markdown

fix(routeros): address RouterOS per-family address forms and make duplicate inserts idempotent

改动目标和用户/运维影响

修复 ros_address_list 在 RouterOS 上对 IPv6 条目的写入/查询形式不一致导致的重复插入报错。

RouterOS 按地址族以不同形式存储 host 条目,且查询是对存储属性的字符串相等比较(RouterOS 7.24.5 实测):

写入 实际存储 ?address=<裸地址> ?address=<带掩码>
198.51.100.99 或 .../32 198.51.100.99 命中 0 行
2001:db8::1 或 .../128 2001:db8::1/128 0 行 命中

原 router_value() 对两种族都去掉掩码:IPv6 写入落为 /128,之后每次查询用裸地址都查不到 → upsert 判定条目不存在 → add 被 RouterOS 以 failure: already have such entry 拒绝 → 记为写入错误并丢弃租约 → 下一次观察重复整个循环,永不收敛。IPv4 写入与查询形式一致,因此该错误只来自 AAAA 记录。

delete_entry_if_matches 使用同一套裸地址查询,所以 cleanup_on_shutdown: true 时也永远删不掉 IPv6 行——这是比日志更严重的运维影响。

修复后:按族返回 RouterOS 实际存储的形式;重复插入按幂等处理,不再计为写入错误、不再丢弃租约。历史写入的 /128 行重新对本插件可见,可被刷新和清理。

配置、API、持久化、feature 和平台兼容性

  • 无新增/修改/删除配置字段,parse_plugin_config 不变。
  • 无 API 变更,无持久化格式变更。
  • 无 feature / bundle 变更。
  • 无平台相关代码,跨平台行为一致。
  • 无新增系统副作用;RouterOS 命令序列与改动前一致(仅 address 属性文本按族不同)。

是否修改 DNS 请求热路径

否。改动只涉及 RouterOS 管理面的地址文本与错误分类;execute_with_next 的入队路径、mailbox 合并、worker 循环均未改动。异步模式下 DNS 应答路径不受影响。

已同步的表示

  • Rust:ros_address_list/model.rs、ros_address_list/api.rs、executor/routeros/transport.rs(后两者为 ros_route 共用的 transport)
  • 单元测试:三个改动文件内各自新增(model.rs 1、transport.rs 2、api.rs 1)
  • 中英文插件参考:各 1 条 Behavior
  • 无需同步:WebUI plugin definition(无新配置字段)、README*(能力无变化)、config.yaml(默认值无变化)、tests/plugin_integration.rs(该文件对本插件仅有一条 TLS 初始化用例,本次不改配置解析与工厂注册,其覆盖不受影响)

实际运行的验证命令

cargo test --no-default-features --features "minimal,plugin-mikrotik" --lib
# 845 passed; 0 failed

cargo +nightly fmt --all --check        # clean

cargo +nightly clippy --no-default-features --features "minimal,plugin-mikrotik" --lib --tests
# 改动文件零告警

cd docs && node scripts/check-content.mjs
# Documentation content checks passed.

真机验证(RouterOS 7.24.5 + OxiDNS 1.6.0,临时列表与临时 plugin tag,不触碰生产):

  • 预演:IPv4 写裸地址、IPv6 写 /128;过 75% TTL 再次观察时同一 .id 原地刷新,already have such entry 0 条。
  • 关闭清理 A/B(同一种子数据仅替换二进制):原版删除 IPv4 行但留下 3 条 /128 行;本 PR 版本 IPv4/IPv6 全部删除(0/0)。
  • 生产运行(约 10 分钟):ros_address_list_write_error_total 0,对比原版 61 次/96 分钟;日志中 already have such entry 0 条,对比原版每 60 秒 1 条。

尚未验证的平台或环境假设

  • cargo +nightly clippy --all-targets --all-features -- -D warnings(官方 just check 的门禁)在 nightly 1.101.0-nightly (2026-10-10) 下会失败,但两处错误均位于本次未改动的文件:src/infra/upgrade/mod.rs:287(unused import)与 src/infra/network/dial.rs:483(needless_bool)。按"不在同一 PR 中顺手重构无关模块"的要求未一并修复,建议维护者确认其 CI 使用的 nightly 版本。
  • 重复插入的恢复路径(整表扫描回退)需要真实 RouterOS 或等价的 fake peer 才能端到端覆盖;仓库目前没有 RouterOS fake,该路径由 transport 分类器单测 + 真机验证覆盖。
  • 真机验证使用 RouterOS 7.24.5 与 OxiDNS 1.6.0(full,musl 静态);未在 RouterOS 6.x、其它 bundle 组合或 API-SSL(8729)下验证。

提交结构

三个提交,各自独立可编译、可测试:

  1. fix(ros_address_list): use the address form RouterOS stores per family — 根因,只改 model.rs
  2. fix(routeros): treat duplicate-insert rejection as an idempotent outcome — 防御,transport.rs + api.rs
  3. docs(routeros): document idempotent duplicate-insert handling — 中英文档

关于 comment 中的 dm=:v1.5.0 起动态条目已从"域名级归属"改为"IP/CIDR 租约"语义(comment_codec_roundtrip 明确断言 comment 不含 dm=),本 PR 不恢复该字段——同一 IP 在多域名下会因此反复改写 comment 并触发额外 set。


English Version

fix(routeros): address RouterOS per-family address forms and make duplicate inserts idempotent

Goal and operational impact

Fix the duplicate-insert failure caused by a mismatch between how
ros_address_list writes IPv6 address-list entries and how it later queries
them.

RouterOS stores host address-list entries in a family-specific form and
compares the stored property with string equality. Measured on RouterOS
7.24.5:

written stored ?address=<bare> ?address=<masked>
198.51.100.99 or .../32 198.51.100.99 hit 0 rows
2001:db8::1 or .../128 2001:db8::1/128 0 rows hit

router_value() stripped the mask for both families, so IPv6 writes landed as
/128 while every later lookup asked for the bare address and found nothing.
The upsert then concluded the row was absent, add was rejected with
failure: already have such entry, and the resulting error dropped the lease
so the next observation repeated the whole cycle. IPv4 write and lookup
agreed, so the failure only ever came from AAAA records.

delete_entry_if_matches used the same bare-address lookup, so
cleanup_on_shutdown: true could not remove IPv6 rows either — an operational
impact beyond the log noise.

After this change the plugin emits the form RouterOS keeps per family, and a
duplicate insert is treated as idempotent: no write error, no dropped lease.
Rows written as /128 by an older build become visible to the plugin again,
so they can be refreshed and cleaned up.

Config, API, persistence, feature and platform compatibility

  • No config fields added, changed or removed; parse_plugin_config is untouched.
  • No API change, no persistence format change.
  • No feature / bundle change.
  • No platform-specific code; behaviour is identical across platforms.
  • No new system side effects. The RouterOS command sequence is unchanged apart
    from the address text, which now follows the family-specific stored form.

Does it touch the DNS request hot path

No. The change is confined to RouterOS management-plane address text and error
classification. The execute_with_next enqueue path, mailbox coalescing and
worker loop are untouched, so the async-mode DNS response path is unaffected.

Representations kept in sync

  • Rust: ros_address_list/model.rs, ros_address_list/api.rs,
    executor/routeros/transport.rs (the latter is shared with ros_route)
  • Unit tests: one new test in each of the three changed files
    (model.rs ×1, transport.rs ×2, api.rs ×1)
  • Plugin reference: one Behavior bullet in each of the Chinese and English docs
  • Not required: WebUI plugin definitions (no new config fields), README*
    (no capability change), config.yaml (no default change),
    tests/plugin_integration.rs (its only ros_address_list case is a TLS
    init test; config parsing and factory registration are unchanged, so its
    coverage is unaffected)

Verification actually run

cargo test --no-default-features --features "minimal,plugin-mikrotik" --lib
# 845 passed; 0 failed

cargo +nightly fmt --all --check        # clean

cargo +nightly clippy --no-default-features --features "minimal,plugin-mikrotik" --lib --tests
# zero warnings in the changed files

cd docs && node scripts/check-content.mjs
# Documentation content checks passed.

On-device verification (RouterOS 7.24.5 + OxiDNS 1.6.0, throwaway lists and a
throwaway plugin tag; production untouched):

  • Pre-flight: IPv4 written bare, IPv6 written as /128. A second observation
    past 75% of the TTL refreshed the same .id in place, with 0
    already have such entry messages.
  • Shutdown-cleanup A/B (identical seed data, only the binary swapped): the
    unpatched build deleted the IPv4 row but left 3 /128 rows behind; this
    PR's build removed both families (0/0).
  • Production run (~10 minutes): ros_address_list_write_error_total 0, versus
    61 errors per 96 minutes before; 0 already have such entry log lines,
    versus one per 60 seconds before.

Platforms and assumptions not yet verified

  • cargo +nightly clippy --all-targets --all-features -- -D warnings (the
    official just check gate) fails under nightly
    1.101.0-nightly (2026-10-10), but both errors are in files this PR does
    not touch
    : src/infra/upgrade/mod.rs:287 (unused import) and
    src/infra/network/dial.rs:483 (needless_bool). They are left alone to keep
    the change focused; maintainers may want to check which nightly their CI
    pins.
  • The duplicate-insert recovery path (whole-list scan fallback) needs a real
    RouterOS or an equivalent fake peer for end-to-end coverage. The repository
    has no RouterOS fake, so this path is covered by the transport classifier
    unit test plus the on-device verification above.
  • On-device verification used RouterOS 7.24.5 and OxiDNS 1.6.0 (full, static
    musl). RouterOS 6.x, other bundle combinations and API-SSL (8729) were not
    verified.

Commit structure

Three commits, each independently compilable and testable:

  1. fix(ros_address_list): use the address form RouterOS stores per family — root cause, model.rs only
  2. fix(routeros): treat duplicate-insert rejection as an idempotent outcome — defence, transport.rs + api.rs
  3. docs(routeros): document idempotent duplicate-insert handling — Chinese and English docs

On the dm= comment field: dynamic entries moved from domain-level ownership
to IP/CIDR lease semantics in v1.5.0, and comment_codec_roundtrip asserts the
comment contains no dm=. This PR does not restore it — a single IP observed
from several domains would otherwise rewrite the comment on every observation
and trigger extra set calls.

RouterOS stores host address-list entries in a family-specific form and
compares the stored property with string equality. Measured on RouterOS
7.24.5:

| written            | stored            | ?address=<bare> | ?address=<masked> |
| ------------------ | ----------------- | --------------- | ----------------- |
| `198.51.100.99` or `.../32` | `198.51.100.99` | hit             | 0 rows            |
| `2001:db8::1` or `.../128`  | `2001:db8::1/128` | **0 rows**    | hit               |

`router_value()` stripped the mask for both families, so IPv6 writes landed
as `/128` while every later lookup asked for the bare address and found
nothing. The upsert then concluded the row was absent, `add` was rejected
with `failure: already have such entry`, and the resulting error dropped the
lease so the next observation repeated the whole cycle. IPv4 write and lookup
agreed, so the failure only ever came from AAAA records.

`delete_entry_if_matches` used the same bare-address lookup, so
`cleanup_on_shutdown` could not remove IPv6 rows either.

Emit the form RouterOS keeps for each family: bare address for IPv4 hosts,
`/128` for IPv6 hosts. Writes and lookups now agree, and rows written by an
older build become visible to this plugin again, so they can be refreshed and
cleaned up.
RouterOS has no upsert primitive and rejects a plain insert when the target
already holds the value. The trap carries the message
`failure: already have such entry` and no stable category (measured: absent),
so `is_duplicate_entry` classifies it by message, next to the existing
`is_missing_item`.

`add_entry` now returns `RouterOsResult<()>`. The previous `send_rows` path
flattened every trap into `DnsError`, which left no way to tell a duplicate
from a real failure.

`upsert_owned_entry` is split into `upsert_attempt` plus `insert_entry`, with
two lookup scopes: the existing exact-key query, and a whole-list scan matched
on normalized keys. When an insert is rejected as a duplicate, the upsert
retries once with the list scan. A row that belongs to this plugin goes
through the normal set and timeout-transition path; anything else resolves to
`Ok(None)`, meaning the key is occupied. That is neither an error nor a
retry, so `write_error_total` stays flat and the lease is not dropped.

The list-scan fallback covers rows stored in a form the filtered query cannot
report, rows hidden from filtered prints, and races with another writer. It
holds on its own: even if RouterOS reverts to bare IPv6 storage, a duplicate
insert stops being an error.

`list_entries` gains a `list_entries_for_list` helper that both the cold-path
scan and the recovery path share.
State in both plugin references that RouterOS rejects an insert when the live
list already holds the address, and that the plugin treats this as an
idempotent outcome: it rescans the target list, updates the row through the
normal ownership path when the row belongs to it, and otherwise leaves the key
to the existing entry.
@svenshi
svenshi changed the base branch from main to release/v1.7.0 October 12, 2026 02:54
@svenshi
svenshi changed the base branch from release/v1.7.0 to fix/ros_address_list October 12, 2026 03:10
@svenshi
svenshi merged commit 55c6984 into svenshi:fix/ros_address_list Oct 12, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants