Skip to content

fix(gcpsmfs): handle secret listing and add concurrency - #1335

Merged
hairyhenderson merged 10 commits into
hairyhenderson:mainfrom
sthomson-wyn:wyn_gcp_sm
Jul 28, 2026
Merged

fix(gcpsmfs): handle secret listing and add concurrency#1335
hairyhenderson merged 10 commits into
hairyhenderson:mainfrom
sthomson-wyn:wyn_gcp_sm

Conversation

@sthomson-wyn

Copy link
Copy Markdown
Contributor

This fixes the "getProjectAndFileName" to correctly return "." when we just provide the project, so that we can iterate of the "directory" (e.g. using has in gomplate)

This also introduces configurable concurrency. API requests to GSM can be fairly time consuming, and using has in gomplate requires iterating over each secret in a project (there can be many). This allows the end user to configure concurrency using the GCP_SM_MAX_CONCURRENCY environment variable

AI slop summary:

Summary

This PR extends gcpsmfs with several correctness fixes and performance improvements for the GCP Secret Manager filesystem.

Correctness fixes

  • Project-only path as directoryOpen("projects/<id>") (no /secrets/... suffix) now returns a directory-typed file instead of fs.ErrInvalid. Stat() on such a handle correctly returns IsDir() == true, enabling callers that only have a project ID to resolve it as a directory entry.

  • ReadDir via getProjectAndFileNameReadDir now routes through getProjectAndFileName for path parsing, enabling ReadDir("projects/<id>") to work consistently alongside Open.

  • Skip secrets with no accessible version — During list(), secrets whose latest version returns NOT_FOUND (no version ever created) or FAILED_PRECONDITION (version is DISABLED or DESTROYED) are silently skipped rather than aborting the entire listing. Both GCP status codes are mapped through convertGCPError to fs.ErrNotExist.

Performance improvements

  • Parallel Stat()loadContent (fetches secret payload) and ensureModTime (fetches version metadata) are now run in parallel via errgroup within Stat(), reducing latency from 2× serial RPC time to ~1× RPC time.

  • Concurrent directory listinglist() has been rewritten with a two-phase approach: the GCP iterator is drained serially (it is not goroutine-safe), then all per-secret fetches are fanned out concurrently bounded by a configurable limit. Each secret's loadContent + ensureModTime also run in parallel within their own inner errgroup.

  • Configurable concurrency — A new WithMaxConcurrencyFS(n, fsys) function (following the existing With*FS pattern) sets the maximum number of secrets fetched concurrently during listing. The default is read from the GCP_SM_MAX_CONCURRENCY environment variable, falling back to 1 (serial, preserving previous behaviour).

  • In-memory cache — Secret payload and version metadata are cached per FS instance in a secretCache (two sync.Map values keyed by "project/name"). Subsequent ReadDir, ReadFile, or Stat calls for already-fetched secrets skip the GCP RPCs entirely. The cache is shared across all With* clones of the same FS instance.

Signed-off-by: Stewart Thomson <sthomson@wynshop.com>
Signed-off-by: Stewart Thomson <sthomson@wynshop.com>
Signed-off-by: Stewart Thomson <sthomson@wynshop.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances the gcpsmfs filesystem implementation for GCP Secret Manager by fixing project-only path handling and improving directory listing performance via bounded concurrency and caching.

Changes:

  • Treat bare projects/<id> paths as a directory by mapping them to "." in getProjectAndFileName.
  • Add configurable max concurrency (GCP_SM_MAX_CONCURRENCY + WithMaxConcurrencyFS) and concurrent secret fetch during listings.
  • Add per-FS in-memory caching for secret payloads and version mod-times, plus new/updated tests around concurrency, caching, and skip-on-inaccessible-version behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
gcpsmfs/gcpsm.go Implements project-only path support, bounded concurrency for listing, parallel Stat fetches, caching, and extended GCP error conversion.
gcpsmfs/gcpsm_test.go Adds tests for concurrency configuration, env var defaulting, caching dedupe behavior, project-only open, and skipping inaccessible/versionless secrets.
gcpsmfs/fake_client_test.go Extends the mock client to simulate versionless/disabled secrets and count RPC calls for cache/concurrency tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread gcpsmfs/gcpsm.go
Comment thread gcpsmfs/gcpsm.go Outdated
Comment thread gcpsmfs/gcpsm.go Outdated
Comment thread gcpsmfs/gcpsm.go
Comment thread gcpsmfs/gcpsm.go Outdated
Signed-off-by: Stewart Thomson <sthomson@wynshop.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

gcpsmfs/gcpsm.go:589

  • In list(), each secret does two RPCs in parallel via an inner errgroup, but the inner group doesn’t cancel the sibling RPC when one fails. This can add avoidable latency on error paths (including when skipping NOT_FOUND / FAILED_PRECONDITION secrets), because the goroutine waits for both calls even though the overall result is already determined. Consider using errgroup.WithContext for the inner group and setting child.ctx to the derived context so the second RPC is canceled as soon as the first returns an error.
			var inner errgroup.Group
			inner.Go(child.loadContent)
			inner.Go(child.ensureModTime)

Comment thread gcpsmfs/gcpsm.go
Comment thread gcpsmfs/gcpsm_test.go
Signed-off-by: Stewart Thomson <sthomson@wynshop.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

gcpsmfs/gcpsm.go:367

  • ReadDir forwards getProjectAndFileName() errors directly, so callers can receive a *fs.PathError with Op "getProjectAndFileName" instead of the expected Op "readdir". Wrapping/parsing errors as a readdir failure keeps error reporting consistent with the rest of the fs.FS API.
	if name != "." {
		var err error

		project, fileName, err = f.getProjectAndFileName(name)
		if err != nil {
			return nil, err
		}

Comment thread gcpsmfs/gcpsm.go
Signed-off-by: Stewart Thomson <sthomson@wynshop.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

gcpsmfs/gcpsm.go:388

  • ReadDir(".") on an unscoped FS (project == "") currently calls getClient() before returning the intended descriptive "requires a project" error. If client creation fails, the user sees an auth/client error instead. Check project == "" before constructing the Secret Manager client so the error is deterministic and avoids unnecessary setup.
	client, err := f.getClient()
	if err != nil {
		return nil, err
	}

	if project == "" {
		return nil, errors.New("listing secrets requires a project in the URL (e.g. gcp+sm:///projects/<project-id>)")
	}

Comment thread gcpsmfs/gcpsm.go Outdated
Signed-off-by: Stewart Thomson <sthomson@wynshop.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@hairyhenderson
hairyhenderson merged commit feaad43 into hairyhenderson:main Jul 28, 2026
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants