feat(events): include actor and request metadata in webhook payloads#3959
Conversation
fd33f76 to
45af4fd
Compare
8546997 to
15a705f
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends the events extension’s CloudEvents payloads to optionally include actor (authenticated username) and HTTP request metadata by propagating an EventContext from the API layer down into the storage layer where events are emitted.
Changes:
- Introduces
EventContext(ActorInfo,RequestInfo) plus context helpers inpkg/extensions/events, and teaches the event builder to injectactor/requestinto eventdata. - Plumbs
context.ContextthroughImageStoreAPIs (InitRepo,PutImageManifest,DeleteImageManifest) and updates call sites so storage-emitted events can access the context at fire time. - Adds/updates tests and mocks across storage, extensions, and API to cover the new event payload fields.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/test/mocks/image_store_mock.go | Updates mock ImageStore method signatures to accept context.Context. |
| pkg/test/image-utils/write.go | Passes context.Background() into updated ImageStore APIs in test helpers. |
| pkg/test/image-utils/write_test.go | Updates tests to match new mock function signatures with context.Context. |
| pkg/storage/types/types.go | Adds context.Context to ImageStore interface methods used by API/storage. |
| pkg/storage/storage_test.go | Updates storage tests and event-capture interface for new event context params. |
| pkg/storage/scrub_test.go | Updates scrub tests to call InitRepo/PutImageManifest with context. |
| pkg/storage/s3/s3_test.go | Updates S3 storage tests for context-bearing ImageStore APIs. |
| pkg/storage/local/local_test.go | Updates local storage tests/fuzzers for context-bearing ImageStore APIs. |
| pkg/storage/local/local_elevated_test.go | Updates elevated local test to call InitRepo with context. |
| pkg/storage/imagestore/imagestore.go | Plumbs context into repo init / manifest put/delete and attaches EventContext to emitted events. |
| pkg/storage/gcs/gcs_test.go | Updates GCS storage tests for new PutImageManifest/InitRepo/DeleteImageManifest signatures. |
| pkg/storage/gc/gc_test.go | Updates GC tests to call PutImageManifest with context. |
| pkg/storage/gc/gc_internal_test.go | Updates GC internal tests to call PutImageManifest with context. |
| pkg/storage/common/common_test.go | Updates common storage tests to call PutImageManifest with context. |
| pkg/meta/parse_test.go | Updates parse tests to call DeleteImageManifest with context. |
| pkg/meta/hooks.go | Updates rollback/restore paths to call ImageStore APIs with context. |
| pkg/meta/hooks_test.go | Updates test ImageStore implementation to match new Delete signature. |
| pkg/meta/hooks_internal_test.go | Updates mock function types for new context-bearing ImageStore methods. |
| pkg/extensions/sync/sync_test.go | Updates sync tests to call DeleteImageManifest with context. |
| pkg/extensions/sync/sync_internal_test.go | Updates sync internal tests to call PutImageManifest with context. |
| pkg/extensions/sync/destination.go | Uses context.Background() when syncing manifests into destination store. |
| pkg/extensions/search/search_test.go | Updates search tests’ mock store functions for new context-bearing signatures. |
| pkg/extensions/events/events.go | Extends recorder methods to accept *EventContext and inject it into payloads. |
| pkg/extensions/events/events_test.go | Adds coverage asserting actor/request presence/omission, incl. HTTP sink. |
| pkg/extensions/events/common.go | Adds EventContext types and context helpers; updates Recorder interface signatures. |
| pkg/extensions/events/builder.go | Adds WithEventContext to conditionally include actor/request in event data. |
| pkg/api/routes.go | Builds EventContext from HTTP request and attaches it to context passed into storage. |
| pkg/api/routes_test.go | Updates route tests’ mocked store functions to accept context.Context. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Take a look at this as well. |
|
yes i saw it i will work on this PR a bit. I had it all figured out and then broke it on the rebase. copilot had a similar solution as this one. |
b9a9589 to
b655d8e
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3959 +/- ##
==========================================
+ Coverage 91.64% 91.67% +0.03%
==========================================
Files 200 200
Lines 29663 29704 +41
==========================================
+ Hits 27184 27232 +48
+ Misses 1596 1591 -5
+ Partials 883 881 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
b655d8e to
1e81b7d
Compare
|
@cainydev are you planning on working on this? |
yes - i just couldn't figure out why the ci failed tbh. maybe flaky? |
|
The fix should target the failing blackbox test behind this workflow step: Job definition: .github/workflows/ecosystem-tools.yaml PullImage from image service failed ok 20 [release] push OCI artifact with regclient Solution The logs show many blob uploads completing with PUT, then the runtime pull fails while copying config. That points to a race: the test proceeds as soon as push returns, but CRI pulls before the config blob is consistently available to the image service. Recommended code change bash wait for manifest to be readablefor i in $(seq 1 30); do resolve and wait for config blobconfig_digest="$(regctl manifest get "localhost:${PORT}/test-regclient@${manifest_digest}" --format '{{ .Config.Digest }}')" for i in $(seq 1 30); do sudo crictl pull "localhost:${PORT}/test-regclient@${manifest_digest}" Why this should fix it copying config: context canceled Additional hardening suggestion bash bash Summary Locate the failing regclient pull test in the blackbox suite run by make run-blackbox-ci. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pkg/storage/imagestore/imagestore.go:945
- Calling
InitRepo(context.Background(), repo)here means that if a repository is first created via blob upload (a common push flow),initRepo()will emitRepositoryCreatedwith a nil EventContext, even when the trigger was an authenticated HTTP request. To preserve actor/request metadata, consider threading a request context through blob-upload APIs (or ensure higher layers callInitRepo(ctx, repo)before invoking blob upload methods).
// NewBlobUpload returns the unique ID for an upload in progress.
func (is *ImageStore) NewBlobUpload(repo string) (string, error) {
if err := is.InitRepo(context.Background(), repo); err != nil {
is.log.Error().Err(err).Msg("failed to initialize repo")
return "", err
}
vrajashkr
left a comment
There was a problem hiding this comment.
@cainydev thanks for the PR!
The changes look good to me overall. Left a couple of comments
I think there is a file where the context hasn't been added in yet:
Error: pkg/extensions/monitoring/monitoring_test.go:539:87: not enough arguments in call to imgStore.FullBlobUpload
a9a7da2 to
fc4ce28
Compare
|
Rebased onto latest main and pushed updates addressing the review feedback and the CI failure. CI failure (lint + extensions tests). This was a build break, not flakiness. After main merged the trivy SBOM artifact feature and a new GC-metrics test, both call ImageStore methods whose signatures this PR changes to take context.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
pkg/meta/hooks.go:1
rollbackDigestManifestTagsreceives actx context.Contextbut usescontext.Background()for store operations. This drops cancellation/deadline and also loses any request/event metadata that was attached to the incoming context. Use the providedctxforDeleteImageManifestandPutImageManifesthere so rollbacks behave consistently with the initiating request.
package meta
test/blackbox/helpers_zot.bash:1
- This retry loop depends on external
seq, which may not exist in minimal CI environments (and adds overhead). Prefer a native bash arithmetic loop (e.g.,for ((i=1; i<=attempts; i++))) to avoid the dependency and improve portability. Consider also treatingattemptsas an integer (local -i attempts=...) to avoid surprising behavior when given non-numeric input.
|
Also please rebase. |
fc4ce28 to
5ce21f7
Compare
Signed-off-by: cainydev <wajo432@gmail.com>
Add ctx context.Context to NewBlobUpload, FullBlobUpload, PutBlobChunk, PutBlobChunkStreamed and CheckBlob in the storage ImageStore interface. HTTP handlers now thread events.WithEventContext through these calls so RepositoryCreated events emitted on first push or cross-repo mount carry actor and request metadata instead of context.Background(). Signed-off-by: cainydev <wajo432@gmail.com>
On busy CI runners, CRI-O occasionally cancels its own context during the post-download "copying config" step even though zot has served every blob with 200 OK. The pull-by-name test in pushpull, fips140 and upgrade fails as a result. A fresh pull replays from cri-o's local cache and almost always succeeds, so wrap crictl pull in a bounded retry helper. Signed-off-by: cainydev <wajo432@gmail.com>
Signed-off-by: cainydev <wajo432@gmail.com>
Upstream main added a FullBlobUpload call site in the GC metrics test after this branch changed the method signature to accept context.Context. Pass context.Background() so the package compiles after rebase. Signed-off-by: cainydev <wajo432@gmail.com>
Lint is a property of the manifest, not the tag(s) it is applied under, so emit a single ImageLintFailed event per manifest rather than one per changed tag. Set changedTags to the path reference in the non-extra-tags path so ImageUpdated emission collapses to a single loop over changedTags instead of an if/else fallback. Addresses review feedback from @andaaron. Signed-off-by: cainydev <wajo432@gmail.com>
The trivy SBOM artifact generation (project-zot#4088) calls CheckBlob, FullBlobUpload and PutImageManifest, whose signatures now take context.Context. Pass the context already in scope through storeSBOMAsOCIArtifact so the search extension compiles. Signed-off-by: cainydev <wajo432@gmail.com>
Add a nil-context guard to EventContextFromContext so it returns nil instead of panicking on a nil ctx, and use the request-scoped ctx in rollbackDigestManifestTags for DeleteImageManifest/PutImageManifest so rollbacks honor cancellation and carry event metadata. Signed-off-by: cainydev <wajo432@gmail.com>
5ce21f7 to
318998e
Compare
Summary
Closes #3859
actor(username) andrequest(addr, method, useragent) fields to CloudEvents payloads forImageUpdated,ImageDeleted,ImageLintFailed, andRepositoryCreatedeventsEventContext,ActorInfo, andRequestInfotypes inpkg/extensions/events/common.go(no build tag — always compiled so the storage layer can reference them unconditionally)UpdateManifest,DeleteManifest) builds anEventContextfrom the live HTTP request and attaches it to the context viaevents.WithEventContextbefore passing it down to storageimagestore) extracts the context at event-fire time viaevents.EventContextFromContext; nil context (internal/GC paths) cleanly omits both fields from the payloadExample payload:
{ "name": "space/my-image", "reference": "latest", "digest": "sha256:abc...", "mediaType": "application/vnd.oci.image.manifest.v1+json", "actor": { "name": "john" }, "request": { "addr": "192.168.0.1:54321", "method": "PUT", "useragent": "docker/24.0.5" } }