hcp: fix hcp artifact extraction method

With Packer 1.10.1 we started warning when a build failed to complete
because of a potential incompatibility with the builder being used.

This led to cases in which the build failed for other reasons, and
Packer would still warn of potential incompatibilities, even if the
builder was in effect HCP compatible.

We attempted to fix this issue by introducing a new error type, and
checks when we read the artifacts linked to a build, however this loop
would fail when any one of the artifacts is not compatible with HCP
Packer, leading to false failures.

To avoid this problem, we log incompatibilities to the verbose logger,
and only signal the problem if all the artifacts could not be used to
upload data to HCP Packer, in which case it's almost certain that if the
build succeeded and no artifacts are registered to the build, that all
the components used are not compatible with HCP, and should be reported
as such to users.

internal/hcp/registry/types.bucket.go

@@ -643,26 +643,34 @@ func (bucket *Bucket) completeBuild(
 
 		state := art.State(packerSDKRegistry.ArtifactStateURI)
 		if state == nil {
-			return packerSDKArtifacts, &NotAHCPArtifactError{
-				fmt.Errorf("The HCP artifact returned by the builder is nil, this is likely because the builder does not support HCP Packer."),
-			}
+			log.Printf("[WARN] - artifact %q returned a nil value for the HCP state, ignoring", art.BuilderId())
+			continue
 		}
 
 		err = decoder.Decode(state)
 		if err != nil {
-			return packerSDKArtifacts, &NotAHCPArtifactError{
-				fmt.Errorf("Failed to obtain HCP Packer compliant artifact: %s", err),
-			}
+			log.Printf("[WARN] - artifact %q failed to be decoded to an HCP artifact, this is probably because it is not compatible: %s", art.BuilderId(), err)
+			continue
 		}
 
-		log.Printf("[TRACE] updating artifacts for build %q", buildName)
 		err = bucket.UpdateArtifactForBuild(buildName, sdkImages...)
-
 		if err != nil {
 			return packerSDKArtifacts, fmt.Errorf("failed to add artifact for %q: %s", buildName, err)
 		}
 	}
 
+	build, err := bucket.Version.Build(buildName)
+	if err != nil {
+		return packerSDKArtifacts, fmt.Errorf(
+			"failed to get build %q from version being built. This is a Packer bug.",
+			buildName)
+	}
+	if len(build.Artifacts) == 0 {
+		return packerSDKArtifacts, &NotAHCPArtifactError{
+			fmt.Errorf("No HCP Packer-compatible artifacts were found for the build"),
+		}
+	}
+
 	parErr := bucket.markBuildComplete(ctx, buildName)
 	if parErr != nil {
 		return packerSDKArtifacts, fmt.Errorf(

internal/hcp/registry/types.bucket_test.go

@@ -5,10 +5,15 @@ package registry
 
 import (
 	"context"
+	"reflect"
 	"strconv"
+	"sync"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/hashicorp/hcp-sdk-go/clients/cloud-packer-service/stable/2023-01-01/models"
+	"github.com/hashicorp/packer-plugin-sdk/packer"
+	"github.com/hashicorp/packer-plugin-sdk/packer/registry/image"
 	"github.com/hashicorp/packer/hcl2template"
 	hcpPackerAPI "github.com/hashicorp/packer/internal/hcp/api"
 )
@@ -385,3 +390,175 @@ func TestReadFromHCLBuildBlock(t *testing.T) {
 		})
 	}
 }
