feat: expand upstreams model and validation for 3.0 #311

GGabriele · 2022-07-22T09:58:51Z

Kong related changes

internal/server/kong/ws/config/version_compatibility.go

tjasko

Changes seem sane but would require a bit deeper investigation to verify all the validation logic.

internal/server/kong/ws/config/version_compatibility.go

hbagdi · 2022-07-25T23:18:19Z

Adding a hold to discuss downgrades.

hbagdi

I did an initial pass.
An integration test that ensures that these new fields make it down to the DP are missing.
The functionality for that is also missing (no proto updates in kong/kong or this repository).

internal/resource/upstream.go

hbagdi · 2022-07-28T19:43:10Z

Please rebase.

hbagdi · 2022-08-01T20:04:27Z

@GGabriele Please rebase.

GGabriele · 2022-08-02T12:02:57Z

@GGabriele Please rebase.

Rebased on top of the feat/services-vc branch in order to avoid duplicating effort

internal/test/e2e/version_compatibility_test.go

tjasko · 2022-08-03T16:34:22Z

internal/server/kong/ws/config/version_compatibility.go

+						conditionUpdate := fieldUpdate.Field
+						if fieldUpdate.Value == nil && len(fieldUpdate.ValueFromField) == 0 {
+							// Handle field removal
+							if updatedRaw, err = sjson.Delete(updatedRaw, conditionUpdate); err != nil {


Can we ensure this case is handled in the integration test?

This is already tested

Testing this on a 2.8 DP does not execute this code. Am I doing something wrong? This also does not run on a 3.0 DP (commented about this).

Maybe we can run these together when you get online? I'm not able to run integration tests (running on a MacOS), but I test connecting a DP with koko and this is what I see when I push something like this (needing fields removal and updates):

http :3000/v1/upstreams\?cluster.id=4168295f-015e-4190-837e-0fcc5d72a52f name=foo-with-hash_on_query_arg hash_on=ip hash_on_query_arg=test http :3000/v1/upstreams\?cluster.id=4168295f-015e-4190-837e-0fcc5d72a52f name=foo-with-hash_on hash_on=path hash_on_query_arg=test http :3000/v1/upstreams\?cluster.id=4168295f-015e-4190-837e-0fcc5d72a52f name=foo-with-hash_fallback hash_on=ip hash_fallback=path hash_on_query_arg=test

2022-08-05T09:51:46.892+0200 warn config/version_compatibility.go:498 removing entity field which is incompatible with data plane {"component": "version-compatibility", "entity": "upstreams", "name": "\"foo-with-hash_on_query_arg\"", "field": "hash_on_query_arg", "data-plane": 2008000000} 2022-08-05T09:51:46.892+0200 warn config/version_compatibility.go:498 removing entity field which is incompatible with data plane {"component": "version-compatibility", "entity": "upstreams", "name": "\"foo-with-hash_on\"", "field": "hash_on_query_arg", "data-plane": 2008000000} 2022-08-05T09:51:46.892+0200 warn config/version_compatibility.go:561 removing entity field which is incompatible with data plane {"component": "version-compatibility", "entity": "upstreams", "field": "hash_on", "condition": "hash_on=path", "new-value": "none", "data-plane": 2008000000} 2022-08-05T09:51:46.892+0200 warn config/version_compatibility.go:498 removing entity field which is incompatible with data plane {"component": "version-compatibility", "entity": "upstreams", "name": "\"foo-with-hash_fallback\"", "field": "hash_on_query_arg", "data-plane": 2008000000} 2022-08-05T09:51:46.892+0200 warn config/version_compatibility.go:561 removing entity field which is incompatible with data plane {"component": "version-compatibility", "entity": "upstreams", "field": "hash_fallback", "condition": "hash_fallback=path", "new-value": "none", "data-plane": 2008000000}

Also things like this in the e2e should error out, but they pass, which confuses me.

Yeah I have to work around some tooling to run them on macOS. I have some work tracked to get that improved, I do want to make testing of these things significantly easier.

We can pair on this & I can show you how I did this.

Did we figure anything out here?

Yeah what you were asking for is now covered here. That's not a e2e test because we don't have a compatibility change leveraging that portion of the code just yet (and e2e tests use the "real" compatibility table used by koko), so I just added a test case in the unit tests for the upstreams making sure we hit that portion of logic not otherwise tested in the integrations.

Let me know what you think!

That part looks good now, can we get this covered part of the unit test too?

koko/internal/server/kong/ws/config/version_compatibility.go

Lines 590 to 598 in be56518

} else {

vc.logger.With(zap.String("entity", entityName)).

With(zap.String("field", update.Field)).

With(zap.String("condition", update.Condition)).

With(zap.Any("new-value", fieldUpdate.Value)).

With(zap.String("data-plane", dataPlaneVersion)).

With(zap.Error(err)).

Error("entity configuration does not contain field value")

break

Added. Just note that we should never get hit this and it only goes here in case of errors .

internal/server/kong/ws/config/version_compatibility.go

hbagdi · 2022-08-04T19:02:53Z

Please get the final review from Taylor and please merge feat/services-vc into main and then rebase this on top of main.

tjasko · 2022-08-04T20:57:07Z

internal/test/e2e/version_compatibility_test.go

@@ -371,6 +377,139 @@ func TestVersionCompatibility(t *testing.T) {
 	})
 }

