This should probably be the version/hash of the Collector binary, right? I think the version can be the one obtained during bootstrapping.

Yeah, I think that makes sense. I think the mental hurdle I'm having here is the sort of disconnect between the artifact the agent needs to end up with and what is available from the releases.

Because the release artifact is a tarball, but the agent executable is not, it means that the artifact offered by the server and the one the agent ends up with have completely different hashes. This initial status is kind of a weird edge case.

Could we link to an issue here? It would make it easier to track and provides some context.

I think this is okay given that it's the only other usage of this and is a very simple call to filepath.Join.

The other options I can think of aren't worth the additional complexity:

1. Extract the packagesStatusPath functionality to a function and call the function in the method as well. Too much abstraction for a simple call.
2. Instantiate packageManager above and call pathagesStatusPath here, then fill in other details on the struct afterward. Splits the initialization too much for fairly minimal gains.

Would it make sense to instead use the persistent state file to store this? I see how the purpose of each file is distinct, but having two small-ish YAML files also feels a little odd.

This may also be premature, but I think abstracting state storage through the persistent state struct may keep things cleaner in the long run.

I think this is another place where consolidating our storage to the persistent state may make things cleaner. We have two different data flows for the instance ID vs. the agent version in this function, when I think we should probably store the two next two each other so all of the agent's information is recorded in the same place. What do you think?

Can you clarify on what's being proposed here? I don't think we should store the agent version anywhere, we should always treat the reported version from the agent as the source of truth (as best we can, anyways).

Do you think it would substantially complicate things if we limited agent process management to the Supervisor struct and only deal with managing the binary file here? My first impression is that we should keep those responsibilities separated if possible and not have the package manager know anything about the Collector process, even if the current implementation isn't too complicated.

My thought was that we need to stop the agent process to write the binary in the first place.

We could write the binary somewhere temporarily, and send a message to the agent goroutine to do the replacement, which could be cleaner (better separation of concerns). The package manager would still need to have some handle to signal that, but it wouldn't necessarily be tied to the collector.

Would we be able to accomplish this with synchronous, one-way communication? I was thinking maybe we could do something vaguely like this in the Supervisor struct:

oldVersion := 0
newVersion := 1

s.commander.stopAgent()
s.packageManager.SwapInVersion(newVersion)

if err := s.commander.startAgent(); err != nil {
	s.packageManager.SwapInVersion(oldVersion)
	s.commander.startAgent()
	s.reportFailedInstallation()
	return
}

s.packageManager.DeleteVersion(oldVersion)
s.reportSuccessfulInstallation()

Yeah, I think we could do something like that. I think this also sets us up nicely for restoring the previous version if the new one fails. I'll look into making those changes.

Alright, so I'm looking a bit deeper and the reason I'm a little iffy on this is because it seems like there will be some odd consequences with doing this outside of the package syncer.

Essentially, the package syncer will run it's logic, then it will report the status off every package at the end. So it's possible that the package downloads and verifies fine, so the package syncer reports the package as installed - but then there is an error when swapping in the version, which causes an error.

I would prefer if we didn't report the package is installed before it's actually successfully installed, basically. Which means we'd have to do it in the package manager during the UpdateContent call, as far as I can tell. This would require 2 way communication, one way from (packagemanager) -> (agent goroutine) to do the swap, then one from (agent goroutine) -> (package manager) to indicate success/failure.

If PackageSyncer's design and API makes this difficult we should feel free to redesign it. Please don't feel constrained by it. This was the most basic design I could come up with when implementing the example and there may very well be a better design. Feel free to suggest one.

From what I'm seeing, the Done channel only communicates that the PackageSyncer is done communicating package statuses to the server, and doesn't track whether the packages have been downloaded: https://github.com/open-telemetry/opamp-go/blob/main/client/internal/packagessyncer.go#L51. It may be worth waiting for this before communicating any additional updates about package statuses to the server.

I'm actually really confused reading that code. It looks like once Sync is done, that channel is closed, so waiting on Done here wouldn't actually add any extra synchronization.

Good call at looking at that impl, I definitely had a different idea of its purpose in my head.

Please document AgentSignature and AgentSignatureIdentity.

Incomplete comment?

Is this signature format documented somewhere? This imposes a requirement on the OpAMP server to generate signatures in a specific way, right?

Excellent diagram and workflow description! Thank you. This is very heplful.

Can you clarify what is the difference between "verifies the certificate via the root certificates" and checks against the Rekor transparency log to verify the certificate hasn't been tampered with". I assume certificate is signed by a root certificate (the CA), so if it was tampered it would not pass the first verification step. How is possible that it passes the first check but fails the second? Are we thinking about a scenario that the root certificate is tampered with?

I'm actually a little iffy on this myself. I think the transparency log is related to the time of signing in some way, I'll dig a little more to see if I can understand better.

It would be interesting to explore the possibility of offline certificate verification. We could embed the root certificates in the Supervisor at build time, but I don't know if the verification process itself can be done without connecting to Rekor.
This can be a future work.

there is an option to skip the Rekor/transparency log verification. I couldn't get it to work as I was developing, but I figure it's probably something simple I was missing. We should definitely be able to get it working in some follow-up.

Wrong link?

Would it make sense to embed (//go:embed) the root certificate into the executable? How often is the root certificate updated and how long is it valid for? Embedding could simplify things for the user (one less thing to obtain and supply via config) if it is valid for long enough.

What is the reason that these need to be configurable? Is it so that non-official Collector builds can be supported?

Nit: .proto extension is normally used for Protobuf source code (IDL) files. .binpb is recommended extension for wire format: https://protobuf.dev/programming-guides/techniques/#suffixes

Is this the executable file path or a directory path where executable is contained? Perhaps agentExePath or agentFilePath to make it clear?

Do we know the content is gzip? Is this specified somewhere?

How do we know the part name is "otelcol-contrib"? It would nice to document this and the fact that it is gziped tarball.

A possible alternate instead of copying the files is to rename them. Delete backup, rename original to backup, write to the original name. Skips the copying and is likely faster.

Do we need to keep the backup file or we can rename instead of copying? Renaming likely is less susceptible to out-of-disk-space issues.

It is a bit confusing that we have both AllPackagesHash and topLevelHash. Are they the same or different? If different it would be nice to explain in a comment somewhere.