Skip to content

Commit

Permalink
fix: cache: serialize legacy import field (#603)
Browse files Browse the repository at this point in the history
Fixes #592 by serializing the `wasLegacyImport` field to ensure that the
cache records the correct value when an image has it set.

Includes a test based on Serge's description in #592.

The symptom is an error "cache miss because layer definition was
changed" when the definition could not have changed since build.
This happens during multi file builds in builds with dependencies, and
in build-then-publish flows, where the build will be fine but the
publish will fail with the same error.

Using `--debug` to show the cache mismatch shows that somewhere along
the line, the layer is being serialized with the wrong default value
when writing to the cache.

Needs to change currentCacheVersion to ensure compatibility, see the
TestCacheEntryChanged function comment for more info.

Signed-off-by: Michael McCracken <mikmccra@cisco.com>
  • Loading branch information
mikemccracken authored Mar 20, 2024
1 parent f3a8d3b commit 1910fed
Show file tree
Hide file tree
Showing 6 changed files with 42 additions and 12 deletions.
6 changes: 4 additions & 2 deletions pkg/stacker/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ func (b *Builder) updateOCIConfigForOutput(sf *types.Stackerfile, s types.Storag
}

inDir := types.InternalStackerDir
if l.WasLegacyImport() {
if l.WasLegacyImport {
inDir = types.LegacyInternalStackerDir
}

Expand Down Expand Up @@ -372,7 +372,9 @@ func (b *Builder) build(s types.Storage, file string) error {

log.Infof("preparing image %s...", name)
inDir := types.InternalStackerDir
if l.WasLegacyImport() {
if l.WasLegacyImport {
log.Debugf("image %s uses legacy import syntax, will also mount imports at %s",
name, types.LegacyInternalStackerDir)
inDir = types.LegacyInternalStackerDir
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/stacker/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (
"stackerbuild.io/stacker/pkg/types"
)

const currentCacheVersion = 13
const currentCacheVersion = 14

type ImportType int

Expand Down
2 changes: 1 addition & 1 deletion pkg/stacker/cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,5 +125,5 @@ func TestCacheEntryChanged(t *testing.T) {
// This test works because the type information is included in the
// hashstructure hash above, so using a zero valued CacheEntry is
// enough to capture changes in types.
assert.Equal(uint64(0x4ef432528243e356), h)
assert.Equal(uint64(0xa26696f335211127), h)
}
9 changes: 2 additions & 7 deletions pkg/types/layer.go
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ type Layer struct {
OS *string `yaml:"os" json:"os,omitempty"`
Arch *string `yaml:"arch" json:"arch,omitempty"`
Bom *Bom `yaml:"bom" json:"bom,omitempty"`
wasLegacyImport bool
WasLegacyImport bool `yaml:"was_legacy_import" json:"was_legacy_import,omitempty"`
}

func parseLayers(referenceDirectory string, lms yaml.MapSlice, requireHash bool) (map[string]Layer, error) {
Expand Down Expand Up @@ -369,7 +369,7 @@ func parseLayers(referenceDirectory string, lms yaml.MapSlice, requireHash bool)
if len(layer.LegacyImport) != 0 {
layer.Imports = layer.LegacyImport
layer.LegacyImport = nil
layer.wasLegacyImport = true
layer.WasLegacyImport = true
}

ret[name], err = layer.absolutify(referenceDirectory)
Expand Down Expand Up @@ -442,11 +442,6 @@ func (l Layer) absolutify(referenceDirectory string) (Layer, error) {
return ret, nil
}

// WasLegacyImport - return true if this layer used legacy 'import' directive.
func (l Layer) WasLegacyImport() bool {
return l.wasLegacyImport
}

func requireImportHash(imports Imports) error {
for _, imp := range imports {
url, err := NewDockerishUrl(imp.Path)
Expand Down
2 changes: 1 addition & 1 deletion pkg/types/stackerfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ func NewStackerfile(stackerfile string, validateHash bool, substitutions []strin

for _, name := range sf.FileOrder {
layer := sf.internal[name]
if layer.WasLegacyImport() {
if layer.WasLegacyImport {
log.Warnf("'import' directive used in layer '%s' inside file '%s' is deprecated. "+
"Support for 'import' will be removed in releases after 2025-01-01. "+
"Migrate by changing 'import' to 'imports' and '/stacker' to '/stacker/imports'. "+
Expand Down
33 changes: 33 additions & 0 deletions test/caching.bats
Original file line number Diff line number Diff line change
Expand Up @@ -225,3 +225,36 @@ EOF
echo '{"version": 1, "cache": "lolnope"}' > .stacker/build.cache
stacker build --substitute BUSYBOX_OCI=${BUSYBOX_OCI}
}

@test "cache comparison of legacy import works across prereqs" {
echo "well, hi!"> helloworld

cat >> stacker-req.yaml << "EOF"
base:
from:
type: oci
url: ${{BUSYBOX_OCI}}
build_only: true
import:
- helloworld
run: |
cp /stacker/helloworld /
EOF

cat >> stacker.yaml << "EOF"
config:
prerequisites:
- stacker-req.yaml
buildenv:
from:
type: built
tag: base
import:
- stacker://base/helloworld
run: |
cp /stacker/helloworld /h3
EOF
stacker --debug build --substitute BUSYBOX_OCI=${BUSYBOX_OCI}
}

0 comments on commit 1910fed

Please sign in to comment.