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(upstream) add host_header to upstream entities #4959

Merged
merged 1 commit into from Oct 2, 2019

Conversation

locao
Copy link
Contributor

@locao locao commented Aug 23, 2019

Add support to hostname attribute in upstream entities.

This attribute is used as the Host header when proxying requests through Kong. It takes precedence over the Host that clients send to Kong.

@hbagdi
Copy link
Member

hbagdi commented Aug 23, 2019

Question: Will active HTTP health-check set the host header to this new property (if present) as well?

@locao locao force-pushed the feat/upstream_hostname branch 2 times, most recently from 66774d9 to 5fbf7d6 Compare August 23, 2019 20:54
@locao
Copy link
Contributor Author

locao commented Aug 23, 2019

Will active HTTP health-check set the host header to this new property (if present) as well?

No, it won't. I will change this behavior, thanks for pointing that, @hbagdi!

@Tieske
Copy link
Member

Tieske commented Aug 24, 2019

This change doesn't make sense to me.

afaik we have 2 possible values for the upstream host-header; the incoming host-header, or the hostname of the upstream server.

This subject is complicated because we do not cleanly separate teh construction of the upstream request in our flow. As mentioned elsewhere; we need to collect info in pre-access phase and only build teh upstream properties in post-access.

Adding this extra property only makes thing more complicated imo.

In pseudo code:

-- pre-access
host_header_incoming = request.headers["Host"]
host_header_upstream = nil
host_header_preserve = route.preserve_hostname

-- pdk
function set_upstream_host_header(name)
  host_header_upstream = name
end
function set_host_preserve(bool)
  host_header_preserve = bool
end

--post-access
ip, port, resolved_name = dns.resolve()  -- includes balancers, returns hostname resolved, or target name if balancer

host_header_upstream = host_header_upstream or
                       (host_header_preserve and host_header_incoming) or
                       resolved_name

Does this make sense?

@locao locao force-pushed the feat/upstream_hostname branch 3 times, most recently from 0cff500 to 819b1a0 Compare August 30, 2019 15:53
kong/runloop/balancer.lua Outdated Show resolved Hide resolved
@locao locao force-pushed the feat/upstream_hostname branch 3 times, most recently from af43908 to 9200e81 Compare September 30, 2019 07:33
Copy link
Contributor

@hishamhm hishamhm left a comment

Choose a reason for hiding this comment

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

Nice, getting there! Left a couple of suggestions.

-- configure healthchecks
begin_testcase_setup(strategy, bp)
local upstream_name, upstream_id = add_upstream(bp, {
hostname = "localhost",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this a value other than localhost, some invalid name like upstream-hostname.test to make sure this test does not use the underlying system's name resolution in any way?

kong/runloop/balancer.lua Outdated Show resolved Hide resolved
@locao locao force-pushed the feat/upstream_hostname branch 3 times, most recently from ced94ad to d67109b Compare October 1, 2019 21:14
@locao locao changed the title feat(upstream) add hostname to upstream entities feat(upstream) add host_header to upstream entities Oct 1, 2019
@hishamhm hishamhm merged commit da34f0a into next Oct 2, 2019
@hishamhm hishamhm deleted the feat/upstream_hostname branch October 2, 2019 00:37
kikito pushed a commit that referenced this pull request Oct 22, 2019
This attribute is used as the Host header when proxying requests through Kong. It takes precedence over the Host that clients send to Kong.
dndx pushed a commit that referenced this pull request Oct 22, 2019
This attribute is used as the Host header when proxying requests through Kong. It takes precedence over the Host that clients send to Kong.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants