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.

From what I can understand:

1. Release flow requests to sign something with a given OIDC identity through Cosign.
2. Cosign requests a signing certificate from Fulcio based on this OIDC identity.
3. Fulcio then verifies the identity, generates and signs the short-lived cert/pub-key pair for Cosign.
4. Fulcio delivers the cert/pub-key pair to Cosign
5. Cosign then uses the pub-key to sign the released binaries.
6. Cosign stores in Rekor: the cert, the pub-key, the sha of the binary that was signed.

Now, the verification happens:

1. Supervisor sees a binary with a certificate, a signature, and it says "I want to verify that this package was built by this (could be an email address), at , and the issuer of the identity is ".
2. Fulcio has a record of which certificates were issued to who and when... it is a CA after all.
3. Supervisor verifies that the pub key from that signing cert generated the provided signature of the binary.
4. Supervisor verifies with Fulcio's root cert that the signing certificate it got is valid and came from Fulcio.
5. But who can guarantee to the Supervisor that the signing certificate from Fulcio was really signed by that OIDC identity? What if somebody other than Cosign requested that cert directly to Fulcio? Did they really verify control of the OIDC identity? That's why Rekor exists. It's basically a WAL of what Cosign did. So the Supervisor can then verify that record that Cosign created by double-checking in it: the sha of the binary, the signature, and the public key.

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?

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

Yea

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

It's the executable, so renaming to agentExePath

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

This isn't strictly specified anywhere, but I think the idea is the releases repo where the signatures and binaries are stored have all collector binaries available as gzip artifacts, ignoring the different package installations that are posted there too.

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

Similar to the gzip thing, this is based on the way the releases repo formats the collector releases. The collector binary in the gzip is titled "otelcol-contrib".

I can add documentation around these things. What do you think about making this part name configurable?

We are already making a big enough assumption that packages will be always in a gzip archive, so I think we can also assume the archive should contain only the Collector binary and nothing else, so we don't need to care about the name?

Ah, my proposal is only valid for updates of the top-level package. 😅

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.

Sounds good, I'll implement this change

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.

Implemented this similar to the process of backing up

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.

I added some comments to try and reduce the confusion. Technically they are different. The AllPackagesHash is a combination of all the hashes of the packages this package manager manages. topLevelHash is the hash for the collector package. Technically they are different, but right now in practice they should be the same because we're only managing the collector package.

I think this select case could result in a hung supervisor in some circumstances.

When commander.Stop is called, it blocks until the agent process has terminated. If the agent process does not terminate promptly, the supervisor will wait a little bit of time before issuing SIGKILL to the agent process. However, whether or not the process actually reliably terminates after this SIGKILL is OS-dependent and situation-dependent.

It's a rare circumstance, but processes can get into states where they don't respond to SIGKILL. For a Linux-specific example of one way this can happen: https://chrisdown.name/2024/02/05/reliably-creating-d-state-processes-on-demand.html

When systemd encounters a process that doesn't terminate on SIGKILL after a timeout, it marks the unit as failed and moves on with its other duties. However it's not completely clear what the opamp supervisor should do, since it only supervises one thing. My instinct is that it should probably crash after a further timeout.

In any case, I think that this condition should be detected by the supervisor and handled in some way, perhaps in the commander package for the sake of abstraction.

Before removing the backup, consider starting/health-checking the agent. We might also want to keep the backup instead of deleting it immediately -- e.g., retain it for a grace period (TTL) and delete only after the agent has been healthy for N minutes or after the next successful update, so we preserve a quick rollback path if an intermittent/latent bug surfaces.

Great point, @OrangeFlag. I'm +1 to this.