fix(identity): normalise the endpoint list in get_identity - #28
Conversation
get_identity interpolated $scfg->{lb_api_host} verbatim. That option is a
free-form, comma-separated list of management nodes, so the same cluster
is routinely written differently on different Proxmox nodes: a different
node order, padding whitespace, or a hostname in another case. Each
spelling produced a different identity, so PVE could not recognise the
entries as the same backend - exactly what the storage API 14 method
exists to do.
Parse the list, lowercase each endpoint (hostnames and IP literals are
case-insensitive) and sort it, so the identity depends on the set of
endpoints rather than on how the list happened to be typed.
Two entries listing a genuinely different subset of the cluster's nodes
still differ. The fix for that would be to ask the cluster for its own
UUID, which is deliberately not done here: get_identity must stay a pure,
non-failing function of the config, and an identity that changed whenever
the API was unreachable would be worse than one that is conservative.
Signed-off-by: Ron <ron@lightbitslabs.com>
📝 WalkthroughWalkthrough
ChangesIdentity normalization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant StorageConfig
participant PVEStorage
participant LightbitsPlugin
participant E2ETest
StorageConfig->>PVEStorage: configure main and derived storages
E2ETest->>PVEStorage: query storage identities
PVEStorage->>LightbitsPlugin: call get_identity
LightbitsPlugin->>LightbitsPlugin: canonicalize and sort endpoints
LightbitsPlugin-->>PVEStorage: return canonical identity
PVEStorage-->>E2ETest: return identities
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@LightbitsPlugin.pm`:
- Around line 421-422: Update the endpoint construction around _api_endpoints
and the returned lightbits:// identity to normalize bare hosts, including
bracketed IPv6 literals, to an explicit :443 before sorting; preserve existing
explicit ports and ensure equivalent host forms produce the same identity. Add a
regression test covering bare and :443 host inputs.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8d7e2c9-a48d-4c15-aaf6-4ee40810e6a9
📒 Files selected for processing (3)
CHANGELOG.mdLightbitsPlugin.pmt/api_version.t
Addresses CodeRabbit review on PR #28. lb_api_host accepts a bare host, and _api always builds an https:// URL, so "10.0.0.1" and "10.0.0.1:443" address exactly the same endpoint. Only lowercasing and sorting still gave those two spellings different identities, which is the same class of bug this change set out to fix. Canonicalise each entry to an explicit port before sorting. A genuinely different port is preserved, so a non-default port still yields a distinct identity. Bracketed IPv6 literals are handled the same way, matching the convention _nvme_endpoints already uses. The canonical form is used only to build the identity string, never a request URL, so it cannot change what the plugin connects to. Signed-off-by: Ron <ron@lightbitslabs.com>
Signed-off-by: Lightbits Labs <165641972+roiyz-lb@users.noreply.github.com>
Registers a second storage entry for the same cluster spelled differently (order reversed, case changed, one :443 left implicit) plus a third entry with a different project, and asserts through the real PVE storage stack that spelling never changes the identity, the identity is canonical (lowercase, explicit ports, sorted), and a genuinely different backend keeps a distinct one. No volumes are created. Signed-off-by: Lightbits Labs <165641972+roiyz-lb@users.noreply.github.com>
Live e2e + coverage report (2026-07-29)Environment: nested PVE 9.2.5 (rack08) against a 3-node LightOS 3.20.1 cluster, reverted to a clean snapshot, this branch's merge with current main ( Full regression suite on this build
New:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@t/e2e/get_identity.sh`:
- Around line 71-84: The TWIN_HOSTS transformation must always change one
endpoint’s default-port spelling, not merely reorder or uppercase it. Update the
Python logic to toggle bare host/IPv6 endpoints and explicit :443 endpoints in
both hostname and bracketed-IPv6 forms; if no endpoint can be toggled, abort
instead of producing an invalid twin.
- Around line 45-52: Update scfg_val to return the complete value for the
requested storage property rather than only the second whitespace-delimited
field, preserving values containing spaces and multiple endpoints such as
lb_api_host. Keep the existing lightbits storage-block matching and default
PROJECT behavior unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ceef7dd6-413a-478e-9442-f3258eaba121
📒 Files selected for processing (1)
t/e2e/get_identity.sh
| scfg_val() { | ||
| awk -v s="$1" -v k="$2" ' | ||
| /^[a-z]+: / { in_blk = ($0 == "lightbits: " s) ; next } | ||
| in_blk && $1 == k { print $2; exit }' /etc/pve/storage.cfg | ||
| } | ||
| API_HOSTS="$(scfg_val "$STORAGE" lb_api_host)" | ||
| JWT="$(scfg_val "$STORAGE" lb_jwt)" | ||
| PROJECT="$(scfg_val "$STORAGE" lb_project)"; PROJECT="${PROJECT:-default}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve the full storage property value.
scfg_val prints only $2; an lb_api_host such as node1:443, node2:443 becomes node1:443,. The main storage retains both endpoints while the twin has one, causing a false e2e failure for valid whitespace-formatted configuration.
Proposed fix
- in_blk && $1 == k { print $2; exit }' /etc/pve/storage.cfg
+ in_blk && $1 == k {
+ sub(/^[[:space:]]*[^[:space:]]+[[:space:]]+/, "")
+ print
+ exit
+ }' /etc/pve/storage.cfg📝 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.
| scfg_val() { | |
| awk -v s="$1" -v k="$2" ' | |
| /^[a-z]+: / { in_blk = ($0 == "lightbits: " s) ; next } | |
| in_blk && $1 == k { print $2; exit }' /etc/pve/storage.cfg | |
| } | |
| API_HOSTS="$(scfg_val "$STORAGE" lb_api_host)" | |
| JWT="$(scfg_val "$STORAGE" lb_jwt)" | |
| PROJECT="$(scfg_val "$STORAGE" lb_project)"; PROJECT="${PROJECT:-default}" | |
| scfg_val() { | |
| awk -v s="$1" -v k="$2" ' | |
| /^[a-z]+: / { in_blk = ($0 == "lightbits: " s) ; next } | |
| in_blk && $1 == k { | |
| sub(/^[[:space:]]*[^[:space:]]+[[:space:]]+/, "") | |
| exit | |
| }' /etc/pve/storage.cfg | |
| } | |
| API_HOSTS="$(scfg_val "$STORAGE" lb_api_host)" | |
| JWT="$(scfg_val "$STORAGE" lb_jwt)" | |
| PROJECT="$(scfg_val "$STORAGE" lb_project)"; PROJECT="${PROJECT:-default}" |
🤖 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 `@t/e2e/get_identity.sh` around lines 45 - 52, Update scfg_val to return the
complete value for the requested storage property rather than only the second
whitespace-delimited field, preserving values containing spaces and multiple
endpoints such as lb_api_host. Keep the existing lightbits storage-block
matching and default PROJECT behavior unchanged.
| # A different spelling of the same endpoint set: reverse the order, uppercase, | ||
| # and drop one explicit :443 (the plugin always speaks HTTPS, so a bare host | ||
| # and host:443 are the same endpoint). | ||
| TWIN_HOSTS="$(python3 - "$API_HOSTS" <<'EOF' | ||
| import sys | ||
| eps = [e.strip() for e in sys.argv[1].split(",") if e.strip()] | ||
| eps.reverse() | ||
| eps = [e.upper() for e in eps] | ||
| for i, e in enumerate(eps): | ||
| if e.endswith(":443") and not e.startswith("["): | ||
| eps[i] = e[: -len(":443")] | ||
| break | ||
| print(",".join(eps)) | ||
| EOF |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guarantee that the twin changes default-port spelling.
The loop removes :443 only for non-bracketed endpoints already using that port. A source configuration with bare hosts or IPv6 endpoints changes only ordering/casing, yet the test still claims explicit-versus-implicit port coverage. Toggle a bare/:443 endpoint in both hostname and bracketed-IPv6 forms, or abort when no default-port endpoint can be transformed.
🤖 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 `@t/e2e/get_identity.sh` around lines 71 - 84, The TWIN_HOSTS transformation
must always change one endpoint’s default-port spelling, not merely reorder or
uppercase it. Update the Python logic to toggle bare host/IPv6 endpoints and
explicit :443 endpoints in both hostname and bracketed-IPv6 forms; if no
endpoint can be toggled, abort instead of producing an invalid twin.
2 participants
Description
get_identityinterpolated$scfg->{lb_api_host}verbatim:lb_api_hostis a free-form, comma-separated list of management nodes, so the same cluster is routinely written differently on different Proxmox nodes — a different node order, padding whitespace, or a hostname in another case. Each spelling produced a different identity:That defeats the purpose of the storage API 14 method, which exists so PVE can recognise two storage entries as the same backend.
Fix
Parse the list, lowercase each endpoint (hostnames and IP literals are case-insensitive), and sort — so the identity depends on the set of endpoints rather than on how the list happened to be typed. Reuses the existing
_api_endpointsparser, so whitespace and empty elements are handled the same way as everywhere else.Known limitation, deliberately not fixed here
Two entries listing a genuinely different subset of the cluster's nodes still produce different identities. The complete fix is to key off the cluster's own UUID, which I chose not to do:
get_identitymust stay a pure, non-failing function of the config, and an identity that changed depending on whether the API happened to be reachable would be worse than one that is merely conservative. Documented in a comment above the method.Type of change
Testing
perl -c LightbitsPlugin.pmpasses (and the taint-mode check)shellcheck scripts/install.sh scripts/uninstall.shpasses (no shell files touched)Test notes:
Extended
t/api_version.t, which already coveredget_identity. New cases assert that four different spellings of the same three-node cluster (as-written, reordered, whitespace-padded, stray empty element) collapse to one identity; that hostnames are lowercased; that genuinely different clusters and different projects still do not collide; and that the method neither dies nor calls the API whenlb_api_hostis unset. The two pre-existing assertions are unchanged and still pass. Full suite green (22 files, 245 assertions).Pure config-string manipulation with no runtime dependency, so there is little for a hardware run to add here beyond confirming existing storages still load.
DCO
By submitting this pull request I certify that my contribution is made under the terms of the Developer Certificate of Origin and that each commit includes a
Signed-off-byline (git commit -s).Summary by CodeRabbit
:443when omitted, and applies consistent IPv6 bracket/port handling).