+
+type ArtifactHCP struct {
+	builderId func() string
+	files     func() []string
+	id        func() string
+	string    func() string
+	state     func(name string) interface{}
+	destroy   func() error
+}
+
+// NewArtifactHCP returns an artifact which complies to HCP artifact expectation
+//
+// i.e. the builder id is ok, and the state for hcp artifact isn't nil
+//
+// Note: this works, but may be changed by redefining the functions.
+func NewArtifactHCP() *ArtifactHCP {
+	return &ArtifactHCP{
+		builderId: func() string {
+			return "builder.test.hcp"
+		},
+		files: func() []string {
+			return []string{"one.file", "two.file"}
+		},
+		id: func() string {
+			return "HCPTest"
+		},
+		string: func() string {
+			return "dummy artifact for testing HCP"
+		},
+		state: func(name string) interface{} {
+			switch name {
+			case "builder.test.hcp":
+				return "OK"
+			case image.ArtifactStateURI:
+				return &image.Image{
+					ImageID:        "hcp-test",
+					ProviderName:   "none",
+					ProviderRegion: "none",
+					Labels:         map[string]string{},
+					SourceImageID:  "",
+				}
+			}
+			return nil
+		},
+		destroy: func() error {
+			return nil
+		},
+	}
+}
+
+func (ha ArtifactHCP) BuilderId() string {
+	return ha.builderId()
+}
+func (ha ArtifactHCP) Files() []string {
+	return ha.files()
+}
+func (ha ArtifactHCP) Id() string {
+	return ha.id()
+}
+func (ha ArtifactHCP) String() string {
+	return ha.string()
+}
+func (ha ArtifactHCP) State(name string) interface{} {
+	return ha.state(name)
+}
+func (ha ArtifactHCP) Destroy() error {
+	return ha.destroy()
+}
+
+func TestCompleteBuild(t *testing.T) {
+	hcpArtifact := NewArtifactHCP()
+	nonHCPArtifact := &packer.MockArtifact{
+		BuilderIdValue: "builder.test",
+		FilesValue:     []string{"file.one"},
+		IdValue:        "Test",
+		StateValues: map[string]interface{}{
+			"builder.test": "OK",
+		},
+		DestroyCalled: false,
+		StringValue:   "",
+	}
+
+	testCases := []struct {
+		name           string
+		artifactsToUse []packer.Artifact
+		expectError    bool
+		wantNotHCPErr  bool
+	}{
+		{
+			"OK - one artifact compatible with HCP",
+			[]packer.Artifact{
+				hcpArtifact,
+			},
+			false, false,
+		},
+		{
+			"Fail - no artifacts",
+			[]packer.Artifact{},
+			true, false,
+		},
+		{
+			"Fail - only non HCP compatible artifacts",
+			[]packer.Artifact{
+				nonHCPArtifact,
+			},
+			true, true,
+		},
+		{
+			"OK - one hcp artifact, one non hcp artifact (order matters)",
+			[]packer.Artifact{
+				hcpArtifact,
+				nonHCPArtifact,
+			},
+			false, false,
+		},
+		{
+			"OK - one non hcp artifact, one hcp artifact (order matters)",
+			[]packer.Artifact{
+				nonHCPArtifact,
+				hcpArtifact,
+			},
+			false, false,
+		},
+	}
+	mockCli := &hcpPackerAPI.Client{
+		Packer: hcpPackerAPI.NewMockPackerClientService(),
+	}
+
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			dummyBucket := &Bucket{
+				Name:        "test-bucket",
+				Description: "test",
+				Destination: "none",
+				RunningBuilds: map[string]chan struct{}{
+					// Need buffer with 1 cap so we can signal end of
+					// heartbeats in test, otherwise it'll block
+					"test-build": make(chan struct{}, 1),
+				},
+				Version: &Version{
+					ID:          "noneID",
+					Fingerprint: "TestFingerprint",
+					RunUUID:     "testuuid",
+					builds:      sync.Map{},
+				},
+				client: mockCli,
+			}
+
+			dummyBucket.Version.StoreBuild("test-build", &Build{
+				ID:            "test-build",
+				Platform:      "none",
+				ComponentType: "none",
+				RunUUID:       "testuuid",
+				Artifacts:     make(map[string]image.Image),
+				Status:        models.HashicorpCloudPacker20230101BuildStatusBUILDRUNNING,
+			})
+
+			_, err := dummyBucket.completeBuild(context.Background(), "test-build", tt.artifactsToUse, nil)
+			if err != nil != tt.expectError {
+				t.Errorf("expected %t error; got %t", tt.expectError, err != nil)
+				t.Logf("error was: %s", err)
+			}
+
+			if err != nil && tt.wantNotHCPErr {
+				_, ok := err.(*NotAHCPArtifactError)
+				if !ok {
+					t.Errorf("expected a NotAHCPArtifactError, got a %q", reflect.TypeOf(err).String())
+				}
+			}
+		})
+	}
+}