feat: implement webhook receiver for Docker, GHCR and Harbor to receive triggers for image update #1159
Conversation
binhnguyenduc
requested review from
jannfis,
chengfang,
pasha-codefresh and
ishitasequeira
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR adds webhook server functionality to the image updater, enabling it to respond to container registry events (Docker Hub, GHCR, Harbor) and trigger ArgoCD application updates. Key changes include the implementation of a dedicated WebhookServer and multiple registry-specific webhook handlers with associated tests, as well as CLI configuration enhancements.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/webhook/webhook*.go | Core webhook handler definitions and registry type detection |
| pkg/webhook/server.go | New HTTP server implementation for webhook requests |
| pkg/webhook/harbor.go | Added Harbor registry webhook handler and payload parsing |
| pkg/webhook/ghcr.go | Added GHCR webhook handler with validation and payload parsing |
| pkg/webhook/docker*.go | Docker Hub webhook handler and tests |
| pkg/argocd/updater_config.go | New updater configuration for image update operations |
| cmd/webhook.go | New CLI command for starting the webhook server |
| cmd/run.go & cmd/main.go | Integration of the webhook server into the main application flow |
pkg/webhook/harbor.go
Outdated
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read request body: %w", err) | ||
| } | ||
| fmt.Printf("Received Harbor webhook payload: %s", string(body)) |
There was a problem hiding this comment.
Consider replacing fmt.Printf with the project's logging utility (e.g. log.Infof) for consistency in logging throughout the codebase.
| fmt.Printf("Received Harbor webhook payload: %s", string(body)) | |
| log.Infof("Received Harbor webhook payload: %s", string(body)) |
Copilot uses AI. Check for mistakes.
| body, err := io.ReadAll(r.Body) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read request body: %w", err) | ||
| } | ||
|
|
||
| // Reset body for later reading | ||
| r.Body = io.NopCloser(strings.NewReader(string(body))) | ||
|
|
There was a problem hiding this comment.
[nitpick] The pattern for reading and then resetting the request body is repeated across several webhook validation methods. Consider extracting this logic into a shared helper function to reduce duplication and maintain consistency.
| body, err := io.ReadAll(r.Body) | |
| if err != nil { | |
| return fmt.Errorf("failed to read request body: %w", err) | |
| } | |
| // Reset body for later reading | |
| r.Body = io.NopCloser(strings.NewReader(string(body))) | |
| body, err := readAndResetBody(r) | |
| if err != nil { | |
| return fmt.Errorf("failed to read request body: %w", err) | |
| } |
Copilot uses AI. Check for mistakes.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1159 +/- ##
==========================================
- Coverage 63.34% 60.85% -2.49%
==========================================
Files 15 22 +7
Lines 2357 2989 +632
==========================================
+ Hits 1493 1819 +326
- Misses 769 1066 +297
- Partials 95 104 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ve update of image pushes and trigger application updates Signed-off-by: Binh Nguyen <binh.nguyen@encapital.io>
|
UpdaterConfig in pkg/argocd/updater_config.go: is it possible to use other existing struct, instead of creating a new one? webhook secrets: can we keep them in a secret file, similar to https://argo-cd.readthedocs.io/en/latest/operator-manual/webhook/#2-configure-argo-cd-with-the-webhook-secret-optional ? |
Yes, the service, ingress and related manifests would be great to have. |
|
How do you currently use this feature, using the standalone |
I'll update as soon as I can |
Currently, we enable webhook with the run command (via env var) and reduce the interval to 30m or 60m. That way, we have both the webhook trigger and registry scanner running side by side. One way is to potentially run the webhook command, with the registry scanner off altogether, but we haven't actually done that in production. |
I'll see what I can do. |
This PR is to address #1 and is essentially a continuation of #284
This pull request introduces a webhook server to handle container registry events, enabling automated image updates in ArgoCD applications. Key changes include adding webhook server functionality, integrating registry-specific webhook handlers, and providing configuration options for the webhook server.
Webhook Server Implementation:
WebhookServerclass to handle registry events, including initialization, event handling, and graceful shutdown. (cmd/main.go,cmd/run.go,pkg/webhook/docker.go,pkg/webhook/docker_test.go) [1] [2] [3] [4] [5] [6]WebhookOptionsand CLI command: Added a newwebhookCLI command with configuration options such as port, registry secrets, and ArgoCD integration settings. (cmd/webhook.go)Registry-Specific Webhook Handlers:
pkg/webhook/docker.go,pkg/webhook/docker_test.go) [1] [2]Configuration Enhancements:
UpdaterConfigto encapsulate settings for image updates, such as Git commit details and concurrency limits. (pkg/argocd/updater_config.go)Integration with ArgoCD:
kubernetesorargocd) for webhook server operations. (cmd/run.go,cmd/webhook.go) [1] [2]These changes collectively enhance the automation capabilities of the ArgoCD Image Updater by enabling it to respond to registry events and update applications accordingly.
I've also made sure to address concerns from the original PR:
enable-webhookand is off by defaultwebhook-portI have not update the manifests to add new Service and Ingress. If needed, let me know.
Disclaimer: the integration with Harbor is well tested and currently running on our own cluster. Unfortunately, I don't have access to Dockerhub and GHCR to test thoroughly, so any help is welcomed.