feat(webhook): add webhook support for harbor (#27810)#27884
feat(webhook): add webhook support for harbor (#27810)#27884adityaraj178 wants to merge 2 commits into
Conversation
Signed-off-by: Aditya Raj <adityaraj10600@gmail.com>
✅ Preview Environment deployed on Bunnyshell
See: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #27884 +/- ##
==========================================
+ Coverage 64.06% 64.26% +0.20%
==========================================
Files 422 423 +1
Lines 57822 57895 +73
==========================================
+ Hits 37043 37206 +163
+ Misses 17296 17180 -116
- Partials 3483 3509 +26 ☔ View full report in Codecov by Sentry. |
| if p.secret == "" { | ||
| return false | ||
| } | ||
| return r.Header.Get("Authorization") == p.secret |
There was a problem hiding this comment.
| return r.Header.Get("Authorization") == p.secret | |
| return subtle.ConstantTimeCompare([]byte(r.Header.Get("Authorization")), []byte(p.secret)) == 1 |
| [Harbor](https://goharbor.io/) is a CNCF open-source OCI-compliant registry that supports sending webhook events when artifacts are pushed. Argo CD can be configured to receive these events and instantly refresh applications backed by OCI artifacts stored in Harbor. | ||
|
|
||
| > [!NOTE] | ||
| > Harbor does not send a distinctive request header to identify webhook events, unlike GitHub which sends `X-GitHub-Event`. |
There was a problem hiding this comment.
This is cruicial information. This means if anyone captures the header value once can forget events indefinitely which further means it's weaker than GHCR's HMAC signing. Can you provide recommended methods here for users to prevent this such as rotating the secret periodically, terminate TLS etc.?
| normalizedSourceURL := normalizeOCI(source.RepoURL) | ||
| // For Helm OCI sources, repoURL is the registry+project and chart is the chart name. | ||
| // The effective full OCI URL is repoURL/chart, so also try that when chart is set. | ||
| normalizedSourceURLWithChart := normalizedSourceURL | ||
| if source.Chart != "" { | ||
| normalizedSourceURLWithChart = normalizeOCI(source.RepoURL + "/" + source.Chart) | ||
| } | ||
| if normalizedSourceURL != normalizedRepoURL && normalizedSourceURLWithChart != normalizedRepoURL { |
There was a problem hiding this comment.
these changes doesn't look specific to harbor. I want to see this in a separate PR for a good discussion there.
There was a problem hiding this comment.
Hii @nitishfy
Without this fix, Harbor webhook support is broken. In Harbor users actually configure [repoURL = registry/project]+ [chart = chartname] in practice
| if i := strings.Index(u, "://"); i != -1 { | ||
| u = u[i+3:] | ||
| } | ||
| before, _, ok := strings.Cut(u, "/") | ||
| if !ok { | ||
| return "", fmt.Errorf("harbor webhook: cannot parse registry hostname from resource_url %q", resourceURL) | ||
| } |
There was a problem hiding this comment.
Rather than stripping all this by ourselves, can't we use the net/url package here for parsing?
There was a problem hiding this comment.
net/url approach requires a hasScheme helper which will be prepending a dummy scheme because Harbor's resource_url often omits the scheme (e.g. "hub.harbor.com/test-webhook/debian:latest"). The result will be more code and less readable than now i guess.
Happy to change it if you still prefer net/url.
nitishfy
left a comment
nitishfy
left a comment
There was a problem hiding this comment.
Left some comments, PTAL.
Also, please share a recording of this feature in the PR description itself.
2 participants
Closes: #27810
Checklist: