fix(demo): remove host headers in SMI policies#3644
fix(demo): remove host headers in SMI policies#3644nojnhuh merged 1 commit intoopenservicemesh:mainfrom nojnhuh:host-headers
Conversation
shashankram
left a comment
shashankram
left a comment
There was a problem hiding this comment.
@nojnhuh the change looks correct, although I don't think we should incorporate this into our demo given it opens the door for misconfiguration like the one seen in #3616. Specifying the host header in the route literally provides no benefit at the moment, given it won't even come into effect if the correct host header is not programmed in virtual_hosts.domains[].
I suggest removing it instead of updating it.
| - host: "bookstore.bookstore:14001" | ||
| - "user-agent": ".*-http-client/*.*" | ||
| - "client-app": "bookbuyer" | ||
| - name: buy-a-book | ||
| pathRegex: ".*a-book.*new" | ||
| methods: | ||
| - GET | ||
| headers: | ||
| - host: "bookstore.bookstore" | ||
| - host: "bookstore.bookstore:14001" |
There was a problem hiding this comment.
Rather than specifying this, I suggest removing it entirely because host header matching is already enforced using virtual_host.domains[].
@snehachhabria added plumbing for this, but specifying host headers here is just going to mislead users to think they can specify whatever they want, when in fact this has to explicitly be a service FQDN:port combination that is already programmed in virtual_hosts.domains[].
More details in #2369 (comment).
Specifying host headers here serves no real purpose in our existing implementation.
There was a problem hiding this comment.
I agree with @shashankram, there is no real purpose of this in our current implementation given that it has to be the service FQDN:port, which is provided by default in the domains.
There was a problem hiding this comment.
Makes sense. I see the bookwarehouse HTTPRouteGroup also has a host header match (which doesn't appear to be causing any issues. Should I remove that too?
There was a problem hiding this comment.
Yes we should remove that as well.
There was a problem hiding this comment.
I see the bookwarehouse HTTPRouteGroup also has a host header match (which doesn't appear to be causing any issues.
I had discussed the removal of this with @snehachhabria a month or so ago, and the only reason I suspect this works is because maestro doesn't fail if requests from the bookstore -> bookwarehouse fail.
Regardless, we should fix this.
|
FYI change that broke the manual demo: openservicemesh/osm-docs#121 |
The `:authority` header sent by the bookbuyer to the bookstore is
actually `bookstore.bookstore:14001` where the bookstore's
HTTPRouteGroup used in the manual demo specified a `host` header match
of `bookstore.bookstore`, resulting in
"response_code_details": "route_not_found"
in the bookstore's logs for requests from the bookbuyer.
This change removes the `host` header matches all of the demo manifests.
Fixes #3616
Signed-off-by: Jon Huhn <johuhn@microsoft.com>
nojnhuh
changed the title
shashankram
left a comment
shashankram
left a comment
There was a problem hiding this comment.
Thanks @nojnhuh. Please backport to release-v0.9 as well.
Codecov Report
@@ Coverage Diff @@
## main #3644 +/- ##
==========================================
+ Coverage 67.04% 67.07% +0.02%
==========================================
Files 179 179
Lines 8704 8705 +1
==========================================
+ Hits 5836 5839 +3
+ Misses 2837 2835 -2
Partials 31 31
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
4 participants
Description:
fix(demo): remove host headers in SMI policies
The
:authorityheader sent by the bookbuyer to the bookstore isactually
bookstore.bookstore:14001where the bookstore'sHTTPRouteGroup used in the manual demo specified a
hostheader matchof
bookstore.bookstore, resulting inin the bookstore's logs for requests from the bookbuyer.
This change removes the
hostheader matches all of the demo manifests.Fixes #3616
Affected area:
Please answer the following questions with yes/no.
Does this change contain code from or inspired by another project? No
Is this a breaking change? No