starred/netbird
1
0
Fork 0
mirror of https://github.com/netbirdio/netbird.git synced 2026-08-03 19:19:47 -04:00
Code Issues Packages Projects Releases Wiki Activity

[management] Make the settings update transactional; harden the bootstrap e2e

Browse Source 
The row lock from LockingStrengthUpdate only holds for the duration of
the surrounding transaction, so the settings read, the cluster
immutability check, and the save now share one via ExecuteInTransaction
— concurrent PUTs could otherwise interleave between them.
bootstrapSettingsIfNeeded takes the store to operate on so the
settings-PUT bootstrap participates in that transaction.

The bootstrap e2e now sets every toggle away from its default so each
assertion can fail, covers a post-bootstrap update without the cluster,
and verifies a rejected cluster change leaves the row untouched.
This commit is contained in:
mlsmaycon
2026-08-03 15:35:22 +00:00
parent a98ffa50ed
commit 7436a1cbf3
2 changed files with 80 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

40
e2e/agentnetwork/settings_bootstrap_test.go
View File

@@ -60,29 +60,55 @@ func TestSettingsBootstrapViaPut(t *testing.T) {
requireClientError(t, err)
// A PUT carrying a cluster bootstraps the account and applies the
// mutable fields from the same request.
// mutable fields from the same request. Every toggle is set away from
// its bootstrap default so each assertion can actually fail.
const cluster = "e2e.bootstrap.netbird.selfhosted"
bootstrapped, err := fresh.UpdateSettings(ctx, api.AgentNetworkSettingsRequest{
Cluster: ptr(cluster),
EnableLogCollection: true,
EnableLogCollection: false,
EnablePromptCollection: true,
RedactPii: false,
RedactPii: true,
})
require.NoError(t, err, "bootstrap settings via PUT must succeed")
assert.Equal(t, cluster, bootstrapped.Cluster, "cluster must be pinned from the request")
require.NotEmpty(t, bootstrapped.Subdomain, "subdomain must be assigned at bootstrap")
assert.Equal(t, bootstrapped.Subdomain+"."+cluster, bootstrapped.Endpoint, "endpoint must combine subdomain and cluster")
assert.True(t, bootstrapped.EnablePromptCollection, "toggle from the bootstrap request must apply")
assert.False(t, bootstrapped.EnableLogCollection, "log collection from the bootstrap request must override the default")
assert.True(t, bootstrapped.EnablePromptCollection, "prompt collection from the bootstrap request must apply")
assert.True(t, bootstrapped.RedactPii, "redact toggle from the bootstrap request must apply")
// The row is persisted and the cluster immutable: reads agree, and a
// different cluster is rejected rather than silently ignored.
// The row is persisted: an independent read agrees on every field.
after, err := fresh.GetSettings(ctx)
require.NoError(t, err, "get settings after bootstrap must succeed")
assert.Equal(t, bootstrapped.Endpoint, after.Endpoint, "bootstrap must persist across reads")
assert.Equal(t, bootstrapped.EnableLogCollection, after.EnableLogCollection, "log collection must persist")
assert.Equal(t, bootstrapped.EnablePromptCollection, after.EnablePromptCollection, "prompt collection must persist")
assert.Equal(t, bootstrapped.RedactPii, after.RedactPii, "redact toggle must persist")
// Once bootstrapped, later updates may omit the cluster entirely.
persisted, err := fresh.UpdateSettings(ctx, api.AgentNetworkSettingsRequest{
EnableLogCollection: true,
EnablePromptCollection: false,
RedactPii: true,
})
require.NoError(t, err, "post-bootstrap update without cluster must succeed")
assert.Equal(t, cluster, persisted.Cluster, "omitted cluster must keep the pinned value")
assert.True(t, persisted.EnableLogCollection, "post-bootstrap toggle must apply")
assert.False(t, persisted.EnablePromptCollection, "post-bootstrap toggle must apply")
// The cluster is immutable: a different value is rejected rather than
// silently ignored, and the rejected update must not disturb anything.
_, err = fresh.UpdateSettings(ctx, api.AgentNetworkSettingsRequest{
Cluster: ptr("other.cluster.invalid"),
EnableLogCollection: true,
EnableLogCollection: false,
})
requireClientError(t, err)
final, err := fresh.GetSettings(ctx)
require.NoError(t, err, "get settings after the rejected cluster change must succeed")
assert.Equal(t, persisted.Cluster, final.Cluster, "rejected update must not change the cluster")
assert.Equal(t, persisted.Endpoint, final.Endpoint, "rejected update must not change the endpoint")
assert.Equal(t, persisted.EnableLogCollection, final.EnableLogCollection, "rejected update must not apply its toggles")
assert.Equal(t, persisted.EnablePromptCollection, final.EnablePromptCollection, "rejected update must not apply its toggles")
assert.Equal(t, persisted.RedactPii, final.RedactPii, "rejected update must not apply its toggles")
}

