diff --git a/CHANGELOG.md b/CHANGELOG.md index 20d56c0b70e..d5e43a400a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Improve performance by reducing allocations in the gRPC stats handler in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`. (#8035) +### Fixed + +- Fix issue in `go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin` middleware where temporary files for multipart/form-data were not being removed. (#6609) + ### Deprecated - `WithRouteTag` in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` is deprecated. diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/gin.go b/instrumentation/github.com/gin-gonic/gin/otelgin/gin.go index 313d4d2abf0..87f98406dbd 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/gin.go +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/gin.go @@ -81,7 +81,11 @@ func Middleware(service string, opts ...Option) gin.HandlerFunc { c.Set(tracerKey, tracer) savedCtx := c.Request.Context() defer func() { - c.Request = c.Request.WithContext(savedCtx) + if c.Request.MultipartForm != nil { + c.Request = c.Request.Clone(savedCtx) + } else { + c.Request = c.Request.WithContext(savedCtx) + } }() ctx := cfg.Propagators.Extract(savedCtx, propagation.HeaderCarrier(c.Request.Header)) diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/gin_test.go b/instrumentation/github.com/gin-gonic/gin/otelgin/gin_test.go index 8676222e7ad..4ec0e55c2ca 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/gin_test.go +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/gin_test.go @@ -6,11 +6,16 @@ package otelgin_test import ( + "bytes" "errors" "fmt" "html/template" + "io/fs" + "mime/multipart" "net/http" "net/http/httptest" + "os" + "runtime" "strconv" "strings" "testing" @@ -479,6 +484,91 @@ func TestWithGinFilter(t *testing.T) { }) } +func TestTemporaryFormFileRemove(t *testing.T) { + if runtime.GOOS == "windows" { + // Windows sometimes refuses to remove a file that was just closed. + t.Skip("https://go.dev/issue/25965") + } + sr := tracetest.NewSpanRecorder() + provider := sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(sr)) + + router := gin.New() + router.MaxMultipartMemory = 1 + + tempDir := t.TempDir() + + // We register three middlewares here, with the otel one in the middle. + // When the response is returned, the post-handler execution order is: + // MiddlewareC -> otel -> MiddlewareA (reverse of registration order). + // In MiddlewareC, the multipart form file still exists, so opening it succeeds. + // However, by the time MiddlewareA runs, the otel middleware has already + // removed the temporary multipart files, so opening the file results in a + // "file does not exist" error. + + // MiddlewareA + router.Use(func(c *gin.Context) { + c.Next() + _, err := c.FormFile("files") + require.NoError(t, err) + form, _ := c.MultipartForm() + files := form.File["files"] + require.Len(t, files, 1) + _, err = fs.ReadDir(os.DirFS(tempDir), files[0].Filename) + require.Error(t, err) + require.True(t, errors.Is(err, fs.ErrNotExist)) + }) + + router.Use(otelgin.Middleware("foobar", otelgin.WithTracerProvider(provider))) + + // MiddlewareC + router.Use(func(c *gin.Context) { + c.Next() + _, err := c.FormFile("files") + require.NoError(t, err) + form, _ := c.MultipartForm() + files := form.File["files"] + require.Len(t, files, 1) + _, err = fs.ReadDir(os.DirFS(tempDir), files[0].Filename) + require.Error(t, err) + require.True(t, errors.Is(err, fs.ErrNotExist)) + }) + + var fileHeader *multipart.FileHeader + router.POST("/upload", func(c *gin.Context) { + _, err := c.FormFile("files") + require.NoError(t, err) + fileHeader = c.Request.MultipartForm.File["files"][0] + _, err = fileHeader.Open() + require.NoError(t, err) + c.JSON(http.StatusOK, nil) + }) + + var body bytes.Buffer + + mw := multipart.NewWriter(&body) + fw, err := mw.CreateFormFile("files", "file") + require.NoError(t, err) + + _, err = fw.Write([]byte("hello world")) + require.NoError(t, err) + err = mw.Close() + require.NoError(t, err) + r := httptest.NewRequest("POST", "/upload", &body) + r.Header.Add("Content-Type", mw.FormDataContentType()) + w := httptest.NewRecorder() + + router.ServeHTTP(w, r) + assert.Len(t, sr.Ended(), 1) + require.Equal(t, http.StatusOK, w.Code) + _, err = fileHeader.Open() + require.NoError(t, err) + + // check TempDir is empty + entries, err := os.ReadDir(tempDir) + require.NoError(t, err) + require.Len(t, entries, 0) +} + func TestMetrics(t *testing.T) { tests := []struct { name string