From 3421784e3793f282669cd3edd2b49a1f8ccce46a Mon Sep 17 00:00:00 2001 From: Marwan Sulaiman Date: Mon, 11 Sep 2023 21:32:51 -0400 Subject: [PATCH] cmd/tailscale/cli: use optimistic concurrency control on SetServeConfig This PR uses the etag/if-match pattern to ensure multiple calls to SetServeConfig are synchronized. It currently errors out and asks the user to retry but we can add occ retries as a follow up. Updates #8489 Signed-off-by: Marwan Sulaiman --- client/tailscale/localclient.go | 67 ++++++++++++++++++++++++++++----- cmd/tailscale/cli/serve_dev.go | 3 ++ ipn/ipn_clone.go | 1 + ipn/ipn_view.go | 2 + ipn/serve.go | 6 +++ 5 files changed, 70 insertions(+), 9 deletions(-) diff --git a/client/tailscale/localclient.go b/client/tailscale/localclient.go index 1b2579772..32cb1041b 100644 --- a/client/tailscale/localclient.go +++ b/client/tailscale/localclient.go @@ -140,6 +140,10 @@ func (lc *LocalClient) doLocalRequestNiceError(req *http.Request) (*http.Respons all, _ := io.ReadAll(res.Body) return nil, &AccessDeniedError{errors.New(errorMessageFromBody(all))} } + if res.StatusCode == http.StatusPreconditionFailed { + all, _ := io.ReadAll(res.Body) + return nil, &PreconditionsFailedError{errors.New(errorMessageFromBody(all))} + } return res, nil } if ue, ok := err.(*url.Error); ok { @@ -170,6 +174,24 @@ func IsAccessDeniedError(err error) bool { return errors.As(err, &ae) } +// PreconditionsFailedError is returned when the server responds +// with an HTTP 412 status code. +type PreconditionsFailedError struct { + err error +} + +func (e *PreconditionsFailedError) Error() string { + return fmt.Sprintf("Preconditions failed: %v", e.err) +} + +func (e *PreconditionsFailedError) Unwrap() error { return e.err } + +// IsPreconditionsFailedError reports whether err is or wraps an PreconditionsFailedError. +func IsPreconditionsFailedError(err error) bool { + var ae *PreconditionsFailedError + return errors.As(err, &ae) +} + // bestError returns either err, or if body contains a valid JSON // object of type errorJSON, its non-empty error body. func bestError(err error, body []byte) error { @@ -198,27 +220,42 @@ func SetVersionMismatchHandler(f func(clientVer, serverVer string)) { } func (lc *LocalClient) send(ctx context.Context, method, path string, wantStatus int, body io.Reader) ([]byte, error) { + slurp, _, err := lc.sendWithHeaders(ctx, method, path, wantStatus, body, nil) + return slurp, err +} + +func (lc *LocalClient) sendWithHeaders( + ctx context.Context, + method, + path string, + wantStatus int, + body io.Reader, + h http.Header, +) ([]byte, http.Header, error) { if jr, ok := body.(jsonReader); ok && jr.err != nil { - return nil, jr.err // fail early if there was a JSON marshaling error + return nil, nil, jr.err // fail early if there was a JSON marshaling error } req, err := http.NewRequestWithContext(ctx, method, "http://"+apitype.LocalAPIHost+path, body) if err != nil { - return nil, err + return nil, nil, err + } + if h != nil { + req.Header = h } res, err := lc.doLocalRequestNiceError(req) if err != nil { - return nil, err + return nil, nil, err } defer res.Body.Close() slurp, err := io.ReadAll(res.Body) if err != nil { - return nil, err + return nil, nil, err } if res.StatusCode != wantStatus { err = fmt.Errorf("%v: %s", res.Status, bytes.TrimSpace(slurp)) - return nil, bestError(err, slurp) + return nil, nil, bestError(err, slurp) } - return slurp, nil + return slurp, res.Header, nil } func (lc *LocalClient) get200(ctx context.Context, path string) ([]byte, error) { @@ -1093,7 +1130,11 @@ func (lc *LocalClient) NetworkLockSubmitRecoveryAUM(ctx context.Context, aum tka // SetServeConfig sets or replaces the serving settings. // If config is nil, settings are cleared and serving is disabled. func (lc *LocalClient) SetServeConfig(ctx context.Context, config *ipn.ServeConfig) error { - _, err := lc.send(ctx, "POST", "/localapi/v0/serve-config", 200, jsonBody(config)) + h := make(http.Header) + if config != nil { + h.Set("If-Match", config.ETag) + } + _, _, err := lc.sendWithHeaders(ctx, "POST", "/localapi/v0/serve-config", 200, jsonBody(config), h) if err != nil { return fmt.Errorf("sending serve config: %w", err) } @@ -1112,11 +1153,19 @@ func (lc *LocalClient) NetworkLockDisable(ctx context.Context, secret []byte) er // // If the serve config is empty, it returns (nil, nil). func (lc *LocalClient) GetServeConfig(ctx context.Context) (*ipn.ServeConfig, error) { - body, err := lc.send(ctx, "GET", "/localapi/v0/serve-config", 200, nil) + body, h, err := lc.sendWithHeaders(ctx, "GET", "/localapi/v0/serve-config", 200, nil, nil) if err != nil { return nil, fmt.Errorf("getting serve config: %w", err) } - return getServeConfigFromJSON(body) + sc, err := getServeConfigFromJSON(body) + if err != nil { + return nil, err + } + if sc == nil { + sc = new(ipn.ServeConfig) + } + sc.ETag = h.Get("Etag") + return sc, nil } func getServeConfigFromJSON(body []byte) (sc *ipn.ServeConfig, err error) { diff --git a/cmd/tailscale/cli/serve_dev.go b/cmd/tailscale/cli/serve_dev.go index 64cb875fd..967eb2680 100644 --- a/cmd/tailscale/cli/serve_dev.go +++ b/cmd/tailscale/cli/serve_dev.go @@ -275,6 +275,9 @@ func (e *serveEnv) runServeCombined(subcmd serveMode) execFunc { } if err := e.lc.SetServeConfig(ctx, parentSC); err != nil { + if tailscale.IsPreconditionsFailedError(err) { + fmt.Fprintln(os.Stderr, "Another client is changing the serve config; please try again.") + } return err } diff --git a/ipn/ipn_clone.go b/ipn/ipn_clone.go index 5f30caa92..68c942dfb 100644 --- a/ipn/ipn_clone.go +++ b/ipn/ipn_clone.go @@ -91,6 +91,7 @@ var _ServeConfigCloneNeedsRegeneration = ServeConfig(struct { Web map[HostPort]*WebServerConfig AllowFunnel map[HostPort]bool Foreground map[string]*ServeConfig + ETag string }{}) // Clone makes a deep copy of TCPPortHandler. diff --git a/ipn/ipn_view.go b/ipn/ipn_view.go index cd89fa151..dbbf37476 100644 --- a/ipn/ipn_view.go +++ b/ipn/ipn_view.go @@ -182,6 +182,7 @@ func (v ServeConfigView) Foreground() views.MapFn[string, *ServeConfig, ServeCon return t.View() }) } +func (v ServeConfigView) ETag() string { return v.ж.ETag } // A compilation failure here means this code must be regenerated, with the command at the top of this file. var _ServeConfigViewNeedsRegeneration = ServeConfig(struct { @@ -189,6 +190,7 @@ var _ServeConfigViewNeedsRegeneration = ServeConfig(struct { Web map[HostPort]*WebServerConfig AllowFunnel map[HostPort]bool Foreground map[string]*ServeConfig + ETag string }{}) // View returns a readonly view of TCPPortHandler. diff --git a/ipn/serve.go b/ipn/serve.go index 6b0a593a7..08d4dd747 100644 --- a/ipn/serve.go +++ b/ipn/serve.go @@ -44,6 +44,12 @@ type ServeConfig struct { // of either the client or the LocalBackend does not expose ports // that users are not aware of. Foreground map[string]*ServeConfig `json:",omitempty"` + + // ETag is the checksum of the serve config that's populated + // by the LocalClient through the HTTP ETag header during a + // GetServeConfig request and is translated to an If-Match header + // during a SetServeConfig request. + ETag string `json:"-"` } // HostPort is an SNI name and port number, joined by a colon.