-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Using request.host
in expressions for metrics is problematic at ingress gateway
#18818
Comments
Compounding this issue, our grafana dashboards render |
For the grafana issue, a short-term workaround is to replace all service templating as follows: replace: I'll want someone to double-check that, however. |
I fully understand the problem, however I can’t stress how useful this feature is for us identifying black hole traffic. As we don’t use ingress gateway (we use Nginx) this isnt much of a concern for us either as random external hosts won’t get to envoy. I would ask that you at least make the feature optionally enabled so the operator can make the call. |
@douglas-reid IIUC, the issue is only in ingress gateways when you have non-wildcard host matching in Gateways and VirtualService, and for any non-matching host traffic the blackhole cluster will be hit you can have a metric cardinality explosion due to random internet traffic host getting populated in destination service label. It’s very easy to set destination service label to BlackHoleCluster only for ingress gateway but leave it as is for sidecars like you suggested. This means for any outbound traffic from the mesh sidecars we still capture the host header @Stono. I can that in today before 1.4 goes out. Based on our conversation yesterday Doug, you were saying that we never capture inbound traffic metrics for ingress gateway, isn’t this inbound? |
I believe this is happening at ingress when traffic does not hit BlackHole cluster but is 404ing as well I will double check the config and add more details here later. |
You might be right, does ingress gateways even have a concept of BlackHoleCluster? Afaik ingress gateways just return a direct response of 404 for non matching traffic. |
Gateways do not have a black hole cluster. So these are real 404 NRs. |
For v1 Mixer this is a purely configuration issue. |
@mandarjog I agree, we can update the default config to be this:
This will make sure that the value is set to |
Either that or use response_flag == NR |
Ok, I will send a PR soon after verifying configuration changes work and we can cherry-pick in release-1.3, 1.4 branches. |
@mandarjog as long as there is a way to get the host header for BlackHoleCluster I'm not fussed about the implementation. I (and multiple) people tweeted about this feature and its value - so please make sure you clearly document how to keep the capability for those who are, or want to use it! |
Don't worry, you won't lose this functionality. :) |
Updated the default metrics to only use `request.host` in `destination_service` if the cluster is known including BlackHole/Passthrough, otherwise set it to `unknown`. This prevents random traffic via ingress gateway to report `request.host` in telemetry which can cause cardinality explosion. Fixes: istio#18818
Updated the default metrics to only use `request.host` in `destination_service` if the cluster is known including BlackHole/Passthrough, otherwise set it to `unknown`. This prevents random traffic via ingress gateway to report `request.host` in telemetry which can cause cardinality explosion. Fixes: #18818
Updated the default metrics to only use `request.host` in `destination_service` if the cluster is known including BlackHole/Passthrough, otherwise set it to `unknown`. This prevents random traffic via ingress gateway to report `request.host` in telemetry which can cause cardinality explosion. Fixes: istio#18818
Updated the default metrics to only use `request.host` in `destination_service` if the cluster is known including BlackHole/Passthrough, otherwise set it to `unknown`. This prevents random traffic via ingress gateway to report `request.host` in telemetry which can cause cardinality explosion. Fixes: istio#18818
Updated the default metrics to only use `request.host` in `destination_service` if the cluster is known including BlackHole/Passthrough, otherwise set it to `unknown`. This prevents random traffic via ingress gateway to report `request.host` in telemetry which can cause cardinality explosion. Fixes: #18818
Updated the default metrics to only use `request.host` in `destination_service` if the cluster is known including BlackHole/Passthrough, otherwise set it to `unknown`. This prevents random traffic via ingress gateway to report `request.host` in telemetry which can cause cardinality explosion. Fixes: #18818
Updated the default metrics to only use `request.host` in `destination_service` if the cluster is known including BlackHole/Passthrough, otherwise set it to `unknown`. This prevents random traffic via ingress gateway to report `request.host` in telemetry which can cause cardinality explosion. Fixes: istio#18818
Bug description
In #16892, we added
request.host
to the expression fordestination_service
labels. This was added as a fallback for cases whendestination.service.host
would not be set (pass-thru, blackhole, NR).In theory, this is a nice idea. However, this is problematic at ingress, as the
Host
header can be set to arbitrary values.Example of sending such a request:
curl --verbose --header 'Host: %22%3E%3Cscript%3Ealert('Qualys_XSS_Joomla_2.5.3')%3C%2Fscript%3E' <gateway ip>/no-route
This results in metrics with a destination service label containing junk -- some of it potentially harmful.
Example from Prometheus:
This is problematic for a number of reasons:
Host
headersservice
variables in grafana dashboardsrequest.host
Expected behavior
Arbitrary NR traffic at ingress should not be able to pollute istio generated metrics with junk
destination_service
labels.Either we should set
destination.service.host
to be "BlackHole" or similar OR we should excluderequest.host
from the expression fordestination_service
in configuration for ingressgateway (or a third, better option that someone may have).Steps to reproduce the bug
Install Istio, have a gateway for wildcard host on port 80. Have a virtual service with an exact match route and nothing else. send traffic to gw not matching the route with a host header with junk info. observe results.
Version (include the output of
istioctl version --remote
andkubectl version
)1.4.0-beta.2, but any version of istio with the linked PR is affected.
How was Istio installed?
helm template
Environment where bug was observed (cloud vendor, OS, etc)
GKE
The text was updated successfully, but these errors were encountered: