sendnrw/netbird
2
0
Fork 0
mirror of https://github.com/netbirdio/netbird.git synced 2026-05-29 12:09:59 +00:00
Code Issues Packages Projects Releases Wiki Activity

chore(api,ci,docs,test): private-service schema, proto-check, fixups

Browse Source 
Non-functional cleanups and contract/CI hardening around the
private-service work:

API schema (openapi.yml):
- Require a non-empty access_groups and mode=http when private=true,
  on both Service and ServiceRequest, mirroring
  validatePrivateRequirements. mode stays optional-but-constrained
  (empty defaults to http server-side), matching runtime.

CI (proto-version-check.yml):
- Cover renamed .pb.go files (read base via previous_filename).
- Match protoc-gen-go-grpc version headers (optional "- " prefix and
  -gen-go-grpc suffix) so grpc-generated files are in scope.

Docs / comments:
- Reword Config field docs to say defaults are applied at Server.Start
  (initDefaults), not New.
- Rename the obsolete --private-inbound flag to --private across
  comments and the proto doc.

Pre-existing test fixups surfaced by review:
- Repair the integration-tagged validate_session_test.go (SignToken
  signature growth + new Manager interface methods).
- Fix the CI-skip boolean precedence so Windows isn't skipped
  unconditionally.
- Guard the router.HTTPListener type assertion with comma-ok.
This commit is contained in:
mlsmaycon
2026-05-26 22:00:08 +02:00
parent 43c0cb1dc2
commit 924be2116b
11 changed files with 111 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files

39
.github/workflows/proto-version-check.yml vendored
View File

