Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Finish isolating sessions to each client #6916

Closed
wants to merge 4 commits into from

Conversation

sipsma
Copy link
Contributor

@sipsma sipsma commented Mar 21, 2024

This is the last part of the saga around #6747 that fully isolates sessions to each client and thus enables support for passing host unix sockets to modules, plugging some existing holes in our secret isolation, etc.

This PR doesn't have all that yet, been in the process of rebuilding it cleanly after an initial messy prototype. Sending it out anyways because what I have here seems to fix the problem in #6914, so may want to merge a minimum safe changeset here early.

  • The only extra thing strictly needed to safely merge is isolating the secret store to each client; in the current state of things nested execs end up with a shared secret store from their parent caller, which is probably a breaking change (though it's complicated, not 100% sure)
  • Brief description of changes so far:
    • Consolidate module caller digest to just be the client ID. This is a pretty nice simplification from before since we less IDs to think about and pass around, but especially simplifies the rest of the pending changes
    • Share the server with nested execs, whereas previously they got their own. This is again just a generally nice simplification made possible by the fact that we can now isolate their session attachables.

@vito unfortunately these are the changes I was referring to that may cause rebase headaches with your OTEL PR (and there's more coming on top of the commits already here), so we'll have to sort that out if we merge this soon.

Signed-off-by: Erik Sipsma <erik@dagger.io>
Signed-off-by: Erik Sipsma <erik@dagger.io>
@@ -28,11 +28,12 @@ const (
)

type ClientMetadata struct {
// ClientID is unique to every session created by every client
// ClientID is unique to each client. The main client's ID is the empty string,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: fix this doc string; the client ID is now what used to be ModuleCallerDigest, so it's actually a non-unique content hash for all clients other than "main callers" from the host.

Signed-off-by: Erik Sipsma <erik@dagger.io>
Signed-off-by: Erik Sipsma <erik@dagger.io>
@sipsma sipsma mentioned this pull request Apr 4, 2024
13 tasks

This comment was marked as outdated.

This comment was marked as outdated.

@sipsma
Copy link
Contributor Author

sipsma commented Apr 30, 2024

Just to update, I extracted a bit of this out to #7213 in order to get bug fixes in faster. Also been chipping away locally at a re-work of this PR locally as I got thing like 95% working but then hit a corner case that called for a different approach.

I'm trying to do it in such a way that it cleans up as many related things as possible since we accumulated quite a bit of cruft over the last 6ish months adding module support. The biggest things that I may end up incorporating here are:

  1. Consolidate BuildkitController and DaggerServer into one thing
    • Even though the goal of this PR is to isolate client sessions more, we currently have DaggerServer, dagql.Server, Query, BuildkitClient and more things that all need to be isolated, which can easily result in profound convolution. So removing entities from that list by making them shared ends up simplifying quite a bit
  2. Removing ftp_proxy hack and instead taking advantage of the new custom Executor added recently
    • This should help simplify the way sessions get setup and served to modules/nested-execs which is obviously tied into the changes here

Obviously don't want the scope of this to grow out of control either, so seeing what makes sense to do here still and what can be follow ups.

Copy link
Contributor

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the stale label May 15, 2024
Copy link
Contributor

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants