From 7436a1cbf3b7986bba802a7d6e607f4f86f5cc96 Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Mon, 3 Aug 2026 15:35:22 +0000 Subject: [PATCH] [management] Make the settings update transactional; harden the bootstrap e2e MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- e2e/agentnetwork/settings_bootstrap_test.go | 40 +++++++-- .../internals/modules/agentnetwork/manager.go | 82 +++++++++++-------- 2 files changed, 80 insertions(+), 42 deletions(-) diff --git a/e2e/agentnetwork/settings_bootstrap_test.go b/e2e/agentnetwork/settings_bootstrap_test.go index 266f631b3..ea56f7064 100644 --- a/e2e/agentnetwork/settings_bootstrap_test.go +++ b/e2e/agentnetwork/settings_bootstrap_test.go @@ -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") } diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index cc7ca9d28..2e39bde5d 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -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