@@ -20,15 +20,30 @@ jobs:
per_page: 100,
});
const modifiedPbFiles = files.filter(
f => f.filename.endsWith('.pb.go') && f.status === 'modified'
);
if (modifiedPbFiles.length === 0) {
console.log('No modified .pb.go files to check');
// Cover renamed .pb.go files in addition to plain edits.
// Renamed entries land under the new path with previous_filename
// pointing at the base-side name, so we read the base content
// from the old path when present.
const changedPbFiles = files
.filter(f => (f.status === 'modified' || f.status === 'renamed')
&& f.filename.endsWith('.pb.go'))
.map(f => ({
headPath: f.filename,
basePath: f.previous_filename || f.filename,
}));
if (changedPbFiles.length === 0) {
console.log('No modified or renamed .pb.go files to check');
return;
}
const versionPattern = /^\s*\/\/\s+protoc(?:-gen-go)?\s+v[\d.]+/;
// Matches the generator version headers protoc writes at the top
// of generated files:
// // protoc v3.21.12
// // protoc-gen-go v1.26.0
// // - protoc-gen-go-grpc v1.6.1 (grpc files prefix with "- ")
// The optional "- " prefix and the optional -gen-go / -gen-go-grpc
// suffixes keep the *_grpc.pb.go headers in scope.
const versionPattern = /^\s*\/\/\s+(?:-\s+)?protoc(?:-gen-go(?:-grpc)?)?\s+v[\d.]+/;
const baseSha = context.payload.pull_request.base.sha;
const headSha = context.payload.pull_request.head.sha;
@@ -55,20 +70,22 @@ jobs:
}
const violations = [];
for (const file of modifiedPbFiles) {
for (const file of changedPbFiles) {
const [base, head] = await Promise.all([
getVersionHeader(file.filename, baseSha),
getVersionHeader(file.filename, headSha),
getVersionHeader(file.basePath, baseSha),
getVersionHeader(file.headPath, headSha),
]);
if (!base.ok || !head.ok) {
core.warning(
`Skipping ${file.filename}: base=${base.ok ? 'ok' : base.reason}, head=${head.ok ? 'ok' : head.reason}`
`Skipping ${file.headPath}: base=${base.ok ? 'ok' : base.reason}, head=${head.ok ? 'ok' : head.reason}`
);
continue;
}
if (base.lines.join('\n') !== head.lines.join('\n')) {
violations.push({
file: file.filename,
file: file.basePath === file.headPath
? file.headPath
: `${file.basePath} → ${file.headPath}`,
base: base.lines,
head: head.lines,
});

27
management/internals/shared/grpc/validate_session_test.go
View File

@@ -102,7 +102,7 @@ func generateSessionKeyPair(t *testing.T) (string, string) {
func createSessionToken(t *testing.T, privKeyB64, userID, domain string) string {
t.Helper()
token, err := sessionkey.SignToken(privKeyB64, userID, domain, auth.MethodOIDC, nil, time.Hour)
token, err := sessionkey.SignToken(privKeyB64, userID, "", domain, auth.MethodOIDC, nil, nil, time.Hour)
require.NoError(t, err)
return token
}
@@ -394,6 +394,10 @@ func (m *testValidateSessionProxyManager) ClusterSupportsCrowdSec(_ context.Cont
return nil
}
func (m *testValidateSessionProxyManager) ClusterSupportsPrivate(_ context.Context, _ string) *bool {
return nil
}
type testValidateSessionUsersManager struct {
store store.Store
}
@@ -401,3 +405,24 @@ type testValidateSessionUsersManager struct {
func (m *testValidateSessionUsersManager) GetUser(ctx context.Context, userID string) (*types.User, error) {
return m.store.GetUserByUserID(ctx, store.LockingStrengthNone, userID)
}
func (m *testValidateSessionUsersManager) GetUserWithGroups(ctx context.Context, userID string) (*types.User, []*types.Group, error) {
user, err := m.store.GetUserByUserID(ctx, store.LockingStrengthNone, userID)
if err != nil {
return nil, nil, err
}
if len(user.AutoGroups) == 0 {
return user, nil, nil
}
groupsMap, err := m.store.GetGroupsByIDs(ctx, store.LockingStrengthNone, user.AccountID, user.AutoGroups)
if err != nil {
return nil, nil, err
}
groups := make([]*types.Group, 0, len(user.AutoGroups))
for _, id := range user.AutoGroups {
if g, ok := groupsMap[id]; ok && g != nil {
groups = append(groups, g)
}
}
return user, groups, nil
}

2
management/server/store/sql_store_service_test.go
View File

@@ -13,7 +13,7 @@ import (
)
func TestSqlStore_GetAccount_PrivateServiceRoundtrip(t *testing.T) {
if (os.Getenv("CI") == "true" && runtime.GOOS == "darwin") || runtime.GOOS == "windows" {
if os.Getenv("CI") == "true" && (runtime.GOOS == "darwin" || runtime.GOOS == "windows") {
t.Skip("skip CI tests on darwin and windows")
}

6
proxy/inbound.go
View File

@@ -43,7 +43,7 @@ const privateInboundPortHTTPS = 443
const privateInboundPortHTTP = 80
// inboundManager wires per-account inbound listeners into the proxy
// pipeline when --private-inbound is enabled. When disabled the manager
// pipeline when --private is enabled. When disabled the manager
// is nil and every method on *Server that touches it short-circuits.
type inboundManager struct {
logger *log.Logger
@@ -390,7 +390,7 @@ func (m *inboundManager) ListenerInfo(accountID types.AccountID) (InboundListene
}
// Snapshot returns the inbound listener state for every account that has
// a live listener at call time. Empty when --private-inbound is off or
// a live listener at call time. Empty when --private is off or
// no accounts have come up yet.
func (m *inboundManager) Snapshot() map[types.AccountID]InboundListenerInfo {
if m == nil {
@@ -513,7 +513,7 @@ func accountTunnelLookup(client *embed.Client) auth.TunnelLookupFunc {
// peerstore lookup to every request's context before delegating to next.
// Calling on the host-level listener is a no-op because that path never
// installs this wrapper, so the existing behaviour stays byte-for-byte
// identical when --private-inbound is off or the request didn't arrive
// identical when --private is off or the request didn't arrive
// on a per-account listener.
func withTunnelLookup(next http.Handler, lookup auth.TunnelLookupFunc) http.Handler {
if lookup == nil {

4
proxy/inbound_test.go
View File

@@ -140,7 +140,7 @@ func TestInboundManager_AddRouteAfterReady_RegistersDirectly(t *testing.T) {
// TestPrivateCapability_DerivedFromPrivateOnly tests that the capability
// bit reported upstream tracks --private exclusively. The previous
// --private-inbound flag has been folded into --private.
// --private flag has been folded into --private.
func TestPrivateCapability_DerivedFromPrivateOnly(t *testing.T) {
tests := []struct {
name string
@@ -319,7 +319,7 @@ func TestInboundManager_ListenerInfo(t *testing.T) {
}
// TestInboundManager_NilManagerSafe ensures the observability accessors
// are safe to call when --private-inbound is off (nil manager).
// are safe to call when --private is off (nil manager).
func TestInboundManager_NilManagerSafe(t *testing.T) {
var mgr *inboundManager
_, ok := mgr.ListenerInfo("anything")

2
proxy/internal/debug/handler.go
View File

@@ -131,7 +131,7 @@ func (h *Handler) SetCertStatus(cs certStatus) {
// SetInboundProvider wires per-account inbound listener observability.
// Pass nil (or skip the call) to keep the inbound section out of debug
// responses on proxies that don't run --private-inbound.
// responses on proxies that don't run --private.
func (h *Handler) SetInboundProvider(p InboundProvider) {
h.inbound = p
}

5
proxy/internal/tcp/router_test.go
View File

@@ -1781,11 +1781,14 @@ func TestRouter_PlainHTTP_RoutesToPlainChannel(t *testing.T) {
}
}()
tlsListener, ok := router.HTTPListener().(*chanListener)
require.True(t, ok, "router.HTTPListener() must be the test's chanListener; the test relies on observing its channel directly")
select {
case conn := <-acceptDone:
require.NotNil(t, conn)
_ = conn.Close()
case <-router.HTTPListener().(*chanListener).ch:
case <-tlsListener.ch:
t.Fatal("plain HTTP request leaked into TLS channel")
case <-time.After(3 * time.Second):
t.Fatal("plain HTTP connection never reached plain channel")

13
proxy/lifecycle.go
View File

@@ -20,14 +20,17 @@ import (
type Config struct {
// ListenAddr is the TCP address the main listener binds. Required.
ListenAddr string
// ID identifies this proxy instance to management. Empty value lets
// New generate a timestamped default.
// ID identifies this proxy instance to management. Empty values are
// replaced with a timestamped default at Server.Start time (see
// initDefaults), not in New.
ID string
// Logger is the logrus logger used everywhere. Empty value falls back
// to log.StandardLogger().
// Logger is the logrus logger used everywhere. Empty values fall
// back to log.StandardLogger() at Server.Start time (see
// initDefaults), not in New.
Logger *log.Logger
// Version is the build version string reported to management. Empty
// becomes "dev".
// values are replaced with "dev" at Server.Start time (see
// initDefaults), not in New.
Version string
// ProxyURL is the public address operators use to reach this proxy.
ProxyURL string

4
proxy/server.go
View File

@@ -281,7 +281,7 @@ func (s *Server) NotifyCertificateIssued(ctx context.Context, accountID types.Ac
}
// inboundListenerProto resolves the per-account inbound listener state for
// the SendStatusUpdate payload. Returns nil when --private-inbound is off
// the SendStatusUpdate payload. Returns nil when --private is off
// or the account has no live listener so management treats the field as
// absent.
func (s *Server) inboundListenerProto(accountID types.AccountID) *proto.ProxyInboundListener {
@@ -528,7 +528,7 @@ func (s *Server) initManagementClient() error {
}
// initNetBirdClient builds the multi-tenant embedded NetBird client used
// for outbound RoundTripping and (when --private-inbound is on) per-account
// for outbound RoundTripping and (when --private is on) per-account
// inbound listeners.
func (s *Server) initNetBirdClient() {
s.netbird = roundtrip.NewNetBird(s.ID, s.ProxyURL, roundtrip.ClientConfig{

35
shared/management/http/api/openapi.yml
View File

@@ -3086,6 +3086,24 @@ components:
- enabled
- auth
- meta
allOf:
# When private=true, access_groups must be present and non-empty,
# and the service mode must be "http". The bearer-auth mutex is
# enforced at the service-validation layer
# (validatePrivateRequirements) because it sits in a nested
# ServiceAuthConfig and isn't cleanly expressible here.
- if:
required: [private]
properties:
private:
const: true
then:
required: [access_groups]
properties:
access_groups:
minItems: 1
mode:
const: http
ServiceMeta:
type: object
properties:
@@ -3173,6 +3191,23 @@ components:
- name
- domain
- enabled
allOf:
# Mirror of the Service conditional: when private=true the
# request must carry a non-empty access_groups list and the
# mode must be "http". The bearer-auth mutex is enforced at the
# service-validation layer (validatePrivateRequirements).
- if:
required: [private]
properties:
private:
const: true
then:
required: [access_groups]
properties:
access_groups:
minItems: 1
mode:
const: http
ServiceTargetOptions:
type: object
properties:

2
shared/management/proto/proxy_service.proto
View File

@@ -237,7 +237,7 @@ message SendStatusUpdateRequest {
bool certificate_issued = 4;
optional string error_message = 5;
// Per-account inbound listener state for the account that owns
// service_id. Populated only when --private-inbound is enabled and the
// service_id. Populated only when --private is enabled and the
// embedded client for the account is up. Field numbers >=50 reserved
// for observability extensions.
optional ProxyInboundListener inbound_listener = 50;