Skip to content

Commit

Permalink
fix: fix panic in KongUpstreamPolicyReconciler when using with Ingres…
Browse files Browse the repository at this point in the history 
…s having a nil HTTP rule (#6651)
  • Loading branch information
@pmalek
pmalek authored Nov 11, 2024
1 parent a2887b2 commit d593a67
Show file tree
Hide file tree
Showing 3 changed files with 254 additions and 7 deletions.
4 changes: 3 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,9 @@ Adding a new version? You'll need three changes:
- Fixed annotation `konghq.com/rewrite` that was not being applied sometimes
when `Ingress` without annotation and a different `Ingress` with annotation
pointed to the same `Service`.
[#6569](https://github.com/Kong/kubernetes-ingress-controller/pull/6626)
[#6626](https://github.com/Kong/kubernetes-ingress-controller/pull/6626)
- Fix panic in `KongUpstreamPolicyReconciler` when using with Ingress having a nil HTTP rule
[#6651](https://github.com/Kong/kubernetes-ingress-controller/pull/6651)

## [3.3.1]

Expand Down
19 changes: 13 additions & 6 deletions internal/controllers/configuration/kongupstreampolicy_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -324,18 +324,25 @@ func (r *KongUpstreamPolicyReconciler) getUpstreamPoliciesForIngressServices(ctx
}
var requests []reconcile.Request
for _, rule := range ingress.Spec.Rules {
if rule.HTTP == nil {
continue
}

for _, path := range rule.HTTP.Paths {
if path.Backend.Service == nil {
continue
}
service := &corev1.Service{}
if err := r.Client.Get(ctx, k8stypes.NamespacedName{
Namespace: ingress.Namespace,
Name: path.Backend.Service.Name,
}, service); err != nil {
var (
nn = k8stypes.NamespacedName{
Namespace: ingress.Namespace,
Name: path.Backend.Service.Name,
}
service corev1.Service
)
if err := r.Client.Get(ctx, nn, &service); err != nil {
if !apierrors.IsNotFound(err) {
r.Log.Error(err, "Failed to retrieve Service in watch predicates",
"Service", fmt.Sprintf("%s/%s", ingress.Namespace, path.Backend.Service.Name),
"Service", nn,
)
}
continue
Expand Down
238 changes: 238 additions & 0 deletions internal/controllers/configuration/kongupstreampolicy_controller_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,16 +1,23 @@
package configuration

import (
"context"
"testing"

"github.com/go-logr/logr"
"github.com/samber/lo"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
corev1 "k8s.io/api/core/v1"
netv1 "k8s.io/api/networking/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
k8stypes "k8s.io/apimachinery/pkg/types"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"

"github.com/kong/kubernetes-ingress-controller/v3/internal/gatewayapi"
kongv1beta1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1beta1"
)

func TestIndexIngressesOnBackendServiceName(t *testing.T) {
Expand Down Expand Up @@ -182,3 +189,234 @@ func TestIndexRoutesOnBackendRefServiceFacadeName(t *testing.T) {
})
}
}

func TestGetUpstreamPoliciesForIngressServices(t *testing.T) {
tests := []struct {
name string
services []client.Object
ingress netv1.Ingress
expectedRequests []ctrl.Request
}{
{
name: "Multiple services with annotations",
services: []client.Object{
&corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "service1",
Namespace: "default",
Annotations: map[string]string{
kongv1beta1.KongUpstreamPolicyAnnotationKey: "policy1",
},
},
},
&corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "service2",
Namespace: "default",
Annotations: map[string]string{
kongv1beta1.KongUpstreamPolicyAnnotationKey: "policy2",
},
},
},
&corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "service3",
Namespace: "default",
},
},
},
ingress: netv1.Ingress{
ObjectMeta: metav1.ObjectMeta{
Name: "test-ingress",
Namespace: "default",
},
Spec: netv1.IngressSpec{
Rules: []netv1.IngressRule{
{
IngressRuleValue: netv1.IngressRuleValue{
HTTP: &netv1.HTTPIngressRuleValue{
Paths: []netv1.HTTPIngressPath{
{
Backend: netv1.IngressBackend{
Service: &netv1.IngressServiceBackend{
Name: "service1",
},
},
},
{
Backend: netv1.IngressBackend{
Service: &netv1.IngressServiceBackend{
Name: "service2",
},
},
},
{
Backend: netv1.IngressBackend{
Service: &netv1.IngressServiceBackend{
Name: "service3",
},
},
},
},
},
},
},
},
},
},
expectedRequests: []ctrl.Request{
{
NamespacedName: k8stypes.NamespacedName{
Namespace: "default", Name: "policy1",
},
},
{
NamespacedName: k8stypes.NamespacedName{
Namespace: "default", Name: "policy2",
},
},
},
},
{
name: "No services with annotations",
services: []client.Object{
&corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "service1",
Namespace: "default",
},
},
&corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "service2",
Namespace: "default",
},
},
},
ingress: netv1.Ingress{
ObjectMeta: metav1.ObjectMeta{
Name: "test-ingress",
Namespace: "default",
},
Spec: netv1.IngressSpec{
Rules: []netv1.IngressRule{
{
IngressRuleValue: netv1.IngressRuleValue{
HTTP: &netv1.HTTPIngressRuleValue{
Paths: []netv1.HTTPIngressPath{
{
Backend: netv1.IngressBackend{
Service: &netv1.IngressServiceBackend{
Name: "service1",
},
},
},
{
Backend: netv1.IngressBackend{
Service: &netv1.IngressServiceBackend{
Name: "service2",
},
},
},
},
},
},
},
},
},
},
expectedRequests: []ctrl.Request{},
},
{
name: "Service in different namespace cannot be used",
services: []client.Object{
&corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "service1",
Namespace: "other",
Annotations: map[string]string{
kongv1beta1.KongUpstreamPolicyAnnotationKey: "policy1",
},
},
},
},
ingress: netv1.Ingress{
ObjectMeta: metav1.ObjectMeta{
Name: "test-ingress",
Namespace: "default",
},
Spec: netv1.IngressSpec{
Rules: []netv1.IngressRule{
{
IngressRuleValue: netv1.IngressRuleValue{
HTTP: &netv1.HTTPIngressRuleValue{
Paths: []netv1.HTTPIngressPath{
{
Backend: netv1.IngressBackend{
Service: &netv1.IngressServiceBackend{
Name: "service1",
},
},
},
},
},
},
},
},
},
},
expectedRequests: []ctrl.Request{},
},
{
name: "Ingress without HTTP rules does not panic",
services: []client.Object{
&corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "service1",
Namespace: "default",
Annotations: map[string]string{
kongv1beta1.KongUpstreamPolicyAnnotationKey: "policy1",
},
},
},
},
ingress: netv1.Ingress{
ObjectMeta: metav1.ObjectMeta{
Name: "test-ingress",
Namespace: "default",
},
Spec: netv1.IngressSpec{
Rules: []netv1.IngressRule{
{
IngressRuleValue: netv1.IngressRuleValue{
HTTP: nil,
},
},
},
},
},
expectedRequests: []ctrl.Request{},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// Create a fake client
fakeClient := fake.NewClientBuilder().
WithObjects(tt.services...).
Build()

// Create a KongUpstreamPolicyReconciler
reconciler := &KongUpstreamPolicyReconciler{
Client: fakeClient,
Log: logr.Discard(),
}

// Call the function
requests := reconciler.getUpstreamPoliciesForIngressServices(context.Background(), &tt.ingress)

// Assert the results
assert.ElementsMatch(t, tt.expectedRequests, requests)
})
}
}

0 comments on commit d593a67

Please sign in to comment.