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: add option debug to conf headers to disable kong_debug header function #10054

Merged
merged 10 commits into from
Jan 17, 2023

Conversation

oowl
Copy link
Member

@oowl oowl commented Jan 4, 2023

Summary

add option debug to conf headers to disable kong_debug header function

Checklist

Full changelog

  • introduce option debug to conf headers to disable kong_debug header function

Issue reference

FTI-4521

kong/conf_loader/init.lua Outdated Show resolved Hide resolved
@chronolaw
Copy link
Contributor

Should we add a change log entry for this new config item?

@oowl oowl added this to the 3.2 milestone Jan 5, 2023
kong.conf.default Outdated Show resolved Hide resolved
@chronolaw
Copy link
Contributor

I think we need a change log entry for it.

@bungle
Copy link
Member

bungle commented Jan 10, 2023

I suggest we add debug option here:
https://github.com/Kong/kong/blob/master/kong.conf.default#L796

And not enable it by default, rather than introduce a new configuration option (perhaps even then only send these when requested with KONG_DEBUG request header).

Copy link
Member

@kikito kikito left a comment

Choose a reason for hiding this comment

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

I suggest we add debug option here: [...]

I agree with @bungle on this one. We already have too many config options. I'd rather modify headers to have a debug option.

@oowl oowl changed the title feat: add enable_debug_header conf to disable kong_debug header function feat: add option debug to conf headers to disable kong_debug header function Jan 11, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
kong/router/utils.lua Outdated Show resolved Hide resolved
kong.conf.default Outdated Show resolved Hide resolved
@hbagdi
Copy link
Member

hbagdi commented Jan 13, 2023

I'm sympathetic to our situation of too many configuration options and I'm generally fully onboard with situations around reusing configuration properties. In this specific case, I'm -1 and recommend adding a new configuration field instead.

Here is my rationale:

  1. While debugging, assume that the user doesn't know the default value of a configuration and wants to solve their problem with the least amount of friction. Assuming that, the user behavior will be to set "KONG_HEADERS=debug". And that will work, but suddenly you won't have other headers that you were probably relying on to debug your problem. The user is either going to be left puzzled or will successfully navigate their way around documentation to find that the property that they said needs two other values as well. This may seem like a minor inconvenience but details matter. Especially, when the user is frustrated.
  2. Before this patch, 'headers' option controls headers that provide additional insight into request/response flows and provide some observability data. This configuration option doesn't modify the behavior of Kong based on input at proxy time. The debug header on the other hand only conditionally adds additional context to request/response headers. There is a small semantic difference between the two.

What do you all think?

@kikito
Copy link
Member

kikito commented Jan 13, 2023

I see both points that @hbagdi is saying.

It will be very easy to miss that instead of KONG_DEBUG=true you have to write KONG_HEADERS=server_tokens, latency_tokens,debug, which is error-prone and easy to forget.

I missed that this particular option will only act when the consumer request has the kong-debug=1 header set, while the others act in all requests (regardless of their headers). That makes it slightly different.

These two things separately would not change my opinion, but together they made me change my mind. Thank you, Harry.

@attenuation could you please restore this PR to its previous state, using a single global variable for debugging?. Apologies for the back-and-forth. (I would recommend keeping a local copy of this PR as it is - with headers just in case)

@oowl
Copy link
Member Author

oowl commented Jan 14, 2023

@kikito Hi Enrique, I have restored the code base to the old version, please help me review it

@oowl oowl force-pushed the feat/disable-debug-header branch 2 times, most recently from e352740 to 05635e1 Compare January 16, 2023 10:01
Copy link
Member

@kikito kikito left a comment

Choose a reason for hiding this comment

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

I'm preapproving this in case we are adding enable_debug_header because we cannot use debug_header directly. (Perhaps KONG_DEBUG_HEADER is already used for something else).

If we can rename it without issues, please rename it before merging.

If we must use something other than debug_header for some reason, then please merge as-is.

kong.conf.default Outdated Show resolved Hide resolved
@oowl oowl force-pushed the feat/disable-debug-header branch 2 times, most recently from a0e472d to cdec7cb Compare January 17, 2023 03:39
@fffonion fffonion merged this pull request into master Jan 17, 2023
@fffonion fffonion deleted the feat/disable-debug-header branch January 17, 2023 06:53
dndx pushed a commit that referenced this pull request Jan 17, 2023
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.

7 participants