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(http-log) add custom header for authorization #6449

Merged
merged 2 commits into from Nov 23, 2020
Merged

Conversation

Tieske
Copy link
Member

@Tieske Tieske commented Oct 8, 2020

Adds an option to provide a custom header, and value to add to the log request. Typically required for Splunk for example.

@Tieske Tieske changed the base branch from master to next October 8, 2020 09:13
@Tieske Tieske force-pushed the httplog-header branch 2 times, most recently from 2b988d3 to 8158e57 Compare October 8, 2020 09:17
@Tieske Tieske force-pushed the httplog-header branch 2 times, most recently from 8e7c791 to 97e752a Compare October 21, 2020 11:28
@bungle
Copy link
Member

bungle commented Nov 19, 2020

Should it allow plural? Aka custom headers?

@dndx
Copy link
Member

dndx commented Nov 19, 2020

@bungle I asked the same question in #6449 (comment). I guess there is little use case for people adding two statistically defined headers, so I tends to agree this current implementation should satisfies more than 90% of the use cases out there.

In particular, for authenticating against a log collection API, one set of header is usually enough. Whether it is Authorization or API-Key or something similar.

If we support templating for header values then supporting more than one custom header may be more useful.

@Tieske
Copy link
Member Author

Tieske commented Nov 19, 2020

@dndx @bungle if using this; https://github.com/Kong/kong/blob/next/kong/db/schema/typedefs.lua#L509

do you have an example of where/how it is to be used?

@Tieske
Copy link
Member Author

Tieske commented Nov 19, 2020

had a look at it, that is not suitable. That headers typedef only checks for "Host", and doesn't allow it.
To do the same here, we need to validate the other headers as well, see

["Content-Type"] = content_type,
["Content-Length"] = #payload,
["Authorization"] = parsed_url.userinfo and (

That is really overkill for such a simple feature.

@Tieske
Copy link
Member Author

Tieske commented Nov 20, 2020

  • the headers one is not suited, it contains an error message specific to the route entity
  • the keys in the headers typedef are just strings, not valid header names. currently there is 1 header name in this PR, but it is properly validated
  • if checking for Host, we also must check for the others, so that would require a complete new typedef anyway.
  • If we check for Authorization there must be a check that it does not collide with the userinfo in the URL provided, which I think requires a custom validator function, which we don't like.

I really think this is overcomplicating it.

@Tieske Tieske force-pushed the httplog-header branch 7 times, most recently from 4c467a3 to 818bfe7 Compare November 22, 2020 13:05
the typedef for `headers` was custom to the `route` entity, now
separated out properly
@Tieske Tieske force-pushed the httplog-header branch 2 times, most recently from 32e301d to 32c7cfb Compare November 22, 2020 14:07
@Tieske
Copy link
Member Author

Tieske commented Nov 22, 2020

@bungle @dndx fixed up to support multiple headers now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants