feat: support scoping plugins to consumer-groups #959

GGabriele · 2023-07-11T13:29:36Z

No description provided.

codecov-commenter · 2023-07-17T16:45:24Z

Codecov Report

Patch coverage: 8.06% and project coverage change: -0.40% ⚠️

Comparison is base (37196ea) 33.98% compared to head (2c970cd) 33.59%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #959      +/-   ##
==========================================
- Coverage   33.98%   33.59%   -0.40%     
==========================================
  Files         101      101              
  Lines       12201    12364     +163     
==========================================
+ Hits         4147     4154       +7     
- Misses       7651     7801     +150     
- Partials      403      409       +6

Files Changed	Coverage Δ
cmd/common.go	`4.26% <0.00%> (-0.04%)`	⬇️
cmd/reset.go	`0.00% <0.00%> (ø)`
dump/dump.go	`1.44% <0.00%> (-0.02%)`	⬇️
file/types.go	`56.13% <0.00%> (-1.74%)`	⬇️
state/builder.go	`0.00% <0.00%> (ø)`
utils/utils.go	`25.61% <0.00%> (-1.34%)`	⬇️
file/writer.go	`14.28% <3.57%> (-0.58%)`	⬇️
file/builder.go	`44.90% <6.81%> (-3.40%)`	⬇️
state/types.go	`57.40% <10.00%> (-0.55%)`	⬇️
state/plugin.go	`84.21% <77.77%> (-0.73%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

czeslavo · 2023-07-31T09:53:25Z

Can we please ensure that we have integration test coverage for Konnect Runtime Group API as well?

GGabriele · 2023-07-31T10:42:35Z

Can we please ensure that we have integration test coverage for Konnect Runtime Group API as well?

Some have already been added here: 16e0a24

czeslavo · 2023-07-31T10:50:04Z

Can we please ensure that we have integration test coverage for Konnect Runtime Group API as well?

Some have already been added here: 16e0a24

Is there any reason to not run the ones added in this PR against Konnect (and in general, do that for every integration test we add)? As we're aware Konnect API is not always fully compatible with Kong, I think we should be very careful assuming they will work the same (that is a lesson I learned from #978). I think it would be ideal to have the same coverage for Konnect as for the regular Gateway and write the tests in a way so they're not specific to any of those.

GGabriele · 2023-07-31T11:13:23Z

Can we please ensure that we have integration test coverage for Konnect Runtime Group API as well?

Some have already been added here: 16e0a24

Is there any reason to not run the ones added in this PR against Konnect (and in general, do that for every integration test we add)?

The one test added here against the Gateway that is not mapped to a Konnect test is the one testing the Consumer Groups functionality in the runtime (e.g. deploying CGs and checking that proxy requests are limited as expected). This sort of e2e with Konnect is not covered in this repo (it would require spinning up a real DP and connect it to Konnect), but it's covered elsewhere.

As we're aware Konnect API is not always fully compatible with Kong, I think we should be very careful assuming they will work the same (that is a lesson I learned from #978).

Totally agree with that and any deficiency should be fixed.

I think it would be ideal to have the same coverage for Konnect as for the regular Gateway and write the tests in a way so they're not specific to any of those.

For the most part that's already the case. Again, any deficiency of this is an overlook and it should be fixed.

Also agree that the testing framework needs some love indeed.

czeslavo

The one test added here against the Gateway that is not mapped to a Konnect test is the one testing the Consumer Groups functionality in the runtime (e.g. deploying CGs and checking that proxy requests are limited as expected). This sort of e2e with Konnect is not covered in this repo (it would require spinning up a real DP and connect it to Konnect), but it's covered elsewhere.

Yeah, thanks for the explanation. That makes sense. That for sure is something that couldn't be easily done in this PR (testing DP behavior with Konnect).

I left comments on tests that I believe could be run against Konnect, please correct me if I'm wrong about them. 🙇

tests/integration/dump_test.go

tests/integration/sync_test.go

tests/integration/dump_test.go

cmd/common_konnect.go

tests/integration/dump_test.go

file/builder.go

tests/integration/dump_test.go

tests/integration/sync_test.go

pmalek · 2023-08-02T09:19:31Z

state/plugin.go

 // Otherwise, a plugin with name and the supplied foreign references is
 // searched.
 // name is required.
-func (k *PluginsCollection) GetByProp(name, serviceID,
-	routeID string, consumerID string,
+func (k *PluginsCollection) GetByProp(


Nothing actionable for now but given that GetByProp has grown to have 5 params now, it became hard to understand which parameter is used for what. Especially since all of those are of type string.

It's also "easy to misuse": e.g. use argument in different order than expected.

pmalek · 2023-08-02T09:21:05Z

file/kong_json_schema.json

@@ -444,7 +444,6 @@
        },
        "groups": {
          "items": {
-            "$schema": "http://json-schema.org/draft-04/schema#",


Is this change related to Consumer Groups work? 🤔

This file is auto-generated from the go-kong update.

In that case: #991, let's mark it as generated to prevent folks from reviewing this.

pmalek · 2023-08-02T09:22:59Z

file/builder.go

+			return errors.New("a rate-limiting-advanced plugin with config.consumer_groups\n" +
+				"and/or config.enforce_consumer_groups was found. Please use Consumer Groups scoped\n" +
+				"Plugins when running against Kong Enterprise 3.4.0 and above.\n\n" +
+				"Check DOC_LINK for more information")


Perhaps we could put this error in a variable which could then be compared against at the call site?

czeslavo · 2023-08-02T11:19:06Z

cmd/common_konnect.go

@@ -129,6 +129,7 @@ func resetKonnectV2(ctx context.Context) error {
 	if err != nil {
 		return err
 	}
+	dumpConfig.IsConsumerGroupScopedPluginSupported = true


Why is this true only in resetKonnectV2 function while in the others (e.g. dumpKonnectV2) it's not? How does it correspond with the fact that Konnect support for CG-scoped plugins was not released yet?

Actually we don't need this flag set here. We only need when reading from a configuration file (so when running sync) as opposite of reset and dump. I'll remove it.

utils/utils.go

czeslavo · 2023-08-02T12:12:35Z

tests/integration/dump_test.go

+	}
+}
+
+func Test_Dump_SkipConsumers_34x(t *testing.T) {


Nit: we could avoid test code duplication if we extended the testcase struct in the original Test_Dump_SkipConsumers test like so:

func Test_Dump_SkipConsumers(t *testing.T) { tests := []struct { name string stateFile string expectedFile string skipConsumers bool runWhen func(t *testing.T) }{ ... { name: "3.4 dump with skip-consumers", stateFile: "testdata/dump/002-skip-consumers/kong34.yaml", expectedFile: "testdata/dump/002-skip-consumers/expected.yaml", skipConsumers: true, runWhen: func(t *testing.T) { runWhen(t, "enterprise", ">=3.4.0 <3.5.0") }, }, { name: "3.4 dump with no skip-consumers", stateFile: "testdata/dump/002-skip-consumers/kong34.yaml", expectedFile: "testdata/dump/002-skip-consumers/expected-no-skip-34.yaml", skipConsumers: false, runWhen: func(t *testing.T) { runWhen(t, "enterprise", ">=3.4.0 <3.5.0") }, }, { name: "3.5 dump with skip-consumers", stateFile: "testdata/dump/002-skip-consumers/kong34.yaml", expectedFile: "testdata/dump/002-skip-consumers/expected.yaml", skipConsumers: true, runWhen: func(t *testing.T) { runWhen(t, "enterprise", ">=3.5.0") }, }, { name: "3.5 dump with no skip-consumers", stateFile: "testdata/dump/002-skip-consumers/kong34.yaml", expectedFile: "testdata/dump/002-skip-consumers/expected-no-skip-35.yaml", skipConsumers: false, runWhen: func(t *testing.T) { runWhen(t, "enterprise", ">=3.5.0") }, }, ... } ... t.Run(tc.name, func(t *testing.T) { tc.runWhen(t)

czeslavo

LGTM 👍

file/builder.go

GGabriele force-pushed the feat/new-consumer-groups branch from cf552c4 to 0a896f6 Compare July 17, 2023 16:42

GGabriele marked this pull request as ready for review July 17, 2023 16:50

GGabriele requested a review from a team July 17, 2023 16:50

GGabriele force-pushed the feat/new-consumer-groups branch from 1123668 to bc50703 Compare July 26, 2023 13:03

mflendrich mentioned this pull request Jul 26, 2023

Consumer groups staged delivery Kong/kubernetes-ingress-controller#4411

Closed

6 tasks

czeslavo reviewed Jul 31, 2023

View reviewed changes

tests/integration/dump_test.go Show resolved Hide resolved

tests/integration/dump_test.go Outdated Show resolved Hide resolved

tests/integration/sync_test.go Outdated Show resolved Hide resolved

GGabriele force-pushed the feat/new-consumer-groups branch 6 times, most recently from 19f7fe1 to 40eb7b2 Compare July 31, 2023 17:17

feat: support scoping plugins to consumer-groups

9f68186

GGabriele force-pushed the feat/new-consumer-groups branch from 40eb7b2 to 9f68186 Compare July 31, 2023 17:57

GGabriele requested a review from czeslavo July 31, 2023 18:23

feat: disable Consumer Groups scoped Plugins for Konnect

b2b24d3

pmalek reviewed Aug 1, 2023

View reviewed changes

tests/integration/dump_test.go Outdated Show resolved Hide resolved

czeslavo reviewed Aug 1, 2023

View reviewed changes

addressing comments

f7222bc

GGabriele force-pushed the feat/new-consumer-groups branch from e2d635f to f7222bc Compare August 2, 2023 09:08

GGabriele requested review from pmalek and czeslavo August 2, 2023 09:11

pmalek reviewed Aug 2, 2023

View reviewed changes

more comments

cd55b4f

czeslavo reviewed Aug 2, 2023

View reviewed changes

utils/utils.go Show resolved Hide resolved

czeslavo reviewed Aug 2, 2023

View reviewed changes

GGabriele force-pushed the feat/new-consumer-groups branch from f97ffbb to bbef9fb Compare August 2, 2023 15:28

more comments - take 2

ef83a97

GGabriele force-pushed the feat/new-consumer-groups branch from bbef9fb to ef83a97 Compare August 2, 2023 15:33

czeslavo previously approved these changes Aug 2, 2023

View reviewed changes

programmer04 requested changes Aug 3, 2023

View reviewed changes

file/builder.go Outdated Show resolved Hide resolved

GGabriele dismissed czeslavo’s stale review via 2c970cd August 3, 2023 09:37

fix

2c970cd

GGabriele requested review from czeslavo, pmalek and programmer04 August 3, 2023 16:13

programmer04 mentioned this pull request Aug 3, 2023

feat(consumer group): allow assigning plugins to consumer groups Kong/kubernetes-ingress-controller#4452

Merged

1 task

programmer04 approved these changes Aug 4, 2023

View reviewed changes

GGabriele merged commit af7eaa2 into main Aug 4, 2023
34 checks passed

GGabriele deleted the feat/new-consumer-groups branch August 4, 2023 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support scoping plugins to consumer-groups #959

feat: support scoping plugins to consumer-groups #959

GGabriele commented Jul 11, 2023

codecov-commenter commented Jul 17, 2023 •

edited

Loading

czeslavo commented Jul 31, 2023

GGabriele commented Jul 31, 2023

czeslavo commented Jul 31, 2023

GGabriele commented Jul 31, 2023

czeslavo left a comment

pmalek Aug 2, 2023

pmalek Aug 2, 2023

GGabriele Aug 2, 2023

pmalek Aug 2, 2023

pmalek Aug 2, 2023

czeslavo Aug 2, 2023

GGabriele Aug 2, 2023

czeslavo Aug 2, 2023

GGabriele Aug 2, 2023

czeslavo left a comment

feat: support scoping plugins to consumer-groups #959

feat: support scoping plugins to consumer-groups #959

Conversation

GGabriele commented Jul 11, 2023

codecov-commenter commented Jul 17, 2023 • edited Loading

Codecov Report

czeslavo commented Jul 31, 2023

GGabriele commented Jul 31, 2023

czeslavo commented Jul 31, 2023

GGabriele commented Jul 31, 2023

czeslavo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czeslavo left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 17, 2023 •

edited

Loading