solo-io / gloo

The Feature-rich, Kubernetes-native, Next-Generation API Gateway Built on Envoy
https://docs.solo.io/
Apache License 2.0
4.04k stars 431 forks source link

Istio Integration: Host header is rewritten for Kube service NOT in the mesh when it should NOT be #7263

Open murphye opened 1 year ago

murphye commented 1 year ago

Gloo Edge Version

1.12.x (latest stable)

Kubernetes Version

1.21.x

Describe the bug

See: https://github.com/solo-io/gloo/pull/6897/commits/e4449aacd8bc1d9f36f0287c0d1ae4258f2cc0aa#r954311101 https://github.com/solo-io/gloo/pull/6897/commits/e4449aacd8bc1d9f36f0287c0d1ae4258f2cc0aa#r983729207 https://github.com/solo-io/gloo/pull/6897/commits/e4449aacd8bc1d9f36f0287c0d1ae4258f2cc0aa#r983731927

hostInMesh Seems to be set for ANY Kube service when it should be set for Kube services that are actually in the mesh. I assume you would check if an Istio sidecar is present? I do not see the logic for this in func GetHostFromDestination.

Steps to reproduce the bug

WE ARE CURRENTLY WORKING TO VERIFY THESE STEPS TODAY

  1. Deploy Edge with Istio Integration
  2. Deploy Echo service (without sidecar)
  3. Call Echo the through Edge (that has the Istio sidecar)
  4. See that the Host header has been rewritten per https://github.com/solo-io/gloo/pull/6897/commits/e4449aacd8bc1d9f36f0287c0d1ae4258f2cc0aa#diff-cd2bcd18b504eb17a5fa945e48c4dbdf4da0b3dda93ec62f13785c1d7e536920R98

Expected Behavior

Host is not rewritten if the Service is NOT running in the mesh. Would work the same as Edge without Istio

Additional Context

This causes problems with Portal when Istio is enabled. There might be a workaround possible. TBD https://github.com/solo-io/dev-portal/issues/2428

Original GitHub Issue: https://github.com/solo-io/gloo/issues/6538

murphye commented 1 year ago

It might be possible to check for the Istio sidecar injection annotation on the Pod/Deployment and/or Namespace to implement the fix for this

nfuden commented 1 year ago

As part of this we can do something like add an annotation to upstreams on discovery if they contain a sidecar with istio in it.

murphye commented 1 year ago

@nfuden @jbohanon Whatever we do, it needs to also work with Gloo Portal as well. Just FYI.

This is also blocking the customer and they are trying to roll out to production as we speak.

kdorosh commented 1 year ago

we are running gloo and istio as two control planes, gloo for advanced ingress/egress features and istio for mtls / rotating certs and other mesh functionality. to get this to work, we need to update istio to rewrite the host header on the sidecar to gloo's gateway proxy, akin to:

apiVersion: networking.istio.io/v1alpha3
kind: EnvoyFilter
metadata:
  name: host-rewrite
spec:
  configPatches:
  - applyTo: HTTP_ROUTE
    match:
      routeConfiguration:
        vhost:
          name: nginx1.pawelp.local.com:6644
          route:
            action: ANY
    patch:
      operation: MERGE
      value:
        route:
          auto_host_rewrite: true # specify a value here from another header (xff?)
  workloadSelector:
    labels:
      app: <application label>

also for performance reasons, we should update the istio plugin in gloo to use the snapshot rather than make network calls during translation

kdorosh commented 1 year ago

another perhaps easier option for this is to just add it to the mesh service registry (even without sidecar) via ServiceEntry (with location: MESH_EXTERNAL) as proposed here https://stackoverflow.com/questions/62620648/istio-and-the-http-host-header

just give the service entry the hostname the backend was expecting

kdorosh commented 1 year ago

to be clear @murphye the request flow is:

client -H "Host: abc.com" --> gateway-proxy -H "Host: ?" "XFF: abc.com" --> gateway-proxy sidecar -H "Host: ??" "XFF: abc.com,?" --> backend workload -H "Host: ??" "XFF: abc.com,?"

the host header is rewritten here because we need to do that to make request to envoy's envoy sidecar (this feels silly but i digress). the smarts should be in istio's envoy to send the right host header to the backing service, which means it needs to be in the mesh, even if its an external service or adding via service entry.

jbohanon commented 1 year ago

I have confirmed that by applying the following EnvoyFilter the Host header that arrives at the upstream is that which was included in the original request. This rewrites the headers for in-mesh and out-of-mesh proxied requests, but they were still reaching destinations because I believe specifying the SIDECAR_OUTBOUND context causes the rewrite to happen after DNS lookup.

apiVersion: networking.istio.io/v1alpha3
kind: EnvoyFilter
metadata:
  name: host-rewrite
  namespace: istio-system
spec:
  configPatches:
  - applyTo: HTTP_ROUTE
    match:
      context: SIDECAR_OUTBOUND
    patch:
      operation: MERGE
      value:
        route:
          host_rewrite_header: x-forwarded-host
  workloadSelector:
    labels:
      gloo: gateway-proxy
gsustek commented 8 months ago

we are running gloo and istio as two control planes, gloo for advanced ingress/egress features and istio for mtls / rotating certs and other mesh functionality. to get this to work, we need to update istio to rewrite the host header on the sidecar to gloo's gateway proxy, akin to:

apiVersion: networking.istio.io/v1alpha3
kind: EnvoyFilter
metadata:
  name: host-rewrite
spec:
  configPatches:
  - applyTo: HTTP_ROUTE
    match:
      routeConfiguration:
        vhost:
          name: nginx1.pawelp.local.com:6644
          route:
            action: ANY
    patch:
      operation: MERGE
      value:
        route:
          auto_host_rewrite: true # specify a value here from another header (xff?)
  workloadSelector:
    labels:
      app: <application label>

also for performance reasons, we should update the istio plugin in gloo to use the snapshot rather than make network calls during translation

@kdorosh this is not an option on RH Service Mesh which does NOT support envoy filters... Any workaround for a workaround? :-)

kdorosh commented 8 months ago

only other obvious option is to rewrite the host header using istio primitives: https://istio.io/latest/docs/reference/config/networking/virtual-service/#HTTPRewrite

other than that, seems like the pod just shouldn't have a sidecar then? if you don't want it in the mesh


separate from that, the original GH issue title is a bit confusing.

pods are in or out of the mesh services are not (they could select pods that themselves are in or out of the mesh)

gsustek commented 8 months ago

Thnx for the suggestion. But for me the question is why pods in a mesh need to have host rewritten to the kubernetes service name in first place(some functionally/standard perhaps), which effect reverse proxy functionality if it is in pod protected by sidecar. Could it be turn on or off in future?

@kdorosh

seems like the pod just shouldn't have a sidecar then? if you don't want it in the mesh

Even if our pods does not have sidecar infront of them, istio sidecar on gloo-proxy is already doing host rewrite.