82
management/internals/modules/agentnetwork/manager.go
View File

@@ -202,7 +202,7 @@ func (m *managerImpl) CreateProvider(ctx context.Context, userID string, provide
}
if strings.TrimSpace(bootstrapCluster) != "" {
if _, err := m.bootstrapSettingsIfNeeded(ctx, provider.AccountID, bootstrapCluster); err != nil {
if _, err := m.bootstrapSettingsIfNeeded(ctx, m.store, provider.AccountID, bootstrapCluster); err != nil {
// The provider create has already succeeded; logging the
// bootstrap miss matches the plan's PoC behaviour. The synth
// path treats a missing settings row as a no-op, and the next
@@ -571,42 +571,54 @@ func (m *managerImpl) UpdateSettings(ctx context.Context, userID string, setting
requestedCluster := strings.TrimSpace(settings.Cluster)
existing, err := m.store.GetAgentNetworkSettings(ctx, store.LockingStrengthUpdate, settings.AccountID)
switch {
case err == nil:
if requestedCluster != "" && requestedCluster != existing.Cluster {
return nil, status.Errorf(status.InvalidArgument, "cluster is immutable once assigned (current: %s)", existing.Cluster)
// The row lock from LockingStrengthUpdate only holds for the duration of
// the surrounding transaction, so the read, the cluster-immutability
// check, and the save must share one — otherwise concurrent PUTs could
// interleave between them.
var updated *types.Settings
err := m.store.ExecuteInTransaction(ctx, func(tx store.Store) error {
existing, err := tx.GetAgentNetworkSettings(ctx, store.LockingStrengthUpdate, settings.AccountID)
switch {
case err == nil:
if requestedCluster != "" && requestedCluster != existing.Cluster {
return status.Errorf(status.InvalidArgument, "cluster is immutable once assigned (current: %s)", existing.Cluster)
}
case isNotFound(err):
if requestedCluster == "" {
return status.Errorf(status.NotFound, "agent network settings have not been bootstrapped yet; pass cluster to bootstrap them, or create a provider with bootstrap_cluster set")
}
existing, err = m.bootstrapSettingsIfNeeded(ctx, tx, settings.AccountID, requestedCluster)
if err != nil {
return err
}
default:
return fmt.Errorf("get agent network settings: %w", err)
}
case isNotFound(err):
if requestedCluster == "" {
return nil, status.Errorf(status.NotFound, "agent network settings have not been bootstrapped yet; pass cluster to bootstrap them, or create a provider with bootstrap_cluster set")
}
existing, err = m.bootstrapSettingsIfNeeded(ctx, settings.AccountID, requestedCluster)
if err != nil {
return nil, err
}
default:
return nil, fmt.Errorf("get agent network settings: %w", err)
}
existing.EnableLogCollection = settings.EnableLogCollection
existing.EnablePromptCollection = settings.EnablePromptCollection
existing.RedactPii = settings.RedactPii
existing.AccessLogRetentionDays = settings.AccessLogRetentionDays
existing.UpdatedAt = time.Now().UTC()
existing.EnableLogCollection = settings.EnableLogCollection
existing.EnablePromptCollection = settings.EnablePromptCollection
existing.RedactPii = settings.RedactPii
existing.AccessLogRetentionDays = settings.AccessLogRetentionDays
existing.UpdatedAt = time.Now().UTC()
if err := m.store.SaveAgentNetworkSettings(ctx, existing); err != nil {
return nil, fmt.Errorf("save agent network settings: %w", err)
if err := tx.SaveAgentNetworkSettings(ctx, existing); err != nil {
return fmt.Errorf("save agent network settings: %w", err)
}
updated = existing
return nil
})
if err != nil {
return nil, err
}
m.accountManager.StoreEvent(ctx, userID, settings.AccountID, settings.AccountID, activity.AgentNetworkSettingsUpdated, map[string]any{
"log_collection": existing.EnableLogCollection,
"prompt_collection": existing.EnablePromptCollection,
"redact_pii": existing.RedactPii,
"log_collection": updated.EnableLogCollection,
"prompt_collection": updated.EnablePromptCollection,
"redact_pii": updated.RedactPii,
})
m.reconcile(ctx, settings.AccountID)
return existing, nil
return updated, nil
}
// isNotFound reports whether err is a status.NotFound error.
@@ -660,8 +672,9 @@ func (m *managerImpl) GetSettings(ctx context.Context, accountID, userID string)
// hint the dashboard sends (auto-picked from the active cluster list);
// the subdomain is picked from the curated wordlist avoiding
// collisions on the same cluster. Idempotent: if a row already exists
// it is returned untouched and the hint is ignored.
func (m *managerImpl) bootstrapSettingsIfNeeded(ctx context.Context, accountID, providerCluster string) (*types.Settings, error) {
// it is returned untouched and the hint is ignored. st is the store to
// operate on — pass the transaction store when calling from within one.
func (m *managerImpl) bootstrapSettingsIfNeeded(ctx context.Context, st store.Store, accountID, providerCluster string) (*types.Settings, error) {
if accountID == "" {
return nil, fmt.Errorf("bootstrap settings: account id is required")
}
@@ -669,16 +682,15 @@ func (m *managerImpl) bootstrapSettingsIfNeeded(ctx context.Context, accountID,
return nil, fmt.Errorf("bootstrap settings: provider cluster is required")
}
existing, err := m.store.GetAgentNetworkSettings(ctx, store.LockingStrengthNone, accountID)
existing, err := st.GetAgentNetworkSettings(ctx, store.LockingStrengthNone, accountID)
if err == nil {
return existing, nil
}
var sErr *status.Error
if !errors.As(err, &sErr) || sErr.Type() != status.NotFound {
if !isNotFound(err) {
return nil, fmt.Errorf("get agent network settings: %w", err)
}
siblings, err := m.store.GetAgentNetworkSettingsByCluster(ctx, store.LockingStrengthNone, providerCluster)
siblings, err := st.GetAgentNetworkSettingsByCluster(ctx, store.LockingStrengthNone, providerCluster)
if err != nil {
return nil, fmt.Errorf("list agent network settings on cluster: %w", err)
}
@@ -702,7 +714,7 @@ func (m *managerImpl) bootstrapSettingsIfNeeded(ctx context.Context, accountID,
settings.Subdomain = subdomain
settings.CreatedAt = now
settings.UpdatedAt = now
if err := m.store.SaveAgentNetworkSettings(ctx, settings); err != nil {
if err := st.SaveAgentNetworkSettings(ctx, settings); err != nil {
return nil, fmt.Errorf("save agent network settings: %w", err)
}
return settings, nil