From 04b21a8fe6a8ecaccd7e9df082f076f217927800 Mon Sep 17 00:00:00 2001 From: Nick Hale <4175918+njhale@users.noreply.github.com> Date: Mon, 7 Aug 2023 19:13:31 -0400 Subject: [PATCH] Stop recording events on app image pull Don't record events when an app image is pulled. These events are fired too frequently and don't provide much value when they do. Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com> --- pkg/controller/appdefinition/pullappimage.go | 116 +--------- .../appdefinition/pullappimage_test.go | 209 +----------------- 2 files changed, 12 insertions(+), 313 deletions(-) diff --git a/pkg/controller/appdefinition/pullappimage.go b/pkg/controller/appdefinition/pullappimage.go index 66de4d9ec..85758532b 100644 --- a/pkg/controller/appdefinition/pullappimage.go +++ b/pkg/controller/appdefinition/pullappimage.go @@ -6,7 +6,6 @@ import ( "net/http" "github.com/acorn-io/baaah/pkg/router" - apiv1 "github.com/acorn-io/runtime/pkg/apis/api.acorn.io/v1" v1 "github.com/acorn-io/runtime/pkg/apis/internal.acorn.io/v1" "github.com/acorn-io/runtime/pkg/autoupgrade" "github.com/acorn-io/runtime/pkg/condition" @@ -15,29 +14,19 @@ import ( "github.com/acorn-io/runtime/pkg/tags" imagename "github.com/google/go-containerregistry/pkg/name" "github.com/google/go-containerregistry/pkg/v1/remote" - "github.com/sirupsen/logrus" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" kclient "sigs.k8s.io/controller-runtime/pkg/client" ) func PullAppImage(transport http.RoundTripper, recorder event.Recorder) router.HandlerFunc { return pullAppImage(transport, pullClient{ - recorder: recorder, - resolve: tags.ResolveLocal, - pull: images.PullAppImage, - now: metav1.NowMicro, + pull: images.PullAppImage, }) } -type resolveImageFunc func(ctx context.Context, c kclient.Client, namespace, image string) (resolved string, isLocal bool, error error) - type pullImageFunc func(ctx context.Context, c kclient.Reader, namespace, image, nestedDigest string, opts ...remote.Option) (*v1.AppImage, error) type pullClient struct { - recorder event.Recorder - resolve resolveImageFunc - pull pullImageFunc - now func() metav1.MicroTime + pull pullImageFunc } func pullAppImage(transport http.RoundTripper, client pullClient) router.HandlerFunc { @@ -58,16 +47,19 @@ func pullAppImage(transport http.RoundTripper, client pullClient) router.Handler var ( _, autoUpgradeOn = autoupgrade.Mode(appInstance.Spec) resolved string - err error - isLocal bool ) // Only attempt to resolve locally if auto-upgrade is not on, or if auto-upgrade is on but we know the image is not remote. if !autoUpgradeOn || !images.IsImageRemote(req.Ctx, req.Client, appInstance.Namespace, target, true, remote.WithTransport(transport)) { - resolved, isLocal, err = client.resolve(req.Ctx, req.Client, appInstance.Namespace, target) + var ( + isLocal bool + err error + ) + resolved, isLocal, err = tags.ResolveLocal(req.Ctx, req.Client, appInstance.Namespace, target) if err != nil { cond.Error(err) return nil } + if !isLocal { if autoUpgradeOn && !tags.IsLocalReference(target) { ref, err := imagename.ParseReference(target, imagename.WithDefaultRegistry(images.NoDefaultRegistry)) @@ -87,21 +79,7 @@ func pullAppImage(transport http.RoundTripper, client pullClient) router.Handler resolved = target } - var ( - previousImage = appInstance.Status.AppImage - targetImage *v1.AppImage - ) - defer func() { - // Record the results as an event - if err != nil { - targetImage = &v1.AppImage{ - Name: resolved, - } - } - recordPullEvent(req.Ctx, client.recorder, client.now(), req.Object, autoUpgradeOn, err, previousImage, *targetImage) - }() - - targetImage, err = client.pull(req.Ctx, req.Client, appInstance.Namespace, resolved, "", remote.WithTransport(transport)) + targetImage, err := client.pull(req.Ctx, req.Client, appInstance.Namespace, resolved, "", remote.WithTransport(transport)) if err != nil { cond.Error(err) return nil @@ -163,79 +141,3 @@ func determineTargetImage(appInstance *v1.AppInstance) (string, string) { } } } - -const ( - AppImagePullFailureEventType = "AppImagePullFailure" - AppImagePullSuccessEventType = "AppImagePullSuccess" -) - -// AppImagePullEventDetails captures additional info about an App image pull. -type AppImagePullEventDetails struct { - // ResourceVersion is the resourceVersion of the App the image is being pulled for. - ResourceVersion string `json:"resourceVersion"` - - // AutoUpgrade is true if the pull was triggered by an auto-upgrade, false otherwise. - AutoUpgrade bool `json:"autoUpgrade"` - - // Previous is the App image before pulling, if any. - // +optional - Previous ImageSummary `json:"previous,omitempty"` - - // Target is the image being pulled. - Target ImageSummary `json:"target"` - - // Err is an error that occurred during the pull, if any. - // +optional - Err string `json:"err,omitempty"` -} - -type ImageSummary struct { - Name string `json:"name,omitempty"` - Digest string `json:"digest,omitempty"` - VCS v1.VCS `json:"vcs,omitempty"` -} - -func newImageSummary(appImage v1.AppImage) ImageSummary { - return ImageSummary{ - Name: appImage.Name, - Digest: appImage.Digest, - VCS: appImage.VCS, - } -} - -func recordPullEvent(ctx context.Context, recorder event.Recorder, observed metav1.MicroTime, obj kclient.Object, autoUpgradeOn bool, err error, previousImage, targetImage v1.AppImage) { - // Initialize with values for a success event - previous, target := newImageSummary(previousImage), newImageSummary(targetImage) - e := apiv1.Event{ - Type: AppImagePullSuccessEventType, - Severity: apiv1.EventSeverityInfo, - Description: fmt.Sprintf("Pulled %s", target.Name), - AppName: obj.GetName(), - Resource: event.Resource(obj), - Observed: apiv1.MicroTime(observed), - } - e.SetNamespace(obj.GetNamespace()) - - details := AppImagePullEventDetails{ - ResourceVersion: obj.GetResourceVersion(), - AutoUpgrade: autoUpgradeOn, - Previous: previous, - Target: target, - } - - if err != nil { - // It's a failure, overwrite with failure event values - e.Type = AppImagePullFailureEventType - e.Severity = v1.EventSeverityError - e.Description = fmt.Sprintf("Failed to pull %s", target.Name) - details.Err = err.Error() - } - - if e.Details, err = apiv1.Mapify(details); err != nil { - logrus.Warnf("Failed to mapify event details: %s", err.Error()) - } - - if err := recorder.Record(ctx, &e); err != nil { - logrus.Warnf("Failed to record event: %s", err.Error()) - } -} diff --git a/pkg/controller/appdefinition/pullappimage_test.go b/pkg/controller/appdefinition/pullappimage_test.go index 250403d9b..709159198 100644 --- a/pkg/controller/appdefinition/pullappimage_test.go +++ b/pkg/controller/appdefinition/pullappimage_test.go @@ -2,22 +2,15 @@ package appdefinition import ( "context" - "fmt" "net/http" "testing" "github.com/acorn-io/baaah/pkg/router" "github.com/acorn-io/baaah/pkg/router/tester" - apiv1 "github.com/acorn-io/runtime/pkg/apis/api.acorn.io/v1" v1 "github.com/acorn-io/runtime/pkg/apis/internal.acorn.io/v1" - "github.com/acorn-io/runtime/pkg/event" "github.com/acorn-io/runtime/pkg/scheme" - "github.com/acorn-io/runtime/pkg/tags" "github.com/google/go-containerregistry/pkg/v1/remote" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" kclient "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -85,207 +78,14 @@ func app(specImage, statusImageID, statusAvailableImage, statusConfirmUpgradeIma } } -func withMeta[T kclient.Object](name, uid, resourceVersion string, obj T) T { - obj.SetName(name) - obj.SetUID(types.UID(uid)) - obj.SetResourceVersion(resourceVersion) - - return obj -} - -func TestPullAppImageEvents(t *testing.T) { - // Test cases below this comment ensure the handler produces the correct events - now := v1.NowMicro() - // Manual upgrade should record an event - testRecordPullEvent(t, - "ImageChange", - withMeta("foo", "foo-uid", "1", app("acorn.io/img:1", "acorn.io/img:0", "", "", false, false)), - resolveImageErr(nil), - pullImageTo(&v1.AppImage{Name: "acorn.io/img:1", Digest: "sha256:abcd", VCS: v1.VCS{Revision: "r", Modified: false}}, nil), - now, - &apiv1.Event{ - Type: AppImagePullSuccessEventType, - Severity: v1.EventSeverityInfo, - Description: "Pulled acorn.io/img:1", - AppName: "foo", - Resource: &v1.EventResource{Kind: "app", Name: "foo", UID: types.UID("foo-uid")}, - Observed: now, - Details: mustMapify(t, AppImagePullEventDetails{ - ResourceVersion: "1", - AutoUpgrade: false, - Previous: ImageSummary{Name: "acorn.io/img:0"}, - Target: ImageSummary{Name: "acorn.io/img:1", Digest: "sha256:abcd", VCS: v1.VCS{Revision: "r", Modified: false}}, - }), - }, - ) - - // Auto-upgrade should record an event when re-pulling - testRecordPullEvent(t, - "AutoUpgrade", - withMeta("foo", "", "", app("acorn.io/img:1", "acorn.io/img:1", "acorn.io/img:1", "", true, false)), - resolveImageErr(nil), - pullImageTo(&v1.AppImage{Name: "acorn.io/img:1", Digest: "sha256:abcd", VCS: v1.VCS{Revision: "r", Modified: false}}, nil), - now, - &apiv1.Event{ - Type: AppImagePullSuccessEventType, - Severity: v1.EventSeverityInfo, - Description: "Pulled acorn.io/img:1", - AppName: "foo", - Resource: &v1.EventResource{Kind: "app", Name: "foo"}, - Observed: now, - Details: mustMapify(t, AppImagePullEventDetails{ - AutoUpgrade: true, - Previous: ImageSummary{Name: "acorn.io/img:1"}, - Target: ImageSummary{Name: "acorn.io/img:1", Digest: "sha256:abcd", VCS: v1.VCS{Revision: "r", Modified: false}}, - }), - }, - ) - - testRecordPullEvent(t, - "Pattern", - withMeta("foo", "", "", app("acorn.io/img:#", "", "", "acorn.io/img:1", false, true)), - resolveImageErr(nil), - pullImageTo(&v1.AppImage{Name: "acorn.io/img:1"}, nil), - now, - &apiv1.Event{ - Type: AppImagePullSuccessEventType, - Severity: v1.EventSeverityInfo, - Description: "Pulled acorn.io/img:1", - AppName: "foo", - Resource: &v1.EventResource{Kind: "app", Name: "foo"}, - Observed: now, - Details: mustMapify(t, AppImagePullEventDetails{ - AutoUpgrade: true, - Target: ImageSummary{Name: "acorn.io/img:1"}, - }), - }, - ) - - // Errors on image pull records an event - testRecordPullEvent(t, - "PullError", - withMeta("foo", "", "", app("acorn.io/img:1", "", "", "", false, false)), - resolveImageTo("acorn.io/img:1", false, nil), - pullImageTo(nil, fmt.Errorf("pull error!")), - now, - &apiv1.Event{ - Type: AppImagePullFailureEventType, - Severity: v1.EventSeverityError, - Description: "Failed to pull acorn.io/img:1", - AppName: "foo", - Resource: &v1.EventResource{Kind: "app", Name: "foo"}, - Observed: now, - Details: mustMapify(t, AppImagePullEventDetails{ - Target: ImageSummary{Name: "acorn.io/img:1"}, - Err: "pull error!", - }), - }, - ) - - // Test cases below this comment should produce no events - - // No image change records NO events - testRecordPullEvent(t, - "NoImageChange", - withMeta("foo", "", "", app("acorn.io/img:1", "acorn.io/img:1", "", "", false, false)), - nil, - nil, - now, - nil, - ) - - // Error during local image resolution records NO events - testRecordPullEvent(t, - "ResolutionError", - withMeta("foo", "", "", app("acorn.io/img:1", "", "", "", false, false)), - resolveImageTo("", false, fmt.Errorf("resolution error!")), - nil, - now, - nil, - ) - - testRecordPullEvent(t, - "Pattern/2", - withMeta("foo", "", "", app("acorn.io/img:#", "acorn.io/img:1", "", "acorn.io/img:2", false, true)), - resolveImageErr(nil), - pullImageTo(&v1.AppImage{Name: "acorn.io/img:2"}, nil), - now, - nil, - ) -} - -func mustMapify(t *testing.T, obj any) v1.GenericMap { - t.Helper() - m, err := v1.Mapify(obj) - require.NoError(t, err) - return m -} - -func resolveImageTo(resolved string, isLocal bool, err error) resolveImageFunc { - return func(context.Context, kclient.Client, string, string) (string, bool, error) { - return resolved, isLocal, err - } -} - -func resolveImageErr(err error) resolveImageFunc { - return func(context.Context, kclient.Client, string, string) (string, bool, error) { - return "", false, err - } -} - -func pullImageTo(image *v1.AppImage, err error) pullImageFunc { - return func(context.Context, kclient.Reader, string, string, string, ...remote.Option) (*v1.AppImage, error) { - return image, err - } -} - -func testRecordPullEvent(t *testing.T, testName string, appInstance *v1.AppInstance, resolve resolveImageFunc, pull pullImageFunc, now v1.MicroTime, expect *apiv1.Event) { - t.Helper() - var recording []*apiv1.Event - fakeRecorder := func(_ context.Context, e *apiv1.Event) error { - recording = append(recording, e) - return nil - } - - handler := pullAppImage(mockRoundTripper{}, pullClient{ - recorder: event.RecorderFunc(fakeRecorder), - resolve: resolve, - pull: pull, - now: func() metav1.MicroTime { - return metav1.MicroTime(now) - }, - }) - - t.Run(testName, func(t *testing.T) { - results, err := (&tester.Harness{ - Scheme: scheme.Scheme, - }).Invoke(t, appInstance, handler) - - require.NoError(t, err) - assert.Zero(t, results.Client.Created) - - if expect == nil { - assert.Len(t, recording, 0) - return - } - - assert.Len(t, recording, 1) - assert.EqualValues(t, expect, recording[0]) - }) -} - func TestAutoUpgradeImageResolution(t *testing.T) { // Auto-upgrade apps are not supposed to use Docker Hub implicitly. // In this test, we create an auto-upgrade App with the image "myimage:latest". // In the first test case, this image exists locally and should be resolved properly. // In the second test case, no such image exists locally, and Acorn should not reach out to Docker Hub, and should instead return an error. - fakeRecorder := func(_ context.Context, _ *apiv1.Event) error { - return nil - } - // First, test to make sure that the local image is properly resolved - tester.DefaultTest(t, scheme.Scheme, "testdata/autoupgrade/with-local-image", testPullAppImage(mockRoundTripper{}, event.RecorderFunc(fakeRecorder))) + tester.DefaultTest(t, scheme.Scheme, "testdata/autoupgrade/with-local-image", testPullAppImage(mockRoundTripper{})) // Next, test to make sure that Docker Hub is not implicitly used when no local image is found // There should be a helpful error message instead @@ -293,24 +93,21 @@ func TestAutoUpgradeImageResolution(t *testing.T) { if err != nil { t.Fatal(err) } - _, err = harness.InvokeFunc(t, obj, testPullAppImage(mockRoundTripper{}, event.RecorderFunc(fakeRecorder))) + _, err = harness.InvokeFunc(t, obj, testPullAppImage(mockRoundTripper{})) if err == nil { t.Fatalf("expected error when no local image was found for auto-upgrade app without a specified registry") } assert.ErrorContains(t, err, "no local image found for myimage:latest - if you are trying to use a remote image, specify the full registry") } -func testPullAppImage(transport http.RoundTripper, recorder event.Recorder) router.HandlerFunc { +func testPullAppImage(transport http.RoundTripper) router.HandlerFunc { return pullAppImage(transport, pullClient{ - recorder: recorder, - resolve: tags.ResolveLocal, pull: func(_ context.Context, _ kclient.Reader, _ string, _ string, _ string, _ ...remote.Option) (*v1.AppImage, error) { return &v1.AppImage{ Name: "myimage:latest", Digest: "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", }, nil }, - now: metav1.NowMicro, }) }