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.

Sign up for GitHub

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

api: Support Server header transformations #2500

Merged
merged 4 commits into from
Feb 9, 2024
Merged

api: Support Server header transformations #2500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8fb86d2
api: support Server header transformations
liorokman Jan 25, 2024
21a4036
Fix typo, make linter happy
liorokman Jan 25, 2024
e775ba9
Getting the API into shape based on the reviews.
liorokman Feb 7, 2024
b771af6
Removed the unneeded enum.
liorokman Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions api/v1alpha1/clienttrafficpolicy_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,6 @@ type ClientTrafficPolicySpec struct {
//
// +optional
TCPKeepalive *TCPKeepalive `json:"tcpKeepalive,omitempty"`
// SuppressEnvoyHeaders configures the Envoy Router filter to suppress the "x-envoy-'
// headers from both requests and responses.
// By default these headers are added to both requests and responses.
//
// +optional
SuppressEnvoyHeaders *bool `json:"suppressEnvoyHeaders,omitempty"`
// EnableProxyProtocol interprets the ProxyProtocol header and adds the
// Client Address into the X-Forwarded-For header.
// Note Proxy Protocol must be present when this field is set, else the connection
Expand Down Expand Up @@ -86,6 +80,18 @@ type ClientTrafficPolicySpec struct {
//
// +optional
HTTP1 *HTTP1Settings `json:"http1,omitempty"`
// HeaderSettings provides configuration for header management.
//
// +optional
Headers *HeaderSettings `json:"headers,omitempty"`
}

// HeaderSettings providess configuration options for headers on the listener.
type HeaderSettings struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we discussed this in the community meeting today, here are some follow up questions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we discussed this in the community meeting today, here are some follow up questions

No. The ResponseHeaderFilter unconditionally adds/overwrites a header whose value is given in the filter. This feature's passThrough option will keep the value of the Server header provided by the upstream, and there's no way to express the appendIfAbsent option with ResponseHeaderFilter.

  • can the suppressEnvoyHeaders be represented with another name closer to the goal of the field such as passThrough etc ?

You meant serverHeaderTransformation, not suppressEnvoyHeaders, right? suppressEnvoyHeaders has nothing to do with passing anything through and isn't the topic of this PR.

I used the name of the underlying feature in Envoy Proxy for the serverHeaderTransformation field. I think passThrough isn't a good name - it's only one of three options, and is exactly the opposite of the overwrite option.

I'm open to any other name you have that you think fits better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from a user perspective, (correct me if im wrong), what you're really trying to solve here is - hey envoy, dont add any new response headers like x-envoy-* or server
we should come up with a name for this knob to define this opt in behavior

Copy link
Contributor

@arkodg arkodg Feb 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about a knob called addEnvoyResponseHeaders, making adding envoy headers along with server as opt in, so we supress by default

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe istio/api#2240 will give a little help.

Copy link
Contributor

@arkodg arkodg Feb 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for sharing this @zirain, this is super useful
how about removing this API and disabling

  • x-envoy-*
  • server (passthrough)
    by default

and we can introduce APIs in the future to opt into this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine to me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, to summarise the above:

  1. The HeaderSettings structure is removed, and the default behaviour becomes that the X-Envoy- headers are removed and the Server header is defined as passthrough. This is done to try to effectively hide the presence of an Envoy Proxy between the client and the upstream target.
  2. There is no support at this time for allowing the X-Envoy headers. This makes it harder for developers to perform certain tasks, for example when trying to measure latency with the x-envoy-upstream-service-time, or debugging various rewrite rules with the x-envoy-original-path header.

How to proceed? Should I close this PR and create a new PR removing the suppressEnvoyHeaders option and implementing the behavioural changes listed here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @liorokman, agree with 1.
for 2. we can make a opt in API e.g.

headers:
  enableEnvoyHeaders: true
``

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to reflect this thread.

This PR changes the API and keeps the project building, a different PR will actually implement the feature.

// EnableEnvoyHeaders configures Envoy Proxy to add the "X-Envoy-" headers to requests
// and responses.
// +optional
EnableEnvoyHeaders *bool `json:"enableEnvoyHeaders,omitempty"`
}

// ClientIPDetectionSettings provides configuration for determining the original client IP address for requests.
Expand Down
30 changes: 25 additions & 5 deletions api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 8 additions & 5 deletions charts/gateway-helm/crds/generated/gateway.envoyproxy.io_clienttrafficpolicies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,14 @@ spec:
Proxy Protocol must be present when this field is set, else the
connection is closed.
type: boolean
headers:
description: HeaderSettings provides configuration for header management.
properties:
enableEnvoyHeaders:
description: EnableEnvoyHeaders configures Envoy Proxy to add
the "X-Envoy-" headers to requests and responses.
type: boolean
type: object
http1:
description: HTTP1 provides HTTP/1 configuration on the listener.
properties:
Expand Down Expand Up @@ -106,11 +114,6 @@ spec:
- UnescapeAndRedirect
type: string
type: object
suppressEnvoyHeaders:
description: SuppressEnvoyHeaders configures the Envoy Router filter
to suppress the "x-envoy-' headers from both requests and responses.
By default these headers are added to both requests and responses.
type: boolean
targetRef:
description: TargetRef is the name of the Gateway resource this policy
is being attached to. This Policy and the TargetRef MUST be in the
Expand Down
11 changes: 6 additions & 5 deletions internal/gatewayapi/clienttrafficpolicy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,8 +295,8 @@
// Translate Client IP Detection
translateClientIPDetection(policySpec.ClientIPDetection, httpIR)

