From 1ad03da6877434c9caca43e1c727d068968e90ed Mon Sep 17 00:00:00 2001 From: Thomas Cuthbert Date: Mon, 29 Dec 2025 09:50:22 +0800 Subject: [PATCH] fix: Logging regression for manifest HEAD requests Since version 3.0.0, the response completed log line is no longer present for HEAD requests to manifests that return 200. The regression is caused by the implicit handling of manifest HEAD responses that bypass the logging middleware when returning from `GetManifest`. This change ensures that the logging middleware handles responses for manifest HEAD requests by explicitly writing `StatusOK` into the response header before returning from `GetManifest`. Closes: https://github.com/distribution/distribution/issues/4733 Signed-off-by: Thomas Cuthbert --- registry/handlers/api_test.go | 14 ++- registry/handlers/manifests.go | 1 + .../sirupsen/logrus/hooks/test/test.go | 91 +++++++++++++++++++ vendor/modules.txt | 1 + 4 files changed, 106 insertions(+), 1 deletion(-) create mode 100644 vendor/github.com/sirupsen/logrus/hooks/test/test.go diff --git a/registry/handlers/api_test.go b/registry/handlers/api_test.go index 754def6c4..bdbf4b111 100644 --- a/registry/handlers/api_test.go +++ b/registry/handlers/api_test.go @@ -34,6 +34,7 @@ import ( "github.com/opencontainers/go-digest" "github.com/opencontainers/image-spec/specs-go" v1 "github.com/opencontainers/image-spec/specs-go/v1" + hookstest "github.com/sirupsen/logrus/hooks/test" ) var headerConfig = http.Header{ @@ -1709,7 +1710,10 @@ func testManifestAPISchema2(t *testing.T, env *testEnv, imageName reference.Name // ------------------ // Fetch by tag name - // HEAD requests should not contain a body + // HEAD requests should emit a logging entry and not contain a body + hook := hookstest.NewGlobal() + defer hook.Reset() + headReq, err := http.NewRequest(http.MethodHead, manifestURL, nil) if err != nil { t.Fatalf("Error constructing request: %s", err) @@ -1726,6 +1730,14 @@ func testManifestAPISchema2(t *testing.T, env *testEnv, imageName reference.Name "ETag": []string{fmt.Sprintf(`"%s"`, dgst)}, }) + lastMsg := hook.LastEntry() + if v := lastMsg.Data["http.request.method"]; v != http.MethodHead { + t.Errorf("expected http.request.method to be %q, got %q", http.MethodHead, v) + } + if v := lastMsg.Data["http.response.status"]; v != http.StatusOK { + t.Errorf("expected http.response.status to be %d, got %d", http.StatusOK, v) + } + headBody, err := io.ReadAll(headResp.Body) if err != nil { t.Fatalf("reading body for head manifest: %v", err) diff --git a/registry/handlers/manifests.go b/registry/handlers/manifests.go index a78dca32b..f9ded0570 100644 --- a/registry/handlers/manifests.go +++ b/registry/handlers/manifests.go @@ -217,6 +217,7 @@ func (imh *manifestHandler) GetManifest(w http.ResponseWriter, r *http.Request) w.Header().Set("Etag", fmt.Sprintf(`"%s"`, imh.Digest)) if r.Method == http.MethodHead { + w.WriteHeader(http.StatusOK) return } diff --git a/vendor/github.com/sirupsen/logrus/hooks/test/test.go b/vendor/github.com/sirupsen/logrus/hooks/test/test.go new file mode 100644 index 000000000..046f0bf6c --- /dev/null +++ b/vendor/github.com/sirupsen/logrus/hooks/test/test.go @@ -0,0 +1,91 @@ +// The Test package is used for testing logrus. +// It provides a simple hooks which register logged messages. +package test + +import ( + "io/ioutil" + "sync" + + "github.com/sirupsen/logrus" +) + +// Hook is a hook designed for dealing with logs in test scenarios. +type Hook struct { + // Entries is an array of all entries that have been received by this hook. + // For safe access, use the AllEntries() method, rather than reading this + // value directly. + Entries []logrus.Entry + mu sync.RWMutex +} + +// NewGlobal installs a test hook for the global logger. +func NewGlobal() *Hook { + + hook := new(Hook) + logrus.AddHook(hook) + + return hook + +} + +// NewLocal installs a test hook for a given local logger. +func NewLocal(logger *logrus.Logger) *Hook { + + hook := new(Hook) + logger.AddHook(hook) + + return hook + +} + +// NewNullLogger creates a discarding logger and installs the test hook. +func NewNullLogger() (*logrus.Logger, *Hook) { + + logger := logrus.New() + logger.Out = ioutil.Discard + + return logger, NewLocal(logger) + +} + +func (t *Hook) Fire(e *logrus.Entry) error { + t.mu.Lock() + defer t.mu.Unlock() + t.Entries = append(t.Entries, *e) + return nil +} + +func (t *Hook) Levels() []logrus.Level { + return logrus.AllLevels +} + +// LastEntry returns the last entry that was logged or nil. +func (t *Hook) LastEntry() *logrus.Entry { + t.mu.RLock() + defer t.mu.RUnlock() + i := len(t.Entries) - 1 + if i < 0 { + return nil + } + return &t.Entries[i] +} + +// AllEntries returns all entries that were logged. +func (t *Hook) AllEntries() []*logrus.Entry { + t.mu.RLock() + defer t.mu.RUnlock() + // Make a copy so the returned value won't race with future log requests + entries := make([]*logrus.Entry, len(t.Entries)) + for i := 0; i < len(t.Entries); i++ { + // Make a copy, for safety + entries[i] = &t.Entries[i] + } + return entries +} + +// Reset removes all Entries from this test hook. +func (t *Hook) Reset() { + t.mu.Lock() + defer t.mu.Unlock() + t.Entries = make([]logrus.Entry, 0) +} diff --git a/vendor/modules.txt b/vendor/modules.txt index d7d1bbee9..212bf46cd 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -442,6 +442,7 @@ github.com/redis/go-redis/v9/internal/util # github.com/sirupsen/logrus v1.9.3 ## explicit; go 1.13 github.com/sirupsen/logrus +github.com/sirupsen/logrus/hooks/test # github.com/spf13/cobra v1.8.0 ## explicit; go 1.15 github.com/spf13/cobra