catalog: correctly program shorthand service FQDN #1935
Conversation
Currently, while programming the service FQDNs for an upstream service in the downstream envoy's virtual_host config the shorthand service FQDN using the service name without the namespace is always programmed. This is incorrect because the same service (name) could exist in multiple namespaces. The service FQDN without the namespace should only be programmed on downstream clients that are in the same namespace. This change addresses this bug and adds tests for the same. It also removes a duplicated test copied into an unrelated package. Resolves openservicemesh#951 Signed-off-by: Shashank Ram <shashank08@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #1935 +/- ##
==========================================
- Coverage 58.50% 58.40% -0.10%
==========================================
Files 131 131
Lines 5270 5272 +2
==========================================
- Hits 3083 3079 -4
- Misses 2184 2190 +6
Partials 3 3
Continue to review full report at Codecov.
|
| // GetHostnamesForService returns the hostnames for a service | ||
| GetHostnamesForService(service service.MeshService) (string, error) | ||
| // GetResolvableHostnamesForUpstreamService returns the hostnames over which an upstream service is accessible from a downstream service | ||
| GetResolvableHostnamesForUpstreamService(downstream, upstream service.MeshService) (string, error) |
There was a problem hiding this comment.
I'm starting to wonder if some of the mental overload of this could be decreased (or increased) if we had upstream.GetHostnamesFor(downstream) 🤔
There was a problem hiding this comment.
That kind of semantic would work if the caller expects and API to simply return the hostnames, however the current API has more business logic of doing things differently for permissive mode, when traffic split isn't applicable, when traffic split is applied etc. Currently this is called in RDS which expects catalog to do the magic of returning all possible hostnames based on some business logic (permissive mode, traffic split etc.)
Description:
Currently, while programming the service FQDNs for an
upstream service in the downstream envoy's virtual_host config
the shorthand service FQDN using the service name without the namespace
is always programmed. This is incorrect because the same service (name)
could exist in multiple namespaces. The service FQDN without the
namespace should only be programmed on downstream clients that are
in the same namespace. If the downstream client is in a different namespace,
then shorthand FQDNs without the namespace for the upstream service
should not be programmed.
Consider the following example:
Imagine the following 3 services of the form
namespace/service:foo/bookbuyer, foo/bookstore, bar/bookstorebookbuyer is allowed to reach the bookstore service in both namespaces foo and bar.
Previously:
foo/bookstorewould be
bookstore, bookstore.foo, bookstore.foo.svc.cluster.local ....bar/bookstorewould be
bookstore, bookstore.bar, bookstore.bar.svc.cluster.local ....The above config would result in an error because the same hostname
bookstoreis programmed for 2 different destinations in the outbound route.
With this change, the
bookstorehostname will only be programmed for thefoo/bookstoredestination becausefoo/bookbuyeris in the same namespace,and is allowed to use shorthand FQDN to make a request of the form
http://bookstore/,without resulting in invalid virtual_host configurations on
bookbuyerenvoy.This change addresses this bug and adds tests for the same. It also
removes a duplicated test copied into an unrelated package.
Resolves #951
Affected area:
Please answer the following questions with yes/no.
No