[proxy] Send proxy updates on account delete#5375
[proxy] Send proxy updates on account delete#5375pascal-fischer wants to merge 9 commits intomainfrom
Conversation
|
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 Manager.DeleteAllServices and store support to fetch all account services, converts reverse-proxy streaming messages from Changes
Sequence Diagram(s)sequenceDiagram
participant M as Manager
participant S as ProxyServiceServer
participant C as Cluster
participant P as Proxy
M->>M: gather services grouped by cluster
M->>S: Send GetMappingUpdateResponse (per-cluster batch)
S->>S: perProxyMessage — generate per-proxy tokens, build Mapping[]
S->>C: deliver GetMappingUpdateResponse to cluster
C->>P: distribute mapping update to proxies
P-->>C: ack
C-->>S: ack
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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.
Actionable comments posted: 4
🧹 Nitpick comments (5)
management/server/store/store.go (1)
259-259: Consider consolidating with GetAccountServices to avoid API duplication.GetServicesByAccountID appears to overlap with the existing GetAccountServices signature. If both are required, add a brief doc comment clarifying the behavioral difference; otherwise, consider aliasing or deprecating one to keep the interface surface minimal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/store/store.go` at line 259, GetServicesByAccountID overlaps GetAccountServices; either consolidate by removing one and making the remaining function serve both callers (rename callers to use GetAccountServices or add an alias that calls GetAccountServices), or if both must exist, add a clear doc comment on GetServicesByAccountID explaining the behavioral difference (locking behavior, return values, or error semantics) relative to GetAccountServices; update any callers to use the canonical function (or the alias) and ensure tests reflect the chosen consolidation.management/server/store/sql_store.go (1)
4909-4929: Deduplicate service-loading logic to avoid drift.This implementation mirrors GetAccountServices. Delegating keeps behavior consistent and reduces maintenance.
♻️ Suggested refactor
func (s *SqlStore) GetServicesByAccountID(ctx context.Context, lockStrength LockingStrength, accountID string) ([]*reverseproxy.Service, error) { - tx := s.db.Preload("Targets") - if lockStrength != LockingStrengthNone { - tx = tx.Clauses(clause.Locking{Strength: string(lockStrength)}) - } - - var serviceList []*reverseproxy.Service - result := tx.Find(&serviceList, accountIDCondition, accountID) - if result.Error != nil { - log.WithContext(ctx).Errorf("failed to get services from the store: %s", result.Error) - return nil, status.Errorf(status.Internal, "failed to get services from store") - } - - for _, service := range serviceList { - if err := service.DecryptSensitiveData(s.fieldEncrypt); err != nil { - return nil, fmt.Errorf("decrypt service data: %w", err) - } - } - - return serviceList, nil + return s.GetAccountServices(ctx, lockStrength, accountID) }🤖 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 4909 - 4929, GetServicesByAccountID duplicates the logic in GetAccountServices; refactor it to delegate to the existing implementation to avoid drift. Replace the body of GetServicesByAccountID so it simply calls s.GetAccountServices(ctx, lockStrength, accountID) (or the appropriate exported/internal signature) and returns its results, preserving the same LockingStrength and accountID parameters and propagating any errors; ensure you remove the duplicated DB Preload/Find and decryption logic from GetServicesByAccountID so all service-loading behavior lives in GetAccountServices.management/internals/modules/reverseproxy/manager/manager.go (2)
347-351:oldServiceparameter insendServiceUpdateis misleading and always"".The second positional argument to
service.ToProtoMappingis an auth token, not an old service ID (compare withproxy.go's snapshot path:service.ToProtoMapping(reverseproxy.Create, token, ...)). Every call site passes""foroldService, making it a dead parameter with a name that implies a service-rename semantic.Either drop the parameter and hard-code
"", or rename it totokento matchToProtoMapping's actual contract:♻️ Proposed fix
-func (m *managerImpl) sendServiceUpdate(service *reverseproxy.Service, operation reverseproxy.Operation, cluster, oldService string) { +func (m *managerImpl) sendServiceUpdate(service *reverseproxy.Service, operation reverseproxy.Operation, cluster string) { oidcCfg := m.proxyGRPCServer.GetOIDCValidationConfig() - mapping := service.ToProtoMapping(operation, oldService, oidcCfg) + mapping := service.ToProtoMapping(operation, "", oidcCfg) m.sendMappingsToCluster([]*proto.ProxyMapping{mapping}, cluster) }All call sites drop the trailing
""argument accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/manager/manager.go` around lines 347 - 351, The parameter named oldService on managerImpl.sendServiceUpdate is misleading (ToProtoMapping expects an auth token) — rename the parameter to token (and the local variable usage) so the signature becomes sendServiceUpdate(service *reverseproxy.Service, operation reverseproxy.Operation, cluster, token string) and update all call sites that pass "" to pass token (or leave "" where appropriate); ensure the call to service.ToProtoMapping(operation, token, oidcCfg) reflects the renamed parameter and that any documentation/comments are updated to reflect it represents an auth token, not an old service ID.
439-443:DeleteAllServicesissues N individualDeleteServicecalls inside a single transaction — consider a bulk delete.For accounts with many services, this results in N round-trips inside the transaction. A single
WHERE account_id = ?delete query would be more efficient and reduces lock contention. TheGetServicesByAccountID→ individual-delete pattern also means the lock is held for longer.Would you like me to draft a
DeleteAllServicesByAccountID(ctx, accountID)store method (and its mock counterpart) that executes a single bulk-delete query, removing the need for the loop here?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/manager/manager.go` around lines 439 - 443, DeleteAllServices currently fetches services then calls transaction.DeleteService repeatedly, causing N round-trips and long locks; replace this with a single bulk delete in the store. Add a new store method DeleteAllServicesByAccountID(ctx, accountID) (and update its mock) that runs one DELETE FROM ... WHERE account_id = ? query and returns any error, then modify the manager's DeleteAllServices to call transaction.DeleteAllServicesByAccountID(ctx, accountID) instead of looping over GetServicesByAccountID/transaction.DeleteService to eliminate the per-service loop and reduce lock contention.management/internals/shared/grpc/proxy_test.go (1)
43-101: No test coverage for batched multi-mapping responses — the core new behavior.All three test functions send a
GetMappingUpdateResponsewith exactly oneProxyMapping. The key behavioral change in this PR is thatperProxyMessagenow iterates theMappingslice and generates a per-mapping, per-proxy token for every non-delete entry. A test with two or more non-delete mappings in a single response would verify that:
- Each mapping in the received response has a distinct token from the others.
- Tokens for the same mapping index across proxies are distinct.
- All generated tokens are independently consumable.
This is the logic path that
DeleteAllServicesandReloadAllServicesForAccountnow exercise in production.Would you like me to generate a
TestSendServiceUpdateToCluster_BatchedMappingsUniqueTokenstest?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/shared/grpc/proxy_test.go` around lines 43 - 101, Add a new unit test (e.g., TestSendServiceUpdateToCluster_BatchedMappingsUniqueTokens) that uses NewOneTimeTokenStore and a ProxyServiceServer, registers multiple fake proxies via registerFakeProxy, then sends a single GetMappingUpdateResponse containing multiple non-delete ProxyMapping entries to s.SendServiceUpdateToCluster; for each proxy received (via drainChannel) assert each mapping has a non-empty distinct AuthToken within that proxy message and that tokens for the same mapping index across different proxies are distinct, then call tokenStore.ValidateAndConsume for every token+account+service combination to ensure each token is independently consumable; reference tokenStore, ProxyServiceServer, registerFakeProxy, SendServiceUpdateToCluster, drainChannel, and ValidateAndConsume in the test.
🤖 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/shared/grpc/proxy.go`:
- Around line 457-479: In perProxyMessage, avoid aborting the whole batch on a
single token generation error: when tokenStore.GenerateToken(mapping.AccountId,
mapping.Id, 5*time.Minute) fails, log the error and skip that mapping (do not
return nil), continue processing the rest, and only return nil if the resulting
resp slice is empty; keep using shallowCloneMapping to copy entries and set
msg.AuthToken for successful tokens so
SendServiceUpdate/SendServiceUpdateToCluster still receives partial batches
rather than discarding all mappings.
- Line 65: Remove the unused field updatesChan from the proxy server struct and
its initialization in NewProxyServiceServer; specifically, delete the
updatesChan field declaration and the code that allocates it (since
SendServiceUpdate and SendServiceUpdateToCluster already write to each
conn.sendChan and the sender goroutine consumes conn.sendChan). Ensure no other
code references updatesChan and run tests/build to confirm removal is safe; use
symbol names updatesChan, NewProxyServiceServer, SendServiceUpdate,
SendServiceUpdateToCluster, conn.sendChan, and sender to locate the related
code.
In `@management/server/account.go`:
- Around line 717-720: Guard against a nil reverseProxyManager before calling
DeleteAllServices: update the account deletion flow (where
am.reverseProxyManager.DeleteAllServices(ctx, accountID, userID) is called) to
first check if am.reverseProxyManager != nil and only call DeleteAllServices
when configured; if nil, skip the call. Also fix the error message returned from
status.Errorf to accurately describe the operation (e.g., "failed to delete
reverse proxy services for account %s: %v") and keep using accountID and err for
context.
In `@management/server/store/store_mock.go`:
- Around line 1097-1110: Duplicate API methods GetServicesByAccountID and
GetAccountServices exist; remove the redundant GetServicesByAccountID
implementation and its mock helpers to consolidate on a single method name
(choose one, e.g., GetAccountServices). Update the Store interface to only
declare the chosen method, delete MockStore.GetServicesByAccountID and
MockStoreMockRecorder.GetServicesByAccountID from store_mock.go, and update any
callers/tests to call the remaining method (GetAccountServices) and regenerate
mocks so gomock reflect types remain consistent.
---
Nitpick comments:
In `@management/internals/modules/reverseproxy/manager/manager.go`:
- Around line 347-351: The parameter named oldService on
managerImpl.sendServiceUpdate is misleading (ToProtoMapping expects an auth
token) — rename the parameter to token (and the local variable usage) so the
signature becomes sendServiceUpdate(service *reverseproxy.Service, operation
reverseproxy.Operation, cluster, token string) and update all call sites that
pass "" to pass token (or leave "" where appropriate); ensure the call to
service.ToProtoMapping(operation, token, oidcCfg) reflects the renamed parameter
and that any documentation/comments are updated to reflect it represents an auth
token, not an old service ID.
- Around line 439-443: DeleteAllServices currently fetches services then calls
transaction.DeleteService repeatedly, causing N round-trips and long locks;
replace this with a single bulk delete in the store. Add a new store method
DeleteAllServicesByAccountID(ctx, accountID) (and update its mock) that runs one
DELETE FROM ... WHERE account_id = ? query and returns any error, then modify
the manager's DeleteAllServices to call
transaction.DeleteAllServicesByAccountID(ctx, accountID) instead of looping over
GetServicesByAccountID/transaction.DeleteService to eliminate the per-service
loop and reduce lock contention.
In `@management/internals/shared/grpc/proxy_test.go`:
- Around line 43-101: Add a new unit test (e.g.,
TestSendServiceUpdateToCluster_BatchedMappingsUniqueTokens) that uses
NewOneTimeTokenStore and a ProxyServiceServer, registers multiple fake proxies
via registerFakeProxy, then sends a single GetMappingUpdateResponse containing
multiple non-delete ProxyMapping entries to s.SendServiceUpdateToCluster; for
each proxy received (via drainChannel) assert each mapping has a non-empty
distinct AuthToken within that proxy message and that tokens for the same
mapping index across different proxies are distinct, then call
tokenStore.ValidateAndConsume for every token+account+service combination to
ensure each token is independently consumable; reference tokenStore,
ProxyServiceServer, registerFakeProxy, SendServiceUpdateToCluster, drainChannel,
and ValidateAndConsume in the test.
In `@management/server/store/sql_store.go`:
- Around line 4909-4929: GetServicesByAccountID duplicates the logic in
GetAccountServices; refactor it to delegate to the existing implementation to
avoid drift. Replace the body of GetServicesByAccountID so it simply calls
s.GetAccountServices(ctx, lockStrength, accountID) (or the appropriate
exported/internal signature) and returns its results, preserving the same
LockingStrength and accountID parameters and propagating any errors; ensure you
remove the duplicated DB Preload/Find and decryption logic from
GetServicesByAccountID so all service-loading behavior lives in
GetAccountServices.
In `@management/server/store/store.go`:
- Line 259: GetServicesByAccountID overlaps GetAccountServices; either
consolidate by removing one and making the remaining function serve both callers
(rename callers to use GetAccountServices or add an alias that calls
GetAccountServices), or if both must exist, add a clear doc comment on
GetServicesByAccountID explaining the behavioral difference (locking behavior,
return values, or error semantics) relative to GetAccountServices; update any
callers to use the canonical function (or the alias) and ensure tests reflect
the chosen consolidation.
|
|
||
| // Channel for broadcasting reverse proxy updates to all proxies | ||
| updatesChan chan *proto.ProxyMapping | ||
| updatesChan chan *proto.GetMappingUpdateResponse |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "updatesChan" --type=goRepository: netbirdio/netbird
Length of output: 271
🏁 Script executed:
sed -n '60,70p' management/internals/shared/grpc/proxy.go
sed -n '108,118p' management/internals/shared/grpc/proxy.goRepository: netbirdio/netbird
Length of output: 942
Remove unused updatesChan field — dead code.
updatesChan is declared (line 65) and initialized (line 113) in NewProxyServiceServer, but is never written to or read from anywhere in the codebase. Both SendServiceUpdate and SendServiceUpdateToCluster write directly to each conn.sendChan; the sender goroutine reads from conn.sendChan. The field should be removed.
♻️ Proposed fix
type ProxyServiceServer struct {
proto.UnimplementedProxyServiceServer
connectedProxies sync.Map
clusterProxies sync.Map
- // Channel for broadcasting reverse proxy updates to all proxies
- updatesChan chan *proto.GetMappingUpdateResponse
// Manager for access logs
accessLogManager accesslogs.Manager s := &ProxyServiceServer{
- updatesChan: make(chan *proto.GetMappingUpdateResponse, 100),
accessLogManager: accessLogMgr,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/internals/shared/grpc/proxy.go` at line 65, Remove the unused
field updatesChan from the proxy server struct and its initialization in
NewProxyServiceServer; specifically, delete the updatesChan field declaration
and the code that allocates it (since SendServiceUpdate and
SendServiceUpdateToCluster already write to each conn.sendChan and the sender
goroutine consumes conn.sendChan). Ensure no other code references updatesChan
and run tests/build to confirm removal is safe; use symbol names updatesChan,
NewProxyServiceServer, SendServiceUpdate, SendServiceUpdateToCluster,
conn.sendChan, and sender to locate the related code.
| func (s *ProxyServiceServer) perProxyMessage(update *proto.GetMappingUpdateResponse, proxyID string) *proto.GetMappingUpdateResponse { | ||
| resp := make([]*proto.ProxyMapping, 0, len(update.Mapping)) | ||
| for _, mapping := range update.Mapping { | ||
| if mapping.Type == proto.ProxyMappingUpdateType_UPDATE_TYPE_REMOVED { | ||
| resp = append(resp, mapping) | ||
| continue | ||
| } | ||
|
|
||
| token, err := s.tokenStore.GenerateToken(update.AccountId, update.Id, 5*time.Minute) | ||
| if err != nil { | ||
| log.Warnf("Failed to generate token for proxy %s: %v", proxyID, err) | ||
| return nil | ||
| token, err := s.tokenStore.GenerateToken(mapping.AccountId, mapping.Id, 5*time.Minute) | ||
| if err != nil { | ||
| log.Warnf("Failed to generate token for proxy %s: %v", proxyID, err) | ||
| return nil | ||
| } | ||
|
|
||
| msg := shallowCloneMapping(mapping) | ||
| msg.AuthToken = token | ||
| resp = append(resp, msg) | ||
| } | ||
|
|
||
| msg := shallowCloneMapping(update) | ||
| msg.AuthToken = token | ||
| return msg | ||
| return &proto.GetMappingUpdateResponse{ | ||
| Mapping: resp, | ||
| } | ||
| } |
There was a problem hiding this comment.
perProxyMessage: a single token-generation failure silently drops the entire batch for that proxy.
The early return nil on line 468 causes SendServiceUpdate/SendServiceUpdateToCluster to skip the proxy entirely, discarding all other mappings in the batch that may have been processed successfully. Before this PR, each mapping was dispatched in its own GetMappingUpdateResponse, so a failure only lost one service update. With batching (e.g. DeleteAllServices deletes N services in one shot), a transient token-store error now silently loses all N delete notifications for one proxy.
Consider continuing to build the response with successfully tokenized mappings and only skipping the failed entry, returning nil only when resp ends up empty:
🛡️ Proposed fix
func (s *ProxyServiceServer) perProxyMessage(update *proto.GetMappingUpdateResponse, proxyID string) *proto.GetMappingUpdateResponse {
resp := make([]*proto.ProxyMapping, 0, len(update.Mapping))
for _, mapping := range update.Mapping {
if mapping.Type == proto.ProxyMappingUpdateType_UPDATE_TYPE_REMOVED {
resp = append(resp, mapping)
continue
}
token, err := s.tokenStore.GenerateToken(mapping.AccountId, mapping.Id, 5*time.Minute)
if err != nil {
log.Warnf("Failed to generate token for proxy %s mapping %s: %v", proxyID, mapping.Id, err)
- return nil
+ continue
}
msg := shallowCloneMapping(mapping)
msg.AuthToken = token
resp = append(resp, msg)
}
+ if len(resp) == 0 {
+ return nil
+ }
return &proto.GetMappingUpdateResponse{
Mapping: resp,
}
}📝 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.
| func (s *ProxyServiceServer) perProxyMessage(update *proto.GetMappingUpdateResponse, proxyID string) *proto.GetMappingUpdateResponse { | |
| resp := make([]*proto.ProxyMapping, 0, len(update.Mapping)) | |
| for _, mapping := range update.Mapping { | |
| if mapping.Type == proto.ProxyMappingUpdateType_UPDATE_TYPE_REMOVED { | |
| resp = append(resp, mapping) | |
| continue | |
| } | |
| token, err := s.tokenStore.GenerateToken(update.AccountId, update.Id, 5*time.Minute) | |
| if err != nil { | |
| log.Warnf("Failed to generate token for proxy %s: %v", proxyID, err) | |
| return nil | |
| token, err := s.tokenStore.GenerateToken(mapping.AccountId, mapping.Id, 5*time.Minute) | |
| if err != nil { | |
| log.Warnf("Failed to generate token for proxy %s: %v", proxyID, err) | |
| return nil | |
| } | |
| msg := shallowCloneMapping(mapping) | |
| msg.AuthToken = token | |
| resp = append(resp, msg) | |
| } | |
| msg := shallowCloneMapping(update) | |
| msg.AuthToken = token | |
| return msg | |
| return &proto.GetMappingUpdateResponse{ | |
| Mapping: resp, | |
| } | |
| } | |
| func (s *ProxyServiceServer) perProxyMessage(update *proto.GetMappingUpdateResponse, proxyID string) *proto.GetMappingUpdateResponse { | |
| resp := make([]*proto.ProxyMapping, 0, len(update.Mapping)) | |
| for _, mapping := range update.Mapping { | |
| if mapping.Type == proto.ProxyMappingUpdateType_UPDATE_TYPE_REMOVED { | |
| resp = append(resp, mapping) | |
| continue | |
| } | |
| token, err := s.tokenStore.GenerateToken(mapping.AccountId, mapping.Id, 5*time.Minute) | |
| if err != nil { | |
| log.Warnf("Failed to generate token for proxy %s mapping %s: %v", proxyID, mapping.Id, err) | |
| continue | |
| } | |
| msg := shallowCloneMapping(mapping) | |
| msg.AuthToken = token | |
| resp = append(resp, msg) | |
| } | |
| if len(resp) == 0 { | |
| return nil | |
| } | |
| return &proto.GetMappingUpdateResponse{ | |
| Mapping: resp, | |
| } | |
| } |
🤖 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 457 - 479, In
perProxyMessage, avoid aborting the whole batch on a single token generation
error: when tokenStore.GenerateToken(mapping.AccountId, mapping.Id,
5*time.Minute) fails, log the error and skip that mapping (do not return nil),
continue processing the rest, and only return nil if the resulting resp slice is
empty; keep using shallowCloneMapping to copy entries and set msg.AuthToken for
successful tokens so SendServiceUpdate/SendServiceUpdateToCluster still receives
partial batches rather than discarding all mappings.
| err = am.reverseProxyManager.DeleteAllServices(ctx, accountID, userID) | ||
| if err != nil { | ||
| return status.Errorf(status.Internal, "failed to delete service %s: %v", accountID, err) | ||
| } |
There was a problem hiding this comment.
Guard against a nil reverseProxyManager before calling DeleteAllServices.
If reverseProxyManager isn’t configured (e.g., module disabled or not initialized in tests), this will panic during account deletion. Also, the error message reads like a single service deletion but uses the account ID.
🔧 Suggested fix
- err = am.reverseProxyManager.DeleteAllServices(ctx, accountID, userID)
- if err != nil {
- return status.Errorf(status.Internal, "failed to delete service %s: %v", accountID, err)
- }
+ if am.reverseProxyManager != nil {
+ err = am.reverseProxyManager.DeleteAllServices(ctx, accountID, userID)
+ if err != nil {
+ return status.Errorf(status.Internal, "failed to delete reverse proxy services for account %s: %v", accountID, err)
+ }
+ }📝 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.
| err = am.reverseProxyManager.DeleteAllServices(ctx, accountID, userID) | |
| if err != nil { | |
| return status.Errorf(status.Internal, "failed to delete service %s: %v", accountID, err) | |
| } | |
| if am.reverseProxyManager != nil { | |
| err = am.reverseProxyManager.DeleteAllServices(ctx, accountID, userID) | |
| if err != nil { | |
| return status.Errorf(status.Internal, "failed to delete reverse proxy services for account %s: %v", accountID, err) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/server/account.go` around lines 717 - 720, Guard against a nil
reverseProxyManager before calling DeleteAllServices: update the account
deletion flow (where am.reverseProxyManager.DeleteAllServices(ctx, accountID,
userID) is called) to first check if am.reverseProxyManager != nil and only call
DeleteAllServices when configured; if nil, skip the call. Also fix the error
message returned from status.Errorf to accurately describe the operation (e.g.,
"failed to delete reverse proxy services for account %s: %v") and keep using
accountID and err for context.
| // GetServicesByAccountID mocks base method. | ||
| func (m *MockStore) GetServicesByAccountID(ctx context.Context, lockStrength LockingStrength, accountID string) ([]*reverseproxy.Service, error) { | ||
| m.ctrl.T.Helper() | ||
| ret := m.ctrl.Call(m, "GetServicesByAccountID", ctx, lockStrength, accountID) | ||
| ret0, _ := ret[0].([]*reverseproxy.Service) | ||
| ret1, _ := ret[1].(error) | ||
| return ret0, ret1 | ||
| } | ||
|
|
||
| // GetServicesByAccountID indicates an expected call of GetServicesByAccountID. | ||
| func (mr *MockStoreMockRecorder) GetServicesByAccountID(ctx, lockStrength, accountID interface{}) *gomock.Call { | ||
| mr.mock.ctrl.T.Helper() | ||
| return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetServicesByAccountID", reflect.TypeOf((*MockStore)(nil).GetServicesByAccountID), ctx, lockStrength, accountID) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
rg -n "func.*GetServicesByAccountID|func.*GetAccountServices" --type=go -A 10 management/server/store/Repository: netbirdio/netbird
Length of output: 5484
🏁 Script executed:
rg -n "\.GetServicesByAccountID|\.GetAccountServices" --type=go -B 2 -A 2Repository: netbirdio/netbird
Length of output: 7321
🏁 Script executed:
sed -n '4909,4930p' management/server/store/sql_store.goRepository: netbirdio/netbird
Length of output: 860
🏁 Script executed:
sed -n '4972,4993p' management/server/store/sql_store.goRepository: netbirdio/netbird
Length of output: 856
Remove duplicate method GetServicesByAccountID — it is functionally identical to GetAccountServices.
Both GetServicesByAccountID and GetAccountServices have identical implementations in sql_store.go (lines 4909 and 4972). They perform the same database query, locking, decryption, and error handling. The only difference is naming convention and usage context (read vs. transaction), not actual behavior. Having two identically-implemented methods with different names in the Store interface unnecessarily duplicates the API surface and creates maintenance burden. Consolidate to a single method.
🤖 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 1097 - 1110, Duplicate
API methods GetServicesByAccountID and GetAccountServices exist; remove the
redundant GetServicesByAccountID implementation and its mock helpers to
consolidate on a single method name (choose one, e.g., GetAccountServices).
Update the Store interface to only declare the chosen method, delete
MockStore.GetServicesByAccountID and
MockStoreMockRecorder.GetServicesByAccountID from store_mock.go, and update any
callers/tests to call the remaining method (GetAccountServices) and regenerate
mocks so gomock reflect types remain consistent.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
management/internals/modules/reverseproxy/interface_mock.go (1)
52-64: LGTM — new mock follows established patterns correctly.The
DeleteAllServicesmock and its recorder are structurally identical to the other single-error-return mocked methods (e.g.,DeleteService,ReloadService). Argument count inctrl.Call(3) andRecordCallWithMethodType(3 variadic) matches the interface signature, and thereflect.TypeOf((*MockManager)(nil).DeleteAllServices)type-capture idiom is standard gomock v1.6.0 usage.One optional process note: since this file carries
// Code generated by MockGen. DO NOT EDIT., future interface changes should ideally be followed by regenerating the mock viago generate(or the equivalentmockgeninvocation) to keep it in sync and avoid manual-edit drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/interface_mock.go` around lines 52 - 64, No changes required to the DeleteAllServices mock implementation itself—DeleteAllServices on MockManager and its recorder MockManagerMockRecorder correctly follow the gomock pattern—but ensure future interface updates are propagated by regenerating the mocks (run mockgen or go generate for the MockGen-generated file) so DeleteAllServices, MockManager and MockManagerMockRecorder stay in sync with the real interface and avoid manual-edit drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@management/internals/modules/reverseproxy/interface_mock.go`:
- Around line 52-64: No changes required to the DeleteAllServices mock
implementation itself—DeleteAllServices on MockManager and its recorder
MockManagerMockRecorder correctly follow the gomock pattern—but ensure future
interface updates are propagated by regenerating the mocks (run mockgen or go
generate for the MockGen-generated file) so DeleteAllServices, MockManager and
MockManagerMockRecorder stay in sync with the real interface and avoid
manual-edit drift.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
management/internals/shared/grpc/proxy_group_access_test.go (1)
20-22: No test cases exercise theDeleteAllServicespath.The mock satisfies the interface, but neither
TestValidateUserGroupAccessnorTestGetAccountProxyByDomaininvokes — or exercises an error path through —DeleteAllServices. If theaccount.godeletion flow is the primary consumer of this new method, consider adding a dedicated test (e.g., inproxy_test.goor a new table-driven case here) that verifies the bulk-delete call propagates correctly, including the error path viam.err.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/shared/grpc/proxy_group_access_test.go` around lines 20 - 22, Add a test that exercises mockReverseProxyManager.DeleteAllServices (both success and error paths) so the account deletion flow calls and propagates its result; update or add a table-driven case in proxy_group_access_test.go (or add a new proxy_test.go) that triggers the account.go deletion logic and asserts DeleteAllServices is invoked and that when mockReverseProxyManager.m.err is set the error bubbles up; specifically reference the mock method DeleteAllServices and the existing tests TestValidateUserGroupAccess and TestGetAccountProxyByDomain as locations to mirror the pattern and assert proper error propagation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@management/internals/shared/grpc/proxy_group_access_test.go`:
- Around line 20-22: Add a test that exercises
mockReverseProxyManager.DeleteAllServices (both success and error paths) so the
account deletion flow calls and propagates its result; update or add a
table-driven case in proxy_group_access_test.go (or add a new proxy_test.go)
that triggers the account.go deletion logic and asserts DeleteAllServices is
invoked and that when mockReverseProxyManager.m.err is set the error bubbles up;
specifically reference the mock method DeleteAllServices and the existing tests
TestValidateUserGroupAccess and TestGetAccountProxyByDomain as locations to
mirror the pattern and assert proper error propagation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/account_test.go`:
- Around line 3126-3127: The test creates a ProxyServiceServer via
nbgrpc.NewProxyServiceServer and never stops its background goroutine; call
proxyGrpcServer.Close() during test teardown (or t.Cleanup) after creating
proxyGrpcServer and before calling
manager.SetServiceManager(reverseproxymanager.NewManager(...)) to avoid leaks;
additionally replace the nil accessLogManager/tokenStore/usersManager args with
lightweight no-op fakes (implementing SendAccessLog, ValidateSession,
ValidateUserGroupAccess) or safe mocks so future calls won't panic.
| proxyGrpcServer := nbgrpc.NewProxyServiceServer(nil, nil, nbgrpc.ProxyOIDCConfig{}, peersManager, nil) | ||
| manager.SetServiceManager(reverseproxymanager.NewManager(store, manager, permissionsManager, proxyGrpcServer, nil)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect ProxyServiceServer API and nil-usage patterns
rg -n --context 3 "type ProxyServiceServer" management/internals/shared/grpc/proxy.go
rg -n --context 3 "func \\(.*\\*ProxyServiceServer\\)" management/internals/shared/grpc/proxy.go
rg -n --context 3 "accessLogManager|tokenStore|usersManager" management/internals/shared/grpc/proxy.go
rg -n --context 3 "NewProxyServiceServer" -g'*.go'Repository: netbirdio/netbird
Length of output: 19256
🏁 Script executed:
#!/bin/bash
# Find createManager function and its test usage
rg -n "func createManager|createManager\(" management/server/account_test.go | head -20
rg -n "defer.*Close|proxyGrpcServer.Close" management/server/account_test.go
# Check if any of the nil-sensitive methods are called in tests
rg -n "SendAccessLog|ValidateUserGroupAccess|ValidateSession|SendStatusUpdate" management/server/account_test.go
# Check delete account flows
rg -n "delete.*account|DeleteAccount" management/server/account_test.go | head -20Repository: netbirdio/netbird
Length of output: 1485
🏁 Script executed:
#!/bin/bash
# Check DeleteAccount implementation
rg -n "func.*DeleteAccount" management/server/account.go
# Look for the actual implementation
sed -n '856,920p' management/server/account_test.go
# Check if reverse proxy operations are involved in delete
rg -n "reverseProxyManager|ServiceManager" management/server/account.go | head -30Repository: netbirdio/netbird
Length of output: 2280
🏁 Script executed:
#!/bin/bash
# Check DeleteAllServices implementation in reverse proxy manager
rg -n "func.*DeleteAllServices" -A 30
# Also check if it accesses the proxy service server methods
rg -n "tokenStore|accessLogManager|usersManager" management/internals/business/reverseproxy/manager.go | head -20Repository: netbirdio/netbird
Length of output: 13107
🏁 Script executed:
#!/bin/bash
# Get the complete DeleteAllServices implementation
sed -n '422,480p' management/internals/modules/reverseproxy/manager/manager.goRepository: netbirdio/netbird
Length of output: 2150
Add cleanup for ProxyServiceServer background goroutine.
NewProxyServiceServer starts a background cleanup goroutine that is never stopped. The Close() method exists and should be called during test teardown. Since createManager is used in 20+ tests without cleanup, this causes goroutine leaks.
The nil parameters (accessLogManager, tokenStore, usersManager) passed in the test are a latent risk—they would panic if methods like SendAccessLog, ValidateSession, or ValidateUserGroupAccess are invoked, though the current delete-account flow does not directly trigger them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/server/account_test.go` around lines 3126 - 3127, The test creates
a ProxyServiceServer via nbgrpc.NewProxyServiceServer and never stops its
background goroutine; call proxyGrpcServer.Close() during test teardown (or
t.Cleanup) after creating proxyGrpcServer and before calling
manager.SetServiceManager(reverseproxymanager.NewManager(...)) to avoid leaks;
additionally replace the nil accessLogManager/tokenStore/usersManager args with
lightweight no-op fakes (implementing SendAccessLog, ValidateSession,
ValidateUserGroupAccess) or safe mocks so future calls won't panic.
|
1 participant
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
Performance / Efficiency
Refactor
Tests