From 4fdc39c8f804de7b5f6397f67c91d678a8ad7527 Mon Sep 17 00:00:00 2001 From: crn4 Date: Tue, 24 Mar 2026 15:37:31 +0100 Subject: [PATCH] review comments --- management/internals/server/boot.go | 2 +- management/internals/shared/grpc/proxy.go | 27 +++++++++++++++-- .../shared/grpc/validate_session_test.go | 2 +- management/server/http/handler.go | 3 +- .../testing/testing_tools/channel/channel.go | 2 +- management/server/store/sql_store.go | 5 +++- shared/management/http/api/openapi.yml | 29 ------------------- 7 files changed, 33 insertions(+), 37 deletions(-) diff --git a/management/internals/server/boot.go b/management/internals/server/boot.go index 255cb1b7d..f7161637a 100644 --- a/management/internals/server/boot.go +++ b/management/internals/server/boot.go @@ -95,7 +95,7 @@ func (s *BaseServer) EventStore() activity.Store { func (s *BaseServer) APIHandler() http.Handler { return Create(s, func() http.Handler { - httpAPIHandler, err := nbhttp.NewAPIHandler(context.Background(), s.AccountManager(), s.NetworksManager(), s.ResourcesManager(), s.RoutesManager(), s.GroupsManager(), s.GeoLocationManager(), s.AuthManager(), s.Metrics(), s.IntegratedValidator(), s.ProxyController(), s.PermissionsManager(), s.PeersManager(), s.SettingsManager(), s.ZonesManager(), s.RecordsManager(), s.NetworkMapController(), s.IdpManager(), s.ServiceManager(), s.ReverseProxyDomainManager(), s.AccessLogsManager(), s.ReverseProxyGRPCServer(), s.Config.ReverseProxy.TrustedHTTPProxies, s.ProxyManager()) + httpAPIHandler, err := nbhttp.NewAPIHandler(context.Background(), s.AccountManager(), s.NetworksManager(), s.ResourcesManager(), s.RoutesManager(), s.GroupsManager(), s.GeoLocationManager(), s.AuthManager(), s.Metrics(), s.IntegratedValidator(), s.ProxyController(), s.PermissionsManager(), s.PeersManager(), s.SettingsManager(), s.ZonesManager(), s.RecordsManager(), s.NetworkMapController(), s.IdpManager(), s.ServiceManager(), s.ReverseProxyDomainManager(), s.AccessLogsManager(), s.ReverseProxyGRPCServer(), s.Config.ReverseProxy.TrustedHTTPProxies) if err != nil { log.Fatalf("failed to create API handler: %v", err) } diff --git a/management/internals/shared/grpc/proxy.go b/management/internals/shared/grpc/proxy.go index 573014cf9..45bce424d 100644 --- a/management/internals/shared/grpc/proxy.go +++ b/management/internals/shared/grpc/proxy.go @@ -196,7 +196,11 @@ func (s *ProxyServiceServer) GetMappingUpdate(req *proto.GetMappingUpdateRequest existingProxy, err := s.proxyManager.GetAccountProxy(ctx, *accountID) if err != nil { - log.WithContext(ctx).Debugf("failed to get account proxy for %s: %v", *accountID, err) + if s, ok := nbstatus.FromError(err); ok && s.ErrorType == nbstatus.NotFound { + log.WithContext(ctx).Debugf("no existing BYOP proxy for account %s", *accountID) + } else { + return status.Errorf(codes.Internal, "failed to check existing proxy: %v", err) + } } if existingProxy != nil && existingProxy.ID != proxyID { if existingProxy.Status == proxy.StatusConnected { @@ -465,12 +469,21 @@ func (s *ProxyServiceServer) SendServiceUpdate(update *proto.GetMappingUpdateRes } s.connectedProxies.Range(func(key, value interface{}) bool { conn := value.(*proxyConnection) + connUpdate := update if conn.accountID != nil && len(updateAccountIDs) > 0 { if _, ok := updateAccountIDs[*conn.accountID]; !ok { return true } + filtered := filterMappingsForAccount(update.Mapping, *conn.accountID) + if len(filtered) == 0 { + return true + } + connUpdate = &proto.GetMappingUpdateResponse{ + Mapping: filtered, + InitialSyncComplete: update.InitialSyncComplete, + } } - resp := s.perProxyMessage(update, conn.proxyID) + resp := s.perProxyMessage(connUpdate, conn.proxyID) if resp == nil { return true } @@ -494,6 +507,16 @@ func (s *ProxyServiceServer) ForceDisconnect(proxyID string) { } } +func filterMappingsForAccount(mappings []*proto.ProxyMapping, accountID string) []*proto.ProxyMapping { + var filtered []*proto.ProxyMapping + for _, m := range mappings { + if m.AccountId == accountID { + filtered = append(filtered, m) + } + } + return filtered +} + // GetConnectedProxies returns a list of connected proxy IDs func (s *ProxyServiceServer) GetConnectedProxies() []string { var proxies []string diff --git a/management/internals/shared/grpc/validate_session_test.go b/management/internals/shared/grpc/validate_session_test.go index 9f159ce35..e2332bfd8 100644 --- a/management/internals/shared/grpc/validate_session_test.go +++ b/management/internals/shared/grpc/validate_session_test.go @@ -339,7 +339,7 @@ func (m *testValidateSessionProxyManager) Disconnect(_ context.Context, _ string return nil } -func (m *testValidateSessionProxyManager) Heartbeat(_ context.Context, _ string) error { +func (m *testValidateSessionProxyManager) Heartbeat(_ context.Context, _, _, _ string) error { return nil } diff --git a/management/server/http/handler.go b/management/server/http/handler.go index 3a65e8a35..11c112720 100644 --- a/management/server/http/handler.go +++ b/management/server/http/handler.go @@ -19,7 +19,6 @@ import ( "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/accesslogs" "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service" - rpproxy "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/proxy" "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/proxytoken" reverseproxymanager "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service/manager" @@ -75,7 +74,7 @@ const ( ) // NewAPIHandler creates the Management service HTTP API handler registering all the available endpoints. -func NewAPIHandler(ctx context.Context, accountManager account.Manager, networksManager nbnetworks.Manager, resourceManager resources.Manager, routerManager routers.Manager, groupsManager nbgroups.Manager, LocationManager geolocation.Geolocation, authManager auth.Manager, appMetrics telemetry.AppMetrics, integratedValidator integrated_validator.IntegratedValidator, proxyController port_forwarding.Controller, permissionsManager permissions.Manager, peersManager nbpeers.Manager, settingsManager settings.Manager, zManager zones.Manager, rManager records.Manager, networkMapController network_map.Controller, idpManager idpmanager.Manager, serviceManager service.Manager, reverseProxyDomainManager *manager.Manager, reverseProxyAccessLogsManager accesslogs.Manager, proxyGRPCServer *nbgrpc.ProxyServiceServer, trustedHTTPProxies []netip.Prefix, proxyMgr rpproxy.Manager) (http.Handler, error) { +func NewAPIHandler(ctx context.Context, accountManager account.Manager, networksManager nbnetworks.Manager, resourceManager resources.Manager, routerManager routers.Manager, groupsManager nbgroups.Manager, LocationManager geolocation.Geolocation, authManager auth.Manager, appMetrics telemetry.AppMetrics, integratedValidator integrated_validator.IntegratedValidator, proxyController port_forwarding.Controller, permissionsManager permissions.Manager, peersManager nbpeers.Manager, settingsManager settings.Manager, zManager zones.Manager, rManager records.Manager, networkMapController network_map.Controller, idpManager idpmanager.Manager, serviceManager service.Manager, reverseProxyDomainManager *manager.Manager, reverseProxyAccessLogsManager accesslogs.Manager, proxyGRPCServer *nbgrpc.ProxyServiceServer, trustedHTTPProxies []netip.Prefix) (http.Handler, error) { // Register bypass paths for unauthenticated endpoints if err := bypass.AddBypassPath("/api/instance"); err != nil { diff --git a/management/server/http/testing/testing_tools/channel/channel.go b/management/server/http/testing/testing_tools/channel/channel.go index 533d679e3..2fd414da5 100644 --- a/management/server/http/testing/testing_tools/channel/channel.go +++ b/management/server/http/testing/testing_tools/channel/channel.go @@ -135,7 +135,7 @@ func BuildApiBlackBoxWithDBState(t testing_tools.TB, sqlFile string, expectedPee customZonesManager := zonesManager.NewManager(store, am, permissionsManager, "") zoneRecordsManager := recordsManager.NewManager(store, am, permissionsManager) - apiHandler, err := http2.NewAPIHandler(context.Background(), am, networksManagerMock, resourcesManagerMock, routersManagerMock, groupsManagerMock, geoMock, authManagerMock, metrics, validatorMock, proxyController, permissionsManager, peersManager, settingsManager, customZonesManager, zoneRecordsManager, networkMapController, nil, serviceManager, nil, nil, nil, nil, nil) + apiHandler, err := http2.NewAPIHandler(context.Background(), am, networksManagerMock, resourcesManagerMock, routersManagerMock, groupsManagerMock, geoMock, authManagerMock, metrics, validatorMock, proxyController, permissionsManager, peersManager, settingsManager, customZonesManager, zoneRecordsManager, networkMapController, nil, serviceManager, nil, nil, nil, nil) if err != nil { t.Fatalf("Failed to create API handler: %v", err) } diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index bb384cf3d..a35df5e3a 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -5458,7 +5458,10 @@ func isUniqueConstraintError(err error) bool { return true } errStr := err.Error() - return strings.Contains(errStr, "UNIQUE constraint") || strings.Contains(errStr, "duplicate key") + return strings.Contains(errStr, "UNIQUE constraint") || + strings.Contains(errStr, "duplicate key") || + strings.Contains(errStr, "Duplicate entry") || + strings.Contains(errStr, "Error 1062") } func (s *SqlStore) DisconnectProxy(ctx context.Context, proxyID string) error { diff --git a/shared/management/http/api/openapi.yml b/shared/management/http/api/openapi.yml index eee816766..f38c25506 100644 --- a/shared/management/http/api/openapi.yml +++ b/shared/management/http/api/openapi.yml @@ -10011,35 +10011,6 @@ paths: "$ref": "#/components/responses/conflict" '500': "$ref": "#/components/responses/internal_error" - /api/reverse-proxies/clusters/{clusterId}: - delete: - summary: Delete a self-hosted proxy cluster - description: Removes a self-hosted (BYOP) proxy cluster and disconnects it. Only self-hosted clusters can be deleted. - tags: [ Services ] - security: - - BearerAuth: [ ] - - TokenAuth: [ ] - parameters: - - in: path - name: clusterId - required: true - schema: - type: string - description: The unique identifier of the proxy cluster - responses: - '200': - description: Proxy cluster deleted successfully - content: { } - '400': - "$ref": "#/components/responses/bad_request" - '401': - "$ref": "#/components/responses/requires_authentication" - '403': - "$ref": "#/components/responses/forbidden" - '404': - "$ref": "#/components/responses/not_found" - '500': - "$ref": "#/components/responses/internal_error" /api/reverse-proxies/services/{serviceId}: get: summary: Retrieve a Service