[release/v1.4.x] fix(konnect): fix panic in KonnectAPIAuthConfigurationReconciler (#890)

controller/gateway/controller.go

@@ -331,11 +331,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	}
 
 	// if the controlplane wasn't ready before this reconciliation loop and now is ready, log this event
-	if !k8sutils.IsConditionTrue(ControlPlaneReadyType, oldGwConditionsAware) {
+	if !k8sutils.HasConditionTrue(ControlPlaneReadyType, oldGwConditionsAware) {
 		log.Debug(logger, "controlplane is ready", gateway)
 	}
 	// If the dataplane has not been marked as ready yet, return and wait for the next reconciliation loop.
-	if !k8sutils.IsConditionTrue(DataPlaneReadyType, gwConditionAware) {
+	if !k8sutils.HasConditionTrue(DataPlaneReadyType, gwConditionAware) {
 		log.Debug(logger, "controlplane is ready, but dataplane is not ready yet", gateway)
 		return ctrl.Result{}, nil
 	}

controller/konnect/reconciler_konnectapiauth.go

@@ -146,7 +146,18 @@ func (r *KonnectAPIAuthConfigurationReconciler) Reconcile(
 	// NOTE: This is needed because currently the SDK only lists the prod global API as supported:
 	// https://github.com/Kong/sdk-konnect-go/blob/999d9a987e1aa7d2e09ac11b1450f4563adf21ea/models/operations/getorganizationsme.go#L10-L12
 	respOrg, err := sdk.GetMeSDK().GetOrganizationsMe(ctx, sdkkonnectops.WithServerURL(serverURL.String()))
-	if err != nil {
+	if err != nil ||
+		respOrg == nil ||
+		respOrg.MeOrganization == nil ||
+		respOrg.MeOrganization.ID == nil {
+
+		var errMsg string
+		if err != nil {
+			errMsg = err.Error()
+		} else {
+			errMsg = "response from Konnect is nil"
+		}
+
 		logger.Error(err, "failed to get organization info from Konnect")
 		if cond, ok := k8sutils.GetCondition(konnectv1alpha1.KonnectEntityAPIAuthConfigurationValidConditionType, &apiAuth); !ok ||
 			cond.Status != metav1.ConditionFalse ||
@@ -163,7 +174,7 @@ func (r *KonnectAPIAuthConfigurationReconciler) Reconcile(
 				konnectv1alpha1.KonnectEntityAPIAuthConfigurationValidConditionType,
 				metav1.ConditionFalse,
 				konnectv1alpha1.KonnectEntityAPIAuthConfigurationReasonInvalid,
-				err.Error(),
+				errMsg,
 			)
 
 			_, errUpdate := patch.ApplyStatusPatchIfNotEmpty(ctx, r.client, ctrllog.FromContext(ctx), &apiAuth, old)

pkg/utils/kubernetes/status.go

@@ -65,16 +65,27 @@ func GetCondition(cType consts.ConditionType, resource ConditionsAware) (metav1.
 	return metav1.Condition{}, false
 }
 
-// IsConditionTrue returns a true value whether the condition is ConditionTrue, false otherwise
-func IsConditionTrue(cType consts.ConditionType, resource ConditionsAware) bool {
+// hasConditionWithStatus returns true if the provided resource has a condition
+// with the given type and status.
+func hasConditionWithStatus(cType consts.ConditionType, resource ConditionsAware, status metav1.ConditionStatus) bool {
 	for _, condition := range resource.GetConditions() {
 		if condition.Type == string(cType) {
-			return condition.Status == metav1.ConditionTrue
+			return condition.Status == status
 		}
 	}
 	return false
 }
 
+// HasConditionFalse returns true if the condition on the resource has Status set to ConditionFalse, false otherwise.
+func HasConditionFalse(cType consts.ConditionType, resource ConditionsAware) bool {
+	return hasConditionWithStatus(cType, resource, metav1.ConditionFalse)
+}
+
+// HasConditionTrue returns true if the condition on the resource has Status set to ConditionTrue, false otherwise.
+func HasConditionTrue(cType consts.ConditionType, resource ConditionsAware) bool {
+	return hasConditionWithStatus(cType, resource, metav1.ConditionTrue)
+}
+
 // InitReady initializes the Ready status to False if Ready condition is not
 // yet set on the resource.
 func InitReady(resource ConditionsAndGenerationAware) bool {

pkg/utils/kubernetes/status_test.go

@@ -332,7 +332,7 @@ func TestIsValidCondition(t *testing.T) {
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
-			current := IsConditionTrue(consts.ConditionType(tt.input), resource)
+			current := HasConditionTrue(consts.ConditionType(tt.input), resource)
 			assert.Equal(t, current, tt.expected)
 		})
 	}

test/envtest/controller.go

@@ -36,6 +36,10 @@ func NewManager(t *testing.T, ctx context.Context, cfg *rest.Config, s *runtime.
 			BindAddress: "0",
 		},
 		Controller: config.Controller{
+			// We don't want to hide panics in tests so expose them by failing the test run rather
+			// than silently ignoring them.
+			RecoverPanic: lo.ToPtr(false),
+
 			// This is needed because controller-runtime keeps a global list of controller
 			// names and panics if there are duplicates.
 			// This is a workaround for that in tests.

test/envtest/konnect_entities_konnectapiauthconfiguration_test.go

@@ -0,0 +1,160 @@
+package envtest
+
+import (
+	"context"
+	"testing"
+
+	sdkkonnectcomp "github.com/Kong/sdk-konnect-go/models/components"
+	sdkkonnectops "github.com/Kong/sdk-konnect-go/models/operations"
+	sdkkonnecterrs "github.com/Kong/sdk-konnect-go/models/sdkerrors"
+	"github.com/samber/lo"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/watch"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/kong/gateway-operator/controller/konnect"
+	sdkmocks "github.com/kong/gateway-operator/controller/konnect/ops/sdk/mocks"
+	"github.com/kong/gateway-operator/modules/manager/scheme"
+	k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes"
+	"github.com/kong/gateway-operator/test/helpers/deploy"
+
+	konnectv1alpha1 "github.com/kong/kubernetes-configuration/api/konnect/v1alpha1"
+)
+
+func TestKonnectAPIAuthConfiguration(t *testing.T) {
+	t.Parallel()
+	ctx, cancel := Context(t, context.Background())
+	defer cancel()
+	cfg, ns := Setup(t, ctx, scheme.Get())
+
+	t.Log("Setting up the manager with reconcilers")
+	mgr, logs := NewManager(t, ctx, cfg, scheme.Get())
+	factory := sdkmocks.NewMockSDKFactory(t)
+	sdk := factory.SDK
+	StartReconcilers(ctx, t, mgr, logs,
+		konnect.NewKonnectAPIAuthConfigurationReconciler(factory, false, mgr.GetClient()),
+	)
+
+	t.Log("Setting up clients")
+	cl, err := client.NewWithWatch(mgr.GetConfig(), client.Options{
+		Scheme: scheme.Get(),
+	})
+	require.NoError(t, err)
+	clientNamespaced := client.NewNamespacedClient(mgr.GetClient(), ns.Name)
+
+	t.Run("gets APIAuthValid=true status condition set on success", func(t *testing.T) {
+		call := sdk.MeSDK.EXPECT().
+			GetOrganizationsMe(mock.Anything, mock.Anything).
+			Return(
+				&sdkkonnectops.GetOrganizationsMeResponse{
+					MeOrganization: &sdkkonnectcomp.MeOrganization{
+						ID:   lo.ToPtr("12345"),
+						Name: lo.ToPtr("org-12345"),
+					},
+				},
+				nil,
+			)
+		t.Cleanup(func() { call.Unset() })
+
+		w := setupWatch[konnectv1alpha1.KonnectAPIAuthConfigurationList](t, ctx, cl, client.InNamespace(ns.Name))
+		apiAuth := deploy.KonnectAPIAuthConfiguration(t, ctx, clientNamespaced)
+		t.Cleanup(func() { assert.NoError(t, cl.Delete(ctx, apiAuth)) })
+
+		t.Log("Waiting for KonnectAPIAuthConfiguration to be APIAuthValid=true")
+		watchFor(t, ctx, w, watch.Modified, func(r *konnectv1alpha1.KonnectAPIAuthConfiguration) bool {
+			return client.ObjectKeyFromObject(r) == client.ObjectKeyFromObject(apiAuth) &&
+				r.Status.OrganizationID == "12345" &&
+				k8sutils.HasConditionTrue("APIAuthValid", r)
+		}, "KonnectAPIAuthConfiguration didn't get APIAuthValid status condition set to true or didn't get the Org ID set")
+	})
+
+	t.Run("gets APIAuthValid=false status condition set on invalid token", func(t *testing.T) {
+		call := sdk.MeSDK.EXPECT().
+			GetOrganizationsMe(mock.Anything, mock.Anything).
+			Return(
+				nil,
+				&sdkkonnecterrs.UnauthorizedError{
+					Status: 401,
+					Title:  "Unauthenticated",
+					Detail: "A valid token is required",
+				},
+			)
+		t.Cleanup(func() { call.Unset() })
+
+		w := setupWatch[konnectv1alpha1.KonnectAPIAuthConfigurationList](t, ctx, cl, client.InNamespace(ns.Name))
+		apiAuth := deploy.KonnectAPIAuthConfiguration(t, ctx, clientNamespaced)
+		t.Cleanup(func() { assert.NoError(t, cl.Delete(ctx, apiAuth)) })
+
+		t.Log("Waiting for KonnectAPIAuthConfiguration to be APIAuthValid=true")
+		watchFor(t, ctx, w, watch.Modified, func(r *konnectv1alpha1.KonnectAPIAuthConfiguration) bool {
+			return client.ObjectKeyFromObject(r) == client.ObjectKeyFromObject(apiAuth) &&
+				k8sutils.HasConditionFalse("APIAuthValid", r)
+		}, "KonnectAPIAuthConfiguration didn't get APIAuthValid status condition set to false")
+	})
+
+	t.Run("does not panic when response MeOrganization has no ID", func(t *testing.T) {
+		call := sdk.MeSDK.EXPECT().
+			GetOrganizationsMe(mock.Anything, mock.Anything).
+			Return(
+				&sdkkonnectops.GetOrganizationsMeResponse{
+					MeOrganization: &sdkkonnectcomp.MeOrganization{},
+				},
+				nil,
+			)
+		t.Cleanup(func() { call.Unset() })
+
+		w := setupWatch[konnectv1alpha1.KonnectAPIAuthConfigurationList](t, ctx, cl, client.InNamespace(ns.Name))
+		apiAuth := deploy.KonnectAPIAuthConfiguration(t, ctx, clientNamespaced)
+		t.Cleanup(func() { assert.NoError(t, cl.Delete(ctx, apiAuth)) })
+
+		t.Log("Waiting for KonnectAPIAuthConfiguration to be APIAuthValid=false")
+		watchFor(t, ctx, w, watch.Modified, func(r *konnectv1alpha1.KonnectAPIAuthConfiguration) bool {
+			return client.ObjectKeyFromObject(r) == client.ObjectKeyFromObject(apiAuth) &&
+				k8sutils.HasConditionFalse("APIAuthValid", r)
+		}, "KonnectAPIAuthConfiguration didn't get APIAuthValid status condition set to false")
+	})
+
+	t.Run("does not panic when response MeOrganization is nil", func(t *testing.T) {
+		call := sdk.MeSDK.EXPECT().
+			GetOrganizationsMe(mock.Anything, mock.Anything).
+			Return(
+				&sdkkonnectops.GetOrganizationsMeResponse{
+					MeOrganization: nil,
+				},
+				nil,
+			)
+		t.Cleanup(func() { call.Unset() })
+
+		w := setupWatch[konnectv1alpha1.KonnectAPIAuthConfigurationList](t, ctx, cl, client.InNamespace(ns.Name))
+		apiAuth := deploy.KonnectAPIAuthConfiguration(t, ctx, clientNamespaced)
+		t.Cleanup(func() { assert.NoError(t, cl.Delete(ctx, apiAuth)) })
+
+		t.Log("Waiting for KonnectAPIAuthConfiguration to be APIAuthValid=false")
+		watchFor(t, ctx, w, watch.Modified, func(r *konnectv1alpha1.KonnectAPIAuthConfiguration) bool {
+			return client.ObjectKeyFromObject(r) == client.ObjectKeyFromObject(apiAuth) &&
+				k8sutils.HasConditionFalse("APIAuthValid", r)
+		}, "KonnectAPIAuthConfiguration didn't get APIAuthValid status condition set to false")
+	})
+
+	t.Run("does not panic when response is nil", func(t *testing.T) {
+		call := sdk.MeSDK.EXPECT().
+			GetOrganizationsMe(mock.Anything, mock.Anything).
+			Return(
+				nil,
+				nil,
+			)
+		t.Cleanup(func() { call.Unset() })
+
+		w := setupWatch[konnectv1alpha1.KonnectAPIAuthConfigurationList](t, ctx, cl, client.InNamespace(ns.Name))
+		apiAuth := deploy.KonnectAPIAuthConfiguration(t, ctx, clientNamespaced)
+		t.Cleanup(func() { assert.NoError(t, cl.Delete(ctx, apiAuth)) })
+
+		t.Log("Waiting for KonnectAPIAuthConfiguration to be APIAuthValid=false")
+		watchFor(t, ctx, w, watch.Modified, func(r *konnectv1alpha1.KonnectAPIAuthConfiguration) bool {
+			return client.ObjectKeyFromObject(r) == client.ObjectKeyFromObject(apiAuth) &&
+				k8sutils.HasConditionFalse("APIAuthValid", r)
+		}, "KonnectAPIAuthConfiguration didn't get APIAuthValid status condition set to false")
+	})
+}