Extract registry wrappers #11967

lbajolet-hashicorp · 2022-08-31T19:09:46Z

To be able to track HCP builds during a Packer build, we need to propagate the name used for registering the build to HCP.

This works out-of-the-box for JSON templates, as the build's name is always the builder's, so there's no confusion on that.

For HCP templates however, there's a possibility for a template to define a name for the build, which is returned by a CoreBuild's Name() function, in addition to the source's Type/Name. The builds are registered to HCP with the source.String() function however, which does not contain the build's name. This prevents any build containing a name to work, as their name/source.String does not match.

The name we registered the build with is stored in a CoreBuild as the Type attribute, but cannot be safely accessed, as builds are types within the command as packersdk.Build, which only exposes Name() for this purpose.

In order to circumvent this problem, and as a way to present a prototype of solution that will likely have to be discussed before we merge it, this commit hotfixes the issue.

nywilken

Just working through the code and leaving questions as I step through. We can discuss in detail but please feel free to iterate on my initial questions/suggestions as you see fit.

nywilken · 2022-09-09T13:43:07Z

acctest/testing.go

@@ -146,7 +146,7 @@ func Test(t TestT, c TestCase) {
 		},
 		Template: tpl,
 	})
-	err = core.Initialize()
+	err = core.Initialize(packer.InitializeOptions{})


Why Packer an empty structure here? Maybe we can switch to functional args.

We can change this yes. We'd change the Handler interface to make the arguments a variadic suite of functions, and we can adapt this for both PackerConfig and Core, I like the idea.

If you don't mind, I think it can be done in a later PR maybe? I don't want to bloat this one with extra changes, there's already a lot going on, but we can open an issue to track this, and we can iterate on the solution after this one is in a satisfactory state?

nywilken · 2022-09-09T13:43:53Z

command/build.go

