Require Registered Branch To Exist (#3024)

When working with empty repository, Porch may push the first branch to
the remote. GitHub makes the first branch the default which cannot be
easily deleted.

For more predictable user experience, require the branch the client
provided at registration (defaulting to `main`) to exist.

Improve the archive command to set consistent file timestamps and delete
more unnecessary content.

Author: martinmaly

porch/engine/pkg/engine/clone.go

@@ -128,7 +128,6 @@ func (m *clonePackageMutation) cloneFromGit(ctx context.Context, gitPackage *api
 
 	spec := configapi.GitRepository{
 		Repo:      gitPackage.Repo,
-		Branch:    gitPackage.Ref,
 		Directory: gitPackage.Directory,
 		SecretRef: configapi.SecretRef{
 			Name: gitPackage.SecretRef.Name,
@@ -142,7 +141,8 @@ func (m *clonePackageMutation) cloneFromGit(ctx context.Context, gitPackage *api
 	defer os.RemoveAll(dir)
 
 	r, err := git.OpenRepository(ctx, "", "", &spec, dir, git.GitRepositoryOptions{
-		CredentialResolver: m.credentialResolver,
+		CredentialResolver:         m.credentialResolver,
+		SkipMainBranchVerification: true, // We are only reading so we don't need the main branch to exist.
 	})
 	if err != nil {
 		return repository.PackageResources{}, fmt.Errorf("cannot clone Git repository: %w", err)

porch/hack/tar-test-repo.sh

@@ -22,8 +22,19 @@ if [[ $# -ne 2 ]]; then
   error "Invalid # of arguments; ${#}. 2 expected: GIT_DIRECTORY OUTPUT_TAR_FILE"
 fi
 
-tar -c -f "${2}" -C "${1}" --owner=porch:0 --group=porch:0 \
-  --exclude '.git/logs' \
-  --exclude '.git/COMMIT_EDITMSG' \
-  --exclude '.git/ORIG_HEAD' \
-  .git
+GIT_DIRECTORY="${1}"
+OUTPUT_TAR_FILE="${2}"
+
+if [ -d "${GIT_DIRECTORY}" ]; then
+
+  tar -c -v -f "${OUTPUT_TAR_FILE}" -C "${GIT_DIRECTORY}" --owner=porch:0 --group=porch:0 \
+    --sort=name --mtime='PST 2022-02-02' \
+    --exclude '.git/logs' \
+    --exclude '.git/COMMIT_EDITMSG' \
+    --exclude '.git/ORIG_HEAD' \
+    --exclude '.git/info/exclude' \
+    .git
+
+else
+  error "${GIT_DIRECTORY} doesn't exist"
+fi

porch/repository/pkg/git/git.go

@@ -50,14 +50,23 @@ type GitRepository interface {
 }
 
 type GitRepositoryOptions struct {
-	CredentialResolver repository.CredentialResolver
-	UserInfoProvider   repository.UserInfoProvider
+	CredentialResolver         repository.CredentialResolver
+	UserInfoProvider           repository.UserInfoProvider
+	SkipMainBranchVerification bool
 }
 
 func OpenRepository(ctx context.Context, name, namespace string, spec *configapi.GitRepository, root string, opts GitRepositoryOptions) (GitRepository, error) {
 	replace := strings.NewReplacer("/", "-", ":", "-")
 	dir := filepath.Join(root, replace.Replace(spec.Repo))
 
+	// Cleanup the cache directory in case initialization fails.
+	cleanup := dir
+	defer func() {
+		if cleanup != "" {
+			os.RemoveAll(cleanup)
+		}
+	}()
+
 	var repo *git.Repository
 
 	if fi, err := os.Stat(dir); err != nil {
@@ -72,9 +81,11 @@ func OpenRepository(ctx context.Context, name, namespace string, spec *configapi
 
 		repo = r
 	} else if !fi.IsDir() {
-		// Internal error - corrupted cache.
+		// Internal error - corrupted cache. We will cleanup on the way out.
 		return nil, fmt.Errorf("cannot clone git repository %q: %w", spec.Repo, err)
 	} else {
+		cleanup = "" // Existing directory; do not delete it.
+
 		r, err := openRepository(dir)
 		if err != nil {
 			return nil, err
@@ -90,8 +101,6 @@ func OpenRepository(ctx context.Context, name, namespace string, spec *configapi
 
 	branch := MainBranch
 	if spec.Branch != "" {
-		// TODO: Validate branch name syntax (we can't check whether the branch exists;
-		// the repository may be empty).
 		branch = BranchName(spec.Branch)
 	}
 
@@ -110,6 +119,12 @@ func OpenRepository(ctx context.Context, name, namespace string, spec *configapi
 		return nil, err
 	}
 
+	if err := repository.verifyRepository(&opts); err != nil {
+		return nil, err
+	}
+
+	cleanup = "" // Success. Keep the git directory.
+
 	return repository, nil
 }
 
@@ -705,6 +720,22 @@ func (r *gitRepository) fetchRemoteRepository(ctx context.Context) error {
 	return nil
 }
 
+// Verifies repository. Repository must be fetched already.
+func (r *gitRepository) verifyRepository(opts *GitRepositoryOptions) error {
+	// When opening a temporary repository, such as for cloning a package
+	// from unregistered upstream, we won't be pushing into the remote so
+	// we don't need to verify presence of the main branch.
+	if !opts.SkipMainBranchVerification {
+		// Check existence of main branch. If it doesn't exist, fail.
+		// If the main branch doesn't exist, we could create draft or proposal branch
+		// which could become the default branch and those are hard to delete.
+		if _, err := r.repo.Reference(r.branch.RefInLocal(), false); err != nil {
+			return fmt.Errorf("branch %q doesn't exist: %v", r.branch, err)
+		}
+	}
+	return nil
+}
+
 // Creates a commit which deletes the package from the branch, and returns its commit hash.
 // If the branch doesn't exist, will return zero hash and no error.
 func (r *gitRepository) createPackageDeleteCommit(ctx context.Context, branch plumbing.ReferenceName, pkg *gitPackageRevision) (plumbing.Hash, error) {

porch/repository/pkg/git/git_test.go

@@ -44,6 +44,32 @@ func TestMain(m *testing.M) {
 	os.Exit(m.Run())
 }
 
+func TestOpenEmptyRepository(t *testing.T) {
+	tempdir := t.TempDir()
+	tarfile := filepath.Join("testdata", "empty-repository.tar")
+	_, address := ServeGitRepository(t, tarfile, tempdir)
+
+	ctx := context.Background()
+	const (
+		name      = "empty"
+		namespace = "default"
+	)
+
+	repository := &configapi.GitRepository{
+		Repo:      address,
+		Branch:    "main",
+		Directory: "/",
+	}
+
+	if _, err := OpenRepository(ctx, name, namespace, repository, tempdir, GitRepositoryOptions{}); err == nil {
+		t.Errorf("Unexpectedly succeeded opening empty repository with main branch validation enabled.")
+	}
+
+	if _, err := OpenRepository(ctx, name, namespace, repository, tempdir, GitRepositoryOptions{SkipMainBranchVerification: true}); err != nil {
+		t.Errorf("Failed to open empty git repository with main branch validation disabled: %v", err)
+	}
+}
+
 // TestGitPackageRoundTrip creates a package in git and verifies we can read the contents back.
 func TestGitPackageRoundTrip(t *testing.T) {
 	tempdir := t.TempDir()
@@ -283,9 +309,9 @@ info:
   description: Empty Package
 `
 
-func TestListPackagesEmpty(t *testing.T) {
+func TestListPackagesTrivial(t *testing.T) {
 	tempdir := t.TempDir()
-	tarfile := filepath.Join("testdata", "empty-repository.tar")
+	tarfile := filepath.Join("testdata", "trivial-repository.tar")
 	_, address := ServeGitRepository(t, tarfile, tempdir)
 
 	ctx := context.Background()

porch/repository/pkg/git/testdata/.gitignore

@@ -0,0 +1,5 @@
+empty-repository/
+trivial-repository/
+simple-repository/
+drafts-repository/
+nested-repository/

porch/repository/pkg/git/testdata/Makefile

@@ -1,8 +1,53 @@
+# Copyright 2022 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+REPOSITORIES = \
+  empty-repository.tar \
+  trivial-repository.tar \
+  simple-repository.tar \
+  drafts-repository.tar \
+  nested-repository.tar
+
+
 .PHONY: expand
 expand:
-	rm -rf drafts-repository; mkdir drafts-repository
-	cd drafts-repository; tar xfv ../drafts-repository.tar; git reset --hard main
+	@for f in $(REPOSITORIES); do \
+	  dir=$${f%.*}; \
+	  rm -rf $${dir}; mkdir $${dir}; \
+	  tar xf $${f} -C $${dir}; \
+	  echo "$${f} --> $${dir}/"; \
+	done
+
+.PHONY: checkout
+checkout: expand
+	@for f in $(REPOSITORIES); do \
+	  dir=$${f%.*}; \
+	  (cd $${dir}; git reset --hard refs/heads/main -- 2>/dev/null ); \
+	done
 
 .PHONY: tars
 tars:
-	../../../../hack/tar-test-repo.sh drafts-repository/ drafts-repository.tar
+	@for f in $(REPOSITORIES); do \
+	  dir=$${f%.*}; \
+	  ../../../../hack/tar-test-repo.sh $${dir}/ $${f}; \
+	  rm -rf $${dir}; \
+	  echo "$${dir}/ --> $${f}"; \
+	done
+
+.PHONY: clean
+clean:
+	@for f in $(REPOSITORIES); do \
+	  dir=$${f%.*}; \
+	  rm -rf $${dir}; \
+	done

porch/repository/pkg/git/testdata/drafts-repository.md

@@ -0,0 +1,46 @@
+# Drafts Repository
+
+For easier orientation, an overview of the contents of the `drafts-repository.tar`.
+
+## Contents
+
+The repository in `drafts-repository.tar` has the following contents in `main` branch:
+
+```
+.
+├── basens
+│   ├── Kptfile
+│   ├── namespace.yaml
+│   ├── package-context.yaml
+│   └── README.md
+├── empty
+│   ├── Kptfile
+│   └── README.md
+└── istions
+    ├── istions.yaml
+    ├── Kptfile
+    ├── package-context.yaml
+    └── README.md
+```
+
+## Commits
+
+The commit graph of the repository is:
+
+```
+* 487eaa0fe7652a313dcdb05790aa32034398593a (drafts/none/v1) Add none package with Kptfile marker
+* c7edca419782f88646f9572b0a829d686b2d91bd (HEAD -> main, tag: istions/v2) Add Istio Namespace Resource
+* c93d417f1393ae5d7def978da70c42b62e645cda (tag: basens/v2) Add Base Namespace Resource
+| * 032c503a3921f322850e9bd49319346e0e0b129d (drafts/bucket/v1) Add Bucket Resource
+| * 1950b803f552e4c89ac17c528ec466c1a7375083 Add Bucket Package Context
+| * 5ea104c29951f3c3995ae15c4a367823794bd47d Empty Bucket Package
+|/  
+* f8fb59f626182319ec78dd542afcce35f98811e2 (tag: istions/v1) Add Istio Namespace Package Context
+* 740397bf8f594b785f6802755945bd58a5e94192 (tag: basens/v1) Add Base Namespace Package Context
+* 3d21cf677b3afcca132e0b415144e83f1f2ca2a9 Empty Istio Namespace
+* ca725b9cd90d5a3ee22101f0ea66f83e40217a6f Empty Base Namespace
+* 7a6308e79524221d74f549bac53bf1ed771958f7 (tag: empty/v1) Empty Package
+
+```
+
+In addition to several published packages there are two drafts (`none` and `bucket`).

porch/repository/pkg/git/testdata/drafts-repository.tar

@@ -0,0 +1,13 @@
+# Empty Repository
+
+For easier orientation, an overview of the contents of the `empty-repository.tar`.
+
+## Contents
+
+The repository in `empty-repository.tar` is truly empty. It has no commits, no refs.
+Because `HEAD` is required for a valid git repository, `HEAD` is a symbolic ref to
+(non-existent) `refs/heads/main`.
+
+## Commits
+
+There are no commits in this repository.