NE-1366: Revisions for set-delete-http-headers EP #1446
Conversation
miheer
force-pushed
the
headers-followup
branch
2 times, most recently
from
bf77874
to
3027850
Compare
|
|
||
| Please note that the setting of forwardedHeaderPolicy happens at the plain backend section of haproxy.config i.e. per route level and not at the front-end section | ||
| of the haproxy.config level as per the above-mentioned link. | ||
| The custom HTTP response value of `x-forwarded` header set using `spec.httpHeaders.actions` of the IngressController will take over the value defined in annotation `haproxy.router.openshift.io/set-forwarded-headers` of Route, `spec.httpHeaders.forwardedHeaderPolicy` of Ingress Controller |
There was a problem hiding this comment.
I would add something more explicit, like "In contrast to request headers, in which the IngressController values override the Route spec.httpHeaders.actions values, for response headers the Route values override the IngressController values".
There was a problem hiding this comment.
Ok added this line above the added line in the diff
There was a problem hiding this comment.
i have added sequence of execution.
| @@ -782,6 +765,26 @@ For this reason, the `host` header is in the deny list of headers that are prohi | |||
|
|
|||
| Please note that a custom `host` header may be specified via RouteSpec. Please refer to `Open Questions` for more details. | |||
|
|
|||
| ### Cookie and Set-Cookie | |||
| There are three reasons we blocked `Cookie` and `Set-Cookie`: | |||
There was a problem hiding this comment.
| There are three reasons we blocked `Cookie` and `Set-Cookie`: | |
| We do not allow setting `Cookie` or `Set-Cookie` headers, for the following three reasons: |
| 1. There were no use cases mentioned for these headers in [RFE](https://issues.redhat.com/browse/RFE-464). | ||
|
|
||
| 2. The cookies that HAProxy sets are used for session tracking, to map a client connection to a particular |
There was a problem hiding this comment.
Item 2 adds more meaning to the decision than item 1. I would swap the ordering.
| The cookie name is an MD5 hash value derived from the backend's name (similarly, the cookie value is a hashed value | ||
| derived from the backend server's endpoint). | ||
|
|
||
| So the cookie names that we would want to forbid are all the possible values | ||
| of the MD5 hashing algorithm, but it's extremely unlikely that someone would accidentally choose the same cookie name | ||
| that HAProxy used. |
There was a problem hiding this comment.
These lines don't add any meaning to the discussion.
There was a problem hiding this comment.
but we need to explain what the interference may be right ?
There was a problem hiding this comment.
Your comment makes it look like the whole thing is "extremely unlikely" to happen, so I don't think this is the interference we're concerned with.
| However, if there is an interference caused by setting these then either the HAProxy's session affinity might not work correctly, | ||
| or HAProxy might overwrite the cookie. |
There was a problem hiding this comment.
| However, if there is an interference caused by setting these then either the HAProxy's session affinity might not work correctly, | |
| or HAProxy might overwrite the cookie. | |
| We don't want to risk interfering with HAProxy's session affinity, nor restrict HAProxy's ownership of a cookie. |
There was a problem hiding this comment.
or HAProxy might overwrite the cookie. sounds more better than HAProxy's ownership of a cookie.
There was a problem hiding this comment.
nor restrict--we can say nor affect as well ?
There was a problem hiding this comment.
What do you suggest? I wanted to clarify what an "interference" is, as this is not a generally used tech term.
There was a problem hiding this comment.
We don't want to risk interfering with HAProxy's session affinity, nor restrict HAProxy's ownership of a cookie. -- added this
| However, if there is an interference caused by setting these then either the HAProxy's session affinity might not work correctly, | ||
| or HAProxy might overwrite the cookie. | ||
|
|
||
| 3. It was thought that it would be better to have a structured API for it. |
There was a problem hiding this comment.
| 3. It was thought that it would be better to have a structured API for it. | |
| 3. A better solution for cookie headers is a new structured API feature. |
| than the ones admin has set cluster-wide. | ||
| 2. The IngressController and Route might be owned by the same person who wants to specify some global configuration | ||
| and then override that global configuration on a small subset of routes. | ||
| Answer: In case of HTTP response headers, the headers set in spec.httpHeaders.actions of IngressController will override |
There was a problem hiding this comment.
For the title of this section, I would clarify it by stating: "How do Route headers interact with IngressController headers of the same value?"
Answer: It depends if they are response headers or request headers. In case of HTTP response headers, the headers set in spec.httpHeaders.actions of IngressController will override
| In case of HTTP request headers, the headers set in Route using spec.httpHeaders.actions or annotation will override the | ||
| headers with the same name present in the IngressController's spec.httpHeaders.actions. | ||
|
|
||
| This is because of the way Haproxy is designed to work. We cannot change the processing order; we're constrained by HAProxy's design: |
There was a problem hiding this comment.
nit: use the same capitalization for HAProxy throughout.
| This is because of the way Haproxy is designed to work. We cannot change the processing order; we're constrained by HAProxy's design: | |
| This is because of the way HAProxy is designed to work. We cannot change the processing order; we're constrained by HAProxy's design: |
| response from the backend. | ||
|
|
||
| In case of HTTP response, Haproxy overrides the header value of the same header name in the backend with | ||
| the value of the header defined in the frontend. So, [the backend rules are applied first, followed by the frontend's rules.](https://cbonte.github.io/haproxy-dconv/1.7/configuration.html#4.2-http-response) |
There was a problem hiding this comment.
nit: update the url to point to 2.6 documentation, not 1.7
| However, in case of the HTTP request, haproxy overrides the values of the header value of the same header name present in the frontend with the value of the | ||
| header defined in backend. [The rules are evaluated in their declaration order when they are met in a frontend, | ||
| listen or backend section.](https://cbonte.github.io/haproxy-dconv/1.7/configuration.html#4.2-http-request) | ||
| So, the `http-request set or delete` stanzas are applied and interpreted this way by the haproxy. |
There was a problem hiding this comment.
Add an example of how to explicitly set the headers for a desired result, both for request and response. I assume this means that they have to set request headers on Route and corresponding response headers on IC if they want predictability in the entire transaction.
There was a problem hiding this comment.
predictability can happen only if cluster admin shares what response he is setting on ingress-controller
There was a problem hiding this comment.
they have to set request headers on Route and corresponding response headers ---
You can't set response header in http-request stanzas and vice-versa. haproxy does not allow that.
For example you can't set response header X-Frame-Options in request list as
it will get added in http-request stanza which is not allowed by haproxy.
There was a problem hiding this comment.
Is that what you were testing recently?
There was a problem hiding this comment.
Yes.....I take my words back.
| However, in case of the HTTP request, haproxy overrides the values of the header value of the same header name present in the frontend with the value of the | ||
| header defined in backend. [The rules are evaluated in their declaration order when they are met in a frontend, | ||
| listen or backend section.](https://cbonte.github.io/haproxy-dconv/1.7/configuration.html#4.2-http-request) | ||
| So, the `http-request set or delete` stanzas are applied and interpreted this way by the haproxy. | ||
|
|
||
| #### Does a custom header value defined in RouteSpec override the route annotation ? | ||
| Answer: Yes. A value defined in RouteSpec overrides the route annotation. |
There was a problem hiding this comment.
i would add: except for response headers, where the IngressControllerSpec overrides both.
| gets the response from the backend. | ||
|
|
||
| So, the override behavior happens the way mentioned in the question `Why does a custom header value defined in RouteSpec override IngressControllerSpec value ?` | ||
| under section `Open Questions` where HTTP Response headers defined in the IngressController are overridden by headers |
There was a problem hiding this comment.
Isn't this the behavior for request headers?
| under section `Open Questions` where HTTP Response headers defined in the IngressController are overridden by headers | |
| under section `Open Questions` where HTTP request headers defined in the IngressController are overridden by headers |
| However, if we want the same behavior of override for HTTP response headers as that of HTTP request headers then we will have | ||
| to inject the set-header from the IngressController config into every backend and not the frontend. | ||
|
|
||
| The downside is that it would grow the generated haproxy.config significantly. |
There was a problem hiding this comment.
Can you be more explicit? Would it grow it by one line per backend or would it grow it by one line per affected backend?
There was a problem hiding this comment.
check line 1167 below
There was a problem hiding this comment.
Yes it is there in ### Proposed Alternative Solution for overriding the HTTP response headers defined in the IngressController by the Route
|
|
||
| It would give us better control over the ordering. | ||
|
|
||
| It would eliminate the issue with the host header. So, we could allow it to be set via IngressController. |
There was a problem hiding this comment.
Can you explain a little why it eliminates the issue?
| It would eliminate the issue with the host header. So, we could allow it to be set via IngressController. | |
| Because it prevents _____, we could allow the host header to be set via IngressController. |
| The reason for this proposal was that it's quite common to have a global setting with an escape hatch locally to | ||
| override the global setting. | ||
| The purpose of this EP is to define custom headers. So, there are two reasons we need such override: | ||
| 1. A route owner shall have the choice to define what headers he wants as per his application requirements(as they might be varying) | ||
| than the ones admin has set cluster-wide. | ||
| 2. The IngressController and Route might be owned by the same person who wants to specify some global configuration | ||
| and then override that global configuration on a small subset of routes. | ||
|
|
There was a problem hiding this comment.
This doesn't belong here, it doesn't add to the meaning.
There was a problem hiding this comment.
it does not exist now.
| The purpose of this EP is to define custom headers. So, there are two reasons we need such override: | ||
| 1. A route owner shall have the choice to define what headers he wants as per his application requirements(as they might be varying) | ||
| than the ones admin has set cluster-wide. | ||
| 2. The IngressController and Route might be owned by the same person who wants to specify some global configuration | ||
| and then override that global configuration on a small subset of routes. |
There was a problem hiding this comment.
I would keep this section in here, not move it to the Alternatives section.
There was a problem hiding this comment.
i think this comes under upside of route overriding IC
There was a problem hiding this comment.
i think we will need to remove this as we no-longer use this over-ride behavior where route over-rides ingresscontroller in all cases.
| For example: If someone creates an Ingress Controller with the following: | ||
| ```sh | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: IngressController | ||
| metadata: | ||
| name: default | ||
| namespace: openshift-ingress-operator | ||
| spec: | ||
| httpHeaders: | ||
| forwardedHeaderPolicy: Replace | ||
| ``` | ||
| Or if someone add a route annotation `haproxy.router.openshift.io/set-forwarded-headers`, | ||
| then the `set-header` stanzas will be added in `haproxy.config` using the [haproxy-config.template](https://github.com/openshift/router/blob/0cd9a58dff77127562f7851ccfa5cda79b8cef48/images/router/haproxy/conf/haproxy-config.template#L565-L580) | ||
| In contrast to request headers, in which the IngressController values override the Route spec.httpHeaders.actions values, | ||
| for response headers the Route values override the IngressController values. | ||
|
|
||
| However, if someone wants to set custom values for these then they need to use the API of this EP. | ||
|
|
||
| The custom values set using `spec.httpHeaders.actions` will take over the value defined in annotation `haproxy.router.openshift.io/set-forwarded-headers` or `spec.httpHeaders.forwardedHeaderPolicy`. | ||
|
|
||
| Please note that the setting of forwardedHeaderPolicy happens at the plain backend section of haproxy.config i.e. per route level and not at the front-end section | ||
| of the haproxy.config level as per the above-mentioned link. | ||
| The custom HTTP response value of `x-forwarded` header set using `spec.httpHeaders.actions` of the IngressController will take over the value defined in annotation `haproxy.router.openshift.io/set-forwarded-headers` of Route, `spec.httpHeaders.forwardedHeaderPolicy` of Ingress Controller | ||
| or `spec.httpHeaders.actions` of Route as in case of HTTP response headers, HAProxy overrides the value of the backend of same header name which | ||
| was present in the frontend section. |
There was a problem hiding this comment.
Without this enhancement, does HAProxy ever set x-forwarded-* in response headers? The x-forwarded-* headers are request headers; given that, none of what you wrote makes any sense.
There was a problem hiding this comment.
ok. i tried to update this.
| @@ -791,6 +789,7 @@ The following table summarizes which headers are allowed to be customized via th | |||
| | `strict-transport-security` | No | No | `haproxy.router.openshift.io/hsts_header` Route annotation | | |||
| | `x-forwarded-for` | Yes | Yes | `haproxy.router.openshift.io/set-forwarded-headers` Route annotation or IngressController `spec.httpHeaders.forwardedHeaderPolicy` API field | | |||
| | `x-ssl` | Yes | Yes | No | | |||
| | `cookie` or `set-cookie` | No | No | No | | |||
There was a problem hiding this comment.
We do have the haproxy.router.openshift.io/disable_cookies and router.openshift.io/cookie_name route annotations that you could list in this table.
| In case of HTTP response headers, the headers set in spec.httpHeaders.actions of IngressController will override | ||
| the headers with same name in Route's spec.httpHeaders.actions or Route annotation. |
There was a problem hiding this comment.
It is more precise to describe the order in which actions are executed. Also, change "In case" to "In the case".
| In case of HTTP response headers, the headers set in spec.httpHeaders.actions of IngressController will override | |
| the headers with same name in Route's spec.httpHeaders.actions or Route annotation. | |
| In case of HTTP response headers, the actions specified in `spec.httpHeaders.actions` on the IngressController will be executed after | |
| the actions specified in the Route's `spec.httpHeaders.actions` field. |
Please remember to use backticks when referencing API fields.
There was a problem hiding this comment.
I agree - please describe the order in which the actions are executed.
| In case of HTTP request headers, the headers set in Route using spec.httpHeaders.actions or annotation will override the | ||
| headers with the same name present in the IngressController's spec.httpHeaders.actions. |
There was a problem hiding this comment.
| In case of HTTP request headers, the headers set in Route using spec.httpHeaders.actions or annotation will override the | |
| headers with the same name present in the IngressController's spec.httpHeaders.actions. | |
| In the case of HTTP request headers, the actions specified in `spec.httpHeaders.actions` on the Route will be executed after | |
| the actions specified in the IngressController's `spec.httpHeaders.actions` field. |
|
|
||
| In case of HTTP response, HAProxy overrides the header value of the same header name in the backend with | ||
| the value of the header defined in the frontend. So, [the backend rules are applied first, followed by the frontend's rules.](https://cbonte.github.io/haproxy-dconv/2.6/configuration.html#4.2-http-response) |
There was a problem hiding this comment.
The current phrasing seems confusingly repetitive.
| In case of HTTP response, HAProxy overrides the header value of the same header name in the backend with | |
| the value of the header defined in the frontend. So, [the backend rules are applied first, followed by the frontend's rules.](https://cbonte.github.io/haproxy-dconv/2.6/configuration.html#4.2-http-response) | |
| This means that for requests, the frontend's rules are applied first, followed by the backend's, | |
| and conversely, for responses, [the backend's rules are applied first, followed by the frontend's](https://cbonte.github.io/haproxy-dconv/2.6/configuration.html#4.2-http-response). |
| In case of HTTP response, HAProxy overrides the header value of the same header name in the backend with | ||
| the value of the header defined in the frontend. So, [the backend rules are applied first, followed by the frontend's rules.](https://cbonte.github.io/haproxy-dconv/2.6/configuration.html#4.2-http-response) | ||
| The `http-response set or delete` stanzas are applied and interpreted in this way by the HAProxy. | ||
| For example: A cluster admin wants to `DENY` response header `X-Frame-Options`. However, a route owner sets `X-Frame-Options` to `'SAMEORIGIN'` then the haproxy.config will look like the following where |
There was a problem hiding this comment.
The current phrasing seems to suggest that the cluster-admin can deny the router owner from setting a header.
| For example: A cluster admin wants to `DENY` response header `X-Frame-Options`. However, a route owner sets `X-Frame-Options` to `'SAMEORIGIN'` then the haproxy.config will look like the following where | |
| For example: A cluster admin sets a response header with the name `X-Frame-Options` and the value `DENY` using the IngressController API. However, a route owner sets the `X-Frame-Options` header with the value `SAMEORIGIN`. In this scenario, `haproxy.config` will look like the following where |
| header defined in backend. [The rules are evaluated in their declaration order when they are met in a frontend, | ||
| listen or backend section.](https://cbonte.github.io/haproxy-dconv/2.6/configuration.html#4.2-http-request) |
There was a problem hiding this comment.
How is this statement relevant here?
There was a problem hiding this comment.
I just tried to give more information. However I removed this and added a link of http-request -
This means that for requests, the frontend's rules are applied first, followed by the backend's,
and conversely,
| There might be a case, where admin wants to set a response header for all connections to the IngressController as per company policy which he won't be able to do so | ||
| using this approach. | ||
| For Example: A cluster admin wants to deny X-Frame-Options for all routes the ingress controller is handling. |
There was a problem hiding this comment.
This seems like a critical advantage to the current design.
| There might be a case, where admin wants to set a response header for all connections to the IngressController as per company policy which he won't be able to do so | |
| using this approach. | |
| For Example: A cluster admin wants to deny X-Frame-Options for all routes the ingress controller is handling. | |
| There might be a case, where admin wants to set a response header for all connections to the IngressController as per company policy which he won't be able to do so | |
| using this approach. | |
| For Example: A cluster admin wants to set the `X-Frame-Options` header with the value `DENY` for all routes the ingress controller is handling, overwriting any existing `X-Frame-Options` header that may have been set by the application or Route configuration. |
| The upside is: | ||
|
|
||
| It would give us better control over the ordering. |
There was a problem hiding this comment.
Upside of what? What would? Give whom control? Don't force the reader to guess.
| The upside is: | |
| It would give us better control over the ordering. | |
| The upside of putting all the header configuration on the backends is that | |
| it would give OpenShift router full control over the ordering of header actions. |
There was a problem hiding this comment.
sorry. thanks for the improvement!
|
@openshift-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/reopen |
|
@miheer: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@miheer: This pull request references NE-1366 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Rotten enhancement proposals close after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Reopen the proposal by commenting /close |
|
@openshift-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
(automated message) This pull request is closed with lifecycle/rotten. The associated Jira ticket, NE-982, has status "Closed, Done". Should the PR be reopened, updated, and merged? If not, removing the lifecycle/rotten label will tell this bot to ignore it in the future. |
dhellmann
removed
the
lifecycle/rotten
|
/reopen |
|
@miheer: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@miheer: This pull request references NE-1366 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@miheer: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
#1555 is changing the enhancement template in a way that will cause the header check in the linter job to fail for existing PRs. If this PR is merged within the development period for 4.16 you may override the linter if the only failures are caused by issues with the headers (please make sure the markdown formatting is correct). If this PR is not merged before 4.16 development closes, please update the enhancement to conform to the new template. |
|
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale enhancement proposals rot after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
Rotten enhancement proposals close after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Reopen the proposal by commenting /close |
|
@openshift-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
(automated message) This pull request is closed with lifecycle/rotten. The associated Jira ticket, NE-982, has status "Closed, Done". Should the PR be reopened, updated, and merged? If not, removing the lifecycle/rotten label will tell this bot to ignore it in the future. |
5 similar comments
|
(automated message) This pull request is closed with lifecycle/rotten. The associated Jira ticket, NE-982, has status "Closed, Done". Should the PR be reopened, updated, and merged? If not, removing the lifecycle/rotten label will tell this bot to ignore it in the future. |
|
(automated message) This pull request is closed with lifecycle/rotten. The associated Jira ticket, NE-982, has status "Closed, Done". Should the PR be reopened, updated, and merged? If not, removing the lifecycle/rotten label will tell this bot to ignore it in the future. |
|
(automated message) This pull request is closed with lifecycle/rotten. The associated Jira ticket, NE-982, has status "Closed, Done". Should the PR be reopened, updated, and merged? If not, removing the lifecycle/rotten label will tell this bot to ignore it in the future. |
|
(automated message) This pull request is closed with lifecycle/rotten. The associated Jira ticket, NE-982, has status "Closed, Done". Should the PR be reopened, updated, and merged? If not, removing the lifecycle/rotten label will tell this bot to ignore it in the future. |
|
(automated message) This pull request is closed with lifecycle/rotten. The associated Jira ticket, NE-982, has status "Closed, Done". Should the PR be reopened, updated, and merged? If not, removing the lifecycle/rotten label will tell this bot to ignore it in the future. |
This a followup commit for PR #1296
It adds changes related to Cookies as to why they are in the deny list. It mentions the correct override behavior when setting same headers on Ingress Controller and Route.
It also adds an alternative solution for the override behavior.