// Translate Suppress Envoy Headers
translateListenerSuppressEnvoyHeaders(policySpec.SuppressEnvoyHeaders, httpIR)
// Translate Header Settings
translateListenerHeaderSettings(policySpec.Headers, httpIR)

// Translate Path Settings
translatePathSettings(policySpec.Path, httpIR)
Expand Down Expand Up @@ -387,10 +387,11 @@
httpIR.ClientIPDetection = (*ir.ClientIPDetectionSettings)(clientIPDetection)
}

func translateListenerSuppressEnvoyHeaders(suppressEnvoyHeaders *bool, httpIR *ir.HTTPListener) {
if suppressEnvoyHeaders != nil {
httpIR.SuppressEnvoyHeaders = *suppressEnvoyHeaders
func translateListenerHeaderSettings(headerSettings *egv1a1.HeaderSettings, httpIR *ir.HTTPListener) {
if headerSettings == nil {
arkodg marked this conversation as resolved.
Show resolved Hide resolved
return
}
httpIR.SuppressEnvoyHeaders = true

Check warning on line 394 in internal/gatewayapi/clienttrafficpolicy.go

View check run for this annotation

Codecov / codecov/patch

internal/gatewayapi/clienttrafficpolicy.go#L394 

Added line #L394 was not covered by tests
}

func translateHTTP1Settings(http1Settings *egv1a1.HTTP1Settings, httpIR *ir.HTTPListener) {
Expand Down
35 changes: 0 additions & 35 deletions internal/gatewayapi/testdata/clienttrafficpolicy-suppress-envoy-headers.in.yaml
View file
Open in desktop

This file was deleted.

140 changes: 0 additions & 140 deletions internal/gatewayapi/testdata/clienttrafficpolicy-suppress-envoy-headers.out.yaml
View file
Open in desktop

This file was deleted.

16 changes: 15 additions & 1 deletion site/content/en/latest/api/extension_types.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,13 +291,13 @@ _Appears in:_
| --- | --- | --- | --- |
| `targetRef` | _[PolicyTargetReferenceWithSectionName](https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1alpha2.PolicyTargetReferenceWithSectionName)_ | true | TargetRef is the name of the Gateway resource this policy is being attached to. This Policy and the TargetRef MUST be in the same namespace for this Policy to have effect and be applied to the Gateway. TargetRef |
| `tcpKeepalive` | _[TCPKeepalive](#tcpkeepalive)_ | false | TcpKeepalive settings associated with the downstream client connection. If defined, sets SO_KEEPALIVE on the listener socket to enable TCP Keepalives. Disabled by default. |
| `suppressEnvoyHeaders` | _boolean_ | false | SuppressEnvoyHeaders configures the Envoy Router filter to suppress the "x-envoy-' headers from both requests and responses. By default these headers are added to both requests and responses. |
| `enableProxyProtocol` | _boolean_ | false | EnableProxyProtocol interprets the ProxyProtocol header and adds the Client Address into the X-Forwarded-For header. Note Proxy Protocol must be present when this field is set, else the connection is closed. |
| `clientIPDetection` | _[ClientIPDetectionSettings](#clientipdetectionsettings)_ | false | ClientIPDetectionSettings provides configuration for determining the original client IP address for requests. |
| `http3` | _[HTTP3Settings](#http3settings)_ | false | HTTP3 provides HTTP/3 configuration on the listener. |
| `tls` | _[TLSSettings](#tlssettings)_ | false | TLS settings configure TLS termination settings with the downstream client. |
| `path` | _[PathSettings](#pathsettings)_ | false | Path enables managing how the incoming path set by clients can be normalized. |
| `http1` | _[HTTP1Settings](#http1settings)_ | false | HTTP1 provides HTTP/1 configuration on the listener. |
| `headers` | _[HeaderSettings](#headersettings)_ | false | HeaderSettings provides configuration for header management. |



Expand Down Expand Up @@ -1177,6 +1177,20 @@ _Appears in:_



#### HeaderSettings



HeaderSettings providess configuration options for headers on the listener.

_Appears in:_
- [ClientTrafficPolicySpec](#clienttrafficpolicyspec)

| Field | Type | Required | Description |
| --- | --- | --- | --- |
| `enableEnvoyHeaders` | _boolean_ | false | EnableEnvoyHeaders configures Envoy Proxy to add the "X-Envoy-" headers to requests and responses. |


#### HealthCheck


Expand Down
Loading