#Private packages in modules 🧵

1 messages · Page 1 of 1 (latest)

vapid niche
exotic mulch
#

hm, i actually don't think that 9007 is actually the solution here

#

we'd implement 8809 in a similar way to how the git socket forwarding works i think

#

essentially, having a buildkit session attachable of some variety that can load git information from the client

#

one of the methods on that would fetch the gitconfig

vapid niche
exotic mulch
#

(although probably making the file a secret?)

vapid niche
exotic mulch
#

imo, the session config is more a mechanism to load config into a single dagger session for things that don't actually require a full engine reload - e.g. changing mirrors, etc.

wooden topaz
#

How do we determine where to draw the line regarding which configurations should be restricted to a single Dagger session—such as a mirror—and which should be treated as more permanent or engine-wide settings, like .gitconfig ?

exotic mulch
#

Anything that can be session wide should be

#

Engine wide settings are more difficult to share, harder to update, etc

#

So if there's a way to implement session wide settings for a setting, we should do it imo

wooden topaz
#

Mmmh, still a bit confused:

  • why do we rely on automatic mounting of the .gitconfig
  • and not have it manually specified in the session config file

And what's the heuristic to classify them ?

exotic mulch
#

Ah good question

#

Because there's no session config yet

#

We have to go and invent it

#

But also, we already automount the git socket right?

#

Makes sense to make git "just work" if we can

silk kestrel
#

I have been looking at this issue, and my understanding so far is that this fails in modulesource.go -> func moduleSource when getting commit info.

#

in there we do a select operation on query git, and the handler for that select does support using auth socket if provided.

I am trying to see if we can pass along the AuthSocket somehow in this call

#

if for some reason you think that is not possible or a bad idea, pls let me know.

silk kestrel
#

I am wondering if we should only allow specific keys to be imported from git config instead of everything

silk kestrel
#

some notes:

  • if SSH_AUTH_SOCK is set, cloning the private module git repo works
  • for downloading the private dependencies, we need changes to codegen container
  • mount ssh auth socket in codegen container and set SSH_AUTH_SOCK env variable. This was straightforward once I figured where this needed to be set.
  • set GOPRIVATE env variable - probably need to hook as input to engine somehow?
  • create a /root/.gitconfig to ask git to use ssh instead of https. (probably need to configure for all well known git repos or somehow use wildcard host)
silk kestrel
#

(we do need some of those other changes, but once we have that config, we can have this working).

#

populating the complate .gitconfig from user's profile seems a little too much as this may bring in user specific settings, which we may or may not need

#

one workaround for that could be to "safelist" the keys that we will import from git config.

#

one other thing that may complicate bringing in the .gitconfig is that which .gitconfig should we pull? user-specific, system-specific or local or effective .gitconfig?

#

what do you think?

wooden topaz
silk kestrel
#

