BCH-1296: AWS ECR compliance plugin#4
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 8 minutes and 27 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR refactors GitHub Actions CI/CD workflows into reusable components and refines policy evaluation scoping. Test and build-and-upload workflows are extracted as callable workflows, invoked by dedicated push and release dispatchers. The plugin policy-to-behavior mapping is updated to scope each bundle to a single behavior. ChangesGitHub Actions Workflow Consolidation
Policy Behavior Scoping
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
…setup The three explicit behavior mappings cover all supported bundle names. A silent fallback to repository/registry for unrecognised bundle names is more surprising than leaving them uncovered. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ttern Split monolithic ci.yml into reusable test.yml + push.yml trigger. Extract goreleaser/gooci steps into reusable build-and-upload.yml with release.yml as a thin tag trigger, matching the four-file structure established in plugin-aws-acm. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Maps ecr-repository-policy, ecr-registry-policy, and ecr-image-policy bundle name substrings to their respective resource type behaviors. Legacy combined bundle keys retained for backward compatibility. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…cies
Rename ecr-{repository,registry,image}-policy keys to -policies to match
the repository naming convention. Removes duplicate map entries introduced
by the mechanical rename.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The single combined policy repo was never released. Only the three per-resource-type bundles are supported. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.go (1)
124-130: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider skipping repository fetch when neither repository nor image policies are configured.
The registry (line 141) and image (line 155) fetch operations are conditional on their respective policy paths, but repository fetch always executes. When only registry policies are configured, this results in unnecessary
DescribeRepositoriesAPI calls per region.⚡ Proposed optimization to skip fetch when policies not needed
+ var repos []internal.RepositoryContext + if len(repositoryPaths) > 0 || len(imagePaths) > 0 { // CONFIG — repository checks - repos, err := dataFetcher.FetchRepositories(ctx, region) + var err error + repos, err = dataFetcher.FetchRepositories(ctx, region) if err != nil { return &proto.EvalResponse{Status: proto.ExecutionStatus_FAILURE}, fmt.Errorf("region %s: fetching repositories: %w", region, err) } // Filter to configured accounts if any are specified. repos = internal.FilterByAccounts(repos, l.config.Accounts) + } + if len(repositoryPaths) > 0 { for _, repo := range repos { evidences, err := policyEvaluator.EvalRepository(ctx, repo, repositoryPaths, l.policyData, l.config.PolicyLabels) allEvidences = append(allEvidences, evidences...) if err != nil { evalErrors = errors.Join(evalErrors, fmt.Errorf("evaluating repository %s: %w", repo.RepositoryName, err)) } } + }🤖 Prompt for 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. In `@main.go` around lines 124 - 130, The code always calls dataFetcher.FetchRepositories(ctx, region) even when neither repository nor image policies are configured, causing unnecessary DescribeRepositories calls; update the logic in the block around FetchRepositories and internal.FilterByAccounts to first check the config policy paths (e.g., l.config.RepositoryPolicyPath and l.config.ImagePolicyPath or whatever flags indicate repository/image policy usage) and only call dataFetcher.FetchRepositories when repository policies are configured or image policies require repository metadata; if skipped, ensure repos is set to an empty slice (or nil) so subsequent code that uses repos behaves correctly and keep the existing error handling for FetchRepositories inside the guarded branch.
🤖 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 @.github/workflows/build-and-upload.yml:
- Around line 13-18: The workflow currently references actions by floating tags
`actions/checkout@v4` and `actions/setup-go@v5`; update these to immutable
commit SHAs by replacing the tag refs with the corresponding commit SHA values
(e.g., `actions/checkout@<SHA>` and `actions/setup-go@<SHA>`), then commit the
updated .github/workflows/build-and-upload.yml; ensure you pick SHAs that match
the intended release versions and add a note to add Dependabot/Renovate to keep
those SHAs updated.
- Around line 16-18: The workflow currently uses actions/setup-go@v5 with
go-version-file: go.mod which enables caching by default; for deterministic
release builds, add the setup-go input to disable caching (set cache to "none")
in the job that builds releases so the Go toolchain is installed without using
cached modules or toolchain state. Update the step referencing
actions/setup-go@v5 (the step that includes go-version-file: go.mod) to include
cache: "none" to ensure a clean build environment for release artifacts.
- Around line 30-31: Replace the direct template expansions in the "gooci Upload
Version" run command with environment variables and use proper shell quoting to
prevent injection: add env entries (e.g., UPLOAD_OWNER: ${{
github.repository_owner }}, UPLOAD_REPO: ${{ github.event.repository.name }},
UPLOAD_REF: ${{ github.ref_name }}) and change the run line in that step to call
gooci with a single, quoted image argument (e.g., gooci upload dist/
"ghcr.io/${UPLOAD_OWNER}/${UPLOAD_REPO}:${UPLOAD_REF}") so the expanded values
are treated as data, not code; reference the step name "gooci Upload Version"
and the run command when making the change.
- Around line 28-29: Replace the inline template expansion in the "Authenticate
gooci cli" step with environment variables to prevent shell injection: set env
entries (e.g., GITHUB_ACTOR and GITHUB_TOKEN) from ${{ github.actor }} and ${{
secrets.GITHUB_TOKEN }}, then call the same command using those env vars (use
quoted variable references like "$GITHUB_ACTOR" and "$GITHUB_TOKEN") so the run
step no longer directly expands template values into the shell.
- Around line 32-34: The "gooci Upload Latest" step is vulnerable to shell
injection because it expands github.repository_owner and
github.event.repository.name directly in the run line; fix it by moving those
GitHub expressions into step-level environment variables (e.g., OWNER and REPO)
and then call the upload with the env vars (quoting them) instead of inlining
expressions—update the step named "gooci Upload Latest" to set env: OWNER: ${{
github.repository_owner }} and REPO: ${{ github.event.repository.name }} and use
those variables in the run command (e.g., refer to $OWNER/$REPO:latest, quoted)
to prevent injection.
In @.github/workflows/release.yml:
- Around line 10-12: Remove the redundant permissions block from release.yml by
deleting the "permissions:" section that contains "packages: write" and
"contents: write" since those permissions are already provided by the called
reusable workflow; ensure no subsequent steps in release.yml rely on an
overridden permissions block after removal.
In @.github/workflows/test.yml:
- Around line 21-22: Restore a GitHub Actions step that runs go vet to re-enable
static analysis: add a step named "Vet" (or insert a run step into the existing
"Test" job) that executes "go vet ./..." prior to running "go test ./...";
ensure it runs after the Go setup (actions/setup-go) step so the Go toolchain is
available.
- Around line 14-20: The workflow uses unpinned third-party actions
(actions/checkout@v4 and actions/setup-go@v5); update those refs to pinned
commit SHAs (the full @<40-hex> ref) to harden the supply chain by replacing the
version tags with the corresponding commit SHA for each action (locate the SHA
on the action's GitHub repo or Marketplace and substitute it for
actions/checkout@v4 and actions/setup-go@v5), keeping the existing inputs like
persist-credentials and go-version-file unchanged.
---
Outside diff comments:
In `@main.go`:
- Around line 124-130: The code always calls dataFetcher.FetchRepositories(ctx,
region) even when neither repository nor image policies are configured, causing
unnecessary DescribeRepositories calls; update the logic in the block around
FetchRepositories and internal.FilterByAccounts to first check the config policy
paths (e.g., l.config.RepositoryPolicyPath and l.config.ImagePolicyPath or
whatever flags indicate repository/image policy usage) and only call
dataFetcher.FetchRepositories when repository policies are configured or image
policies require repository metadata; if skipped, ensure repos is set to an
empty slice (or nil) so subsequent code that uses repos behaves correctly and
keep the existing error handling for FetchRepositories inside the guarded
branch.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5aeda375-65ed-40d9-84fd-12c6b4c2c48a
📒 Files selected for processing (5)
.github/workflows/build-and-upload.yml.github/workflows/push.yml.github/workflows/release.yml.github/workflows/test.ymlmain.go
…s guard - test.yml: pin actions/checkout and actions/setup-go to commit SHAs; add go vet ./... step before go test ./... - build-and-upload.yml: pin checkout/setup-go SHAs; add cache:"none" to setup-go for deterministic release builds; move github.actor, repository_owner, event.repository.name and ref_name template expansions into step env vars to prevent shell injection - release.yml: remove redundant permissions block (already provided by the called reusable workflow) - main.go: guard FetchRepositories behind repositoryPaths/imagePaths check to avoid unnecessary DescribeRepositories calls when only registry checks are configured Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
1 participant
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.
Summary
plugin-aws-ecrbinary — fetches ECR repository, registry, and image data via AWS SDK v2 and evaluates OPA policy bundles for each resource typeecr-repository(CONFIG),ecr-registry(CONFIG),ecr-image(DYNAMIC with 90-day lookback)ecr-repository-policies,ecr-registry-policies,ecr-image-policies) to their respective resource type evaluationsTest plan
go test ./...passesgo build ./...produces a valid binaryeval $(aws configure export-credentials ...) && PLUGIN_REF=feat/BCH-1296 POLICY_REF=feat/BCH-1296 make e2efromlocal-dev/e2e/ecr-repository/make start && make injectfromlocal-dev/envs/aws-ecr/🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
go vetchecks from testing pipeline.Changes