Skip to content

Handle 403 during first GCP up - #2061

Merged
lionello merged 2 commits into
mainfrom
lio/fix-gcp-403
Apr 16, 2026
Merged

Handle 403 during first GCP up#2061
lionello merged 2 commits into
mainfrom
lio/fix-gcp-403

Conversation

@lionello

@lionello lionello commented Apr 16, 2026

Copy link
Copy Markdown
Member

Description

Handle permission error that happens when we try to load the ProjectUpdate bucket object the first time we do "up". This happens before the SetupCD is run, so before bootstrapping.

Linked Issues

Fixes #2051

Checklist

  • I have performed a self-review of my code
  • I have added appropriate tests
  • I have updated the Defang CLI docs and/or README to reflect my changes, if necessary

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Improved error handling for Google Cloud Storage bucket operations to correctly identify missing objects during access-denied scenarios
    • Updated install script to skip shell completion setup in continuous integration environments
  • Tests

    • Added test coverage for HTTP 403 error scenarios in GCP service deployment
    • Removed unused test helper utilities
  • Chores

    • Removed unused helper function
    • Fixed typo in test documentation

@lionello
lionello requested a review from KevyVo April 16, 2026 19:02
@coderabbitai

coderabbitai Bot commented Apr 16, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

The PR improves GCP error handling to treat access-denied scenarios as "object not found" conditions, removes unused test utilities, skips completion installation during CI runs, and deletes an unused error-detail extraction helper function.

Changes

Cohort / File(s) Summary
CI and Installation
src/bin/install
Wrapped completion script installation logic in a conditional to skip execution when CI=1, preventing unnecessary installation attempts in CI environments.
GCP Error Handling
src/pkg/cli/client/byoc/gcp/byoc.go, src/pkg/cli/client/byoc/gcp/byoc_test.go
Enhanced GetProjectUpdate error handling to treat 403 Forbidden errors containing "(or it may not exist)" as ErrNotExist. Added TestGetServices subtest covering this scenario with googleapi.Error mocking.
GCP Error Utilities
src/pkg/cli/client/byoc/gcp/errors.go
Removed the GetGoogleAPIErrorDetail helper function and its strings import, consolidating error-annotation functionality.
Test Cleanup
src/pkg/cli/client/byoc/aws/byoc_test.go
Deleted unused FakeLoader test helper type and its two methods.
Documentation Fixes
src/pkg/cli/client/byoc/gcp/errors_test.go
Corrected typo in test comment from "wrapper" to "wrapped".

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • jordanstephens
  • edwardrf

Poem

🐰 In CI's realm, we skip with grace,
GCP errors find their place,
403s now mean "not there,"
Tests cleaned up with utmost care,
Error handling, refined and fair! ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'Handle 403 during first GCP up' is partially related to the changeset, referring to a real aspect (GCP 403 error handling), but is vague and incomplete. Clarify the title to be more specific about the context, such as 'Handle 403 errors when loading GCP ProjectUpdate on first deployment' or similar.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch lio/fix-gcp-403

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.11.4)

level=warning msg="[linters_context] running gomodguard failed: unable to read module file go.mod: current working directory must have a go.mod file: if you are not using go modules it is suggested to disable this linter"
level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread src/bin/install

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/pkg/cli/client/byoc/gcp/byoc.go`:
- Around line 851-852: The current predicate treats any error containing "(or it
may not exist)" as a not-exist case which can mask real auth failures; change
the conditional around gcp.ErrObjectNotExist so you only return
client.ErrNotExist when the error is explicitly gcp.ErrObjectNotExist OR when
the error is a 403 permission response that also includes the exact "(or it may
not exist)" text (i.e., narrow to HTTP 403 + that phrase). Implement a small
helper (e.g., is403WithMaybeNotExist(err)) and use it in place of
strings.Contains(err.Error(), "(or it may not exist)") so only the specific
forbidden-with-maybe-not-exist scenario falls back to client.ErrNotExist instead
of all errors containing that substring.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 96e7028f-0d2b-4337-9c7e-7e8a718bda38

📥 Commits

Reviewing files that changed from the base of the PR and between 77c810b and f7638a9.

📒 Files selected for processing (6)
  • src/bin/install
  • src/pkg/cli/client/byoc/aws/byoc_test.go
  • src/pkg/cli/client/byoc/gcp/byoc.go
  • src/pkg/cli/client/byoc/gcp/byoc_test.go
  • src/pkg/cli/client/byoc/gcp/errors.go
  • src/pkg/cli/client/byoc/gcp/errors_test.go
💤 Files with no reviewable changes (2)
  • src/pkg/cli/client/byoc/aws/byoc_test.go
  • src/pkg/cli/client/byoc/gcp/errors.go

Comment thread src/pkg/cli/client/byoc/gcp/byoc.go
@lionello
lionello merged commit 8185d6d into main Apr 16, 2026
15 checks passed
@lionello
lionello deleted the lio/fix-gcp-403 branch April 16, 2026 19:34
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.

GCP API Activation error on first deployment

3 participants