Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds account-scoped BYOP: per-account cluster allow-lists, proxy lifecycle APIs (Connect with accountID, account proxy queries, deletion), proxy access tokens (HTTP endpoints, store APIs), and gRPC/server changes enforcing account-scoped access and token validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Proxy as BYOP Proxy
participant GRPC as Proxy gRPC Server
participant TokenChk as ProxyTokenChecker
participant Store as Store
participant SvcMgr as ServiceManager
Proxy->>GRPC: GetMappingUpdate(proxyID, token)
GRPC->>TokenChk: IsProxyAccessTokenValid(tokenID)
TokenChk->>Store: GetProxyAccessTokenByID(tokenID)
Store-->>TokenChk: ProxyAccessToken (includes AccountID)
TokenChk-->>GRPC: true
GRPC->>GRPC: enforceAccountScope(requestAccountID, tokenAccountID)
alt mismatch
GRPC-->>Proxy: PermissionDenied
else scope ok
GRPC->>Store: CountProxiesByAccountID(accountID)
Store-->>GRPC: count
alt count >= 1 (enforce single)
GRPC->>GRPC: ForceDisconnect(existingProxyID)
end
GRPC->>Store: GetActiveProxyClusterAddressesForAccount(accountID)
Store-->>GRPC: [clusterAddrs]
GRPC->>GRPC: Validate/claim clusterAddress
GRPC->>GRPC: proxyManager.Connect(proxyID, clusterAddr, ip, &accountID)
GRPC->>SvcMgr: GetAccountServices(accountID)
SvcMgr-->>GRPC: [services]
GRPC-->>Proxy: MappingUpdate{services}
end
sequenceDiagram
participant Client as HTTP Client
participant Handler as ProxyToken Handler
participant Perm as PermissionsManager
participant Store as Store
Client->>Handler: POST /api/reverse-proxies/proxy-tokens {name, expires_in}
Handler->>Perm: Validate(Create)
alt denied
Perm-->>Handler: Forbidden
Handler-->>Client: 403
else allowed
Handler->>Handler: CreateNewProxyAccessToken()
Handler->>Store: SaveProxyAccessToken(token)
Store-->>Handler: saved
Handler-->>Client: 201 {plain_token, id, created_at}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds “bring your own proxy” (BYOD) support to the reverse-proxy subsystem by introducing account-scoped proxy access tokens, self-hosted proxy visibility/deletion APIs, and gRPC-side enforcement so BYOD proxies only see their own account’s services.
Changes:
- Add API models + OpenAPI endpoints for managing proxy tokens and listing/deleting self-hosted proxies.
- Enforce account scoping in the proxy gRPC service (service snapshots/updates + method-level account checks) and add token validity checks during heartbeat.
- Extend store + proxy manager to support account-scoped proxies (unique per account), cluster conflict checks, and related queries.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/management/http/api/types.gen.go | Adds generated API models/enums for proxy tokens and self-hosted proxies. |
| shared/management/http/api/openapi.yml | Documents new REST endpoints and schemas for proxy tokens and self-hosted proxies. |
| proxy/management_integration_test.go | Updates mocks/signatures for new proxy manager API. |
| proxy/management_byod_integration_test.go | Adds integration coverage for BYOD proxy scoping and constraints. |
| management/server/store/store_mock.go | Updates gomock store with new proxy/token-related methods. |
| management/server/store/store.go | Extends Store interface with proxy-token queries and BYOD proxy operations. |
| management/server/store/sql_store.go | Implements new store methods (token queries, proxy lookup/count/conflict, disconnect, delete). |
| management/server/http/testing/testing_tools/channel/channel.go | Updates API/proxy server wiring for new constructor signatures. |
| management/server/http/handlers/proxy/auth_callback_integration_test.go | Updates test wiring and service manager interface changes. |
| management/server/http/handler.go | Registers new proxytoken + selfhostedproxy HTTP endpoints and updates handler signature. |
| management/server/account_test.go | Updates proxy service server constructor signature usage. |
| management/internals/shared/grpc/validate_session_test.go | Updates test wiring and mocks for new service manager/proxy manager APIs. |
| management/internals/shared/grpc/proxy_group_access_test.go | Implements GetServiceByDomain in mock reverse proxy manager. |
| management/internals/shared/grpc/proxy_auth.go | Removes outdated TODO now that account scoping is enforced elsewhere. |
| management/internals/shared/grpc/proxy_address_test.go | Adds tests for proxy address validation (domain + IP). |
| management/internals/shared/grpc/proxy.go | Core BYOD logic: account scoping, token validity heartbeat checks, service filtering, force disconnect. |
| management/internals/server/boot.go | Wires API handler and proxy service server with new dependencies (store/token checker/proxy manager). |
| management/internals/modules/reverseproxy/service/manager/manager_test.go | Updates proxy service server constructor signature in tests. |
| management/internals/modules/reverseproxy/service/manager/manager.go | Adds GetServiceByDomain passthrough to store. |
| management/internals/modules/reverseproxy/service/interface_mock.go | Updates gomock for new Manager method. |
| management/internals/modules/reverseproxy/service/interface.go | Extends service.Manager interface with GetServiceByDomain. |
| management/internals/modules/reverseproxy/selfhostedproxy/handler_test.go | Adds handler tests for listing/deleting self-hosted proxies. |
| management/internals/modules/reverseproxy/selfhostedproxy/handler.go | Implements REST handlers for self-hosted proxy list/delete. |
| management/internals/modules/reverseproxy/proxytoken/handler_test.go | Adds handler tests for creating/listing/revoking proxy tokens. |
| management/internals/modules/reverseproxy/proxytoken/handler.go | Implements REST handlers for proxy token CRUD (account-scoped). |
| management/internals/modules/reverseproxy/proxy/proxy.go | Adds AccountID and status constants to proxy model. |
| management/internals/modules/reverseproxy/proxy/manager_mock.go | Updates gomock proxy.Manager interface with BYOD methods. |
| management/internals/modules/reverseproxy/proxy/manager/manager_test.go | Adds tests for new proxy manager behaviors (account + conflict checks). |
| management/internals/modules/reverseproxy/proxy/manager/manager.go | Implements account-scoped connect/disconnect and new proxy queries/ops. |
| management/internals/modules/reverseproxy/proxy/manager.go | Extends proxy.Manager interface for BYOD support. |
| management/internals/modules/reverseproxy/domain/manager/manager_test.go | Adds tests for BYOD-aware cluster allow-list behavior. |
| management/internals/modules/reverseproxy/domain/manager/manager.go | Uses BYOD-aware cluster allow-list for domain operations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
shared/management/http/api/openapi.yml (1)
9704-9804: Define theSelf-Hosted Proxiestag in the global tags section.These new operations reference
Self-Hosted Proxies, but the tag is not declared in top-leveltags. Adding it keeps docs grouping and descriptions consistent.Proposed tag declaration
tags: - name: Services description: Interact with and view information about reverse proxy services. + - name: Self-Hosted Proxies + description: Manage self-hosted reverse proxy instances and proxy access tokens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/http/api/openapi.yml` around lines 9704 - 9804, The OpenAPI spec is using the tag name "Self-Hosted Proxies" in several paths but that tag is not defined in the root-level tags array; add a root-level tag entry with name: "Self-Hosted Proxies" and a short description (e.g., "Endpoints for managing self-hosted reverse proxies and proxy tokens") to the top-level tags section of the OpenAPI document so these operations are properly documented and grouped (look for usages in paths like /api/reverse-proxies/self-hosted-proxies and /api/reverse-proxies/proxy-tokens to verify).management/internals/modules/reverseproxy/proxy/manager_mock.go (1)
1-2: Regenerate mock instead of manual edits.The header states "Code generated by MockGen. DO NOT EDIT." but the changes appear to be manual additions. Manually editing generated mocks risks drift from the source interface and makes future regeneration problematic. Run
mockgento regenerate this file from the updatedManagerinterface inmanager.go.#!/bin/bash # Check for go:generate directive in the source interface file rg -n "go:generate.*mockgen" management/internals/modules/reverseproxy/proxy/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/proxy/manager_mock.go` around lines 1 - 2, The generated mock file manager_mock.go has been manually edited; regenerate it from the current Manager interface to avoid drift. Run mockgen against the Manager interface declared in manager.go (or use the project's go:generate directive if present) to recreate manager_mock.go so it matches the source interface exactly; replace the manual edits with the freshly generated output and commit that instead of editing MockManager or any symbols in manager_mock.go by hand.management/server/store/store_mock.go (1)
1403-1407: Inconsistent type annotation:anyvsinterface{}.Line 1404 uses
anyfor the parameter types while all other recorder methods in this generated file useinterface{}. Although functionally equivalent (Go 1.18+ alias), this inconsistency suggests the file may have been manually edited or generated with mixed mockgen versions.If this file is meant to be regenerated, consider running
go generateto ensure consistent output. If manual additions are intentional, align with the existing pattern.♻️ Suggested fix for consistency
// IsProxyAccessTokenValid indicates an expected call of IsProxyAccessTokenValid. -func (mr *MockStoreMockRecorder) IsProxyAccessTokenValid(ctx, tokenID any) *gomock.Call { +func (mr *MockStoreMockRecorder) IsProxyAccessTokenValid(ctx, tokenID interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsProxyAccessTokenValid", reflect.TypeOf((*MockStore)(nil).IsProxyAccessTokenValid), ctx, tokenID) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/store/store_mock.go` around lines 1403 - 1407, The recorder method IsProxyAccessTokenValid on type MockStoreMockRecorder uses the Go 1.18 alias `any` for parameters but all other generated recorder methods use `interface{}`, creating an inconsistent style; change the parameter types in the IsProxyAccessTokenValid signature (and any related occurrences in the same method) from `any` to `interface{}` so it matches the rest of the generated mocks (or regenerate the file via go generate/mockgen to restore uniform output).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/internals/modules/reverseproxy/domain/manager/manager.go`:
- Around line 288-296: The getClusterAllowList implementation should fall back
to shared clusters if GetActiveClusterAddressesForAccount fails: change
Manager.getClusterAllowList so that if
m.proxyManager.GetActiveClusterAddressesForAccount(ctx, accountID) returns an
error you do not immediately return, but instead call
m.proxyManager.GetActiveClusterAddresses(ctx) and return its result; keep the
current path that returns byodAddresses when len(byodAddresses) > 0, and only
return an error if both GetActiveClusterAddressesForAccount and
GetActiveClusterAddresses fail (combine or wrap errors for context). This
preserves behaviour expected by GetDomains/CreateDomain/DeriveClusterFromDomain
and the tests.
In `@management/internals/modules/reverseproxy/proxytoken/handler.go`:
- Around line 135-138: The current call to h.store.GetProxyAccessTokenByID
collapses every error into a 404; change the error handling so only a genuine
"not found" error maps to util.WriteErrorResponse("token not found",
http.StatusNotFound, w) and all other errors return a 5xx (e.g.,
util.WriteErrorResponse("internal server error", http.StatusInternalServerError,
w)). Detect the not-found case by checking the store's sentinel error (e.g.,
sql.ErrNoRows or store.ErrNotFound) from GetProxyAccessTokenByID, log the actual
error for non-404 failures, and preserve early return behavior around token, err
in the same handler function.
In `@management/internals/modules/reverseproxy/selfhostedproxy/handler.go`:
- Around line 106-109: The code currently maps every error from
h.proxyMgr.GetAccountProxy to a 404; change the handler to distinguish a genuine
"not found" sentinel from other errors: call h.proxyMgr.GetAccountProxy(ctx,
userAuth.AccountId), then if errors.Is(err, <the package's NotFound error, e.g.
proxymgr.ErrNotFound or store.ErrNotFound>) return
util.WriteErrorResponse("proxy not found", http.StatusNotFound, w); for any
other non-nil err log the error and return util.WriteErrorResponse("internal
error loading proxy", http.StatusInternalServerError, w). Ensure you
import/errors.Is and reference the actual NotFound sentinel used by
GetAccountProxy.
- Around line 70-77: The handler currently ignores errors from
h.serviceMgr.GetAccountServices which can yield an incorrect serviceCount and a
200 response; update the logic around h.serviceMgr.GetAccountServices in
handler.go to handle the error instead of swallowing it—if GetAccountServices
returns an error, log the error (including context such as userAuth.AccountId
and p.ClusterAddress), and return an appropriate error response (e.g.,
500/internal server error or propagate the error via the handler's error flow)
rather than continuing to compute serviceCount; ensure references to
serviceCount, h.serviceMgr.GetAccountServices, and the surrounding proxy-listing
handler code are updated consistently.
In `@management/internals/shared/grpc/proxy_group_access_test.go`:
- Around line 93-102: The mock GetServiceByDomain method ignores the injected
error field m.err; update GetServiceByDomain to first check if m.err != nil and
return nil, m.err to mirror other mock methods and allow manager-failure
scenarios to be tested, then proceed to iterate m.proxiesByAccount for the
domain (function: mockReverseProxyManager.GetServiceByDomain; symbol: m.err).
In `@management/internals/shared/grpc/proxy.go`:
- Around line 456-463: The current BYOD filter only checks update.Mapping[0]
causing cross-account leaks; replace the single updateAccountID check in the
s.connectedProxies.Range loop with a membership test against the set of all
account IDs in update.Mapping (e.g., build a map/set of account IDs from
update.Mapping before calling s.connectedProxies.Range), then inside the Range
use conn.accountID to test membership in that set (and preserve the existing
behavior when update.Mapping is empty). Update references: update.Mapping, the
temporary updateAccountID logic, s.connectedProxies.Range and
proxyConnection.accountID.
- Around line 186-187: The call to proxyManager.GetAccountProxy currently
ignores its error and only checks existingProxy, which can silently bypass the
1-proxy-per-account check; update the GetAccountProxy call in the handler to
capture the returned error (err := s.proxyManager.GetAccountProxy(...)), and if
err is non-nil propagate it (return the error or convert to an appropriate gRPC
error) instead of discarding it, then continue to compare existingProxy.ID to
proxyID as before to enforce the BYOD limit.
In `@shared/management/http/api/openapi.yml`:
- Around line 3225-3227: The OpenAPI field service_count is documented as an
exact integer but can be 0 on backend failures; update the contract to reflect
reality by changing the description for service_count to state it is
best-effort/approximate (e.g., "Approximate number of services routed through
this proxy's cluster; may be 0 if service retrieval fails") OR change the API
behavior so that the endpoint returns an error when service retrieval/count
computation fails instead of returning 0; locate the service_count YAML entry
and either update its description text to mark it best-effort or adjust the
server-side handler that computes service_count to surface errors.
- Around line 3161-3164: The expires_in property in the OpenAPI schema currently
permits negative integers; update the ProxyTokenRequest schema's expires_in
definition to enforce non-negative values by adding a minimum constraint
(minimum: 0) so generated clients validate that expires_in >= 0 while keeping
type: integer and the existing example; target the expires_in property in the
OpenAPI YAML (ProxyTokenRequest.expires_in).
---
Nitpick comments:
In `@management/internals/modules/reverseproxy/proxy/manager_mock.go`:
- Around line 1-2: The generated mock file manager_mock.go has been manually
edited; regenerate it from the current Manager interface to avoid drift. Run
mockgen against the Manager interface declared in manager.go (or use the
project's go:generate directive if present) to recreate manager_mock.go so it
matches the source interface exactly; replace the manual edits with the freshly
generated output and commit that instead of editing MockManager or any symbols
in manager_mock.go by hand.
In `@management/server/store/store_mock.go`:
- Around line 1403-1407: The recorder method IsProxyAccessTokenValid on type
MockStoreMockRecorder uses the Go 1.18 alias `any` for parameters but all other
generated recorder methods use `interface{}`, creating an inconsistent style;
change the parameter types in the IsProxyAccessTokenValid signature (and any
related occurrences in the same method) from `any` to `interface{}` so it
matches the rest of the generated mocks (or regenerate the file via go
generate/mockgen to restore uniform output).
In `@shared/management/http/api/openapi.yml`:
- Around line 9704-9804: The OpenAPI spec is using the tag name "Self-Hosted
Proxies" in several paths but that tag is not defined in the root-level tags
array; add a root-level tag entry with name: "Self-Hosted Proxies" and a short
description (e.g., "Endpoints for managing self-hosted reverse proxies and proxy
tokens") to the top-level tags section of the OpenAPI document so these
operations are properly documented and grouped (look for usages in paths like
/api/reverse-proxies/self-hosted-proxies and /api/reverse-proxies/proxy-tokens
to verify).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8342e502-eaa1-4ed0-8933-1c57a8e3c02d
📒 Files selected for processing (32)
management/internals/modules/reverseproxy/domain/manager/manager.gomanagement/internals/modules/reverseproxy/domain/manager/manager_test.gomanagement/internals/modules/reverseproxy/proxy/manager.gomanagement/internals/modules/reverseproxy/proxy/manager/manager.gomanagement/internals/modules/reverseproxy/proxy/manager/manager_test.gomanagement/internals/modules/reverseproxy/proxy/manager_mock.gomanagement/internals/modules/reverseproxy/proxy/proxy.gomanagement/internals/modules/reverseproxy/proxytoken/handler.gomanagement/internals/modules/reverseproxy/proxytoken/handler_test.gomanagement/internals/modules/reverseproxy/selfhostedproxy/handler.gomanagement/internals/modules/reverseproxy/selfhostedproxy/handler_test.gomanagement/internals/modules/reverseproxy/service/interface.gomanagement/internals/modules/reverseproxy/service/interface_mock.gomanagement/internals/modules/reverseproxy/service/manager/manager.gomanagement/internals/modules/reverseproxy/service/manager/manager_test.gomanagement/internals/server/boot.gomanagement/internals/shared/grpc/proxy.gomanagement/internals/shared/grpc/proxy_address_test.gomanagement/internals/shared/grpc/proxy_auth.gomanagement/internals/shared/grpc/proxy_group_access_test.gomanagement/internals/shared/grpc/validate_session_test.gomanagement/server/account_test.gomanagement/server/http/handler.gomanagement/server/http/handlers/proxy/auth_callback_integration_test.gomanagement/server/http/testing/testing_tools/channel/channel.gomanagement/server/store/sql_store.gomanagement/server/store/store.gomanagement/server/store/store_mock.goproxy/management_byod_integration_test.goproxy/management_integration_test.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.go
💤 Files with no reviewable changes (1)
- management/internals/shared/grpc/proxy_auth.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
management/internals/shared/grpc/proxy.go (2)
186-187:⚠️ Potential issue | 🟠 MajorHandle
GetAccountProxyerrors instead of discarding them.Dropping this error can silently skip account-level proxy-limit enforcement during backend/store failures.
💡 Suggested fix
- existingProxy, _ := s.proxyManager.GetAccountProxy(ctx, *accountID) + existingProxy, err := s.proxyManager.GetAccountProxy(ctx, *accountID) + if err != nil { + return status.Errorf(codes.Internal, "get account proxy: %v", err) + } if existingProxy != nil && existingProxy.ID != proxyID {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/shared/grpc/proxy.go` around lines 186 - 187, The code is ignoring the error returned from s.proxyManager.GetAccountProxy which can hide backend/store failures and skip proxy-limit enforcement; update the call to check the returned error from GetAccountProxy(ctx, *accountID) (and the existingProxy result), and handle it appropriately in the surrounding function (e.g., return the error to the caller or log/contextualize it and abort the operation) instead of discarding it; ensure references to existingProxy and proxyID remain and that any decision that relied on existingProxy only proceeds when err == nil.
456-463:⚠️ Potential issue | 🔴 CriticalFilter BYOP updates per mapping, not by
Mapping[0]account only.Using only the first mapping’s account can leak mappings from other accounts when a batch contains mixed-account entries.
💡 Suggested fix
- var updateAccountID string - if len(update.Mapping) > 0 { - updateAccountID = update.Mapping[0].AccountId - } s.connectedProxies.Range(func(key, value interface{}) bool { conn := value.(*proxyConnection) - if conn.accountID != nil && updateAccountID != "" && *conn.accountID != updateAccountID { + filtered := make([]*proto.ProxyMapping, 0, len(update.Mapping)) + for _, m := range update.Mapping { + if conn.accountID != nil && m.AccountId != "" && *conn.accountID != m.AccountId { + continue + } + filtered = append(filtered, m) + } + if len(filtered) == 0 { return true } - msg := s.perProxyMessage(update, conn.proxyID) + msg := s.perProxyMessage(&proto.GetMappingUpdateResponse{ + Mapping: filtered, + InitialSyncComplete: update.InitialSyncComplete, + }, conn.proxyID) if msg == nil { return true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/shared/grpc/proxy.go` around lines 456 - 463, The code currently uses only update.Mapping[0] to decide whether to skip a proxy, which can leak mappings for other accounts; instead build the set of account IDs present in update.Mapping (or iterate update.Mapping) and check membership against conn.accountID inside connectedProxies.Range (use update.Mapping slice rather than Mapping[0]); update the logic in the connectedProxies.Range closure (and the temporary updateAccountID variable) so you compare *conn.accountID to any account id in the update.Mapping set and only skip when there is no match.management/internals/modules/reverseproxy/domain/manager/manager.go (1)
288-296:⚠️ Potential issue | 🟠 MajorFall back to shared clusters when BYOP lookup errors.
If account-scoped cluster lookup fails, this path currently returns immediately and blocks
GetDomains,CreateDomain, andDeriveClusterFromDomaineven when shared clusters are available.💡 Suggested fix
func (m Manager) getClusterAllowList(ctx context.Context, accountID string) ([]string, error) { byopAddresses, err := m.proxyManager.GetActiveClusterAddressesForAccount(ctx, accountID) if err != nil { - return nil, fmt.Errorf("get BYOP cluster addresses: %w", err) + log.WithContext(ctx).WithError(err).Warn("failed to load account-scoped proxy clusters, falling back to shared clusters") + sharedAddresses, sharedErr := m.proxyManager.GetActiveClusterAddresses(ctx) + if sharedErr != nil { + return nil, fmt.Errorf("get shared cluster addresses after BYOP lookup failed: %w", sharedErr) + } + return sharedAddresses, nil } if len(byopAddresses) > 0 { return byopAddresses, nil } return m.proxyManager.GetActiveClusterAddresses(ctx) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/domain/manager/manager.go` around lines 288 - 296, In getClusterAllowList, don't return early on an error from m.proxyManager.GetActiveClusterAddressesForAccount; instead treat BYOP lookup failures as non-fatal and fall back to shared clusters by calling m.proxyManager.GetActiveClusterAddresses(ctx). Concretely, replace the current error branch in Manager.getClusterAllowList so that when GetActiveClusterAddressesForAccount returns an error you call and return m.proxyManager.GetActiveClusterAddresses(ctx); otherwise, if byopAddresses has entries return them, else return the shared addresses. Reference: Manager.getClusterAllowList, m.proxyManager.GetActiveClusterAddressesForAccount, m.proxyManager.GetActiveClusterAddresses.
🧹 Nitpick comments (2)
management/internals/modules/reverseproxy/proxy/manager/manager_test.go (1)
308-321: Consider adding an error case for consistency.Other tests in this file (
TestIsClusterAddressAvailable,TestCountAccountProxies,TestDeleteProxy) include error propagation subtests. Adding an error case here would maintain consistency and ensure error handling is verified.🔧 Suggested addition
func TestGetActiveClusterAddressesForAccount(t *testing.T) { - expected := []string{"byop.example.com"} - s := &mockStore{ - getActiveProxyClusterAddressesForAccFunc: func(_ context.Context, accID string) ([]string, error) { - assert.Equal(t, "acc-123", accID) - return expected, nil - }, - } + t.Run("success", func(t *testing.T) { + expected := []string{"byop.example.com"} + s := &mockStore{ + getActiveProxyClusterAddressesForAccFunc: func(_ context.Context, accID string) ([]string, error) { + assert.Equal(t, "acc-123", accID) + return expected, nil + }, + } - mgr := newTestManager(s) - result, err := mgr.GetActiveClusterAddressesForAccount(context.Background(), "acc-123") - require.NoError(t, err) - assert.Equal(t, expected, result) + mgr := newTestManager(s) + result, err := mgr.GetActiveClusterAddressesForAccount(context.Background(), "acc-123") + require.NoError(t, err) + assert.Equal(t, expected, result) + }) + + t.Run("store error", func(t *testing.T) { + s := &mockStore{ + getActiveProxyClusterAddressesForAccFunc: func(_ context.Context, _ string) ([]string, error) { + return nil, errors.New("db error") + }, + } + + mgr := newTestManager(s) + _, err := mgr.GetActiveClusterAddressesForAccount(context.Background(), "acc-123") + assert.Error(t, err) + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/proxy/manager/manager_test.go` around lines 308 - 321, Add a subtest to TestGetActiveClusterAddressesForAccount that exercises the error path: configure the mockStore.getActiveProxyClusterAddressesForAccFunc to return a non-nil error for the given account ID, call mgr.GetActiveClusterAddressesForAccount(context.Background(), "acc-123") and assert that the returned error is the same (or non-nil) and the result is nil/empty as appropriate; place this alongside the existing success case so both success and error propagation for GetActiveClusterAddressesForAccount are verified.proxy/management_byop_integration_test.go (1)
357-357: Avoid fixedSleepin reconnect flow assertions.Using a fixed delay here can make the reconnect test timing-sensitive under CI load. Prefer an eventual condition tied to stream shutdown/disconnect state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/management_byop_integration_test.go` at line 357, Replace the brittle fixed time.Sleep(200 * time.Millisecond) in the reconnect test with an eventual/wait assertion that polls the actual disconnect/shutdown condition of the stream/connection (e.g., use require.Eventually from testify or a small loop that checks stream.IsClosed()/stream.Shutdown() or a channel that signals disconnect) with a reasonable timeout and interval; update the test that currently contains the Sleep call to wait until the stream/connection reaches the expected state instead of sleeping, referencing the test's stream/connection variable and the condition method (e.g., IsClosed, Closed(), Shutdown, or its disconnect channel) to make the assertion robust under CI load.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@proxy/management_byop_integration_test.go`:
- Around line 277-285: Test is brittle because gRPC can return the terminal
error either from the RPC call or from the first Recv; update the
negative-stream tests (the calls to client.GetMappingUpdate and the first
stream.Recv) to accept an error from either location: attempt to call
GetMappingUpdate (e.g., GetMappingUpdate for "byop-proxy-a-second"/"test-v1"),
if it returns an error assert it is the expected terminal error and return;
otherwise proceed to call stream.Recv and assert that that call returns the
expected error—do the same pattern for the other occurrences around the
GetMappingUpdate/Recv pairs (the blocks referenced at ~318-326 and ~395-403) so
tests pass whether the error surfaces at call time or on first Recv.
---
Duplicate comments:
In `@management/internals/modules/reverseproxy/domain/manager/manager.go`:
- Around line 288-296: In getClusterAllowList, don't return early on an error
from m.proxyManager.GetActiveClusterAddressesForAccount; instead treat BYOP
lookup failures as non-fatal and fall back to shared clusters by calling
m.proxyManager.GetActiveClusterAddresses(ctx). Concretely, replace the current
error branch in Manager.getClusterAllowList so that when
GetActiveClusterAddressesForAccount returns an error you call and return
m.proxyManager.GetActiveClusterAddresses(ctx); otherwise, if byopAddresses has
entries return them, else return the shared addresses. Reference:
Manager.getClusterAllowList, m.proxyManager.GetActiveClusterAddressesForAccount,
m.proxyManager.GetActiveClusterAddresses.
In `@management/internals/shared/grpc/proxy.go`:
- Around line 186-187: The code is ignoring the error returned from
s.proxyManager.GetAccountProxy which can hide backend/store failures and skip
proxy-limit enforcement; update the call to check the returned error from
GetAccountProxy(ctx, *accountID) (and the existingProxy result), and handle it
appropriately in the surrounding function (e.g., return the error to the caller
or log/contextualize it and abort the operation) instead of discarding it;
ensure references to existingProxy and proxyID remain and that any decision that
relied on existingProxy only proceeds when err == nil.
- Around line 456-463: The code currently uses only update.Mapping[0] to decide
whether to skip a proxy, which can leak mappings for other accounts; instead
build the set of account IDs present in update.Mapping (or iterate
update.Mapping) and check membership against conn.accountID inside
connectedProxies.Range (use update.Mapping slice rather than Mapping[0]); update
the logic in the connectedProxies.Range closure (and the temporary
updateAccountID variable) so you compare *conn.accountID to any account id in
the update.Mapping set and only skip when there is no match.
---
Nitpick comments:
In `@management/internals/modules/reverseproxy/proxy/manager/manager_test.go`:
- Around line 308-321: Add a subtest to TestGetActiveClusterAddressesForAccount
that exercises the error path: configure the
mockStore.getActiveProxyClusterAddressesForAccFunc to return a non-nil error for
the given account ID, call
mgr.GetActiveClusterAddressesForAccount(context.Background(), "acc-123") and
assert that the returned error is the same (or non-nil) and the result is
nil/empty as appropriate; place this alongside the existing success case so both
success and error propagation for GetActiveClusterAddressesForAccount are
verified.
In `@proxy/management_byop_integration_test.go`:
- Line 357: Replace the brittle fixed time.Sleep(200 * time.Millisecond) in the
reconnect test with an eventual/wait assertion that polls the actual
disconnect/shutdown condition of the stream/connection (e.g., use
require.Eventually from testify or a small loop that checks
stream.IsClosed()/stream.Shutdown() or a channel that signals disconnect) with a
reasonable timeout and interval; update the test that currently contains the
Sleep call to wait until the stream/connection reaches the expected state
instead of sleeping, referencing the test's stream/connection variable and the
condition method (e.g., IsClosed, Closed(), Shutdown, or its disconnect channel)
to make the assertion robust under CI load.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50de4469-ae6b-42f6-819e-68d274967b3f
📒 Files selected for processing (7)
management/internals/modules/reverseproxy/domain/manager/manager.gomanagement/internals/modules/reverseproxy/domain/manager/manager_test.gomanagement/internals/modules/reverseproxy/proxy/manager/manager_test.gomanagement/internals/modules/reverseproxy/selfhostedproxy/handler_test.gomanagement/internals/shared/grpc/proxy.gomanagement/internals/shared/grpc/proxy_address_test.goproxy/management_byop_integration_test.go
✅ Files skipped from review due to trivial changes (1)
- management/internals/shared/grpc/proxy_address_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- management/internals/modules/reverseproxy/domain/manager/manager_test.go
- management/internals/modules/reverseproxy/selfhostedproxy/handler_test.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shared/management/http/api/openapi.yml (1)
3204-3223:⚠️ Potential issue | 🟠 MajorMake the cluster deletion key discoverable.
The new delete route takes
clusterId, but the list response still only exposesaddress,connected_proxies, andself_hosted. As documented, clients cannot tell which value should be passed to{clusterId}. Either add the delete identifier toProxyCluster, or make it explicit that the path parameter is the cluster address and align the parameter name/description with that.Also applies to: 9825-9853
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/http/api/openapi.yml` around lines 3204 - 3223, The ProxyCluster schema lacks the identifier required by the new delete route: update the OpenAPI by either adding an explicit identifier property (e.g., id or cluster_id) to the ProxyCluster model (include type/string, description "cluster identifier for delete operations", and an example) and mark it required, or change the delete path parameter and its description to clearly use the cluster address (rename clusterId → address in the path parameter and state it maps to ProxyCluster.address). Ensure references to ProxyCluster and the delete path/parameter are consistent (also apply the same change at the other occurrences noted around lines 9825-9853).
🧹 Nitpick comments (2)
management/server/store/sql_store.go (2)
5432-5448: Consider checkingRowsAffectedfor consistency.
DisconnectProxydoesn't verify whether any rows were actually updated. If called with a non-existentproxyID, it silently succeeds. Other similar methods likeDeleteProxy(line 5537) returnstatus.NotFoundwhen no rows are affected.If idempotent behavior is intended (disconnect is a no-op for missing proxies), a comment clarifying this would help. Otherwise, consider adding a check:
♻️ Suggested fix
func (s *SqlStore) DisconnectProxy(ctx context.Context, proxyID string) error { now := time.Now() result := s.db.WithContext(ctx). Model(&proxy.Proxy{}). Where("id = ?", proxyID). Updates(map[string]interface{}{ "status": proxy.StatusDisconnected, "disconnected_at": now, "last_seen": now, }) if result.Error != nil { log.WithContext(ctx).Errorf("failed to disconnect proxy: %v", result.Error) return status.Errorf(status.Internal, "failed to disconnect proxy") } + if result.RowsAffected == 0 { + return status.Errorf(status.NotFound, "proxy not found") + } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/store/sql_store.go` around lines 5432 - 5448, DisconnectProxy currently updates proxy.StatusDisconnected/disconnected_at/last_seen via s.db.Model(&proxy.Proxy{}) but doesn't check result.RowsAffected, so calling it with a non-existent proxyID silently succeeds; update DisconnectProxy in SqlStore to inspect result.RowsAffected and, if zero, return status.Errorf(status.NotFound, "proxy not found") (matching behavior of DeleteProxy) or, if idempotent behavior is desired, add a clear comment documenting that a missing proxy is treated as a no-op instead of returning NotFound—look for function DisconnectProxy, type proxy.Proxy{}, and DeleteProxy to mirror existing behavior.
5482-5496: Minor: Add error logging for consistency.This method lacks the
log.WithContext(ctx).Errorfcall that the globalGetActiveProxyClusterAddresses(line 5475) includes. Adding it would maintain consistency and aid debugging.♻️ Suggested fix
func (s *SqlStore) GetActiveProxyClusterAddressesForAccount(ctx context.Context, accountID string) ([]string, error) { var addresses []string result := s.db.WithContext(ctx). Model(&proxy.Proxy{}). Where("account_id = ? AND status = ? AND last_seen > ?", accountID, proxy.StatusConnected, time.Now().Add(-2*time.Minute)). Distinct("cluster_address"). Pluck("cluster_address", &addresses) if result.Error != nil { + log.WithContext(ctx).Errorf("failed to get active proxy cluster addresses for account: %v", result.Error) return nil, status.Errorf(status.Internal, "failed to get active proxy cluster addresses for account") } return addresses, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/store/sql_store.go` around lines 5482 - 5496, The GetActiveProxyClusterAddressesForAccount method is missing error logging; when s.db.WithContext(...).Pluck(...) returns result.Error, call log.WithContext(ctx).Errorf and include the error and the accountID for context before returning the status error. Update the error branch in SqlStore.GetActiveProxyClusterAddressesForAccount to mirror GetActiveProxyClusterAddresses by logging result.Error (and accountID) using log.WithContext(ctx).Errorf, then return the existing status.Errorf.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@shared/management/http/api/openapi.yml`:
- Around line 3204-3223: The ProxyCluster schema lacks the identifier required
by the new delete route: update the OpenAPI by either adding an explicit
identifier property (e.g., id or cluster_id) to the ProxyCluster model (include
type/string, description "cluster identifier for delete operations", and an
example) and mark it required, or change the delete path parameter and its
description to clearly use the cluster address (rename clusterId → address in
the path parameter and state it maps to ProxyCluster.address). Ensure references
to ProxyCluster and the delete path/parameter are consistent (also apply the
same change at the other occurrences noted around lines 9825-9853).
---
Nitpick comments:
In `@management/server/store/sql_store.go`:
- Around line 5432-5448: DisconnectProxy currently updates
proxy.StatusDisconnected/disconnected_at/last_seen via
s.db.Model(&proxy.Proxy{}) but doesn't check result.RowsAffected, so calling it
with a non-existent proxyID silently succeeds; update DisconnectProxy in
SqlStore to inspect result.RowsAffected and, if zero, return
status.Errorf(status.NotFound, "proxy not found") (matching behavior of
DeleteProxy) or, if idempotent behavior is desired, add a clear comment
documenting that a missing proxy is treated as a no-op instead of returning
NotFound—look for function DisconnectProxy, type proxy.Proxy{}, and DeleteProxy
to mirror existing behavior.
- Around line 5482-5496: The GetActiveProxyClusterAddressesForAccount method is
missing error logging; when s.db.WithContext(...).Pluck(...) returns
result.Error, call log.WithContext(ctx).Errorf and include the error and the
accountID for context before returning the status error. Update the error branch
in SqlStore.GetActiveProxyClusterAddressesForAccount to mirror
GetActiveProxyClusterAddresses by logging result.Error (and accountID) using
log.WithContext(ctx).Errorf, then return the existing status.Errorf.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d3b0e87-24fd-420b-9131-06ed8e93f6f1
📒 Files selected for processing (10)
management/internals/modules/reverseproxy/domain/manager/manager_test.gomanagement/internals/modules/reverseproxy/proxy/proxy.gomanagement/internals/modules/reverseproxy/proxytoken/handler.gomanagement/internals/shared/grpc/proxy.gomanagement/internals/shared/grpc/proxy_group_access_test.gomanagement/internals/shared/grpc/proxy_test.gomanagement/server/http/handler.gomanagement/server/store/sql_store.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.go
✅ Files skipped from review due to trivial changes (1)
- shared/management/http/api/types.gen.go
🚧 Files skipped from review as they are similar to previous changes (5)
- management/internals/modules/reverseproxy/domain/manager/manager_test.go
- management/internals/modules/reverseproxy/proxytoken/handler.go
- management/internals/modules/reverseproxy/proxy/proxy.go
- management/server/http/handler.go
- management/internals/shared/grpc/proxy.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/internals/shared/grpc/validate_session_test.go (1)
332-344:⚠️ Potential issue | 🔴 CriticalFix the stale
Heartbeatmock signature.
NewProxyServiceServernow uses aproxy.ManagerwhoseHeartbeattakes(ctx, proxyID, clusterAddress, ipAddress). This stub still has the old two-parameter form, so the helper no longer satisfies the interface and the integration test build will fail.Suggested fix
-func (m *testValidateSessionProxyManager) Heartbeat(_ context.Context, _ string) error { +func (m *testValidateSessionProxyManager) Heartbeat(_ context.Context, _, _, _ string) error { return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/shared/grpc/validate_session_test.go` around lines 332 - 344, The test stub testValidateSessionProxyManager no longer implements proxy.Manager because its Heartbeat method has the old two-parameter signature; update the Heartbeat method on testValidateSessionProxyManager to match the current interface used by NewProxyServiceServer by changing its signature to Heartbeat(ctx context.Context, proxyID, clusterAddress, ipAddress string) error and keep the body returning nil so the mock satisfies proxy.Manager again.
♻️ Duplicate comments (2)
management/internals/shared/grpc/proxy.go (2)
197-200:⚠️ Potential issue | 🟠 MajorDon’t continue past unexpected
GetAccountProxyerrors.Anything other than
NotFoundshould stop the BYOP registration path here. Right now a backend failure only gets logged, which can silently disable the 1-proxy-per-account check for that request.Based on learnings, methods returning
(T, error)should propagate errors and return early instead of continuing on a non-nil error.Suggested fix
existingProxy, err := s.proxyManager.GetAccountProxy(ctx, *accountID) if err != nil { - log.WithContext(ctx).Debugf("failed to get account proxy for %s: %v", *accountID, err) + sErr, isNbErr := nbstatus.FromError(err) + if !isNbErr || sErr.Type() != nbstatus.NotFound { + return status.Errorf(codes.Internal, "get account proxy: %v", err) + } } if existingProxy != nil && existingProxy.ID != proxyID {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/shared/grpc/proxy.go` around lines 197 - 200, The code logs but continues when s.proxyManager.GetAccountProxy(ctx, *accountID) returns a non-nil error; change this to treat any error other than a NotFound sentinel as fatal for the BYOP registration path by returning early with that error (or mapping it to the appropriate gRPC status) instead of proceeding. Locate the call to GetAccountProxy and the variables existingProxy/err, check the error value: if err != nil and not a NotFound error, log and return the error (do not continue); only allow continuation when err is nil or explicitly a NotFound result.
460-474:⚠️ Potential issue | 🔴 CriticalFilter each batch per connection before minting tokens.
The set check fixes the old
update.Mapping[0]issue, but it still sends the full batch to any BYOP proxy whose account appears anywhere inupdate.Mapping. A mixed-account batch will still leak other accounts’ mappings and fresh auth tokens.Suggested fix
- updateAccountIDs := make(map[string]struct{}) - for _, m := range update.Mapping { - if m.AccountId != "" { - updateAccountIDs[m.AccountId] = struct{}{} - } - } s.connectedProxies.Range(func(key, value interface{}) bool { conn := value.(*proxyConnection) - if conn.accountID != nil && len(updateAccountIDs) > 0 { - if _, ok := updateAccountIDs[*conn.accountID]; !ok { - return true - } + scoped := update + if conn.accountID != nil { + mappings := make([]*proto.ProxyMapping, 0, len(update.Mapping)) + for _, m := range update.Mapping { + if m.AccountId != *conn.accountID { + continue + } + mappings = append(mappings, m) + } + if len(mappings) == 0 { + return true + } + scoped = &proto.GetMappingUpdateResponse{ + Mapping: mappings, + InitialSyncComplete: update.GetInitialSyncComplete(), + } } - resp := s.perProxyMessage(update, conn.proxyID) + resp := s.perProxyMessage(scoped, conn.proxyID) if resp == nil { return true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/shared/grpc/proxy.go` around lines 460 - 474, The code currently builds updateAccountIDs but still calls perProxyMessage with the full update, which leaks mappings/tokens across accounts; change the loop over s.connectedProxies to build a per-connection filtered update that contains only mappings for that connection's account (use conn.accountID and filter update.Mapping into a new slice), skip sending if the filtered slice is empty for BYOP connections, and pass that filtered update into s.perProxyMessage (and any token-minting logic) so tokens/mappings are minted/sent only for that account; reference: update.Mapping, updateAccountIDs, s.connectedProxies, proxyConnection (conn.accountID, conn.proxyID), and perProxyMessage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/server/http/handler.go`:
- Line 78: The NewAPIHandler function signature includes an unused parameter
proxyMgr of type rpproxy.Manager; remove this unused parameter from the
NewAPIHandler declaration and any corresponding callers/signatures so the
function no longer declares proxyMgr (ensure to update all places that call
NewAPIHandler to match the new parameter list). Locate NewAPIHandler in the file
(symbol: NewAPIHandler) and delete the proxyMgr rpproxy.Manager parameter, then
run tests/compile to find and update any callers to remove their proxyMgr
argument.
In `@management/server/store/sql_store.go`:
- Around line 5455-5461: The isUniqueConstraintError in sql_store.go currently
only checks Postgres pgconn.PgError and generic strings; update it to also
detect MySQL duplicate-key errors the same way account.go does by checking the
error string for "Error 1062 (23000)" and/or "Duplicate entry"/"duplicate key"
patterns so MySQL save operations return proxy.ErrAccountProxyAlreadyExists;
also remove the duplicate implementations by extracting this logic into a shared
helper (e.g., a single isUniqueConstraintError utility used by both sql_store.go
and account.go) so future DB-specific checks live in one place.
In `@shared/management/http/api/openapi.yml`:
- Around line 10014-10042: Remove the duplicate DELETE operation for the path
"/api/reverse-proxies/clusters/{clusterId}" (the redundant mapping that mirrors
the existing DELETE at the other location) to avoid duplicate YAML keys; locate
the second occurrence of the DELETE under the path
"/api/reverse-proxies/clusters/{clusterId}" and delete that entire operation
block (including its summary, description, parameters and responses) so only the
original DELETE definition remains.
---
Outside diff comments:
In `@management/internals/shared/grpc/validate_session_test.go`:
- Around line 332-344: The test stub testValidateSessionProxyManager no longer
implements proxy.Manager because its Heartbeat method has the old two-parameter
signature; update the Heartbeat method on testValidateSessionProxyManager to
match the current interface used by NewProxyServiceServer by changing its
signature to Heartbeat(ctx context.Context, proxyID, clusterAddress, ipAddress
string) error and keep the body returning nil so the mock satisfies
proxy.Manager again.
---
Duplicate comments:
In `@management/internals/shared/grpc/proxy.go`:
- Around line 197-200: The code logs but continues when
s.proxyManager.GetAccountProxy(ctx, *accountID) returns a non-nil error; change
this to treat any error other than a NotFound sentinel as fatal for the BYOP
registration path by returning early with that error (or mapping it to the
appropriate gRPC status) instead of proceeding. Locate the call to
GetAccountProxy and the variables existingProxy/err, check the error value: if
err != nil and not a NotFound error, log and return the error (do not continue);
only allow continuation when err is nil or explicitly a NotFound result.
- Around line 460-474: The code currently builds updateAccountIDs but still
calls perProxyMessage with the full update, which leaks mappings/tokens across
accounts; change the loop over s.connectedProxies to build a per-connection
filtered update that contains only mappings for that connection's account (use
conn.accountID and filter update.Mapping into a new slice), skip sending if the
filtered slice is empty for BYOP connections, and pass that filtered update into
s.perProxyMessage (and any token-minting logic) so tokens/mappings are
minted/sent only for that account; reference: update.Mapping, updateAccountIDs,
s.connectedProxies, proxyConnection (conn.accountID, conn.proxyID), and
perProxyMessage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62b51e60-d838-4f24-8731-aa7c8740373b
📒 Files selected for processing (25)
management/internals/modules/reverseproxy/domain/manager/manager.gomanagement/internals/modules/reverseproxy/domain/manager/manager_test.gomanagement/internals/modules/reverseproxy/proxy/manager.gomanagement/internals/modules/reverseproxy/proxy/manager/manager.gomanagement/internals/modules/reverseproxy/proxy/manager/manager_test.gomanagement/internals/modules/reverseproxy/proxy/manager_mock.gomanagement/internals/modules/reverseproxy/proxy/proxy.gomanagement/internals/modules/reverseproxy/service/interface.gomanagement/internals/modules/reverseproxy/service/interface_mock.gomanagement/internals/modules/reverseproxy/service/manager/manager.gomanagement/internals/modules/reverseproxy/service/manager/manager_test.gomanagement/internals/server/boot.gomanagement/internals/shared/grpc/proxy.gomanagement/internals/shared/grpc/proxy_group_access_test.gomanagement/internals/shared/grpc/proxy_test.gomanagement/internals/shared/grpc/validate_session_test.gomanagement/server/http/handler.gomanagement/server/http/handlers/proxy/auth_callback_integration_test.gomanagement/server/http/testing/testing_tools/channel/channel.gomanagement/server/store/sql_store.gomanagement/server/store/store.gomanagement/server/store/store_mock.goproxy/management_integration_test.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.go
✅ Files skipped from review due to trivial changes (3)
- management/internals/modules/reverseproxy/domain/manager/manager_test.go
- management/internals/shared/grpc/proxy_test.go
- management/internals/modules/reverseproxy/proxy/manager/manager_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- management/internals/modules/reverseproxy/service/manager/manager.go
- management/internals/modules/reverseproxy/service/manager/manager_test.go
- management/server/http/testing/testing_tools/channel/channel.go
- management/internals/modules/reverseproxy/service/interface.go
- management/internals/modules/reverseproxy/proxy/proxy.go
- management/server/http/handlers/proxy/auth_callback_integration_test.go
- shared/management/http/api/types.gen.go
- management/internals/modules/reverseproxy/proxy/manager.go
- management/internals/modules/reverseproxy/proxy/manager/manager.go
- management/server/store/store_mock.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
proxy/management_integration_test.go (1)
135-144:⚠️ Potential issue | 🟠 MajorAvoid a nil token checker in this integration harness.
At Line 143, passing
nilfor the new token-checker dependency means this suite can pass without exercising token-validation paths introduced by this PR. That weakens confidence in the security-critical behavior.Use a test token checker (with allow/deny cases) and assert rejection on invalid token flows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/management_integration_test.go` around lines 135 - 144, The test currently passes nil for the token-checker into NewProxyServiceServer (call at proxyService := nbgrpc.NewProxyServiceServer(...)), which avoids exercising token-validation; replace that nil with a test token checker implementation (e.g., a small test helper that implements the expected TokenChecker interface with allow and deny branches) and instantiate it in the integration harness alongside tokenStore, pkceStore and oidcConfig; update tests to exercise both allowed and denied token flows by asserting proxied requests succeed with a valid token and are rejected (error/403) with an invalid token to ensure token-validation paths are exercised.
🧹 Nitpick comments (1)
proxy/management_integration_test.go (1)
205-247: Make account-scoped proxy-manager methods stateful for meaningful BYOP coverage.The new methods currently return fixed values (
CountAccountProxies=0,IsClusterAddressAvailable=true, empty account cluster data). This can mask regressions in account isolation and per-account proxy constraints.Consider backing these methods with in-memory maps keyed by
accountID/proxyIDso tests validate real behavior rather than permissive defaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/management_integration_test.go` around lines 205 - 247, The account-scoped test doubles return hardcoded values; make them stateful by adding in-memory storage (e.g., maps protected by a sync.Mutex) on testProxyManager and use those maps in GetAccountProxy, CountAccountProxies, IsClusterAddressAvailable, GetActiveClusterAddressesForAccount and DeleteProxy (and any account-related methods) so tests exercise real account isolation and constraints: add fields like accounts map[string]map[string]*nbproxy.Proxy or accountCounts map[string]int (and clusterAddresses map[string][]string), initialize them in the testProxyManager constructor, update Connect/Disconnect/Create/Delete operations to add/remove entries, and have the getters consult these maps under the mutex to return realistic values/errors rather than fixed defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@proxy/management_integration_test.go`:
- Around line 135-144: The test currently passes nil for the token-checker into
NewProxyServiceServer (call at proxyService :=
nbgrpc.NewProxyServiceServer(...)), which avoids exercising token-validation;
replace that nil with a test token checker implementation (e.g., a small test
helper that implements the expected TokenChecker interface with allow and deny
branches) and instantiate it in the integration harness alongside tokenStore,
pkceStore and oidcConfig; update tests to exercise both allowed and denied token
flows by asserting proxied requests succeed with a valid token and are rejected
(error/403) with an invalid token to ensure token-validation paths are
exercised.
---
Nitpick comments:
In `@proxy/management_integration_test.go`:
- Around line 205-247: The account-scoped test doubles return hardcoded values;
make them stateful by adding in-memory storage (e.g., maps protected by a
sync.Mutex) on testProxyManager and use those maps in GetAccountProxy,
CountAccountProxies, IsClusterAddressAvailable,
GetActiveClusterAddressesForAccount and DeleteProxy (and any account-related
methods) so tests exercise real account isolation and constraints: add fields
like accounts map[string]map[string]*nbproxy.Proxy or accountCounts
map[string]int (and clusterAddresses map[string][]string), initialize them in
the testProxyManager constructor, update Connect/Disconnect/Create/Delete
operations to add/remove entries, and have the getters consult these maps under
the mutex to return realistic values/errors rather than fixed defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25648bd7-dd6f-4d76-81b8-3a81e241c569
📒 Files selected for processing (2)
management/internals/modules/reverseproxy/proxy/manager/manager_test.goproxy/management_integration_test.go
✅ Files skipped from review due to trivial changes (1)
- management/internals/modules/reverseproxy/proxy/manager/manager_test.go
# Conflicts: # management/internals/modules/reverseproxy/domain/manager/manager.go # management/internals/modules/reverseproxy/proxy/manager.go # management/internals/modules/reverseproxy/proxy/manager/manager.go # management/internals/modules/reverseproxy/proxy/manager_mock.go # management/internals/shared/grpc/proxy.go # management/server/store/sql_store.go # proxy/management_integration_test.go
pascal-fischer
requested changes
Apr 27, 2026
Comment on lines
+4502
to
+4503
| tx := s.db.WithContext(ctx) | ||
| if lockStrength != LockingStrengthNone { |
| } | ||
|
|
||
| func (s *SqlStore) GetProxyAccessTokenByID(ctx context.Context, lockStrength LockingStrength, tokenID string) (*types.ProxyAccessToken, error) { | ||
| tx := s.db.WithContext(ctx) |
|
|
||
| func (s *SqlStore) DisconnectProxy(ctx context.Context, proxyID string) error { | ||
| now := time.Now() | ||
| result := s.db.WithContext(ctx). |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
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
New Features
API Changes