emissary: Crash when adding request header `x-forwarded-proto`

Describe the bug I am trying to set the x-forwarded-proto header to the upstream server using add_request_headers config. When I access the URL, ambassador crashes.

Here’s my mapping:

---
apiVersion: v1
kind: Service
metadata:
  name: myservice
  labels:
    app: myservice
  annotations:
    getambassador.io/config: |
      ---
      apiVersion: ambassador/v0
      kind: Mapping
      name: myservice
      prefix: /
      host: myservice.mydomain.com
      service: http://myservice.default:8088
      add_request_headers:
        x-forwarded-proto: "%PROTOCOL%"
spec:
  type: NodePort
  ports:
  - name: myservice
    port: 8088
    targetPort: 8088
  selector:
    app: myservice

To Reproduce Steps to reproduce the behavior:

Configure a route as described above
Access the URL
See error

Expected behavior

I expected the service to get the x-forwarded-proto header, but instead ambassador crashes.

Versions (please complete the following information):

Ambassador: 0.37.0
Kubernetes environment: Azure Kubernetes Services (AKS)
Version: v1.10.3

About this issue

Original URL
State: closed
Created 6 years ago
Reactions: 3
Comments: 19 (9 by maintainers)

Most upvoted comments

The final decision of what the behavior of ambassador should be is, of course, up to your team. To me, however, I tend to think that anything that defies the user’s expectations is a defect. It is either: 1) a defect of implementation (A doesn’t behave as required), or 2) a defect of explanation (method A really does B).

Here, when one adds a header, I would expect the header to be set to that value, not that value plus something else, not a variant, but the value exactly as provided. Thus, that the resulting header value is “http, https” would be a defect by my standard because it is not literally “https”, as provided. I would have expected to need to do something like:

add_request_headers:
  x-forwarded-proto: "%PROTOCOL%,https"

to get the behavior I am seeing (to concatenate what I want with what I have). Instead, I’m forced into the current behavior (a comma-separated list) without having an option for the behavior I want (a single value) where other implementations could do either.

If, as you say, “Ambassador seems to be doing the right thing,” maybe the naming is inaccurate because the behavior does not seem correct to me. If you discuss with your team at some point, please consider whether this is this how all the headers work, or if this is an undocumented special case, and whether this is the most flexible implementation for your users.

Thanks for your help, regardless.

dpankros on Nov 15, 2018

In EA4, this crash no longer occurs, but the header is not modified either. An annotation like:

     apiVersion: ambassador/v0
      kind:  Mapping
      name:  httpbin_mapping
      prefix: /
      service: httpbin.org:80
      host_rewrite: httpbin.org
      add_request_headers:
        x-forwarded-proto: 'https'

Will still forwarded from envoy as:X-Forwarded-Proto: http (when connecting to ambassador/envoy over http)

dpankros on Nov 1, 2018

Edit: Looks like an upstream bug with Envoy: https://github.com/envoyproxy/envoy/issues/3919.

Same issue here. Here’s the crash log of the null pointer assertion that causes the crash. It’s important to note that every time a request is received for the Mapping in question, Ambassador crashes. Thus a single Mapping configuration can make the entire k8s cluster unreachable via the gateway.

frickenate on Aug 31, 2018