Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

Extend support for host headers with configuring routes #2369

Closed
ksubrmnn opened this issue Jan 25, 2021 · 7 comments
Closed

Extend support for host headers with configuring routes #2369

ksubrmnn opened this issue Jan 25, 2021 · 7 comments
Labels
kind/enhancement size/M 7 days (~1.5 week)

Comments

@ksubrmnn
Copy link
Contributor

ksubrmnn commented Jan 25, 2021 •
edited by draychev
Loading

SMI Traffic Spec allows us to specify headers and Host is one of these.

We use the kubernetes host names when building the envoy configs. This means we disregard any host header specified in the traffic spec ( for both a traffic target and traffic split)

We need to extend out the route configuration to provide support for it
@ksubrmnn ksubrmnn added the area/documentation work items associated with the improvements or additions to documentation label Jan 25, 2021
@shashankram
Copy link
Member

Can this be addressed with the routes refactor?
/cc @michelleN

@snehachhabria
Copy link
Contributor

As of today we are ignoring host headers :

osm/pkg/envoy/route/config.go

Line 152 in b25fd46

if headerKey == httpHostHeader {

We will have to ensure that it's included in the headers and domains while configuring routes. Since we want this support, changing it from documentation to enhancement required

@snehachhabria snehachhabria changed the title Document that host headers will not be used in data plane Extend support for host headers with configuring routes Jan 25, 2021
@snehachhabria snehachhabria added kind/enhancement and removed area/documentation work items associated with the improvements or additions to documentation labels Jan 25, 2021
@draychev draychev added the size/L 14 days (~2.5 weeks) label Jan 26, 2021
@michelleN michelleN added this to the v0.8.0 milestone Jan 29, 2021
@michelleN michelleN added priority/P2 P2 priority size/M 7 days (~1.5 week) and removed size/L 14 days (~2.5 weeks) labels Feb 11, 2021
@draychev draychev self-assigned this Feb 23, 2021
@draychev
Copy link
Contributor

draychev commented Feb 23, 2021 •
edited
Loading

This was not implemented in routes v1 and not yet ready in routes v2.
Let's push this to v0.9 or 0.8.1 cc: @phillipgibson @michelleN

@michelleN michelleN mentioned this issue Feb 24, 2021
Closed
@michelleN
Copy link
Contributor

sounds good. added to #2611

@draychev draychev removed their assignment Mar 1, 2021
@draychev draychev modified the milestones: v0.8.0, v0.9.0 Mar 1, 2021
@shashankram shashankram mentioned this issue Mar 9, 2021
Closed
@snehachhabria snehachhabria self-assigned this Apr 6, 2021
@snehachhabria
Copy link
Contributor

snehachhabria commented Apr 10, 2021 •
edited
Loading

Once we support configuring routes with host headers, users will still not be able to make requests to the hosts. As there's limitation in OSM to only supports service fqdn's on its filter chains.

snehachhabria added a commit to snehachhabria/osm that referenced this issue Apr 12, 2021
@snehachhabria
updating merge logic for inbound and outbound traffic policies
cc7e9a7 
This commit allows partial hostname matches while merging both inbound
and outbound traffic policies. This change is necessary to support host
headers that users will specify as a part of the TrafficSpec and as this
will be a single host, is can only partially match to an existing
traffic policy and if that is the case the two policies should be
merged.

The commit also imporves the logic of the 'subset` function in this
package. It checks is either slice is a subset of the other and if true,
returns a unified slice of the two.

This is part of openservicemesh#2369

Signed-off-by: Sneha Chhabria <snchh@microsoft.com>
snehachhabria added a commit to snehachhabria/osm that referenced this issue Apr 12, 2021
@snehachhabria
updating merge logic for inbound and outbound traffic policies
ebf4312 
This commit allows partial hostname matches while merging both inbound
and outbound traffic policies. This change is necessary to support host
headers that users will specify as a part of the TrafficSpec and as this
will be a single host, is can only partially match to an existing
traffic policy and if that is the case the two policies should be
merged.

The commit also imporves the logic of the 'subset` function in this
package. It checks is either slice is a subset of the other and if true,
returns a unified slice of the two.

This is part of openservicemesh#2369

Signed-off-by: Sneha Chhabria <snchh@microsoft.com>
@snehachhabria snehachhabria mentioned this issue Apr 12, 2021
Merged
snehachhabria added a commit to snehachhabria/osm that referenced this issue Apr 12, 2021
@snehachhabria
updating merge logic for inbound and outbound traffic policies
c70bb19 
This commit allows partial hostname matches while merging both inbound
and outbound traffic policies. This change is necessary to support host
headers that users will specify as a part of the TrafficSpec and as this
will be a single host, is can only partially match to an existing
traffic policy and if that is the case the two policies should be
merged.

The commit also imporves the logic of the 'subset` function in this
package. It checks is either slice is a subset of the other and if true,
returns a unified slice of the two.

This is part of openservicemesh#2369

Signed-off-by: Sneha Chhabria <snchh@microsoft.com>
@snehachhabria snehachhabria added the kind/blocked The issue is blocked and no further progress can be made. label Apr 13, 2021
snehachhabria added a commit to snehachhabria/osm that referenced this issue Apr 13, 2021
@snehachhabria
configure inbound and outbound routes with host headers
80e511c 
This commit adds new inbound and outbound traffic policies if host
header is provided in the route.

Part of  openservicemesh#2369

Signed-off-by: Sneha Chhabria <snchh@microsoft.com>
@snehachhabria snehachhabria mentioned this issue Apr 13, 2021
Merged
snehachhabria added a commit to snehachhabria/osm that referenced this issue Apr 13, 2021
@snehachhabria
configure inbound and outbound routes with host headers
41997e1 
This commit adds new inbound and outbound traffic policies if host
header is provided in the route.

Part of  openservicemesh#2369

Signed-off-by: Sneha Chhabria <snchh@microsoft.com>
snehachhabria added a commit to snehachhabria/osm that referenced this issue Apr 19, 2021
@snehachhabria
configure inbound and outbound routes with host headers
99e7fcc 
This commit adds new inbound and outbound traffic policies if host
header is provided in the route.

Part of  openservicemesh#2369

Signed-off-by: Sneha Chhabria <snchh@microsoft.com>
snehachhabria added a commit to snehachhabria/osm that referenced this issue Apr 20, 2021
@snehachhabria
configure inbound and outbound routes with host headers
48662e0 
This commit adds new inbound and outbound traffic policies if host
header is provided in the route.

Part of  openservicemesh#2369

Signed-off-by: Sneha Chhabria <snchh@microsoft.com>
snehachhabria added a commit to snehachhabria/osm that referenced this issue Apr 26, 2021
@snehachhabria
configure inbound and outbound routes with host headers
c3f3308 
This commit adds new inbound and outbound traffic policies if host
header is provided in the route.

Part of  openservicemesh#2369

Signed-off-by: Sneha Chhabria <snchh@microsoft.com>
snehachhabria added a commit to snehachhabria/osm that referenced this issue Apr 26, 2021
@snehachhabria
configure inbound and outbound routes with host headers
b195d94 
This commit adds new inbound and outbound traffic policies if host
header is provided in the route.

Part of  openservicemesh#2369

Signed-off-by: Sneha Chhabria <snchh@microsoft.com>
snehachhabria added a commit to snehachhabria/osm that referenced this issue Apr 26, 2021
@snehachhabria
configure inbound and outbound routes for policies with host headers
d9cb374 
This commit adds new inbound and outbound traffic policies if host
header is provided in the route.

Part of  openservicemesh#2369

Signed-off-by: Sneha Chhabria <snchh@microsoft.com>
snehachhabria added a commit to snehachhabria/osm that referenced this issue Apr 27, 2021
@snehachhabria
configure inbound and outbound routes for policies with host headers
2e78a44 
This commit adds new inbound and outbound traffic policies if host
header is provided in the route.

Part of  openservicemesh#2369

Signed-off-by: Sneha Chhabria <snchh@microsoft.com>
@snehachhabria snehachhabria mentioned this issue Apr 27, 2021
Closed
snehachhabria added a commit to snehachhabria/osm that referenced this issue Apr 27, 2021
@snehachhabria
configure inbound and outbound routes for policies with host headers
619a7f3 
This commit adds new inbound and outbound traffic policies if host
header is provided in the route.

Part of  openservicemesh#2369

Signed-off-by: Sneha Chhabria <snchh@microsoft.com>
snehachhabria added a commit to snehachhabria/osm that referenced this issue Apr 27, 2021
@snehachhabria
configure inbound and outbound routes for policies with host headers
829073c 
This commit adds new inbound and outbound traffic policies if host
header is provided in the route and updated the hostRewriteSepecifier
for the envoy.

Part of  openservicemesh#2369

Signed-off-by: Sneha Chhabria <snchh@microsoft.com>
OSM-PR-bot pushed a commit to snehachhabria/osm that referenced this issue Apr 27, 2021
@snehachhabria
configure inbound and outbound routes for policies with host headers
06669f5 
This commit adds new inbound and outbound traffic policies if host
header is provided in the route and updated the hostRewriteSepecifier
for the envoy.

Part of  openservicemesh#2369

Signed-off-by: Sneha Chhabria <snchh@microsoft.com>
@shashankram
Copy link
Member

I wanted to call out that specifying a custom host header in SMI's traffic specs doesn't serve any purpose at the moment. Per the current implementation, a host header derived from an SMI HTTPRouteGroup resource is programmed as a header match in a route's match criteria (route.match.headers[]).

Such a route needs to first match the virtual_host.domains in the route configuration, where matching is already performed based on the HTTP :authority (same as Host) header. With the existing implementation, a route with a header match based on the host header derived from an SMI HTTPRouteGroup resource will be programmed, though it will never be used because traffic with that host header will not match the top-level virtual_host the route belongs to.

The only use case to specify a host header match in an envoy route is when using a wildcard (*) virtual_host, to be able to differentiate routes with different host headers part of the wildcard virtual_host.

@snehachhabria snehachhabria mentioned this issue May 4, 2021
Merged
@snehachhabria snehachhabria added needs discussion and removed kind/blocked The issue is blocked and no further progress can be made. labels May 4, 2021
@snehachhabria snehachhabria removed the priority/P2 P2 priority label May 4, 2021
@snehachhabria snehachhabria removed this from the v0.9.0 milestone May 18, 2021
shashankram added a commit to shashankram/osm-docs that referenced this issue Jun 2, 2021
@shashankram
manual_demo: fix HTTP route policy
94f6bef 
The existing demo is broken because of incorrect
host header matching specified which prevents
bookbuyer from accessing the bookstore service.
The host header in the HTTP request includes
the port, but the spec was missing this. Since
the host header feature is not complete (see
openservicemesh/osm#2369) and this is unnecessary
for this demo, this change removes it.

The demo works as expected with SMI policies
with this fix.

Resolves openservicemesh/osm#3491

Signed-off-by: Shashank Ram <shashr2204@gmail.com>
shashankram added a commit to shashankram/osm-docs that referenced this issue Jun 2, 2021
@shashankram
manual_demo: fix HTTP route policy
51b2223 
The existing demo is broken because of incorrect
host header matching specified which prevents
bookbuyer from accessing the bookstore service.
The host header in the HTTP request includes
the port, but the spec was missing this. Since
the host header feature is not complete (see
openservicemesh/osm#2369) and this is unnecessary
for this demo, this change removes it.

The demo works as expected with SMI policies
with this fix.

Resolves openservicemesh/osm#3491

Signed-off-by: Shashank Ram <shashr2204@gmail.com>
@shashankram shashankram mentioned this issue Jun 2, 2021
Merged
@shashankram shashankram mentioned this issue Jun 22, 2021
Merged
@snehachhabria snehachhabria removed their assignment Sep 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
kind/enhancement size/M 7 days (~1.5 week)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@michelleN @shashankram @ksubrmnn @draychev @snehachhabria