feat: add categraf collect-config wizard with plugin templates and metric verification - #2224
Conversation
…tric verification
📝 WalkthroughWalkthroughAdds synchronized collector templates and a host-page collection wizard. The wizard supports component selection, structured or TOML configuration, command generation, installation handoff, localized UI text, and Prometheus-based metric arrival verification. ChangesCollector setup
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant HostList
participant CollectSetup
participant Categraf
participant Prometheus
HostList->>CollectSetup: open collection setup
CollectSetup->>Categraf: generate and run collect command
Categraf-->>CollectSetup: collect command completes
CollectSetup->>Prometheus: poll configured metric query
Prometheus-->>CollectSetup: return reporting idents
CollectSetup-->>HostList: show detected, waiting, or timeout state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (1)
src/pages/hosts/pages/List/CollectSetup/useMetricArrival.ts (1)
58-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRef mutated during render (react-doctor
no-ref-current-in-render).Assigning
datasourcesRef.current/identsRef.currentat render time is flagged by the static analyzer. Under this repo's React 17 (no concurrent rendering), render replay/discard isn't a concern, so this is functionally safe today. If you want to future-proof against a later React upgrade, move these assignments into auseEffect/useLayoutEffectinstead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/hosts/pages/List/CollectSetup/useMetricArrival.ts` around lines 58 - 64, Move the render-time assignments to datasourcesRef.current and identsRef.current into an appropriate useEffect or useLayoutEffect in the useMetricArrival hook. Keep datasourceKey and identsKey memoization unchanged, and ensure the polling logic still reads the latest datasource and ident values through the refs.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/n9e-collect-templates/elasticsearch.toml`:
- Around line 20-22: Comment out the username and password entries in the
Elasticsearch template so they are not active by default, preventing
authentication with the known password when only servers are configured. Require
users to explicitly uncomment and configure both credential fields.
In `@public/n9e-collect-templates/ethtool.toml`:
- Line 8: Remove the host-specific ens33 and ens38 values from interface_include
in the ethtool template; leave the example commented out or otherwise make
interface selection derive from the target host so collection does not depend on
fixed interface names.
In `@public/n9e-collect-templates/haproxy.toml`:
- Around line 9-10: Update the TLS settings in
public/n9e-collect-templates/haproxy.toml:9-10 by changing the active ssl_verify
default to true or removing it. In
public/n9e-collect-templates/influxdb.toml:29-36,
public/n9e-collect-templates/tomcat.toml:23-30, and
public/n9e-collect-templates/zookeeper.toml:12-19, change the example
insecure_skip_verify values to false.
In `@public/n9e-collect-templates/kafka.toml`:
- Around line 4-6: Update the comment immediately above the active [[instances]]
block to remove the stale instruction about uncommenting it and instead describe
that the plugin is enabled through this active configuration block.
In `@public/n9e-collect-templates/kubernetes.toml`:
- Around line 37-42: Update the Kubernetes template’s insecure_skip_verify
setting to be disabled by default by commenting it out or setting it to false,
while preserving explicit opt-in for users who require skipped TLS verification.
In `@public/n9e-collect-templates/mongodb.toml`:
- Around line 10-15: Remove the active username and password values from the
MongoDB template by commenting out the username and password entries, while
keeping the surrounding configuration guidance intact. Require users to
explicitly provide secure credentials rather than supplying usable defaults.
In `@public/n9e-collect-templates/nfsclient.toml`:
- Around line 7-25: Correct the user-facing template comments and resynchronize
generated templates: in public/n9e-collect-templates/nfsclient.toml lines 7-25
fix “explictly,” “Symantics,” and “mulitple”; in
public/n9e-collect-templates/nginx_upstream_check.toml lines 16-17 change “it's
is not set” to “it is not set”; in public/n9e-collect-templates/nvidia_smi.toml
lines 16-17 change “date collection” to “data collection”; and in
public/n9e-collect-templates/phpfpm.toml lines 4-6 describe PHP-FPM endpoints
rather than Nginx stub_status URIs. Then rerun the template synchronization
script.
In `@public/n9e-collect-templates/nvidia_smi.toml`:
- Around line 8-10: Update the commented nvidia_smi_command example to remove
insecure SSH options that disable host-key verification and discard known hosts.
Require strict host-key checking and configure a secure, persistent known-hosts
database instead.
In `@public/n9e-collect-templates/postgresql.toml`:
- Around line 42-49: Replace the Oracle-specific v$session example query in the
PostgreSQL template’s commented metrics block with a valid PostgreSQL query
using pg_stat_activity, preserving equivalent session status/type counts where
supported, or remove the example block entirely.
In `@public/n9e-collect-templates/tomcat.toml`:
- Around line 23-30: Update the commented insecure_skip_verify example in the
Optional TLS Config section to use false, so the template defaults to secure
certificate validation while retaining the option for an explicit true opt-out.
In `@public/n9e-collect-templates/zookeeper.toml`:
- Around line 12-19: Update the commented insecure_skip_verify example in the
Optional TLS Config section of zookeeper.toml from true to false, so the example
enables certificate validation by default while retaining the explicit opt-out
setting.
In `@scripts/sync_collect_templates.sh`:
- Around line 45-60: Make the synchronization flow in
scripts/sync_collect_templates.sh transactional: build and validate the complete
template set in a temporary staging directory, fail when no expected catalog
files are found or any copy fails, and only then replace OUT_DIR atomically. Do
not remove the existing published templates before validation succeeds; preserve
the excluded-name filtering and count reporting while ensuring failures cannot
leave a partial destination.
In `@src/pages/hosts/pages/List/CollectSetup/buildCommand.ts`:
- Around line 42-59: The collect commands expose reversible TOML credentials
through the --conf-b64 argv argument. Update buildCollectCommand and
buildManualCollectCommand, together with the corresponding script interface, to
transport the encoded configuration through an environment variable, stdin, or a
short-lived file rather than the command line, while preserving command
functionality and avoiding shell history/process-argument exposure.
In `@src/pages/hosts/pages/List/List.tsx`:
- Around line 993-1011: Expose the machines detected by InstallCategraf through
its detectedExtra handoff, preferably by providing the detected idents to the
render callback or an equivalent onDetected callback. In List.tsx, store those
idents when the install completes and pass them as CollectSetup’s defaultIdents
for the next_entry flow instead of the stale selectedIdents, while preserving
the existing close, refresh, and collect-opening behavior.
---
Nitpick comments:
In `@src/pages/hosts/pages/List/CollectSetup/useMetricArrival.ts`:
- Around line 58-64: Move the render-time assignments to datasourcesRef.current
and identsRef.current into an appropriate useEffect or useLayoutEffect in the
useMetricArrival hook. Keep datasourceKey and identsKey memoization unchanged,
and ensure the polling logic still reads the latest datasource and ident values
through the refs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4bdf7603-480a-4bc5-8c55-9e1bf458f7d9
📒 Files selected for processing (78)
public/n9e-collect-templates/apache.tomlpublic/n9e-collect-templates/arp_packet.tomlpublic/n9e-collect-templates/bind.tomlpublic/n9e-collect-templates/cadvisor.tomlpublic/n9e-collect-templates/chrony.tomlpublic/n9e-collect-templates/clickhouse.tomlpublic/n9e-collect-templates/consul.tomlpublic/n9e-collect-templates/dns_query.tomlpublic/n9e-collect-templates/docker.tomlpublic/n9e-collect-templates/elasticsearch.tomlpublic/n9e-collect-templates/ethtool.tomlpublic/n9e-collect-templates/exec.tomlpublic/n9e-collect-templates/filecount.tomlpublic/n9e-collect-templates/greenplum.tomlpublic/n9e-collect-templates/haproxy.tomlpublic/n9e-collect-templates/http_response.tomlpublic/n9e-collect-templates/influxdb.tomlpublic/n9e-collect-templates/iptables.tomlpublic/n9e-collect-templates/jenkins.tomlpublic/n9e-collect-templates/kafka.tomlpublic/n9e-collect-templates/keepalived.tomlpublic/n9e-collect-templates/kubernetes.tomlpublic/n9e-collect-templates/ldap.tomlpublic/n9e-collect-templates/logstash.tomlpublic/n9e-collect-templates/mongodb.tomlpublic/n9e-collect-templates/mtail.tomlpublic/n9e-collect-templates/mysql.tomlpublic/n9e-collect-templates/nats.tomlpublic/n9e-collect-templates/net_response.tomlpublic/n9e-collect-templates/netstat_filter.tomlpublic/n9e-collect-templates/nfsclient.tomlpublic/n9e-collect-templates/nginx.tomlpublic/n9e-collect-templates/nginx_upstream_check.tomlpublic/n9e-collect-templates/nsq.tomlpublic/n9e-collect-templates/ntp.tomlpublic/n9e-collect-templates/nvidia_smi.tomlpublic/n9e-collect-templates/phpfpm.tomlpublic/n9e-collect-templates/ping.tomlpublic/n9e-collect-templates/postgresql.tomlpublic/n9e-collect-templates/procstat.tomlpublic/n9e-collect-templates/prometheus.tomlpublic/n9e-collect-templates/rabbitmq.tomlpublic/n9e-collect-templates/redis.tomlpublic/n9e-collect-templates/redis_sentinel.tomlpublic/n9e-collect-templates/rocketmq_offset.tomlpublic/n9e-collect-templates/smart.tomlpublic/n9e-collect-templates/sqlserver.tomlpublic/n9e-collect-templates/supervisor.tomlpublic/n9e-collect-templates/systemd.tomlpublic/n9e-collect-templates/tengine.tomlpublic/n9e-collect-templates/tomcat.tomlpublic/n9e-collect-templates/traffic_server.tomlpublic/n9e-collect-templates/whois.tomlpublic/n9e-collect-templates/x509_cert.tomlpublic/n9e-collect-templates/zookeeper.tomlscripts/sync_collect_templates.shsrc/App.tsxsrc/pages/hosts/locale/en_US.tssrc/pages/hosts/locale/ja_JP.tssrc/pages/hosts/locale/ru_RU.tssrc/pages/hosts/locale/zh_CN.tssrc/pages/hosts/locale/zh_HK.tssrc/pages/hosts/pages/List/CollectSetup/ComponentPicker.tsxsrc/pages/hosts/pages/List/CollectSetup/buildArrivalPromql.test.tssrc/pages/hosts/pages/List/CollectSetup/buildArrivalPromql.tssrc/pages/hosts/pages/List/CollectSetup/buildCommand.test.tssrc/pages/hosts/pages/List/CollectSetup/buildCommand.tssrc/pages/hosts/pages/List/CollectSetup/buildToml.test.tssrc/pages/hosts/pages/List/CollectSetup/buildToml.tssrc/pages/hosts/pages/List/CollectSetup/catalog.tssrc/pages/hosts/pages/List/CollectSetup/index.tsxsrc/pages/hosts/pages/List/CollectSetup/useMetricArrival.tssrc/pages/hosts/pages/List/InstallCategraf/buildCommand.tssrc/pages/hosts/pages/List/InstallCategraf/index.tsxsrc/pages/hosts/pages/List/List.tsxsrc/pages/hosts/pages/List/components/CommandBlock.tsxsrc/pages/hosts/services.tssrc/pages/hosts/types.ts
| ## HTTP Basic Authentication username and password. | ||
| username = "elastic" | ||
| password = "password" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not ship an active known password in the template.
If a user only fills servers, the collector will attempt authentication with the public, predictable password password. Keep both credential fields commented out and require explicit configuration.
Suggested fix
-username = "elastic"
-password = "password"
+# username = "elastic"
+# password = ""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## HTTP Basic Authentication username and password. | |
| username = "elastic" | |
| password = "password" | |
| ## HTTP Basic Authentication username and password. | |
| # username = "elastic" | |
| # password = "" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@public/n9e-collect-templates/elasticsearch.toml` around lines 20 - 22,
Comment out the username and password entries in the Elasticsearch template so
they are not active by default, preventing authentication with the known
password when only servers are configured. Require users to explicitly uncomment
and configure both credential fields.
| [[instances]] | ||
| ## List of interfaces to pull metrics for | ||
| # interface_include = ["eth0"] | ||
| interface_include = ["ens33","ens38"] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the host-specific active interface list.
ens33 and ens38 are environment-specific names. On other hosts this template will silently collect no interfaces, so keep the example commented or generate the list from the target host.
Suggested fix
-interface_include = ["ens33","ens38"]
+# interface_include = ["ens33", "ens38"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface_include = ["ens33","ens38"] | |
| # interface_include = ["ens33", "ens38"] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@public/n9e-collect-templates/ethtool.toml` at line 8, Remove the
host-specific ens33 and ens38 values from interface_include in the ethtool
template; leave the example commented out or otherwise make interface selection
derive from the target host so collection does not depend on fixed interface
names.
| # Flag that enables SSL certificate verification for the scrape URI | ||
| ssl_verify = false |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files 'public/n9e-collect-templates/*.toml' | sed -n '1,120p'
echo
echo "Target TLS settings:"
for f in haproxy influxdb tomcat zookeeper; do
echo "--- ${f}.toml ---"
sed -n '1,45p' "public/n9e-collect-templates/${f}.toml"
done
echo
echo "Search for disable/ insecure / ssl_verify settings:"
rg -n "ssl_verify|insecure_skip_verify|use_tls|tls_min_version|tls_ca|tls_cert|tls_key" public/n9e-collect-templates -S || trueRepository: n9e/fe
Length of output: 1901
Use secure TLS verification defaults across the collector templates.
public/n9e-collect-templates/haproxy.toml: change activessl_verify = falsetotrueor omit it so TLS is verified by default.public/n9e-collect-templates/influxdb.toml,tomcat.toml,zookeeper.toml: change the exampleinsecure_skip_verifyvalues tofalseto avoid exposing a skip-certification setup.
📍 Affects 4 files
public/n9e-collect-templates/haproxy.toml#L9-L10(this comment)public/n9e-collect-templates/influxdb.toml#L29-L36public/n9e-collect-templates/tomcat.toml#L23-L30public/n9e-collect-templates/zookeeper.toml#L12-L19
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@public/n9e-collect-templates/haproxy.toml` around lines 9 - 10, Update the
TLS settings in public/n9e-collect-templates/haproxy.toml:9-10 by changing the
active ssl_verify default to true or removing it. In
public/n9e-collect-templates/influxdb.toml:29-36,
public/n9e-collect-templates/tomcat.toml:23-30, and
public/n9e-collect-templates/zookeeper.toml:12-19, change the example
insecure_skip_verify values to false.
| ############################################################################ | ||
| # !!! uncomment [[instances]] to enable this plugin | ||
| [[instances]] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the stale “uncomment instances” instruction.
[[instances]] is already active on Line 6, so Line 5 contradicts the actual template and may make users think the plugin is disabled. Update the comment to describe the active block.
Suggested change
-# !!! uncomment [[instances]] to enable this plugin
+# Configure the active instance below to enable this plugin.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ############################################################################ | |
| # !!! uncomment [[instances]] to enable this plugin | |
| [[instances]] | |
| ############################################################################ | |
| # Configure the active instance below to enable this plugin. | |
| [[instances]] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@public/n9e-collect-templates/kafka.toml` around lines 4 - 6, Update the
comment immediately above the active [[instances]] block to remove the stale
instruction about uncommenting it and instead describe that the plugin is
enabled through this active configuration block.
| use_tls = true | ||
| # tls_ca = "/etc/categraf/ca.pem" | ||
| # tls_cert = "/etc/categraf/cert.pem" | ||
| # tls_key = "/etc/categraf/key.pem" | ||
| ## Use TLS but skip chain & host verification | ||
| insecure_skip_verify = true No newline at end of file |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not disable kubelet TLS verification by default.
insecure_skip_verify = true makes the generated kubelet connection accept untrusted certificates and hostnames. The Categraf client applies this TLS configuration to its HTTP transport, so users who only fill in url are exposed to man-in-the-middle risk. (raw.githubusercontent.com)
Comment this option out or set it to false; users should explicitly opt in when required.
Suggested change
use_tls = true
# tls_ca = "/etc/categraf/ca.pem"
# tls_cert = "/etc/categraf/cert.pem"
# tls_key = "/etc/categraf/key.pem"
## Use TLS but skip chain & host verification
-insecure_skip_verify = true
+# insecure_skip_verify = true#!/bin/sh
set -eu
curl -fsSL https://raw.githubusercontent.com/flashcatcloud/categraf/main/inputs/kubernetes/kubernetes.go |
grep -n -A12 'TLSConfig()'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@public/n9e-collect-templates/kubernetes.toml` around lines 37 - 42, Update
the Kubernetes template’s insecure_skip_verify setting to be disabled by default
by commenting it out or setting it to false, while preserving explicit opt-in
for users who require skipped TLS verification.
| ## Optional TLS Config | ||
| # use_tls = false | ||
| # tls_min_version = "1.2" | ||
| # tls_ca = "/etc/categraf/ca.pem" | ||
| # tls_cert = "/etc/categraf/cert.pem" | ||
| # tls_key = "/etc/categraf/key.pem" | ||
| ## Use TLS but skip chain & host verification | ||
| # insecure_skip_verify = true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Locate tomcat.toml"
fd -a 'tomcat\.toml$' . || true
echo
echo "## Relevant snippet"
if [ -f public/n9e-collect-templates/tomcat.toml ]; then
nl -ba public/n9e-collect-templates/tomcat.toml | sed -n '1,80p'
fi
echo
echo "## Similar insecure_skip_verify examples"
rg -n "insecure_skip_verify.*true|use_tls\s*=|tls_" -S . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' | head -200Repository: n9e/fe
Length of output: 1901
Use secure TLS verification in the example.
Change the commented insecure_skip_verify example to false; disabling certificate validation should be an explicit opt-out, not the copy-paste value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@public/n9e-collect-templates/tomcat.toml` around lines 23 - 30, Update the
commented insecure_skip_verify example in the Optional TLS Config section to use
false, so the template defaults to secure certificate validation while retaining
the option for an explicit true opt-out.
| ## Optional TLS Config | ||
| # use_tls = false | ||
| # tls_min_version = "1.2" | ||
| # tls_ca = "/etc/categraf/ca.pem" | ||
| # tls_cert = "/etc/categraf/cert.pem" | ||
| # tls_key = "/etc/categraf/key.pem" | ||
| ## Use TLS but skip chain & host verification | ||
| # insecure_skip_verify = true No newline at end of file |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'zookeeper\.toml$' . || true
echo "== locate TLS settings in zookeeper templates =="
if [ -f public/n9e-collect-templates/zookeeper.toml ]; then
nl -ba public/n9e-collect-templates/zookeeper.toml | sed -n '1,80p'
fi
echo "== search insecure_skip_verify occurrences =="
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' 'insecure_skip_verify|use_tls|tls_ca|tls_cert|tls_key' . || true
echo "== git diff stat/name =="
git diff --stat || true
git diff -- public/n9e-collect-templates/zookeeper.toml || trueRepository: n9e/fe
Length of output: 1901
Use secure TLS verification in the example.
Change the commented insecure_skip_verify example to false; disabling certificate validation should be an explicit opt-out.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@public/n9e-collect-templates/zookeeper.toml` around lines 12 - 19, Update the
commented insecure_skip_verify example in the Optional TLS Config section of
zookeeper.toml from true to false, so the example enables certificate validation
by default while retaining the explicit opt-out setting.
| mkdir -p "$OUT_DIR" | ||
| # Full resync: a plugin dropped upstream must not linger here as a stale file. | ||
| rm -f "$OUT_DIR"/*.toml | ||
|
|
||
| count=0 | ||
| for dir in "$CATEGRAF_DIR"/conf/input.*/; do | ||
| name="$(basename "$dir")" | ||
| name="${name#input.}" | ||
| excluded "$name" && continue | ||
| src="$dir$name.toml" | ||
| if [ ! -f "$src" ]; then | ||
| echo "skip $name: no $name.toml in $dir" >&2 | ||
| continue | ||
| fi | ||
| cp "$src" "$OUT_DIR/$name.toml" | ||
| count=$((count + 1)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make template synchronization transactional and fail on an empty source.
The script deletes the published templates before validating or completing the copy. An empty/incompatible checkout can therefore remove all templates and exit with synced 0, while a later cp failure leaves a partial directory. Since the wizard fetches these files by catalog name, this can cause runtime template-load failures.
Stage the complete output in a temporary directory, validate the expected catalog files, then replace the destination only after success.
Suggested approach
+stage_dir="$(mktemp -d "${OUT_DIR}.staging.XXXXXX")"
+trap 'rm -rf "$stage_dir"' EXIT
-rm -f "$OUT_DIR"/*.toml
...
-cp "$src" "$OUT_DIR/$name.toml"
+cp "$src" "$stage_dir/$name.toml"
...
+if [ "$count" -eq 0 ]; then
+ echo "no templates found in $CATEGRAF_DIR" >&2
+ exit 1
+fi
+rm -rf "$OUT_DIR"
+mv "$stage_dir" "$OUT_DIR"
+trap - EXIT📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mkdir -p "$OUT_DIR" | |
| # Full resync: a plugin dropped upstream must not linger here as a stale file. | |
| rm -f "$OUT_DIR"/*.toml | |
| count=0 | |
| for dir in "$CATEGRAF_DIR"/conf/input.*/; do | |
| name="$(basename "$dir")" | |
| name="${name#input.}" | |
| excluded "$name" && continue | |
| src="$dir$name.toml" | |
| if [ ! -f "$src" ]; then | |
| echo "skip $name: no $name.toml in $dir" >&2 | |
| continue | |
| fi | |
| cp "$src" "$OUT_DIR/$name.toml" | |
| count=$((count + 1)) | |
| stage_dir="$(mktemp -d "${OUT_DIR}.staging.XXXXXX")" | |
| trap 'rm -rf "$stage_dir"' EXIT | |
| mkdir -p "$OUT_DIR" | |
| # Full resync: a plugin dropped upstream must not linger here as a stale file. | |
| rm -f "$OUT_DIR"/*.toml | |
| count=0 | |
| for dir in "$CATEGRAF_DIR"/conf/input.*/; do | |
| name="$(basename "$dir")" | |
| name="${name#input.}" | |
| excluded "$name" && continue | |
| src="$dir$name.toml" | |
| if [ ! -f "$src" ]; then | |
| echo "skip $name: no $name.toml in $dir" >&2 | |
| continue | |
| fi | |
| cp "$src" "$stage_dir/$name.toml" | |
| count=$((count + 1)) | |
| done | |
| if [ "$count" -eq 0 ]; then | |
| echo "no templates found in $CATEGRAF_DIR" >&2 | |
| exit 1 | |
| fi | |
| rm -rf "$OUT_DIR" | |
| mv "$stage_dir" "$OUT_DIR" | |
| trap - EXIT |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/sync_collect_templates.sh` around lines 45 - 60, Make the
synchronization flow in scripts/sync_collect_templates.sh transactional: build
and validate the complete template set in a temporary staging directory, fail
when no expected catalog files are found or any copy fails, and only then
replace OUT_DIR atomically. Do not remove the existing published templates
before validation succeeds; preserve the excluded-name filtering and count
reporting while ensuring failures cannot leave a partial destination.
| export function buildCollectCommand(options: BuildCollectCommandOptions): string { | ||
| const addr = normalizeServerAddr(options.serverAddr); | ||
| if (!addr || !_.trim(options.toml)) return ''; | ||
| const conf = shellQuote(toBase64(options.toml)); | ||
| return `curl -sSfL${buildCurlAuth(options)} ${shellQuote(`${addr}${COLLECT_SCRIPT_PATH}`)} | sudo bash -s -- --input ${shellQuote(options.input)} --conf-b64 ${conf}`; | ||
| } | ||
|
|
||
| /** 不放心直接 pipe 到 bash 的用户,可以先下载审阅再执行 */ | ||
| export function buildManualCollectCommand(options: BuildCollectCommandOptions): string { | ||
| const addr = normalizeServerAddr(options.serverAddr); | ||
| if (!addr || !_.trim(options.toml)) return ''; | ||
| const conf = shellQuote(toBase64(options.toml)); | ||
| return [ | ||
| `curl -sSfL${buildCurlAuth(options)} ${shellQuote(`${addr}${COLLECT_SCRIPT_PATH}`)} -o collect-config.sh`, | ||
| 'less collect-config.sh', | ||
| `sudo bash collect-config.sh --input ${shellQuote(options.input)} --conf-b64 ${conf}`, | ||
| ].join('\n'); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Base64-encoded TOML (with credentials) is embedded in argv — visible via ps//proc/shell history.
buildCollectCommand and buildManualCollectCommand both pass the full plugin config as --conf-b64 <payload> on the command line. Base64 is reversible in one step, so this doesn't achieve the "passwords aren't nakedly in the command" goal stated in the comment at Line 9 — any local user (or anything scraping ps aux//proc/<pid>/cmdline while the install runs) can recover DB/service credentials that may be present in the TOML (mysql/redis/postgresql passwords, kafka sasl_password, tomcat/rabbitmq passwords, etc., per catalog.ts).
Consider transporting the config via an env var (not shown by default ps) or having the script read it from stdin/a short-lived file instead of argv.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/hosts/pages/List/CollectSetup/buildCommand.ts` around lines 42 -
59, The collect commands expose reversible TOML credentials through the
--conf-b64 argv argument. Update buildCollectCommand and
buildManualCollectCommand, together with the corresponding script interface, to
transport the encoded configuration through an environment variable, stdin, or a
short-lived file rather than the command line, while preserving command
functionality and avoiding shell history/process-argument exposure.
| detectedExtra={ | ||
| installMeta.collect ? ( | ||
| <Button | ||
| size='small' | ||
| type='primary' | ||
| onClick={() => { | ||
| // 承接安装引导:机器上报后顺手进入采集配置,安装弹窗关闭时正常走刷新逻辑 | ||
| setInstallVisible(false); | ||
| setRefreshFlag(_.uniqueId('refreshFlag_')); | ||
| setCollectVisible(true); | ||
| }} | ||
| > | ||
| {t('collect.next_entry')} | ||
| </Button> | ||
| ) : undefined | ||
| } | ||
| /> | ||
| )} | ||
| {collectVisible && installMeta?.collect && <CollectSetup meta={installMeta} defaultIdents={selectedIdents} onClose={() => setCollectVisible(false)} />} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
CollectSetup's defaultIdents doesn't carry forward the newly detected machines.
The "next_entry" CTA (Line 999 comment: seamlessly continue from install to collect-config) closes InstallCategraf and opens CollectSetup, but defaultIdents={selectedIdents} at Line 1011 uses the host table's pre-existing checkbox selection — not the machine(s) InstallCategraf's useTargetArrival() just detected (newIdents, which never leaves InstallCategraf). The newly-installed host the user is trying to configure won't be pre-selected in step 3's target picker; the user has to find and select it manually, defeating the stated purpose of this handoff.
Consider exposing the detected idents from InstallCategraf (e.g. change detectedExtra to a render-prop (idents: string[]) => React.ReactNode, or add an onDetected: (idents: string[]) => void callback) so List.tsx can thread them into CollectSetup's defaultIdents instead of selectedIdents.
🐛 Sketch of the fix
interface Props {
meta: CategrafInstallMeta;
/** detected 表示期间确实有新机器上报,父级据此决定是否刷新列表 */
onClose: (detected: boolean) => void;
/** 检测到新机器后展示的追加动作(如「下一步:配置采集」CTA) */
- detectedExtra?: React.ReactNode;
+ detectedExtra?: (newIdents: string[]) => React.ReactNode;
}- description={
- <>
- <div>{t('install.detected_next')}</div>
- {detectedExtra ? <div className='mt-2'>{detectedExtra}</div> : null}
- </>
- }
+ description={
+ <>
+ <div>{t('install.detected_next')}</div>
+ {detectedExtra ? <div className='mt-2'>{detectedExtra(newIdents)}</div> : null}
+ </>
+ }- detectedExtra={
- installMeta.collect ? (
+ detectedExtra={
+ installMeta.collect
+ ? (newIdents) => (
<Button
size='small'
type='primary'
onClick={() => {
setInstallVisible(false);
setRefreshFlag(_.uniqueId('refreshFlag_'));
+ setDetectedIdents(newIdents);
setCollectVisible(true);
}}
>
{t('collect.next_entry')}
</Button>
- ) : undefined
+ )
+ : undefined
}- {collectVisible && installMeta?.collect && <CollectSetup meta={installMeta} defaultIdents={selectedIdents} onClose={() => setCollectVisible(false)} />}
+ {collectVisible && installMeta?.collect && (
+ <CollectSetup meta={installMeta} defaultIdents={detectedIdents.length > 0 ? detectedIdents : selectedIdents} onClose={() => setCollectVisible(false)} />
+ )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/hosts/pages/List/List.tsx` around lines 993 - 1011, Expose the
machines detected by InstallCategraf through its detectedExtra handoff,
preferably by providing the detected idents to the render callback or an
equivalent onDetected callback. In List.tsx, store those idents when the install
completes and pass them as CollectSetup’s defaultIdents for the next_entry flow
instead of the stale selectedIdents, while preserving the existing close,
refresh, and collect-opening behavior.
…erride in collect verify
…ponsive icon-only labels
Summary by CodeRabbit