Skip to content

Commit

Permalink
fix(konnect): fix panic in KonnectAPIAuthConfigurationReconciler (#890)
Browse files Browse the repository at this point in the history
* fix(konnect): fix panic in KonnectAPIAuthConfigurationReconciler

* chore: refactor condition funcs
  • Loading branch information
pmalek committed Nov 27, 2024
1 parent a3240de commit a6e3031
Show file tree
Hide file tree
Showing 6 changed files with 194 additions and 8 deletions.
4 changes: 2 additions & 2 deletions controller/gateway/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
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
17 changes: 14 additions & 3 deletions pkg/utils/kubernetes/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion pkg/utils/kubernetes/status_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
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.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")
})
}

0 comments on commit a6e3031

Please sign in to comment.