filesync: append rather than replace grpc md.

[sipsma]

Before this, CopyFileWriter just used metadata.NewOutgoingContext to set metadata, which results in any pre-existing metadata from the provided context to be removed.

Now, it gets the current metadata and then sets its own on top of that, so any pre-existing unrelated metadata is retained.

---

In Dagger we are integrating with buildkit at a somewhat deeper level and are making use of grpc metadata sent to client session attachables. There's a few other workarounds we can make if this change isn't desired for some reason, but overall just having buildkit retain unrelated grpc metadata during this call would be most convenient.

[jedevc]

Couple notes:

• There's a few other places we have this pattern as well, that if we do this here, we should also do in FSSync and maybe in sshforward.
• Some of the buildkit metadata keys are scoped (some with buildkit-, some with buildkit.) and some not all - like in FSSync. I'm slightly worried about accidentally clashing with these - buildkit would override in this case, but that could be confusing. Maybe we could error if a known name was already set by the caller?

No objections to this kind of change in general, but I am curious about what kind of metadata needs to be attached through the session mechanism in general 🤔

[sipsma]

There's a few other places we have this pattern as well, that if we do this here, we should also do in FSSync and maybe in sshforward.

Yeah good call, if we are good with the change here then I'll update the other spots too!

Some of the buildkit metadata keys are scoped (some with buildkit-, some with buildkit.) and some not all - like in FSSync. I'm slightly worried about accidentally clashing with these - buildkit would override in this case, but that could be confusing. Maybe we could error if a known name was already set by the caller?

For our use case, we purposely use our own x-dagger- prefix to prevent any collisions, but agree that for the general use case it would make sense to check if any pre-existing ones get overridden and either error out or print an error log.

I guess that just printing an error log in this case would be most backwards compatible as if someone for some reason was setting headers already that would collide the current behavior would be no error (since they are just ignored entirely atm), so changing that to be an error would be breaking. But that's also such an insanely obscure case maybe it doesn't matter. I'll update to print logs for now, let me know if anyone feels strongly about it being an error instead.

No objections to this kind of change in general, but I am curious about what kind of metadata needs to be attached through the session mechanism in general 🤔

Yeah so the dagger PR where this need arose is moving the implementation of Dagger's API to no longer be "client-side" but instead be in the "server-side" buildkitd process (technically, this isn't vanilla buildkitd but instead our own main func that imports buildkitd components as a library, very similar to what moby/moby does).

We have dagger APIs built on top of the standard buildkit session attachables already with some light extra logic on top to enforce certain constraints that can depend on the current state of the filesystem local to the dagger client. By moving our API implementation to no longer be local to the client, we can no longer enforce all of those constraints by just reading the local filesystem and instead need to enforce them in the clients' session attachables. This is very easy to implement by just passing a few extra pieces of metadata from the API request to those attachables, but in order to do that we need the grpc metadata to makes it way there, which worked out-of-the-box for local import attachables, but not local export attachables, hence the fix here.

[sipsma]

I meant to leave this comment a few days ago but got distracted:

I updated w/ the warning log in case metadata is gonna be overwritten somehow. For now, still only updating this one piece of the code since it's the only one impacting us, but if everyone's good with this change and would prefer squashing this everywhere, happy to also update the other parts of the code. Or I can just squash as they come up and leave as is for now, good with either.

[jedevc]

@tonistiigi wdyt? This likely has some conflicts with #2760, so since is a lot simpler, would be nice to get this in earlier.

[sipsma]

ping @tonistiigi