Skip to content
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

Enable setting headers on all routes with dynamic service values #3258

Closed
erwbgy opened this issue Jan 14, 2021 · 4 comments
Closed

Enable setting headers on all routes with dynamic service values #3258

erwbgy opened this issue Jan 14, 2021 · 4 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/needs-triage Indicates that an issue needs to be triaged by a project contributor.
Milestone

Comments

@erwbgy
Copy link
Contributor

erwbgy commented Jan 14, 2021

Using Linkerd with Contour requires setting the l5d-dst-override header on every request to the destination service hostname and port - for example: kuard.default.svc.cluster.local:80.

I agree with @jpeach's comment on #2089 that Contour shouldn't have special support for Linkerd headers - like Ambassador's add_linkerd_headers option - but I think it should be possible to support it more generically including @stevesloka's idea of adding default headers.

My proposal is to enable request and response headers specified in the config file to be set on all routes with dynamic values for namespace, service name and service port:

  1. Add support for %CONTOUR_NAMESPACE%, %CONTOUR_SERVICE_NAME% and %CONTOUR_SERVICE_PORT% dynamic variables and have these expanded like Envoy dynamic variables are in Support specific dynamic request headers #3234. (The CONTOUR_ prefix is used so that there is no possibility of a clash with a future Envoy dynamic variable.)

This would allow a header to be set in an HTTPProxy object:

      requestHeadersPolicy:
        set:
        - name: X-Envoy-Hostname
          value: "%HOSTNAME%"
        - name: l5d-dst-override
          value: "%CONTOUR_SERVICE_NAME%.%CONTOUR_NAMESPACE%.svc.cluster.local:%CONTOUR_SERVICE_PORT%"

If any of these variables are used where they can't be expanded then they are just passed through literally. For example if %CONTOUR_SERVICE_NAME% is used at the route level then it is not expanded and becomes %%CONTOUR_SERVICE_NAME%% when passed to Envoy. This avoids any Envoy configuration errors due to bad dynamic variables.

  1. Allow default request and response headers added to every route to be set in the Contour config file:
headers:
  set-request-headers:
  - "X-Envoy-Hostname=%HOSTNAME%"
  - "l5d-dst-override=%CONTOUR_SERVICE_NAME%.%CONTOUR_NAMESPACE%.svc.cluster.local:%CONTOUR_SERVICE_PORT%"
  set-response-headers:
  - "X-Envoy-Response-Flags=%RESPONSE_FLAGS%"

This is the same as if the above requestHeadersPolicy was added to every route service. These headers would need to be set at the service level so that the service name and port are available.

With these changes the Linkerd header requirement is met but without anything specific for Linkerd, enabling similar requirements to be met in future.

Using a mutating webhook would be another way to get the l5d-dst-override header set on every request, but this makes it more difficult to use GitOps solutions like ArgoCD or Flux as the object changes, so is not great in practice. The webhook is also another component to operate and maintain.

If this design sounds reasonable then I have a pull request that implements these changes, that can be the basis of further discussion.

@erwbgy erwbgy added kind/feature Categorizes issue or PR as related to a new feature. lifecycle/needs-triage Indicates that an issue needs to be triaged by a project contributor. labels Jan 14, 2021
@youngnick
Copy link
Member

I like the idea of Contour being able to interact with service meshes, that sounds awesome.

However, I think there's two things to add as part of this:

  • add %CONTOUR_NAMESPACE%, %CONTOUR_SERVICE_NAME% and %CONTOUR_SERVICE_PORT% interpolation variables to the header expansion list.
  • add default header policy to the Contour configuration file, which will be applied by default across all routes added.

I think the first is straightforward and worth doing regardless, but I think that the second needs a bit more thought.

@erwbgy, could you split your PR into two pieces?

The things I'm thinking for the second part are:

  • Do we apply the default to all routes? Whatever the config method? So, for Ingress and HTTPRoute (from service-APIs) as well as HTTPProxy?
  • Do we add a way to disable or wait for people to ask?
  • What happens if someone defines a header policy in their HTTPProxy that conflicts with the globally-set one? That is, what happens if someone tries to set the same header, or tries to unset the default one?

erwbgy added a commit to erwbgy/contour that referenced this issue Jan 23, 2021
Add support for %CONTOUR_NAMESPACE%, %CONTOUR_SERVICE_NAME% and
%CONTOUR_SERVICE_PORT% dynamic variables and have these expanded like
Envoy dynamic variables are in projectcontour#3234. (The CONTOUR_ prefix is used so
that there is no possibility of a clash with a future Envoy dynamic
variable.)

If any of these variables are used where they can't be expanded then
they are just passed through literally. For example if
%CONTOUR_SERVICE_NAME% is used at the route level then it is not
expanded and becomes %%CONTOUR_SERVICE_NAME%% when passed to Envoy. This
avoids any Envoy configuration errors due to bad dynamic variables.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
erwbgy added a commit to erwbgy/contour that referenced this issue Jan 23, 2021
Add default header policy to the Contour configuration file, which will
be applied by default across all routes added.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
erwbgy added a commit to erwbgy/contour that referenced this issue Jan 27, 2021
Add support for %CONTOUR_NAMESPACE%, %CONTOUR_SERVICE_NAME% and
%CONTOUR_SERVICE_PORT% dynamic variables and have these expanded like
Envoy dynamic variables are in projectcontour#3234. (The CONTOUR_ prefix is used so
that there is no possibility of a clash with a future Envoy dynamic
variable.)

If any of these variables are used where they can't be expanded then
they are just passed through literally. For example if
%CONTOUR_SERVICE_NAME% is used at the route level then it is not
expanded and becomes %%CONTOUR_SERVICE_NAME%% when passed to Envoy. This
avoids any Envoy configuration errors due to bad dynamic variables.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
erwbgy added a commit to erwbgy/contour that referenced this issue Jan 27, 2021
Add support for %CONTOUR_NAMESPACE%, %CONTOUR_SERVICE_NAME% and
%CONTOUR_SERVICE_PORT% dynamic variables and have these expanded like
Envoy dynamic variables are in projectcontour#3234. (The CONTOUR_ prefix is used so
that there is no possibility of a clash with a future Envoy dynamic
variable.)

If any of these variables are used where they can't be expanded then
they are just passed through literally. For example if
%CONTOUR_SERVICE_NAME% is used at the route level then it is not
expanded and becomes %%CONTOUR_SERVICE_NAME%% when passed to Envoy. This
avoids any Envoy configuration errors due to bad dynamic variables.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
erwbgy added a commit to erwbgy/contour that referenced this issue Jan 29, 2021
Add support for %CONTOUR_NAMESPACE%, %CONTOUR_SERVICE_NAME% and
%CONTOUR_SERVICE_PORT% dynamic variables and have these expanded like
Envoy dynamic variables are in projectcontour#3234. (The CONTOUR_ prefix is used so
that there is no possibility of a clash with a future Envoy dynamic
variable.)

