Add Header "Replace" Function

[ccravens]

Please describe the problem you have

TL;DR
Proposing adding a "replace" function to responseHeaderPolicy and requestHeaderPolicy with the following manifest format:

          responseHeaderPolicy:
            replace:
              - header: <header name>
                regex: <regex search pattern>
                value: <value to replace>

END TL;DR

We are embedding some of our services hosted behind a contour proxy in an <iframe>. Some of these services respond with a 302 redirect to a url with insecure "http", which causes the browser not to load the content due to protocol mismatch because the parent frame is served over https (https://stackoverflow.com/questions/38072510/blocked-iframe-due-to-mismatch-protocols). This is due to the application in the pod not being configured for TLS (cert-manager and contour handle the SSL termination), so the the application thinks it must respond with "http" rather than "https".

The following is the desired behavior:

We tried to invoke x-forwarded-proto with PROXY support settings (https://projectcontour.io/guides/proxy-proto/), but this did not cause the applications to respond with the proper https protocol.

Currently Contour supports both "set" and "remove" functions for both request and response HTTP headers (VERY useful: https://projectcontour.io/announcing-contour-1.1/), however the "set" function will not work in this use case for 2 reasons:

1. "set" would add the location header to all HTTP responses, we only want this to be applied when the "Location" header already exists (ie - for a 302 redirect response). By adding it to all responses it causes an infinite loop where all requests redirect back to the application, which responds again with a redirect and on and on. This causes Chrome to crash with the following screen:

2. The "set" function sets an HTTP header to static content, we are only looking to replace a portion of the pre-existing header rather than setting it to a static value.

Proposed Feature Enhancement
We are proposing the addition of a "replace" header function, in addition to "set" and "remove" that we believe gives the most optimal resolution to this particular problem, as well as giving any developer full control to manipulate HTTP headers as they need without adding complicated templating logic to the manifests. This new function will take 3 parameters 1) the header to apply the replace 2) a regex of what to replace and 3) the value to replace. For example, this is a sample manifest with the proposed solution that would resolve the problem described above:

          responseHeaderPolicy:
            set:
              - name: X-Service-Name
                value: s1
            remove:
              - X-Internal-Secret
            replace:
              - header: location
                regex: \http:\
                value: https:

This is an example of the header as it passes back through contour:

This would cause the location header to only exist if it had already been originally set by the application (resolves issue 1).

Additionally, this method would also allow the header value to stay correct, even if the location header is changed by the application (resolves issue 2) without complicated templating logic in the manifest. For example, if the application ever returned a different 302 redirect URL, it would still provide completely valid functionality:

[jpeach]

Similar to #2516 - use the value of a header to set the value of another header (in this case the source and destination headers are both Location).

[ccravens]

In regards to #2516, we may be able to include that in this update as well. Maybe instead of calling the function "replace" we call it "modify" and include both a regex replace (my proposal) as well as a copyFrom and copyTo (#2516 proposal). So for example:

          responseHeaderPolicy:
            set:
              - name: X-Service-Name
                value: s1
            remove:
              - X-Internal-Secret
            modify:
              - header: location
                regex: \http:\
                replace: https:

and for #2516 it would be something like:

          responseHeaderPolicy:
            set:
              - name: X-Service-Name
                value: s1
            remove:
              - X-Internal-Secret
            modify:
              - copyFrom: x-forwarded-for
                copyTo: x-real-client-ip
                default: {src.ip} <<--- Not sure about this?

I'm not sure about the "else set x-forwarded-for to the src ip" requirement, as that would require templating logic and from my conversations with @stevesloka he seemed to want to avoid that. If this has been brought up before maybe it's something to start looking into?

[youngnick]

Firstly, thanks for the detailed feature request @ccravens. It's very thorough and well-explained.

Rereading the traceback of linked issues here, we have a problem at the moment, where when we're doing this:

• we have no way to verify if the config will work before passing to Envoy
• If the config does not work, Envoy will stop accepting config updates at all from Contour until it's restarted. (This is internal/grpc: properly handle Envoy ACK/NACK protocol #1176).

I agree that this is a worthwhile feature, that we should figure out how to implement, but wanted to flag that because of underlying issues, it's not straightforward to build. Probably we're going to need to do something about #1176 first - that is a big job though, as most of the Go support for interacting with Envoy doesn't support the config ACKs properly (thinking of go-control-plane here, being worked on in #2134, but which also turns out to require a bunch of refactoring, #2775 is one).

[ccravens]

@youngnick awesome thank you for that info! We are definitely happy to look into this with the contour team and maybe help to get some of these items addressed. We've started taking a look at Header policies and compiling, writing tests, etc from a branch from our fork here: https://github.com/ossys/contour/tree/2776_add_header_replace_function

We're working on getting envoy to compile now and will be reviewing that source as well to get our feet wet and comfortable w/ the architecture.

[youngnick]

Fantastic @ccravens! Please come along to the community meetings or the office hours and talk about where you're up to and what you're working on - we've got some refactoring being worked on at the moment which may make lots of conflicts if we don't manage it carefully.

[jpeach]

We're working on getting envoy to compile now and will be reviewing that source as well to get our feet wet and comfortable w/ the architecture.

For an envoy build set up, maybe https://github.com/jpeach/envoy-build helps, but mostly running envoy from a container is fine for contour dev.

[ccravens]

@youngnick I found this looks like Envoy V3 is offering this type of capability, so I think we can just implement this Envoy feature using the proposed manifest descriptors above? What are your thoughts?

https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-field-config-route-v3-routeaction-regex-rewrite

[ccravens]

Also it looks like there is an open WIP on Envoy for this exact request as well :)

envoyproxy/envoy#12938

[ccravens]

Also another good one to follow: envoyproxy/envoy#12591

[youngnick]

Thanks for the research @ccravens!

A couple of things:

• We don't use the xDS v3 APIs yet. We are refactoring some things to be able to get there, but Add support for Envoy xDS v3. #1898 is the current issue to watch for updates.
• Even once Envoy grows the replace feature, I'm concerned about allowing the specifying of a regex, just because of the problem I mentioned with Envoy stopping receiving updates on config error.

The thing I'm worried about in the second case is something like this:

• User specifies a regex replace for a header.
• User makes a typo in their regex (let's face it, it's not hard to do it!)
• Envoy rejects the update from Contour, and connection to Envoy breaks.
• Envoy needs to be restarted to pick up a fix.

I know this sounds ridiculous, and it is to me too, but it is a real problem we've seen before with allowing more complex config to be entered by the user. So I'd certainly like to see some testing of what happens when you send a bad regex to Envoy.

[youngnick]

Okay, this hasn't been touched in a long time, here's what's changed in the meantime:

• The NACK problem described in internal/grpc: properly handle Envoy ACK/NACK protocol #1176 isn't as severe as I thought - if we make a mistake, Envoy will stop accepting updates until we stop sending the broken config, but it will recover once we remove the broken config.
• However, we still don't have a good way to validate the regex before sending it to Envoy to avoid the problem altogether.

I understand that having the ability to specify a regex would be a far more flexible solution to this problem than anything else, but I don't see how we can justify the risk that a regex that doesn't compile can break the connection between Contour and Envoy, and you will need to look at Contour's logs to determine that's happened, since we don't have a way to close the NACK loop yet.

From a user's perspective, what you would see is that you add a thing that uses a regex, and then all updates, including endpoint ones cease arriving at Envoy. Because of the way we (don't) handle NACKs at the moment, this will be completely silent except for a somewhat cryptic log line in Contour's logs, so it will be very hard to find and fix.

I think this situation is one hundred percent not ideal, and @sunjayBhatia is working on changes to the xDS handling code that should eventually allow us to get to a place where we can solve this, but that's an unknown amount of time away.

However, I think that something with some more safety guarantees is more implementable, like a straight string replace instead of a full regex.

So instead of specifying as above, with a regex, we add something like:

          responseHeaderPolicy:
            set:
              - name: X-Service-Name
                value: s1
            remove:
              - X-Internal-Secret
            modify:
              - header: location
                find: 'http:'
                replace: 'https:'

(note that in this case it's imperative to escape the string correctly, because : is a YAML metacharacter, sigh).

The find value would then be combined with the replace value to produce our own regex, something like:
s/http:/https:/g. This is not as flexible, but FAR safer. What do you think @ccravens? I know it's not what you asked for, but I think that it could be useful enough for now?

[github-actions]

The Contour project currently lacks enough contributors to adequately respond to all Issues.

This bot triages Issues according to the following rules:

• After 60d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the Issue is closed

You can:

• Mark this Issue as fresh by commenting
• Close this Issue
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

[github-actions]

The Contour project currently lacks enough contributors to adequately respond to all Issues.

This bot triages Issues according to the following rules:

• After 60d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the Issue is closed

You can:

• Mark this Issue as fresh by commenting
• Close this Issue
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack