Skip to content

Commit 3873de5

Browse files
willmurphyscodewillmurphyscode
authored
Fix panic when pulling OCI-packaged helm chart (#228)
* check that diff IDs exist before dereference Previously, this function would panic when parsing OCI-packaged helm charts, which apparently have no diff IDs. Signed-off-by: Will Murphy <[email protected]> * go mod tidy Signed-off-by: Will Murphy <[email protected]> * Only attempt to parse supported layer types Helm charts were causing a panic, but even if parsing the layer metadata succeeded, an error would be returned. Therefore, just return the error pre-emptively on unknown layer media types, since this probably fixes undiscovered bugs similar to the helm chart panic. Signed-off-by: Will Murphy <[email protected]> * refactor Signed-off-by: Will Murphy <[email protected]> * clean up todo and unused file Signed-off-by: Will Murphy <[email protected]> * refactor: address some renames and other feedback Signed-off-by: Will Murphy <[email protected]> --------- Signed-off-by: Will Murphy <[email protected]>
1 parent 8eecb8f commit 3873de5

File tree

6 files changed

+181
-48
lines changed

6 files changed

+181
-48
lines changed

‎go.mod

+1-1
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ require (
9898
go.opentelemetry.io/otel/trace v1.19.0 // indirect
9999
golang.org/x/mod v0.11.0 // indirect
100100
golang.org/x/net v0.23.0 // indirect
101-
golang.org/x/oauth2 v0.10.0 // indirect
101+
golang.org/x/oauth2 v0.18.0 // indirect
102102
golang.org/x/sync v0.3.0 // indirect
103103
golang.org/x/sys v0.18.0 // indirect
104104
golang.org/x/term v0.18.0 // indirect

‎go.sum

+2-2
Original file line numberDiff line numberDiff line change
@@ -306,8 +306,8 @@ golang.org/x/net v0.0.0-20210505024714-0287a6fb4125/go.mod h1:9nx3DQGgdP8bBQD5qx
306306
golang.org/x/net v0.23.0 h1:7EYJ93RZ9vYSZAIb2x3lnuvqO5zneoD6IvWjuhfxjTs=
307307
golang.org/x/net v0.23.0/go.mod h1:JKghWKKOSdJwpW2GEx0Ja7fmaKnMsbu+MWVZTokSYmg=
308308
golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
309-
golang.org/x/oauth2 v0.10.0 h1:zHCpF2Khkwy4mMB4bv0U37YtJdTGW8jI0glAApi0Kh8=
310-
golang.org/x/oauth2 v0.10.0/go.mod h1:kTpgurOux7LqtuxjuyZa4Gj2gdezIt/jQtGnNFfypQI=
309+
golang.org/x/oauth2 v0.18.0 h1:09qnuIAgzdx1XplqJvW6CQqMCtGZykZWcXzPMPUusvI=
310+
golang.org/x/oauth2 v0.18.0/go.mod h1:Wf7knwG0MPoWIMMBgFlEaSUDaKskp0dCfrlJRJXbBi8=
311311
golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
312312
golang.org/x/sync v0.0.0-20181108010431-42b317875d0f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
313313
golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=

‎pkg/image/image.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ func (i *Image) Read() error {
224224

225225
for idx, v1Layer := range v1Layers {
226226
layer := NewLayer(v1Layer)
227-
err := layer.Read(fileCatalog, i.Metadata, idx, i.contentCacheDir)
227+
err := layer.Read(fileCatalog, idx, i.contentCacheDir)
228228
if err != nil {
229229
return err
230230
}

‎pkg/image/layer.go

+71-39
Original file line numberDiff line numberDiff line change
@@ -80,24 +80,16 @@ func (l *Layer) uncompressedTarCache(uncompressedLayersCacheDir string) (string,
8080

8181
// Read parses information from the underlying layer tar into this struct. This includes layer metadata, the layer
8282
// file tree, and the layer squash tree.
83-
func (l *Layer) Read(catalog *FileCatalog, imgMetadata Metadata, idx int, uncompressedLayersCacheDir string) error {
84-
var err error
85-
tree := filetree.New()
86-
l.Tree = tree
87-
l.fileCatalog = catalog
88-
l.Metadata, err = newLayerMetadata(imgMetadata, l.layer, idx)
83+
func (l *Layer) Read(catalog *FileCatalog, idx int, uncompressedLayersCacheDir string) error {
84+
mediaType, err := l.layer.MediaType()
8985
if err != nil {
9086
return err
9187
}
88+
tree := filetree.New()
89+
l.Tree = tree
90+
l.fileCatalog = catalog
9291

93-
log.Debugf("layer metadata: index=%+v digest=%+v mediaType=%+v",
94-
l.Metadata.Index,
95-
l.Metadata.Digest,
96-
l.Metadata.MediaType)
97-
98-
monitor := trackReadProgress(l.Metadata)
99-
100-
switch l.Metadata.MediaType {
92+
switch mediaType {
10193
case types.OCILayer,
10294
types.OCIUncompressedLayer,
10395
types.OCIRestrictedLayer,
@@ -107,44 +99,84 @@ func (l *Layer) Read(catalog *FileCatalog, imgMetadata Metadata, idx int, uncomp
10799
types.DockerForeignLayer,
108100
types.DockerUncompressedLayer:
109101

110-
tarFilePath, err := l.uncompressedTarCache(uncompressedLayersCacheDir)
102+
err := l.readStandardImageLayer(idx, uncompressedLayersCacheDir, tree)
111103
if err != nil {
112104
return err
113105
}
114-
115-
l.indexedContent, err = file.NewTarIndex(
116-
tarFilePath,
117-
layerTarIndexer(tree, l.fileCatalog, &l.Metadata.Size, l, monitor),
118-
)
119-
if err != nil {
120-
return fmt.Errorf("failed to read layer=%q tar : %w", l.Metadata.Digest, err)
121-
}
122-
123106
case SingularitySquashFSLayer:
124-
r, err := l.layer.Uncompressed()
125-
if err != nil {
126-
return fmt.Errorf("failed to read layer=%q: %w", l.Metadata.Digest, err)
127-
}
128-
// defer r.Close() // TODO: if we close this here, we can't read file contents after we return.
129-
130-
// Walk the more efficient walk if we're blessed with an io.ReaderAt.
131-
if ra, ok := r.(io.ReaderAt); ok {
132-
err = file.WalkSquashFS(ra, squashfsVisitor(tree, l.fileCatalog, &l.Metadata.Size, l, monitor))
133-
} else {
134-
err = file.WalkSquashFSFromReader(r, squashfsVisitor(tree, l.fileCatalog, &l.Metadata.Size, l, monitor))
135-
}
107+
err := l.readSingularityImageLayer(idx, tree)
136108
if err != nil {
137-
return fmt.Errorf("failed to walk layer=%q: %w", l.Metadata.Digest, err)
109+
return err
138110
}
139-
140111
default:
141-
return fmt.Errorf("unknown layer media type: %+v", l.Metadata.MediaType)
112+
return fmt.Errorf("unknown layer media type: %+v", mediaType)
142113
}
143114

144115
l.SearchContext = filetree.NewSearchContext(l.Tree, l.fileCatalog.Index)
145116

117+
return nil
118+
}
119+
120+
func (l *Layer) readStandardImageLayer(idx int, uncompressedLayersCacheDir string, tree *filetree.FileTree) error {
121+
var err error
122+
l.Metadata, err = newLayerMetadata(l.layer, idx)
123+
monitor := trackReadProgress(l.Metadata)
124+
if err != nil {
125+
return err
126+
}
127+
128+
log.Debugf("layer metadata: index=%+v digest=%+v mediaType=%+v",
129+
l.Metadata.Index,
130+
l.Metadata.Digest,
131+
l.Metadata.MediaType)
132+
133+
tarFilePath, err := l.uncompressedTarCache(uncompressedLayersCacheDir)
134+
if err != nil {
135+
return err
136+
}
137+
138+
l.indexedContent, err = file.NewTarIndex(
139+
tarFilePath,
140+
layerTarIndexer(tree, l.fileCatalog, &l.Metadata.Size, l, monitor),
141+
)
142+
if err != nil {
143+
return fmt.Errorf("failed to read layer=%q tar : %w", l.Metadata.Digest, err)
144+
}
145+
146146
monitor.SetCompleted()
147+
return nil
148+
}
147149

150+
func (l *Layer) readSingularityImageLayer(idx int, tree *filetree.FileTree) error {
151+
var err error
152+
l.Metadata, err = newLayerMetadata(l.layer, idx)
153+
if err != nil {
154+
return err
155+
}
156+
157+
log.Debugf("layer metadata: index=%+v digest=%+v mediaType=%+v",
158+
l.Metadata.Index,
159+
l.Metadata.Digest,
160+
l.Metadata.MediaType)
161+
162+
monitor := trackReadProgress(l.Metadata)
163+
r, err := l.layer.Uncompressed()
164+
if err != nil {
165+
return fmt.Errorf("failed to read layer=%q: %w", l.Metadata.Digest, err)
166+
}
167+
// defer r.Close() // TODO: if we close this here, we can't read file contents after we return.
168+
169+
// Walk the more efficient walk if we're blessed with an io.ReaderAt.
170+
if ra, ok := r.(io.ReaderAt); ok {
171+
err = file.WalkSquashFS(ra, squashfsVisitor(tree, l.fileCatalog, &l.Metadata.Size, l, monitor))
172+
} else {
173+
err = file.WalkSquashFSFromReader(r, squashfsVisitor(tree, l.fileCatalog, &l.Metadata.Size, l, monitor))
174+
}
175+
if err != nil {
176+
return fmt.Errorf("failed to walk layer=%q: %w", l.Metadata.Digest, err)
177+
}
178+
179+
monitor.SetCompleted()
148180
return nil
149181
}
150182

‎pkg/image/layer_metadata.go

+7-5
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import (
55
v1Types "github.com/google/go-containerregistry/pkg/v1/types"
66
)
77

8-
// Metadata represents container layer metadata.
8+
// LayerMetadata represents container layer metadata.
99
type LayerMetadata struct {
1010
Index uint
1111
// Digest is the sha256 digest of the layer contents (the docker "diff id")
@@ -16,17 +16,19 @@ type LayerMetadata struct {
1616
}
1717

1818
// newLayerMetadata aggregates pertinent layer metadata information.
19-
func newLayerMetadata(imgMetadata Metadata, layer v1.Layer, idx int) (LayerMetadata, error) {
19+
func newLayerMetadata(layer v1.Layer, idx int) (LayerMetadata, error) {
2020
mediaType, err := layer.MediaType()
2121
if err != nil {
2222
return LayerMetadata{}, err
2323
}
24+
diffID, err := layer.DiffID()
25+
if err != nil {
26+
return LayerMetadata{}, err
27+
}
2428

25-
// digest = diff-id = a digest of the uncompressed layer content
26-
diffIDHash := imgMetadata.Config.RootFS.DiffIDs[idx]
2729
return LayerMetadata{
2830
Index: uint(idx),
29-
Digest: diffIDHash.String(),
31+
Digest: diffID.String(),
3032
MediaType: mediaType,
3133
}, nil
3234
}

‎pkg/image/layer_test.go

+99
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
package image
2+
3+
import (
4+
"errors"
5+
"io"
6+
"strings"
7+
"testing"
8+
9+
v1 "github.com/google/go-containerregistry/pkg/v1"
10+
v1Types "github.com/google/go-containerregistry/pkg/v1/types"
11+
"github.com/stretchr/testify/require"
12+
)
13+
14+
type mockLayer struct {
15+
mediaType v1Types.MediaType
16+
err error
17+
}
18+
19+
func (m mockLayer) Digest() (v1.Hash, error) {
20+
return v1.Hash{
21+
Algorithm: "sha256",
22+
Hex: "aaaaaaaaaa1234",
23+
}, nil
24+
}
25+
26+
func (m mockLayer) DiffID() (v1.Hash, error) {
27+
return v1.Hash{
28+
Algorithm: "sha256",
29+
Hex: "aaaaaaaaaa1234",
30+
}, nil
31+
}
32+
33+
func (m mockLayer) Compressed() (io.ReadCloser, error) {
34+
panic("implement me")
35+
}
36+
37+
func (m mockLayer) Uncompressed() (io.ReadCloser, error) {
38+
return io.NopCloser(strings.NewReader("")), nil
39+
}
40+
41+
func (m mockLayer) Size() (int64, error) {
42+
return 0, nil
43+
}
44+
45+
func (m mockLayer) MediaType() (v1Types.MediaType, error) {
46+
return m.mediaType, m.err
47+
}
48+
49+
var _ v1.Layer = &mockLayer{}
50+
51+
func fakeLayer(mediaType v1Types.MediaType, err error) v1.Layer {
52+
return mockLayer{
53+
mediaType: mediaType,
54+
err: err,
55+
}
56+
}
57+
58+
func TestRead(t *testing.T) {
59+
tests := []struct {
60+
name string
61+
mediaType v1Types.MediaType
62+
mediaTypeErr error
63+
wantErrContents string
64+
}{
65+
{
66+
name: "unsupported media type",
67+
mediaType: "garbage",
68+
mediaTypeErr: nil,
69+
wantErrContents: "unknown layer media type: garbage",
70+
},
71+
{
72+
name: "unsupported media type: helm chart",
73+
mediaType: "application/vnd.cncf.helm.chart.content.v1.tar+gzip",
74+
wantErrContents: "application/vnd.cncf.helm.chart.content.v1.tar+gzip",
75+
},
76+
{
77+
name: "err on media type returned",
78+
mediaTypeErr: errors.New("no media type for you"),
79+
wantErrContents: "no media type for you",
80+
},
81+
{
82+
name: "no error",
83+
mediaType: v1Types.DockerLayer,
84+
},
85+
}
86+
87+
for _, tt := range tests {
88+
t.Run(tt.name, func(t *testing.T) {
89+
layer := Layer{layer: fakeLayer(tt.mediaType, tt.mediaTypeErr)}
90+
catalog := NewFileCatalog()
91+
err := layer.Read(catalog, 0, t.TempDir())
92+
if tt.wantErrContents != "" {
93+
require.ErrorContains(t, err, tt.wantErrContents)
94+
return
95+
}
96+
require.NoError(t, err)
97+
})
98+
}
99+
}

0 commit comments

Comments
 (0)