me and Marcos just had a sync. here is what I will be doing:

  • implement an attachable that gets "effective gitconfig of the user"
  • that attachable will read only a "safelisted" set of keys to start with (this is still under consideration to avoid breaking something else by importing some gitconfig that works only on user's system)
  • find all the "replace https with ssh" entries. find all the hostnames
  • automatically create a GOPRIVATE env variable with those hostnames entries
  • and after the ^^ we should have this working.
exotic mulch
#

sounds good, bit of confusion on this point:

find all the "replace https with ssh" entries. find all the hostnames

#

go does support HTTPS credentials

#

should we not use the same HTTPS authentication methods we have in https://docs.dagger.io/api/remote-modules/
even if we don't support this right away we will need to in the future. we had demand for github PATs almost immediately after adding private SSH support, so no reason to think we can avoid it here

#

implement an attachable that gets "effective gitconfig of the user"
we should also avoid the creation of a new attachable here imo, we should extend the gitcredential.proto, and group all the git credentials stuff into one attachable - mostly to make it more consistent and readable

#

automatically create a GOPRIVATE env variable with those hostnames entries
bit split on whether this should be magic actually. even the go command itself doesn't do this, and requires GOPRIVATE to be set.
in that case, maybe a more sensible approach might be to require that users explicitly put their GOPRIVATE env variable somewhere in their dagger.json config. users with private modules should already be used to configuring this themselves, so i don't think this is a big imposition.

silk kestrel
#

and saw your comments above ^^ @exotic mulch. I will update GitCredential to also allow fetching git config 👍

#

can we rename that attachable to just "Git" (or GitSvc) instead of GitCredential? as its not user facing, so i think that should be ok

exotic mulch
#

🤔 yeahhhh renaming sounds good to me, it's potentially breaking for old clients to old engines, but personally, i don't generally think we should spend lots of effort on that particular code path, so rename is good!

#

i also want to have a clone method in that protocol one day 🙂

#

Git sgtm

silk kestrel
#

about goprivate env variable - do we want to expose that in dagger.json? I usually don't like magic (e.g. guessing GOPRIVATE env from git config), but do you think we can just use that from user's env variable where they are running the dagger call command?

#

if we do want to get from dagger.json, then do we want to specify a generic env variable provider or a more strict GOPRIVATE env variable (or equivalent in other languages)?

exotic mulch
#

🤔 good point

#

i think i'd rather avoid pulling from the user env where possible - it means that there's less of a copy-paste command for invoking (especially remote) modules

#

i think instead of having just GOPRIVATE in the dagger.json, you'd want to have something closer to a buildenvs map?

#

not really sure on whether this is the right idea still

#

i think we'd have the same problem if we wanted private npm or pip packages right?

silk kestrel
#

what could be super cool is to allow user to say "use this as base image to compile/run my module code" with the dependencies/env they need and then we add stuff we need on top of that

exotic mulch
#

i guess with those, the package.json and pyproject.toml are designed to be extensible - go.mod isn't

exotic mulch
#

i don't really like that as a solution to the credentials problem we're discussing, but i like it better than having the magic inference which feels kinda fragile

#

having the ability to set those kind of sdk-specific parameters does feel super useful though (outside of this discussion) - e.g. being able to set custom build + linker flags for richer debug info, etc.

silk kestrel
#

so I have one action item to merge GitConfig attachable with existing GitCredential one, but I still do not have a clear path for specifying GOPRIVATE env.

@vapid niche what do you think?

vapid niche
# silk kestrel so I have one action item to merge GitConfig attachable with existing GitCredent...

@exotic mulch my main argument to make it "magical" initially for Go at least (the requirement of GOPRIVATE) is because I think there's potentially a bigger and more long and difficult decision to take around contexual call envs / options. Currently we have the _DAGGER_ENGINE_SYSTEMENV thing which doesn't fit this use-case as the GOPRIVATE variables could potentially change from project to project.

We could do many things here but what we spoke with Rajat in the context of Go private modules is that if the .gitconfig contains a replace entry, it's very likely then that you have a GOPRIVATE variable set in order to make that config effective. So our first "straightforward" approach was to set the GOPRIVATE env var automatically for the Go SDK.

#

I guess the question is: Do we want to open the build specific flags / envs can of worms now?

#

I'd personally prefer to defer that decision for a longer issue/pr and ship this which is already blocking some users

exotic mulch
#

I guess the question is: Do we want to open the build specific flags / envs can of worms now?
bleh, i think there's two cans of worms here:

  • how we pass caller side env variables (we can continue to defer this)
  • how the user can configure env variables per-module instead of per-engine - i think this might be worth unboxing now - we need to do it at some point anyways right?

if we decide to take the magic approach, it's challenging to undo that later - it's always possible to add magic, but removing makes life a lot harder

#

i think my proposal (at it's most generic is):

  • expand the sdk value in dagger.json to an object (we need this anyways https://github.com/dagger/dagger/issues/9156)
  • add an sdk.env map config field to set sdk environment variables - each sdk can choose where and how to inject these (and may choose to allow/deny-list these)
  • this allows setting GOPRIVATE in dagger.json
#

i think requiring manually encoding GOPRIVATE in dagger.json makes sense right? there's never a case where it's user specific, where a dependency might be private for one user but not for another? it would always be private.

this also just allows it to "just work" with reasonable errors when calling a remote module - if we do magic auto-detection, we would get weird goproxy errors if the user hasn't configured .gitconfig properly. but with manual GOPRIVATE set, we can get the actual git clone error, which is much more informative.

vapid niche
#

shall we also add an env key in dagger.json which applies for all containers created by the engine? 🤔

exotic mulch
#

hm that's a good point actually, i think we could maybe come back to that - i don't think it should be automatic, but we could maybe have a dag.CurrentModule().Env() to get the env vars from the config? not sure, i don't think you'd always need to do it automatically, it's just some cases where the config might need to be in two places

vapid niche
#

@exotic mulch so you still think we should just start with the sdk > env setting and go from there without any other "magical" / comprehensive config?

exotic mulch
#

imo yes - but maybe worth also getting some other folks to weigh in

vapid niche
silk kestrel
#

SGTM to me as well. I can prototype this now.

#

and report back if I hit any other edge case when doing that.

vapid niche
#

so we can send it to the team and ask for feedback 🙏

silk kestrel
#

yep, on it right now. (almost done with the changes to merge new api in existing attachable, running into some issue that i am trying to debug)

#

also, do we want to safelist the keys of gitconfig that we want to import or we can import everything for now?

exotic mulch
silk kestrel
#

🙏

silk kestrel
#

one thing I realized when cleaning up the code changes for this:

git does not allow export/import of git config. I didn't catch this earlier because my explicit git config <use-ssh-instead-of-https> code change remained in the code while I was iterating over other changes.

so what I am doing right now is to get git config using git config -l, which gives me output in key=value format.

i am then parsing that output to create a shell script with git config <key> <value> commands for each of the config key and running that.

I still need to verify if this works fine, but it almost feels like fetching complete git config may not be a good idea. e.g. look at this git config which is fetched when running dagger from inside my private repository:

credential.helper=osxkeychain
init.defaultbranch=main
user.name=Rajat Jindal
user.email=rajatjindal83@gmail.com
commit.gpgsign=true
url.ssh://git@github.com/.insteadof=https://github.com/
core.excludesfile=~/.config/git/.gitignore
protocol.file.allow=always
core.repositoryformatversion=0
core.filemode=true
core.bare=false
core.logallrefupdates=true
core.ignorecase=true
core.precomposeunicode=true
remote.origin.url=git@github.com:rajatjindal/dagger-private-module.git
remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
branch.main.remote=origin
branch.main.merge=refs/heads/main
http.proxy=http://proxyUsername:proxyPassword@proxy.server.com:port
url.git@github.com:.insteadof=https://github.com/
#

I am going to call it a day (feeling quite tired for some reason), will check this out tomorrow morning. Please let me know what you think about ^^

exotic mulch
exotic mulch
#

there's some stuff in there that might only be applicable on the host as well, so isn't a good idea to pass along

#

i think we might just want a protocol method for urlInfo (naming hard) that returns a typed struct of all the fields in there

#

...alternatively. we create an allowlist for what's passed through.

silk kestrel
#

I think allowlist might be the safest bet. its easy to extend. but if we allow everything, then its hard to go back on that

exotic mulch
#

agreed 👍

#

let's go with allowlist approach for now then

exotic mulch
#

if we try and do PAT support for private go modules (we should)

#

then we will need to implement a little git credential helper proxy that proxies git credential requests back to the user

#

(i think)

#

⚠️ but shouldn't be a distraction for now

vapid niche
#

because currently it can be a system level config via .netrc or .gitconfig via credential helpers and/or hardcoded values

#

I'd assume that we want to go all the way with git cred helpers, right?

exotic mulch
#

yeee good point - ideally we would support them all 😛 but yes, since we already do cred helpers for modules, we should support that for private go imports

vapid niche
#

@exotic mulch is there an estimate for 15.2? Would love this to land there 🙏

#

we also have the services fix which is ready for 👀

exotic mulch
#

no estimate right now, but imo we should go for early next week, then get back into the swing of things with weekly releases

#

not sure about this landing in the timeframe for v0.15.2, but we'll see

#

investigating services fix now

silk kestrel
#

Hi @exotic mulch, how do you anticipate the sdk.env to look like in dagger.json. do you recommend replacing sdk (string) to struct or have an additional field, something like sdkConfig while keeping sdk as such?

maybe what we can do is support reading that field as both string and struct and when saving dagger.json, always save it as struct?

exotic mulch
#

potentially this change could be made as a separate pr first? depends on how you want to structure it

#

but yes, essentially dagger develop should always write back this struct

silk kestrel
#

hmm, let me check, but for the user to be able to specify sdk.env in dagger.json, we need to also update the schema for dagger.json isn't it?

exotic mulch
#

yup

silk kestrel
#

an update: the gitconfig thing is working fine. and now I am iterating over making sdk a struct in dagger.json. making progress there.

vapid niche
vapid niche
#

hey @silk kestrel! hope you had a great weekend.

Just wanted to check with you how private packages is coming as we're aiming to have a release tomorrow. Getting a sense if it might make sense to push the release for a day or two so we can get this in or if it might take a bit more time 🙏

#

cc @exotic mulch

silk kestrel
#

the sdk field change is still a WIP, and i am working on that right now. But I dont think I can make it for tomorrows release as I still don't have a working version of it.

#

(The weekend was busy busy. ha ha. thank you for asking. I hope you had a great and relaxing weekend).

#

I'll report an update before my EOD to keep you posted on my progress

vapid niche
silk kestrel
#

an update: I am making progress. Its annoying because changing this code also breaks my dev env (using hack/dev) in difficult to debug ways.

now I am doing an "additive" approach instead of "replacing" the sdk with struct (atleast for dev until I figure out other quirks).

#

I am hoping to get this working tonight and then work on testing/integration-tests tomorrow.

vapid niche
silk kestrel
#

an update. I have dagger init, functions, develop working (still using additive field), but failing on dagger install <dependency> that I am debugging.

vapid niche
vapid niche
#

@silk kestrel is this issue still relevant now that you told me we've decided not to pursue the ModuleDependencyConfig approach?

silk kestrel
#

We still need to change sdk from a string to a struct

#

So i think its still relevant

silk kestrel
#

I have opened the draft PR. I verified that this works for init/develop and for both inbuilt and out-of-tree sdk.

#

now I am working on adding support for env variable to this struct.

silk kestrel
#

to update on progress. I verified the env variable + sdk-to-struct + gitconfig changes together

#

now cleaning up the code and fixing the tests.

exotic mulch
#

awesome ❤️

vapid niche
silk kestrel
#

Hi @exotic mulch, how do we fix this error:

Error: failed to get module SDK: input: module.withSource.sdk.source get field "source": reflect: call of reflect.Value.Field on zero Value```
#

would returning dagql.Instance work for this?