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.

Sign up for GitHub

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: do not set instance_name of plugin if Kong version is below 3.2.0 #5250

Merged
merged 6 commits into from
Nov 29, 2023
Merged

fix: do not set instance_name of plugin if Kong version is below 3.2.0 #5250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8939832
add version cutoff of instance_name field in Kong plugin object
randmonkey Nov 29, 2023
7da31ef
add KongVersion to KongState and disable instance_name for Kong < 3.2.0
randmonkey Nov 29, 2023
2c6e26a
set KongVersion in fetch last good config
randmonkey Nov 29, 2023
83fc3ac
fix get Kong version in config fetcher and fix unit tests
randmonkey Nov 29, 2023
c46d93e
do not add KongVersion in KongState struct
randmonkey Nov 29, 2023
d1b83f1
do not add KongVersion field in intermediate struct kongstate.plugin
randmonkey Nov 29, 2023
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
14 changes: 8 additions & 6 deletions internal/dataplane/kongstate/kongstate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,7 @@ func (ks *KongState) getPluginRelations() map[string]util.ForeignRelations {
func buildPlugins(
log logrus.FieldLogger,
s store.Storer,
kongVersion semver.Version,
failuresCollector *failures.ResourceFailuresCollector,
pluginRels map[string]util.ForeignRelations,
) []Plugin {
Expand All @@ -337,14 +338,14 @@ func buildPlugins(

var plugin Plugin
if k8sPlugin != nil {
plugin, err = kongPluginFromK8SPlugin(s, *k8sPlugin)
plugin, err = kongPluginFromK8SPlugin(s, *k8sPlugin, kongVersion)
if err != nil {
failuresCollector.PushResourceFailure(err.Error(), k8sPlugin)
continue
}
}
if k8sClusterPlugin != nil {
plugin, err = kongPluginFromK8SClusterPlugin(s, *k8sClusterPlugin)
plugin, err = kongPluginFromK8SClusterPlugin(s, *k8sClusterPlugin, kongVersion)
if err != nil {
failuresCollector.PushResourceFailure(err.Error(), k8sClusterPlugin)
continue
Expand Down Expand Up @@ -390,7 +391,7 @@ func buildPlugins(
}
}

globalPlugins, err := globalPlugins(log, s)
globalPlugins, err := globalPlugins(log, s, kongVersion)
if err != nil {
log.WithError(err).Error("failed to fetch global plugins")
}
Expand All @@ -400,7 +401,7 @@ func buildPlugins(
return plugins
}

func globalPlugins(log logrus.FieldLogger, s store.Storer) ([]Plugin, error) {
func globalPlugins(log logrus.FieldLogger, s store.Storer, kongVersion semver.Version) ([]Plugin, error) {
// removed as of 0.10.0
// only retrieved now to warn users
globalPlugins, err := s.ListGlobalKongPlugins()
Expand Down Expand Up @@ -441,7 +442,7 @@ func globalPlugins(log logrus.FieldLogger, s store.Storer) ([]Plugin, error) {
duplicates = append(duplicates, pluginName)
continue
}
if plugin, err := kongPluginFromK8SClusterPlugin(s, k8sPlugin); err == nil {
if plugin, err := kongPluginFromK8SClusterPlugin(s, k8sPlugin, kongVersion); err == nil {
res[pluginName] = plugin
} else {
log.WithFields(logrus.Fields{
Expand All @@ -462,9 +463,10 @@ func globalPlugins(log logrus.FieldLogger, s store.Storer) ([]Plugin, error) {
func (ks *KongState) FillPlugins(
log logrus.FieldLogger,
s store.Storer,
kongVersion semver.Version,
failuresCollector *failures.ResourceFailuresCollector,
) {
ks.Plugins = buildPlugins(log, s, failuresCollector, ks.getPluginRelations())
ks.Plugins = buildPlugins(log, s, kongVersion, failuresCollector, ks.getPluginRelations())
}

// FillIDs iterates over the KongState and fills in the ID field for each entity
Expand Down
12 changes: 7 additions & 5 deletions internal/dataplane/kongstate/kongstate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -786,10 +786,11 @@ func TestKongState_FillIDs(t *testing.T) {

func TestKongState_BuildPluginsCollisions(t *testing.T) {
for _, tt := range []struct {
name string
in []*kongv1.KongPlugin
pluginRels map[string]util.ForeignRelations
want []string
name string
in []*kongv1.KongPlugin
kongVersion semver.Version
pluginRels map[string]util.ForeignRelations
want []string
}{
{
name: "collision test",
Expand All @@ -803,6 +804,7 @@ func TestKongState_BuildPluginsCollisions(t *testing.T) {
InstanceName: "test",
},
},
kongVersion: semver.MustParse("3.4.0"),
pluginRels: map[string]util.ForeignRelations{
"default:foo-plugin": {
// this shouldn't happen in practice, as all generated route names are unique
Expand All @@ -819,7 +821,7 @@ func TestKongState_BuildPluginsCollisions(t *testing.T) {
KongPlugins: tt.in,
})
// this is not testing the kongPluginFromK8SPlugin failure cases, so there is no failures collector
got := buildPlugins(log, store, nil, tt.pluginRels)
got := buildPlugins(log, store, tt.kongVersion, nil, tt.pluginRels)
require.Len(t, got, 2)
require.Equal(t, tt.want, []string{*got[0].InstanceName, *got[1].InstanceName})
})
Expand Down
17 changes: 10 additions & 7 deletions internal/dataplane/kongstate/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"fmt"

"github.com/blang/semver/v4"
"github.com/kong/go-kong/kong"
corev1 "k8s.io/api/core/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
Expand All @@ -13,6 +14,7 @@ import (
"github.com/kong/kubernetes-ingress-controller/v2/internal/annotations"
"github.com/kong/kubernetes-ingress-controller/v2/internal/store"
"github.com/kong/kubernetes-ingress-controller/v2/internal/util"
"github.com/kong/kubernetes-ingress-controller/v2/internal/versions"
kongv1 "github.com/kong/kubernetes-ingress-controller/v2/pkg/apis/configuration/v1"
)

Expand Down Expand Up @@ -111,6 +113,7 @@ func getKongPluginOrKongClusterPlugin(s store.Storer, namespace, name string) (
func kongPluginFromK8SClusterPlugin(
s store.Storer,
k8sPlugin kongv1.KongClusterPlugin,
kongVersion semver.Version,
) (Plugin, error) {
var config kong.Configuration
config, err := RawConfigToConfiguration(k8sPlugin.Config)
Expand Down Expand Up @@ -146,7 +149,7 @@ func kongPluginFromK8SClusterPlugin(
Disabled: k8sPlugin.Disabled,
Protocols: protocolsToStrings(k8sPlugin.Protocols),
Tags: util.GenerateTagsForObject(&k8sPlugin),
}.toKongPlugin(),
}.toKongPlugin(kongVersion),
K8sParent: &k8sPlugin,
}, nil
}
Expand All @@ -168,6 +171,7 @@ func protocolsToStrings(protocols []kongv1.KongProtocol) (res []string) {
func kongPluginFromK8SPlugin(
s store.Storer,
k8sPlugin kongv1.KongPlugin,
kongVersion semver.Version,
) (Plugin, error) {
var config kong.Configuration
config, err := RawConfigToConfiguration(k8sPlugin.Config)
Expand All @@ -193,16 +197,15 @@ func kongPluginFromK8SPlugin(

return Plugin{
Plugin: plugin{
Name: k8sPlugin.PluginName,
Config: config,

Name: k8sPlugin.PluginName,
Config: config,
RunOn: k8sPlugin.RunOn,
Ordering: k8sPlugin.Ordering,
InstanceName: k8sPlugin.InstanceName,
Disabled: k8sPlugin.Disabled,
Protocols: protocolsToStrings(k8sPlugin.Protocols),
Tags: util.GenerateTagsForObject(&k8sPlugin),
}.toKongPlugin(),
}.toKongPlugin(kongVersion),
K8sParent: &k8sPlugin,
}, nil
}
Expand Down Expand Up @@ -294,7 +297,7 @@ type plugin struct {
Tags []*string
}

func (p plugin) toKongPlugin() kong.Plugin {
func (p plugin) toKongPlugin(kongVersion semver.Version) kong.Plugin {
result := kong.Plugin{
Name: kong.String(p.Name),
Config: p.Config.DeepCopy(),
Expand All @@ -310,7 +313,7 @@ func (p plugin) toKongPlugin() kong.Plugin {
if len(p.Protocols) > 0 {
result.Protocols = kong.StringSlice(p.Protocols...)
}
if p.InstanceName != "" {
if p.InstanceName != "" && kongVersion.GT(versions.PluginInstanceNameCutoff) {
result.InstanceName = kong.String(p.InstanceName)
}
return result
Expand Down
67 changes: 63 additions & 4 deletions internal/dataplane/kongstate/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"regexp"
"testing"

"github.com/blang/semver/v4"
"github.com/kong/go-kong/kong"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -36,7 +37,8 @@ func TestKongPluginFromK8SClusterPlugin(t *testing.T) {
},
})
type args struct {
plugin kongv1.KongClusterPlugin
plugin kongv1.KongClusterPlugin
kongVersion semver.Version
}
tests := []struct {
name string
Expand All @@ -55,6 +57,7 @@ func TestKongPluginFromK8SClusterPlugin(t *testing.T) {
Raw: []byte(`{"header_name": "foo"}`),
},
},
kongVersion: semver.MustParse("3.4.0"),
},
want: kong.Plugin{
Name: kong.String("correlation-id"),
Expand All @@ -66,6 +69,29 @@ func TestKongPluginFromK8SClusterPlugin(t *testing.T) {
},
wantErr: false,
},
{
name: "basic configuration with Kong version not supporting instance name",
args: args{
plugin: kongv1.KongClusterPlugin{
Protocols: []kongv1.KongProtocol{"http"},
PluginName: "correlation-id",
InstanceName: "example",
Config: apiextensionsv1.JSON{
Raw: []byte(`{"header_name": "foo"}`),
},
},
kongVersion: semver.MustParse("2.8.0"),
},
want: kong.Plugin{
Name: kong.String("correlation-id"),
Config: kong.Configuration{
"header_name": "foo",
},
Protocols: kong.StringSlice("http"),
InstanceName: nil,
},
wantErr: false,
},
{
name: "secret configuration",
args: args{
Expand All @@ -80,6 +106,7 @@ func TestKongPluginFromK8SClusterPlugin(t *testing.T) {
},
},
},
kongVersion: semver.MustParse("3.4.0"),
},
want: kong.Plugin{
Name: kong.String("correlation-id"),
Expand All @@ -104,6 +131,7 @@ func TestKongPluginFromK8SClusterPlugin(t *testing.T) {
},
},
},
kongVersion: semver.MustParse("3.4.0"),
},
want: kong.Plugin{},
wantErr: true,
Expand All @@ -118,6 +146,7 @@ func TestKongPluginFromK8SClusterPlugin(t *testing.T) {
Raw: []byte(`{{}`),
},
},
kongVersion: semver.MustParse("3.4.0"),
},
want: kong.Plugin{},
wantErr: true,
Expand All @@ -139,14 +168,15 @@ func TestKongPluginFromK8SClusterPlugin(t *testing.T) {
},
},
},
kongVersion: semver.MustParse("3.4.0"),
},
want: kong.Plugin{},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := kongPluginFromK8SClusterPlugin(store, tt.args.plugin)
got, err := kongPluginFromK8SClusterPlugin(store, tt.args.plugin, tt.args.kongVersion)
if (err != nil) != tt.wantErr {
t.Errorf("kongPluginFromK8SClusterPlugin error = %v, wantErr %v", err, tt.wantErr)
return
Expand All @@ -173,7 +203,8 @@ func TestKongPluginFromK8SPlugin(t *testing.T) {
},
})
type args struct {
plugin kongv1.KongPlugin
plugin kongv1.KongPlugin
kongVersion semver.Version
}
tests := []struct {
name string
Expand All @@ -192,6 +223,7 @@ func TestKongPluginFromK8SPlugin(t *testing.T) {
Raw: []byte(`{"header_name": "foo"}`),
},
},
kongVersion: semver.MustParse("3.4.0"),
},
want: kong.Plugin{
Name: kong.String("correlation-id"),
Expand All @@ -203,6 +235,29 @@ func TestKongPluginFromK8SPlugin(t *testing.T) {
},
wantErr: false,
},
{
name: "basic configuration with Kong version not supporting instance name",
args: args{
plugin: kongv1.KongPlugin{
Protocols: []kongv1.KongProtocol{"http"},
PluginName: "correlation-id",
InstanceName: "example",
Config: apiextensionsv1.JSON{
Raw: []byte(`{"header_name": "foo"}`),
},
},
kongVersion: semver.MustParse("2.8.0"),
},
want: kong.Plugin{
Name: kong.String("correlation-id"),
Config: kong.Configuration{
"header_name": "foo",
},
Protocols: kong.StringSlice("http"),
InstanceName: nil,
},
wantErr: false,
},
{
name: "secret configuration",
args: args{
Expand All @@ -220,6 +275,7 @@ func TestKongPluginFromK8SPlugin(t *testing.T) {
},
},
},
kongVersion: semver.MustParse("3.4.0"),
},
want: kong.Plugin{
Name: kong.String("correlation-id"),
Expand Down Expand Up @@ -247,6 +303,7 @@ func TestKongPluginFromK8SPlugin(t *testing.T) {
},
},
},
kongVersion: semver.MustParse("3.4.0"),
},
want: kong.Plugin{},
wantErr: true,
Expand All @@ -261,6 +318,7 @@ func TestKongPluginFromK8SPlugin(t *testing.T) {
Raw: []byte(`{{}`),
},
},
kongVersion: semver.MustParse("3.4.0"),
},
want: kong.Plugin{},
wantErr: true,
Expand All @@ -281,14 +339,15 @@ func TestKongPluginFromK8SPlugin(t *testing.T) {
},
},
},
kongVersion: semver.MustParse("3.4.0"),
},
want: kong.Plugin{},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := kongPluginFromK8SPlugin(store, tt.args.plugin)
got, err := kongPluginFromK8SPlugin(store, tt.args.plugin, tt.args.kongVersion)
if (err != nil) != tt.wantErr {
t.Errorf("kongPluginFromK8SPlugin error = %v, wantErr %v", err, tt.wantErr)
return
Expand Down
2 changes: 1 addition & 1 deletion internal/dataplane/parser/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ func (p *Parser) BuildKongConfig() KongConfigBuildingResult {
}

// process annotation plugins
result.FillPlugins(p.logger, p.storer, p.failuresCollector)
result.FillPlugins(p.logger, p.storer, p.kongVersion, p.failuresCollector)
for i := range result.Plugins {
p.registerSuccessfullyParsedObject(result.Plugins[i].K8sParent)
}
Expand Down
3 changes: 3 additions & 0 deletions internal/versions/versions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,7 @@ var (

// ExpressionRouterL4Cutoff is the lowest Kong version with support of L4 proxy in expression router.
ExpressionRouterL4Cutoff = semver.Version{Major: 3, Minor: 4}

// PluginInstanceNameCutoff is the lowest Kong version having the `instance_name` field in `plugin` objects.
PluginInstanceNameCutoff = semver.Version{Major: 3, Minor: 2}
)