fix(cd): adopt an existing CD state bucket instead of creating a new one#2194
Conversation
The CodeBuild CD (codebuild/cfn) always created a stack-managed, CFN auto-named S3 bucket with RetainExceptOnCreate. So `defang cd teardown` retained the bucket and the next bootstrap minted a fresh one, orphaning the old bucket (and its Pulumi state) on every teardown/re-bootstrap cycle. This re-introduced DefangLabs#358, which DefangLabs#422 had fixed for the old ECS CD before the CodeBuild rewrite dropped the logic. Before creating the stack, look for a retained `<stack>-bucket-*` in the same account+region (matched by the defang:Prefix tag). If exactly one exists, generate the template without a managed bucket and emit the existing bucket name as the bucketName output; RetainExceptOnCreate makes the one-time managed->adopted transition safe (the physical bucket is retained when the resource leaves the template). Ambiguous (>1) matches error rather than guess. Adds unit tests for findAdoptableBucket and the template bucket-adoption branch. The published/quick-create template is unchanged (managed bucket). Fixes DefangLabs#2192. Re-fixes DefangLabs#358 for the CodeBuild CD (regressed since DefangLabs#422). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Sw7j9FYnbChZJbYArxbhuo
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughChangesRetained bucket adoption
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AwsCfn.SetUp
participant S3
participant findAdoptableBucket
participant CreateTemplate
AwsCfn.SetUp->>S3: List CD state buckets
S3-->>findAdoptableBucket: Return locations and tags
findAdoptableBucket-->>AwsCfn.SetUp: Return eligible bucket
AwsCfn.SetUp->>CreateTemplate: Pass existing bucket
CreateTemplate-->>AwsCfn.SetUp: Return adoption template
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pkg/clouds/aws/codebuild/cfn/setup.go`:
- Around line 215-223: Update AwsCfn.SetUp to determine whether the managed
stack already exists before calling findExistingBucket. Only perform
retained-bucket discovery and pass the discovered bucket to CreateTemplate on
the stack-creation path; for existing stacks, preserve the current bucket
configuration in the update template. Add coverage for updating a pre-existing
managed stack.
- Around line 178-183: Normalize the bucketRegion value in the bucket-retention
comparison before checking it against region: map S3’s “EU” LocationConstraint
to “eu-west-1”, while preserving the existing us-east-1 empty-value handling.
Add a test covering an EU response that matches eu-west-1 and confirms the
retained bucket is not duplicated.
- Around line 163-188: Update bucket discovery in setup.go around ListBuckets,
GetBucketLocation, and GetBucketTagging: return any GetBucketLocation error
instead of skipping the bucket, and treat only the S3 NoSuchTagSet error from
GetBucketTagging as a non-match while propagating all other tagging errors. In
bucket_test.go, update mockBucketLister to inject location and tagging errors
and add coverage for both failure paths.
🪄 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: 320d91e0-c165-4fa0-94bf-76d7286cab1a
📒 Files selected for processing (5)
src/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/clouds/aws/codebuild/cfn/bucket_test.gosrc/pkg/clouds/aws/codebuild/cfn/setup.gosrc/pkg/clouds/aws/codebuild/cfn/template.gosrc/pkg/clouds/aws/codebuild/cfn/template_test.go
…flag Per review, drop the automatic bucket discovery (ListBuckets + tag match): that approach mirrored DefangLabs#422, which was reverted, and relies on account-wide list/tagging permissions and unambiguous matching. Instead adopt a bucket only when the operator passes it explicitly: - add a `--bucket` flag to `defang cd install`; - ByocAws.SetCDBucket sets AwsCfn.ExistingBucket before SetUpCD; - SetUp passes ExistingBucket to CreateTemplate (which already skips the managed bucket and outputs the given name when adopting). Removes findAdoptableBucket and its unit test. The template-level adoption logic and its test are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Sw7j9FYnbChZJbYArxbhuo
Implement SetCDBucket on the GCP and DigitalOcean BYOC providers so the explicit adopt-bucket flag works for every object-storage CD backend: - GCP: ByocGcp adopts the given GCS bucket (skips the find-or-create in SetUpCD); CD service-account bucket roles are still applied to it. - DigitalOcean: ByocDo sets DoApp.BucketName, which GetBucketName already honors ahead of prefix-discovery/create. Azure is intentionally not supported: its CD state lives in a Storage Account + Blob Container, not a bucket, so `--bucket` doesn't map. The flag now errors with a clear message for unsupported providers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Sw7j9FYnbChZJbYArxbhuo
|
Addressed the review:
The template-level adoption logic (skip the managed |
DigitalOcean already read DEFANG_CD_BUCKET at construction, so the env var worked for every command there. AWS and GCP only honored the `cd install --bucket` flag. Read DEFANG_CD_BUCKET at construction for AWS and GCP too, so the env var adopts an existing CD bucket across all commands (up, cd down, etc.), and default the `--bucket` flag to $DEFANG_CD_BUCKET (matching the --allow-upgrade convention). The flag still overrides the env on `cd install`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Sw7j9FYnbChZJbYArxbhuo
|
Follow-ups pushed:
|
Problem
The CodeBuild CD driver (
src/pkg/clouds/aws/codebuild/cfn/) always creates a stack-managed, CFN-auto-named S3 bucket for Pulumi state, withRetainExceptOnCreate. Because the bucket is retained on stack delete but named per-stack-instance:defang cd teardowndeletes thedefang-cdstack → the bucket is retained (orphaned);defang-cd-bucket-*and the old one (plus its Pulumi state) is stranded.Repeated over time this leaves many orphaned buckets in an account (observed: 11
defang-cd-bucket-*in one account, only 3 backed by a live stack). This is the same problem as #358, which #422 fixed for the old ECS CD — but that logic (findExistingBucket+ a skip-bucket template override) did not survive the ECS→CodeBuild CD rewrite.Fix
Before creating the stack, look for a retained CD bucket in the same account+region — buckets named
<stack>-bucket-*carrying thedefang:Prefixtag equal to the stack name. If exactly one exists, generate the template without a managedBucketresource and emit that bucket as thebucketNameoutput; otherwise create a managed bucket as before.>1match errors rather than guessing.RetainExceptOnCreatemakes the one-time managed→adopted transition safe: when theBucketresource leaves the template on the next update, CloudFormation retains the physical bucket, and the stack now simply references it.template.go:CreateTemplate(stack, existingBucket string)— skips the managed bucket and outputs the existing name when adopting.setup.go:findAdoptableBucket()(ListBuckets → prefix + region +defang:Prefixtag) wired intoSetUp.byoc/aws/byoc.go) is unchanged — it always creates a managed bucket (existingBucket == "").Tests / verification
TestFindAdoptableBucket(mocked S3): none / one-match / wrong-region / wrong-tag / ambiguous.TestCreateTemplateBucketAdoption: managed variant creates the bucket; adopt variant has noBucketresource and outputs the existing name.go build ./...,go test -short ./pkg/clouds/aws/codebuild/... ./pkg/cli/client/byoc/aws/...,go vet,gofmt, andgolangci-linton the changed packages all pass (no new findings).Notes for reviewers
RetainBucketparameter +RetainS3Bucketcondition are inert (kept defined because the templateMetadata/ParameterGroups reference the parameter). Harmless, but could be gated later.//go:build integrationTestCloudFormationhere (needs live AWS). Worth exercising the adopt path against an account that already has a retaineddefang-cd-bucket-*.Fixes #2192. Re-fixes #358 for the CodeBuild CD (regressed since #422).
Summary by CodeRabbit
New Features
Bug Fixes
Tests