diff --git a/controller/gateway/controller.go b/controller/gateway/controller.go index 48d477d82..52219654d 100644 --- a/controller/gateway/controller.go +++ b/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 } diff --git a/controller/konnect/reconciler_konnectapiauth.go b/controller/konnect/reconciler_konnectapiauth.go index 389242113..b716086f6 100644 --- a/controller/konnect/reconciler_konnectapiauth.go +++ b/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) diff --git a/pkg/utils/kubernetes/status.go b/pkg/utils/kubernetes/status.go index 5700e84e9..b1bb952ad 100644 --- a/pkg/utils/kubernetes/status.go +++ b/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 { diff --git a/pkg/utils/kubernetes/status_test.go b/pkg/utils/kubernetes/status_test.go index 7ed39b4dc..22369eaa5 100644 --- a/pkg/utils/kubernetes/status_test.go +++ b/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) }) } diff --git a/test/envtest/controller.go b/test/envtest/controller.go index d67a98154..9254cc156 100644 --- a/test/envtest/controller.go +++ b/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. diff --git a/test/envtest/konnect_entities_konnectapiauthconfiguration_test.go b/test/envtest/konnect_entities_konnectapiauthconfiguration_test.go new file mode 100644 index 000000000..68ad5ff32 --- /dev/null +++ b/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") + }) +}