Skip to content

Commit

Permalink
fix(plugin): fix plugin reference check (#6295)
Browse files Browse the repository at this point in the history 
Fix the plugin reference check. This was originally providing reference
information backwards of how the reference check functions expected it.

Add util function TypeMetaFromGVK.

Add unit tests for plugin reference checks.
  • Loading branch information
@randmonkey
randmonkey authored Jul 10, 2024
1 parent 0a3aa4c commit ac94a4c
Show file tree
Hide file tree
Showing 5 changed files with 208 additions and 20 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ Adding a new version? You'll need three changes:
- Generate one entity for each attached foreign entity if a `KongCustomEntity`
resource is attached to multiple foreign Kong entities.
[#6280](https://github.com/Kong/kubernetes-ingress-controller/pull/6280)
- Fixed the reference checker in checking permission of remote plugins to use
the correct namespace of `ReferenceGrant` required.
[#6295](https://github.com/Kong/kubernetes-ingress-controller/pull/6295)

## 3.2.2

Expand Down
40 changes: 20 additions & 20 deletions internal/dataplane/kongstate/kongstate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"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"
kongv1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1"
kongv1alpha1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1alpha1"
kongv1beta1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1beta1"
)
Expand Down Expand Up @@ -398,8 +397,11 @@ func (ks *KongState) getPluginRelations(cacheStore store.Storer, log logr.Logger
ingress := ks.Services[i].Routes[j].Ingress
pluginList := annotations.ExtractNamespacedKongPluginsFromAnnotations(ingress.Annotations)
for _, plugin := range pluginList {
// pretend we have a full Ingress struct for reference checks
// pretend we have a full Ingress struct for reference checks.
// REVIEW: we only need an object to carry type meta and object meta here, maybe we should create some other types of virtual object here?
virtualIngress := netv1.Ingress{
// Fill the actual type of the object for reference checks.
TypeMeta: util.TypeMetaFromGVK(ingress.GroupVersionKind),
ObjectMeta: metav1.ObjectMeta{
Namespace: ingress.Namespace,
Name: ingress.Name,
Expand Down Expand Up @@ -428,7 +430,7 @@ func (ks *KongState) getPluginRelations(cacheStore store.Storer, log logr.Logger
}

type pluginReference struct {
Referer client.Object
Referrer client.Object
Namespace string
Name string
}
Expand All @@ -440,26 +442,22 @@ func isRemotePluginReferenceAllowed(s store.Storer, r pluginReference) error {
return fmt.Errorf("could not retrieve ReferenceGrants from store when building plugin relations map: %w", err)
}
allowed := gatewayapi.GetPermittedForReferenceGrantFrom(gatewayapi.ReferenceGrantFrom{
Group: gatewayapi.Group(r.Referer.GetObjectKind().GroupVersionKind().Group),
Kind: gatewayapi.Kind(r.Referer.GetObjectKind().GroupVersionKind().Kind),
Namespace: gatewayapi.Namespace(r.Referer.GetNamespace()),
Group: gatewayapi.Group(r.Referrer.GetObjectKind().GroupVersionKind().Group),
Kind: gatewayapi.Kind(r.Referrer.GetObjectKind().GroupVersionKind().Kind),
Namespace: gatewayapi.Namespace(r.Referrer.GetNamespace()),
}, grants)

// we don't have a full plugin resource here for the grant checker, so we build a fake one with the correct
// name and namespace
virtualReference := gatewayapi.PluginLabelReference{
Namespace: lo.ToPtr(r.Referer.GetNamespace()),
// name and namespace.
pluginReference := gatewayapi.PluginLabelReference{
Namespace: &r.Namespace,
Name: r.Name,
}
virtualPlugin := &kongv1.KongPlugin{
ObjectMeta: metav1.ObjectMeta{
Namespace: r.Namespace,
Name: r.Name,
},
}
if !gatewayapi.NewRefCheckerForKongPlugin(virtualPlugin, virtualReference).IsRefAllowedByGrant(allowed) {
// Because we should check whether it is allowed to refer FROM the referrer TO the plugin here,
// we should put the referrer on the "target" and the plugin on the "backendRef".
if !gatewayapi.NewRefCheckerForKongPlugin(r.Referrer, pluginReference).IsRefAllowedByGrant(allowed) {
return fmt.Errorf("no grant found for %s in %s to plugin %s in %s requested remote KongPlugin bind",
r.Referer.GetObjectKind().GroupVersionKind().Kind, r.Referer.GetNamespace(), r.Name, r.Namespace)
r.Referrer.GetObjectKind().GroupVersionKind().Kind, r.Referrer.GetNamespace(), r.Name, r.Namespace)
}
return nil
}
Expand Down Expand Up @@ -759,6 +757,8 @@ func (ks *KongState) getPluginRelatedEntitiesRef(cacheStore store.Storer, log lo
for _, plugin := range pluginList {
// Pretend we have a full Ingress struct for reference checks.
virtualIngress := netv1.Ingress{
// Fill the actual type of the object for reference checks.
TypeMeta: util.TypeMetaFromGVK(ingress.GroupVersionKind),
ObjectMeta: metav1.ObjectMeta{
Namespace: ingress.Namespace,
Name: ingress.Name,
Expand All @@ -783,7 +783,7 @@ func (ks *KongState) getPluginRelatedEntitiesRef(cacheStore store.Storer, log lo
}

func extractReferredPluginNamespace(
cacheStore store.Storer, referer client.Object, plugin annotations.NamespacedKongPlugin,
cacheStore store.Storer, referrer client.Object, plugin annotations.NamespacedKongPlugin,
) (string, error) {
// There are 2 types of KongPlugin references: local and remote.
// A local reference is one where the KongPlugin is in the same namespace as the referer.
Expand All @@ -793,14 +793,14 @@ func extractReferredPluginNamespace(
//
// The referer is the entity that the KongPlugin is associated with.
if plugin.Namespace == "" {
return referer.GetNamespace(), nil
return referrer.GetNamespace(), nil
}

// remote KongPlugin, permitted if ReferenceGrant allows.
err := isRemotePluginReferenceAllowed(
cacheStore,
pluginReference{
Referer: referer,
Referrer: referrer,
Namespace: plugin.Namespace,
Name: plugin.Name,
},
Expand Down
119 changes: 119 additions & 0 deletions internal/dataplane/kongstate/kongstate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (

"github.com/kong/kubernetes-ingress-controller/v3/internal/annotations"
"github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/failures"
"github.com/kong/kubernetes-ingress-controller/v3/internal/gatewayapi"
"github.com/kong/kubernetes-ingress-controller/v3/internal/labels"
"github.com/kong/kubernetes-ingress-controller/v3/internal/store"
"github.com/kong/kubernetes-ingress-controller/v3/internal/util"
Expand Down Expand Up @@ -1526,3 +1527,121 @@ func (s *fakeSchemaGetter) Get(_ context.Context, entityType string) (kong.Schem
}
return schema, nil
}

func TestIsRemotePluginReferenceAllowed(t *testing.T) {
serviceTypeMeta := metav1.TypeMeta{
Kind: "Service",
}

testCases := []struct {
name string
referrer client.Object
pluginNamespace string
pluginName string
referenceGrants []*gatewayapi.ReferenceGrant
shouldAllow bool
}{
{
name: "no reference grant",
referrer: &corev1.Service{
TypeMeta: serviceTypeMeta,
ObjectMeta: metav1.ObjectMeta{
Namespace: "foo",
Name: "service-foo",
},
},
pluginNamespace: "bar",
pluginName: "plugin-bar",
shouldAllow: false,
},
{
name: "have reference grant",
referrer: &corev1.Service{
TypeMeta: serviceTypeMeta,
ObjectMeta: metav1.ObjectMeta{
Namespace: "foo",
Name: "service-foo",
},
},
pluginNamespace: "bar",
pluginName: "plugin-bar",
referenceGrants: []*gatewayapi.ReferenceGrant{
{
ObjectMeta: metav1.ObjectMeta{
Namespace: "bar", // same namespace as plugin
Name: "grant-1",
},
Spec: gatewayapi.ReferenceGrantSpec{
From: []gatewayapi.ReferenceGrantFrom{
{
Kind: gatewayapi.Kind("Service"),
Namespace: "foo",
},
},
To: []gatewayapi.ReferenceGrantTo{
{
Group: gatewayapi.Group(kongv1.GroupVersion.Group),
Kind: gatewayapi.Kind("KongPlugin"),
},
},
},
},
},
shouldAllow: true,
},
{
name: "reference grant created but in different namespace",
referrer: &corev1.Service{
TypeMeta: serviceTypeMeta,
ObjectMeta: metav1.ObjectMeta{
Namespace: "foo",
Name: "service-foo",
},
},
pluginNamespace: "bar",
pluginName: "plugin-bar",
referenceGrants: []*gatewayapi.ReferenceGrant{
{
ObjectMeta: metav1.ObjectMeta{
Namespace: "foo", // Not same namespace as plugin
Name: "grant-1",
},
Spec: gatewayapi.ReferenceGrantSpec{
From: []gatewayapi.ReferenceGrantFrom{
{
Kind: gatewayapi.Kind("Service"),
Namespace: "foo",
},
},
To: []gatewayapi.ReferenceGrantTo{
{
Group: gatewayapi.Group(kongv1.GroupVersion.Group),
Kind: gatewayapi.Kind("KongPlugin"),
},
},
},
},
},
shouldAllow: false,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
s, err := store.NewFakeStore(store.FakeObjects{
ReferenceGrants: tc.referenceGrants,
})
require.NoError(t, err)
err = isRemotePluginReferenceAllowed(s, pluginReference{
Referrer: tc.referrer,
Namespace: tc.pluginNamespace,
Name: tc.pluginName,
})
if tc.shouldAllow {
require.NoError(t, err)
} else {
require.Error(t, err)
}
})
}
}
14 changes: 14 additions & 0 deletions internal/util/objectmeta.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package util

import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"sigs.k8s.io/controller-runtime/pkg/client"
)
Expand All @@ -24,3 +25,16 @@ func FromK8sObject(obj client.Object) K8sObjectInfo {
GroupVersionKind: obj.GetObjectKind().GroupVersionKind(),
}
}

// TypeMetaFromGVK returns typemeta from groupversionkind of a k8s object.
func TypeMetaFromGVK(gvk schema.GroupVersionKind) metav1.TypeMeta {
return metav1.TypeMeta{
APIVersion: gvk.GroupVersion().String(),
Kind: gvk.Kind,
}
}

// TypeMetaFromK8sObject returns the typemeta of a client Object by its GVK.
func TypeMetaFromK8sObject(obj client.Object) metav1.TypeMeta {
return TypeMetaFromGVK(obj.GetObjectKind().GroupVersionKind())
}
52 changes: 52 additions & 0 deletions internal/util/objectmeta_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"testing"

"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"
"k8s.io/apimachinery/pkg/runtime/schema"
Expand Down Expand Up @@ -94,3 +96,53 @@ func BenchmarkFromK8sObject(b *testing.B) {
_ = out
}
}

func TestTypeMetaFromK8sObject(t *testing.T) {
testCases := []struct {
name string
obj client.Object
typeMeta metav1.TypeMeta
}{
{
name: "empty group",
obj: &corev1.Service{
TypeMeta: metav1.TypeMeta{
APIVersion: "v1",
Kind: "Service",
},
ObjectMeta: metav1.ObjectMeta{
Name: "svc",
Namespace: "default",
},
},
typeMeta: metav1.TypeMeta{
APIVersion: "v1",
Kind: "Service",
},
},
{
name: "non-empty group",
obj: &netv1.Ingress{
TypeMeta: metav1.TypeMeta{
APIVersion: "networking.k8s.io/v1",
Kind: "Ingress",
},
ObjectMeta: metav1.ObjectMeta{
Name: "ing",
Namespace: "default",
},
},
typeMeta: metav1.TypeMeta{
APIVersion: "networking.k8s.io/v1",
Kind: "Ingress",
},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
typeMeta := TypeMetaFromK8sObject(tc.obj)
require.Equal(t, tc.typeMeta, typeMeta)
})
}
}

0 comments on commit ac94a4c

Please sign in to comment.