feat: zipkin traceid response header #496

omegabytes · 2022-10-28T23:33:57Z

Adds http_response_header_for_traceidfor zipkin.

mikefero · 2022-10-29T09:52:05Z

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

@@ -75,6 +75,7 @@ var (
 		"connect_timeout",
 		"send_timeout",
 		"read_timeout",
+		"http_response_header_for_traceid",


This is for 3.1 not 3.0; it will need to be defined in its own 3.1 version compatibility instance.

mikefero · 2022-10-29T09:55:39Z

internal/test/e2e/plugins.go

@@ -405,7 +405,8 @@ var VersionCompatibilityOSSPluginConfigurationTests = []VersionCompatibilityPlug
 	//
 	// DP < 3.0:
 	//   - remove 'http_span_name', 'connect_timeout'
-	//     'send_timeout', 'read_timeout'
+	//     'send_timeout', 'read_timeout',
+	//     'http_response_header_for_traceid'


Same call out here for the test. We should also create a test to ensure that this field (and eventually others) are going down to a 3.1 data plane which is currently kong/kong:master-alpine executed during the integration GitHub Action.

We should also create a test to ensure that this field (and eventually others) are going down to a 3.1 data plane

What does this mean? I provide an image tag when testing locally so I am hitting 3.1. Are you saying I also need to make a change to the CICD configuration? Or do I need to add something to the test suite like a unit test that specifically checks for an expected post-processing configuration for zipkin for DPs < 3.1.

kong/kong:master-alpine is versioned 3.1.0, so the CI is already testing the current status of the 3.1 release. We could probably add a test similar to where we use versions300AndAbove and make sure new fields are not altered by the compatibility stuff (so going down to a 3.1+ DP).

@mikefero feel free to add more details in case I misunderstood this.

Yep, that is exactly what I was referring to. Since this is for a new version of the Kong Gateway we want to ensure that this new field is being sent in the payload to these newer data planes. Validating these new releases is critical.

I've changed the logic to require the field for versions 3.1^ instead of removing the field for versions pre-3.1 7f84e48.
In the absence of documentation on the field, I'm assuming we always want to set the value to X-B3-TraceId based on the preceding work..

The reason I went with "remove for versions below 3.1" instead of "ensure presence for 3.1 and above" is because this field is not required. Also, this probably should be a warning instead of an error, no?

@omegabytes , sorry, but I think there are some misunderstandings with the latest changes. Let me try to clarify:

The value of http_response_header_for_traceid can be set to anything the user want. This line you pointed is only based on a test value, which is a simple plugin configuration.

There is no absence of documentation, it is just not done yet. The gateway documentation is often done only 1-2 weeks before GA, depending on the workload.

We need a ChangeMetadata + test to ensure that if a user configure the zipkin plugin with that field, it is removed for older DPs. At the moment, this is probably crashing (errors at config push in DP logs). This was OK in your previous version IIRC.

We want a test to ensure that if a user configure the zipkin plugin with that field, the plugin configuration is not altered by the version compatibility translator. It should not with your previous version, so it is just a sanity check that new fields of the schema for a version are correctly going down for the new 3.1 release. So that is exactly this (if the plugin in test was configured with that field value)

omegabytes · 2022-10-31T23:20:31Z

Integration Tests [OSS] / integration (kong/kong:master-alpine, false) (pull_request) failing is a known issue

omegabytes · 2022-11-01T19:44:59Z

internal/test/e2e/version_compatibility_test.go

@@ -211,7 +211,7 @@ func TestVersionCompatibility_PluginFieldUpdates(t *testing.T) {
 	for _, plugin := range expectedPluginsMap {
 		var config structpb.Struct
 		if len(plugin.Config) > 0 {
-			require.NoError(t, json.ProtoJSONUnmarshal([]byte(plugin.Config), &config))
+			require.NoError(t, json.ProtoJSONUnmarshal([]byte(plugin.Config), &config), plugin)


Adding this to the test fail message output helps isolate where an issue is. For example, I had a trailing comma, so the fail output was:

Error: Received unexpected error: invalid character '}' looking for beginning of object key string Test: TestVersionCompatibility_PluginFieldUpdates Messages: {Name:zipkin Config:{ "local_service_name": "LOCAL_SERVICE_NAME", "header_type": "ignore", "http_span_name": "method_path", "connect_timeout": 2001, "send_timeout": 2001, "read_timeout": 2001, } Protocols:[] VersionRange: FieldUpdateChecks:map[< 2.7.0:[{Field:header_type Value:preserve}] >= 3.1.0:[{Field:http_response_header_for_traceid Value:X-B3-TraceId}]] ConfigureForService:true ConfigureForRoute:true ExpectedConfig:}

GGabriele · 2022-11-02T07:33:58Z

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

+					"in Kong Gateway versions >= 3.1. " +
+					"The plugin configuration has been updated to use " +
+					"'config.http_response_header_for_traceid' in the data-plane.",
+				Resolution: "Please update the configuration to use " +


I'm confused. Shouldn't we make sure that a new field is removed from payloads destined to older DPs? This rule is making sure that http_response_header_for_traceid is set to X-B3-TraceId for newer DPs, but I think this should actually make sure to remove it for pre-3.1 DPs, basically a copy&paste of this rule for the specific new config field.

I originally had a removal check (7f84e48) but I confused myself and removed it. I might add it back in after I go through Alan's comment: https://github.com//pull/496#discussion_r1010860736

GGabriele · 2022-11-04T08:11:51Z

internal/test/e2e/plugins.go

@@ -423,6 +426,12 @@ var VersionCompatibilityOSSPluginConfigurationTests = []VersionCompatibilityPlug
 					Value: "preserve",
 				},
 			},
+			">= 3.1.0": {


Technically, this is not an update: the version compatibility rule is to remove this http_response_header_for_traceid field from older DPs.

Adding things like this here to verify newer fields are present for newer DPs is not something we have been doing (for example, note that this is not checking that read_timeout or send_timeout are present for >=3.0.0.

I think we should remove this, and maybe add a new test making sure new fields are present for 3.1.0.

Thoughts?

+1, good point. Let's test this case only when it's a 3.1.0 DP. That way, we will also avoid modifying it if anything change in 3.2.0+. @omegabytes Note that we also want a test to check if that field is removed for <3.1 DPs.

ah nvm, I see it here: https://github.com/Kong/koko/pull/496/files#diff-de20d2d66636a4ebbfeaf31f3cd4f7ef469b1fc14c7d2ab47a0e69efb4ba5191R420 👍

1869fc3 asserts a valid config for 310 or newer. Includes some new fields as of 300 as well.

omegabytes · 2022-11-04T18:28:17Z

kong/kong:master-alpine image is several weeks out of date. This causes the validation failure for the field we're adding in this branch. I've validated my changes locally against a kong image I've confirmed is up-to-date and contains the field changes we're adding here.

omegabytes added the work in progress PR is a work in progress label Oct 28, 2022

mikefero reviewed Oct 29, 2022

View reviewed changes

omegabytes marked this pull request as ready for review October 31, 2022 23:19

omegabytes requested a review from a team as a code owner October 31, 2022 23:19

omegabytes commented Nov 1, 2022

View reviewed changes

omegabytes requested review from mikefero and aboudreault November 1, 2022 19:46

GGabriele reviewed Nov 2, 2022

View reviewed changes

omegabytes added 4 commits November 3, 2022 15:34

feat: zipkin traceid response header

9d3396c

move change to 3.1

b78eb94

fieldUpdateCheck for v3.1^

aaf5a5a

remove field pre-3.1

bab9dbc

omegabytes force-pushed the feat/zipkin-traceid-header branch from 0d3d1d8 to bab9dbc Compare November 3, 2022 22:43

GGabriele reviewed Nov 4, 2022

View reviewed changes

validate field config 310 or newer

1869fc3

omegabytes removed the work in progress PR is a work in progress label Nov 4, 2022

omegabytes requested a review from GGabriele November 4, 2022 18:28

GGabriele approved these changes Nov 4, 2022

View reviewed changes

tjasko approved these changes Nov 4, 2022

View reviewed changes

aboudreault approved these changes Nov 5, 2022

View reviewed changes

omegabytes merged commit fd32be8 into main Nov 7, 2022

omegabytes deleted the feat/zipkin-traceid-header branch November 7, 2022 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: zipkin traceid response header #496

feat: zipkin traceid response header #496

omegabytes commented Oct 28, 2022

mikefero Oct 29, 2022

mikefero Oct 29, 2022

omegabytes Oct 31, 2022 •

edited

Loading

aboudreault Nov 1, 2022

mikefero Nov 1, 2022

omegabytes Nov 1, 2022

aboudreault Nov 1, 2022 •

edited

Loading

omegabytes commented Oct 31, 2022

omegabytes Nov 1, 2022 •

edited

Loading

GGabriele Nov 2, 2022

GGabriele Nov 2, 2022

omegabytes Nov 2, 2022 •

edited

Loading

GGabriele Nov 4, 2022 •

edited

Loading

aboudreault Nov 4, 2022 •

edited

Loading

aboudreault Nov 4, 2022

omegabytes Nov 4, 2022

omegabytes commented Nov 4, 2022

feat: zipkin traceid response header #496

feat: zipkin traceid response header #496

Conversation

omegabytes commented Oct 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omegabytes Oct 31, 2022 • edited Loading

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

aboudreault Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

omegabytes commented Oct 31, 2022

omegabytes Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omegabytes Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

GGabriele Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

aboudreault Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omegabytes commented Nov 4, 2022

omegabytes Oct 31, 2022 •

edited

Loading

aboudreault Nov 1, 2022 •

edited

Loading

omegabytes Nov 1, 2022 •

edited

Loading

omegabytes Nov 2, 2022 •

edited

Loading

GGabriele Nov 4, 2022 •

edited

Loading

aboudreault Nov 4, 2022 •

edited

Loading