+func TestUpstreamsVersionCompatibility(t *testing.T) {


I am unable to get this test to pass on a 3.0 DP, it actually fails:

version_compatibility_test.go:466: upstreams validation failed can't match element at upstreams[0] util.go:48: failed to complete operation: can't match element at upstreams[0]

Took a deeper look into the logs here, may be wRPC related:

dp-stderr: 2022/08/10 15:04:51 [debug] 3001#0: *6 [lua] grpc.lua:29: safe_set_type_hook(): no type '.google.protobuf.Timestamp' defined dp-stderr: 2022/08/10 15:04:52 [notice] 3001#0: *6 [lua] message.lua:128: handle_error(): [wRPC] Received error message, 5.4:1 (--: "invalid call: invalid rpc(4) for service(5)"), context: ngx.timer

Running the following DP image:

$ docker images kong/kong:master-ubuntu REPOSITORY TAG IMAGE ID CREATED SIZE kong/kong master-ubuntu 6873404d2bc7 32 minutes ago 291MB

I'm told this resolves it: Kong/kong#9199
#344

tjasko · 2022-08-04T21:01:22Z

internal/server/kong/ws/config/version_compatibility.go

+						conditionUpdate := fieldUpdate.Field
+						if fieldUpdate.Value == nil && len(fieldUpdate.ValueFromField) == 0 {
+							// Handle field removal
+							if updatedRaw, err = sjson.Delete(updatedRaw, conditionUpdate); err != nil {


Testing this on a 2.8 DP does not execute this code. Am I doing something wrong? This also does not run on a 3.0 DP (commented about this).

internal/server/kong/ws/config/compat/compatibility_table.go

hbagdi · 2022-08-10T17:25:14Z

@GGabriele Please work with Taylor to get his comments resolved.

omegabytes · 2022-08-11T20:35:50Z

Also hitting this error when testing against 3.0

dp-stderr: 2022/08/11 20:34:25 [notice] 3000#0: *6 [lua] message.lua:128: handle_error(): [wRPC] Received error message, 5.4:1 (--: "invalid call: invalid rpc(4) for service(5)"), context: ngx.timer

GGabriele · 2022-08-17T13:28:50Z

@tjasko let me know if you have any more comments!

hbagdi · 2022-08-18T17:17:51Z

Please rebase.
I'll take a look after Taylor.

GGabriele · 2022-08-19T15:27:01Z

Tests are passing with DP<3.0 (included in this branch) and I could successfully test this on DP>=3.0 (showed in the our last call). Let me know if we need anything else to move this forward! @tjasko @mikefero

tjasko

Just some minor nits. I'll take your word for it that you were able to test this on 3.0.

internal/server/kong/ws/config/compat/compatibility_table.go

tjasko · 2022-08-19T16:07:00Z

internal/server/kong/ws/config/compat/compatibility_table.go

+					"one or more of the '%s' schema fields are set with one "+
+					"of the following values: %s, but Kong gateway versions < 3.0 "+
+					"do not support these values. Because of this, 'hash_on' and 'hash_fallback'"+
+					"have been changed in the data-plane to 'none' and hashing is "+


nit: We have three ways of representing a DP within the code:

Data plane

Dataplane

Data-plane

Kong Gateway

We're mostly using "Kong Gateway" when referring to DPs to users, so should we be consistent?

No objection, but maybe we should tackle this on a different branch? These inconsistencies are widespread in the codebase.

Potentially, because this is customer-facing, I'm treating it a bit differently from everything else.

internal/server/kong/ws/config/version_compatibility.go

internal/test/e2e/version_compatibility_test.go

hbagdi

Leaving the final approval and merging to @mikefero.

mikefero · 2022-08-23T14:16:44Z

Rebasing and fixing conflicts.

GGabriele requested a review from a team as a code owner July 22, 2022 09:58

GGabriele force-pushed the feat/upstream-vc branch from e9df7b6 to 9fd5c9b Compare July 22, 2022 10:06

mikefero reviewed Jul 22, 2022

View reviewed changes

internal/server/kong/ws/config/version_compatibility.go Outdated Show resolved Hide resolved

tjasko reviewed Jul 25, 2022

View reviewed changes

internal/server/kong/ws/config/version_compatibility.go Outdated Show resolved Hide resolved

hbagdi added the hold/do-not-merge label Jul 25, 2022

GGabriele force-pushed the feat/upstream-vc branch 4 times, most recently from 06c482f to 9bf6ad1 Compare July 26, 2022 13:45

hbagdi suggested changes Jul 26, 2022

View reviewed changes

internal/resource/upstream.go Outdated Show resolved Hide resolved

GGabriele force-pushed the feat/upstream-vc branch from 9bf6ad1 to 2e9032e Compare August 2, 2022 11:22

GGabriele changed the base branch from main to feat/services-vc August 2, 2022 11:22

GGabriele force-pushed the feat/upstream-vc branch 3 times, most recently from cfacd30 to 8bce826 Compare August 2, 2022 12:01

GGabriele requested review from mikefero and hbagdi August 2, 2022 12:03

GGabriele force-pushed the feat/upstream-vc branch from 8bce826 to 7779ac8 Compare August 2, 2022 12:08

GGabriele force-pushed the feat/services-vc branch from 9bf70ef to 0a20d1f Compare August 3, 2022 08:19

tjasko reviewed Aug 3, 2022

View reviewed changes

GGabriele force-pushed the feat/upstream-vc branch from 7779ac8 to ac793c9 Compare August 4, 2022 14:15

GGabriele force-pushed the feat/services-vc branch from 0a20d1f to 9e6b32c Compare August 4, 2022 14:20

GGabriele force-pushed the feat/upstream-vc branch from ac793c9 to 31c17be Compare August 4, 2022 14:20

tjasko reviewed Aug 4, 2022

View reviewed changes

GGabriele force-pushed the feat/services-vc branch from 9e6b32c to 0e39ebf Compare August 5, 2022 06:19

hbagdi removed the hold/do-not-merge label Aug 9, 2022

hbagdi reviewed Aug 9, 2022

View reviewed changes

internal/server/kong/ws/config/compat/compatibility_table.go Outdated Show resolved Hide resolved

GGabriele force-pushed the feat/upstream-vc branch from 3f75b4c to f579008 Compare August 10, 2022 10:33

GGabriele requested a review from hbagdi August 10, 2022 10:33

GGabriele force-pushed the feat/upstream-vc branch 2 times, most recently from 6413278 to be56518 Compare August 11, 2022 11:36

GGabriele requested a review from tjasko August 11, 2022 11:46

GGabriele force-pushed the feat/upstream-vc branch 4 times, most recently from 8777f4a to d52ecfb Compare August 17, 2022 13:20

GGabriele force-pushed the feat/upstream-vc branch from d52ecfb to 84f5eb4 Compare August 18, 2022 09:33

GGabriele force-pushed the feat/upstream-vc branch from 84f5eb4 to 4a617d4 Compare August 18, 2022 23:07

tjasko reviewed Aug 19, 2022

View reviewed changes

GGabriele force-pushed the feat/upstream-vc branch 2 times, most recently from 9af2384 to 18fbc3a Compare August 19, 2022 19:10

hbagdi previously approved these changes Aug 19, 2022

View reviewed changes

feat: expand upstreams model and validation for 3.0

ceaa99a

feat: add version compatibility logic for upstreams

1a5fb2b

mikefero dismissed hbagdi’s stale review via 1a5fb2b August 23, 2022 16:23

mikefero force-pushed the feat/upstream-vc branch from 18fbc3a to 1a5fb2b Compare August 23, 2022 16:23

mikefero approved these changes Aug 23, 2022

View reviewed changes

mikefero merged commit d5af949 into main Aug 23, 2022

mikefero deleted the feat/upstream-vc branch August 23, 2022 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: expand upstreams model and validation for 3.0 #311

feat: expand upstreams model and validation for 3.0 #311

GGabriele commented Jul 22, 2022 •

edited

Loading

tjasko left a comment

hbagdi commented Jul 25, 2022

hbagdi left a comment

hbagdi commented Jul 28, 2022

hbagdi commented Aug 1, 2022

GGabriele commented Aug 2, 2022

tjasko Aug 3, 2022

GGabriele Aug 4, 2022

tjasko Aug 4, 2022

GGabriele Aug 5, 2022

tjasko Aug 5, 2022

tjasko Aug 10, 2022

GGabriele Aug 11, 2022

tjasko Aug 11, 2022

GGabriele Aug 12, 2022

hbagdi commented Aug 4, 2022

tjasko Aug 4, 2022

tjasko Aug 10, 2022

omegabytes Aug 11, 2022 •

edited

Loading

tjasko Aug 4, 2022

hbagdi commented Aug 10, 2022

omegabytes commented Aug 11, 2022

GGabriele commented Aug 17, 2022

hbagdi commented Aug 18, 2022

GGabriele commented Aug 19, 2022

tjasko left a comment

tjasko Aug 19, 2022

GGabriele Aug 19, 2022

tjasko Aug 19, 2022

hbagdi left a comment

mikefero commented Aug 23, 2022

	} else {
	vc.logger.With(zap.String("entity", entityName)).
	With(zap.String("field", update.Field)).
	With(zap.String("condition", update.Condition)).
	With(zap.Any("new-value", fieldUpdate.Value)).
	With(zap.String("data-plane", dataPlaneVersion)).
	With(zap.Error(err)).
	Error("entity configuration does not contain field value")
	break

feat: expand upstreams model and validation for 3.0 #311

feat: expand upstreams model and validation for 3.0 #311

Conversation

GGabriele commented Jul 22, 2022 • edited Loading

tjasko left a comment

Choose a reason for hiding this comment

hbagdi commented Jul 25, 2022

hbagdi left a comment

Choose a reason for hiding this comment

hbagdi commented Jul 28, 2022

hbagdi commented Aug 1, 2022

GGabriele commented Aug 2, 2022

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

hbagdi commented Aug 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omegabytes Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbagdi commented Aug 10, 2022

omegabytes commented Aug 11, 2022

GGabriele commented Aug 17, 2022

hbagdi commented Aug 18, 2022

GGabriele commented Aug 19, 2022

tjasko 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

hbagdi left a comment

Choose a reason for hiding this comment

mikefero commented Aug 23, 2022

GGabriele commented Jul 22, 2022 •

edited

Loading

omegabytes Aug 11, 2022 •

edited

Loading