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

Remove Host matching for RouteRules #1116

Closed
wants to merge 2 commits into from
Closed

Remove Host matching for RouteRules #1116

wants to merge 2 commits into from

Conversation

scothis
Copy link
Contributor

@scothis scothis commented Jun 8, 2018

Inside the cluster it's common to make a request to a service using
a service's short name. For a service name foobar, you can make a
request for http://foobar/. Istio RouteRules effectively intercept
traffic destined for one Service and route it to another Service.
Within the RouteRule, a filter can be applied based on the HTTP Host
header. If the header is not matched, the rule is not applied.

A Knative Route sends traffic destined for a single Service (fronted by
an Ingress) to another Service backed by a Revision. The Ingress already
applies Host based routing, the RouteRule also applying Host based
routing is at best redundant.

With the new Eventing Channel work, we'd like to be able to send
requests from inside the cluster to a Route, utilizing the RouteRule.
Currently, these requests are dropped because the request inside the
cluster is made to the service hostname and not the external fqdn.

Unless there is a reason to keep the redundant Host matching, I'd like
to drop the matching on the RouteRule so that traffic from inside the
cluster can easily be routed. Host matching will still occur at the
Ingress.

Release Note

NONE

Inside the cluster it's common to make a request to a service using
a service's short name. For a service name foobar, you can make a
request for http://foobar/. Istio RouteRules effectivly intercept
traffic destend for one Service and route it to another Service.
Within the RouteRule, a filter can be applied based on the HTTP Host
header. If the header is not matched, the rule is not applied.

A Knative Route sends traffic destend for a single Service (fronted by
an Ingress) to another Service backed by a Revision. The Ingress already
applies Host based routing, the RouteRule also applying Host based
routing is at best redundent.

With the new Eventing Channel work, we'd like to be able to send
requests from inside the cluster to a Route, utilizing the RouteRule.
Currently, these requests are dropped because the request inside the
cluster is made to the service hostname and not the external fqdn.

Unless there is a reason to keep the redundent Host matching, I'd like
to drop the matching on the RouteRule so that traffic from inside the
cluster can easily be routed.
@google-prow-robot google-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 8, 2018
@grantr
Copy link
Contributor

grantr commented Jun 8, 2018

I'm curious what happens if an internal service uses the external domain to contact another internal service. Does that even work today? Would it work after this change?

Note that Istio recommends using fully qualified (internal) destination names to avoid namespace ambiguity:

When short names are used (e.g. “reviews” instead of “reviews.default.svc.cluster.local”), Istio will interpret the short name based on the namespace of the rule, not the service. A rule in the “default” namespace containing a host “reviews will be interpreted as “reviews.default.svc.cluster.local”, irrespective of the actual namespace associated with the reviews service. To avoid potential misconfigurations, it is recommended to always use fully qualified domain names over short names.

From https://istio.io/docs/reference/config/istio.networking.v1alpha3/#Destination

@scothis
Copy link
Contributor Author

scothis commented Jun 8, 2018

@grantr I tried using the fqdn for the Route (hello.default.demo-domain.com). The request failed because the DNS name couldn't be resolved. To be fair, this name doesn't exist outside the cluster either, which might be more confusing if it did.

Note that Istio recommends using fully qualified (internal) destination names to avoid namespace ambiguity

I read the same docs and took it to mean a name like hello.default.svc.cluster.local is recommended, instead of a short name like hello or hello.default. In either case, hello, hello.default and hello.default.svc.cluster.local will all resolve to the same IP (the service IP), but in each case the HTTP Host header will not match the RouteRule pattern of hello.default.demo-domain.com.

@ZhiminXiang
Copy link

I have tried to remove the host constraint from RouteRule locally. But actually I still couldn't successfully send request to the service fronted by Ingress (i.e. the placeholder service) after removing the host constraint. My personal understanding is that the placeholder service is not backed by any proxy. So there is no receiver for the requests sent to the placeholder service.
I am not sure if you have tried, and if it worked in your side.

@google-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: scothis
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: vaikas-google

Assign the PR to them by writing /assign @vaikas-google in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@scothis
Copy link
Contributor Author

scothis commented Jun 11, 2018

@ZhiminXiang good catch. The issue was that the empty Match struct was still present for the RouteRuleSpec. I made the type a pointer.

edit: I spoke too soon, there is still something blocking requests from inside the cluster.

@google-prow-robot
Copy link

@scothis: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-knative-serving-integration-tests 4f398f5 link /test pull-knative-serving-integration-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@scothis
Copy link
Contributor Author

scothis commented Jun 11, 2018

Digging a bit deeper, there is still an issue targeting a service backed by a RouteRule from inside the cluster. I have successfully targeted a service backed by a RouteRule before.

Either I'm missing something simple, or this functionality changed between Istio 0.6 and 0.8. Creating the equivalent service backed by an equivalent VirtualService works.

@scothis
Copy link
Contributor Author

scothis commented Jun 12, 2018

Superseded by #1047

@scothis scothis closed this Jun 12, 2018
@scothis scothis deleted the relax-route-host branch June 12, 2018 13:36
nak3 added a commit to nak3/serving that referenced this pull request May 14, 2022
* Add automated branch cut script

* Tweak create-release-branch.sh and fix mirror-upstream-branches.sh

* Fix typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants