add support for git remote resources #10811

ndeloof · 2023-07-17T13:05:29Z

What I did
This use of compose-spec/compose-go#435 to add support for git remotes as resources in include or extends.

Uses same syntax as docker build: <GIT_URL>#<branch>:<path>. If path is not set, looks for a canonical compose.yaml file or other supported file names.
Remote repo is cloned under ~/.cache/docker-compose in a sub-folder named by commit sha1, so that we can easily detect a known remote is available locally.

include:
  - [email protected]:ndeloof/test.git

Related issue
https://docker.atlassian.net/browse/ENV-251

(not mandatory) A picture of a cute animal, if possible in relation to what you did

cmd/compose/compose.go

pkg/remote/git.go

milas · 2023-07-18T14:19:09Z

Btw you can consider my review a LGTM, I just didn't approve (yet) since this is PR is blocked on waiting for new compose-go to get tagged with compose-spec/compose-go#435

ndeloof · 2023-07-18T14:21:40Z

This really is just a prototype implementation used as demonstration purpose. questions like #10811 (comment) are to be addressed - makes me wonder, how does builkit manage this scenario?

codecov · 2023-08-10T14:08:06Z

Codecov Report

Patch coverage: 15.85% and project coverage change: -0.62% ⚠️

Comparison is base (792afb8) 58.71% compared to head (d02f0f0) 58.10%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10811      +/-   ##
==========================================
- Coverage   58.71%   58.10%   -0.62%     
==========================================
  Files         120      121       +1     
  Lines       10477    10622     +145     
==========================================
+ Hits         6152     6172      +20     
- Misses       3722     3839     +117     
- Partials      603      611       +8

Files Changed	Coverage Δ
pkg/remote/git.go	`9.75% <9.75%> (ø)`
cmd/compose/config.go	`34.13% <25.00%> (+0.59%)`	⬆️
cmd/compose/compose.go	`70.90% <47.05%> (-1.29%)`	⬇️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

milas

A few robustness suggestions but nothing major.

My "blocking" concern right now is gating this somehow since it's not formally part of the spec and we're still experimenting somewhat here.

milas · 2023-08-16T14:41:07Z

