Fix github tarball retrieval (#217)

e2e/connect_repo_test.go

@@ -2,6 +2,7 @@ package e2e
 
 import (
 	"fmt"
+	"os"
 	"strings"
 	"testing"
 
@@ -22,12 +23,15 @@ func TestConnectRepo(t *testing.T) {
 	// test using user's personal organization
 	org := user.Username()
 
+	tarball, err := os.ReadFile("../testdata/github.tar.gz")
+	require.NoError(t, err)
+
 	// create an otf daemon with a fake github backend, ready to sign in a user,
 	// serve up a repo and its contents via tarball.
 	daemon := &daemon{}
 	daemon.withGithubUser(user)
 	daemon.withGithubRepo(repo)
-	daemon.withGithubTarball(otf.NewTestTarball(t, `resource "null_resource" "e2e" {}`))
+	daemon.withGithubTarball(tarball)
 	hostname := daemon.start(t)
 	url := "https://" + hostname
 
@@ -36,7 +40,7 @@ func TestConnectRepo(t *testing.T) {
 
 	orgSelector := fmt.Sprintf("#item-organization-%s a", org)
 
-	err := chromedp.Run(ctx, chromedp.Tasks{
+	err = chromedp.Run(ctx, chromedp.Tasks{
 		chromedp.Navigate(url),
 		// login
 		chromedp.Click(".login-button-github", chromedp.NodeVisible),

github.go

@@ -1,13 +1,12 @@
 package otf
 
 import (
-	"bytes"
-	"compress/gzip"
 	"context"
 	"crypto/tls"
 	"fmt"
-	"io"
 	"net/http"
+	"os"
+	"path"
 	"strings"
 
 	"github.com/google/go-github/v41/github"
@@ -206,18 +205,28 @@ func (g *githubProvider) GetRepoTarball(ctx context.Context, repo *VCSRepo) ([]b
 	if err != nil {
 		return nil, err
 	}
+	defer resp.Body.Close()
 
-	// convert .tar to .tar.gz
-	tarball := new(bytes.Buffer)
-	compressor := gzip.NewWriter(tarball)
-	if _, err := io.Copy(compressor, resp.Body); err != nil {
+	// github tarball contains a parent directory of the format
+	// <owner>-<repo>-<commit>. We need a tarball without this parent directory,
+	// so we untar it to a temp dir, then tar it up the contents of the parent
+	// directory.
+	untarpath, err := os.MkdirTemp("", fmt.Sprintf("github-%s-%s-*", repo.Owner(), repo.Repo()))
+	if err != nil {
 		return nil, err
 	}
-	if err := compressor.Close(); err != nil {
+	if err := Unpack(resp.Body, untarpath); err != nil {
 		return nil, err
 	}
-
-	return tarball.Bytes(), nil
+	contents, err := os.ReadDir(untarpath)
+	if err != nil {
+		return nil, err
+	}
+	if len(contents) != 1 {
+		return nil, fmt.Errorf("malformed tarball archive")
+	}
+	parentDir := path.Join(untarpath, contents[0].Name())
+	return Pack(parentDir)
 }
 
 func NewGithubEnterpriseClient(hostname string, httpClient *http.Client) (*github.Client, error) {

github_test.go

@@ -1,8 +1,10 @@
 package otf
 
 import (
+	"bytes"
 	"context"
 	"net/url"
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -48,7 +50,8 @@ func TestGithub_GetUser(t *testing.T) {
 func TestGithub_GetRepoTarball(t *testing.T) {
 	ctx := context.Background()
 
-	want := NewTestTarball(t, `file1 contents`, `file2 contents`)
+	want, err := os.ReadFile("testdata/github.tar.gz")
+	require.NoError(t, err)
 
 	srv := NewTestGithubServer(t,
 		WithGithubRepo(&Repo{Identifier: "acme/terraform", Branch: "master"}),
@@ -76,5 +79,7 @@ func TestGithub_GetRepoTarball(t *testing.T) {
 	})
 	require.NoError(t, err)
 
-	assert.Equal(t, compress(t, want), got)
+	dst := t.TempDir()
+	err = Unpack(bytes.NewReader(got), dst)
+	require.NoError(t, err)
 }

test_helpers.go

@@ -4,7 +4,6 @@ import (
 	"archive/tar"
 	"bytes"
 	"compress/gzip"
-	"io"
 	"testing"
 
 	"github.com/google/uuid"
@@ -83,12 +82,13 @@ func NewTestRepo() *Repo {
 	}
 }
 
-// NewTestTarball creates a tarball (.tar) consisting of files respectively populated with the
+// NewTestTarball creates a tarball (.tar.gz) consisting of files respectively populated with the
 // given contents. The files are assigned random names with the terraform file
 // extension appended (.tf)
 func NewTestTarball(t *testing.T, contents ...string) []byte {
 	var buf bytes.Buffer
-	tw := tar.NewWriter(&buf)
+	gw := gzip.NewWriter(&buf)
+	tw := tar.NewWriter(gw)
 
 	for _, body := range contents {
 		header := &tar.Header{
@@ -103,21 +103,10 @@ func NewTestTarball(t *testing.T, contents ...string) []byte {
 		require.NoError(t, err)
 	}
 
-	tw.Close()
-	return buf.Bytes()
-}
-
-// NewTestTarGZ wraps NewTestTarball, creating a .tar.gz instead.
-func NewTestTarGZ(t *testing.T, contents ...string) []byte {
-	return compress(t, NewTestTarball(t, contents...))
-}
-
-// compress runs gzip compression on the input bytes
-func compress(t *testing.T, b []byte) []byte {
-	var buf bytes.Buffer
-	compressor := gzip.NewWriter(&buf)
-	_, err := io.Copy(compressor, bytes.NewReader(b))
+	err := tw.Close()
 	require.NoError(t, err)
-	compressor.Close()
+	err = gw.Close()
+	require.NoError(t, err)
+
 	return buf.Bytes()
 }

testdata/pack/dir/file

@@ -0,0 +1 @@
+morestuff

testdata/pack/dir/symlink

@@ -0,0 +1 @@
+../file

testdata/pack/file

@@ -0,0 +1 @@
+stuff

unpack.go

@@ -2,6 +2,7 @@ package otf
 
 import (
 	"archive/tar"
+	"bytes"
 	"compress/gzip"
 	"fmt"
 	"io"
@@ -11,21 +12,15 @@ import (
 
 // Unpack a .tar.gz byte stream to a directory
 func Unpack(r io.Reader, dst string) error {
-	// Decompress as we read.
-	//
-	// TODO: rename decompressor
-	decompressed, err := gzip.NewReader(r)
+	gr, err := gzip.NewReader(r)
 	if err != nil {
 		return fmt.Errorf("failed to decompress archive: %w", err)
 	}
-	// TODO: close decompressed
+	tr := tar.NewReader(gr)
 
-	// Untar as we read.
-	untar := tar.NewReader(decompressed)
-
-	// Unpackage all the contents into the directory.
+	// Unpackage contents to the destination directory.
 	for {
-		header, err := untar.Next()
+		header, err := tr.Next()
 		if err == io.EOF {
 			break
 		}
@@ -40,7 +35,7 @@ func Unpack(r io.Reader, dst string) error {
 		}
 		path = filepath.Join(dst, path)
 
-		// Make the directories to the path.
+		// Ensure path's parent directories exist
 		dir := filepath.Dir(path)
 		if err := os.MkdirAll(dir, 0o755); err != nil {
 			return fmt.Errorf("failed to create directory: %w", err)
@@ -56,11 +51,8 @@ func Unpack(r io.Reader, dst string) error {
 		}
 
 		// Only unpack regular files from this point on.
-		if header.Typeflag == tar.TypeDir {
+		if header.Typeflag != tar.TypeReg {
 			continue
-		} else if header.Typeflag != tar.TypeReg && header.Typeflag != tar.TypeRegA {
-			return fmt.Errorf("failed creating %q: unsupported type %c", path,
-				header.Typeflag)
 		}
 
 		// Open a handle to the destination.
@@ -80,7 +72,8 @@ func Unpack(r io.Reader, dst string) error {
 		}
 
 		// Copy the contents.
-		_, err = io.Copy(fh, untar)
+		_, err = io.Copy(fh, tr)
+		// TODO: why are we closing it here?
 		fh.Close()
 		if err != nil {
 			return fmt.Errorf("failed to copy file %q: %w", path, err)
@@ -95,3 +88,69 @@ func Unpack(r io.Reader, dst string) error {
 	}
 	return nil
 }
+
+// Pack a directory into tarball (.tar.gz) and return its contents
+func Pack(src string) ([]byte, error) {
+	var buf bytes.Buffer
+	gw := gzip.NewWriter(&buf)
+	tw := tar.NewWriter(gw)
+
+	err := filepath.Walk(src, func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return nil
+		}
+
+		target := info.Name()
+
+		if info.Mode()&os.ModeSymlink == os.ModeSymlink {
+			target, err = os.Readlink(path)
+			if err != nil {
+				return nil
+			}
+		}
+
+		hdr, err := tar.FileInfoHeader(info, target)
+		if err != nil {
+			return nil
+		}
+		name, err := filepath.Rel(src, path)
+		if err != nil {
+			return nil
+		}
+		hdr.Name = name
+
+		if err := tw.WriteHeader(hdr); err != nil {
+			return err
+		}
+
+		// Unless it's a normal file there's nothing more to be done
+		if hdr.Typeflag != tar.TypeReg {
+			return nil
+		}
+
+		f, err := os.Open(path)
+		if err != nil {
+			return err
+		}
+		defer f.Close()
+
+		_, err = io.Copy(tw, f)
+		if err != nil {
+			return err
+		}
+		return nil
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	// Write tar and gzip headers
+	if err := tw.Close(); err != nil {
+		return nil, err
+	}
+	if err := gw.Close(); err != nil {
+		return nil, err
+	}
+
+	return buf.Bytes(), nil
+}

unpack_test.go

@@ -1,7 +1,9 @@
 package otf
 
 import (
+	"bytes"
 	"os"
+	"path"
 	"path/filepath"
 	"testing"
 
@@ -32,3 +34,32 @@ func TestUnpack(t *testing.T) {
 		"file",
 	}, got)
 }
+
+// TestUnpack_Github tests unpacking a Github archive of a git repository. Their
+// archive is somewhat idiosyncratic in that it uses the PAX format, with
+// key-values embedded in the tar file. It's tripped up the unpack code before,
+// so putting this test in here to prevent a regression.
+func TestUnpack_Github(t *testing.T) {
+	tarball, err := os.Open("testdata/github.tar.gz")
+	require.NoError(t, err)
+
+	require.NoError(t, Unpack(tarball, t.TempDir()))
+}
+
+func TestPack(t *testing.T) {
+	tarball, err := Pack("testdata/pack")
+	require.NoError(t, err)
+
+	// unpack tarball to test its contents
+	dst := t.TempDir()
+	err = Unpack(bytes.NewReader(tarball), dst)
+	require.NoError(t, err)
+
+	assert.FileExists(t, path.Join(dst, "file"))
+	assert.DirExists(t, path.Join(dst, "dir"))
+	symlink, err := os.Readlink(path.Join(dst, "dir", "symlink"))
+	if assert.NoError(t, err) {
+		assert.Equal(t, "../file", symlink)
+	}
+	assert.FileExists(t, path.Join(dst, "dir", "file"))
+}