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.

Sign up for GitHub

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

Jump to bottom

refactor(meta) add meta._SERVER_TOKENS #3511

Merged
merged 2 commits into from
Jun 14, 2018
Merged

refactor(meta) add meta._SERVER_TOKENS #3511

merged 2 commits into from
Jun 14, 2018

Conversation

bungle
Copy link
Member

@bungle bungle commented Jun 5, 2018

Summary

Add kong.meta._SERVER_TOKENS and change codebase to use that everywhere.

thibaultcha
thibaultcha previously requested changes Jun 5, 2018
View reviewed changes
Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

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

It seems like we should be merging this first, before the delayed response fixes. Then, delayed response PRs can each have two commits:

  1. Handle delayed responses
  2. Add server tokens + tests if necessary

kong/plugins/aws-lambda/handler.lua Outdated
@@ -150,6 +153,10 @@ function AWSLambdaHandler:access(conf)
ngx.status = res.status
end

if singletons.configuration.enabled_headers[constants.HEADERS.VIA] then
ngx.header["Via"] = meta._SERVER_TOKENS
Copy link
Member

Choose a reason for hiding this comment

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

We should also test this (or make it part of a separate commit)

kong/tools/responses.lua Outdated

else
ngx.header["Server"] = server_header
end
Copy link
Member

Choose a reason for hiding this comment

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

Ditto: probably belong to a separate commit (i.e. in the PR already opened for delayed response)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you wanted to comment about request-termination plugin changes here? I will make separate PR for the two plugins.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @thibaultcha really meant a separate commit here, because the rest of the PR is all about internal reorganization of the meta._SERVER_TOKENS but this bit is something else, as it changes the behavior of the Server header sent (and would probably merit a separate test checking the header behavior).

@bungle bungle force-pushed the feat/meta-server-tokens branch 2 times, most recently from 0d907ca to e9f49d7 Compare June 14, 2018 17:13
hishamhm
hishamhm reviewed Jun 14, 2018
View reviewed changes
spec/01-unit/009-responses_spec.lua Outdated
@@ -192,4 +192,33 @@ describe("Response helpers", function()
assert.spy(s).was.called_with(ngx.ctx)
end)
end)

describe("server tokens", function()
it("are send by default", function()
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: are sent by default (likewise below)

@hishamhm
Copy link
Contributor

I guess the first commit should be refactor instead of feat since this is an internal code reorganization without visible functionality change

@bungle bungle changed the title feat(meta) add meta._SERVER_TOKENS refactor(meta) add meta._SERVER_TOKENS Jun 14, 2018
@bungle
Copy link
Member Author

bungle commented Jun 14, 2018

feat was changed to refactor

@hishamhm hishamhm merged commit 2089ec3 into next Jun 14, 2018
@thibaultcha thibaultcha deleted the feat/meta-server-tokens branch June 14, 2018 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hishamhm hishamhm hishamhm approved these changes

@thibaultcha thibaultcha thibaultcha left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bungle @hishamhm @thibaultcha