feat: add CloudEvents webhook support for AWS ECR #1370
Conversation
audacioustux
requested review from
chengfang,
dkarpele,
ishitasequeira,
jannfis and
pasha-codefresh
as code owners
WalkthroughAdds CloudEvents webhook support: a new Changes
sequenceDiagram
autonumber
participant ECR as AWS ECR
participant EB as EventBridge
participant API as API Destination
participant Webhook as Webhook Server
participant Parser as CloudEventsWebhook
ECR->>EB: emit image.push event
EB->>API: rule matches → invoke API Destination (CloudEvent)
API->>Webhook: POST CloudEvents JSON (header or ?secret)
Webhook->>Webhook: validate method, Content-Type, secret
Webhook->>Parser: pass request body
Parser->>Parser: parse CloudEvent fields (specversion,type,source,subject,data)
Parser->>Webhook: return WebhookEvent {RegistryURL, Repository, Tag, Digest}
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)cmd/common.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (2)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1370 +/- ##
==========================================
+ Coverage 70.80% 71.47% +0.66%
==========================================
Files 49 50 +1
Lines 4528 4655 +127
==========================================
+ Hits 3206 3327 +121
- Misses 1125 1128 +3
- Partials 197 200 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
audacioustux
force-pushed
the
feat/cloudevents-webhook
branch
from
6017d44 to
f66440c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/webhook/cloudevents_test.go (1)
124-136: Consider adding test case for tag containing colon.The subject parsing logic at line 131-134 in cloudevents.go uses
strings.Split(payload.Subject, ":")withlen(parts) == 2. Tags likesha256:abc123or timestamps like2025-01-01:latestwould not parse correctly from the subject. Consider adding a test case to document this behavior or verify the data fields take precedence.Example test case to add:
{ name: "ECR event with colon in tag uses data fields", payload: map[string]interface{}{ "specversion": "1.0", "id": "test-id", "type": "com.amazon.ecr.image.push", "source": "urn:aws:ecr:us-east-1:123456789012:repository/myrepo", "subject": "myrepo:v1:latest", // malformed subject "time": "2025-11-27T10:00:00Z", "datacontenttype": "application/json", "data": map[string]interface{}{ "repositoryName": "myrepo", "imageDigest": "sha256:abc", "imageTag": "v1:latest", "registryId": "123456789012", }, }, wantRegistryURL: "123456789012.dkr.ecr.us-east-1.amazonaws.com", wantRepository: "myrepo", wantTag: "v1:latest", wantDigest: "sha256:abc", wantErr: false, },pkg/webhook/cloudevents.go (2)
130-135: Subject parsing is fragile for tags containing colons.The split on
:withlen(parts) == 2check will fail for tags containing colons (e.g.,repo:v1:hotfixor timestamps). Since data fields override subject anyway, consider usingstrings.SplitNwith a limit of 2 to handle this edge case gracefully.- if parts := strings.Split(payload.Subject, ":"); len(parts) == 2 { - repository = parts[0] - tag = parts[1] + if parts := strings.SplitN(payload.Subject, ":", 2); len(parts) == 2 { + repository = parts[0] + tag = parts[1] logCtx.Tracef("Extracted from subject: repository=%s, tag=%s", repository, tag) }
227-252: Simplify extractECRRegistryURL by removing duplicate parsing.The fallback block at lines 239-248 re-parses the source even though the primary block at lines 229-236 already extracts both region and account. If the primary parsing succeeds, the fallback is never needed. If it fails, the fallback with
registryIdstill requires a valid source prefix to get the region, making it partially redundant.func (c *CloudEventsWebhook) extractECRRegistryURL(source string, data map[string]interface{}) string { // Try to extract from source URN if strings.HasPrefix(source, "urn:aws:ecr:") { parts := strings.Split(source, ":") if len(parts) >= 5 { region := parts[3] account := parts[4] + // Use registryId from data if available, otherwise use account from source + if registryID, ok := data["registryId"].(string); ok && registryID != "" { + account = registryID + } return fmt.Sprintf("%s.dkr.ecr.%s.amazonaws.com", account, region) } } - - // Fallback: try to extract from data - if registryID, ok := data["registryId"].(string); ok { - // If we have registryId, we need region to construct the URL - // Try to extract region from source - if strings.HasPrefix(source, "urn:aws:ecr:") { - parts := strings.Split(source, ":") - if len(parts) >= 4 { - region := parts[3] - return fmt.Sprintf("%s.dkr.ecr.%s.amazonaws.com", registryID, region) - } - } - } - return "" }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/common.go(2 hunks)cmd/run.go(1 hunks)cmd/webhook.go(2 hunks)config/examples/cloudevents/README.md(1 hunks)config/examples/cloudevents/terraform/main.tf(1 hunks)config/examples/cloudevents/terraform/outputs.tf(1 hunks)config/examples/cloudevents/terraform/variables.tf(1 hunks)config/examples/cloudevents/test-webhook.sh(1 hunks)docs/configuration/webhook.md(5 hunks)pkg/webhook/cloudevents.go(1 hunks)pkg/webhook/cloudevents_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
cmd/run.go (1)
registry-scanner/pkg/env/env.go (1)
GetStringVal(31-37)
cmd/common.go (1)
pkg/webhook/cloudevents.go (1)
NewCloudEventsWebhook(22-26)
cmd/webhook.go (1)
registry-scanner/pkg/env/env.go (1)
GetStringVal(31-37)
pkg/webhook/cloudevents_test.go (2)
pkg/webhook/cloudevents.go (1)
NewCloudEventsWebhook(22-26)registry-scanner/pkg/registry/mocks/Repository.go (1)
Repository(15-17)
pkg/webhook/cloudevents.go (2)
registry-scanner/pkg/log/log.go (4)
NewContext(34-40)Tracef(159-162)Errorf(183-186)LogContext(26-31)pkg/argocd/types.go (1)
WebhookEvent(196-205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Ensure code is correctly linted
- GitHub Check: Ensure unit tests are passing
- GitHub Check: Ensure registry-scanner Go modules synchronicity and run tests
- GitHub Check: Analyze (go)
- GitHub Check: build_image
🔇 Additional comments (16)
cmd/common.go (2)
29-29: LGTM!The new
CloudEventsSecretfield follows the established pattern for other webhook secrets in the struct.
121-121: LGTM!The CloudEvents webhook handler registration follows the same pattern as existing handlers (Docker, GHCR, Harbor, Quay), maintaining consistency.
config/examples/cloudevents/README.md (1)
1-35: LGTM!Clear and well-structured documentation for the CloudEvents webhook example. The quick start guide, file listing, and documentation link provide a good onboarding experience.
cmd/run.go (1)
307-307: LGTM!The new CLI flag follows the established pattern for other webhook secrets, with consistent naming and environment variable binding.
config/examples/cloudevents/test-webhook.sh (1)
14-92: LGTM!Good test coverage with four distinct scenarios: successful ECR push, generic container push with CloudEvents content-type, missing secret validation, and invalid event type handling. The
|| echo "Expected failure"pattern correctly handles expected failures withset -e.config/examples/cloudevents/terraform/outputs.tf (1)
1-19: LGTM!Well-structured Terraform outputs with clear descriptions. The outputs expose useful ARNs for debugging and integration purposes.
cmd/webhook.go (2)
44-45: LGTM!Help text correctly updated to include Harbor and AWS ECR via EventBridge CloudEvents in the list of supported registries.
93-93: LGTM!The CLI flag binding is consistent with the
runcommand (cmd/run.go, line 307) and follows the established pattern for other webhook secrets.docs/configuration/webhook.md (2)
12-12: Documentation is clear and comprehensive.The AWS ECR via EventBridge CloudEvents section provides thorough setup instructions, prerequisites, and example payloads. The documentation follows the existing patterns for other registries.
Also applies to: 88-159
161-162: Unable to verify directory existence due to repository access issues.The sandbox environment is experiencing persistent failures when attempting to clone the repository, preventing verification of whether the
config/examples/cloudevents/kubernetes/directory exists in the PR. Manual verification of this directory in the repository is required to confirm whether the documentation reference is accurate or if the directory needs to be added.config/examples/cloudevents/terraform/main.tf (2)
1-141: Well-structured Terraform configuration following AWS best practices.The module correctly implements:
- Least-privilege IAM with scoped
InvokeApiDestinationpermission- API_KEY authentication via header
- Configurable repository filtering
- Retry policy for resilience
118-134: Based on the web search verification results, I have sufficient information to assess the review comment.The review comment is substantively correct and identifies a real issue.
The web search confirms:
- ECR does send events without
image-tagfor untagged/manifest-only pushes — the event detail will either omit the field entirely or have it as null/empty- EventBridge InputTransformer with missing source paths — when the
<tag>placeholder references a path that doesn't exist in the source event, it will either fail to substitute or produce invalid JSON- The current template is vulnerable — using
"imageTag": "<tag>"means untagged image pushes will either cause the transformer to fail or produce malformed CloudEventsThe concern about handling manifest-only ECR pushes (which lack tags) is legitimate and should be addressed through either:
- Adding filtering rules to exclude untagged images
- Making the imageTag field optional in the CloudEvents template
- Documenting this limitation
**Input template will fail or produce invalid JSON for untagged ECR images due to missing `image-tag` field.** ECR manifest-only pushes (untagged images) do not include an `image-tag` in the EventBridge event detail. The current template uses `"imageTag": "<tag>"` which will cause the InputTransformer to fail when this field is absent. Either filter these events in the rule or modify the template to handle missing tags gracefully.config/examples/cloudevents/terraform/variables.tf (1)
1-32: Variables are well-defined with appropriate types and sensitivity markers.The
webhook_secretis correctly marked assensitive = true, preventing it from appearing in logs or plan outputs.pkg/webhook/cloudevents_test.go (2)
19-122: Comprehensive validation test coverage.The test suite covers key validation scenarios including HTTP method, content types, query/header secrets, and missing/invalid secrets.
267-420: Good coverage of generic CloudEvents parsing.Tests appropriately cover alternate field names, source URL extraction, and error cases for missing specversion/type.
pkg/webhook/cloudevents.go (1)
14-31: Clean implementation following established webhook patterns.The struct design, constructor, and parsing logic are well-organized. Trace logging for debugging webhook payloads is a good addition for troubleshooting.
Also applies to: 74-115
audacioustux
force-pushed
the
feat/cloudevents-webhook
branch
2 times, most recently
from
9cc93c0 to
b3cac08
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/configuration/webhook.md (1)
247-256: Fix GHCR flag name; CloudEvents env mapping looks correctThe new row mapping
CLOUDEVENTS_WEBHOOK_SECRET→--cloudevents-webhook-secretis consistent withcmd/webhook.go. However, the GHCR row above documents--gchr-webhook-secret, while the actual flag incmd/webhook.gois--ghcr-webhook-secret. Consider correcting that flag name in this table while touching this area.
🧹 Nitpick comments (2)
config/examples/cloudevents/test-webhook.sh (1)
14-90: Consider enforcing expected success/failure via curl exit codesRight now the script just prints HTTP status codes, and the “expected to fail” tests rely on curl exiting non‑zero, which won’t happen for HTTP 4xx/5xx without
--fail/--fail-with-body. To make this a more reliable e2e check, consider adding--fail-with-body(or parsing%{http_code}) for tests 1–4 and explicitly asserting on success vs failure instead of relying on manual inspection.pkg/webhook/cloudevents.go (1)
188-195: Make generic registry URL extraction robust to blank valuesIn the generic container/image path,
registryUrl/registryare treated as authoritative as long as the key exists, even if the value is an empty string, which prevents falling back toextractRegistryFromSourceand leads straight to the “registry URL not found” error. If you want to be resilient to partially-populated data, consider also checking for non-empty strings before assignment, e.g.:- if regURL, ok := payload.Data["registryUrl"].(string); ok { + if regURL, ok := payload.Data["registryUrl"].(string); ok && regURL != "" { registryURL = regURL - } else if regURL, ok := payload.Data["registry"].(string); ok { + } else if regURL, ok := payload.Data["registry"].(string); ok && regURL != "" { registryURL = regURL } else { registryURL = c.extractRegistryFromSource(payload.Source) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/common.go(2 hunks)cmd/run.go(1 hunks)cmd/webhook.go(2 hunks)config/examples/cloudevents/README.md(1 hunks)config/examples/cloudevents/terraform/main.tf(1 hunks)config/examples/cloudevents/terraform/outputs.tf(1 hunks)config/examples/cloudevents/terraform/variables.tf(1 hunks)config/examples/cloudevents/test-webhook.sh(1 hunks)docs/configuration/webhook.md(5 hunks)pkg/webhook/cloudevents.go(1 hunks)pkg/webhook/cloudevents_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/common.go
- config/examples/cloudevents/terraform/outputs.tf
- pkg/webhook/cloudevents_test.go
- config/examples/cloudevents/terraform/main.tf
- config/examples/cloudevents/terraform/variables.tf
- cmd/run.go
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/webhook.go (1)
registry-scanner/pkg/env/env.go (1)
GetStringVal(31-37)
pkg/webhook/cloudevents.go (3)
registry-scanner/pkg/log/log.go (4)
NewContext(34-40)Tracef(159-162)Errorf(183-186)LogContext(26-31)pkg/argocd/types.go (1)
WebhookEvent(196-205)registry-scanner/pkg/registry/mocks/Repository.go (1)
Repository(15-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Analyze (go)
- GitHub Check: Ensure unit tests are passing
- GitHub Check: Ensure registry-scanner Go modules synchronicity and run tests
- GitHub Check: Ensure code is correctly linted
- GitHub Check: build_image
🔇 Additional comments (3)
docs/configuration/webhook.md (1)
102-164: CloudEvents/ECR documentation aligns with implementationThe new “AWS ECR via EventBridge CloudEvents” section (payload shape, webhook URL with
type=cloudevents, and secret handling via query orX-Webhook-Secret) matches the CloudEvents handler and examples. This gives users a clear, end‑to‑end path from EventBridge to the webhook.cmd/webhook.go (1)
40-46: CloudEvents webhook CLI wiring is consistentThe updated help text and the new
--cloudevents-webhook-secretflag (backed byCLOUDEVENTS_WEBHOOK_SECRETandwebhookCfg.CloudEventsSecret) line up with the documentation and secret configuration model used for other registries. No issues here.Also applies to: 88-94
pkg/webhook/cloudevents.go (1)
34-73: CloudEvents validation and secret handling look correctThe handler enforces POST, performs a sensible content-type check for structured CloudEvents JSON, and supports a configurable secret via query parameter or
X-Webhook-Secret, returning clear errors for missing/invalid secrets. This fits the documented contract and doesn’t conflict with body parsing later on.
audacioustux
force-pushed
the
feat/cloudevents-webhook
branch
from
b3cac08 to
ff24c38
Compare
Adds CloudEvents v1.0 webhook handler to support AWS ECR push events via EventBridge, using the CloudEvents specification for a standardized approach instead of registry-specific handlers. Changes: - Add pkg/webhook/cloudevents.go with CloudEvents webhook handler - Add pkg/webhook/cloudevents_test.go with comprehensive test suite (60+ tests) - Add --cloudevents-webhook-secret flag to webhook/run commands - Add documentation in docs/configuration/webhook.md - Add Terraform example for EventBridge setup - Add trace logging for debugging webhook payloads - Use crypto/subtle.ConstantTimeCompare for secret validation Signed-off-by: Tanjim Hossain <[email protected]>
audacioustux
force-pushed
the
feat/cloudevents-webhook
branch
from
ff24c38 to
aaa02b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
config/examples/cloudevents/test-webhook.sh (1)
58-74:set -emay not behave as expected for failure tests.With
set -eenabled (line 5), the script will exit on any command failure. However,curlreturns exit code 0 even for HTTP 4xx/5xx responses unless--failis used. The|| echo "Expected failure"fallback won't trigger sincecurlsucceeds at the transport level.Consider adding
-for--failto make curl return non-zero on HTTP errors, or removeset -efor a more forgiving test script that reports all results regardless of individual test outcomes.docs/configuration/webhook.md (1)
151-155: Consider clarifying the recommended authentication method.The documentation shows the secret in the query parameter (
?type=cloudevents&secret=<YOUR_SECRET>) but then recommends using theX-Webhook-Secretheader. Consider leading with the header-based approach since it's more secure and recommended for EventBridge Connection authentication.pkg/webhook/cloudevents.go (3)
34-70: Avoid using query parameters for webhook secrets where possible.The validation logic is sound and the constant‑time comparison is good, but accepting the secret via
?secret=risks exposure through access logs, proxies, and browser history. I’d recommend:
- Prefer the
X-Webhook-Secretheader path in docs and examples.- Long‑term, consider deprecating or feature‑flagging the query‑param option to reduce accidental leakage.
75-104: Trace logging full payloads can be noisy; consider size limits.Logging the entire request body at trace level is useful for debugging, but if events can grow large this may bloat logs or exceed log size limits. Consider truncating the payload (e.g., first N KB) or guarding the log behind an additional debug flag if you expect large events in some deployments.
118-201: Make ECR registry URL extraction more tolerant of different CloudEvents mappings.
parseEvent’s ECR branch currently depends onextractECRRegistryURL, which assumes the CloudEventssourcehas the fixed URN shapeurn:aws:ecr:<region>:<account>:repository/<repo>and only usesdata["registryId"]as an override. If a user tweaks the EventBridge → CloudEvents transform (for example, populatingdata.registryUrldirectly or using a differentsourceformat),registryURLwill end up empty and the event will be rejected.To make this path more robust without breaking the happy path:
- Prefer an explicit
registryUrlfield when present, and only fall back to deriving fromsource/registryId.- Optionally, extend
extractECRRegistryURLto also look at additional fields (e.g.,data["region"]/data["account"]/data["awsAccountId"]) if you expect those from alternative transforms.Illustrative change:
func (c *CloudEventsWebhook) parseEvent(payload *struct { ... }, logCtx *log.LogContext) (*argocd.WebhookEvent, error) { ... switch { case strings.HasPrefix(payload.Type, "com.amazon.ecr."): ... - // Extract registry URL from source - // Source format: urn:aws:ecr:<region>:<account>:repository/<repo> - registryURL = c.extractECRRegistryURL(payload.Source, payload.Data) + // Prefer explicit registryUrl in data, then fall back to source‑based derivation + if regURL, ok := payload.Data["registryUrl"].(string); ok && regURL != "" { + registryURL = regURL + } else { + // Source format: urn:aws:ecr:<region>:<account>:repository/<repo> + registryURL = c.extractECRRegistryURL(payload.Source, payload.Data) + }func (c *CloudEventsWebhook) extractECRRegistryURL(source string, data map[string]interface{}) string { - // Try to extract from source URN + // If the transform already provided a full registry URL, just use it + if regURL, ok := data["registryUrl"].(string); ok && regURL != "" { + return regURL + } + + // Otherwise, try to extract from source URN if strings.HasPrefix(source, "urn:aws:ecr:") { ... } return "" }This keeps your current Terraform example working, but makes the handler resilient to other CloudEvents mappings users might adopt.
Also applies to: 227-247
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/common.go(2 hunks)cmd/run.go(1 hunks)cmd/webhook.go(2 hunks)config/examples/cloudevents/README.md(1 hunks)config/examples/cloudevents/terraform/main.tf(1 hunks)config/examples/cloudevents/terraform/outputs.tf(1 hunks)config/examples/cloudevents/terraform/variables.tf(1 hunks)config/examples/cloudevents/test-webhook.sh(1 hunks)docs/configuration/webhook.md(5 hunks)pkg/webhook/cloudevents.go(1 hunks)pkg/webhook/cloudevents_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- config/examples/cloudevents/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- config/examples/cloudevents/terraform/main.tf
- cmd/run.go
- config/examples/cloudevents/terraform/outputs.tf
🧰 Additional context used
🧬 Code graph analysis (4)
cmd/common.go (1)
pkg/webhook/cloudevents.go (1)
NewCloudEventsWebhook(23-27)
pkg/webhook/cloudevents_test.go (3)
pkg/webhook/cloudevents.go (1)
NewCloudEventsWebhook(23-27)test/utils/utils.go (1)
Run(50-67)registry-scanner/pkg/registry/mocks/Repository.go (1)
Repository(15-17)
cmd/webhook.go (1)
registry-scanner/pkg/env/env.go (1)
GetStringVal(31-37)
pkg/webhook/cloudevents.go (2)
registry-scanner/pkg/log/log.go (4)
NewContext(34-40)Tracef(159-162)Errorf(183-186)LogContext(26-31)pkg/argocd/types.go (1)
WebhookEvent(196-205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build_image
- GitHub Check: Ensure registry-scanner Go modules synchronicity and run tests
- GitHub Check: Ensure unit tests are passing
- GitHub Check: Ensure code is correctly linted
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
config/examples/cloudevents/terraform/variables.tf (2)
1-32: Well-structured Terraform variables with appropriate security handling.The variable declarations are clear, well-documented, and follow Terraform best practices. The
sensitive = trueflag is correctly applied towebhook_secretto protect credentials from accidental logging.
19-23: I'll help you verify that theecr_repository_filterempty list behavior is properly implemented downstream. Let me search the codebase for the relevant implementation.
<function_calls>
#!/bin/bashSearch for ecr_repository_filter usage in terraform examples
rg -n "repository_filter|ecr_repository_filter" config/examples/cloudevents/terraform/ -A 3 -B 1
</function_calls>
<function_calls>
#!/bin/bashList terraform files in the cloudevents example
ls -la config/examples/cloudevents/terraform/
</function_calls><function_calls>
#!/bin/bashCheck if main.tf exists and read its content
if [ -f config/examples/cloudevents/terraform/main.tf ]; then
wc -l config/examples/cloudevents/terraform/main.tf
fi
</function_calls>config/examples/cloudevents/test-webhook.sh (1)
1-92: Test script provides good coverage of CloudEvents webhook scenarios.The script tests ECR-specific events, generic container events, missing authentication, and invalid event types. The payloads align with the CloudEvents v1.0 specification and match the formats documented in the PR.
cmd/common.go (2)
29-29: LGTM!The
CloudEventsSecretfield follows the established pattern used for other webhook secrets in the struct.
121-121: LGTM!Registration of the CloudEvents webhook handler is consistent with the existing webhook handler registrations (Docker, GHCR, Harbor, Quay).
cmd/webhook.go (2)
44-45: LGTM!Help text appropriately updated to include AWS ECR via EventBridge CloudEvents in the list of supported registries.
93-93: LGTM!The CLI flag follows the established pattern for webhook secrets, with consistent naming (
--cloudevents-webhook-secret) and environment variable binding (CLOUDEVENTS_WEBHOOK_SECRET).docs/configuration/webhook.md (2)
12-12: Documentation updates are comprehensive and well-structured.The AWS ECR via CloudEvents section provides clear prerequisites, setup steps, Terraform references, and payload format examples. Environment variables and secrets are properly documented alongside existing webhook configurations.
Also applies to: 88-99, 102-163, 177-177, 214-214, 255-255
160-163: The kubernetes directory reference cannot be verified with available tools.The shell script execution failed due to a repository clone error, preventing direct verification of whether
config/examples/cloudevents/kubernetes/exists in the codebase. Without access to the repository filesystem, I cannot confirm whether the documentation's reference to this directory is accurate or if the directory exists with actual manifest files.Manual verification is needed: Check if
config/examples/cloudevents/kubernetes/exists in the repository and contains Kubernetes manifest examples as referenced in the documentation.pkg/webhook/cloudevents_test.go (5)
1-17: LGTM!Clean imports and straightforward registry type test.
19-122: Comprehensive validation test coverage.The table-driven tests cover all validation paths: valid requests with/without secrets, CloudEvents content type, query and header secret validation, and error cases for invalid methods, content types, and secrets.
124-265: Good ECR parsing test coverage.Tests cover standard ECR events, namespaced repositories, subject fallback when data fields are missing, and error cases for missing repository and tag fields. The expected registry URL construction (
<account>.dkr.ecr.<region>.amazonaws.com) is properly validated.
267-420: Solid generic CloudEvents parsing tests.Tests validate parsing with different field naming conventions, source URL extraction, and proper error handling for missing
specversion,type, and unsupported event types. The conditional digest assertion (line 414-416) correctly handles optional digest fields.
422-495: LGTM!Helper function tests for
extractECRRegistryURLandextractRegistryFromSourcecover valid ECR URNs, fallback behavior, and various source URL formats including edge cases.pkg/webhook/cloudevents.go (2)
1-32: CloudEventsWebhook type and constructor look consistent and minimal.Struct, constructor, and
GetRegistryTypeare straightforward and align with the existing webhook abstraction; no issues from a correctness or API‑surface perspective.
249-268:extractRegistryFromSourceis simple and effective; considerurl.Parseonly if future needs grow.The current string‑based extraction covers typical
https://host/...andhost.domain/pathpatterns without extra allocations or error handling. Unless you anticipate more exotic URI shapes, there’s no need to complicate this withnet/url; the helper is fine as‑is.
Signed-off-by: audacioustux <[email protected]>
audacioustux
force-pushed
the
feat/cloudevents-webhook
branch
from
3b5c94b to
8d15807
Compare
|
@coderabbitai generate docstrings |
|
Warning Docstrings generation is disabled for your repository or organization. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9040 🏁 Script executed: Length of output: 4438 🏁 Script executed: Length of output: 6734 🏁 Script executed: Length of output: 19303 @audacioustux Great news! I've reviewed the code and found that all exported types and functions already have proper Go docstrings. The CloudEvents implementation in However, I can suggest adding inline field comments for the // WebhookConfig holds the options for the webhook server
type WebhookConfig struct {
// Port is the port number for the webhook server to listen on
Port int
// DockerSecret is the secret for validating Docker Hub webhooks
DockerSecret string
// GHCRSecret is the secret for validating GitHub Container Registry webhooks
GHCRSecret string
// QuaySecret is the secret for validating Quay webhooks
QuaySecret string
// HarborSecret is the secret for validating Harbor webhooks
HarborSecret string
// CloudEventsSecret is the secret for validating CloudEvents webhooks
CloudEventsSecret string
// RateLimitNumAllowedRequests is the number of allowed requests per hour for rate limiting
RateLimitNumAllowedRequests int
}The rest of your code already follows Go documentation best practices:
The existing documentation in |
Improves documentation completeness by adding inline comments to each field in the WebhookConfig struct, following Go documentation best practices. Signed-off-by: audacioustux <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/common.go (1)
24-37: WebhookConfig field docs look good; consider documenting “0 disables rate limiting”.The new/updated comments for all webhook secrets and
CloudEventsSecretare clear and consistent. Given that lines 133–135 treatRateLimitNumAllowedRequests <= 0as “no rate limiting”, you might make that explicit in the comment to help operators understand config behavior:- // RateLimitNumAllowedRequests is the number of allowed requests per hour for rate limiting + // RateLimitNumAllowedRequests is the number of allowed requests per hour for rate limiting (0 disables rate limiting)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/common.go(2 hunks)docs/configuration/webhook.md(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/configuration/webhook.md
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/common.go (1)
pkg/webhook/cloudevents.go (1)
NewCloudEventsWebhook(23-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Ensure registry-scanner Go modules synchronicity and run tests
- GitHub Check: Ensure unit tests are passing
- GitHub Check: Ensure code is correctly linted
- GitHub Check: build_image
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
cmd/common.go (1)
123-129: CloudEvents handler registration is consistent with existing webhook handlers.Registering
webhook.NewCloudEventsWebhook(webhookCfg.CloudEventsSecret)alongside the other registry-specific handlers follows the same pattern and matches the constructor signature frompkg/webhook/cloudevents.go. No issues spotted here.
Updates the RateLimitNumAllowedRequests field comment to explicitly state that setting it to 0 disables rate limiting, helping operators understand the configuration behavior. Signed-off-by: audacioustux <[email protected]>
|
Hi, are you available to review this PR? This change adds support for a generic webhook endpoint. This avoids the need to maintain individual logic for every container registry (like AWS ECR). Instead, we can rely on external transformation (e.g., ECR -> EventBridge Input Transformation -> ArgoCD IU). This allows users of any cloud platform to map their events to the CloudEvents schema using serverless compute or transformation rules. I have pushed an image here for testing purposes: https://github.com/audacioustux/argocd-image-updater/pkgs/container/argocd-image-updater Thank you! |
|
Hi @audacioustux |
3 participants
Summary
Adds CloudEvents v1.0 webhook handler to support AWS ECR push events via EventBridge.
Motivation
Instead of adding registry-specific webhook handlers for each cloud provider (ECR, GCR, ACR), this uses the CloudEvents specification to provide a standardized approach. AWS EventBridge can transform native ECR events to CloudEvents format using input transformers.
Changes
docs/configuration/webhook.mdconfig/examples/cloudevents/Testing
All tests pass:
Terraform validated:
terraform validate # Success!Example Usage
argocd-image-updater webhook --cloudevents-webhook-secret=$SECRETEventBridge transforms ECR events to:
{ "specversion": "1.0", "type": "com.amazon.ecr.image.push", "source": "urn:aws:ecr:us-east-1:123456789012:repository/my-repo", "subject": "my-repo:v1.0.0", "data": { "repositoryName": "my-repo", "imageTag": "v1.0.0", "imageDigest": "sha256:..." } }Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.