Skip to content

Commit

Permalink
fix(konnect): fix panic in KonnectAPIAuthConfigurationReconciler
Browse files Browse the repository at this point in the history
  • Loading branch information
pmalek committed Nov 27, 2024
1 parent d3a522b commit 6cd5da6
Show file tree
Hide file tree
Showing 5 changed files with 192 additions and 5 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@
- Fixed setting `ExternalTrafficPolicy` on `DataPlane`'s ingress `Service` when
the requested value is empty.
[#865](https://github.com/Kong/gateway-operator/pull/865)
- Fixed a `panic` in `KonnectAPIAuthConfigurationReconciler` occuring when nil
response was returned by Konnect API when fetching the organization information.
[#890](https://github.com/Kong/gateway-operator/pull/890)

## [v1.4.0]

Expand Down
15 changes: 13 additions & 2 deletions controller/konnect/reconciler_konnectapiauth.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 ||
Expand All @@ -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)
Expand Down
15 changes: 12 additions & 3 deletions pkg/utils/kubernetes/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,16 +65,25 @@ 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 {
func isCondition(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
}

// IsConditionFalse returns true if the condition has Status set to ConditionFalse, false otherwise.
func IsConditionFalse(cType consts.ConditionType, resource ConditionsAware) bool {
return isCondition(cType, resource, metav1.ConditionFalse)
}

// IsConditionTrue returns true if the condition has Status set to ConditionTrue, false otherwise.
func IsConditionTrue(cType consts.ConditionType, resource ConditionsAware) bool {
return isCondition(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 {
Expand Down
4 changes: 4 additions & 0 deletions test/envtest/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
160 changes: 160 additions & 0 deletions test/envtest/konnect_entities_konnectapiauthconfiguration_test.go
Original file line number Diff line number Diff line change
@@ -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.IsConditionTrue("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.IsConditionFalse("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.IsConditionFalse("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.IsConditionFalse("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.IsConditionFalse("APIAuthValid", r)
}, "KonnectAPIAuthConfiguration didn't get APIAuthValid status condition set to false")
})
}

0 comments on commit 6cd5da6

Please sign in to comment.