cmd/compose/compose.go

 func (o *ProjectOptions) WithServices(fn ProjectServicesFunc) func(cmd *cobra.Command, args []string) error {
 	return Adapt(func(ctx context.Context, args []string) error {
-		project, err := o.ToProject(args, cli.WithResolvedPaths(true), cli.WithDiscardEnvFile)
+		git, err := remote.NewGitRemoteLoader()


I think we should put this behind a flag for now, e.g. COMPOSE_EXPERIMENTAL_INCLUDE_REMOTE=1.

It's not part of the spec at the moment, so I don't think it should be enabled by default until we've done that and ironed out ambiguities, etc.

milas · 2023-08-16T14:44:31Z

pkg/remote/git.go

+	if cache_home, ok := os.LookupEnv("XDG_CACHE_HOME"); ok {
+		base = cache_home


Suggested change

if cache_home, ok := os.LookupEnv("XDG_CACHE_HOME"); ok {

base = cache_home

if cacheHome := os.GetEnv("XDG_CACHE_HOME"); cacheHome != "" {

base = cacheHome

Unlikely, but if it's set and empty, we should still fallback to ~/.cache

milas · 2023-08-16T14:48:58Z

pkg/remote/git.go

+		if len(out) < 40 {
+			return "", fmt.Errorf("repository does not contain ref %s, output: %q", path, string(out))
+		}
+		sha := string(out[:40])
+		if !commitSHA.MatchString(sha) {
+			return "", fmt.Errorf("invalid commit sha %q", sha)
+		}


Can use --exit-code instead of trying to match on stdout:

--exit-code Exit with status "2" when no matching refs are found in the remote repository. Usually the command exits with status "0" to indicate it successfully talked with the remote repository, whether it found any matching refs.

milas · 2023-08-16T14:52:32Z

pkg/remote/git.go

+			return "", err
+		}
+
+		cmd = exec.CommandContext(ctx, "git", "fetch", "origin", ref.Commit)


I'd add --depth=1 here, we don't need the history

milas · 2023-08-16T14:57:08Z

pkg/remote/git.go

+func findFile(names []string, pwd string) (string, error) {
+	for _, n := range names {
+		f := filepath.Join(pwd, n)
+		if _, err := os.Stat(f); err == nil {


Suggested change

if _, err := os.Stat(f); err == nil {

if fi, err := os.Stat(f); err == nil && !fi.IsDir() {

(I mean, I hope nobody has a directory named compose.yaml because that's just chaotic evil but 🤷🏻)

milas · 2023-08-16T15:04:43Z

pkg/remote/git.go

+
+var commitSHA = regexp.MustCompile(`^[a-f0-9]{40}$`)
+
+func (g gitRemoteLoader) Load(ctx context.Context, path string) (string, error) {


Doing background Git fetches is always tricky because of credential helpers.

Take a look at https://github.com/tilt-dev/go-get/blob/295ccc6550921a9766a7294360a77a4d8704d826/get.go#L18-L55

I'd clone os.Environ() and then conditionally add GIT_TERMINAL_PROMPT / GIT_SSH_COMMAND to the env passed to the exec'd processes.

Signed-off-by: Nicolas De Loof <[email protected]>

crazy-max · 2023-08-30T23:51:25Z

There should be entitlements for this feature and I don't think there is any in this implementation. An attacker could potentially read any files from the host if remote repo is compromised such as:

include:
  - [email protected]:hacked/test.git

Compromised compose file on remote repo:

services:
  db:
    image: haxxor-mariadb:latest
    volumes:
      - "db:/var/lib/mysql"
      - "$HOME/.aws/credentials:/aws_credentials"
    restart: always

volumes:
  db:

haxxor-mariadb:latest would then send /aws_credentials to a CNC server.

ndeloof · 2023-08-31T06:27:03Z

@crazy-max sure, but AFAICT this is the case for any form of "sharing compose file" scenario. Any suggestion ?

ndeloof mentioned this pull request Jul 17, 2023

introduce ResourceResolver to accept remote resources compose-spec/compose-go#435

Merged

milas reviewed Jul 18, 2023

View reviewed changes

cmd/compose/compose.go Outdated Show resolved Hide resolved

pkg/remote/git.go Outdated Show resolved Hide resolved

pkg/remote/git.go Outdated Show resolved Hide resolved

ndeloof force-pushed the remote_resource branch from f00f0b9 to 1f5b53e Compare August 10, 2023 12:45

ndeloof requested review from a team, StefanScherer, glours, laurazard, milas, nicksieger and ulyssessouza and removed request for a team August 10, 2023 12:45

ndeloof force-pushed the remote_resource branch 5 times, most recently from 19ca4aa to 462a08d Compare August 10, 2023 13:48

ndeloof force-pushed the remote_resource branch from 462a08d to eefdc9a Compare August 10, 2023 15:26

milas suggested changes Aug 16, 2023

View reviewed changes

ndeloof force-pushed the remote_resource branch 5 times, most recently from ce535e2 to 91bb372 Compare August 17, 2023 16:11

add support for git remote resources

d02f0f0

Signed-off-by: Nicolas De Loof <[email protected]>

ndeloof force-pushed the remote_resource branch from 91bb372 to d02f0f0 Compare August 18, 2023 07:48

ndeloof requested a review from milas August 18, 2023 08:46

milas approved these changes Aug 18, 2023

View reviewed changes

milas merged commit dd34f7a into docker:main Aug 18, 2023

ndeloof deleted the remote_resource branch August 31, 2023 06:26

thespad mentioned this pull request Sep 19, 2023

[BUG] include from git repo works with config but not pull, up #11022

Closed

		if cache_home, ok := os.LookupEnv("XDG_CACHE_HOME"); ok {
		base = cache_home

	if _, err := os.Stat(f); err == nil {
	if fi, err := os.Stat(f); err == nil && !fi.IsDir() {


		var commitSHA = regexp.MustCompile(`^[a-f0-9]{40}$`)

		func (g gitRemoteLoader) Load(ctx context.Context, path string) (string, error) {

add support for git remote resources #10811

add support for git remote resources #10811

Uh oh!

Conversation

ndeloof commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

milas commented Jul 18, 2023

Uh oh!

ndeloof commented Jul 18, 2023

Uh oh!

codecov bot commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

milas left a comment

Choose a reason for hiding this comment

Uh oh!

milas Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

milas Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

milas Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

milas Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

milas Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

milas Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

crazy-max commented Aug 30, 2023

Uh oh!

ndeloof commented Aug 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ndeloof commented Jul 17, 2023 •

edited

Loading

codecov bot commented Aug 10, 2023 •

edited

Loading