Merge pull request #2719 from carolynvs/fix-inconsistent-rebuild

Fix inconsistent rebuild

pkg/cnab/config-adapter/stamp.go

@@ -8,13 +8,15 @@ import (
  "encoding/json"
  "errors"
  "fmt"
+ "sort"
 
  "get.porter.sh/porter/pkg"
  "get.porter.sh/porter/pkg/cnab"
  "get.porter.sh/porter/pkg/config"
  "get.porter.sh/porter/pkg/manifest"
  "get.porter.sh/porter/pkg/portercontext"
  "get.porter.sh/porter/pkg/tracing"
+ "github.com/Masterminds/semver/v3"
 )
 
 // Stamp contains Porter specific metadata about a bundle that we can place
@@ -69,10 +71,48 @@ func (s Stamp) WriteManifest(cxt *portercontext.Context, path string) error {
 // MixinRecord contains information about a mixin used in a bundle
 // For now it is a placeholder for data that we would like to include in the future.
 type MixinRecord struct {
+ // Name of the mixin used in the bundle. This is used for sorting only, and
+ // should not be written to the Porter's stamp in bundle.json because we are
+ // storing these mixin records in a map, keyed by the mixin name.
+ Name string `json:"-"`
+
  // Version of the mixin used in the bundle.
  Version string `json:"version"`
 }
 
+type MixinRecords []MixinRecord
+
+func (m MixinRecords) Len() int {
+ return len(m)
+}
+
+func (m MixinRecords) Less(i, j int) bool {
+ // Currently there can only be a single version of a mixin used in a bundle
+ // I'm considering version as well for sorting in case that changes in the future once mixins are bundles
+ // referenced by a bundle, and not embedded binaries
+ iRecord := m[i]
+ jRecord := m[j]
+ if iRecord.Name == jRecord.Name {
+ // Try to sort by the mixin's semantic version
+ // If it doesn't parse, just fall through and sort as a string instead
+ iVersion, iErr := semver.NewVersion(iRecord.Version)
+ jVersion, jErr := semver.NewVersion(jRecord.Version)
+ if iErr == nil && jErr == nil {
+ return iVersion.LessThan(jVersion)
+ } else {
+ return iRecord.Version < jRecord.Version
+ }
+ }
+
+ return iRecord.Name < jRecord.Name
+}
+
+func (m MixinRecords) Swap(i, j int) {
+ tmp := m[i]
+ m[i] = m[j]
+ m[j] = tmp
+}
+
 func (c *ManifestConverter) GenerateStamp(ctx context.Context) (Stamp, error) {
  log := tracing.LoggerFromContext(ctx)
 
@@ -86,11 +126,9 @@ func (c *ManifestConverter) GenerateStamp(ctx context.Context) (Stamp, error) {
  stamp.EncodedManifest = base64.StdEncoding.EncodeToString(rawManifest)
 
  stamp.Mixins = make(map[string]MixinRecord, len(c.Manifest.Mixins))
- usedMixinsVersion := c.getUsedMixinsVersion()
- for usedMixinName, usedMixinVersion := range usedMixinsVersion {
- stamp.Mixins[usedMixinName] = MixinRecord{
- Version: usedMixinVersion,
- }
+ usedMixins := c.getUsedMixinRecords()
+ for _, record := range usedMixins {
+ stamp.Mixins[record.Name] = record
  }
 
  digest, err := c.DigestManifest()
@@ -122,9 +160,10 @@ func (c *ManifestConverter) DigestManifest() (string, error) {
  v := pkg.Version
  data = append(data, v...)
 
- usedMixinsVersion := c.getUsedMixinsVersion()
- for usedMixinName, usedMixinVersion := range usedMixinsVersion {
- data = append(append(data, usedMixinName...), usedMixinVersion...)
+ usedMixins := c.getUsedMixinRecords()
+ sort.Sort(usedMixins) // Ensure that this is sorted so the digest is consistent
+ for _, mixinRecord := range usedMixins {
+ data = append(append(data, mixinRecord.Name...), mixinRecord.Version...)
  }
 
  digest := sha256.Sum256(data)
@@ -152,17 +191,22 @@ func LoadStamp(bun cnab.ExtendedBundle) (Stamp, error) {
  return stamp, nil
 }
 
-// getUsedMixinsVersion compare the mixins defined in the manifest and the ones installed and then retrieve the mixin's version info
-func (c *ManifestConverter) getUsedMixinsVersion() map[string]string {
- usedMixinsVersion := make(map[string]string)
+// getUsedMixinRecords returns a list of the mixins used by the bundle, including
+// information about the installed mixin, such as its version.
+func (c *ManifestConverter) getUsedMixinRecords() MixinRecords {
+ usedMixins := make(MixinRecords, 0)
 
  for _, usedMixin := range c.Manifest.Mixins {
  for _, installedMixin := range c.InstalledMixins {
  if usedMixin.Name == installedMixin.Name {
- usedMixinsVersion[usedMixin.Name] = installedMixin.GetVersionInfo().Version
+ usedMixins = append(usedMixins, MixinRecord{
+ Name: installedMixin.Name,
+ Version: installedMixin.GetVersionInfo().Version,
+ })
  }
  }
  }
 
- return usedMixinsVersion
+ sort.Sort(usedMixins)
+ return usedMixins
 }

pkg/cnab/config-adapter/stamp_test.go

@@ -2,6 +2,7 @@ package configadapter
 
 import (
  "context"
+ "sort"
  "testing"
 
  "get.porter.sh/porter/pkg"
@@ -36,7 +37,7 @@ func TestConfig_GenerateStamp(t *testing.T) {
  stamp, err := a.GenerateStamp(ctx)
  require.NoError(t, err, "DigestManifest failed")
  assert.Equal(t, simpleManifestDigest, stamp.ManifestDigest)
- assert.Equal(t, map[string]MixinRecord{"exec": {Version: "v1.2.3"}}, stamp.Mixins, "Stamp.Mixins was not populated properly")
+ assert.Equal(t, map[string]MixinRecord{"exec": {Name: "exec", Version: "v1.2.3"}}, stamp.Mixins, "Stamp.Mixins was not populated properly")
  assert.Equal(t, pkg.Version, stamp.Version)
  assert.Equal(t, pkg.Commit, stamp.Commit)
 
@@ -181,3 +182,52 @@ func TestConfig_GenerateStamp_IncludeVersion(t *testing.T) {
  assert.Equal(t, "v1.2.3", stamp.Version)
  assert.Equal(t, "abc123", stamp.Commit)
 }
+
+func TestMixinRecord_Sort(t *testing.T) {
+ records := MixinRecords{
+ {Name: "helm", Version: "0.1.13"},
+ {Name: "helm", Version: "v0.1.2"},
+ {Name: "testmixin", Version: "1.2.3"},
+ {Name: "exec", Version: "2.1.0"},
+ // These won't parse as valid semver, so just sort them by the string representation instead
+ {
+ Name: "az",
+ Version: "invalid-version2",
+ },
+ {
+ Name: "az",
+ Version: "invalid-version1",
+ },
+ }
+
+ sort.Sort(records)
+
+ wantRecords := MixinRecords{
+ {
+ Name: "az",
+ Version: "invalid-version1",
+ },
+ {
+ Name: "az",
+ Version: "invalid-version2",
+ },
+ {
+ Name: "exec",
+ Version: "2.1.0",
+ },
+ {
+ Name: "helm",
+ Version: "v0.1.2",
+ },
+ {
+ Name: "helm",
+ Version: "0.1.13",
+ },
+ {
+ Name: "testmixin",
+ Version: "1.2.3",
+ },
+ }
+
+ assert.Equal(t, wantRecords, records)
+}

pkg/porter/lifecycle.go

@@ -211,7 +211,11 @@ func (p *Porter) resolveBundleReference(ctx context.Context, opts *BundleReferen
  return cnab.BundleReference{}, err
  }
  } else if opts.File != "" { // load the local bundle source
- localBundle, err := p.ensureLocalBundleIsUpToDate(ctx, opts.BundleDefinitionOptions)
+ buildOpts := BuildOptions{
+ BundleDefinitionOptions: opts.BundleDefinitionOptions,
+ InsecureRegistry: opts.InsecureRegistry,
+ }
+ localBundle, err := p.ensureLocalBundleIsUpToDate(ctx, buildOpts)
  if err != nil {
  return cnab.BundleReference{}, err
  }

pkg/porter/publish.go

@@ -99,7 +99,11 @@ func (p *Porter) publishFromFile(ctx context.Context, opts PublishOptions) error
  ctx, log := tracing.StartSpan(ctx)
  defer log.EndSpan()
 
- _, err := p.ensureLocalBundleIsUpToDate(ctx, opts.BundleDefinitionOptions)
+ buildOpts := BuildOptions{
+ BundleDefinitionOptions: opts.BundleDefinitionOptions,
+ InsecureRegistry: opts.InsecureRegistry,
+ }
+ _, err := p.ensureLocalBundleIsUpToDate(ctx, buildOpts)
  if err != nil {
  return err
  }

pkg/porter/stamp.go

@@ -2,6 +2,7 @@ package porter
 
 import (
  "context"
+ "encoding/json"
  "errors"
  "fmt"
  "os"
@@ -18,7 +19,7 @@ import (
 
 // ensureLocalBundleIsUpToDate ensures that the bundle is up-to-date with the porter manifest,
 // if it is out-of-date, performs a build of the bundle.
-func (p *Porter) ensureLocalBundleIsUpToDate(ctx context.Context, opts BundleDefinitionOptions) (cnab.BundleReference, error) {
+func (p *Porter) ensureLocalBundleIsUpToDate(ctx context.Context, opts BuildOptions) (cnab.BundleReference, error) {
  ctx, log := tracing.StartSpan(ctx,
  attribute.Bool("autobuild-disabled", opts.AutoBuildDisabled))
  defer log.EndSpan()
@@ -27,7 +28,7 @@ func (p *Porter) ensureLocalBundleIsUpToDate(ctx context.Context, opts BundleDef
  return cnab.BundleReference{}, nil
  }
 
- upToDate, err := p.IsBundleUpToDate(ctx, opts)
+ upToDate, err := p.IsBundleUpToDate(ctx, opts.BundleDefinitionOptions)
  if err != nil {
  log.Warnf("WARNING: %w", err)
  }
@@ -36,12 +37,15 @@ func (p *Porter) ensureLocalBundleIsUpToDate(ctx context.Context, opts BundleDef
  if opts.AutoBuildDisabled {
  log.Warn("WARNING: The bundle is out-of-date. Skipping autobuild because --autobuild-disabled was specified")
  } else {
+ log.Info("Changes have been detected and the previously built bundle is out-of-date, rebuilding the bundle before proceeding...")
  log.Info("Building bundle ===>")
  // opts.File is non-empty, which overrides opts.CNABFile if set
  // (which may be if a cached bundle is fetched e.g. when running an action)
  opts.CNABFile = ""
- buildOpts := BuildOptions{BundleDefinitionOptions: opts}
- buildOpts.Validate(p)
+ buildOpts := opts
+ if err = buildOpts.Validate(p); err != nil {
+ return cnab.BundleReference{}, log.Errorf("Validation of build options when autobuilding the bundle failed: %w", err)
+ }
  err := p.Build(ctx, buildOpts)
  if err != nil {
  return cnab.BundleReference{}, err
@@ -67,62 +71,99 @@ func (p *Porter) IsBundleUpToDate(ctx context.Context, opts BundleDefinitionOpti
  ctx, span := tracing.StartSpan(ctx)
  defer span.EndSpan()
 
+ span.Debugf("Checking if the bundle is up-to-date...")
+
+ // This is a prefix for any message that explains why the bundle is out-of-date
+ const rebuildMessagePrefix = "Bundle is out-of-date and must be rebuilt"
+
  if opts.File == "" {
- return false, span.Error(errors.New("File is required"))
+ span.Debugf("%s because the current bundle was not specified. Please report this as a bug!", rebuildMessagePrefix)
+ return false, span.Errorf("File is required")
  }
  m, err := manifest.LoadManifestFrom(ctx, p.Config, opts.File)
  if err != nil {
- return false, err
+ err = fmt.Errorf("the current bundle could not be read: %w", err)
+ span.Debugf("%s: %w", rebuildMessagePrefix, err)
+ return false, span.Error(err)
  }
 
  if exists, _ := p.FileSystem.Exists(opts.CNABFile); exists {
  bun, err := cnab.LoadBundle(p.Context, opts.CNABFile)
  if err != nil {
- return false, span.Error(fmt.Errorf("could not marshal data from %s: %w", opts.CNABFile, err))
+ err = fmt.Errorf("the previously built bundle at %s could not be read: %w", opts.CNABFile, err)
+ span.Debugf("%s: %w", rebuildMessagePrefix, err)
+ return false, span.Error(err)
  }
 
  // Check whether invocation images exist in host registry.
  for _, invocationImage := range bun.InvocationImages {
  // if the invocationImage is built before using a random string tag,
  // we should rebuild it with the new format
  if strings.HasSuffix(invocationImage.Image, "-installer") {
+ span.Debugf("%s because it uses the old -installer suffixed image name (%s)", invocationImage.Image)
  return false, nil
  }
 
  imgRef, err := cnab.ParseOCIReference(invocationImage.Image)
  if err != nil {
- return false, span.Errorf("error parsing %s as an OCI image reference: %w", invocationImage.Image, err)
+ err = fmt.Errorf("error parsing %s as an OCI image reference: %w", invocationImage.Image, err)
+ span.Debugf("%s: %w", rebuildMessagePrefix, err)
+ return false, span.Error(err)
  }
 
  _, err = p.Registry.GetCachedImage(ctx, imgRef)
  if err != nil {
  if errors.Is(err, cnabtooci.ErrNotFound{}) {
- span.Debugf("Invocation image %s doesn't exist in the local image cache, will need to build first", invocationImage.Image)
+ span.Debugf("%s because the invocation image %s doesn't exist in the local image cache", rebuildMessagePrefix, invocationImage.Image)
  return false, nil
  }
- return false, err
-
+ err = fmt.Errorf("an error occurred checking the Docker cache for the invocation image: %w", err)
+ span.Debugf("%s: %w", rebuildMessagePrefix, err)
+ return false, span.Error(err)
  }
  }
 
  oldStamp, err := configadapter.LoadStamp(bun)
  if err != nil {
- return false, span.Error(fmt.Errorf("could not load stamp from %s: %w", opts.CNABFile, err))
+ err = fmt.Errorf("could not load stamp from %s: %w", opts.CNABFile, err)
+ span.Debugf("%s: %w", rebuildMessagePrefix)
+ return false, span.Error(err)
  }
 
  mixins, err := p.getUsedMixins(ctx, m)
  if err != nil {
- return false, fmt.Errorf("error while listing used mixins: %w", err)
+ err = fmt.Errorf("an error occurred while listing used mixins: %w", err)
+ span.Debugf("%s: %w", rebuildMessagePrefix, err)
+ return false, span.Error(err)
  }
 
  converter := configadapter.NewManifestConverter(p.Config, m, nil, mixins)
  newDigest, err := converter.DigestManifest()
  if err != nil {
- span.Debugf("could not determine if the bundle is up-to-date so will rebuild just in case: %w", err)
+ err = fmt.Errorf("the current manifest digest cannot be calculated: %w", err)
+ span.Debugf("%s: %w", rebuildMessagePrefix, err)
+ return false, span.Error(err)
+ }
+
+ manifestChanged := oldStamp.ManifestDigest != newDigest
+ if manifestChanged {
+ span.Debugf("%s because the cached bundle is stale", rebuildMessagePrefix)
+ if span.IsTracingEnabled() {
+ previousStampB, _ := json.Marshal(oldStamp)
+ currentStamp, _ := converter.GenerateStamp(ctx)
+ currentStampB, _ := json.Marshal(currentStamp)
+ span.SetAttributes(
+ attribute.String("previous-stamp", string(previousStampB)),
+ attribute.String("current-stamp", string(currentStampB)),
+ )
+ }
  return false, nil
  }
- return oldStamp.ManifestDigest == newDigest, nil
+
+ span.Debugf("Bundle is up-to-date!")
+ return true, nil
  }
 
+ span.Debugf("%s because a previously built bundle was not found", rebuildMessagePrefix)
  return false, nil
 }