From 1bbbb8f039f0484b89cadf65409939af11f5e5ef Mon Sep 17 00:00:00 2001 From: Prashansa Kulshrestha Date: Mon, 6 Jan 2025 15:36:00 +0530 Subject: [PATCH 1/7] fix: returning a validation error in case of wrong configuration. As of now, while using deck to create a route-scoped plugin with a `service` field defined, the sync goes through the first time but errors out subsequently. Under a route, we do not want the user to add the `service` field for a plugin. Thus, we are checking for this specific scenario and erroring out, instead of letting the sync action go through. For: https://github.com/Kong/deck/issues/1353 --- pkg/file/builder.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/file/builder.go b/pkg/file/builder.go index 012e74f..a992ca1 100644 --- a/pkg/file/builder.go +++ b/pkg/file/builder.go @@ -1436,6 +1436,10 @@ func (b *stateBuilder) ingestRoute(r FRoute) error { // plugins for the route var plugins []FPlugin for _, p := range r.Plugins { + if p.Service != nil && !utils.Empty(p.Service.ID) { + return fmt.Errorf("nesting service (%v) under route-scoped plugin (%v) is not allowed", *p.Service.ID, *p.Name) + } + p.Route = utils.GetRouteReference(r.Route) plugins = append(plugins, *p) } From ffccd317210080a0cc0ba74d09fed5c200f2d3ab Mon Sep 17 00:00:00 2001 From: Prashansa Kulshrestha Date: Mon, 6 Jan 2025 16:28:27 +0530 Subject: [PATCH 2/7] fix: added checks for all conflicting nested configs in plugins. A foreign key nested under a plugin of a different scope would error out. This would make sure that a sync does not go through when wrong configurations are passed via deck. An example would be adding a `service` field in a plugin, nested under a route. This change ensures more such issues do not come up. --- pkg/file/builder.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/pkg/file/builder.go b/pkg/file/builder.go index a992ca1..be4f7f1 100644 --- a/pkg/file/builder.go +++ b/pkg/file/builder.go @@ -488,6 +488,10 @@ func (b *stateBuilder) consumers() { // plugins for the Consumer var plugins []FPlugin for _, p := range c.Plugins { + if err := checkForNestedForeignKeys(p.Plugin, "consumer"); err != nil { + b.err = err + return + } p.Consumer = utils.GetConsumerReference(c.Consumer) plugins = append(plugins, *p) } @@ -920,6 +924,9 @@ func (b *stateBuilder) ingestService(s *FService) error { // plugins for the service var plugins []FPlugin for _, p := range s.Plugins { + if err := checkForNestedForeignKeys(p.Plugin, "service"); err != nil { + return err + } p.Service = utils.GetServiceReference(s.Service) plugins = append(plugins, *p) } @@ -1436,6 +1443,9 @@ func (b *stateBuilder) ingestRoute(r FRoute) error { // plugins for the route var plugins []FPlugin for _, p := range r.Plugins { + if err := checkForNestedForeignKeys(p.Plugin, "route"); err != nil { + return err + } if p.Service != nil && !utils.Empty(p.Service.ID) { return fmt.Errorf("nesting service (%v) under route-scoped plugin (%v) is not allowed", *p.Service.ID, *p.Name) } @@ -1574,3 +1584,21 @@ func defaulter( } return defaulter, nil } + +func checkForNestedForeignKeys(plugin kong.Plugin, primary string) error { + var errs []error + + if primary != "consumer" && plugin.Consumer != nil && !utils.Empty(plugin.Consumer.ID) { + errs = append(errs, fmt.Errorf("nesting consumer (%v) under %v-scoped plugin plugin (%v) is not allowed", + *plugin.Consumer.ID, primary, *plugin.Name)) + } + if primary != "route" && plugin.Route != nil && !utils.Empty(plugin.Route.ID) { + errs = append(errs, fmt.Errorf("nesting route (%v) under %v-scoped plugin (%v) is not allowed", + *plugin.Route.ID, primary, *plugin.Name)) + } + if primary != "service" && plugin.Service != nil && !utils.Empty(plugin.Service.ID) { + errs = append(errs, fmt.Errorf("nesting service (%v) under %v-scoped plugin (%v) is not allowed", + *plugin.Service.ID, primary, *plugin.Name)) + } + return errors.Join(errs...) +} From 57b77a7643a66203c53975a4a97ac077e8e7cae0 Mon Sep 17 00:00:00 2001 From: Prashansa Kulshrestha Date: Mon, 6 Jan 2025 17:17:38 +0530 Subject: [PATCH 3/7] test: added integration test for validations related to scoped plugins --- pkg/file/builder.go | 4 ++ tests/integration/sync_test.go | 37 +++++++++++++++++++ .../consumer-plugins.yaml | 20 ++++++++++ .../route-plugins.yaml | 17 +++++++++ .../service-plugins.yaml | 18 +++++++++ 5 files changed, 96 insertions(+) create mode 100644 tests/integration/testdata/sync/036-scoped-plugins-validation/consumer-plugins.yaml create mode 100644 tests/integration/testdata/sync/036-scoped-plugins-validation/route-plugins.yaml create mode 100644 tests/integration/testdata/sync/036-scoped-plugins-validation/service-plugins.yaml diff --git a/pkg/file/builder.go b/pkg/file/builder.go index be4f7f1..9a2b573 100644 --- a/pkg/file/builder.go +++ b/pkg/file/builder.go @@ -1600,5 +1600,9 @@ func checkForNestedForeignKeys(plugin kong.Plugin, primary string) error { errs = append(errs, fmt.Errorf("nesting service (%v) under %v-scoped plugin (%v) is not allowed", *plugin.Service.ID, primary, *plugin.Name)) } + if primary != "consumer-group" && plugin.ConsumerGroup != nil && !utils.Empty(plugin.ConsumerGroup.ID) { + errs = append(errs, fmt.Errorf("nesting consumer-group (%v) under %v-scoped plugin (%v) is not allowed", + *plugin.ConsumerGroup.ID, primary, *plugin.Name)) + } return errors.Join(errs...) } diff --git a/tests/integration/sync_test.go b/tests/integration/sync_test.go index e4e4e12..d85eeaf 100644 --- a/tests/integration/sync_test.go +++ b/tests/integration/sync_test.go @@ -6114,3 +6114,40 @@ func Test_Sync_PluginDeprecatedFields38x(t *testing.T) { }) } } + +func Test_Sync_Scoped_Plugins_3x(t *testing.T) { + runWhen(t, "enterprise", ">=3.0.0") + setup(t) + + //client, err := getTestClient() + // require.NoError(t, err) + + tests := []struct { + name string + file string + errorExpected string + }{ + { + name: "syncing route-scoped plugin with service field set", + file: "testdata/sync/036-scoped-plugins-validation/route-plugins.yaml", + errorExpected: "building state: nesting service (example-service) under route-scoped plugin (request-transformer) is not allowed", + }, + { + name: "syncing service-scoped plugin with route and consumer field set", + file: "testdata/sync/036-scoped-plugins-validation/service-plugins.yaml", + errorExpected: "building state: nesting consumer (foo) under service-scoped plugin plugin (request-transformer) is not allowed\nnesting route (example-route) under service-scoped plugin (request-transformer) is not allowed", + }, + { + name: "syncing consumer-scoped plugin with service and route field set", + file: "testdata/sync/036-scoped-plugins-validation/consumer-plugins.yaml", + errorExpected: "building state: nesting route (example-route) under consumer-scoped plugin (request-transformer) is not allowed\nnesting service (example-service) under consumer-scoped plugin (request-transformer) is not allowed", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := sync(tc.file) + require.Equal(t, tc.errorExpected, err.Error()) + }) + } +} diff --git a/tests/integration/testdata/sync/036-scoped-plugins-validation/consumer-plugins.yaml b/tests/integration/testdata/sync/036-scoped-plugins-validation/consumer-plugins.yaml new file mode 100644 index 0000000..c4191c9 --- /dev/null +++ b/tests/integration/testdata/sync/036-scoped-plugins-validation/consumer-plugins.yaml @@ -0,0 +1,20 @@ +--- +_format_version: "3.0" +services: +- name: example-service + port: 3200 + protocol: http + routes: + - name: example-route + paths: + - ~/r1 +consumers: +- username: foo + plugins: + - name: request-transformer + route: example-route + service: example-service + config: + add: + querystring: + - test \ No newline at end of file diff --git a/tests/integration/testdata/sync/036-scoped-plugins-validation/route-plugins.yaml b/tests/integration/testdata/sync/036-scoped-plugins-validation/route-plugins.yaml new file mode 100644 index 0000000..8624df4 --- /dev/null +++ b/tests/integration/testdata/sync/036-scoped-plugins-validation/route-plugins.yaml @@ -0,0 +1,17 @@ +--- +_format_version: "3.0" +services: +- name: example-service + port: 3200 + protocol: http + routes: + - name: example-route + paths: + - ~/r1 + plugins: + - name: request-transformer + service: example-service + config: + add: + querystring: + - test \ No newline at end of file diff --git a/tests/integration/testdata/sync/036-scoped-plugins-validation/service-plugins.yaml b/tests/integration/testdata/sync/036-scoped-plugins-validation/service-plugins.yaml new file mode 100644 index 0000000..1412d95 --- /dev/null +++ b/tests/integration/testdata/sync/036-scoped-plugins-validation/service-plugins.yaml @@ -0,0 +1,18 @@ +--- +_format_version: "3.0" +services: +- name: example-service + port: 3200 + protocol: http + plugins: + - name: request-transformer + route: example-route + consumer: foo + config: + add: + querystring: + - test + routes: + - name: example-route + paths: + - ~/r1 \ No newline at end of file From 633264cf27adcc9d528485629de9593dd4162c35 Mon Sep 17 00:00:00 2001 From: Prashansa Kulshrestha Date: Mon, 6 Jan 2025 17:18:35 +0530 Subject: [PATCH 4/7] chore: removed redundant code lines --- pkg/file/builder.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg/file/builder.go b/pkg/file/builder.go index 9a2b573..de38d49 100644 --- a/pkg/file/builder.go +++ b/pkg/file/builder.go @@ -1446,10 +1446,6 @@ func (b *stateBuilder) ingestRoute(r FRoute) error { if err := checkForNestedForeignKeys(p.Plugin, "route"); err != nil { return err } - if p.Service != nil && !utils.Empty(p.Service.ID) { - return fmt.Errorf("nesting service (%v) under route-scoped plugin (%v) is not allowed", *p.Service.ID, *p.Name) - } - p.Route = utils.GetRouteReference(r.Route) plugins = append(plugins, *p) } From d0f5fe25760cca4760a7ba516aee7be089ad924e Mon Sep 17 00:00:00 2001 From: Prashansa Kulshrestha Date: Mon, 6 Jan 2025 17:19:21 +0530 Subject: [PATCH 5/7] chore: removed stray code lines from test --- tests/integration/sync_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/integration/sync_test.go b/tests/integration/sync_test.go index d85eeaf..f717849 100644 --- a/tests/integration/sync_test.go +++ b/tests/integration/sync_test.go @@ -6119,9 +6119,6 @@ func Test_Sync_Scoped_Plugins_3x(t *testing.T) { runWhen(t, "enterprise", ">=3.0.0") setup(t) - //client, err := getTestClient() - // require.NoError(t, err) - tests := []struct { name string file string From 9a3a11a89eba55076b7493e93481ec819608774e Mon Sep 17 00:00:00 2001 From: Prashansa Kulshrestha Date: Mon, 6 Jan 2025 17:25:51 +0530 Subject: [PATCH 6/7] test: fixed tests and added more for kong <3.0 --- tests/integration/sync_test.go | 38 ++++++++++++++++++- .../3x/consumer-plugins.yaml | 19 ++++++++++ .../3x/route-plugins.yaml | 16 ++++++++ .../3x/service-plugins.yaml | 17 +++++++++ .../consumer-plugins.yaml | 3 +- .../route-plugins.yaml | 3 +- .../service-plugins.yaml | 5 +-- 7 files changed, 92 insertions(+), 9 deletions(-) create mode 100644 tests/integration/testdata/sync/036-scoped-plugins-validation/3x/consumer-plugins.yaml create mode 100644 tests/integration/testdata/sync/036-scoped-plugins-validation/3x/route-plugins.yaml create mode 100644 tests/integration/testdata/sync/036-scoped-plugins-validation/3x/service-plugins.yaml diff --git a/tests/integration/sync_test.go b/tests/integration/sync_test.go index f717849..db400b0 100644 --- a/tests/integration/sync_test.go +++ b/tests/integration/sync_test.go @@ -6115,8 +6115,8 @@ func Test_Sync_PluginDeprecatedFields38x(t *testing.T) { } } -func Test_Sync_Scoped_Plugins_3x(t *testing.T) { - runWhen(t, "enterprise", ">=3.0.0") +func Test_Sync_Scoped_Plugins(t *testing.T) { + runWhen(t, "kong", "<3.0.0") setup(t) tests := []struct { @@ -6148,3 +6148,37 @@ func Test_Sync_Scoped_Plugins_3x(t *testing.T) { }) } } + +func Test_Sync_Scoped_Plugins_3x(t *testing.T) { + runWhen(t, "kong", ">=3.0.0") + setup(t) + + tests := []struct { + name string + file string + errorExpected string + }{ + { + name: "syncing route-scoped plugin with service field set", + file: "testdata/sync/036-scoped-plugins-validation/3x/route-plugins.yaml", + errorExpected: "building state: nesting service (example-service) under route-scoped plugin (request-transformer) is not allowed", + }, + { + name: "syncing service-scoped plugin with route and consumer field set", + file: "testdata/sync/036-scoped-plugins-validation/3x/service-plugins.yaml", + errorExpected: "building state: nesting consumer (foo) under service-scoped plugin plugin (request-transformer) is not allowed\nnesting route (example-route) under service-scoped plugin (request-transformer) is not allowed", + }, + { + name: "syncing consumer-scoped plugin with service and route field set", + file: "testdata/sync/036-scoped-plugins-validation/3x/consumer-plugins.yaml", + errorExpected: "building state: nesting route (example-route) under consumer-scoped plugin (request-transformer) is not allowed\nnesting service (example-service) under consumer-scoped plugin (request-transformer) is not allowed", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := sync(tc.file) + require.Equal(t, tc.errorExpected, err.Error()) + }) + } +} diff --git a/tests/integration/testdata/sync/036-scoped-plugins-validation/3x/consumer-plugins.yaml b/tests/integration/testdata/sync/036-scoped-plugins-validation/3x/consumer-plugins.yaml new file mode 100644 index 0000000..f52e6dc --- /dev/null +++ b/tests/integration/testdata/sync/036-scoped-plugins-validation/3x/consumer-plugins.yaml @@ -0,0 +1,19 @@ +_format_version: "3.0" +services: +- name: example-service + port: 3200 + protocol: http + routes: + - name: example-route + paths: + - ~/r1 +consumers: +- username: foo + plugins: + - name: request-transformer + route: example-route + service: example-service + config: + add: + querystring: + - test \ No newline at end of file diff --git a/tests/integration/testdata/sync/036-scoped-plugins-validation/3x/route-plugins.yaml b/tests/integration/testdata/sync/036-scoped-plugins-validation/3x/route-plugins.yaml new file mode 100644 index 0000000..c7743dd --- /dev/null +++ b/tests/integration/testdata/sync/036-scoped-plugins-validation/3x/route-plugins.yaml @@ -0,0 +1,16 @@ +_format_version: "3.0" +services: +- name: example-service + port: 3200 + protocol: http + routes: + - name: example-route + paths: + - ~/r1 + plugins: + - name: request-transformer + service: example-service + config: + add: + querystring: + - test \ No newline at end of file diff --git a/tests/integration/testdata/sync/036-scoped-plugins-validation/3x/service-plugins.yaml b/tests/integration/testdata/sync/036-scoped-plugins-validation/3x/service-plugins.yaml new file mode 100644 index 0000000..0e8a04e --- /dev/null +++ b/tests/integration/testdata/sync/036-scoped-plugins-validation/3x/service-plugins.yaml @@ -0,0 +1,17 @@ +_format_version: "3.0" +services: +- name: example-service + port: 3200 + protocol: http + plugins: + - name: request-transformer + route: example-route + consumer: foo + config: + add: + querystring: + - test + routes: + - name: example-route + paths: + - ~/r1 diff --git a/tests/integration/testdata/sync/036-scoped-plugins-validation/consumer-plugins.yaml b/tests/integration/testdata/sync/036-scoped-plugins-validation/consumer-plugins.yaml index c4191c9..6ce609d 100644 --- a/tests/integration/testdata/sync/036-scoped-plugins-validation/consumer-plugins.yaml +++ b/tests/integration/testdata/sync/036-scoped-plugins-validation/consumer-plugins.yaml @@ -1,5 +1,4 @@ ---- -_format_version: "3.0" +_format_version: "1.0" services: - name: example-service port: 3200 diff --git a/tests/integration/testdata/sync/036-scoped-plugins-validation/route-plugins.yaml b/tests/integration/testdata/sync/036-scoped-plugins-validation/route-plugins.yaml index 8624df4..6bda494 100644 --- a/tests/integration/testdata/sync/036-scoped-plugins-validation/route-plugins.yaml +++ b/tests/integration/testdata/sync/036-scoped-plugins-validation/route-plugins.yaml @@ -1,5 +1,4 @@ ---- -_format_version: "3.0" +_format_version: "1.0" services: - name: example-service port: 3200 diff --git a/tests/integration/testdata/sync/036-scoped-plugins-validation/service-plugins.yaml b/tests/integration/testdata/sync/036-scoped-plugins-validation/service-plugins.yaml index 1412d95..739a18e 100644 --- a/tests/integration/testdata/sync/036-scoped-plugins-validation/service-plugins.yaml +++ b/tests/integration/testdata/sync/036-scoped-plugins-validation/service-plugins.yaml @@ -1,5 +1,4 @@ ---- -_format_version: "3.0" +_format_version: "1.0" services: - name: example-service port: 3200 @@ -15,4 +14,4 @@ services: routes: - name: example-route paths: - - ~/r1 \ No newline at end of file + - ~/r1 From c2be37c3f4e0708f30410c094c90609dcfa386e7 Mon Sep 17 00:00:00 2001 From: Prashansa Kulshrestha Date: Thu, 9 Jan 2025 13:31:49 +0530 Subject: [PATCH 7/7] chore: addressed PR comments --- pkg/file/builder.go | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/pkg/file/builder.go b/pkg/file/builder.go index de38d49..70d8c11 100644 --- a/pkg/file/builder.go +++ b/pkg/file/builder.go @@ -19,6 +19,13 @@ import ( const ratelimitingAdvancedPluginName = "rate-limiting-advanced" +const ( + primaryRelationConsumer = "consumer" + primaryRelationConsumerGroup = "consumer-group" + primaryRelationRoute = "route" + primaryRelationService = "service" +) + type stateBuilder struct { targetContent *Content rawState *utils.KongRawState @@ -488,7 +495,7 @@ func (b *stateBuilder) consumers() { // plugins for the Consumer var plugins []FPlugin for _, p := range c.Plugins { - if err := checkForNestedForeignKeys(p.Plugin, "consumer"); err != nil { + if err := checkForNestedForeignKeys(p.Plugin, primaryRelationConsumer); err != nil { b.err = err return } @@ -924,7 +931,7 @@ func (b *stateBuilder) ingestService(s *FService) error { // plugins for the service var plugins []FPlugin for _, p := range s.Plugins { - if err := checkForNestedForeignKeys(p.Plugin, "service"); err != nil { + if err := checkForNestedForeignKeys(p.Plugin, primaryRelationService); err != nil { return err } p.Service = utils.GetServiceReference(s.Service) @@ -1443,7 +1450,7 @@ func (b *stateBuilder) ingestRoute(r FRoute) error { // plugins for the route var plugins []FPlugin for _, p := range r.Plugins { - if err := checkForNestedForeignKeys(p.Plugin, "route"); err != nil { + if err := checkForNestedForeignKeys(p.Plugin, primaryRelationRoute); err != nil { return err } p.Route = utils.GetRouteReference(r.Route) @@ -1584,19 +1591,19 @@ func defaulter( func checkForNestedForeignKeys(plugin kong.Plugin, primary string) error { var errs []error - if primary != "consumer" && plugin.Consumer != nil && !utils.Empty(plugin.Consumer.ID) { + if primary != primaryRelationConsumer && plugin.Consumer != nil && !utils.Empty(plugin.Consumer.ID) { errs = append(errs, fmt.Errorf("nesting consumer (%v) under %v-scoped plugin plugin (%v) is not allowed", *plugin.Consumer.ID, primary, *plugin.Name)) } - if primary != "route" && plugin.Route != nil && !utils.Empty(plugin.Route.ID) { + if primary != primaryRelationRoute && plugin.Route != nil && !utils.Empty(plugin.Route.ID) { errs = append(errs, fmt.Errorf("nesting route (%v) under %v-scoped plugin (%v) is not allowed", *plugin.Route.ID, primary, *plugin.Name)) } - if primary != "service" && plugin.Service != nil && !utils.Empty(plugin.Service.ID) { + if primary != primaryRelationService && plugin.Service != nil && !utils.Empty(plugin.Service.ID) { errs = append(errs, fmt.Errorf("nesting service (%v) under %v-scoped plugin (%v) is not allowed", *plugin.Service.ID, primary, *plugin.Name)) } - if primary != "consumer-group" && plugin.ConsumerGroup != nil && !utils.Empty(plugin.ConsumerGroup.ID) { + if primary != primaryRelationConsumerGroup && plugin.ConsumerGroup != nil && !utils.Empty(plugin.ConsumerGroup.ID) { errs = append(errs, fmt.Errorf("nesting consumer-group (%v) under %v-scoped plugin (%v) is not allowed", *plugin.ConsumerGroup.ID, primary, *plugin.Name)) }