If any of these variables are used where they can't be expanded then
they are just passed through literally. For example if
%CONTOUR_SERVICE_NAME% is used at the route level then it is not
expanded and becomes %%CONTOUR_SERVICE_NAME%% when passed to Envoy. This
avoids any Envoy configuration errors due to bad dynamic variables.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
erwbgy added a commit to erwbgy/contour that referenced this issue Jan 29, 2021
Add default header policy to the Contour configuration file, which will
be applied by default across all routes added.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
erwbgy added a commit to erwbgy/contour that referenced this issue Jan 31, 2021
Add a policy section to the Contour configuration file with
request-headers and response-headers fields that allow
setting or removing default request and response headers unless
overriden by users in their HTTPProxy objects.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
erwbgy added a commit to erwbgy/contour that referenced this issue Jan 31, 2021
Add a policy section to the Contour configuration file with
request-headers and response-headers fields that allow
setting or removing default request and response headers unless
overriden by users in their HTTPProxy objects.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
erwbgy added a commit to erwbgy/contour that referenced this issue Jan 31, 2021
Add a policy section to the Contour configuration file with
request-headers and response-headers fields that allow
setting or removing default request and response headers unless
overriden by users in their HTTPProxy objects.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
erwbgy added a commit to erwbgy/contour that referenced this issue Jan 31, 2021
Add a policy section to the Contour configuration file with
request-headers and response-headers fields that allow
setting or removing default request and response headers unless
overriden by users in their HTTPProxy objects.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
erwbgy added a commit to erwbgy/contour that referenced this issue Feb 11, 2021
Add a policy section to the Contour configuration file with
request-headers and response-headers fields that allow
setting or removing default request and response headers unless
overriden by users in their HTTPProxy objects.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
erwbgy added a commit to erwbgy/contour that referenced this issue Feb 11, 2021
Add a policy section to the Contour configuration file with
request-headers and response-headers fields that allow
setting or removing default request and response headers unless
overriden by users in their HTTPProxy objects.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
erwbgy added a commit to erwbgy/contour that referenced this issue Feb 11, 2021
Add a policy section to the Contour configuration file with
request-headers and response-headers fields that allow
setting or removing default request and response headers unless
overriden by users in their HTTPProxy objects.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
erwbgy added a commit to erwbgy/contour that referenced this issue Feb 12, 2021
Add a policy section to the Contour configuration file with
request-headers and response-headers fields that allow
setting or removing default request and response headers unless
overriden by users in their HTTPProxy objects.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
erwbgy added a commit to erwbgy/contour that referenced this issue Feb 28, 2021
Add a policy section to the Contour configuration file with
request-headers and response-headers fields that allow
setting or removing default request and response headers unless
overriden by users in their HTTPProxy objects.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
erwbgy added a commit to erwbgy/contour that referenced this issue Feb 28, 2021
Add a policy section to the Contour configuration file with
request-headers and response-headers fields that allow
setting or removing default request and response headers unless
overriden by users in their HTTPProxy objects.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
@skriss skriss added this to the 1.14.0 milestone Mar 1, 2021
erwbgy added a commit to erwbgy/contour that referenced this issue Mar 2, 2021
Add a policy section to the Contour configuration file with
request-headers and response-headers fields that allow
setting or removing default request and response headers unless
overriden by users in their HTTPProxy objects.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
erwbgy added a commit to erwbgy/contour that referenced this issue Mar 2, 2021
Add a policy section to the Contour configuration file with
request-headers and response-headers fields that allow
setting or removing default request and response headers unless
overriden by users in their HTTPProxy objects.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
erwbgy added a commit to erwbgy/contour that referenced this issue Mar 2, 2021
Add a policy section to the Contour configuration file with
request-headers and response-headers fields that allow
setting or removing default request and response headers unless
overriden by users in their HTTPProxy objects.

Signed-off-by: Keith Burdis <keith.burdis@gs.com>
@skriss skriss closed this as completed in d978772 Mar 2, 2021
@artificial-aidan
Copy link
Contributor

artificial-aidan commented Aug 27, 2021

This got added for HTTPProxy objects, but is there any plan for adding this to Ingress objects too? Just having it on HTTPProxy objects means you still can't enable linkerd on contour.

@youngnick
Copy link
Member

Oh, I thought that Linkerd would work with HTTPProxy objects from the initial request. There are currently no plans, but please feel free to open an issue, and we can discuss how we could make it work. (It would be a lot of annotations, which we are generally reluctant to do, but we can definitely discuss).

@artificial-aidan
Copy link
Contributor

#4005

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/needs-triage Indicates that an issue needs to be triaged by a project contributor.
Projects
None yet
Development

No branches or pull requests

4 participants