Conversation
Contributor
📝 WalkthroughWalkthroughMigrates BYOC/CD from ECS to AWS CodeBuild, adds CodeBuild implementations (run/status/tail, CFN template and tests), removes ECS drivers/templates and the clouds.Driver/crun abstractions and Docker/Local drivers, updates provider SetUpCD signatures, adjusts tests and Go module dependencies, and minor Makefile/Nix hash tweak. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "Defang CLI / BYOC client"
participant CFN as "CloudFormation (CreateTemplate/Upsert)"
participant CodeBuild as "AWS CodeBuild (Start/Stop/BatchGetBuilds)"
participant CW as "CloudWatch Logs"
participant S3 as "S3 (artifacts/bucket)"
rect rgba(200,200,255,0.5)
CLI->>CFN: Request template / SetUp (CreateTemplate(stack))
CFN->>S3: Ensure bucket & outputs
CFN-->>CLI: Return ProjectName / Outputs
end
rect rgba(200,255,200,0.5)
CLI->>CodeBuild: Start build (image, env, buildspec)
CodeBuild-->>CLI: Return BuildID
CLI->>CodeBuild: Poll status (BatchGetBuilds) / WaitForBuild
CodeBuild-->>CLI: Return status (IN_PROGRESS / SUCCEEDED / FAILED)
end
rect rgba(255,200,200,0.5)
CLI->>CW: Query/Tail logs for BuildID (log stream derived from BuildID)
CW-->>CLI: Stream log events
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/pkg/clouds/aws/codebuild/cfn/setup_test.go (1)
45-58: This integration test never exercises a successful build.It only proves that
StartBuildreturned an ID and then immediately stops the build. Broken runtime behavior—like an invalid buildspec or wrong working directory inRun—would still pass here. Please wait for one happy-path build to finish successfully before callingStop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/codebuild/cfn/setup_test.go` around lines 45 - 58, The test currently calls aws.Run and immediately aws.Stop without waiting for the build to complete; update the test around the taskid returned by aws.Run to poll the build status (using the project’s existing describe/status helper such as aws.DescribeBuild or aws.WaitBuild if available) until it reaches a terminal state, assert the terminal state is SUCCESS (fail the test on FAILURE/FAULT/TIMED_OUT/CANCELLED), and only call aws.Stop if the build is still running; use the same taskid variable and the test subtest "Stop" to ensure the happy-path build finishes before stopping.
🤖 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/clouds/aws/codebuild/cfn/template.go`:
- Around line 247-249: The role currently attaches the broad
"AdministratorAccess" in ManagedPolicyArns; replace this with a least-privilege
policy tailored to the CD flow: remove
"arn:aws:iam::aws:policy/AdministratorAccess" and instead attach only the
AWS-managed policies and/or a custom inline policy granting the specific S3
(GetObject/PutObject/ListBucket), ECR (GetAuthorizationToken, BatchGetImage,
InitiateLayerUpload, UploadLayerPart, CompleteLayerUpload), CloudWatch Logs
(CreateLogGroup, CreateLogStream, PutLogEvents), CloudFormation
(CreateStack/UpdateStack/DescribeStacks/DescribeChangeSet/DeleteStack if used)
and STS (AssumeRole) actions your build requires; update the role definition
that contains ManagedPolicyArns in template.go to reference those policies (or
an iam:Policy resource linked to the Role) and document which actions were
included. Ensure no use of AdministratorAccess remains.
- Line 272: The template sets ServiceRole using
cloudformation.Ref(_codeBuildServiceRole) which yields the role name but
AWS::CodeBuild::Project.ServiceRole requires the role ARN; change the value to
use the role's ARN via a GetAtt (e.g.,
cloudformation.GetAtt(_codeBuildServiceRole, "Arn") or the library equivalent)
so ServiceRole receives the IAM role ARN instead of the name.
In `@src/pkg/clouds/aws/codebuild/run.go`:
- Around line 26-30: The code collapses argv by using strings.Join(cmd, " ") and
hard-codes cd /app into buildspec; update the Run implementation to shell-quote
each element of cmd (e.g., use strconv.Quote or a shell-escaping helper on each
item) and join those quoted args to preserve argument boundaries, and stop
hard-coding /app: use the explicit workingDir parameter or field (validate it)
and build the buildspec to ensure the directory exists (e.g., mkdir -p
"<workingDir>" && cd "<workingDir>" && <quoted command>) before running the
command; change the BuildspecOverride construction to use the quoted command and
the validated workingDir variable instead of "/app".
In `@src/pkg/clouds/aws/codebuild/status.go`:
- Around line 17-47: The function calling BatchGetBuilds must handle the FAULT
terminal status and surface invalid build IDs: when output.Builds is empty check
output.BuildsNotFound and return a terminal BuildFailure (e.g., Reason "invalid
build ID") instead of continuing to poll, and add a case for
cbtypes.StatusTypeFault in the switch on build.BuildStatus that returns true and
BuildFailure{Reason: "build fault"}; update the switch handling around
build.BuildStatus and the nil/empty response logic so invalid IDs and FAULT are
treated as terminal failures.
---
Nitpick comments:
In `@src/pkg/clouds/aws/codebuild/cfn/setup_test.go`:
- Around line 45-58: The test currently calls aws.Run and immediately aws.Stop
without waiting for the build to complete; update the test around the taskid
returned by aws.Run to poll the build status (using the project’s existing
describe/status helper such as aws.DescribeBuild or aws.WaitBuild if available)
until it reaches a terminal state, assert the terminal state is SUCCESS (fail
the test on FAILURE/FAULT/TIMED_OUT/CANCELLED), and only call aws.Stop if the
build is still running; use the same taskid variable and the test subtest "Stop"
to ensure the happy-path build finishes before stopping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0352564-b181-4ae8-868b-7223bc2f9301
⛔ Files ignored due to path filters (1)
src/go.sumis excluded by!**/*.sum
📒 Files selected for processing (63)
Makefilepkgs/defang/cli.nixsrc/cmd/crun/main.gosrc/go.modsrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/aws/byoc_integration_test.gosrc/pkg/cli/client/byoc/aws/byoc_test.gosrc/pkg/cli/client/byoc/aws/cd.gosrc/pkg/cli/client/byoc/aws/validation_test.gosrc/pkg/cli/preview_test.gosrc/pkg/cli/tail_test.gosrc/pkg/cli/waitForCdTaskExit_test.gosrc/pkg/clouds/aws/codebuild/cfn/oidc.gosrc/pkg/clouds/aws/codebuild/cfn/oidc_test.gosrc/pkg/clouds/aws/codebuild/cfn/oidcprovider.gosrc/pkg/clouds/aws/codebuild/cfn/outputs.gosrc/pkg/clouds/aws/codebuild/cfn/params.gosrc/pkg/clouds/aws/codebuild/cfn/quick_create.gosrc/pkg/clouds/aws/codebuild/cfn/quick_create_test.gosrc/pkg/clouds/aws/codebuild/cfn/setup.gosrc/pkg/clouds/aws/codebuild/cfn/setup_test.gosrc/pkg/clouds/aws/codebuild/cfn/template.gosrc/pkg/clouds/aws/codebuild/cfn/template_test.gosrc/pkg/clouds/aws/codebuild/cfn/testdata/template.yamlsrc/pkg/clouds/aws/codebuild/cfn/waiter.gosrc/pkg/clouds/aws/codebuild/common.gosrc/pkg/clouds/aws/codebuild/run.gosrc/pkg/clouds/aws/codebuild/status.gosrc/pkg/clouds/aws/codebuild/tail.gosrc/pkg/clouds/aws/codebuild/upload.gosrc/pkg/clouds/aws/ecs/cfn/outputs.gosrc/pkg/clouds/aws/ecs/cfn/template.gosrc/pkg/clouds/aws/ecs/common.gosrc/pkg/clouds/aws/ecs/common_test.gosrc/pkg/clouds/aws/ecs/fargate.gosrc/pkg/clouds/aws/ecs/fargate_test.gosrc/pkg/clouds/aws/ecs/info.gosrc/pkg/clouds/aws/ecs/run.gosrc/pkg/clouds/aws/ecs/status.gosrc/pkg/clouds/aws/ecs/status_test.gosrc/pkg/clouds/aws/ecs/stop.gosrc/pkg/clouds/aws/ecs/tail.gosrc/pkg/clouds/aws/ecs/tail_test.gosrc/pkg/clouds/driver.gosrc/pkg/clouds/gcp/cloudrun.gosrc/pkg/crun/common.gosrc/pkg/crun/common_test.gosrc/pkg/crun/destroy.gosrc/pkg/crun/docker/common.gosrc/pkg/crun/docker/info.gosrc/pkg/crun/docker/run.gosrc/pkg/crun/docker/run_test.gosrc/pkg/crun/docker/setup.gosrc/pkg/crun/docker/stop.gosrc/pkg/crun/docker/tail.gosrc/pkg/crun/docker/teardown.gosrc/pkg/crun/factory.gosrc/pkg/crun/info.gosrc/pkg/crun/local/local.gosrc/pkg/crun/local/local_test.gosrc/pkg/crun/logs.gosrc/pkg/crun/run.gosrc/pkg/crun/stop.go
💤 Files with no reviewable changes (36)
- src/pkg/crun/info.go
- src/pkg/clouds/aws/codebuild/cfn/params.go
- src/pkg/crun/destroy.go
- src/pkg/clouds/gcp/cloudrun.go
- src/pkg/cli/client/byoc/aws/cd.go
- src/pkg/crun/docker/teardown.go
- src/pkg/clouds/aws/ecs/cfn/outputs.go
- src/pkg/crun/common.go
- src/cmd/crun/main.go
- src/pkg/crun/docker/run_test.go
- src/pkg/clouds/aws/ecs/stop.go
- src/pkg/crun/docker/info.go
- src/pkg/clouds/aws/ecs/tail.go
- src/pkg/crun/factory.go
- src/pkg/clouds/aws/ecs/common_test.go
- src/pkg/clouds/aws/ecs/status.go
- src/pkg/clouds/aws/ecs/common.go
- src/pkg/clouds/aws/ecs/run.go
- src/pkg/crun/logs.go
- src/pkg/clouds/aws/ecs/status_test.go
- src/pkg/clouds/aws/ecs/fargate_test.go
- src/pkg/crun/common_test.go
- src/pkg/clouds/aws/ecs/fargate.go
- src/pkg/clouds/aws/ecs/tail_test.go
- src/pkg/crun/stop.go
- src/pkg/crun/docker/stop.go
- src/pkg/crun/docker/tail.go
- src/pkg/crun/local/local.go
- src/pkg/clouds/driver.go
- src/pkg/clouds/aws/ecs/info.go
- src/pkg/crun/local/local_test.go
- src/pkg/clouds/aws/ecs/cfn/template.go
- src/pkg/crun/docker/run.go
- src/pkg/crun/docker/setup.go
- src/pkg/crun/run.go
- src/pkg/crun/docker/common.go
Comment on lines
+26
to
+30
| // Build the command to run; the buildspec executes it directly | ||
| command := strings.Join(cmd, " ") | ||
|
|
||
| // CodeBuild overrides the image's WORKDIR; use `cd` to restore it before running the command | ||
| buildspec := "version: 0.2\nphases:\n build:\n commands:\n - cd /app && " + command + "\n" |
Contributor
There was a problem hiding this comment.
Don't collapse argv and image assumptions into one shell string.
strings.Join(cmd, " ") loses argument boundaries, and the hard-coded cd /app makes Run fail for any image that doesn't create that directory. Since Run accepts arbitrary images, shell-quote each arg and make the working directory explicit/validated instead of baking both assumptions into BuildspecOverride.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/clouds/aws/codebuild/run.go` around lines 26 - 30, The code collapses
argv by using strings.Join(cmd, " ") and hard-codes cd /app into buildspec;
update the Run implementation to shell-quote each element of cmd (e.g., use
strconv.Quote or a shell-escaping helper on each item) and join those quoted
args to preserve argument boundaries, and stop hard-coding /app: use the
explicit workingDir parameter or field (validate it) and build the buildspec to
ensure the directory exists (e.g., mkdir -p "<workingDir>" && cd "<workingDir>"
&& <quoted command>) before running the command; change the BuildspecOverride
construction to use the quoted command and the validated workingDir variable
instead of "/app".
Comment on lines
+17
to
+47
| output, err := client.BatchGetBuilds(ctx, &cb.BatchGetBuildsInput{ | ||
| Ids: []string{*buildID}, | ||
| }) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| if len(output.Builds) == 0 { | ||
| return false, nil // build doesn't exist yet | ||
| } | ||
|
|
||
| build := output.Builds[0] | ||
| switch build.BuildStatus { | ||
| case cbtypes.StatusTypeSucceeded: | ||
| return true, io.EOF | ||
| case cbtypes.StatusTypeFailed: | ||
| reason := "build failed" | ||
| if build.BuildComplete && len(build.Phases) > 0 { | ||
| for _, phase := range build.Phases { | ||
| if phase.PhaseStatus == cbtypes.StatusTypeFailed && len(phase.Contexts) > 0 { | ||
| reason = *phase.Contexts[0].Message | ||
| break | ||
| } | ||
| } | ||
| } | ||
| return true, BuildFailure{Reason: reason} | ||
| case cbtypes.StatusTypeStopped: | ||
| return true, BuildFailure{Reason: "build stopped"} | ||
| case cbtypes.StatusTypeTimedOut: | ||
| return true, BuildFailure{Reason: "build timed out"} | ||
| default: | ||
| return false, nil // still running (IN_PROGRESS) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In AWS CodeBuild, how does BatchGetBuilds report unknown build IDs, and which BuildStatus values are terminal (for example, is FAULT terminal)?
💡 Result:
1) Unknown build IDs in BatchGetBuilds
BatchGetBuilds returns build IDs it can’t find in the buildsNotFound array (while still returning HTTP 200 if the call itself is valid). The found builds (if any) are returned in builds. [1]
2) Terminal BuildStatus values
For a Build, buildStatus can be: SUCCEEDED | FAILED | FAULT | TIMED_OUT | IN_PROGRESS | STOPPED. [2]
In practice, the terminal statuses are:
SUCCEEDED,FAILED,FAULT,TIMED_OUT,STOPPED(i.e., everything exceptIN_PROGRESS).FAULTis terminal. [2][3]
You can also treat buildComplete == true as the definitive “terminal/completed” signal. [2]
Sources:
[1] AWS CodeBuild API Reference – BatchGetBuilds (response includes buildsNotFound)
[2] AWS CodeBuild API Reference – Build (buildComplete, buildStatus valid values)
[3] AWS CodeBuild SLA (defines “fault” as a terminal state)
Handle FAULT terminal status and distinguish invalid build IDs from builds still in progress.
The code misses the FAULT terminal status (one of five terminal states in CodeBuild: SUCCEEDED, FAILED, FAULT, TIMED_OUT, STOPPED). Additionally, when BatchGetBuilds returns zero builds, the code cannot distinguish between a build not yet available (should return false, nil to continue polling) and an invalid build ID (which appears in the response's buildsNotFound array). Check buildsNotFound to surface invalid IDs as errors rather than spinning indefinitely.
Suggested handling:
output, err := client.BatchGetBuilds(ctx, &cb.BatchGetBuildsInput{
Ids: []string{*buildID},
})
if err != nil {
return false, err
}
if len(output.Builds) == 0 {
if len(output.BuildsNotFound) > 0 {
return true, BuildFailure{Reason: "invalid build ID"}
}
return false, nil // build doesn't exist yet
}
build := output.Builds[0]
switch build.BuildStatus {
case cbtypes.StatusTypeSucceeded:
return true, io.EOF
case cbtypes.StatusTypeFailed:
// ... existing failure handling
case cbtypes.StatusTypeFault:
return true, BuildFailure{Reason: "build fault"}
case cbtypes.StatusTypeStopped:
return true, BuildFailure{Reason: "build stopped"}
case cbtypes.StatusTypeTimedOut:
return true, BuildFailure{Reason: "build timed out"}
default:
return false, nil // IN_PROGRESS
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/clouds/aws/codebuild/status.go` around lines 17 - 47, The function
calling BatchGetBuilds must handle the FAULT terminal status and surface invalid
build IDs: when output.Builds is empty check output.BuildsNotFound and return a
terminal BuildFailure (e.g., Reason "invalid build ID") instead of continuing to
poll, and add a case for cbtypes.StatusTypeFault in the switch on
build.BuildStatus that returns true and BuildFailure{Reason: "build fault"};
update the switch handling around build.BuildStatus and the nil/empty response
logic so invalid IDs and FAULT are treated as terminal failures.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/aws/byoc.go`:
- Around line 168-179: In GetDeploymentStatus, map AWS errors to the friendly
annotated form: when b.driver.LoadConfig(ctx) returns an error, wrap and return
it using AnnotateAwsError before returning; likewise, after calling
awscodebuild.GetBuildStatus(ctx, cfg, b.cdBuildId), if err is non-nil and does
not match awscodebuild.BuildFailure, wrap that err with AnnotateAwsError and
return it; preserve the existing handling that converts BuildFailure into
client.ErrDeploymentFailed and keep returning done on success.
In `@src/pkg/clouds/aws/codebuild/cfn/setup.go`:
- Around line 212-227: The fillWithOutputs method on AwsCfn leaves previous
values for conditional outputs (e.g., CIRoleARN) when the stack no longer
exports them; modify fillWithOutputs to reset/clear the target fields (at least
AwsCfn.LogGroupARN, AwsCfn.BucketName, AwsCfn.CIRoleARN, AwsCfn.ProjectName) to
empty strings before iterating dso.Stacks[0].Outputs, then populate them from
outputs as currently implemented so a reused AwsCfn cannot retain stale
conditional values.
In `@src/pkg/clouds/aws/codebuild/cfn/testdata/template.yaml`:
- Around line 62-63: The template currently double-prefixes OidcProviderIssuer:
stop adding a hardcoded "https://" and use the OidcProviderIssuer parameter
value consistently as the full URL; update the template generator code that
builds the OIDC provider (where it currently prepends "https://") to use the raw
OidcProviderIssuer parameter instead, and ensure the same exact parameter value
is used when constructing IAM condition keys (the `${issuer}:aud` /
`${issuer}:sub` substitutions) so they match the provider host; after making
those changes re-generate the golden template.yaml so the provider ARN and
condition keys no longer contain duplicated "https://".
- Around line 128-150: The Bucket resource in the CloudFormation template is
missing PublicAccessBlockConfiguration; update the template generator to add
PublicAccessBlockConfiguration to the Bucket (resource named "Bucket") with the
four flags BlockPublicAcls, IgnorePublicAcls, BlockPublicPolicy, and
RestrictPublicBuckets all set true, then re-generate the golden file so the
testdata/template.yaml includes this PublicAccessBlockConfiguration for the
AWS::S3::Bucket.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3628f256-18e1-4ee3-bbc1-14f48f79c85f
⛔ Files ignored due to path filters (1)
src/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
src/go.modsrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/clouds/aws/codebuild/cfn/outputs.gosrc/pkg/clouds/aws/codebuild/cfn/params.gosrc/pkg/clouds/aws/codebuild/cfn/setup.gosrc/pkg/clouds/aws/codebuild/cfn/template.gosrc/pkg/clouds/aws/codebuild/cfn/template_test.gosrc/pkg/clouds/aws/codebuild/cfn/testdata/template.yamlsrc/pkg/clouds/aws/codebuild/common.go
💤 Files with no reviewable changes (1)
- src/pkg/clouds/aws/codebuild/cfn/params.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pkg/clouds/aws/codebuild/cfn/template.go
Comment on lines
168
to
179
| func (b *ByocAws) GetDeploymentStatus(ctx context.Context) (bool, error) { | ||
| done, err := b.driver.GetTaskStatus(ctx, b.cdTaskArn) | ||
| cfg, err := b.driver.LoadConfig(ctx) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| done, err := awscodebuild.GetBuildStatus(ctx, cfg, b.cdBuildId) | ||
| if err != nil { | ||
| // check if the task failed; if so, return the a ErrDeploymentFailed error | ||
| if taskErr := new(ecs.TaskFailure); errors.As(err, taskErr) { | ||
| return done, client.ErrDeploymentFailed{Message: taskErr.Error()} | ||
| if buildErr := new(awscodebuild.BuildFailure); errors.As(err, buildErr) { | ||
| return done, client.ErrDeploymentFailed{Message: buildErr.Error()} | ||
| } | ||
| return done, err | ||
| } |
Contributor
There was a problem hiding this comment.
Keep AWS error annotation on the build-status path.
This now returns raw config/SDK errors from LoadConfig and GetBuildStatus, so missing region/credentials regress from the friendly AWS-specific messages used elsewhere in this provider. Please wrap the non-BuildFailure errors with AnnotateAwsError.
🛠 Proposed change
func (b *ByocAws) GetDeploymentStatus(ctx context.Context) (bool, error) {
cfg, err := b.driver.LoadConfig(ctx)
if err != nil {
- return false, err
+ return false, AnnotateAwsError(err)
}
done, err := awscodebuild.GetBuildStatus(ctx, cfg, b.cdBuildId)
if err != nil {
if buildErr := new(awscodebuild.BuildFailure); errors.As(err, buildErr) {
return done, client.ErrDeploymentFailed{Message: buildErr.Error()}
}
- return done, err
+ return done, AnnotateAwsError(err)
}
return done, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/cli/client/byoc/aws/byoc.go` around lines 168 - 179, In
GetDeploymentStatus, map AWS errors to the friendly annotated form: when
b.driver.LoadConfig(ctx) returns an error, wrap and return it using
AnnotateAwsError before returning; likewise, after calling
awscodebuild.GetBuildStatus(ctx, cfg, b.cdBuildId), if err is non-nil and does
not match awscodebuild.BuildFailure, wrap that err with AnnotateAwsError and
return it; preserve the existing handling that converts BuildFailure into
client.ErrDeploymentFailed and keep returning done on success.
Comment on lines
+212
to
227
| func (a *AwsCfn) fillWithOutputs(dso *cloudformation.DescribeStacksOutput) error { | ||
| if len(dso.Stacks) != 1 { | ||
| return fmt.Errorf("expected 1 CloudFormation stack, got %d", len(dso.Stacks)) | ||
| } | ||
| for _, output := range dso.Stacks[0].Outputs { | ||
| switch *output.OutputKey { | ||
| case OutputsSubnetID: | ||
| // Only set the SubNetID if it's not already set; this allows the user to override the subnet | ||
| if a.SubNetID == "" { | ||
| a.SubNetID = *output.OutputValue | ||
| } | ||
| case OutputsDefaultSecurityGroupID: | ||
| a.DefaultSecurityGroupID = *output.OutputValue | ||
| case OutputsTaskDefARN: | ||
| a.TaskDefARN = *output.OutputValue | ||
| case OutputsClusterName: | ||
| a.ClusterName = *output.OutputValue | ||
| case OutputsLogGroupARN: | ||
| a.LogGroupARN = *output.OutputValue | ||
| case OutputsSecurityGroupID: | ||
| a.SecurityGroupID = *output.OutputValue | ||
| case OutputsBucketName: | ||
| a.BucketName = *output.OutputValue | ||
| case OutputsCIRoleARN: | ||
| a.CIRoleARN = *output.OutputValue | ||
| case OutputsCodeBuildProjectName: | ||
| a.ProjectName = *output.OutputValue | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Clear conditional outputs before rehydrating the driver.
ciRoleArn is conditional in the template, but fillWithOutputs only overwrites keys that are present. On a reused AwsCfn, disabling OIDC leaves the old CIRoleARN in memory, so later commands can still act on a role that the stack no longer exports.
🧹 Proposed change
func (a *AwsCfn) fillWithOutputs(dso *cloudformation.DescribeStacksOutput) error {
if len(dso.Stacks) != 1 {
return fmt.Errorf("expected 1 CloudFormation stack, got %d", len(dso.Stacks))
}
+ a.BucketName = ""
+ a.CIRoleARN = ""
+ a.LogGroupARN = ""
+ a.ProjectName = ""
for _, output := range dso.Stacks[0].Outputs {
switch *output.OutputKey {
case OutputsLogGroupARN:
a.LogGroupARN = *output.OutputValue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/clouds/aws/codebuild/cfn/setup.go` around lines 212 - 227, The
fillWithOutputs method on AwsCfn leaves previous values for conditional outputs
(e.g., CIRoleARN) when the stack no longer exports them; modify fillWithOutputs
to reset/clear the target fields (at least AwsCfn.LogGroupARN,
AwsCfn.BucketName, AwsCfn.CIRoleARN, AwsCfn.ProjectName) to empty strings before
iterating dso.Stacks[0].Outputs, then populate them from outputs as currently
implemented so a reused AwsCfn cannot retain stale conditional values.
Comment on lines
+62
to
+63
| OidcProviderIssuer: | ||
| default: OIDC Provider Issuer URL |
Contributor
There was a problem hiding this comment.
Don’t ask for an issuer URL and then prepend https:// again.
OidcProviderIssuer is documented as a URL, but Line 364 always prefixes it with https://, and Lines 167-170 also use the raw value in IAM condition keys. A user entering a normal issuer URL will end up with https://https://... for the provider and broken ${issuer}:aud / ${issuer}:sub conditions. Please fix this in the template generator and then re-generate the golden file.
Also applies to: 108-110, 167-170, 363-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/clouds/aws/codebuild/cfn/testdata/template.yaml` around lines 62 -
63, The template currently double-prefixes OidcProviderIssuer: stop adding a
hardcoded "https://" and use the OidcProviderIssuer parameter value consistently
as the full URL; update the template generator code that builds the OIDC
provider (where it currently prepends "https://") to use the raw
OidcProviderIssuer parameter instead, and ensure the same exact parameter value
is used when constructing IAM condition keys (the `${issuer}:aud` /
`${issuer}:sub` substitutions) so they match the provider host; after making
those changes re-generate the golden template.yaml so the provider ARN and
condition keys no longer contain duplicated "https://".
Comment on lines
+128
to
+150
| Bucket: | ||
| DeletionPolicy: | ||
| Fn::If: | ||
| - RetainS3Bucket | ||
| - RetainExceptOnCreate | ||
| - Delete | ||
| Properties: | ||
| Tags: | ||
| - Key: defang:CreatedBy | ||
| Value: defang | ||
| - Key: defang:Prefix | ||
| Value: test | ||
| - Key: defang:ManagedBy | ||
| Value: CloudFormation | ||
| - Key: defang:CloudFormationStackName | ||
| Value: | ||
| Ref: AWS::StackName | ||
| - Key: defang:CloudFormationStackRegion | ||
| Value: | ||
| Ref: AWS::Region | ||
| VersioningConfiguration: | ||
| Status: Enabled | ||
| Type: AWS::S3::Bucket |
Contributor
There was a problem hiding this comment.
Block public access on the state bucket.
This generated bucket stores deployment state, so leaving out PublicAccessBlockConfiguration weakens the default safety boundary. Please add the four public-access-block flags in the template generator and re-generate this golden file.
🔒 Proposed change
Bucket:
DeletionPolicy:
Fn::If:
- RetainS3Bucket
- RetainExceptOnCreate
- Delete
Properties:
+ PublicAccessBlockConfiguration:
+ BlockPublicAcls: true
+ BlockPublicPolicy: true
+ IgnorePublicAcls: true
+ RestrictPublicBuckets: true
Tags:
- Key: defang:CreatedBy
Value: defang🧰 Tools
🪛 Checkov (3.2.508)
[medium] 128-150: Ensure S3 bucket has block public ACLs enabled
(CKV_AWS_53)
[medium] 128-150: Ensure S3 bucket has block public policy enabled
(CKV_AWS_54)
[medium] 128-150: Ensure S3 bucket has ignore public ACLs enabled
(CKV_AWS_55)
[medium] 128-150: Ensure S3 bucket has RestrictPublicBuckets enabled
(CKV_AWS_56)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/clouds/aws/codebuild/cfn/testdata/template.yaml` around lines 128 -
150, The Bucket resource in the CloudFormation template is missing
PublicAccessBlockConfiguration; update the template generator to add
PublicAccessBlockConfiguration to the Bucket (resource named "Bucket") with the
four flags BlockPublicAcls, IgnorePublicAcls, BlockPublicPolicy, and
RestrictPublicBuckets all set true, then re-generate the golden file so the
testdata/template.yaml includes this PublicAccessBlockConfiguration for the
AWS::S3::Bucket.
allows us to downgrade CFN stack
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
src/pkg/clouds/aws/codebuild/status.go (1)
17-25:⚠️ Potential issue | 🔴 CriticalHandle invalid build IDs and
FAULTas terminal states.Line 23 currently treats
len(output.Builds)==0as “still running,” and the switch omitsStatusTypeFault. Both can cause indefinite polling for terminal failures.Suggested fix
output, err := client.BatchGetBuilds(ctx, &cb.BatchGetBuildsInput{ Ids: []string{*buildID}, }) if err != nil { return false, err } if len(output.Builds) == 0 { - return false, nil // build doesn't exist yet + if len(output.BuildsNotFound) > 0 { + return true, BuildFailure{Reason: "invalid build ID"} + } + return false, nil // not visible yet } @@ case cbtypes.StatusTypeTimedOut: return true, BuildFailure{Reason: "build timed out"} + case cbtypes.StatusTypeFault: + return true, BuildFailure{Reason: "build fault"} default: return false, nil // still running (IN_PROGRESS) }In AWS CodeBuild BatchGetBuilds, when is an ID returned in buildsNotFound, and is buildStatus=FAULT terminal?Also applies to: 28-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/codebuild/status.go` around lines 17 - 25, The current BatchGetBuilds handling treats missing builds (len(output.Builds) == 0) as non-terminal and also omits FAULT from terminal statuses; update the client.BatchGetBuilds handling to check output.BuildsNotFound (or when len(output.Builds) == 0) and treat that case as a terminal error (return terminal=true and an appropriate error), and add cb.StatusTypeFault to the switch of terminal statuses so FAULT returns terminal=true; locate the logic around client.BatchGetBuilds, output.Builds / output.BuildsNotFound and the switch on build.Status and apply these changes.src/pkg/clouds/aws/codebuild/cfn/testdata/template.yaml (2)
128-150:⚠️ Potential issue | 🟠 MajorS3 bucket is still missing explicit public-access blocking.
Please add
PublicAccessBlockConfigurationwith all four controls enabled onBucket.🔒 Suggested template update
Bucket: DeletionPolicy: Fn::If: - RetainS3Bucket - RetainExceptOnCreate - Delete Properties: + PublicAccessBlockConfiguration: + BlockPublicAcls: true + BlockPublicPolicy: true + IgnorePublicAcls: true + RestrictPublicBuckets: true Tags: - Key: defang:CreatedBy Value: defang🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/codebuild/cfn/testdata/template.yaml` around lines 128 - 150, The S3 Bucket resource named "Bucket" is missing an explicit PublicAccessBlockConfiguration; under the Bucket resource's Properties (resource symbol: Bucket) add the PublicAccessBlockConfiguration property and set its four controls—BlockPublicAcls, IgnorePublicAcls, BlockPublicPolicy, and RestrictPublicBuckets—to true to ensure public access is blocked by default.
62-63:⚠️ Potential issue | 🟠 MajorOIDC issuer handling is still inconsistent and can break role assumption.
The parameter is labeled as a URL, but the template both prefixes
https://and reuses the same value for IAM condition keys. This can produce invalid provider URLs or mismatched${issuer}:aud/${issuer}:subkeys depending on input format.Also applies to: 108-110, 167-170, 363-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/codebuild/cfn/testdata/template.yaml` around lines 62 - 63, The OidcProviderIssuer parameter is inconsistently treated as both a full URL and a raw issuer string (used with a prefixed "https://" and directly in IAM condition keys like `${OidcProviderIssuer}:aud`), which can produce invalid provider ARNs or condition keys; update the CloudFormation template to normalize the value once and reuse that normalized form: define a single sanitized logical value (e.g., OidcProviderIssuerNormalized) by stripping any leading "https://" if present or by requiring callers to supply the issuer without scheme, then construct the provider URL as "https://{OidcProviderIssuerNormalized}" wherever the full URL is required and use `{OidcProviderIssuerNormalized}` for IAM condition keys (`:aud`, `:sub`) so the same canonical issuer is used consistently across OidcProviderIssuer usages.src/pkg/clouds/aws/codebuild/cfn/template.go (2)
241-241:⚠️ Potential issue | 🔴 CriticalUse the IAM role ARN for
Project.ServiceRole(notRef).On Line 241,
cloudformation.Ref(_codeBuildServiceRole)resolves to role name, whileAWS::CodeBuild::Project.ServiceRoleexpects an ARN.💡 Proposed fix
- ServiceRole: cloudformation.Ref(_codeBuildServiceRole), + ServiceRole: cloudformation.GetAtt(_codeBuildServiceRole, "Arn"),For AWS CloudFormation, what does Ref return for AWS::IAM::Role, and what value type is required by AWS::CodeBuild::Project ServiceRole?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/codebuild/cfn/template.go` at line 241, The ServiceRole for the CodeBuild Project is using cloudformation.Ref(_codeBuildServiceRole) which yields the IAM role name, but AWS::CodeBuild::Project.ServiceRole requires the role's ARN; update the template to pass the role ARN by replacing the Ref with cloudformation.GetAtt(_codeBuildServiceRole, "Arn") (or equivalent GetAtt call) where Project.ServiceRole is set so the property receives the IAM Role ARN instead of the name.
167-169:⚠️ Potential issue | 🟠 MajorBoth runtime roles are still overly broad (
PowerUserAccess).This keeps a large account-level blast radius. Please replace with scoped policies for the exact CD/CI actions required.
Also applies to: 295-297
src/pkg/clouds/aws/codebuild/cfn/setup.go (1)
212-227:⚠️ Potential issue | 🟡 MinorClear cached outputs before rehydrating from stack outputs.
Without reset, conditional outputs can leave stale values in memory (notably
CIRoleARN) after stack updates.🧹 Proposed fix
func (a *AwsCfn) fillWithOutputs(dso *cloudformation.DescribeStacksOutput) error { if len(dso.Stacks) != 1 { return fmt.Errorf("expected 1 CloudFormation stack, got %d", len(dso.Stacks)) } + a.LogGroupARN = "" + a.BucketName = "" + a.CIRoleARN = "" + a.ProjectName = "" for _, output := range dso.Stacks[0].Outputs { switch *output.OutputKey {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/codebuild/cfn/setup.go` around lines 212 - 227, The fillWithOutputs method can leave stale values when some outputs are absent; before iterating stack outputs in AwsCfn.fillWithOutputs, reset the target fields (e.g. a.LogGroupARN, a.BucketName, a.CIRoleARN, a.ProjectName) to their zero values (empty strings) so conditional/missing outputs don't preserve previous values, then populate them from dso.Stacks[0].Outputs as currently implemented.src/pkg/cli/client/byoc/aws/byoc.go (1)
169-172:⚠️ Potential issue | 🟡 MinorReapply AWS error annotation in deployment-status path.
Line 171 and Line 178 currently return raw errors, which regresses the friendly AWS diagnostics used elsewhere.
🛠️ Proposed fix
cfg, err := b.driver.LoadConfig(ctx) if err != nil { - return false, err + return false, AnnotateAwsError(err) } done, err := awscodebuild.GetBuildStatus(ctx, cfg, b.cdBuildId) if err != nil { if buildErr := new(awscodebuild.BuildFailure); errors.As(err, buildErr) { return done, client.ErrDeploymentFailed{Message: buildErr.Error()} } - return done, err + return done, AnnotateAwsError(err) }Also applies to: 178-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/client/byoc/aws/byoc.go` around lines 169 - 172, The raw errors returned after calling b.driver.LoadConfig(ctx) (and the similar return at the deployment-status path) need to be wrapped with the project’s AWS diagnostics helper instead of returning err directly; replace the plain "return false, err" occurrences (the one after b.driver.LoadConfig(ctx) and the other in the deployment-status path) with a wrapped/annotated error using the same helper used elsewhere (e.g., call the existing annotateAWSError/WrapAWSError function or the b.annotateAWSError(err) helper) so the returned error includes the friendly AWS diagnostic info.
🤖 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/do/byoc.go`:
- Around line 674-677: The SetUpCD method currently returns early whenever
b.SetupDone is true, making the force parameter a no-op; change the early-return
check in SetUpCD to only return when setup is done and force is false (e.g., if
b.SetupDone && !force { return nil }) so that calling SetUpCD(ctx, true) will
bypass the guard and re-run the setup; ensure any stateful steps inside SetUpCD
(references to b.SetupDone and subsequent setup logic) are safe to re-execute or
reset b.SetupDone appropriately before/after the forced run.
In `@src/pkg/clouds/aws/codebuild/cfn/testdata/template.yaml`:
- Around line 216-252: The IAM policy CodeBuildIAMPolicy and the
CodeBuildServiceRole grant overly broad privileges (Resource: '*' and
PowerUserAccess); narrow them by removing unneeded iam mutation actions from the
PolicyDocument (only keep actions CodeBuild actually needs), replace Resource:
'*' with specific ARNs (e.g., the exact role, instance profile, S3 buckets, ECR
repos, CloudWatch log groups) and scope iam:PassRole to the specific execution
role ARN, and remove PowerUserAccess from CodeBuildServiceRole in favor of
least-privilege managed policies (e.g., AWSCodeBuildServiceRole or custom
policies limited to S3, ECR, CloudWatch and the specific IAM role). Ensure
CodeBuildIAMPolicy only contains the precise actions referenced by the build
project (attach/detach only if used) and reference the exact resource ARNs for
those actions.
In `@src/pkg/clouds/aws/codebuild/status.go`:
- Line 18: Guard against nil dereferences for buildID and phase context
messages: before using *buildID when constructing the Ids slice, check buildID
!= nil and return or handle the error if nil; likewise when accessing
phase.Contexts[0].Message ensure phase.Contexts is non-empty and
phase.Contexts[0].Message != nil (or use a safe accessor) before dereferencing,
and provide a sensible fallback or error path. Update the code paths around the
Ids: []string{*buildID} usage and the logic that reads phase.Contexts[0].Message
so both validate pointers/lengths and handle failure cases without panicking.
---
Duplicate comments:
In `@src/pkg/cli/client/byoc/aws/byoc.go`:
- Around line 169-172: The raw errors returned after calling
b.driver.LoadConfig(ctx) (and the similar return at the deployment-status path)
need to be wrapped with the project’s AWS diagnostics helper instead of
returning err directly; replace the plain "return false, err" occurrences (the
one after b.driver.LoadConfig(ctx) and the other in the deployment-status path)
with a wrapped/annotated error using the same helper used elsewhere (e.g., call
the existing annotateAWSError/WrapAWSError function or the
b.annotateAWSError(err) helper) so the returned error includes the friendly AWS
diagnostic info.
In `@src/pkg/clouds/aws/codebuild/cfn/setup.go`:
- Around line 212-227: The fillWithOutputs method can leave stale values when
some outputs are absent; before iterating stack outputs in
AwsCfn.fillWithOutputs, reset the target fields (e.g. a.LogGroupARN,
a.BucketName, a.CIRoleARN, a.ProjectName) to their zero values (empty strings)
so conditional/missing outputs don't preserve previous values, then populate
them from dso.Stacks[0].Outputs as currently implemented.
In `@src/pkg/clouds/aws/codebuild/cfn/template.go`:
- Line 241: The ServiceRole for the CodeBuild Project is using
cloudformation.Ref(_codeBuildServiceRole) which yields the IAM role name, but
AWS::CodeBuild::Project.ServiceRole requires the role's ARN; update the template
to pass the role ARN by replacing the Ref with
cloudformation.GetAtt(_codeBuildServiceRole, "Arn") (or equivalent GetAtt call)
where Project.ServiceRole is set so the property receives the IAM Role ARN
instead of the name.
In `@src/pkg/clouds/aws/codebuild/cfn/testdata/template.yaml`:
- Around line 128-150: The S3 Bucket resource named "Bucket" is missing an
explicit PublicAccessBlockConfiguration; under the Bucket resource's Properties
(resource symbol: Bucket) add the PublicAccessBlockConfiguration property and
set its four controls—BlockPublicAcls, IgnorePublicAcls, BlockPublicPolicy, and
RestrictPublicBuckets—to true to ensure public access is blocked by default.
- Around line 62-63: The OidcProviderIssuer parameter is inconsistently treated
as both a full URL and a raw issuer string (used with a prefixed "https://" and
directly in IAM condition keys like `${OidcProviderIssuer}:aud`), which can
produce invalid provider ARNs or condition keys; update the CloudFormation
template to normalize the value once and reuse that normalized form: define a
single sanitized logical value (e.g., OidcProviderIssuerNormalized) by stripping
any leading "https://" if present or by requiring callers to supply the issuer
without scheme, then construct the provider URL as
"https://{OidcProviderIssuerNormalized}" wherever the full URL is required and
use `{OidcProviderIssuerNormalized}` for IAM condition keys (`:aud`, `:sub`) so
the same canonical issuer is used consistently across OidcProviderIssuer usages.
In `@src/pkg/clouds/aws/codebuild/status.go`:
- Around line 17-25: The current BatchGetBuilds handling treats missing builds
(len(output.Builds) == 0) as non-terminal and also omits FAULT from terminal
statuses; update the client.BatchGetBuilds handling to check
output.BuildsNotFound (or when len(output.Builds) == 0) and treat that case as a
terminal error (return terminal=true and an appropriate error), and add
cb.StatusTypeFault to the switch of terminal statuses so FAULT returns
terminal=true; locate the logic around client.BatchGetBuilds, output.Builds /
output.BuildsNotFound and the switch on build.Status and apply these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34ae1620-1176-47b9-8ea0-36d061137d30
⛔ Files ignored due to path filters (1)
src/go.sumis excluded by!**/*.sum
📒 Files selected for processing (73)
Makefilepkgs/defang/cli.nixsrc/cmd/cli/command/cd.gosrc/cmd/cli/command/commands.gosrc/cmd/crun/main.gosrc/go.modsrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/aws/byoc_integration_test.gosrc/pkg/cli/client/byoc/aws/byoc_test.gosrc/pkg/cli/client/byoc/aws/cd.gosrc/pkg/cli/client/byoc/aws/validation_test.gosrc/pkg/cli/client/byoc/do/byoc.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/byoc/gcp/byoc_test.gosrc/pkg/cli/client/playground.gosrc/pkg/cli/client/provider.gosrc/pkg/cli/install_cd.gosrc/pkg/cli/preview_test.gosrc/pkg/cli/tail_test.gosrc/pkg/cli/waitForCdTaskExit_test.gosrc/pkg/clouds/aws/codebuild/cfn/oidc.gosrc/pkg/clouds/aws/codebuild/cfn/oidc_test.gosrc/pkg/clouds/aws/codebuild/cfn/oidcprovider.gosrc/pkg/clouds/aws/codebuild/cfn/outputs.gosrc/pkg/clouds/aws/codebuild/cfn/params.gosrc/pkg/clouds/aws/codebuild/cfn/quick_create.gosrc/pkg/clouds/aws/codebuild/cfn/quick_create_test.gosrc/pkg/clouds/aws/codebuild/cfn/setup.gosrc/pkg/clouds/aws/codebuild/cfn/setup_test.gosrc/pkg/clouds/aws/codebuild/cfn/template.gosrc/pkg/clouds/aws/codebuild/cfn/template_test.gosrc/pkg/clouds/aws/codebuild/cfn/testdata/template.yamlsrc/pkg/clouds/aws/codebuild/cfn/waiter.gosrc/pkg/clouds/aws/codebuild/common.gosrc/pkg/clouds/aws/codebuild/run.gosrc/pkg/clouds/aws/codebuild/status.gosrc/pkg/clouds/aws/codebuild/tail.gosrc/pkg/clouds/aws/codebuild/upload.gosrc/pkg/clouds/aws/ecs/cfn/outputs.gosrc/pkg/clouds/aws/ecs/cfn/template.gosrc/pkg/clouds/aws/ecs/cfn/template_test.gosrc/pkg/clouds/aws/ecs/cfn/testdata/template.yamlsrc/pkg/clouds/aws/ecs/common.gosrc/pkg/clouds/aws/ecs/common_test.gosrc/pkg/clouds/aws/ecs/fargate.gosrc/pkg/clouds/aws/ecs/fargate_test.gosrc/pkg/clouds/aws/ecs/info.gosrc/pkg/clouds/aws/ecs/run.gosrc/pkg/clouds/aws/ecs/status.gosrc/pkg/clouds/aws/ecs/status_test.gosrc/pkg/clouds/aws/ecs/stop.gosrc/pkg/clouds/aws/ecs/tail.gosrc/pkg/clouds/aws/ecs/tail_test.gosrc/pkg/clouds/driver.gosrc/pkg/clouds/gcp/cloudrun.gosrc/pkg/crun/common.gosrc/pkg/crun/common_test.gosrc/pkg/crun/destroy.gosrc/pkg/crun/docker/common.gosrc/pkg/crun/docker/info.gosrc/pkg/crun/docker/run.gosrc/pkg/crun/docker/run_test.gosrc/pkg/crun/docker/setup.gosrc/pkg/crun/docker/stop.gosrc/pkg/crun/docker/tail.gosrc/pkg/crun/docker/teardown.gosrc/pkg/crun/factory.gosrc/pkg/crun/info.gosrc/pkg/crun/local/local.gosrc/pkg/crun/local/local_test.gosrc/pkg/crun/logs.gosrc/pkg/crun/run.gosrc/pkg/crun/stop.go
💤 Files with no reviewable changes (38)
- src/pkg/clouds/aws/ecs/cfn/template_test.go
- src/pkg/crun/docker/run_test.go
- src/pkg/clouds/aws/ecs/fargate_test.go
- src/pkg/crun/docker/tail.go
- src/pkg/crun/local/local_test.go
- src/pkg/clouds/aws/ecs/cfn/template.go
- src/pkg/crun/run.go
- src/pkg/clouds/aws/ecs/common_test.go
- src/pkg/clouds/aws/ecs/fargate.go
- src/pkg/clouds/aws/ecs/run.go
- src/pkg/clouds/gcp/cloudrun.go
- src/pkg/crun/common_test.go
- src/cmd/crun/main.go
- src/pkg/crun/docker/common.go
- src/pkg/clouds/aws/codebuild/cfn/params.go
- src/pkg/crun/docker/stop.go
- src/pkg/crun/common.go
- src/pkg/crun/destroy.go
- src/pkg/crun/docker/teardown.go
- src/pkg/crun/local/local.go
- src/pkg/clouds/aws/ecs/tail_test.go
- src/pkg/clouds/aws/ecs/cfn/testdata/template.yaml
- src/pkg/cli/client/byoc/aws/cd.go
- src/pkg/crun/docker/run.go
- src/pkg/crun/docker/info.go
- src/pkg/crun/docker/setup.go
- src/pkg/clouds/aws/ecs/info.go
- src/pkg/clouds/aws/ecs/stop.go
- src/pkg/crun/factory.go
- src/pkg/crun/logs.go
- src/pkg/clouds/aws/ecs/common.go
- src/pkg/crun/info.go
- src/pkg/clouds/aws/ecs/tail.go
- src/pkg/clouds/aws/ecs/cfn/outputs.go
- src/pkg/clouds/aws/ecs/status_test.go
- src/pkg/clouds/driver.go
- src/pkg/crun/stop.go
- src/pkg/clouds/aws/ecs/status.go
🚧 Files skipped from review as they are similar to previous changes (5)
- src/pkg/cli/client/byoc/aws/validation_test.go
- Makefile
- src/pkg/clouds/aws/codebuild/run.go
- src/pkg/clouds/aws/codebuild/cfn/outputs.go
- src/pkg/clouds/aws/codebuild/upload.go
Comment on lines
+674
to
677
| func (b *ByocDo) SetUpCD(ctx context.Context, force bool) error { | ||
| if b.SetupDone { | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
force is currently a no-op in DO setup.
SetUpCD ignores force because the early return only checks b.SetupDone. A forced setup path should bypass that guard.
💡 Suggested fix
func (b *ByocDo) SetUpCD(ctx context.Context, force bool) error {
- if b.SetupDone {
+ if b.SetupDone && !force {
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (b *ByocDo) SetUpCD(ctx context.Context, force bool) error { | |
| if b.SetupDone { | |
| return nil | |
| } | |
| func (b *ByocDo) SetUpCD(ctx context.Context, force bool) error { | |
| if b.SetupDone && !force { | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/cli/client/byoc/do/byoc.go` around lines 674 - 677, The SetUpCD
method currently returns early whenever b.SetupDone is true, making the force
parameter a no-op; change the early-return check in SetUpCD to only return when
setup is done and force is false (e.g., if b.SetupDone && !force { return nil })
so that calling SetUpCD(ctx, true) will bypass the guard and re-run the setup;
ensure any stateful steps inside SetUpCD (references to b.SetupDone and
subsequent setup logic) are safe to re-execute or reset b.SetupDone
appropriately before/after the forced run.
Comment on lines
+216
to
+252
| CodeBuildIAMPolicy: | ||
| Properties: | ||
| PolicyDocument: | ||
| Statement: | ||
| - Action: | ||
| - iam:CreateRole | ||
| - iam:GetRole | ||
| - iam:UpdateRole | ||
| - iam:DeleteRole | ||
| - iam:TagRole | ||
| - iam:UntagRole | ||
| - iam:UpdateAssumeRolePolicy | ||
| - iam:ListRolePolicies | ||
| - iam:ListAttachedRolePolicies | ||
| - iam:ListInstanceProfilesForRole | ||
| - iam:CreatePolicy | ||
| - iam:GetPolicy | ||
| - iam:DeletePolicy | ||
| - iam:CreatePolicyVersion | ||
| - iam:DeletePolicyVersion | ||
| - iam:GetPolicyVersion | ||
| - iam:ListPolicyVersions | ||
| - iam:PutRolePolicy | ||
| - iam:GetRolePolicy | ||
| - iam:DeleteRolePolicy | ||
| - iam:AttachRolePolicy | ||
| - iam:DetachRolePolicy | ||
| - iam:CreateInstanceProfile | ||
| - iam:GetInstanceProfile | ||
| - iam:DeleteInstanceProfile | ||
| - iam:AddRoleToInstanceProfile | ||
| - iam:RemoveRoleFromInstanceProfile | ||
| - iam:PassRole | ||
| - iam:CreateServiceLinkedRole | ||
| Effect: Allow | ||
| Resource: '*' | ||
| Version: "2012-10-17" |
Contributor
There was a problem hiding this comment.
IAM permissions are too broad for the CodeBuild execution role.
CodeBuildIAMPolicy grants wide IAM mutation actions on Resource: '*', while CodeBuildServiceRole also has PowerUserAccess. This is a significant least-privilege gap and increases blast radius.
Also applies to: 265-267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/clouds/aws/codebuild/cfn/testdata/template.yaml` around lines 216 -
252, The IAM policy CodeBuildIAMPolicy and the CodeBuildServiceRole grant overly
broad privileges (Resource: '*' and PowerUserAccess); narrow them by removing
unneeded iam mutation actions from the PolicyDocument (only keep actions
CodeBuild actually needs), replace Resource: '*' with specific ARNs (e.g., the
exact role, instance profile, S3 buckets, ECR repos, CloudWatch log groups) and
scope iam:PassRole to the specific execution role ARN, and remove
PowerUserAccess from CodeBuildServiceRole in favor of least-privilege managed
policies (e.g., AWSCodeBuildServiceRole or custom policies limited to S3, ECR,
CloudWatch and the specific IAM role). Ensure CodeBuildIAMPolicy only contains
the precise actions referenced by the build project (attach/detach only if used)
and reference the exact resource ARNs for those actions.
| client := cb.NewFromConfig(cfg) | ||
|
|
||
| output, err := client.BatchGetBuilds(ctx, &cb.BatchGetBuildsInput{ | ||
| Ids: []string{*buildID}, |
Contributor
There was a problem hiding this comment.
Guard nil pointer dereferences in failure paths.
Line 18 dereferences buildID without a nil check, and Line 36 dereferences phase.Contexts[0].Message without verifying Message != nil. Both are panic risks.
Suggested fix
func GetBuildStatus(ctx context.Context, cfg aws.Config, buildID BuildID) (bool, error) {
+ if buildID == nil || *buildID == "" {
+ return true, BuildFailure{Reason: "missing build ID"}
+ }
+
client := cb.NewFromConfig(cfg)
@@
- if phase.PhaseStatus == cbtypes.StatusTypeFailed && len(phase.Contexts) > 0 {
- reason = *phase.Contexts[0].Message
+ if phase.PhaseStatus == cbtypes.StatusTypeFailed && len(phase.Contexts) > 0 && phase.Contexts[0].Message != nil {
+ reason = *phase.Contexts[0].Message
break
}
}
}Also applies to: 35-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/clouds/aws/codebuild/status.go` at line 18, Guard against nil
dereferences for buildID and phase context messages: before using *buildID when
constructing the Ids slice, check buildID != nil and return or handle the error
if nil; likewise when accessing phase.Contexts[0].Message ensure phase.Contexts
is non-empty and phase.Contexts[0].Message != nil (or use a safe accessor)
before dereferencing, and provide a sensible fallback or error path. Update the
code paths around the Ids: []string{*buildID} usage and the logic that reads
phase.Contexts[0].Message so both validate pointers/lengths and handle failure
cases without panicking.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This results it much faster start times (~30s less!) and easier bootstrapping since no VPC and Cluster is needed.
Also removing the obsolete
cruncode.Timing benchmark https://gist.github.com/lionello/583c8bda0254230d6092b692fe5917c4:
Linked Issues
Fixes #1342
Checklist
Summary by CodeRabbit
New Features
Refactor
Chores