Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix refChecker when namespace is specified and is the same as route's namespace #5392

Merged
merged 5 commits into from
Jan 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,6 @@ Adding a new version? You'll need three changes:
[#5354](https://github.com/Kong/kubernetes-ingress-controller/pull/5354)
[#5384](https://github.com/Kong/kubernetes-ingress-controller/pull/5384)


### Fixed

- Validators of `KongPlugin` and `KongClusterPlugin` will not return `500` on
Expand All @@ -161,6 +160,9 @@ Adding a new version? You'll need three changes:
[#5401](https://github.com/Kong/kubernetes-ingress-controller/pull/5401)
- Support properly ConsumerGroups when fallback to the last valid configuration.
[#5438](https://github.com/Kong/kubernetes-ingress-controller/pull/5438)
- When specifying Gateway API Routes' `backendRef`s with namespace specified, those
refs are checked for existence and allowed if they exist.
[#5392](https://github.com/Kong/kubernetes-ingress-controller/pull/5392)

### Changed

Expand Down
94 changes: 64 additions & 30 deletions internal/dataplane/translator/backendref.go
Original file line number Diff line number Diff line change
@@ -1,19 +1,30 @@
package translator

import (
"errors"
"fmt"

"github.com/go-logr/logr"
k8stypes "k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/kongstate"
"github.com/kong/kubernetes-ingress-controller/v3/internal/gatewayapi"
"github.com/kong/kubernetes-ingress-controller/v3/internal/store"
"github.com/kong/kubernetes-ingress-controller/v3/internal/util"
)

// backendRefsToKongStateBackends takes a list of BackendRefs and returns a list of ServiceBackends.
// The backendRefs are checked for the following conditions. If any of these conditions are met, the BackendRef is
// not included in the returned list:
// - If a BackendRef is not permitted by the provided ReferenceGrantTo set,
// - If a BackendRef is not found,
// - If a BackendRef Group & Kind pair is not supported (currently only Service is supported),
// - If a BackendRef is missing a port.
// The provided client is used to retrieve the Backend referenced by the BackendRef
// to check if it exists.
func backendRefsToKongStateBackends(
logger logr.Logger,
storer store.Storer,
route client.Object,
backendRefs []gatewayapi.BackendRef,
allowed map[gatewayapi.Namespace][]gatewayapi.ReferenceGrantTo,
Expand All @@ -23,42 +34,65 @@ func backendRefsToKongStateBackends(
for _, backendRef := range backendRefs {
logger := loggerForBackendRef(logger, route, backendRef)

if util.IsBackendRefGroupKindSupported(
backendRef.Group,
backendRef.Kind,
) && newRefChecker(backendRef).IsRefAllowedByGrant(allowed) {
port := int32(-1)
if backendRef.Port != nil {
port = int32(*backendRef.Port)
}
namespace := route.GetNamespace()
if backendRef.Namespace != nil {
namespace = string(*backendRef.Namespace)
}
backend, err := kongstate.NewServiceBackendForService(
k8stypes.NamespacedName{
Namespace: namespace,
Name: string(backendRef.Name),
},
kongstate.PortDef{
Mode: kongstate.PortModeByNumber,
Number: port,
},
)
if err != nil {
logger.Error(err, "failed to create ServiceBackend for backendRef")
}
if backendRef.Weight != nil {
backend.SetWeight(*backendRef.Weight)
nn := client.ObjectKey{
Name: string(backendRef.Name),
Namespace: route.GetNamespace(),
}
if backendRef.Namespace != nil {
nn.Namespace = string(*backendRef.Namespace)
}

if backendRef.Kind == nil {
// This should never happen as the default value defined by Gateway API is 'Service'. Checking just in case.
logger.Error(nil, "Object requested backendRef to target, but no Kind was specified, skipping...")
pmalek marked this conversation as resolved.
Show resolved Hide resolved
continue
}

var err error
switch *backendRef.Kind {
case "Service":
_, err = storer.GetService(nn.Namespace, nn.Name)
default:
err = fmt.Errorf("unsupported kind %q, only 'Service' is supported", *backendRef.Kind)
}
if err != nil {
if errors.As(err, &store.NotFoundError{}) {
logger.Error(err, "Object requested backendRef to target, but it does not exist, skipping...")
} else {
logger.Error(err, "Object requested backendRef to target, but an error occurred, skipping...")
}
backends = append(backends, backend)
} else {
continue
}

if !util.IsBackendRefGroupKindSupported(backendRef.Group, backendRef.Kind) ||
!newRefCheckerForRoute(route, backendRef).IsRefAllowedByGrant(allowed) {
// we log impermissible refs rather than failing the entire rule. while we cannot actually route to
// these, we do not want a single impermissible ref to take the entire rule offline. in the case of edits,
// failing the entire rule could potentially delete routes that were previously online and in use, and
// that remain viable (because they still have some permissible backendRefs)
logger.Error(nil, "Object requested backendRef to target, but no ReferenceGrant permits it, skipping...")
continue
}

port := int32(-1)
if backendRef.Port != nil {
port = int32(*backendRef.Port)
}
backend, err := kongstate.NewServiceBackendForService(
nn,
kongstate.PortDef{
Mode: kongstate.PortModeByNumber,
Number: port,
},
)
if err != nil {
logger.Error(err, "failed to create ServiceBackend for backendRef")
continue
}
if backendRef.Weight != nil {
backend.SetWeight(*backendRef.Weight)
}
backends = append(backends, backend)
}

return backends
Expand Down
167 changes: 167 additions & 0 deletions internal/dataplane/translator/backendref_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
package translator

import (
"testing"

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

"github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/kongstate"
"github.com/kong/kubernetes-ingress-controller/v3/internal/gatewayapi"
"github.com/kong/kubernetes-ingress-controller/v3/internal/store"
"github.com/kong/kubernetes-ingress-controller/v3/internal/util/builder"
)

func TestBackendRefsToKongStateBackends(t *testing.T) {
testcases := []struct {
name string
route client.Object
backendRefs []gatewayapi.BackendRef
allowed map[gatewayapi.Namespace][]gatewayapi.ReferenceGrantTo
objects store.FakeObjects
expected kongstate.ServiceBackends
}{
{
name: "correct ReferenceGrant and an existing Service as backendRef returns a KongStateBackend with a Service",
route: &gatewayapi.HTTPRoute{
ObjectMeta: metav1.ObjectMeta{
Name: "basic-httproute",
Namespace: corev1.NamespaceDefault,
},
Spec: gatewayapi.HTTPRouteSpec{
CommonRouteSpec: commonRouteSpecMock("fake-gateway-1"),
Hostnames: []gatewayapi.Hostname{
"konghq.com",
"www.konghq.com",
},
Rules: []gatewayapi.HTTPRouteRule{{
// BackendRefs are taken from the backendRefs argument.
// This is done this way because the backendRefs are
// extracted from the route's spec before.
// Potentially this could be refactored to be extracted
// in backendRefsToKongStateBackends using a type switch.
}},
},
},
backendRefs: []gatewayapi.BackendRef{
builder.NewBackendRef("fake-service").WithPort(80).Build(),
},
allowed: map[gatewayapi.Namespace][]gatewayapi.ReferenceGrantTo{
gatewayapi.Namespace(corev1.NamespaceDefault): {
{
Group: "",
Kind: "Service",
Name: lo.ToPtr(gatewayapi.ObjectName("fake-service")),
},
},
},
objects: store.FakeObjects{
Services: []*corev1.Service{
{
ObjectMeta: metav1.ObjectMeta{
Name: "fake-service",
Namespace: corev1.NamespaceDefault,
},
Spec: corev1.ServiceSpec{
Ports: []corev1.ServicePort{
builder.NewServicePort().WithPort(80).Build(),
},
},
},
},
},
expected: func() kongstate.ServiceBackends {
svcBackend, err := kongstate.NewServiceBackend(
kongstate.ServiceBackendTypeKubernetesService,
k8stypes.NamespacedName{Namespace: corev1.NamespaceDefault, Name: "fake-service"},
kongstate.PortDef{
Mode: kongstate.PortModeByNumber,
Number: 80,
},
)
require.NoError(t, err)
return kongstate.ServiceBackends{svcBackend}
}(),
},
{
name: "no ReferenceGrant and an existing Service as backendRef doesn't return a KongStateBackend with the Service",
route: &gatewayapi.HTTPRoute{
ObjectMeta: metav1.ObjectMeta{
Name: "basic-httproute",
Namespace: corev1.NamespaceDefault,
},
Spec: gatewayapi.HTTPRouteSpec{
CommonRouteSpec: commonRouteSpecMock("fake-gateway-1"),
Hostnames: []gatewayapi.Hostname{
"konghq.com",
"www.konghq.com",
},
},
},
allowed: map[gatewayapi.Namespace][]gatewayapi.ReferenceGrantTo{
gatewayapi.Namespace(corev1.NamespaceDefault): {
{
Group: "",
Kind: "ImaginaryKind",
Name: lo.ToPtr(gatewayapi.ObjectName("fake-service")),
},
},
},
objects: store.FakeObjects{
Services: []*corev1.Service{
{
ObjectMeta: metav1.ObjectMeta{
Name: "fake-service",
Namespace: corev1.NamespaceDefault,
},
},
},
},
expected: kongstate.ServiceBackends{},
},
{
name: "ReferenceGrant and a non existing Service as backendRef doesn't return a KongStateBackend with the Service",
route: &gatewayapi.HTTPRoute{
ObjectMeta: metav1.ObjectMeta{
Name: "basic-httproute",
Namespace: corev1.NamespaceDefault,
},
Spec: gatewayapi.HTTPRouteSpec{
CommonRouteSpec: commonRouteSpecMock("fake-gateway-1"),
Hostnames: []gatewayapi.Hostname{
"konghq.com",
"www.konghq.com",
},
},
},
backendRefs: []gatewayapi.BackendRef{
builder.NewBackendRef("fake-service").WithPort(80).Build(),
},
allowed: map[gatewayapi.Namespace][]gatewayapi.ReferenceGrantTo{
gatewayapi.Namespace(corev1.NamespaceDefault): {
{
Group: "",
Kind: "Service",
Name: lo.ToPtr(gatewayapi.ObjectName("fake-service")),
},
},
},
expected: kongstate.ServiceBackends{},
},
}

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
fakestore, err := store.NewFakeStore(tc.objects)
require.NoError(t, err)
logger := logr.Discard()
ret := backendRefsToKongStateBackends(logger, fakestore, tc.route, tc.backendRefs, tc.allowed)
require.Equal(t, tc.expected, ret)
})
}
}
Loading
Loading