-	ArtifactMetadataPublisher, diags := packerStarter.ConfiguredArtifactMetadataPublisher()
-	if diags.HasErrors() {
-		return writeDiags(c.Ui, nil, diags)
+	hcpHandler, err := hcp.GetHCPHandler(packerStarter)


GetHandler(PackerHandler) returns the appropriate HCP Handler for the Packer config type?

Yes, exactly! If there's a HCP_PACKER_REGISTRY variable set, or a hcp_packer_registry block for HCL configs, we create the appropriate handler.
If none are set, we create a NoopHandler that does nothing. This ensures we can call methods later in the process and have them behave appropriately.

nywilken · 2022-09-09T13:45:52Z

command/build.go

-		return writeDiags(c.Ui, nil, diags)
+	hcpHandler, err := hcp.GetHCPHandler(packerStarter)
+	if err != nil {
+		c.Ui.Error(fmt.Sprintf("HCP setup failed: %s", err))


Let's change this to use hcl.Diagnostics as it can be used to provide addtional context and formatting.

nywilken · 2022-09-09T13:47:40Z

command/build.go

-		}
+	err = hcpHandler.PopulateIteration(buildCtx)
+	if err != nil {
+		c.Ui.Error(fmt.Sprintf("HCP populating iteration failed: %s", err))


Need to change this error messaging.

command/build.go

command/validate.go

nywilken · 2022-09-09T13:54:04Z

hcl2template/parser.go

@@ -137,6 +137,7 @@ func (p *Parser) Parse(filename string, varFiles []string, argVars map[string]st
 		Basedir:                 basedir,
 		Cwd:                     wd,
 		CorePackerVersionString: p.CorePackerVersionString,
+		HCPVars:                 map[string]cty.Value{},


What are HCPVars?

HCPVars is essentially how we set variables from the HCPHandler back into the configuration, for example now the iteration ID is propagated to the eval context, so it can be used in the configuration later on.

nywilken · 2022-09-09T13:56:06Z

hcl2template/types.packer_config.go

 		ectx.Variables[packerAccessor] = cty.ObjectVal(map[string]cty.Value{
 			"version":     cty.StringVal(cfg.CorePackerVersionString),
-			"iterationID": cty.StringVal(cfg.Bucket.Iteration.ID),
+			"iterationID": iterID,


Right now this is accessible as packer.iterationID in HCL. Should we change this to be scope to hcp or something else?

We could change this eventually, but this would be a breaking change if we moved it directly.
I guess we should probably first define it in both places, and show a warning stating this is deprecated, and will be removed later on.
I guess having an hcp accessor is relevant if we want to give access to more information that comes back from HCP later on, so we could probably do it.
I think it can be done in a later PR however

nywilken · 2022-09-09T13:57:17Z

hcp/common.go

+	ctx context.Context,
+	handler HCPHandler,
+) error {
+	err := handler.Bucket().Initialize(ctx)


We should probably ensure that handler is not nil.

We could ensure that, but since this is a private function, the only way this is called is inside of the package, and the callsite is from the handler themselves, so we should not reach such a case.
I think if that happens, it might as well be a crash that we should investigate and fix ASAP. We could replace the nil-ref panic by a more explicit one, but at that point I'm not sure it'd help too much, what do you think?

nywilken · 2022-09-09T13:58:23Z

hcp/common.go

+
+	err = handler.Bucket().PopulateIteration(ctx)
+	if err != nil {
+		return fmt.Errorf("HCP Packer Registry build initialization failed: %s", err)


nit: copy pasta on the error message. We want to think about the appropriate context to add for a user. I found the existing error messaging to be a bit terse.

Indeed, copy-pasta issue here.
I guess the HCP error could be returned directly, the context will be improved upon from the callsite anyway as it will end-up wrapped in an hcl.Diagnostic.

nywilken · 2022-09-09T14:32:06Z

hcp/common.go

+		if err != nil {
+			log.Printf("[ERROR] failed to update build %q status to FAILED: %s", buildName, err)
+		}
+		return artifacts, fmt.Errorf("build failed, not uploading artifacts")


What would cause a failure here?

If this is a communication hipcup should we try to dump the build artifact details for later use?

This error here is not a communication hiccup, it's that there was already an error with the build. I guess we could return nil,nil here, since nothing is updated, and the artifacts are not uploaded, my bad.

nywilken · 2022-09-09T14:33:56Z

hcp/common.go

+		return artifacts, fmt.Errorf("build failed, not uploading artifacts")
+	}
+
+	for _, art := range artifacts {


nit: artifact is a bit clearer then art.

nywilken · 2022-09-09T14:49:18Z

hcp/handler.go

+	"context"
+
+	sdkpacker "github.com/hashicorp/packer-plugin-sdk/packer"
+	"github.com/hashicorp/packer/hcl2template"


This lint error appears to be an issue with golangci-lint v1.42.0 < 1.48.0, which is bit using Go 1.17 and not Go1.18.
I am able to reproduce locally. Updating to 1.48.0 does not report an issue. I suspect that it may be do to some of the Go dependencies.

True, the PR I uploaded before that bumps the golint-ci should take care of this one here.

nywilken · 2022-09-09T14:50:25Z

hcp/handler.go

+	BuildDone(ctx context.Context, buildName string, artifacts []sdkpacker.Artifact, buildErr error) ([]sdkpacker.Artifact, error)
+}
+
+// TrySetupHCP attempts to setup the HCP-related structures if HCP is enabled


Comment needs to be updated.

Suggested change

// TrySetupHCP attempts to setup the HCP-related structures if HCP is enabled

// GetHCPHandler .....

nywilken · 2022-09-09T14:51:55Z

hcp/hcl_handler.go

-	case *CoreWrapper:
-		return setupRegistryForPackerCore(cfg)
-	}
+type HCLHandler struct {


Guessing this is meant to be HCPHandler and not HCLHandler. Which is the same name as the HCPHandler interface so that wont work. Let think about this a bit more to see what to call it. We probably want to rename the HCPHandler interface as well.

In this case it's intended to be HCLHandler since it handles HCL templates, but the name is confusing I agree, maybe we can see to replace it by something else.
I should probably add some comment on top to clarify why it's here though, the whole package is poorly documented for now, so I'll amend that for my next reroll.

In order to determine whether or not a build is done or needs processing, we check 3 things: 1. that its status is not done 2. that its ID is not empty 3. that it has no images linked to it The was the image check was phrased indicated that we checked for the reverse, so we reverse the naming to make it less confusing.

To be able to track HCP builds during a Packer build, we need to propagate the name used for registering the build to HCP. This works out-of-the-box for JSON templates, as the build's name is always the builder's, so there's no confusion on that. For HCP templates however, there's a possibility for a template to define a name for the build, which is returned by a CoreBuild's Name() function, in addition to the source's Type/Name. The builds are registered to HCP with the source.String() function however, which does not contain the build's name. This prevents any build containing a name to work, as their name/source.String does not match. The name we registered the build with is stored in a CoreBuild as the Type attribute, but cannot be safely accessed, as builds are types within the command as packersdk.Build, which only exposes Name() for this purpose. In order to circumvent this problem, and as a way to present a prototype of solution that will likely have to be discussed before we merge it, this commit hotfixes the issue.

As part of the work to displace everything related to HCP from scattered places around the Packer code, we move it all to an hcp package. This in turn reduces the amount of code that the commands have to integrate, and leaves the HCP details to its own enclave.

nywilken

The reroll looks good to me. I think there are a few naming changes that we discussed that need to be implemented, and a few comments that might still being referring to handler for the registry. But otherwise I good with it. When ready merge and let’s check if the artifact bug is resolved as well. Approving in advance.

nywilken · 2022-09-21T14:54:09Z

command/build.go

+	if err != nil {
+		return writeDiags(c.Ui, nil, hcl.Diagnostics{
+			&hcl.Diagnostic{
+				Summary:  "HCP: initial setup failed",


Let's update the messaging in the error to include call to action.

This was already updated on the latest reroll, now the diag comes from registry.New and states that an unknown config type was provided, or in the related New.*Registry functions, we get a more accurate diag(s) on the error we got.

I think this was a comment from a review I never submitted. Please disregard.

nywilken · 2022-09-21T15:11:21Z

command/build.go

+				if errors.As(err, &hcp.BuildDone{}) {
+					ui.Say(fmt.Sprintf(
+						"skipping HCP-enabled build %q: already done.",
+						name))
+					return
+				}
+			}


Nice! To not confuse BuildDone{} with BuildDone() I suggest calling this error what it is BuildDoneErr.

Suggested change

if errors.As(err, &hcp.BuildDone{}) {

ui.Say(fmt.Sprintf(

"skipping HCP-enabled build %q: already done.",

name))

return

}

}

if errors.As(err, &hcp.BuildDoneErr{}) {

ui.Say(fmt.Sprintf(

"skipping HCP-enabled build %q: already done.",

name))

return

}

}

I have taken upon your suggestion and renamed it ErrBuildAlreadyDone in the upcoming reroll

command/build.go

nywilken · 2022-11-02T17:54:00Z

command/build.go

@@ -88,38 +90,24 @@ func (c *BuildCommand) RunContext(buildCtx context.Context, cla *BuildArgs) int
 		return ret
 	}

-	diags = TrySetupHCP(packerStarter)
+	hcpHandler, diags := hcp.GetOrchestrator(packerStarter)


What do we call this?

internal/hcp/orchestrator.go

nywilken · 2022-11-14T11:57:05Z

internal/hcp/registry/null_registry.go

+type nullRegistry struct{}
+
+// We don't need a constructor for such a simple type
+//func newNullHandler() Registry {


We can remove this.

nywilken · 2022-11-14T11:58:53Z

internal/hcp/registry/registry.go

+	//Configure(packer.Handler)
+	PopulateIteration(context.Context) error
+	BuildStart(context.Context, string) error
+	BuildDone(ctx context.Context, buildName string, artifacts []sdkpacker.Artifact, buildErr error) ([]sdkpacker.Artifact, error)


Let’s rename these methods as we discussed StartBuild and CompleteBuild

nywilken · 2022-11-14T15:47:02Z

internal/hcp/registry/types.bucket.go

@@ -469,3 +495,115 @@ func (b *Bucket) HeartbeatBuild(ctx context.Context, build string) (func(), erro
 		close(heartbeatChan)
 	}, nil
 }
+
+func (b *Bucket) StartBuild(ctx context.Context, buildName string) error {


With these method names does the Bucket type also implement the Refistry interface?

if it does and that is not intentional we should probably give these different names. Maybe something like MarkBuildAsStarted etc …

Well, that's true, as it stands Bucket is an implementation of registry.
This has the side-effect of having to rename the current CompleteBuild to completeBuild, as it does the update of the status to DONE and uploading the labels and other metadata. This can indeed be renamed in Bucket so it's not an implementation of the Registry interface.
I'll do another reroll later for that

Note: since the two are in the same package, we can make the Bucket functions private, so it's not an implementation anymore. This removes the potential confusion when looking for the Registry implementations.

Sounds good to me. 🚢

This commit reorganises the code for both the registry/API and the Orchestrator/Registry. The main difference with the previous version is how stuff is exposed. Now we only expose a Registry interface to the outside (previously named Orchestrator), which has several implementations: null is the default, and is returned if HCP is not enabled. The other implementations being HCL/JSON, both private to the hcp sub-package. The api (previously `registry') is the set of functionality that abstracts and calls the HCP API. It was meant to be merged with the `hcp' package, but because of a dependency loop with the datasources, both are separated for now.

The ancestry inferral code relied on HCP datasource outputs for deciding what would be the ancestry for an image built with HCP. This brings a dependency into the hcp package, which is not really necessary as we only need a few information from those entities, hence this commit removes the full dependency on the structures, in favour of more focused data structures where we only cherry pick what is necessary for this code.

github-actions · 2022-12-15T02:15:25Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

lbajolet-hashicorp requested a review from a team as a code owner August 31, 2022 19:09

lbajolet-hashicorp force-pushed the extract_registry_wrappers branch 4 times, most recently from b519b12 to dc62051 Compare September 7, 2022 20:59

nywilken reviewed Sep 9, 2022

View reviewed changes

lbajolet-hashicorp force-pushed the extract_registry_wrappers branch 4 times, most recently from ae1eabc to d2be583 Compare September 13, 2022 21:56

lbajolet-hashicorp mentioned this pull request Sep 14, 2022

hcp: use HCP_PACKER_REGISTRY to disable HCP #11994

Merged

lbajolet-hashicorp force-pushed the extract_registry_wrappers branch from d2be583 to 4272d9d Compare September 19, 2022 20:49

lbajolet-hashicorp force-pushed the extract_registry_wrappers branch from 4272d9d to 777e667 Compare September 28, 2022 13:26

lbajolet-hashicorp force-pushed the extract_registry_wrappers branch 3 times, most recently from f33947b to 0a80100 Compare October 25, 2022 19:52

nywilken added this to the 1.8.5 milestone Oct 28, 2022

lbajolet-hashicorp added 4 commits November 7, 2022 17:33

tmp: move HCP logic to unwrap builds/post

4bb72b5

lbajolet-hashicorp force-pushed the extract_registry_wrappers branch 2 times, most recently from 2220846 to da693f0 Compare November 7, 2022 22:37

lbajolet-hashicorp mentioned this pull request Nov 9, 2022

hcp: remove superfluous return value on GetBuilds #12102

Merged

lbajolet-hashicorp force-pushed the extract_registry_wrappers branch 5 times, most recently from 622d76e to ccb1f20 Compare November 10, 2022 15:37

lbajolet-hashicorp force-pushed the extract_registry_wrappers branch from ccb1f20 to d545b24 Compare November 10, 2022 20:45

nywilken approved these changes Nov 14, 2022

View reviewed changes

lbajolet-hashicorp force-pushed the extract_registry_wrappers branch from d545b24 to 3ce7e0b Compare November 14, 2022 15:37

nywilken reviewed Nov 14, 2022

View reviewed changes

lbajolet-hashicorp force-pushed the extract_registry_wrappers branch from 3ce7e0b to c074808 Compare November 14, 2022 16:09

nywilken and others added 2 commits November 14, 2022 11:25

lbajolet-hashicorp force-pushed the extract_registry_wrappers branch from c074808 to 207e099 Compare November 14, 2022 16:25

lbajolet-hashicorp merged commit 719c868 into main Nov 14, 2022

lbajolet-hashicorp deleted the extract_registry_wrappers branch November 14, 2022 18:31

nywilken mentioned this pull request Dec 1, 2022

Input Artifacts with keep_input_artifact=false shouldn't be sent to HCP Packer #11462

Closed

github-actions bot locked as resolved and limited conversation to collaborators Dec 15, 2022

	// TrySetupHCP attempts to setup the HCP-related structures if HCP is enabled
	// GetHCPHandler .....

Extract registry wrappers #11967

Extract registry wrappers #11967

Conversation

lbajolet-hashicorp commented Aug 31, 2022

nywilken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbajolet-hashicorp Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nywilken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 15, 2022

lbajolet-hashicorp Sep 12, 2022 •

edited

Loading