[management] Read reverse-proxy service and target columns in Postgres path - #6886
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe SQL store now reloads service restrictions, renewal metadata, and comprehensive target options, including scalar and JSON fields. Explicit PGX column lists are checked against GORM model columns, and roundtrip tests verify restored values across supported database engines. ChangesSQL service loading
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RoundtripTest
participant GetAccount
participant getServices
participant SQLDatabase
RoundtripTest->>GetAccount: request persisted account
GetAccount->>getServices: load services and targets
getServices->>SQLDatabase: query service metadata
SQLDatabase-->>getServices: service rows
getServices->>SQLDatabase: query target option columns
SQLDatabase-->>getServices: target rows
getServices-->>GetAccount: hydrated account
GetAccount-->>RoundtripTest: restored restrictions and target options
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
management/server/store/sql_store_service_test.go (1)
91-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
Middlewaresto the roundtrip coverage.
Options.Middlewaresis a newly-mapped JSON field (guarded unmarshal added insql_store.go, mirroringCustomHeaders/CaptureContentTypes) but isn't set on the target here nor asserted in the verification block, so its round trip is unverified.♻️ Proposed addition
Options: rpservice.TargetOptions{ SkipTLSVerify: true, RequestTimeout: 30 * time.Second, SessionIdleTimeout: 5 * time.Minute, PathRewrite: rpservice.PathRewritePreserve, CustomHeaders: map[string]string{"X-Foo": "bar"}, DirectUpstream: true, + Middlewares: []string{"rate-limit"}, CaptureMaxRequestBytes: 1024, CaptureMaxResponseBytes: 2048, CaptureContentTypes: []string{"application/json"}, AgentNetwork: true, DisableAccessLog: true, },assert.True(t, tg.Options.DirectUpstream, "options direct upstream") + assert.Equal(t, []string{"rate-limit"}, tg.Options.Middlewares, "options middlewares") assert.Equal(t, int64(1024), tg.Options.CaptureMaxRequestBytes, "options capture max request bytes")Adjust the field type/value above to match the actual
Middlewaresfield type onTargetOptions.Also applies to: 120-133
🤖 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 `@management/server/store/sql_store_service_test.go` around lines 91 - 104, Add Middlewares to the TargetOptions fixture in the round-trip test using a value matching its actual field type, then update the corresponding verification block to assert the value survives serialization and deserialization. Apply the same coverage to the additional target fixture referenced by the comment.
🤖 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.
Nitpick comments:
In `@management/server/store/sql_store_service_test.go`:
- Around line 91-104: Add Middlewares to the TargetOptions fixture in the
round-trip test using a value matching its actual field type, then update the
corresponding verification block to assert the value survives serialization and
deserialization. Apply the same coverage to the additional target fixture
referenced by the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a600a040-9fb3-4538-93a1-fbd422524b58
📒 Files selected for processing (2)
management/server/store/sql_store.gomanagement/server/store/sql_store_service_test.go
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
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 `@management/server/store/sql_store.go`:
- Around line 2405-2406: Update scanService’s listenPort handling to validate
listenPort.Int64 is within the uint16 port range (0–65535) before conversion.
Reject out-of-range persisted values instead of narrowing them, while preserving
assignment for valid values.
🪄 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 Plus
Run ID: 65bb3c14-1f89-47c1-a0a0-8b3ad3e50796
📒 Files selected for processing (1)
management/server/store/sql_store.go
|
2 participants
Describe your changes
The Postgres read path in
getServices(sql_store.go) hand-writes the column list and row scan, and had drifted from the gorm model. Several service and target columns were absent from theSELECTand scan, so on Postgres those fields came back zero-valued while SQLite and MySQL loaded them correctly. This adds the missing columns and a test that prevents the lists from drifting again.meta_last_renewed_attimestamp used for ephemeral service expiryproxy_protocoland the full embedded target options (TLS skip, timeouts, path rewrite, custom headers, direct upstream, capture settings, middlewares, agent-network flags)Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Internal storage-layer parity fix with no user-facing or API change.
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit