firewall: T9076: document remote-group update interval - #2148
firewall: T9076: document remote-group update interval#2148BradKollmyer wants to merge 3 commits into
Conversation
Document the new 'set firewall group remote-group <name> interval' command: per-group update interval from 60 seconds to 4 weeks, given as plain seconds or with s/m/h/d/w suffixes (e.g. 4h), falling back to the global resolver-interval when unset. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
|
| Layer / File(s) | Summary |
|---|---|
Document remote group settings docs/configuration/firewall/groups.md:46-83 |
Documents per-group or global resolver intervals, persistent remote-list caching, URL configuration and query-string quoting, redacted download-failure logging, verification commands, and supported interval formats. |
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | firewall: T9076: document remote-group update interval matches the main docs change in docs/configuration/firewall/groups.md. |
| Description check | ✅ Passed | The description accurately covers the new remote-group interval command, range, suffixes, and fallback behavior. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
✨ Simplify code
- Create PR with simplified code
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 @coderabbitai help to get the list of available commands.
|
Claude finished @BradKollmyer's task in 3m 12s —— View job VyOS Documentation Review — PR #2148
IssuesNone confirmed as factually wrong at ERROR level given the branch-pin caveat below. Needs Verification
StatsValidated 1 MyST file. Skipped 0 RST files awaiting MyST migration. Files reviewed: 1. Commands checked: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ```{cfgcmd} set firewall group remote-group \<name\> interval \<interval\> | ||
|
|
||
| Set the update interval for this remote group, from 60 seconds to 4 weeks. | ||
| The interval can be given in seconds (e.g. `300`) or with the time unit | ||
| suffixes `s`, `m`, `h`, `d` or `w` (e.g. `4h`). If not set, the group | ||
| follows the global `firewall global-options resolver-interval`. | ||
| ``` |
There was a problem hiding this comment.
WARNING — cannot verify this command against the pinned vyos-1x source
Doc says: set firewall group remote-group <name> interval <interval> accepts 60s–4w with s/m/h/d/w suffixes and falls back to firewall global-options resolver-interval when unset.
Source (.vyos-1x/current):
interface-definitions/firewall.xml.in:141-153— theremote-grouptagNode declares only two children:url(viainclude/url-http-https.xml.i) anddescription(viainclude/generic-description.xml.i). NointervalleafNode exists underremote-group.include/firewall/global-options.xml.i:242-254—resolver-intervalisu32:10-3600(seconds, no suffixes),defaultValue 300.src/services/vyos-domain-resolver:74-79, 307-315— the resolver uses a single globaltimeoutderived fromresolver_intervaland callstime.sleep(timeout)in one loop; there is no per-remote_groupinterval read or scheduling.- No T9076 reference is present in
.vyos-1x/current.
Branch: rolling (vyos-1x: current)
If T9076 has already landed in vyos-1x on a commit newer than this repo's pin, this is a false positive — please share the vyos-1x commit / PR that introduces the node so we can confirm:
- exact CLI path (
intervalvs. e.g.update-interval) - validator range (is
60s–4w==60–2419200?) - accepted formats (are
s/m/h/d/wsuffixes actually parsed, or is it plainu32seconds?) - whether an XML
<defaultValue>exists, or fallback is implemented only in code
If T9076 has not landed yet, please hold the doc until it does, so the values above can be pulled straight from the XML / conf_mode source.
There was a problem hiding this comment.
The interval node is added by vyos/vyos-1x#5327 (T9076), which this docs PR accompanies — it isn't in the pinned vyos-1x commit yet. The PRs are cross-linked and intended to merge together.
Warn that remote-group URLs with query strings must be quoted (unquoted '&' silently truncates the value; literal '?' may need Ctrl-V, see T9077) and that vyos-domain-resolver logs redact the query string on download failures (T8829). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @BradKollmyer's task in 2m 15s —— View job VyOS Documentation Review — PR #2148 (synchronize)
IssuesNone confirmed as factually wrong at ERROR level given the branch-pin caveat below. Needs Verification
StatsValidated 1 MyST file. Skipped 0 RST files awaiting MyST migration. Files reviewed: 1. Commands checked: |
| ::::{note} | ||
| When a download fails, `vyos-domain-resolver` logs a redacted URL with the | ||
| query string removed, keeping API keys out of the system log. A logged URL | ||
| without its query parameters does not mean the configured value is wrong — | ||
| verify it with `show firewall group remote-group <name> url` instead. | ||
| :::: |
There was a problem hiding this comment.
WARNING — the log-redaction claim cannot be verified against the pinned vyos-1x source
Doc says: "When a download fails, vyos-domain-resolver logs a redacted URL with the query string removed…"
Source (.vyos-1x/src/services/vyos-domain-resolver:158-160, pin 80e17646):
except:
logger.error(f'Failed to download list-file for {set_name} remote group')
logger.info(f'Using cached list-file for {set_name} remote group')On download failure the resolver logs only the set name, not the URL at all — so there is nothing to redact in this branch. There is no urlparse / query-stripping code path in the file.
Branch: rolling (vyos-1x: current)
Same branch-pin caveat as the interval finding: if T9076 (vyos/vyos-1x#5327) adds the redaction, please link the vyos-1x commit that introduces it so the wording ("query string removed") can be verified. Otherwise consider softening to something like "the domain resolver logs failures by remote-group name only; use show firewall group remote-group <name> url to view the configured URL" until the redaction actually lands.
There was a problem hiding this comment.
The redaction is in vyos-1x upstream already, just newer than the pinned commit: vyos/vyos-1x@516eccd41 (remote: T8829: fall back to GET when HEAD is not supported, merged via vyos/vyos-1x#5275) added the redacted-URL logging to vyos-domain-resolver — on download failure it logs the URL with userinfo, query string and fragment stripped. Related: vyos/vyos-1x@e51e53bf3 (T4732) does the same for upload errors in vyos/remote.py.
|
@LiudmylaNad could you look at the PR? Thank you. |
LiudmylaNad
left a comment
LiudmylaNad
left a comment
There was a problem hiding this comment.
@alexandr-san4ez, the description looks good to me. I only added a small suggestion about combined suffixes
…ined Address review feedback: document that s/m/h/d/w units can be stacked (e.g. 1h30m), matching human_to_seconds and the CLI regex.
|
@LiudmylaNad thanks — applied your combined-suffixes note. The interval section now states that units may be stacked (e.g. |
3 participants
Change Summary
Document the new
set firewall group remote-group <name> intervalcommand: per-group update interval from 60 seconds to 4 weeks, given as plain seconds or with s/m/h/d/w suffixes (e.g.4h), falling back to the globalresolver-intervalwhen unset.Related Task(s)
Related PR(s)
Backport
Not applicable — rolling-only feature.
Checklist:
🤖 Generated with Claude Code