detached 085f464

fix: address code review findings in native registry push

commit
085f46483797235f9024d7dd609baa815f189cb2
author
brice · · 2026-03-14 14:20 UTC
parent
918e8de
signature
unsigned
1 file · +50 -11
Unified
UnifiedSplit
M pkg/registrypush/push.go +50 -11
@@ -4,10 +4,12 @@ import (
44 "context"
55 "crypto/sha256"
66 "encoding/base64"
7+ "errors"
78 "fmt"
89 "io"
910 "net/http"
1011 "net/url"
12+ "os"
1113 "strings"
1214
1315 "github.com/docker/docker/client"
@@ -157,12 +159,44 @@ func defaultImageSource(ctx context.Context, ref string) (v1.Image, error) {
157159 if err != nil {
158160 return nil, fmt.Errorf("failed to export image %s: %w", ref, err)
159161 }
160- defer rc.Close()
161162
162- img, err := tarball.Image(func() (io.ReadCloser, error) {
163- return rc, nil
164- }, &tag)
163+ tmpFile, err := os.CreateTemp("", "gordon-push-*.tar")
165164 if err != nil {
165+ if closeErr := rc.Close(); closeErr != nil {
166+ return nil, fmt.Errorf("failed to create temp file for image export: %w (also failed to close image export stream: %v)", err, closeErr)
167+ }
168+ return nil, fmt.Errorf("failed to create temp file for image export: %w", err)
169+ }
170+ tmpPath := tmpFile.Name()
171+
172+ if _, err := io.Copy(tmpFile, rc); err != nil {
173+ if closeErr := rc.Close(); closeErr != nil {
174+ _ = tmpFile.Close()
175+ _ = os.Remove(tmpPath)
176+ return nil, fmt.Errorf("failed to write image tar to temp file: %w (also failed to close image export stream: %v)", err, closeErr)
177+ }
178+ if closeErr := tmpFile.Close(); closeErr != nil {
179+ _ = os.Remove(tmpPath)
180+ return nil, fmt.Errorf("failed to write image tar to temp file: %w (also failed to close temp file: %v)", err, closeErr)
181+ }
182+ if removeErr := os.Remove(tmpPath); removeErr != nil {
183+ return nil, fmt.Errorf("failed to write image tar to temp file: %w (also failed to remove temp file %s: %v)", err, tmpPath, removeErr)
184+ }
185+ return nil, fmt.Errorf("failed to write image tar to temp file: %w", err)
186+ }
187+ if err := rc.Close(); err != nil {
188+ _ = tmpFile.Close()
189+ _ = os.Remove(tmpPath)
190+ return nil, fmt.Errorf("failed to close image export stream: %w", err)
191+ }
192+ if err := tmpFile.Close(); err != nil {
193+ _ = os.Remove(tmpPath)
194+ return nil, fmt.Errorf("failed to close temp file %s: %w", tmpPath, err)
195+ }
196+
197+ img, err := tarball.ImageFromPath(tmpPath, &tag)
198+ if err != nil {
199+ _ = os.Remove(tmpPath)
166200 return nil, fmt.Errorf("failed to parse image tar for %s: %w", ref, err)
167201 }
168202 return img, nil
@@ -247,7 +281,8 @@ func (p *Pusher) uploadManifest(ctx context.Context, baseURL, repo, tag string,
247281 defer resp.Body.Close()
248282
249283 if resp.StatusCode != http.StatusCreated {
250- return fmt.Errorf("manifest upload returned status %d", resp.StatusCode)
284+ body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024))
285+ return fmt.Errorf("manifest upload returned status %d: %s", resp.StatusCode, string(body))
251286 }
252287
253288 return nil
@@ -279,7 +314,7 @@ func registryBaseURL(host string) string {
279314 return "https://" + host
280315 }
281316
282-var errBlobMissing = fmt.Errorf("blob missing")
317+var errBlobMissing = errors.New("blob missing")
283318
284319 func (p *Pusher) checkBlobExists(ctx context.Context, baseURL, repo, digest, auth string) error {
285320 req, err := http.NewRequestWithContext(ctx, http.MethodHead, fmt.Sprintf("%s/v2/%s/blobs/%s", baseURL, repo, digest), nil)
@@ -300,7 +335,8 @@ func (p *Pusher) checkBlobExists(ctx context.Context, baseURL, repo, digest, aut
300335 case http.StatusNotFound:
301336 return errBlobMissing
302337 default:
303- return fmt.Errorf("blob existence check returned status %d", resp.StatusCode)
338+ body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024))
339+ return fmt.Errorf("blob existence check returned status %d: %s", resp.StatusCode, string(body))
304340 }
305341 }
306342
@@ -318,7 +354,8 @@ func (p *Pusher) startBlobUpload(ctx context.Context, baseURL, repo, auth string
318354 defer resp.Body.Close()
319355
320356 if resp.StatusCode != http.StatusAccepted {
321- return "", fmt.Errorf("blob upload start returned status %d", resp.StatusCode)
357+ body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024))
358+ return "", fmt.Errorf("blob upload start returned status %d: %s", resp.StatusCode, string(body))
322359 }
323360
324361 location, err := resolveLocation(baseURL, resp.Header.Get("Location"))
@@ -357,7 +394,7 @@ func (p *Pusher) uploadBlobChunks(ctx context.Context, uploadURL string, content
357394 }
358395
359396 func (p *Pusher) uploadBlobChunk(ctx context.Context, uploadURL string, chunk []byte, offset int64, auth string) (string, error) {
360- req, err := http.NewRequestWithContext(ctx, http.MethodPatch, uploadURL, io.NopCloser(io.LimitReader(bytesReader(chunk), int64(len(chunk)))))
397+ req, err := http.NewRequestWithContext(ctx, http.MethodPatch, uploadURL, bytesReader(chunk))
361398 if err != nil {
362399 return "", fmt.Errorf("failed to create blob chunk request: %w", err)
363400 }
@@ -373,7 +410,8 @@ func (p *Pusher) uploadBlobChunk(ctx context.Context, uploadURL string, chunk []
373410 defer resp.Body.Close()
374411
375412 if resp.StatusCode != http.StatusAccepted {
376- return "", fmt.Errorf("blob chunk upload returned status %d", resp.StatusCode)
413+ body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024))
414+ return "", fmt.Errorf("blob chunk upload returned status %d: %s", resp.StatusCode, string(body))
377415 }
378416
379417 nextURL, err := resolveLocation(uploadURL, resp.Header.Get("Location"))
@@ -407,7 +445,8 @@ func (p *Pusher) finalizeBlobUpload(ctx context.Context, uploadURL, digest, auth
407445 defer resp.Body.Close()
408446
409447 if resp.StatusCode != http.StatusCreated {
410- return fmt.Errorf("blob upload finalize returned status %d", resp.StatusCode)
448+ body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024))
449+ return fmt.Errorf("blob upload finalize returned status %d: %s", resp.StatusCode, string(body))
411450 }
412451
413452 return nil
@@ -4,10 +4,12 @@ import (
4 "context"4 "context"
5 "crypto/sha256"5 "crypto/sha256"
6 "encoding/base64"6 "encoding/base64"
7+ "errors"
7 "fmt"8 "fmt"
8 "io"9 "io"
9 "net/http"10 "net/http"
10 "net/url"11 "net/url"
12+ "os"
11 "strings"13 "strings"
12 14
13 "github.com/docker/docker/client"15 "github.com/docker/docker/client"
@@ -157,12 +159,44 @@ func defaultImageSource(ctx context.Context, ref string) (v1.Image, error) {
157 if err != nil {159 if err != nil {
158 return nil, fmt.Errorf("failed to export image %s: %w", ref, err)160 return nil, fmt.Errorf("failed to export image %s: %w", ref, err)
159 }161 }
160- defer rc.Close()
161 162
162- img, err := tarball.Image(func() (io.ReadCloser, error) {163+ tmpFile, err := os.CreateTemp("", "gordon-push-*.tar")
163- return rc, nil
164- }, &tag)
165 if err != nil {164 if err != nil {
165+ if closeErr := rc.Close(); closeErr != nil {
166+ return nil, fmt.Errorf("failed to create temp file for image export: %w (also failed to close image export stream: %v)", err, closeErr)
167+ }
168+ return nil, fmt.Errorf("failed to create temp file for image export: %w", err)
169+ }
170+ tmpPath := tmpFile.Name()
171+
172+ if _, err := io.Copy(tmpFile, rc); err != nil {
173+ if closeErr := rc.Close(); closeErr != nil {
174+ _ = tmpFile.Close()
175+ _ = os.Remove(tmpPath)
176+ return nil, fmt.Errorf("failed to write image tar to temp file: %w (also failed to close image export stream: %v)", err, closeErr)
177+ }
178+ if closeErr := tmpFile.Close(); closeErr != nil {
179+ _ = os.Remove(tmpPath)
180+ return nil, fmt.Errorf("failed to write image tar to temp file: %w (also failed to close temp file: %v)", err, closeErr)
181+ }
182+ if removeErr := os.Remove(tmpPath); removeErr != nil {
183+ return nil, fmt.Errorf("failed to write image tar to temp file: %w (also failed to remove temp file %s: %v)", err, tmpPath, removeErr)
184+ }
185+ return nil, fmt.Errorf("failed to write image tar to temp file: %w", err)
186+ }
187+ if err := rc.Close(); err != nil {
188+ _ = tmpFile.Close()
189+ _ = os.Remove(tmpPath)
190+ return nil, fmt.Errorf("failed to close image export stream: %w", err)
191+ }
192+ if err := tmpFile.Close(); err != nil {
193+ _ = os.Remove(tmpPath)
194+ return nil, fmt.Errorf("failed to close temp file %s: %w", tmpPath, err)
195+ }
196+
197+ img, err := tarball.ImageFromPath(tmpPath, &tag)
198+ if err != nil {
199+ _ = os.Remove(tmpPath)
166 return nil, fmt.Errorf("failed to parse image tar for %s: %w", ref, err)200 return nil, fmt.Errorf("failed to parse image tar for %s: %w", ref, err)
167 }201 }
168 return img, nil202 return img, nil
@@ -247,7 +281,8 @@ func (p *Pusher) uploadManifest(ctx context.Context, baseURL, repo, tag string,
247 defer resp.Body.Close()281 defer resp.Body.Close()
248 282
249 if resp.StatusCode != http.StatusCreated {283 if resp.StatusCode != http.StatusCreated {
250- return fmt.Errorf("manifest upload returned status %d", resp.StatusCode)284+ body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024))
285+ return fmt.Errorf("manifest upload returned status %d: %s", resp.StatusCode, string(body))
251 }286 }
252 287
253 return nil288 return nil
@@ -279,7 +314,7 @@ func registryBaseURL(host string) string {
279 return "https://" + host314 return "https://" + host
280 }315 }
281 316
282-var errBlobMissing = fmt.Errorf("blob missing")317+var errBlobMissing = errors.New("blob missing")
283 318
284 func (p *Pusher) checkBlobExists(ctx context.Context, baseURL, repo, digest, auth string) error {319 func (p *Pusher) checkBlobExists(ctx context.Context, baseURL, repo, digest, auth string) error {
285 req, err := http.NewRequestWithContext(ctx, http.MethodHead, fmt.Sprintf("%s/v2/%s/blobs/%s", baseURL, repo, digest), nil)320 req, err := http.NewRequestWithContext(ctx, http.MethodHead, fmt.Sprintf("%s/v2/%s/blobs/%s", baseURL, repo, digest), nil)
@@ -300,7 +335,8 @@ func (p *Pusher) checkBlobExists(ctx context.Context, baseURL, repo, digest, aut
300 case http.StatusNotFound:335 case http.StatusNotFound:
301 return errBlobMissing336 return errBlobMissing
302 default:337 default:
303- return fmt.Errorf("blob existence check returned status %d", resp.StatusCode)338+ body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024))
339+ return fmt.Errorf("blob existence check returned status %d: %s", resp.StatusCode, string(body))
304 }340 }
305 }341 }
306 342
@@ -318,7 +354,8 @@ func (p *Pusher) startBlobUpload(ctx context.Context, baseURL, repo, auth string
318 defer resp.Body.Close()354 defer resp.Body.Close()
319 355
320 if resp.StatusCode != http.StatusAccepted {356 if resp.StatusCode != http.StatusAccepted {
321- return "", fmt.Errorf("blob upload start returned status %d", resp.StatusCode)357+ body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024))
358+ return "", fmt.Errorf("blob upload start returned status %d: %s", resp.StatusCode, string(body))
322 }359 }
323 360
324 location, err := resolveLocation(baseURL, resp.Header.Get("Location"))361 location, err := resolveLocation(baseURL, resp.Header.Get("Location"))
@@ -357,7 +394,7 @@ func (p *Pusher) uploadBlobChunks(ctx context.Context, uploadURL string, content
357 }394 }
358 395
359 func (p *Pusher) uploadBlobChunk(ctx context.Context, uploadURL string, chunk []byte, offset int64, auth string) (string, error) {396 func (p *Pusher) uploadBlobChunk(ctx context.Context, uploadURL string, chunk []byte, offset int64, auth string) (string, error) {
360- req, err := http.NewRequestWithContext(ctx, http.MethodPatch, uploadURL, io.NopCloser(io.LimitReader(bytesReader(chunk), int64(len(chunk)))))397+ req, err := http.NewRequestWithContext(ctx, http.MethodPatch, uploadURL, bytesReader(chunk))
361 if err != nil {398 if err != nil {
362 return "", fmt.Errorf("failed to create blob chunk request: %w", err)399 return "", fmt.Errorf("failed to create blob chunk request: %w", err)
363 }400 }
@@ -373,7 +410,8 @@ func (p *Pusher) uploadBlobChunk(ctx context.Context, uploadURL string, chunk []
373 defer resp.Body.Close()410 defer resp.Body.Close()
374 411
375 if resp.StatusCode != http.StatusAccepted {412 if resp.StatusCode != http.StatusAccepted {
376- return "", fmt.Errorf("blob chunk upload returned status %d", resp.StatusCode)413+ body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024))
414+ return "", fmt.Errorf("blob chunk upload returned status %d: %s", resp.StatusCode, string(body))
377 }415 }
378 416
379 nextURL, err := resolveLocation(uploadURL, resp.Header.Get("Location"))417 nextURL, err := resolveLocation(uploadURL, resp.Header.Get("Location"))
@@ -407,7 +445,8 @@ func (p *Pusher) finalizeBlobUpload(ctx context.Context, uploadURL, digest, auth
407 defer resp.Body.Close()445 defer resp.Body.Close()
408 446
409 if resp.StatusCode != http.StatusCreated {447 if resp.StatusCode != http.StatusCreated {
410- return fmt.Errorf("blob upload finalize returned status %d", resp.StatusCode)448+ body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024))
449+ return fmt.Errorf("blob upload finalize returned status %d: %s", resp.StatusCode, string(body))
411 }450 }
412 451
413 return nil452 return nil

Keyboard shortcuts

Single-key shortcuts are on. ctrl k and esc work whatever this says.

ctrl k :
go to a section, branch, tag, file or repository
?
this list
j k
move the row cursor down / up
enter
open the row under the cursor; on Files, a directory opens or closes in place
↑ ↓ ← → home end
in the file tree (focus on a row): previous / next row, close / open a directory or go to its parent / first entry, first / last row. They never open a file
l h
on Files: open a directory or a file / close a directory or go up to its parent, then to the parent page. Elsewhere: into the side panel (on a merge request, its files) / back to the content; there j k move between its controls
g g G
first / last row
/
filter the list; on Files, find a file anywhere in the repository (esc clears, enter opens the best match)
b
switch branch or tag
g s g f g c g b g m g i g r g a
tabs: summary, files, commits, branches & tags, merge requests, issues, releases, actions (when available)
g h
home
w
wide mode: use the whole width of the screen, or not (remembered)
y
copy a permalink pinned to the commit
d u x
diff: split / unified / collapse or expand all
e E
diff: the whole file in view / every file, with the lines the hunks hide
n
merge request: next note in the diff
esc
close a dialog, leave a field

Branches 8

Tags 106

↑↓ moveenter openesc close