Fix bugs in recursive repo resolution (#82)

The previous recursive implementation is flawed in a few ways:

1.  It queries the entire registry for each repo, which is horribly inefficient.

2.  It misses cases where the repo FQDN might not exactly match the given root (e.g. trailing slashes).

3.  It lacks decision logging, making it incredibly difficult to debug.

The New and Improved Implementation ™️ removes the need for recursion:

1.  Since the Docker v2 API requires we list the entire catalog anyway, the new implementation parses the repos and extracts and de-dupes the registry components. Since it is impossible for a catalog entry to point to something outside of its registry, this is same to pre-compute.

2.  Each registry is queried exactly once, and compared against the supplied root prefixes. If the catalog entry begins with the root, it is considered a valid target. This means a repo of "gcr.io/foo" lists all of gcr.io and then does client-side selection any catalog entries that begin with "gcr.io/foo".

3.  Every action and decision is logged at the debug level, so users and maintainers can more easily identify potential issues.

README.md

@@ -206,10 +206,20 @@ The payload is expected to be JSON with the following fields:
     the service account in order to query the registry. The most minimal
     permissions are `roles/browser`.
 
-    **WARNING!** If you have access to many Container Registry repos, this will
-    be very slow! This is because the Docker v2 API does not support server-side
-    filtering, meaning GCR Cleaner must download a manifest of all repositories
-    to which you have access and then do client-side filtering.
+    **WARNING!** If the authenticated principal has access to many Container
+    Registry or Artifact Registry repos, this will be very slow! This is because
+    the Docker v2 API does not support server-side filtering, meaning GCR
+    Cleaner must download a manifest of all repositories to which you have
+    access and then do client-side filtering. The most granular filter is at the
+    _host_ layer, meaning GCR Cleaner will perform a list operation on `gcr.io`
+    (for Container Registry) or `us-docker.pkg.dev` (for Artifact Registry),
+    parse the response and do client-side filtering to match against the
+    provided patterns, then start deleting. To re-iterate, this operation is
+    **not segmented by project** - if the authenticated principal has access to
+    10,000 repos, the client will need to filter through 10,000 repos. The
+    easiest way to mitigate this is to practice the Principle of Least Privilege
+    and create a dedicated service account that has granular permissions on a
+    subset of repositories.
 
 - `tag_filter` (_Deprecated_) - This option is deprecated and only exists to
   maintain backwards compatibility with some existing broken behavior. You

cmd/gcr-cleaner-cli/main.go

@@ -159,13 +159,18 @@ func realMain(ctx context.Context, logger *gcrcleaner.Logger) error {
 	// Gather the repositories.
 	if *recursivePtr {
 		logger.Debug("gathering child repositories recursively")
-		for _, repo := range repos {
-			childRepos, err := cleaner.ListChildRepositories(ctx, repo)
-			if err != nil {
-				return err
-			}
-			repos = append(repos, childRepos...)
+
+		allRepos, err := cleaner.ListChildRepositories(ctx, repos)
+		if err != nil {
+			return err
 		}
+		logger.Debug("recursively listed child repositories",
+			"in", repos,
+			"out", allRepos)
+
+		// This is safe because ListChildRepositories is guaranteed to include at
+		// least the list repos givenh to it.
+		repos = allRepos
 	}
 
 	// Log dry-run mode.

pkg/gcrcleaner/cleaner.go

@@ -246,29 +246,97 @@ func (c *Cleaner) shouldDelete(m *manifest, since time.Time, tagFilter TagFilter
 	return false
 }
 
-func (c *Cleaner) ListChildRepositories(ctx context.Context, rootRepository string) ([]string, error) {
-	rootRepo, err := gcrname.NewRepository(rootRepository)
-	if err != nil {
-		return nil, fmt.Errorf("failed to create repository %s: %w", rootRepository, err)
-	}
+// ListChildRepositories lists all child repositores for the given roots. Roots
+// can be entire registries (e.g. us-docker.pkg.dev) or a subpath within a
+// registry (e.g. gcr.io/my-project/my-container).
+func (c *Cleaner) ListChildRepositories(ctx context.Context, roots []string) ([]string, error) {
+	c.logger.Debug("finding all child repositories", "roots", roots)
+
+	// registriesMap is a cache of registries to all the repos in that registry.
+	// Since multiple repos might use the same registry, the result is cached to
+	// limit upstream API calls.
+	registriesMap := make(map[string]*gcrname.Registry, len(roots))
+
+	// Iterate over each root and attempt to extract the registry component. Some
+	// roots will be registries themselves whereas other roots could be a subpath
+	// in a registry and we need to extract just the registry part.
+	for _, root := range roots {
+		registryName := ""
+
+		parts := strings.Split(root, "/")
+		switch len(parts) {
+		case 0:
+			panic("got 0 parts from string split (impossible)")
+		case 1:
+			// Most likely this is a registry, since it contains no slashes.
+			registryName = parts[0]
+		default:
+			repo, err := gcrname.NewRepository(root, gcrname.StrictValidation)
+			if err != nil {
+				return nil, fmt.Errorf("failed to parse root repository %q: %w", root, err)
+			}
 
-	registry, err := gcrname.NewRegistry(rootRepo.RegistryStr())
-	if err != nil {
-		return nil, fmt.Errorf("failed to create registry %s: %w", rootRepo.RegistryStr(), err)
-	}
+			registryName = repo.RegistryStr()
+		}
 
-	allRepos, err := gcrremote.Catalog(ctx, registry, gcrremote.WithAuth(c.auther))
-	if err != nil {
-		return nil, fmt.Errorf("failed to fetch all repositories from registry %s: %w", registry.Name(), err)
+		registry, err := gcrname.NewRegistry(registryName)
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse registry name %q: %w", registryName, err)
+		}
+		registriesMap[registryName] = &registry
 	}
 
-	var childRepos = make([]string, 0, len(allRepos))
-	for _, repo := range allRepos {
-		if strings.HasPrefix(repo, rootRepo.RepositoryStr()) {
-			childRepos = append(childRepos, fmt.Sprintf("%s/%s", registry.Name(), repo))
+	// candidateRepos is the list of full repository names that match any of the
+	// given root repositories. This list is appended to so the range function
+	// below is psuedo-recursive.
+	candidateRepos := make([]string, 0, len(roots))
+
+	// Iterate through each registry, query the entire registry (yea, that's how
+	// you "search"), and collect a list of candidate repos.
+	for _, registry := range registriesMap {
+		c.logger.Debug("listing child repositories for registry",
+			"registry", registry.Name())
+
+		// List all repos in the registry.
+		allRepos, err := gcrremote.Catalog(ctx, *registry,
+			gcrremote.WithAuth(c.auther),
+			gcrremote.WithContext(ctx))
+		if err != nil {
+			return nil, fmt.Errorf("failed to fetch catalog for registry %q: %w", registry.Name(), err)
+		}
+
+		c.logger.Debug("found child repositories for registry",
+			"registry", registry.Name(),
+			"repos", allRepos)
+
+		// Search through each repository and append any repository that matches any
+		// of the prefixes defined by roots.
+		for _, repo := range allRepos {
+			// Compute the full repo name by appending the repo to the registry
+			// identifier.
+			fullRepoName := registry.Name() + "/" + repo
+
+			hasPrefix := false
+			for _, root := range roots {
+				if strings.HasPrefix(fullRepoName, root) {
+					hasPrefix = true
+					break
+				}
+			}
+			if hasPrefix {
+				c.logger.Debug("appending new repository candidate",
+					"registry", registry.Name(),
+					"repo", repo)
+				candidateRepos = append(candidateRepos, fullRepoName)
+			} else {
+				c.logger.Debug("skipping repository candidate (does not match any roots)",
+					"registry", registry.Name(),
+					"repo", repo)
+			}
 		}
 	}
 
-	sort.Strings(childRepos)
-	return childRepos, nil
+	// De-duplicate and sort the list.
+	sort.Strings(candidateRepos)
+	return candidateRepos, nil
 }

pkg/gcrcleaner/server.go

@@ -160,13 +160,19 @@ func (s *Server) clean(ctx context.Context, r io.ReadCloser) (map[string][]strin
 		repos = append(repos, t)
 	}
 	if p.Recursive {
-		for _, repo := range repos {
-			childRepos, err := s.cleaner.ListChildRepositories(ctx, repo)
-			if err != nil {
-				return nil, http.StatusBadRequest, fmt.Errorf("failed to list child repositories for %q: %w", p.Repo, err)
-			}
-			repos = append(repos, childRepos...)
+		s.logger.Debug("gathering child repositories recursively")
+
+		allRepos, err := s.cleaner.ListChildRepositories(ctx, repos)
+		if err != nil {
+			return nil, http.StatusBadRequest, fmt.Errorf("failed to list child repositories for %q: %w", p.Repo, err)
 		}
+		s.logger.Debug("recursively listed child repositories",
+			"in", repos,
+			"out", allRepos)
+
+		// This is safe because ListChildRepositories is guaranteed to include at
+		// least the list repos givenh to it.
+		repos = allRepos
 	}
 
 	s.logger.Info("deleting refs",