From 9f0f22c036e6dc0c729b02be2a8a0f590e0620fd Mon Sep 17 00:00:00 2001 From: chungthuang Date: Thu, 22 Aug 2024 08:34:27 -0400 Subject: [PATCH 01/14] Release 2024.8.3 --- RELEASE_NOTES | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/RELEASE_NOTES b/RELEASE_NOTES index a40a0251..77838d9b 100644 --- a/RELEASE_NOTES +++ b/RELEASE_NOTES @@ -1,3 +1,8 @@ +2024.8.3 +- 2024-08-15 TUN-8591 login command without extra text +- 2024-03-25 remove code that will not be executed +- 2024-03-25 remove code that will not be executed + 2024.8.2 - 2024-08-05 TUN-8583: change final directory of artifacts - 2024-08-05 TUN-8585: Avoid creating GH client when dry-run is true From d6b0833209e9b45921b20dfd4c6e1f56e9ba88fa Mon Sep 17 00:00:00 2001 From: chungthuang Date: Fri, 9 Aug 2024 14:43:35 -0500 Subject: [PATCH 02/14] TUN-8592: Use metadata from the edge to determine if request body is empty for QUIC transport If the metadata is missing, fallback to decide based on protocol, http method, transferring and content length --- connection/quic.go | 53 +++++++++++++++--- connection/quic_test.go | 119 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 164 insertions(+), 8 deletions(-) diff --git a/connection/quic.go b/connection/quic.go index c5e218f3..e1048e3a 100644 --- a/connection/quic.go +++ b/connection/quic.go @@ -42,12 +42,26 @@ const ( HTTPMethodKey = "HttpMethod" // HTTPHostKey is used to get or set http Method in QUIC ALPN if the underlying proxy connection type is HTTP. HTTPHostKey = "HttpHost" + // HTTPRequestBodyHintKey is used in ConnectRequest metadata to indicate if the request has body + HTTPRequestBodyHintKey = "HttpReqBodyHint" QUICMetadataFlowID = "FlowID" // emperically this capacity has been working well demuxChanCapacity = 16 ) +type RequestBodyHint uint64 + +const ( + RequestBodyHintMissing RequestBodyHint = iota + RequestBodyHintEmpty + RequestBodyHintHasData +) + +func (rbh RequestBodyHint) String() string { + return [...]string{"missing", "empty", "data"}[rbh] +} + var ( portForConnIndex = make(map[uint8]int, 0) portMapMutex sync.Mutex @@ -474,7 +488,6 @@ func buildHTTPRequest( dest := connectRequest.Dest method := metadata[HTTPMethodKey] host := metadata[HTTPHostKey] - isWebsocket := connectRequest.Type == pogs.ConnectionTypeWebsocket req, err := http.NewRequestWithContext(ctx, method, dest, body) if err != nil { @@ -499,13 +512,8 @@ func buildHTTPRequest( return nil, fmt.Errorf("Error setting content-length: %w", err) } - // Go's client defaults to chunked encoding after a 200ms delay if the following cases are true: - // * the request body blocks - // * the content length is not set (or set to -1) - // * the method doesn't usually have a body (GET, HEAD, DELETE, ...) - // * there is no transfer-encoding=chunked already set. - // So, if transfer cannot be chunked and content length is 0, we dont set a request body. - if !isWebsocket && !isTransferEncodingChunked(req) && req.ContentLength == 0 { + if shouldSetRequestBodyToEmpty(connectRequest, metadata, req) { + log.Debug().Str("host", req.Host).Str("method", req.Method).Msg("Set request to have no body") req.Body = http.NoBody } stripWebsocketUpgradeHeader(req) @@ -530,6 +538,35 @@ func isTransferEncodingChunked(req *http.Request) bool { return strings.Contains(strings.ToLower(transferEncodingVal), "chunked") } +// Borrowed from https://github.com/golang/go/blob/go1.22.6/src/net/http/request.go#L1541 +func requestMethodUsuallyLacksBody(req *http.Request) bool { + switch strings.ToUpper(req.Method) { + case "GET", "HEAD", "DELETE", "OPTIONS", "PROPFIND", "SEARCH": + return true + } + return false +} + +func shouldSetRequestBodyToEmpty(connectRequest *pogs.ConnectRequest, metadata map[string]string, req *http.Request) bool { + switch metadata[HTTPRequestBodyHintKey] { + case RequestBodyHintEmpty.String(): + return true + case RequestBodyHintHasData.String(): + return false + default: + } + + isWebsocket := connectRequest.Type == pogs.ConnectionTypeWebsocket + // Go's client defaults to chunked encoding after a 200ms delay if the following cases are true: + // * the request body blocks + // * the content length is not set (or set to -1) + // * the method doesn't usually have a body (GET, HEAD, DELETE, ...) + // * there is no transfer-encoding=chunked already set. + // So, if transfer cannot be chunked and content length is 0, we dont set a request body. + // Reference: https://github.com/golang/go/blob/go1.22.2/src/net/http/transfer.go#L192-L206 + return !isWebsocket && requestMethodUsuallyLacksBody(req) && !isTransferEncodingChunked(req) && req.ContentLength == 0 +} + // A helper struct that guarantees a call to close only affects read side, but not write side. type nopCloserReadWriter struct { io.ReadWriteCloser diff --git a/connection/quic_test.go b/connection/quic_test.go index c81d53fb..302cb7f9 100644 --- a/connection/quic_test.go +++ b/connection/quic_test.go @@ -484,6 +484,125 @@ func TestBuildHTTPRequest(t *testing.T) { }, body: io.NopCloser(&bytes.Buffer{}), }, + { + name: "if edge sends the body is empty hint, set body to empty", + connectRequest: &pogs.ConnectRequest{ + Dest: "http://test.com", + Metadata: []pogs.Metadata{ + { + Key: "HttpHeader:Another-Header", + Val: "Misc", + }, + { + Key: "HttpHost", + Val: "cf.host", + }, + { + Key: "HttpMethod", + Val: "put", + }, + { + Key: HTTPRequestBodyHintKey, + Val: RequestBodyHintEmpty.String(), + }, + }, + }, + req: &http.Request{ + Method: "put", + URL: &url.URL{ + Scheme: "http", + Host: "test.com", + }, + Proto: "HTTP/1.1", + ProtoMajor: 1, + ProtoMinor: 1, + Header: http.Header{ + "Another-Header": []string{"Misc"}, + }, + ContentLength: 0, + Host: "cf.host", + Body: http.NoBody, + }, + body: io.NopCloser(&bytes.Buffer{}), + }, + { + name: "if edge sends the body has data hint, don't set body to empty", + connectRequest: &pogs.ConnectRequest{ + Dest: "http://test.com", + Metadata: []pogs.Metadata{ + { + Key: "HttpHeader:Another-Header", + Val: "Misc", + }, + { + Key: "HttpHost", + Val: "cf.host", + }, + { + Key: "HttpMethod", + Val: "put", + }, + { + Key: HTTPRequestBodyHintKey, + Val: RequestBodyHintHasData.String(), + }, + }, + }, + req: &http.Request{ + Method: "put", + URL: &url.URL{ + Scheme: "http", + Host: "test.com", + }, + Proto: "HTTP/1.1", + ProtoMajor: 1, + ProtoMinor: 1, + Header: http.Header{ + "Another-Header": []string{"Misc"}, + }, + ContentLength: 0, + Host: "cf.host", + Body: io.NopCloser(&bytes.Buffer{}), + }, + body: io.NopCloser(&bytes.Buffer{}), + }, + { + name: "if the http method usually has body, don't set body to empty", + connectRequest: &pogs.ConnectRequest{ + Dest: "http://test.com", + Metadata: []pogs.Metadata{ + { + Key: "HttpHeader:Another-Header", + Val: "Misc", + }, + { + Key: "HttpHost", + Val: "cf.host", + }, + { + Key: "HttpMethod", + Val: "post", + }, + }, + }, + req: &http.Request{ + Method: "post", + URL: &url.URL{ + Scheme: "http", + Host: "test.com", + }, + Proto: "HTTP/1.1", + ProtoMajor: 1, + ProtoMinor: 1, + Header: http.Header{ + "Another-Header": []string{"Misc"}, + }, + ContentLength: 0, + Host: "cf.host", + Body: io.NopCloser(&bytes.Buffer{}), + }, + body: io.NopCloser(&bytes.Buffer{}), + }, } log := zerolog.Nop() From ab0bce58f84193767a1751a191e2ef182306e015 Mon Sep 17 00:00:00 2001 From: GoncaloGarcia Date: Wed, 26 Jun 2024 14:17:20 +0100 Subject: [PATCH 03/14] TUN-8484: Print response when QuickTunnel can't be unmarshalled --- cmd/cloudflared/tunnel/quick_tunnel.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/cmd/cloudflared/tunnel/quick_tunnel.go b/cmd/cloudflared/tunnel/quick_tunnel.go index 64013e58..d262fb9b 100644 --- a/cmd/cloudflared/tunnel/quick_tunnel.go +++ b/cmd/cloudflared/tunnel/quick_tunnel.go @@ -3,6 +3,7 @@ package tunnel import ( "encoding/json" "fmt" + "io" "net/http" "strings" "time" @@ -47,8 +48,17 @@ func RunQuickTunnel(sc *subcommandContext) error { } defer resp.Body.Close() + // This will read the entire response into memory so we can print it in case of error + rsp_body, err := io.ReadAll(resp.Body) + if err != nil { + return errors.Wrap(err, "failed to read quick-tunnel response") + } + var data QuickTunnelResponse - if err := json.NewDecoder(resp.Body).Decode(&data); err != nil { + if err := json.Unmarshal(rsp_body, &data); err != nil { + rsp_string := string(rsp_body) + fields := map[string]interface{}{"status_code": resp.Status} + sc.log.Err(err).Fields(fields).Msgf("Error unmarshaling QuickTunnel response: %s", rsp_string) return errors.Wrap(err, "failed to unmarshal quick Tunnel") } From e05939f1c9b9405c11dd4c8684c4cb6f7f359ad3 Mon Sep 17 00:00:00 2001 From: GoncaloGarcia Date: Fri, 30 Aug 2024 12:51:20 +0100 Subject: [PATCH 04/14] TUN-8621: Prevent QUIC connection from closing before grace period after unregistering Whenever cloudflared receives a SIGTERM or SIGINT it goes into graceful shutdown mode, which unregisters the connection and closes the control stream. Unregistering makes it so we no longer receive any new requests and makes the edge close the connection, allowing in-flight requests to finish (within a 3 minute period). This was working fine for http2 connections, but the quic proxy was cancelling the context as soon as the controls stream ended, forcing the process to stop immediately. This commit changes the behavior so that we wait the full grace period before cancelling the request --- CHANGES.md | 4 ++++ component-tests/test_termination.py | 23 ++++++++++++++++------- connection/control.go | 15 +++++++++++---- connection/http2_test.go | 3 ++- connection/quic.go | 16 ++++++++++++++-- connection/quic_test.go | 1 + supervisor/tunnel.go | 1 + tunnelrpc/registration_client.go | 6 ++++-- 8 files changed, 53 insertions(+), 16 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index ba3cac48..f01f62c6 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,7 @@ +## 2024.9.1 +### Bug Fixes +- We fixed a bug related to `--grace-period`. Tunnels that use QUIC as transport weren't abiding by this waiting period before forcefully closing the connections to the edge. From now on, both QUIC and HTTP2 tunnels will wait for either the grace period to end (defaults to 30 seconds) or until the last in-flight request is handled. Users that wish to maintain the previous behavior should set `--grace-period` to 0 if `--protocol` is set to `quic`. This will force `cloudflared` to shutdown as soon as either SIGTERM or SIGINT is received. + ## 2024.2.1 ### Notices - Starting from this version, tunnel diagnostics will be enabled by default. This will allow the engineering team to remotely get diagnostics from cloudflared during debug activities. Users still have the capability to opt-out of this feature by defining `--management-diagnostics=false` (or env `TUNNEL_MANAGEMENT_DIAGNOSTICS`). diff --git a/component-tests/test_termination.py b/component-tests/test_termination.py index 26f4fea4..128d95d6 100644 --- a/component-tests/test_termination.py +++ b/component-tests/test_termination.py @@ -45,9 +45,10 @@ class TestTermination: with connected: connected.wait(self.timeout) # Send signal after the SSE connection is established - self.terminate_by_signal(cloudflared, signal) - self.wait_eyeball_thread( - in_flight_req, self.grace_period + self.timeout) + with self.within_grace_period(): + self.terminate_by_signal(cloudflared, signal) + self.wait_eyeball_thread( + in_flight_req, self.grace_period + self.timeout) # test cloudflared terminates before grace period expires when all eyeball # connections are drained @@ -66,7 +67,7 @@ class TestTermination: with connected: connected.wait(self.timeout) - with self.within_grace_period(): + with self.within_grace_period(has_connection=False): # Send signal after the SSE connection is established self.terminate_by_signal(cloudflared, signal) self.wait_eyeball_thread(in_flight_req, self.grace_period) @@ -78,7 +79,7 @@ class TestTermination: with start_cloudflared( tmp_path, config, cfd_pre_args=["tunnel", "--ha-connections", "1"], new_process=True, capture_output=False) as cloudflared: wait_tunnel_ready(tunnel_url=config.get_url()) - with self.within_grace_period(): + with self.within_grace_period(has_connection=False): self.terminate_by_signal(cloudflared, signal) def terminate_by_signal(self, cloudflared, sig): @@ -92,13 +93,21 @@ class TestTermination: # Using this context asserts logic within the context is executed within grace period @contextmanager - def within_grace_period(self): + def within_grace_period(self, has_connection=True): try: start = time.time() yield finally: + + # If the request takes longer than the grace period then we need to wait at most the grace period. + # If the request fell within the grace period cloudflared can close earlier, but to ensure that it doesn't + # close immediately we add a minimum boundary. If cloudflared shutdown in less than 1s it's likely that + # it shutdown as soon as it received SIGINT. The only way cloudflared can close immediately is if it has no + # in-flight requests + minimum = 1 if has_connection else 0 duration = time.time() - start - assert duration < self.grace_period + # Here we truncate to ensure that we don't fail on minute differences like 10.1 instead of 10 + assert minimum <= int(duration) <= self.grace_period def stream_request(self, config, connected, early_terminate): expected_terminate_message = "502 Bad Gateway" diff --git a/connection/control.go b/connection/control.go index e0bfeae9..94e0d66b 100644 --- a/connection/control.go +++ b/connection/control.go @@ -6,6 +6,8 @@ import ( "net" "time" + "github.com/pkg/errors" + "github.com/cloudflare/cloudflared/management" "github.com/cloudflare/cloudflared/tunnelrpc" tunnelpogs "github.com/cloudflare/cloudflared/tunnelrpc/pogs" @@ -116,27 +118,32 @@ func (c *controlStream) ServeControlStream( } } - c.waitForUnregister(ctx, registrationClient) - return nil + return c.waitForUnregister(ctx, registrationClient) } -func (c *controlStream) waitForUnregister(ctx context.Context, registrationClient tunnelrpc.RegistrationClient) { +func (c *controlStream) waitForUnregister(ctx context.Context, registrationClient tunnelrpc.RegistrationClient) error { // wait for connection termination or start of graceful shutdown defer registrationClient.Close() + var shutdownError error select { case <-ctx.Done(): + shutdownError = ctx.Err() break case <-c.gracefulShutdownC: c.stoppedGracefully = true } c.observer.sendUnregisteringEvent(c.connIndex) - registrationClient.GracefulShutdown(ctx, c.gracePeriod) + err := registrationClient.GracefulShutdown(ctx, c.gracePeriod) + if err != nil { + return errors.Wrap(err, "Error shutting down control stream") + } c.observer.log.Info(). Int(management.EventTypeKey, int(management.Cloudflared)). Uint8(LogFieldConnIndex, c.connIndex). IPAddr(LogFieldIPAddress, c.edgeAddress). Msg("Unregistered tunnel connection") + return shutdownError } func (c *controlStream) IsStopped() bool { diff --git a/connection/http2_test.go b/connection/http2_test.go index a0ec8b45..92665688 100644 --- a/connection/http2_test.go +++ b/connection/http2_test.go @@ -192,8 +192,9 @@ func (mc mockNamedTunnelRPCClient) RegisterConnection( }, nil } -func (mc mockNamedTunnelRPCClient) GracefulShutdown(ctx context.Context, gracePeriod time.Duration) { +func (mc mockNamedTunnelRPCClient) GracefulShutdown(ctx context.Context, gracePeriod time.Duration) error { close(mc.unregistered) + return nil } func (mockNamedTunnelRPCClient) Close() {} diff --git a/connection/quic.go b/connection/quic.go index e1048e3a..b24a6cce 100644 --- a/connection/quic.go +++ b/connection/quic.go @@ -83,6 +83,7 @@ type QUICConnection struct { rpcTimeout time.Duration streamWriteTimeout time.Duration + gracePeriod time.Duration } // NewQUICConnection returns a new instance of QUICConnection. @@ -100,6 +101,7 @@ func NewQUICConnection( packetRouterConfig *ingress.GlobalRouterConfig, rpcTimeout time.Duration, streamWriteTimeout time.Duration, + gracePeriod time.Duration, ) (*QUICConnection, error) { udpConn, err := createUDPConnForConnIndex(connIndex, localAddr, logger) if err != nil { @@ -136,6 +138,7 @@ func NewQUICConnection( connIndex: connIndex, rpcTimeout: rpcTimeout, streamWriteTimeout: streamWriteTimeout, + gracePeriod: gracePeriod, }, nil } @@ -158,8 +161,17 @@ func (q *QUICConnection) Serve(ctx context.Context) error { // In the future, if cloudflared can autonomously push traffic to the edge, we have to make sure the control // stream is already fully registered before the other goroutines can proceed. errGroup.Go(func() error { - defer cancel() - return q.serveControlStream(ctx, controlStream) + // err is equal to nil if we exit due to unregistration. If that happens we want to wait the full + // amount of the grace period, allowing requests to finish before we cancel the context, which will + // make cloudflared exit. + if err := q.serveControlStream(ctx, controlStream); err == nil { + select { + case <-ctx.Done(): + case <-time.Tick(q.gracePeriod): + } + } + cancel() + return err }) errGroup.Go(func() error { defer cancel() diff --git a/connection/quic_test.go b/connection/quic_test.go index 302cb7f9..0a54e345 100644 --- a/connection/quic_test.go +++ b/connection/quic_test.go @@ -855,6 +855,7 @@ func testQUICConnection(udpListenerAddr net.Addr, t *testing.T, index uint8) *QU nil, 15*time.Second, 0*time.Second, + 0*time.Second, ) require.NoError(t, err) return qc diff --git a/supervisor/tunnel.go b/supervisor/tunnel.go index 85637798..03d7f930 100644 --- a/supervisor/tunnel.go +++ b/supervisor/tunnel.go @@ -604,6 +604,7 @@ func (e *EdgeTunnelServer) serveQUIC( e.config.PacketConfig, e.config.RPCTimeout, e.config.WriteStreamTimeout, + e.config.GracePeriod, ) if err != nil { connLogger.ConnAwareLogger().Err(err).Msgf("Failed to create new quic connection") diff --git a/tunnelrpc/registration_client.go b/tunnelrpc/registration_client.go index f41819f3..7d945f58 100644 --- a/tunnelrpc/registration_client.go +++ b/tunnelrpc/registration_client.go @@ -23,7 +23,7 @@ type RegistrationClient interface { edgeAddress net.IP, ) (*pogs.ConnectionDetails, error) SendLocalConfiguration(ctx context.Context, config []byte) error - GracefulShutdown(ctx context.Context, gracePeriod time.Duration) + GracefulShutdown(ctx context.Context, gracePeriod time.Duration) error Close() } @@ -79,7 +79,7 @@ func (r *registrationClient) SendLocalConfiguration(ctx context.Context, config return err } -func (r *registrationClient) GracefulShutdown(ctx context.Context, gracePeriod time.Duration) { +func (r *registrationClient) GracefulShutdown(ctx context.Context, gracePeriod time.Duration) error { ctx, cancel := context.WithTimeout(ctx, gracePeriod) defer cancel() defer metrics.CapnpMetrics.ClientOperations.WithLabelValues(metrics.Registration, metrics.OperationUnregisterConnection).Inc() @@ -88,7 +88,9 @@ func (r *registrationClient) GracefulShutdown(ctx context.Context, gracePeriod t err := r.client.UnregisterConnection(ctx) if err != nil { metrics.CapnpMetrics.ClientFailures.WithLabelValues(metrics.Registration, metrics.OperationUnregisterConnection).Inc() + return err } + return nil } func (r *registrationClient) Close() { From a29184a171b56cbee7511b89790f39b10dd621c5 Mon Sep 17 00:00:00 2001 From: Devin Carr Date: Fri, 6 Sep 2024 11:33:42 -0700 Subject: [PATCH 05/14] PPIP-2310: Update quick tunnel disclaimer --- cmd/cloudflared/tunnel/quick_tunnel.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/cmd/cloudflared/tunnel/quick_tunnel.go b/cmd/cloudflared/tunnel/quick_tunnel.go index d262fb9b..ee438450 100644 --- a/cmd/cloudflared/tunnel/quick_tunnel.go +++ b/cmd/cloudflared/tunnel/quick_tunnel.go @@ -16,10 +16,7 @@ import ( const httpTimeout = 15 * time.Second -const disclaimer = "Thank you for trying Cloudflare Tunnel. Doing so, without a Cloudflare account, is a quick way to" + - " experiment and try it out. However, be aware that these account-less Tunnels have no uptime guarantee. If you " + - "intend to use Tunnels in production you should use a pre-created named tunnel by following: " + - "https://developers.cloudflare.com/cloudflare-one/connections/connect-apps" +const disclaimer = "Thank you for trying Cloudflare Tunnel. Doing so, without a Cloudflare account, is a quick way to experiment and try it out. However, be aware that these account-less Tunnels have no uptime guarantee, are subject to the Cloudflare Online Services Terms of Use (https://www.cloudflare.com/website-terms/), and Cloudflare reserves the right to investigate your use of Tunnels for violations of such terms. If you intend to use Tunnels in production you should use a pre-created named tunnel by following: https://developers.cloudflare.com/cloudflare-one/connections/connect-apps" // RunQuickTunnel requests a tunnel from the specified service. // We use this to power quick tunnels on trycloudflare.com, but the From 3ac69f2d062bc73815626867789dc02243bceb5c Mon Sep 17 00:00:00 2001 From: GoncaloGarcia Date: Tue, 10 Sep 2024 10:00:56 +0100 Subject: [PATCH 06/14] TUN-8621: Fix cloudflared version in change notes. --- CHANGES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index f01f62c6..e3a8a23c 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,4 +1,4 @@ -## 2024.9.1 +## 2024.9.0 ### Bug Fixes - We fixed a bug related to `--grace-period`. Tunnels that use QUIC as transport weren't abiding by this waiting period before forcefully closing the connections to the edge. From now on, both QUIC and HTTP2 tunnels will wait for either the grace period to end (defaults to 30 seconds) or until the last in-flight request is handled. Users that wish to maintain the previous behavior should set `--grace-period` to 0 if `--protocol` is set to `quic`. This will force `cloudflared` to shutdown as soon as either SIGTERM or SIGINT is received. From ec072691229aa2508cfa465d228db96cade96484 Mon Sep 17 00:00:00 2001 From: GoncaloGarcia Date: Mon, 9 Sep 2024 20:06:26 +0100 Subject: [PATCH 07/14] Release 2024.9.0 --- RELEASE_NOTES | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/RELEASE_NOTES b/RELEASE_NOTES index 77838d9b..8fd1175a 100644 --- a/RELEASE_NOTES +++ b/RELEASE_NOTES @@ -1,3 +1,10 @@ +2024.9.0 +- 2024-09-10 TUN-8621: Fix cloudflared version in change notes. +- 2024-09-06 PPIP-2310: Update quick tunnel disclaimer +- 2024-08-30 TUN-8621: Prevent QUIC connection from closing before grace period after unregistering +- 2024-08-09 TUN-8592: Use metadata from the edge to determine if request body is empty for QUIC transport +- 2024-06-26 TUN-8484: Print response when QuickTunnel can't be unmarshalled + 2024.8.3 - 2024-08-15 TUN-8591 login command without extra text - 2024-03-25 remove code that will not be executed From 2437675c04927ba78f452d6191a778e5bc734b06 Mon Sep 17 00:00:00 2001 From: GoncaloGarcia Date: Tue, 10 Sep 2024 16:47:36 +0100 Subject: [PATCH 08/14] Reverts the following: Revert "TUN-8621: Fix cloudflared version in change notes." Revert "PPIP-2310: Update quick tunnel disclaimer" Revert "TUN-8621: Prevent QUIC connection from closing before grace period after unregistering" Revert "TUN-8484: Print response when QuickTunnel can't be unmarshalled" Revert "TUN-8592: Use metadata from the edge to determine if request body is empty for QUIC transport" --- CHANGES.md | 4 - cmd/cloudflared/tunnel/quick_tunnel.go | 17 ++-- component-tests/test_termination.py | 23 ++--- connection/control.go | 15 +--- connection/http2_test.go | 3 +- connection/quic.go | 69 +++----------- connection/quic_test.go | 120 ------------------------- supervisor/tunnel.go | 1 - tunnelrpc/registration_client.go | 6 +- 9 files changed, 29 insertions(+), 229 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index e3a8a23c..ba3cac48 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,7 +1,3 @@ -## 2024.9.0 -### Bug Fixes -- We fixed a bug related to `--grace-period`. Tunnels that use QUIC as transport weren't abiding by this waiting period before forcefully closing the connections to the edge. From now on, both QUIC and HTTP2 tunnels will wait for either the grace period to end (defaults to 30 seconds) or until the last in-flight request is handled. Users that wish to maintain the previous behavior should set `--grace-period` to 0 if `--protocol` is set to `quic`. This will force `cloudflared` to shutdown as soon as either SIGTERM or SIGINT is received. - ## 2024.2.1 ### Notices - Starting from this version, tunnel diagnostics will be enabled by default. This will allow the engineering team to remotely get diagnostics from cloudflared during debug activities. Users still have the capability to opt-out of this feature by defining `--management-diagnostics=false` (or env `TUNNEL_MANAGEMENT_DIAGNOSTICS`). diff --git a/cmd/cloudflared/tunnel/quick_tunnel.go b/cmd/cloudflared/tunnel/quick_tunnel.go index ee438450..64013e58 100644 --- a/cmd/cloudflared/tunnel/quick_tunnel.go +++ b/cmd/cloudflared/tunnel/quick_tunnel.go @@ -3,7 +3,6 @@ package tunnel import ( "encoding/json" "fmt" - "io" "net/http" "strings" "time" @@ -16,7 +15,10 @@ import ( const httpTimeout = 15 * time.Second -const disclaimer = "Thank you for trying Cloudflare Tunnel. Doing so, without a Cloudflare account, is a quick way to experiment and try it out. However, be aware that these account-less Tunnels have no uptime guarantee, are subject to the Cloudflare Online Services Terms of Use (https://www.cloudflare.com/website-terms/), and Cloudflare reserves the right to investigate your use of Tunnels for violations of such terms. If you intend to use Tunnels in production you should use a pre-created named tunnel by following: https://developers.cloudflare.com/cloudflare-one/connections/connect-apps" +const disclaimer = "Thank you for trying Cloudflare Tunnel. Doing so, without a Cloudflare account, is a quick way to" + + " experiment and try it out. However, be aware that these account-less Tunnels have no uptime guarantee. If you " + + "intend to use Tunnels in production you should use a pre-created named tunnel by following: " + + "https://developers.cloudflare.com/cloudflare-one/connections/connect-apps" // RunQuickTunnel requests a tunnel from the specified service. // We use this to power quick tunnels on trycloudflare.com, but the @@ -45,17 +47,8 @@ func RunQuickTunnel(sc *subcommandContext) error { } defer resp.Body.Close() - // This will read the entire response into memory so we can print it in case of error - rsp_body, err := io.ReadAll(resp.Body) - if err != nil { - return errors.Wrap(err, "failed to read quick-tunnel response") - } - var data QuickTunnelResponse - if err := json.Unmarshal(rsp_body, &data); err != nil { - rsp_string := string(rsp_body) - fields := map[string]interface{}{"status_code": resp.Status} - sc.log.Err(err).Fields(fields).Msgf("Error unmarshaling QuickTunnel response: %s", rsp_string) + if err := json.NewDecoder(resp.Body).Decode(&data); err != nil { return errors.Wrap(err, "failed to unmarshal quick Tunnel") } diff --git a/component-tests/test_termination.py b/component-tests/test_termination.py index 128d95d6..26f4fea4 100644 --- a/component-tests/test_termination.py +++ b/component-tests/test_termination.py @@ -45,10 +45,9 @@ class TestTermination: with connected: connected.wait(self.timeout) # Send signal after the SSE connection is established - with self.within_grace_period(): - self.terminate_by_signal(cloudflared, signal) - self.wait_eyeball_thread( - in_flight_req, self.grace_period + self.timeout) + self.terminate_by_signal(cloudflared, signal) + self.wait_eyeball_thread( + in_flight_req, self.grace_period + self.timeout) # test cloudflared terminates before grace period expires when all eyeball # connections are drained @@ -67,7 +66,7 @@ class TestTermination: with connected: connected.wait(self.timeout) - with self.within_grace_period(has_connection=False): + with self.within_grace_period(): # Send signal after the SSE connection is established self.terminate_by_signal(cloudflared, signal) self.wait_eyeball_thread(in_flight_req, self.grace_period) @@ -79,7 +78,7 @@ class TestTermination: with start_cloudflared( tmp_path, config, cfd_pre_args=["tunnel", "--ha-connections", "1"], new_process=True, capture_output=False) as cloudflared: wait_tunnel_ready(tunnel_url=config.get_url()) - with self.within_grace_period(has_connection=False): + with self.within_grace_period(): self.terminate_by_signal(cloudflared, signal) def terminate_by_signal(self, cloudflared, sig): @@ -93,21 +92,13 @@ class TestTermination: # Using this context asserts logic within the context is executed within grace period @contextmanager - def within_grace_period(self, has_connection=True): + def within_grace_period(self): try: start = time.time() yield finally: - - # If the request takes longer than the grace period then we need to wait at most the grace period. - # If the request fell within the grace period cloudflared can close earlier, but to ensure that it doesn't - # close immediately we add a minimum boundary. If cloudflared shutdown in less than 1s it's likely that - # it shutdown as soon as it received SIGINT. The only way cloudflared can close immediately is if it has no - # in-flight requests - minimum = 1 if has_connection else 0 duration = time.time() - start - # Here we truncate to ensure that we don't fail on minute differences like 10.1 instead of 10 - assert minimum <= int(duration) <= self.grace_period + assert duration < self.grace_period def stream_request(self, config, connected, early_terminate): expected_terminate_message = "502 Bad Gateway" diff --git a/connection/control.go b/connection/control.go index 94e0d66b..e0bfeae9 100644 --- a/connection/control.go +++ b/connection/control.go @@ -6,8 +6,6 @@ import ( "net" "time" - "github.com/pkg/errors" - "github.com/cloudflare/cloudflared/management" "github.com/cloudflare/cloudflared/tunnelrpc" tunnelpogs "github.com/cloudflare/cloudflared/tunnelrpc/pogs" @@ -118,32 +116,27 @@ func (c *controlStream) ServeControlStream( } } - return c.waitForUnregister(ctx, registrationClient) + c.waitForUnregister(ctx, registrationClient) + return nil } -func (c *controlStream) waitForUnregister(ctx context.Context, registrationClient tunnelrpc.RegistrationClient) error { +func (c *controlStream) waitForUnregister(ctx context.Context, registrationClient tunnelrpc.RegistrationClient) { // wait for connection termination or start of graceful shutdown defer registrationClient.Close() - var shutdownError error select { case <-ctx.Done(): - shutdownError = ctx.Err() break case <-c.gracefulShutdownC: c.stoppedGracefully = true } c.observer.sendUnregisteringEvent(c.connIndex) - err := registrationClient.GracefulShutdown(ctx, c.gracePeriod) - if err != nil { - return errors.Wrap(err, "Error shutting down control stream") - } + registrationClient.GracefulShutdown(ctx, c.gracePeriod) c.observer.log.Info(). Int(management.EventTypeKey, int(management.Cloudflared)). Uint8(LogFieldConnIndex, c.connIndex). IPAddr(LogFieldIPAddress, c.edgeAddress). Msg("Unregistered tunnel connection") - return shutdownError } func (c *controlStream) IsStopped() bool { diff --git a/connection/http2_test.go b/connection/http2_test.go index 92665688..a0ec8b45 100644 --- a/connection/http2_test.go +++ b/connection/http2_test.go @@ -192,9 +192,8 @@ func (mc mockNamedTunnelRPCClient) RegisterConnection( }, nil } -func (mc mockNamedTunnelRPCClient) GracefulShutdown(ctx context.Context, gracePeriod time.Duration) error { +func (mc mockNamedTunnelRPCClient) GracefulShutdown(ctx context.Context, gracePeriod time.Duration) { close(mc.unregistered) - return nil } func (mockNamedTunnelRPCClient) Close() {} diff --git a/connection/quic.go b/connection/quic.go index b24a6cce..c5e218f3 100644 --- a/connection/quic.go +++ b/connection/quic.go @@ -42,26 +42,12 @@ const ( HTTPMethodKey = "HttpMethod" // HTTPHostKey is used to get or set http Method in QUIC ALPN if the underlying proxy connection type is HTTP. HTTPHostKey = "HttpHost" - // HTTPRequestBodyHintKey is used in ConnectRequest metadata to indicate if the request has body - HTTPRequestBodyHintKey = "HttpReqBodyHint" QUICMetadataFlowID = "FlowID" // emperically this capacity has been working well demuxChanCapacity = 16 ) -type RequestBodyHint uint64 - -const ( - RequestBodyHintMissing RequestBodyHint = iota - RequestBodyHintEmpty - RequestBodyHintHasData -) - -func (rbh RequestBodyHint) String() string { - return [...]string{"missing", "empty", "data"}[rbh] -} - var ( portForConnIndex = make(map[uint8]int, 0) portMapMutex sync.Mutex @@ -83,7 +69,6 @@ type QUICConnection struct { rpcTimeout time.Duration streamWriteTimeout time.Duration - gracePeriod time.Duration } // NewQUICConnection returns a new instance of QUICConnection. @@ -101,7 +86,6 @@ func NewQUICConnection( packetRouterConfig *ingress.GlobalRouterConfig, rpcTimeout time.Duration, streamWriteTimeout time.Duration, - gracePeriod time.Duration, ) (*QUICConnection, error) { udpConn, err := createUDPConnForConnIndex(connIndex, localAddr, logger) if err != nil { @@ -138,7 +122,6 @@ func NewQUICConnection( connIndex: connIndex, rpcTimeout: rpcTimeout, streamWriteTimeout: streamWriteTimeout, - gracePeriod: gracePeriod, }, nil } @@ -161,17 +144,8 @@ func (q *QUICConnection) Serve(ctx context.Context) error { // In the future, if cloudflared can autonomously push traffic to the edge, we have to make sure the control // stream is already fully registered before the other goroutines can proceed. errGroup.Go(func() error { - // err is equal to nil if we exit due to unregistration. If that happens we want to wait the full - // amount of the grace period, allowing requests to finish before we cancel the context, which will - // make cloudflared exit. - if err := q.serveControlStream(ctx, controlStream); err == nil { - select { - case <-ctx.Done(): - case <-time.Tick(q.gracePeriod): - } - } - cancel() - return err + defer cancel() + return q.serveControlStream(ctx, controlStream) }) errGroup.Go(func() error { defer cancel() @@ -500,6 +474,7 @@ func buildHTTPRequest( dest := connectRequest.Dest method := metadata[HTTPMethodKey] host := metadata[HTTPHostKey] + isWebsocket := connectRequest.Type == pogs.ConnectionTypeWebsocket req, err := http.NewRequestWithContext(ctx, method, dest, body) if err != nil { @@ -524,8 +499,13 @@ func buildHTTPRequest( return nil, fmt.Errorf("Error setting content-length: %w", err) } - if shouldSetRequestBodyToEmpty(connectRequest, metadata, req) { - log.Debug().Str("host", req.Host).Str("method", req.Method).Msg("Set request to have no body") + // Go's client defaults to chunked encoding after a 200ms delay if the following cases are true: + // * the request body blocks + // * the content length is not set (or set to -1) + // * the method doesn't usually have a body (GET, HEAD, DELETE, ...) + // * there is no transfer-encoding=chunked already set. + // So, if transfer cannot be chunked and content length is 0, we dont set a request body. + if !isWebsocket && !isTransferEncodingChunked(req) && req.ContentLength == 0 { req.Body = http.NoBody } stripWebsocketUpgradeHeader(req) @@ -550,35 +530,6 @@ func isTransferEncodingChunked(req *http.Request) bool { return strings.Contains(strings.ToLower(transferEncodingVal), "chunked") } -// Borrowed from https://github.com/golang/go/blob/go1.22.6/src/net/http/request.go#L1541 -func requestMethodUsuallyLacksBody(req *http.Request) bool { - switch strings.ToUpper(req.Method) { - case "GET", "HEAD", "DELETE", "OPTIONS", "PROPFIND", "SEARCH": - return true - } - return false -} - -func shouldSetRequestBodyToEmpty(connectRequest *pogs.ConnectRequest, metadata map[string]string, req *http.Request) bool { - switch metadata[HTTPRequestBodyHintKey] { - case RequestBodyHintEmpty.String(): - return true - case RequestBodyHintHasData.String(): - return false - default: - } - - isWebsocket := connectRequest.Type == pogs.ConnectionTypeWebsocket - // Go's client defaults to chunked encoding after a 200ms delay if the following cases are true: - // * the request body blocks - // * the content length is not set (or set to -1) - // * the method doesn't usually have a body (GET, HEAD, DELETE, ...) - // * there is no transfer-encoding=chunked already set. - // So, if transfer cannot be chunked and content length is 0, we dont set a request body. - // Reference: https://github.com/golang/go/blob/go1.22.2/src/net/http/transfer.go#L192-L206 - return !isWebsocket && requestMethodUsuallyLacksBody(req) && !isTransferEncodingChunked(req) && req.ContentLength == 0 -} - // A helper struct that guarantees a call to close only affects read side, but not write side. type nopCloserReadWriter struct { io.ReadWriteCloser diff --git a/connection/quic_test.go b/connection/quic_test.go index 0a54e345..c81d53fb 100644 --- a/connection/quic_test.go +++ b/connection/quic_test.go @@ -484,125 +484,6 @@ func TestBuildHTTPRequest(t *testing.T) { }, body: io.NopCloser(&bytes.Buffer{}), }, - { - name: "if edge sends the body is empty hint, set body to empty", - connectRequest: &pogs.ConnectRequest{ - Dest: "http://test.com", - Metadata: []pogs.Metadata{ - { - Key: "HttpHeader:Another-Header", - Val: "Misc", - }, - { - Key: "HttpHost", - Val: "cf.host", - }, - { - Key: "HttpMethod", - Val: "put", - }, - { - Key: HTTPRequestBodyHintKey, - Val: RequestBodyHintEmpty.String(), - }, - }, - }, - req: &http.Request{ - Method: "put", - URL: &url.URL{ - Scheme: "http", - Host: "test.com", - }, - Proto: "HTTP/1.1", - ProtoMajor: 1, - ProtoMinor: 1, - Header: http.Header{ - "Another-Header": []string{"Misc"}, - }, - ContentLength: 0, - Host: "cf.host", - Body: http.NoBody, - }, - body: io.NopCloser(&bytes.Buffer{}), - }, - { - name: "if edge sends the body has data hint, don't set body to empty", - connectRequest: &pogs.ConnectRequest{ - Dest: "http://test.com", - Metadata: []pogs.Metadata{ - { - Key: "HttpHeader:Another-Header", - Val: "Misc", - }, - { - Key: "HttpHost", - Val: "cf.host", - }, - { - Key: "HttpMethod", - Val: "put", - }, - { - Key: HTTPRequestBodyHintKey, - Val: RequestBodyHintHasData.String(), - }, - }, - }, - req: &http.Request{ - Method: "put", - URL: &url.URL{ - Scheme: "http", - Host: "test.com", - }, - Proto: "HTTP/1.1", - ProtoMajor: 1, - ProtoMinor: 1, - Header: http.Header{ - "Another-Header": []string{"Misc"}, - }, - ContentLength: 0, - Host: "cf.host", - Body: io.NopCloser(&bytes.Buffer{}), - }, - body: io.NopCloser(&bytes.Buffer{}), - }, - { - name: "if the http method usually has body, don't set body to empty", - connectRequest: &pogs.ConnectRequest{ - Dest: "http://test.com", - Metadata: []pogs.Metadata{ - { - Key: "HttpHeader:Another-Header", - Val: "Misc", - }, - { - Key: "HttpHost", - Val: "cf.host", - }, - { - Key: "HttpMethod", - Val: "post", - }, - }, - }, - req: &http.Request{ - Method: "post", - URL: &url.URL{ - Scheme: "http", - Host: "test.com", - }, - Proto: "HTTP/1.1", - ProtoMajor: 1, - ProtoMinor: 1, - Header: http.Header{ - "Another-Header": []string{"Misc"}, - }, - ContentLength: 0, - Host: "cf.host", - Body: io.NopCloser(&bytes.Buffer{}), - }, - body: io.NopCloser(&bytes.Buffer{}), - }, } log := zerolog.Nop() @@ -855,7 +736,6 @@ func testQUICConnection(udpListenerAddr net.Addr, t *testing.T, index uint8) *QU nil, 15*time.Second, 0*time.Second, - 0*time.Second, ) require.NoError(t, err) return qc diff --git a/supervisor/tunnel.go b/supervisor/tunnel.go index 03d7f930..85637798 100644 --- a/supervisor/tunnel.go +++ b/supervisor/tunnel.go @@ -604,7 +604,6 @@ func (e *EdgeTunnelServer) serveQUIC( e.config.PacketConfig, e.config.RPCTimeout, e.config.WriteStreamTimeout, - e.config.GracePeriod, ) if err != nil { connLogger.ConnAwareLogger().Err(err).Msgf("Failed to create new quic connection") diff --git a/tunnelrpc/registration_client.go b/tunnelrpc/registration_client.go index 7d945f58..f41819f3 100644 --- a/tunnelrpc/registration_client.go +++ b/tunnelrpc/registration_client.go @@ -23,7 +23,7 @@ type RegistrationClient interface { edgeAddress net.IP, ) (*pogs.ConnectionDetails, error) SendLocalConfiguration(ctx context.Context, config []byte) error - GracefulShutdown(ctx context.Context, gracePeriod time.Duration) error + GracefulShutdown(ctx context.Context, gracePeriod time.Duration) Close() } @@ -79,7 +79,7 @@ func (r *registrationClient) SendLocalConfiguration(ctx context.Context, config return err } -func (r *registrationClient) GracefulShutdown(ctx context.Context, gracePeriod time.Duration) error { +func (r *registrationClient) GracefulShutdown(ctx context.Context, gracePeriod time.Duration) { ctx, cancel := context.WithTimeout(ctx, gracePeriod) defer cancel() defer metrics.CapnpMetrics.ClientOperations.WithLabelValues(metrics.Registration, metrics.OperationUnregisterConnection).Inc() @@ -88,9 +88,7 @@ func (r *registrationClient) GracefulShutdown(ctx context.Context, gracePeriod t err := r.client.UnregisterConnection(ctx) if err != nil { metrics.CapnpMetrics.ClientFailures.WithLabelValues(metrics.Registration, metrics.OperationUnregisterConnection).Inc() - return err } - return nil } func (r *registrationClient) Close() { From a57fc25b5410d3ed43297dce7b34b92321e1c64b Mon Sep 17 00:00:00 2001 From: GoncaloGarcia Date: Tue, 10 Sep 2024 16:55:27 +0100 Subject: [PATCH 09/14] Release 2024.9.1 --- RELEASE_NOTES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/RELEASE_NOTES b/RELEASE_NOTES index 8fd1175a..c76edb52 100644 --- a/RELEASE_NOTES +++ b/RELEASE_NOTES @@ -1,3 +1,6 @@ +2024.9.1 +- 2024-09-10 Revert Release 2024.9.0 + 2024.9.0 - 2024-09-10 TUN-8621: Fix cloudflared version in change notes. - 2024-09-06 PPIP-2310: Update quick tunnel disclaimer From 2484df1f81e58165b26f82519cfcd8561d1acd37 Mon Sep 17 00:00:00 2001 From: Devin Carr Date: Wed, 11 Sep 2024 16:00:00 -0700 Subject: [PATCH 10/14] TUN-8630: Check checksum of downloaded binary to compare to current for auto-updating In the rare case that the updater downloads the same binary (validated via checksum) we want to make sure that the updater does not attempt to upgrade and restart the cloudflared process. The binaries are equivalent and this would provide no value. However, we are covering this case because there was an errant deployment of cloudflared that reported itself as an older version and was then stuck in an infinite loop attempting to upgrade to the latest version which didn't exist. By making sure that the binary is different ensures that the upgrade will be attempted and cloudflared will be restarted to run the new version. This change only affects cloudflared tunnels running with default settings or `--no-autoupdate=false` which allows cloudflared to auto-update itself in-place. Most distributions that handle package management at the operating system level are not affected by this change. --- cmd/cloudflared/cliutil/build_info.go | 32 ++++++++++++++- cmd/cloudflared/main.go | 2 +- cmd/cloudflared/updater/update.go | 12 +++--- cmd/cloudflared/updater/update_test.go | 6 +++ cmd/cloudflared/updater/workers_service.go | 5 +++ cmd/cloudflared/updater/workers_update.go | 47 +++++++++++----------- 6 files changed, 73 insertions(+), 31 deletions(-) diff --git a/cmd/cloudflared/cliutil/build_info.go b/cmd/cloudflared/cliutil/build_info.go index fff4febf..78ef775a 100644 --- a/cmd/cloudflared/cliutil/build_info.go +++ b/cmd/cloudflared/cliutil/build_info.go @@ -1,7 +1,10 @@ package cliutil import ( + "crypto/sha256" "fmt" + "io" + "os" "runtime" "github.com/rs/zerolog" @@ -13,6 +16,7 @@ type BuildInfo struct { GoArch string `json:"go_arch"` BuildType string `json:"build_type"` CloudflaredVersion string `json:"cloudflared_version"` + Checksum string `json:"checksum"` } func GetBuildInfo(buildType, version string) *BuildInfo { @@ -22,11 +26,12 @@ func GetBuildInfo(buildType, version string) *BuildInfo { GoArch: runtime.GOARCH, BuildType: buildType, CloudflaredVersion: version, + Checksum: currentBinaryChecksum(), } } func (bi *BuildInfo) Log(log *zerolog.Logger) { - log.Info().Msgf("Version %s", bi.CloudflaredVersion) + log.Info().Msgf("Version %s (Checksum %s)", bi.CloudflaredVersion, bi.Checksum) if bi.BuildType != "" { log.Info().Msgf("Built%s", bi.GetBuildTypeMsg()) } @@ -51,3 +56,28 @@ func (bi *BuildInfo) GetBuildTypeMsg() string { func (bi *BuildInfo) UserAgent() string { return fmt.Sprintf("cloudflared/%s", bi.CloudflaredVersion) } + +// FileChecksum opens a file and returns the SHA256 checksum. +func FileChecksum(filePath string) (string, error) { + f, err := os.Open(filePath) + if err != nil { + return "", err + } + defer f.Close() + + h := sha256.New() + if _, err := io.Copy(h, f); err != nil { + return "", err + } + + return fmt.Sprintf("%x", h.Sum(nil)), nil +} + +func currentBinaryChecksum() string { + currentPath, err := os.Executable() + if err != nil { + return "" + } + sum, _ := FileChecksum(currentPath) + return sum +} diff --git a/cmd/cloudflared/main.go b/cmd/cloudflared/main.go index 61d2c41d..b0b93cf8 100644 --- a/cmd/cloudflared/main.go +++ b/cmd/cloudflared/main.go @@ -91,7 +91,7 @@ func main() { tunnel.Init(bInfo, graceShutdownC) // we need this to support the tunnel sub command... access.Init(graceShutdownC, Version) - updater.Init(Version) + updater.Init(bInfo) tracing.Init(Version) token.Init(Version) tail.Init(bInfo) diff --git a/cmd/cloudflared/updater/update.go b/cmd/cloudflared/updater/update.go index 07b382f5..439b129a 100644 --- a/cmd/cloudflared/updater/update.go +++ b/cmd/cloudflared/updater/update.go @@ -14,6 +14,7 @@ import ( "github.com/urfave/cli/v2" "golang.org/x/term" + "github.com/cloudflare/cloudflared/cmd/cloudflared/cliutil" "github.com/cloudflare/cloudflared/config" "github.com/cloudflare/cloudflared/logger" ) @@ -31,7 +32,7 @@ const ( ) var ( - version string + buildInfo *cliutil.BuildInfo BuiltForPackageManager = "" ) @@ -81,8 +82,8 @@ func (uo *UpdateOutcome) noUpdate() bool { return uo.Error == nil && uo.Updated == false } -func Init(v string) { - version = v +func Init(info *cliutil.BuildInfo) { + buildInfo = info } func CheckForUpdate(options updateOptions) (CheckResult, error) { @@ -100,11 +101,12 @@ func CheckForUpdate(options updateOptions) (CheckResult, error) { cfdPath = encodeWindowsPath(cfdPath) } - s := NewWorkersService(version, url, cfdPath, Options{IsBeta: options.isBeta, + s := NewWorkersService(buildInfo.CloudflaredVersion, url, cfdPath, Options{IsBeta: options.isBeta, IsForced: options.isForced, RequestedVersion: options.intendedVersion}) return s.Check() } + func encodeWindowsPath(path string) string { // We do this because Windows allows spaces in directories such as // Program Files but does not allow these directories to be spaced in batch files. @@ -237,7 +239,7 @@ func (a *AutoUpdater) Run(ctx context.Context) error { for { updateOutcome := loggedUpdate(a.log, updateOptions{updateDisabled: !a.configurable.enabled}) if updateOutcome.Updated { - Init(updateOutcome.Version) + buildInfo.CloudflaredVersion = updateOutcome.Version if IsSysV() { // SysV doesn't have a mechanism to keep service alive, we have to restart the process a.log.Info().Msg("Restarting service managed by SysV...") diff --git a/cmd/cloudflared/updater/update_test.go b/cmd/cloudflared/updater/update_test.go index f977b96e..3159f7ab 100644 --- a/cmd/cloudflared/updater/update_test.go +++ b/cmd/cloudflared/updater/update_test.go @@ -9,8 +9,14 @@ import ( "github.com/rs/zerolog" "github.com/stretchr/testify/assert" "github.com/urfave/cli/v2" + + "github.com/cloudflare/cloudflared/cmd/cloudflared/cliutil" ) +func init() { + Init(cliutil.GetBuildInfo("TEST", "TEST")) +} + func TestDisabledAutoUpdater(t *testing.T) { listeners := &gracenet.Net{} log := zerolog.Nop() diff --git a/cmd/cloudflared/updater/workers_service.go b/cmd/cloudflared/updater/workers_service.go index 4b52571c..b5883f1f 100644 --- a/cmd/cloudflared/updater/workers_service.go +++ b/cmd/cloudflared/updater/workers_service.go @@ -3,6 +3,7 @@ package updater import ( "encoding/json" "errors" + "fmt" "net/http" "runtime" ) @@ -79,6 +80,10 @@ func (s *WorkersService) Check() (CheckResult, error) { } defer resp.Body.Close() + if resp.StatusCode != 200 { + return nil, fmt.Errorf("unable to check for update: %d", resp.StatusCode) + } + var v VersionResponse if err := json.NewDecoder(resp.Body).Decode(&v); err != nil { return nil, err diff --git a/cmd/cloudflared/updater/workers_update.go b/cmd/cloudflared/updater/workers_update.go index b2d451a9..800fa4fe 100644 --- a/cmd/cloudflared/updater/workers_update.go +++ b/cmd/cloudflared/updater/workers_update.go @@ -3,7 +3,6 @@ package updater import ( "archive/tar" "compress/gzip" - "crypto/sha256" "errors" "fmt" "io" @@ -16,6 +15,10 @@ import ( "strings" "text/template" "time" + + "github.com/getsentry/sentry-go" + + "github.com/cloudflare/cloudflared/cmd/cloudflared/cliutil" ) const ( @@ -86,8 +89,25 @@ func (v *WorkersVersion) Apply() error { return err } - // check that the file is what is expected - if err := isValidChecksum(v.checksum, newFilePath); err != nil { + downloadSum, err := cliutil.FileChecksum(newFilePath) + if err != nil { + return err + } + + // Check that the file downloaded matches what is expected. + if v.checksum != downloadSum { + return errors.New("checksum validation failed") + } + + // Check if the currently running version has the same checksum + if downloadSum == buildInfo.Checksum { + // Currently running binary matches the downloaded binary so we have no reason to update. This is + // typically unexpected, as such we emit a sentry event. + localHub := sentry.CurrentHub().Clone() + err := errors.New("checksum validation matches currently running process") + localHub.CaptureException(err) + // Make sure to cleanup the new downloaded file since we aren't upgrading versions. + os.Remove(newFilePath) return err } @@ -189,27 +209,6 @@ func isCompressedFile(urlstring string) bool { return strings.HasSuffix(u.Path, ".tgz") } -// checks if the checksum in the json response matches the checksum of the file download -func isValidChecksum(checksum, filePath string) error { - f, err := os.Open(filePath) - if err != nil { - return err - } - defer f.Close() - - h := sha256.New() - if _, err := io.Copy(h, f); err != nil { - return err - } - - hash := fmt.Sprintf("%x", h.Sum(nil)) - - if checksum != hash { - return errors.New("checksum validation failed") - } - return nil -} - // writeBatchFile writes a batch file out to disk // see the dicussion on why it has to be done this way func writeBatchFile(targetPath string, newPath string, oldPath string) error { From cd8cb47866a68de1180511ee5326ed6f028c1c2b Mon Sep 17 00:00:00 2001 From: Devin Carr Date: Thu, 12 Sep 2024 12:17:39 -0700 Subject: [PATCH 11/14] TUN-8632: Delay checking auto-update by the provided frequency Delaying the auto-update check timer to start after one full round of the provided frequency reduces the chance of upgrading immediately after starting. --- cmd/cloudflared/updater/update.go | 38 ++++++++++++------------------- 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/cmd/cloudflared/updater/update.go b/cmd/cloudflared/updater/update.go index 439b129a..1d3cbc2e 100644 --- a/cmd/cloudflared/updater/update.go +++ b/cmd/cloudflared/updater/update.go @@ -198,10 +198,9 @@ func loggedUpdate(log *zerolog.Logger, options updateOptions) UpdateOutcome { // AutoUpdater periodically checks for new version of cloudflared. type AutoUpdater struct { - configurable *configurable - listeners *gracenet.Net - updateConfigChan chan *configurable - log *zerolog.Logger + configurable *configurable + listeners *gracenet.Net + log *zerolog.Logger } // AutoUpdaterConfigurable is the attributes of AutoUpdater that can be reconfigured during runtime @@ -212,10 +211,9 @@ type configurable struct { func NewAutoUpdater(updateDisabled bool, freq time.Duration, listeners *gracenet.Net, log *zerolog.Logger) *AutoUpdater { return &AutoUpdater{ - configurable: createUpdateConfig(updateDisabled, freq, log), - listeners: listeners, - updateConfigChan: make(chan *configurable), - log: log, + configurable: createUpdateConfig(updateDisabled, freq, log), + listeners: listeners, + log: log, } } @@ -234,9 +232,17 @@ func createUpdateConfig(updateDisabled bool, freq time.Duration, log *zerolog.Lo } } +// Run will perodically check for cloudflared updates, download them, and then restart the current cloudflared process +// to use the new version. It delays the first update check by the configured frequency as to not attempt a +// download immediately and restart after starting (in the case that there is an upgrade available). func (a *AutoUpdater) Run(ctx context.Context) error { ticker := time.NewTicker(a.configurable.freq) for { + select { + case <-ctx.Done(): + return ctx.Err() + case <-ticker.C: + } updateOutcome := loggedUpdate(a.log, updateOptions{updateDisabled: !a.configurable.enabled}) if updateOutcome.Updated { buildInfo.CloudflaredVersion = updateOutcome.Version @@ -256,25 +262,9 @@ func (a *AutoUpdater) Run(ctx context.Context) error { } else if updateOutcome.UserMessage != "" { a.log.Warn().Msg(updateOutcome.UserMessage) } - - select { - case <-ctx.Done(): - return ctx.Err() - case newConfigurable := <-a.updateConfigChan: - ticker.Stop() - a.configurable = newConfigurable - ticker = time.NewTicker(a.configurable.freq) - // Check if there is new version of cloudflared after receiving new AutoUpdaterConfigurable - case <-ticker.C: - } } } -// Update is the method to pass new AutoUpdaterConfigurable to a running AutoUpdater. It is safe to be called concurrently -func (a *AutoUpdater) Update(updateDisabled bool, newFreq time.Duration) { - a.updateConfigChan <- createUpdateConfig(updateDisabled, newFreq, a.log) -} - func isAutoupdateEnabled(log *zerolog.Logger, updateDisabled bool, updateFreq time.Duration) bool { if !supportAutoUpdate(log) { return false From 5c5d1dc1615ac22348b04d7a474272e9f7071b91 Mon Sep 17 00:00:00 2001 From: Dean Sundquist Date: Mon, 9 Sep 2024 16:47:07 +0000 Subject: [PATCH 12/14] TUN-8629: Cloudflared update on Windows requires running it twice to update --- cmd/cloudflared/updater/workers_update.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/cloudflared/updater/workers_update.go b/cmd/cloudflared/updater/workers_update.go index 800fa4fe..b7a86ff1 100644 --- a/cmd/cloudflared/updater/workers_update.go +++ b/cmd/cloudflared/updater/workers_update.go @@ -30,9 +30,9 @@ const ( // start the service // exit with code 0 if we've reached this point indicating success. windowsUpdateCommandTemplate = `sc stop cloudflared >nul 2>&1 +del "{{.OldPath}}" rename "{{.TargetPath}}" {{.OldName}} rename "{{.NewPath}}" {{.BinaryName}} -del "{{.OldPath}}" sc start cloudflared >nul 2>&1 exit /b 0` batchFileName = "cfd_update.bat" From ea1c4a327d1e2248a86770fe215dc06a4991699b Mon Sep 17 00:00:00 2001 From: Hrushikesh Deshpande Date: Thu, 19 Sep 2024 21:52:45 -0400 Subject: [PATCH 13/14] Adding semgrep yaml file --- .github/workflows/semgrep.yml | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 .github/workflows/semgrep.yml diff --git a/.github/workflows/semgrep.yml b/.github/workflows/semgrep.yml new file mode 100644 index 00000000..c821e5a5 --- /dev/null +++ b/.github/workflows/semgrep.yml @@ -0,0 +1,25 @@ + +on: + pull_request: {} + workflow_dispatch: {} + push: + branches: + - main + - master + schedule: + - cron: '0 0 * * *' +name: Semgrep config +jobs: + semgrep: + name: semgrep/ci + runs-on: ubuntu-20.04 + env: + SEMGREP_APP_TOKEN: ${{ secrets.SEMGREP_APP_TOKEN }} + SEMGREP_URL: https://cloudflare.semgrep.dev + SEMGREP_APP_URL: https://cloudflare.semgrep.dev + SEMGREP_VERSION_CHECK_URL: https://cloudflare.semgrep.dev/api/check-version + container: + image: returntocorp/semgrep + steps: + - uses: actions/checkout@v3 + - run: semgrep ci From d7d81384c2a39db7bec488c9ff5cfc40f2aedc20 Mon Sep 17 00:00:00 2001 From: Devin Carr Date: Tue, 1 Oct 2024 15:21:01 -0700 Subject: [PATCH 14/14] TUN-8646: Add datagram v3 support feature flag --- features/features.go | 1 + 1 file changed, 1 insertion(+) diff --git a/features/features.go b/features/features.go index 76f8ff8f..574f55ae 100644 --- a/features/features.go +++ b/features/features.go @@ -8,6 +8,7 @@ const ( FeaturePostQuantum = "postquantum" FeatureQUICSupportEOF = "support_quic_eof" FeatureManagementLogs = "management_logs" + FeatureDatagramV3 = "support_datagram_v3" ) var (