diff --git a/e2e/agentnetwork/settings_bootstrap_test.go b/e2e/agentnetwork/settings_bootstrap_test.go index 806ce82f4..5f74684c5 100644 --- a/e2e/agentnetwork/settings_bootstrap_test.go +++ b/e2e/agentnetwork/settings_bootstrap_test.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/require" "github.com/netbirdio/netbird/e2e/harness" + "github.com/netbirdio/netbird/shared/management/client/rest" "github.com/netbirdio/netbird/shared/management/http/api" ) @@ -177,3 +178,86 @@ func TestSettingsBootstrapSelfAddressed(t *testing.T) { require.NoError(t, err, "bootstrap after delete must succeed") assert.Equal(t, "gw2.e2e.netbird.selfhosted", recreated.Endpoint, "the fresh bootstrap claims the new hostname") } + +// TestSettingsConditionalWrites covers the lost-update guard end to end, over +// the same REST client the Terraform provider uses: read the settings, take +// the entity-tag, and have a write refused when the row moved underneath it. +// +// The scenario is the one that motivates the feature. A client reads the +// settings and computes an update. An operator turns PII redaction on in the +// dashboard in the meantime. Without a precondition the client's write puts +// redaction straight back off — no error, no drift warning, a +// compliance-relevant control silently disabled. With one, the write is +// refused and the client can read again. +func TestSettingsConditionalWrites(t *testing.T) { + ctx := context.Background() + + fresh, err := harnessStartFresh(ctx, t) + require.NoError(t, err, "start dedicated combined server") + + const cluster = "eu.e2e.netbird.selfhosted" + bootstrapped, err := fresh.CreateSettings(ctx, api.AgentNetworkSettingsCreateRequest{ + ProxyAddress: ptr(cluster), + }) + require.NoError(t, err, "bootstrap must succeed") + + // What the client plans against. + planned, etag, err := fresh.GetSettingsWithETag(ctx) + require.NoError(t, err, "read must succeed") + require.NotEmpty(t, etag, "the read must carry a validator") + assert.Equal(t, bootstrapped.Endpoint, planned.Endpoint) + + _, again, err := fresh.GetSettingsWithETag(ctx) + require.NoError(t, err, "second read must succeed") + assert.Equal(t, etag, again, "an unchanged row must read as the same validator") + + update := func(redactPii bool, retention int) api.AgentNetworkSettingsRequest { + return api.AgentNetworkSettingsRequest{ + Endpoint: planned.Endpoint, + ProxyAddress: planned.ProxyAddress, + EnableLogCollection: true, + EnablePromptCollection: true, + RedactPii: redactPii, + AccessLogRetentionDays: retention, + } + } + + // The operator's change, which the planning client never saw. + _, err = fresh.UpdateSettings(ctx, update(true, 21)) + require.NoError(t, err, "the intervening update must succeed") + + // The client's write, planned against the earlier read, would have turned + // redaction back off. It is refused instead. + _, _, err = fresh.UpdateSettingsIfMatch(ctx, update(false, 7), etag) + require.Error(t, err, "a stale precondition must be refused") + require.True(t, rest.IsPreconditionFailed(err), + "the refusal must be a precondition failure, got: %v", err) + + intact, current, err := fresh.GetSettingsWithETag(ctx) + require.NoError(t, err, "read after the refusal must succeed") + assert.True(t, intact.RedactPii, "the refused write must not have turned redaction off") + require.NotNil(t, intact.AccessLogRetentionDays) + assert.Equal(t, 21, *intact.AccessLogRetentionDays, "the refused write must not have changed retention") + assert.NotEqual(t, etag, current, "the validator must have moved with the intervening update") + + // Retrying against the current validator goes through, and hands back the + // validator for the write after it. + updated, next, err := fresh.UpdateSettingsIfMatch(ctx, update(true, 7), current) + require.NoError(t, err, "a matching precondition must be honoured") + require.NotNil(t, updated.AccessLogRetentionDays) + assert.Equal(t, 7, *updated.AccessLogRetentionDays, "the conditional write must apply") + assert.NotEmpty(t, next, "the write must return a validator") + assert.NotEqual(t, current, next, "the write must move the validator") + + // The delete is conditional too, and refusing a stale one leaves the + // endpoint claimed. + require.Error(t, fresh.DeleteSettingsIfMatch(ctx, etag), "a stale precondition must refuse the delete") + stillThere, err := fresh.GetSettings(ctx) + require.NoError(t, err, "read after the refused delete must succeed") + assert.Equal(t, planned.Endpoint, stillThere.Endpoint, "the refused delete must leave the endpoint claimed") + + require.NoError(t, fresh.DeleteSettingsIfMatch(ctx, next), "a matching precondition must be honoured") + gone, err := fresh.GetSettings(ctx) + require.NoError(t, err, "read after the delete must succeed") + assert.Empty(t, gone.Endpoint, "the row must be gone") +} diff --git a/e2e/harness/agentnetwork.go b/e2e/harness/agentnetwork.go index e85475dff..9c0e63d61 100644 --- a/e2e/harness/agentnetwork.go +++ b/e2e/harness/agentnetwork.go @@ -153,6 +153,37 @@ func (c *Combined) DeleteSettings(ctx context.Context) error { return anDelete(ctx, c, "/api/agent-network/settings") } +// The conditional-request wrappers go through the typed REST client rather +// than anRequest, so the e2e run exercises the client's own header handling — +// the quoting on the way out and the unquoting on the way back — against a +// real server, which is the path the Terraform provider takes. + +// GetSettingsWithETag reads the settings along with the entity-tag that makes +// a following write conditional. +func (c *Combined) GetSettingsWithETag(ctx context.Context) (api.AgentNetworkSettings, string, error) { + settings, etag, err := c.api.AgentNetwork.GetSettingsWithETag(ctx) + if err != nil { + return api.AgentNetworkSettings{}, "", err + } + return *settings, etag, nil +} + +// UpdateSettingsIfMatch applies the update only if etag is still current, +// returning the entity-tag of the row it wrote. +func (c *Combined) UpdateSettingsIfMatch(ctx context.Context, req api.AgentNetworkSettingsRequest, etag string) (api.AgentNetworkSettings, string, error) { + settings, newETag, err := c.api.AgentNetwork.UpdateSettingsIfMatch(ctx, req, etag) + if err != nil { + return api.AgentNetworkSettings{}, "", err + } + return *settings, newETag, nil +} + +// DeleteSettingsIfMatch deletes the settings row only if etag is still +// current. +func (c *Combined) DeleteSettingsIfMatch(ctx context.Context, etag string) error { + return c.api.AgentNetwork.DeleteSettingsIfMatch(ctx, etag) +} + // ListConsumption returns the account's consumption rows (possibly empty). func (c *Combined) ListConsumption(ctx context.Context) ([]api.AgentNetworkConsumption, error) { return anRequest[[]api.AgentNetworkConsumption](ctx, c, http.MethodGet, "/api/agent-network/consumption", nil) diff --git a/shared/management/client/rest/agentnetwork.go b/shared/management/client/rest/agentnetwork.go index 13328aab7..54cb64229 100644 --- a/shared/management/client/rest/agentnetwork.go +++ b/shared/management/client/rest/agentnetwork.go @@ -6,6 +6,8 @@ import ( "encoding/json" "io" "net/http" + "strconv" + "strings" "github.com/netbirdio/netbird/shared/management/http/api" ) @@ -336,25 +338,35 @@ func (a *AgentNetworkAPI) DeleteBudgetRule(ctx context.Context, ruleID string) e // to an APIError matchable via IsNotFound rather than fabricating defaults // the server never stated. func (a *AgentNetworkAPI) GetSettings(ctx context.Context) (*api.AgentNetworkSettings, error) { + settings, _, err := a.GetSettingsWithETag(ctx) + return settings, err +} + +// GetSettingsWithETag is GetSettings, additionally returning the entity-tag +// the server derived for the settings it returned. Hand that validator to +// UpdateSettingsIfMatch or DeleteSettingsIfMatch to make the write conditional +// on nothing having changed in between — the read-modify-write cycle that +// otherwise silently reverts a concurrent change. +func (a *AgentNetworkAPI) GetSettingsWithETag(ctx context.Context) (*api.AgentNetworkSettings, string, error) { resp, err := a.c.NewRequest(ctx, "GET", "/api/agent-network/settings", nil, nil) if err != nil { - return nil, err + return nil, "", err } if resp.Body != nil { defer resp.Body.Close() } body, err := io.ReadAll(resp.Body) if err != nil { - return nil, err + return nil, "", err } if trimmed := bytes.TrimSpace(body); len(trimmed) == 0 || bytes.Equal(trimmed, []byte("null")) { - return nil, &APIError{StatusCode: http.StatusNotFound, Message: "agent network settings not found"} + return nil, "", &APIError{StatusCode: http.StatusNotFound, Message: "agent network settings not found"} } var ret api.AgentNetworkSettings if err := json.Unmarshal(body, &ret); err != nil { - return nil, err + return nil, "", err } - return &ret, nil + return &ret, etagFrom(resp), nil } // CreateSettings bootstraps the account's Agent Network settings row, @@ -363,19 +375,30 @@ func (a *AgentNetworkAPI) GetSettings(ctx context.Context) (*api.AgentNetworkSet // request.Endpoint (self-addressed dedicated endpoint, claimed verbatim) must // be set. Returns a conflict when the account already has a settings row. func (a *AgentNetworkAPI) CreateSettings(ctx context.Context, request api.PostApiAgentNetworkSettingsJSONRequestBody) (*api.AgentNetworkSettings, error) { + settings, _, err := a.CreateSettingsWithETag(ctx, request) + return settings, err +} + +// CreateSettingsWithETag is CreateSettings, additionally returning the +// entity-tag of the row it bootstrapped, so a client can follow the bootstrap +// with a conditional write without an intervening read. +func (a *AgentNetworkAPI) CreateSettingsWithETag(ctx context.Context, request api.PostApiAgentNetworkSettingsJSONRequestBody) (*api.AgentNetworkSettings, string, error) { requestBytes, err := json.Marshal(request) if err != nil { - return nil, err + return nil, "", err } resp, err := a.c.NewRequest(ctx, "POST", "/api/agent-network/settings", bytes.NewReader(requestBytes), nil) if err != nil { - return nil, err + return nil, "", err } if resp.Body != nil { defer resp.Body.Close() } ret, err := parseResponse[api.AgentNetworkSettings](resp) - return &ret, err + if err != nil { + return nil, "", err + } + return &ret, etagFrom(resp), nil } // UpdateSettings updates the account's Agent Network settings; the request @@ -385,19 +408,35 @@ func (a *AgentNetworkAPI) CreateSettings(ctx context.Context, request api.PostAp // a request carrying different values is rejected. Returns not-found until // the account is bootstrapped. func (a *AgentNetworkAPI) UpdateSettings(ctx context.Context, request api.PutApiAgentNetworkSettingsJSONRequestBody) (*api.AgentNetworkSettings, error) { + settings, _, err := a.UpdateSettingsIfMatch(ctx, request, "") + return settings, err +} + +// UpdateSettingsIfMatch is UpdateSettings made conditional on etag — the +// validator from an earlier read — still being current, and returns the +// validator of the row it wrote. This is what closes the read-modify-write +// window: a settings change made between the read and this write makes the +// request fail with a precondition-failed APIError instead of reverting it. +// +// An empty etag sends no precondition and updates unconditionally, which is +// what UpdateSettings does. +func (a *AgentNetworkAPI) UpdateSettingsIfMatch(ctx context.Context, request api.PutApiAgentNetworkSettingsJSONRequestBody, etag string) (*api.AgentNetworkSettings, string, error) { requestBytes, err := json.Marshal(request) if err != nil { - return nil, err + return nil, "", err } - resp, err := a.c.NewRequest(ctx, "PUT", "/api/agent-network/settings", bytes.NewReader(requestBytes), nil) + resp, err := a.c.newRequest(ctx, "PUT", "/api/agent-network/settings", bytes.NewReader(requestBytes), nil, ifMatchHeader(etag)) if err != nil { - return nil, err + return nil, "", err } if resp.Body != nil { defer resp.Body.Close() } ret, err := parseResponse[api.AgentNetworkSettings](resp) - return &ret, err + if err != nil { + return nil, "", err + } + return &ret, etagFrom(resp), nil } // DeleteSettings deletes the account's Agent Network settings row, releasing @@ -405,7 +444,20 @@ func (a *AgentNetworkAPI) UpdateSettings(ctx context.Context, request api.PutApi // exists for the account or while a proxy is actively serving the endpoint. // Bootstrapping again afterwards allocates a new endpoint. func (a *AgentNetworkAPI) DeleteSettings(ctx context.Context) error { - resp, err := a.c.NewRequest(ctx, "DELETE", "/api/agent-network/settings", nil, nil) + return a.DeleteSettingsIfMatch(ctx, "") +} + +// DeleteSettingsIfMatch is DeleteSettings made conditional on etag — the +// validator from an earlier read — still being current. Sending it matters +// more here than on update: the server's other two refusals are about state +// (no providers, no serving proxy), so this is the only thing that stops a +// client working from an old read of one row from releasing the endpoint of +// the row that replaced it. +// +// An empty etag sends no precondition and deletes unconditionally, which is +// what DeleteSettings does. +func (a *AgentNetworkAPI) DeleteSettingsIfMatch(ctx context.Context, etag string) error { + resp, err := a.c.newRequest(ctx, "DELETE", "/api/agent-network/settings", nil, nil, ifMatchHeader(etag)) if err != nil { return err } @@ -415,3 +467,20 @@ func (a *AgentNetworkAPI) DeleteSettings(ctx context.Context) error { return nil } + +// etagFrom returns the bare validator from a response, with the transport's +// quoting stripped so a caller can hand it straight back to an If-Match +// parameter without knowing the wire syntax. +func etagFrom(resp *http.Response) string { + return strings.Trim(resp.Header.Get("ETag"), `"`) +} + +// ifMatchHeader renders the precondition headers for a bare validator, +// re-applying the quoting etagFrom stripped. An empty validator yields no +// headers at all — an unconditional request. +func ifMatchHeader(etag string) map[string]string { + if etag == "" { + return nil + } + return map[string]string{"If-Match": strconv.Quote(etag)} +} diff --git a/shared/management/client/rest/agentnetwork_test.go b/shared/management/client/rest/agentnetwork_test.go index 049aa6cfd..b30984ea3 100644 --- a/shared/management/client/rest/agentnetwork_test.go +++ b/shared/management/client/rest/agentnetwork_test.go @@ -559,3 +559,123 @@ func TestAgentNetwork_DeleteSettings_Guarded(t *testing.T) { assert.Contains(t, err.Error(), "cannot be deleted") }) } + +// TestAgentNetwork_GetSettings_ETag pins that the validator surfaces to the +// caller with the transport's quoting stripped, so it can be handed straight +// back to a conditional write without the caller knowing the wire syntax. +func TestAgentNetwork_GetSettings_ETag(t *testing.T) { + withMockClient(func(c *rest.Client, mux *http.ServeMux) { + mux.HandleFunc("/api/agent-network/settings", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("ETag", `"9f86d081884c7d65"`) + retBytes, _ := json.Marshal(testAgentNetworkSettings) + _, err := w.Write(retBytes) + require.NoError(t, err) + }) + ret, etag, err := c.AgentNetwork.GetSettingsWithETag(context.Background()) + require.NoError(t, err) + assert.Equal(t, testAgentNetworkSettings, *ret) + assert.Equal(t, "9f86d081884c7d65", etag, "the validator must arrive unquoted") + }) +} + +// TestAgentNetwork_UpdateSettings_IfMatch covers the round trip that makes the +// whole feature usable: a validator taken from a read goes back out quoted on +// the write, and the write's own validator comes back for the next one. +func TestAgentNetwork_UpdateSettings_IfMatch(t *testing.T) { + withMockClient(func(c *rest.Client, mux *http.ServeMux) { + mux.HandleFunc("/api/agent-network/settings", func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, `"9f86d081884c7d65"`, r.Header.Get("If-Match"), + "the precondition must go out quoted as a strong entity-tag") + w.Header().Set("ETag", `"0011223344556677"`) + retBytes, _ := json.Marshal(testAgentNetworkSettings) + _, err := w.Write(retBytes) + require.NoError(t, err) + }) + _, etag, err := c.AgentNetwork.UpdateSettingsIfMatch(context.Background(), api.PutApiAgentNetworkSettingsJSONRequestBody{ + Endpoint: "brave-otter.eu.proxy.netbird.io", + ProxyAddress: "eu.proxy.netbird.io", + EnableLogCollection: true, + }, "9f86d081884c7d65") + require.NoError(t, err) + assert.Equal(t, "0011223344556677", etag, "the write must return the new validator") + }) +} + +// TestAgentNetwork_UpdateSettings_NoPrecondition pins the back-compatible +// path: the plain method sends no If-Match at all, rather than an empty or +// wildcard one, so it stays the unconditional update it has always been. +func TestAgentNetwork_UpdateSettings_NoPrecondition(t *testing.T) { + withMockClient(func(c *rest.Client, mux *http.ServeMux) { + mux.HandleFunc("/api/agent-network/settings", func(w http.ResponseWriter, r *http.Request) { + assert.Empty(t, r.Header.Values("If-Match"), "an unconditional update must send no precondition") + retBytes, _ := json.Marshal(testAgentNetworkSettings) + _, err := w.Write(retBytes) + require.NoError(t, err) + }) + _, err := c.AgentNetwork.UpdateSettings(context.Background(), api.PutApiAgentNetworkSettingsJSONRequestBody{ + Endpoint: "brave-otter.eu.proxy.netbird.io", + ProxyAddress: "eu.proxy.netbird.io", + EnableLogCollection: true, + }) + require.NoError(t, err) + }) +} + +// TestAgentNetwork_UpdateSettings_StalePrecondition pins how a refused write +// reaches the caller: as an APIError a client can recognise as staleness and +// answer by reading again, rather than as an opaque failure. +func TestAgentNetwork_UpdateSettings_StalePrecondition(t *testing.T) { + withMockClient(func(c *rest.Client, mux *http.ServeMux) { + mux.HandleFunc("/api/agent-network/settings", func(w http.ResponseWriter, r *http.Request) { + retBytes, _ := json.Marshal(util.ErrorResponse{Message: "if-match precondition failed: the settings have changed since they were read; get them again and retry", Code: 412}) + w.WriteHeader(412) + _, err := w.Write(retBytes) + require.NoError(t, err) + }) + _, _, err := c.AgentNetwork.UpdateSettingsIfMatch(context.Background(), api.PutApiAgentNetworkSettingsJSONRequestBody{ + Endpoint: "brave-otter.eu.proxy.netbird.io", + ProxyAddress: "eu.proxy.netbird.io", + EnableLogCollection: true, + }, "9f86d081884c7d65") + require.Error(t, err) + assert.True(t, rest.IsPreconditionFailed(err), "a refused precondition must be recognisable as one") + assert.False(t, rest.IsNotFound(err), "it must not be confused with an unbootstrapped account") + }) +} + +// TestAgentNetwork_CreateSettings_ETag pins that the bootstrap hands back a +// validator, which is what lets a client follow it with a conditional write +// without an intervening read. +func TestAgentNetwork_CreateSettings_ETag(t *testing.T) { + withMockClient(func(c *rest.Client, mux *http.ServeMux) { + mux.HandleFunc("/api/agent-network/settings", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("ETag", `"9f86d081884c7d65"`) + retBytes, _ := json.Marshal(testAgentNetworkSettings) + _, err := w.Write(retBytes) + require.NoError(t, err) + }) + _, etag, err := c.AgentNetwork.CreateSettingsWithETag(context.Background(), api.PostApiAgentNetworkSettingsJSONRequestBody{ + ProxyAddress: ptr("eu.proxy.netbird.io"), + }) + require.NoError(t, err) + assert.Equal(t, "9f86d081884c7d65", etag, "the bootstrap must return a validator") + }) +} + +// TestAgentNetwork_DeleteSettings_IfMatch covers the conditional delete on the +// wire, and that the plain method still sends nothing. +func TestAgentNetwork_DeleteSettings_IfMatch(t *testing.T) { + withMockClient(func(c *rest.Client, mux *http.ServeMux) { + var seen []string + mux.HandleFunc("/api/agent-network/settings", func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "DELETE", r.Method) + seen = append(seen, r.Header.Get("If-Match")) + _, err := w.Write([]byte("{}")) + require.NoError(t, err) + }) + require.NoError(t, c.AgentNetwork.DeleteSettingsIfMatch(context.Background(), "9f86d081884c7d65")) + require.NoError(t, c.AgentNetwork.DeleteSettings(context.Background())) + assert.Equal(t, []string{`"9f86d081884c7d65"`, ""}, seen, + "the conditional delete must carry the quoted validator and the plain one must carry nothing") + }) +} diff --git a/shared/management/client/rest/client.go b/shared/management/client/rest/client.go index 6154a6637..c03e07823 100644 --- a/shared/management/client/rest/client.go +++ b/shared/management/client/rest/client.go @@ -31,6 +31,19 @@ func IsNotFound(err error) bool { return false } +// IsPreconditionFailed returns true if the error represents a 412 Precondition +// Failed response — an If-Match the server refused, or an endpoint's own +// precondition. A caller that sent a conditional request can use this to tell +// "someone else changed it, read again and retry" apart from a real failure; +// the message distinguishes it from an endpoint's other 412s. +func IsPreconditionFailed(err error) bool { + var apiErr *APIError + if ok := errors.As(err, &apiErr); ok { + return apiErr.StatusCode == http.StatusPreconditionFailed + } + return false +} + // Client Management service HTTP REST API Client type Client struct { managementURL string @@ -218,6 +231,12 @@ func (c *Client) initialize() { // NewRequest creates and executes new management API request func (c *Client) NewRequest(ctx context.Context, method, path string, body io.Reader, query map[string]string) (*http.Response, error) { + return c.newRequest(ctx, method, path, body, query, nil) +} + +// newRequest is NewRequest with request headers, for the endpoints whose +// contract includes one — conditional requests carrying If-Match. +func (c *Client) newRequest(ctx context.Context, method, path string, body io.Reader, query, headers map[string]string) (*http.Response, error) { req, err := http.NewRequestWithContext(ctx, method, c.managementURL+path, body) if err != nil { return nil, err @@ -231,6 +250,9 @@ func (c *Client) NewRequest(ctx context.Context, method, path string, body io.Re if c.userAgent != "" { req.Header.Set("User-Agent", c.userAgent) } + for name, value := range headers { + req.Header.Set(name, value) + } if len(query) != 0 { q := req.URL.Query()