Skip to content

Commit

Permalink
Merge pull request #868 from akutz/fix/panic-in-cls-controllers
Browse files Browse the repository at this point in the history 
🐛 Do not panic in CLS controllers
  • Loading branch information
@akutz
akutz authored Jan 24, 2025
2 parents 2b75142 + 1ccf839 commit 0101029
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 15 deletions.
6 changes: 5 additions & 1 deletion controllers/contentlibrary/contentlibraryitem/contentlibraryitem_controller_intg_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions controllers/contentlibrary/utils/controller_builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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())

Expand Down
41 changes: 27 additions & 14 deletions controllers/contentlibrary/utils/controller_builder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)

Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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())
Expand Down Expand Up @@ -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)

Expand All @@ -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)

Expand All @@ -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)

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)

Expand All @@ -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)

Expand Down

0 comments on commit 0101029

Please sign in to comment.