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.

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

feat: introduce konghq.com/headers-separator annotation and make parsing more robust #5977

Merged
merged 1 commit into from
May 7, 2024

Conversation

programmer04
Copy link
Member

@programmer04 programmer04 commented May 7, 2024

What this PR does / why we need it:

This PR resolves real-world problems reported by the user that konghq.com/headers.* doesn't allow to specify headers that contain , which is permitted in header values, but can't be used because KIC uses it as a separator.

HTTP specification is very permissive in terms of what is allowed as header value
https://datatracker.ietf.org/doc/html/rfc9110#name-field-values

Field values are usually constrained to the range of US-ASCII characters [USASCII].

see also Cloudflare docs.

Hence coming up with an escaping pattern for commas is hard and won't be easy to use/interpret by a user. So this PR introduces an additional annotation konghq.com/headers-separator that allows specifying a different separator than the default ,.

Moreover, this PR makes parsing header values from konghq.com/headers.* more robust - now it discards leading and trailing whitespace characters so the below annotations are equivalent

konghq.com/headers.foo: foo,bar,baz
konghq.com/headers.foo: foo, bar   , baz

this is a safe change, because according to https://datatracker.ietf.org/doc/html/rfc9110#section-5.5-3

A field value does not include leading or trailing whitespace. When a specific version of HTTP allows such whitespace to appear in a message, a field parsing implementation MUST exclude such whitespace before evaluating the field value.

that will make users' lives easier.

Furthermore, a test case for handling a header with an empty value has been added because it's a valid use case, see https://datatracker.ietf.org/doc/html/rfc9110#section-12.5.3-12

An Accept-Encoding header field with a field value that is empty implies that the user agent does not want any content coding in response.

Which issue this PR fixes:

Closes #5427

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

@programmer04 programmer04 self-assigned this May 7, 2024
@programmer04 programmer04 added this to the KIC v3.2.x milestone May 7, 2024
@programmer04 programmer04 marked this pull request as ready for review May 7, 2024 11:58
@programmer04 programmer04 requested a review from a team as a code owner May 7, 2024 11:58
@programmer04 programmer04 enabled auto-merge (squash) May 7, 2024 12:01
Copy link
Contributor

@czeslavo czeslavo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@programmer04 programmer04 merged commit fe717a3 into main May 7, 2024
37 checks passed
@programmer04 programmer04 deleted the header-separator branch May 7, 2024 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KIC annotation konghq.com/headers.* does not allow header values that contain commas ,
2 participants