From 3294b91bed2faefa772c6e107a9d049aa3d33ca3 Mon Sep 17 00:00:00 2001 From: Ziqi Zhao Date: Tue, 5 Jul 2022 06:48:25 +0800 Subject: [PATCH 1/7] fix nil panic in confighttp Signed-off-by: Ziqi Zhao --- config/confighttp/compression.go | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/config/confighttp/compression.go b/config/confighttp/compression.go index 13537d599f1..e85b2cfa90c 100644 --- a/config/confighttp/compression.go +++ b/config/confighttp/compression.go @@ -82,19 +82,21 @@ func (r *compressRoundTripper) RoundTrip(req *http.Request) (*http.Response, err if writerErr != nil { return nil, writerErr } - _, copyErr := io.Copy(compressWriter, req.Body) - closeErr := req.Body.Close() + if req.Body != nil { + _, copyErr := io.Copy(compressWriter, req.Body) + closeErr := req.Body.Close() - if err := compressWriter.Close(); err != nil { - return nil, err - } + if copyErr != nil { + return nil, copyErr + } - if copyErr != nil { - return nil, copyErr + if closeErr != nil { + return nil, closeErr + } } - if closeErr != nil { - return nil, closeErr + if err := compressWriter.Close(); err != nil { + return nil, err } // Create a new request since the docs say that we cannot modify the "req" From 4cb9eb2bd89474f747759cb5852df7764141988a Mon Sep 17 00:00:00 2001 From: Ziqi Zhao Date: Wed, 6 Jul 2022 13:26:39 +0800 Subject: [PATCH 2/7] add unit test and changelog Signed-off-by: Ziqi Zhao --- CHANGELOG.md | 1 + config/confighttp/compression_test.go | 34 +++++++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 00caf6ec5df..feeb2541edb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,6 +98,7 @@ - Unconfigured receivers are not identified, this was not a real problem in final binaries since the validation of the config catch this. - Allow configurations to contain "unused" receivers. Receivers that are configured but not used in any pipeline, this was possible already for exporters and processors. - Remove the enforcement/check that Receiver factories create the same instance for the same config. +- Fix confighttp.compression panic due to nil request.Body. (#5628) ## v0.53.0 Beta diff --git a/config/confighttp/compression_test.go b/config/confighttp/compression_test.go index 9a477559a2e..8418187670a 100644 --- a/config/confighttp/compression_test.go +++ b/config/confighttp/compression_test.go @@ -233,6 +233,40 @@ func TestHTTPContentDecompressionHandler(t *testing.T) { } } +func TestHTTPContentCompressionNilRequestBody(t *testing.T) { + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + }) + + addr := testutil.GetAvailableLocalAddress(t) + ln, err := net.Listen("tcp", addr) + require.NoError(t, err, "failed to create listener: %v", err) + srv := &http.Server{ + Handler: handler, + } + go func() { + _ = srv.Serve(ln) + }() + // Wait for the servers to start + <-time.After(10 * time.Millisecond) + + serverURL := fmt.Sprintf("http://%s", ln.Addr().String()) + + req, err := http.NewRequest("GET", serverURL, nil) + require.NoError(t, err, "failed to create request to test handler") + + client := http.Client{} + client.Transport = newCompressRoundTripper(http.DefaultTransport, configcompression.Gzip) + + res, err := client.Do(req) + require.NoError(t, err) + + _, err = ioutil.ReadAll(res.Body) + require.NoError(t, err) + require.NoError(t, res.Body.Close(), "failed to close request body: %v", err) + require.NoError(t, srv.Close()) +} + func compressGzip(body []byte) (*bytes.Buffer, error) { var buf bytes.Buffer From 53a1265601b8d2f07b75f74dab1a90e66d2a69e9 Mon Sep 17 00:00:00 2001 From: Ziqi Zhao Date: Thu, 14 Jul 2022 13:04:11 +0800 Subject: [PATCH 3/7] fix the review comments to use httptest package Signed-off-by: Ziqi Zhao --- config/confighttp/compression_test.go | 30 +++++++++------------------ 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/config/confighttp/compression_test.go b/config/confighttp/compression_test.go index 8418187670a..38537f2d165 100644 --- a/config/confighttp/compression_test.go +++ b/config/confighttp/compression_test.go @@ -22,6 +22,7 @@ import ( "io/ioutil" "net" "net/http" + "net/http/httptest" "testing" "time" @@ -233,38 +234,27 @@ func TestHTTPContentDecompressionHandler(t *testing.T) { } } -func TestHTTPContentCompressionNilRequestBody(t *testing.T) { - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { +func TestHTTPContentCompressionRequestWithNilBody(t *testing.T) { + compressedGzipBody, _ := compressGzip([]byte{}) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) - }) + body, err := ioutil.ReadAll(r.Body) + require.NoError(t, err, "failed to read request body: %v", err) + assert.EqualValues(t, compressedGzipBody.Bytes(), body) + })) + defer server.Close() - addr := testutil.GetAvailableLocalAddress(t) - ln, err := net.Listen("tcp", addr) - require.NoError(t, err, "failed to create listener: %v", err) - srv := &http.Server{ - Handler: handler, - } - go func() { - _ = srv.Serve(ln) - }() - // Wait for the servers to start - <-time.After(10 * time.Millisecond) - - serverURL := fmt.Sprintf("http://%s", ln.Addr().String()) - - req, err := http.NewRequest("GET", serverURL, nil) + req, err := http.NewRequest("GET", server.URL, nil) require.NoError(t, err, "failed to create request to test handler") client := http.Client{} client.Transport = newCompressRoundTripper(http.DefaultTransport, configcompression.Gzip) - res, err := client.Do(req) require.NoError(t, err) _, err = ioutil.ReadAll(res.Body) require.NoError(t, err) require.NoError(t, res.Body.Close(), "failed to close request body: %v", err) - require.NoError(t, srv.Close()) } func compressGzip(body []byte) (*bytes.Buffer, error) { From c5539ad1c6dfe73eedfee6b188fcbfd7d44805c8 Mon Sep 17 00:00:00 2001 From: Ziqi Zhao Date: Thu, 14 Jul 2022 13:06:31 +0800 Subject: [PATCH 4/7] move changelog to right place Signed-off-by: Ziqi Zhao --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index feeb2541edb..db981f7a6e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ - Fix Collector panic when disabling telemetry metrics (#5642) - Fix Collector panic when featuregate value is empty (#5663) +- Fix confighttp.compression panic due to nil request.Body. (#5628) ## v0.55.0 Beta @@ -98,7 +99,6 @@ - Unconfigured receivers are not identified, this was not a real problem in final binaries since the validation of the config catch this. - Allow configurations to contain "unused" receivers. Receivers that are configured but not used in any pipeline, this was possible already for exporters and processors. - Remove the enforcement/check that Receiver factories create the same instance for the same config. -- Fix confighttp.compression panic due to nil request.Body. (#5628) ## v0.53.0 Beta From e5ec09d863b08c31e202f51917ec66a96a2ac9e2 Mon Sep 17 00:00:00 2001 From: Ziqi Zhao Date: Thu, 14 Jul 2022 19:10:59 +0800 Subject: [PATCH 5/7] add tests to cover different errors Signed-off-by: Ziqi Zhao --- config/confighttp/compression_test.go | 57 +++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/config/confighttp/compression_test.go b/config/confighttp/compression_test.go index 38537f2d165..dbca537888c 100644 --- a/config/confighttp/compression_test.go +++ b/config/confighttp/compression_test.go @@ -19,10 +19,12 @@ import ( "compress/gzip" "compress/zlib" "fmt" + "io" "io/ioutil" "net" "net/http" "net/http/httptest" + "net/url" "testing" "time" @@ -257,6 +259,61 @@ func TestHTTPContentCompressionRequestWithNilBody(t *testing.T) { require.NoError(t, res.Body.Close(), "failed to close request body: %v", err) } +func TestHTTPContentCompressionCopyError(t *testing.T) { + copyErrorCompressRoundTripper := &compressRoundTripper{ + RoundTripper: http.DefaultTransport, + compressionType: "copyFailed", + writer: func(buf *bytes.Buffer) (io.WriteCloser, error) { + return nil, fmt.Errorf("copy failed") + }, + } + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + })) + defer server.Close() + + req, err := http.NewRequest("GET", server.URL, nil) + require.NoError(t, err, "failed to create request to test handler") + + client := http.Client{} + client.Transport = copyErrorCompressRoundTripper + _, err = client.Do(req) + require.Error(t, err) +} + +type closeFailBody struct { + *bytes.Buffer +} + +func (*closeFailBody) Close() error { + return fmt.Errorf("close failed") +} + +func TestHTTPContentCompressionRequestBodyCloseError(t *testing.T) { + testBody := []byte("blank") + body := &closeFailBody{ + Buffer: bytes.NewBuffer(testBody), + } + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + })) + defer server.Close() + + url, _ := url.Parse(server.URL) + req := &http.Request{ + Method: "GET", + URL: url, + Body: body, + } + + client := http.Client{} + client.Transport = newCompressRoundTripper(http.DefaultTransport, configcompression.Gzip) + _, err := client.Do(req) + require.Error(t, err) +} + func compressGzip(body []byte) (*bytes.Buffer, error) { var buf bytes.Buffer From c7567ac3f92440d9f26c8a3fdfa35b0705977a44 Mon Sep 17 00:00:00 2001 From: Ziqi Zhao Date: Tue, 19 Jul 2022 07:06:42 +0800 Subject: [PATCH 6/7] fix unit test Signed-off-by: Ziqi Zhao --- config/confighttp/compression_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/config/confighttp/compression_test.go b/config/confighttp/compression_test.go index dbca537888c..c5fc726f9d4 100644 --- a/config/confighttp/compression_test.go +++ b/config/confighttp/compression_test.go @@ -260,6 +260,7 @@ func TestHTTPContentCompressionRequestWithNilBody(t *testing.T) { } func TestHTTPContentCompressionCopyError(t *testing.T) { + testBody := bytes.NewBuffer([]byte("test")) copyErrorCompressRoundTripper := &compressRoundTripper{ RoundTripper: http.DefaultTransport, compressionType: "copyFailed", @@ -273,7 +274,7 @@ func TestHTTPContentCompressionCopyError(t *testing.T) { })) defer server.Close() - req, err := http.NewRequest("GET", server.URL, nil) + req, err := http.NewRequest("GET", server.URL, testBody) require.NoError(t, err, "failed to create request to test handler") client := http.Client{} From b289f0ea76706a8c783bc26c32fb7b9b8a53719a Mon Sep 17 00:00:00 2001 From: Ziqi Zhao Date: Tue, 19 Jul 2022 09:31:56 +0800 Subject: [PATCH 7/7] fix unit test Signed-off-by: Ziqi Zhao --- config/confighttp/compression_test.go | 34 ++++++++++++++++----------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/config/confighttp/compression_test.go b/config/confighttp/compression_test.go index c5fc726f9d4..37aeaf5aff1 100644 --- a/config/confighttp/compression_test.go +++ b/config/confighttp/compression_test.go @@ -19,7 +19,6 @@ import ( "compress/gzip" "compress/zlib" "fmt" - "io" "io/ioutil" "net" "net/http" @@ -259,27 +258,34 @@ func TestHTTPContentCompressionRequestWithNilBody(t *testing.T) { require.NoError(t, res.Body.Close(), "failed to close request body: %v", err) } -func TestHTTPContentCompressionCopyError(t *testing.T) { - testBody := bytes.NewBuffer([]byte("test")) - copyErrorCompressRoundTripper := &compressRoundTripper{ - RoundTripper: http.DefaultTransport, - compressionType: "copyFailed", - writer: func(buf *bytes.Buffer) (io.WriteCloser, error) { - return nil, fmt.Errorf("copy failed") - }, - } +type copyFailBody struct { +} +func (*copyFailBody) Read(p []byte) (n int, err error) { + return 0, fmt.Errorf("read failed") +} + +func (*copyFailBody) Close() error { + return nil +} + +func TestHTTPContentCompressionCopyError(t *testing.T) { + body := ©FailBody{} server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) })) defer server.Close() - req, err := http.NewRequest("GET", server.URL, testBody) - require.NoError(t, err, "failed to create request to test handler") + url, _ := url.Parse(server.URL) + req := &http.Request{ + Method: "GET", + URL: url, + Body: body, + } client := http.Client{} - client.Transport = copyErrorCompressRoundTripper - _, err = client.Do(req) + client.Transport = newCompressRoundTripper(http.DefaultTransport, configcompression.Gzip) + _, err := client.Do(req) require.Error(t, err) }