From 1ccf839dbc4df79719235522ac0bebbf9a7a86b3 Mon Sep 17 00:00:00 2001 From: akutz Date: Fri, 24 Jan 2025 13:18:14 -0600 Subject: [PATCH] Do not panic in CLS controllers This patch fixes a panic in the ContentLibrary controllers that occurs due to a missing JoinContext for ovfcache in the Reconcile function. --- ...contentlibraryitem_controller_intg_test.go | 6 ++- .../utils/controller_builder.go | 2 + .../utils/controller_builder_test.go | 41 ++++++++++++------- 3 files changed, 34 insertions(+), 15 deletions(-) diff --git a/controllers/contentlibrary/contentlibraryitem/contentlibraryitem_controller_intg_test.go b/controllers/contentlibrary/contentlibraryitem/contentlibraryitem_controller_intg_test.go index d1242fa19..2d10851b1 100644 --- a/controllers/contentlibrary/contentlibraryitem/contentlibraryitem_controller_intg_test.go +++ b/controllers/contentlibrary/contentlibraryitem/contentlibraryitem_controller_intg_test.go @@ -21,6 +21,7 @@ import ( "github.com/vmware-tanzu/vm-operator/controllers/contentlibrary/utils" "github.com/vmware-tanzu/vm-operator/pkg/conditions" "github.com/vmware-tanzu/vm-operator/pkg/constants/testlabels" + "github.com/vmware-tanzu/vm-operator/pkg/util/ovfcache" "github.com/vmware-tanzu/vm-operator/test/builder" ) @@ -76,7 +77,10 @@ func intgTestsReconcile() { ctx = suite.NewIntegrationTestContext() intgFakeVMProvider.Lock() - intgFakeVMProvider.SyncVirtualMachineImageFn = func(_ context.Context, _, vmiObj client.Object) error { + intgFakeVMProvider.SyncVirtualMachineImageFn = func(ctx context.Context, _, vmiObj client.Object) error { + // Verify ovfcache is in context and does not panic. + _, _ = ovfcache.GetOVFEnvelope(ctx, "", "") + vmi := vmiObj.(*vmopv1.VirtualMachineImage) // Use Firmware field to verify the provider function is called. vmi.Status.Firmware = firmwareValue diff --git a/controllers/contentlibrary/utils/controller_builder.go b/controllers/contentlibrary/utils/controller_builder.go index 59b173012..c0329c765 100644 --- a/controllers/contentlibrary/utils/controller_builder.go +++ b/controllers/contentlibrary/utils/controller_builder.go @@ -33,6 +33,7 @@ import ( "github.com/vmware-tanzu/vm-operator/pkg/providers" "github.com/vmware-tanzu/vm-operator/pkg/record" imgutil "github.com/vmware-tanzu/vm-operator/pkg/util/image" + "github.com/vmware-tanzu/vm-operator/pkg/util/ovfcache" vmopv1util "github.com/vmware-tanzu/vm-operator/pkg/util/vmopv1" ) @@ -123,6 +124,7 @@ func (r *Reconciler) Reconcile( req ctrl.Request) (_ ctrl.Result, reterr error) { ctx = pkgcfg.JoinContext(ctx, r.Context) + ctx = ovfcache.JoinContext(ctx, r.Context) logger := ctrl.Log.WithName(r.Kind).WithValues("name", req.String()) diff --git a/controllers/contentlibrary/utils/controller_builder_test.go b/controllers/contentlibrary/utils/controller_builder_test.go index 9dafe32da..521215e5a 100644 --- a/controllers/contentlibrary/utils/controller_builder_test.go +++ b/controllers/contentlibrary/utils/controller_builder_test.go @@ -30,6 +30,7 @@ import ( pkgerr "github.com/vmware-tanzu/vm-operator/pkg/errors" providerfake "github.com/vmware-tanzu/vm-operator/pkg/providers/fake" pkgutil "github.com/vmware-tanzu/vm-operator/pkg/util" + "github.com/vmware-tanzu/vm-operator/pkg/util/ovfcache" "github.com/vmware-tanzu/vm-operator/pkg/util/ptr" "github.com/vmware-tanzu/vm-operator/test/builder" ) @@ -140,7 +141,10 @@ var _ = Describe("Reconcile", "ContentLibraryItem", ) - fakeVMProvider.SyncVirtualMachineImageFn = func(_ context.Context, _, vmiObj client.Object) error { + fakeVMProvider.SyncVirtualMachineImageFn = func(ctx context.Context, _, vmiObj client.Object) error { + // Verify ovfcache is in context and does not panic. + _, _ = ovfcache.GetOVFEnvelope(ctx, "", "") + vmi := vmiObj.(*vmopv1.VirtualMachineImage) // Use Firmware field to verify the provider function is called. @@ -170,7 +174,7 @@ var _ = Describe("Reconcile", }) It("should add the finalizer", func() { - _, err := reconciler.Reconcile(ctx, req) + _, err := reconciler.Reconcile(context.Background(), req) Expect(err).ToNot(HaveOccurred()) cliObj, _, _ = getCLI(ctx, req.Namespace, req.Name) @@ -189,7 +193,7 @@ var _ = Describe("Reconcile", }) It("should mark image resource condition as provider not ready", func() { - _, err := reconciler.Reconcile(ctx, req) + _, err := reconciler.Reconcile(context.Background(), req) Expect(err).ToNot(HaveOccurred()) _, _, vmiStatus := getVMI(ctx, req.Namespace, vmiName) @@ -207,7 +211,7 @@ var _ = Describe("Reconcile", }) It("should mark image resource condition as provider security not compliant", func() { - _, err := reconciler.Reconcile(ctx, req) + _, err := reconciler.Reconcile(context.Background(), req) Expect(err).ToNot(HaveOccurred()) _, _, vmiStatus := getVMI(ctx, req.Namespace, vmiName) @@ -221,13 +225,16 @@ var _ = Describe("Reconcile", When("SyncVirtualMachineImage returns an error", func() { BeforeEach(func() { - fakeVMProvider.SyncVirtualMachineImageFn = func(_ context.Context, _, _ client.Object) error { + fakeVMProvider.SyncVirtualMachineImageFn = func(ctx context.Context, _, _ client.Object) error { + // Verify ovfcache is in context and does not panic. + _, _ = ovfcache.GetOVFEnvelope(ctx, "", "") + return fmt.Errorf("sync-error") } }) It("should mark image resource condition synced failed", func() { - _, err := reconciler.Reconcile(ctx, req) + _, err := reconciler.Reconcile(context.Background(), req) Expect(err).To(MatchError("sync-error")) _, _, vmiStatus := getVMI(ctx, req.Namespace, vmiName) @@ -239,13 +246,16 @@ var _ = Describe("Reconcile", When("error is ErrVMICacheNotReady", func() { JustBeforeEach(func() { - fakeVMProvider.SyncVirtualMachineImageFn = func(_ context.Context, _, _ client.Object) error { + fakeVMProvider.SyncVirtualMachineImageFn = func(ctx context.Context, _, _ client.Object) error { + // Verify ovfcache is in context and does not panic. + _, _ = ovfcache.GetOVFEnvelope(ctx, "", "") + return fmt.Errorf("failed with %w", pkgerr.VMICacheNotReadyError{Name: vmicName}) } }) It("should place a label on the library item resource", func() { - _, err := reconciler.Reconcile(ctx, req) + _, err := reconciler.Reconcile(context.Background(), req) var e pkgerr.VMICacheNotReadyError ExpectWithOffset(1, errors.As(err, &e)).To(BeTrue()) @@ -283,7 +293,7 @@ var _ = Describe("Reconcile", When("Image resource has not been created yet", func() { It("should create a new image resource syncing up with the library item resource", func() { - _, err := reconciler.Reconcile(ctx, req) + _, err := reconciler.Reconcile(context.Background(), req) Expect(err).ToNot(HaveOccurred()) cliObj, cliSpec, cliStatus = getCLI(ctx, req.Namespace, req.Name) @@ -308,7 +318,7 @@ var _ = Describe("Reconcile", It("should update the existing image resource with the library item resource", func() { cliStatus.ContentVersion += "-updated" - _, err := reconciler.Reconcile(ctx, req) + _, err := reconciler.Reconcile(context.Background(), req) Expect(err).ToNot(HaveOccurred()) cliObj, cliSpec, cliStatus = getCLI(ctx, req.Namespace, req.Name) @@ -332,7 +342,7 @@ var _ = Describe("Reconcile", }) It("should still update the image resource status from the library item resource", func() { - _, err := reconciler.Reconcile(ctx, req) + _, err := reconciler.Reconcile(context.Background(), req) Expect(err).ToNot(HaveOccurred()) cliObj, cliSpec, cliStatus = getCLI(ctx, req.Namespace, req.Name) @@ -367,7 +377,10 @@ var _ = Describe("Reconcile", "ClusterContentLibraryItem", ) - fakeVMProvider.SyncVirtualMachineImageFn = func(_ context.Context, _, vmiObj client.Object) error { + fakeVMProvider.SyncVirtualMachineImageFn = func(ctx context.Context, _, vmiObj client.Object) error { + // Verify ovfcache is in context and does not panic. + _, _ = ovfcache.GetOVFEnvelope(ctx, "", "") + vmi := vmiObj.(*vmopv1.ClusterVirtualMachineImage) // Use Firmware field to verify the provider function is called. @@ -408,7 +421,7 @@ var _ = Describe("Reconcile", When("Image resource has not been created yet", func() { It("should create a new image resource syncing up with the library item resource", func() { - _, err := reconciler.Reconcile(ctx, req) + _, err := reconciler.Reconcile(context.Background(), req) Expect(err).ToNot(HaveOccurred()) cliObj, cliSpec, cliStatus = getCLI(ctx, req.Namespace, req.Name) @@ -431,7 +444,7 @@ var _ = Describe("Reconcile", }) It("should update the existing image resource with the library item resource", func() { cliStatus.ContentVersion += "-updated" - _, err := reconciler.Reconcile(ctx, req) + _, err := reconciler.Reconcile(context.Background(), req) Expect(err).ToNot(HaveOccurred()) cliObj, cliSpec, cliStatus = getCLI(ctx, req.Namespace, req.Name)