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

proposal: ClientTrafficPolicy #1846

Closed
Alice-Lilith opened this issue Aug 29, 2023 · 9 comments
Closed

proposal: ClientTrafficPolicy #1846

Alice-Lilith opened this issue Aug 29, 2023 · 9 comments
Assignees
Labels
area/api API-related issues area/policy kind/enhancement New feature or request kind/question Further information is requested no stalebot
Milestone

Comments

@Alice-Lilith
Copy link
Member

Alice-Lilith commented Aug 29, 2023

Relates to

#1492
#1821
#1845

What is this?

ClientTrafficPolicy is a proposal for a new policy attachment resource that can be applied to Gateway resources. It is meant to supplement a lot of config that Gateway API currently lacks while also providing a way to specify global defaults and is the counterpart to the other proposed BackendTrafficPolicy resource.

How would I use this resource?

You can attach a ClientTrafficPolicy to a Gateway to configure behaviour for how Envoy Proxy communicates with downstream clients.

What if Gateway API implements a GEP that delivers the same functionality as some of the fields of this resource?

If Gateway improves any of their existing resources to deliver functionality that meets all of the needs of any of the below config,
then we will deprecate the field in this resource and use the Gateway API config instead. Ideally, a lot of this config can be
upstreamed into Gateway API in some form eventually since this resource only exists to solve areas where Gateway API is lacking for the needs of Envoy Gateway.

Can you use multiple ClientTrafficPolicy resources at the same time?

You may use multiple ClientTrafficPolicy that target different Gateways, but you may not attach multiple ClientTrafficPolicies to the same Gateway. Merging config in this scenario would quickly become convoluted and confusing, and there are many edge-case scenarios that make supporting this undesirable.

How will this be developed/implemented?

This issue serves as a proposal for the high-level design and fields of the API, but that does not mean that every field included below will be immediately available. My plan for the implementation is to add and implement one high-level field at a time until the whole resource is implemented.

API outline

---
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: ClientTrafficPolicy
metadata:
  name: example-policy
spec:
  tls: # optional
    clientCerts: # optional
      forwarding: Enum(sanitize/forward-only/append-forward/sanitize-set/always-forward-only) # required
      setDetails: string # optional
      requireClientCert: bool  # optional
    version: # optional
      min: string # optional
      max: string # optional
    cipers: []Enum(list of supported cipher suites...) # optional
    ecdhCurves: []string # optional
    alpnProtocols: []Enum(h2/http-1.1) # optional
    sniOverride: string # optional
    certificateRevocationList: # optional
      secretRef: # required
        name: string # required
        namespace: # optional
  protocols: # optional
    protocolOrder: []Enum(http/https/proxy/tcp/tls/udp)
    # How we consider incoming requests to be originally using https or not
    httpsTrustModel: Enum(x-forwarded-proto/secure/insecure)
    http10: # optional
      enabled: bool # required, default=false
      defaultHost: string # optional
      enableTrailers: bool bool # optional, default=false
    grpc: # optional
      enableGRPCHttp11Bridge: bool # optional, default=false
      enableGRPCWeb: bool # optional, default=false
  requests: # optional
    path: # optional
      mergeAdjacentSlashes: bool # optional, default=false
      escapedSlashesAction: Enum(keep/reject/unescape-redirect/unescape-forward)
    headers: # optional
      serverName: string # optional
      allowChunkedLength: bool # optional, default=false
      maxHeaderCount: int # optional, default=100
      maxHeaderKbs: int # optional, default=60
      underscoreInNameAction: Enum(allow/reject/drop-header) # optional, default=allow
      stripMatchingHostPort: bool # optional, default=false
      stripAnyHostPort: bool # optional, deftault=false
      preserveExternalRequestId: bool # optional, default=false
  responses: # optional
    headers: # optional
      suppressEnvoyHeaders: bool # optional, deftault=false
    body: # optional
      bufferBytesLimit: int # optional, default=1MiB
  connections: # optional
    clientIps: # optional
      useXForwardedFor: bool # optional, default=false
      l7ProxyCount: int # optional, default=0
    # keepAliveProbes are also on the BackendTrafficPolicy. Here they will probe the downstream client,
    # on the BackendTrafficPolicy they will probe the backend service.
    keepAliveProbes: # optional
      idleTime: duration # optional, default=7200s
      interval: duration # optional, default=75s
      idleInterval: duration # optional, default=0s (not enabled)
  targetRef: # This struct is defined as part of Gateway API
    group: "" # Empty string means core - this is a standard convention
    kind: Service
    name: fooService

Open questions

  • This resource can be attached to a Gateway, but you can configure multiple listeners on a single Gateway. Realistically, this resource makes the most sense targeting listeners instead of forcing this to apply to the entire gateway. There should be a way to specify which listeners it applies to so that you can have different config for each listener.
  • Gateway API already provides a way to support additional tls settings via the options field on the listener. We could add support for a new annotation there such as gateway.envoyproxy.io/config-from-clienttrafficpolicy: true or similar to defer logic here, but I'm open to ideas on how to handle this.
  • Additionally, some of the fields such as protocolOrder would technically overwrite Gateway's protocol field. Is this fine, or do we want to handle these cases differently?

Edits / Changes

Renamed DownstreamTrafficPolicy to ClientTrafficPolicy to reduce potential user confusion about the types of traffic this resource will affect.

@Alice-Lilith Alice-Lilith added the kind/enhancement New feature or request label Aug 29, 2023
@Alice-Lilith Alice-Lilith added area/api API-related issues kind/question Further information is requested no stalebot labels Aug 29, 2023
@Alice-Lilith Alice-Lilith self-assigned this Aug 29, 2023
@arkodg
Copy link
Contributor

arkodg commented Aug 30, 2023

thanks for designing this @AliceProxy ! im a +1 on the Name (DownstreamTrafficPolicy), Type of Policy Attachment (Direct) and the Resource its attached to (Gateway ) .
The above issue defines a lot of fields that already intersect with some defaults we have in Envoy or fields defined upstream. My vote would be to start off with most asked features such as proxyProtocol & keepAlive that have been asked by numerous users.

@Alice-Lilith
Copy link
Member Author

thanks for designing this @AliceProxy ! im a +1 on the Name (DownstreamTrafficPolicy), Type of Policy Attachment (Direct) and the Resource its attached to (Gateway ) . The above issue defines a lot of fields that already intersect with some defaults we have in Envoy or fields defined upstream. My vote would be to start off with most asked features such as proxyProtocol & keepAlive that have been asked by numerous users.

Yeah I think starting with keepAlive is fine. I was just trying to provide an "initial"/"example" API to showcase what all these other fields we probably need to support at some point to at least have feature parity with other solutions like Emissary might look like. After all this is just a rough proposal and I'm sure there will be further refinement when I go to file implementation PRs (probably one top-level field at a time).

@arkodg
Copy link
Contributor

arkodg commented Aug 30, 2023

sg @AliceProxy do you want to convert this issue into a design doc & API or wait until agreement on UpstreamTrafficPolicy so we've covered all the bases before proceeding ? im fine either ways

@arkodg
Copy link
Contributor

arkodg commented Aug 30, 2023

one more question around overlapping intent (which doesnt need to be tackled now since the problem doesnt exist today) - what happens when a user defines a DownstreamTrafficPolicy for the entire Gateway as well as another policy for a specific listener within the Gateway,

  • does the most specific policy win ?

imo multiple DownstreamTrafficPolicy on the same Gateway or the same Gateway listener should not be supported

@Alice-Lilith
Copy link
Member Author

one more question around overlapping intent (which doesnt need to be tackled now since the problem doesnt exist today) - what >happens when a user defines a DownstreamTrafficPolicy for the entire Gateway as well as another policy for a specific listener >within the Gateway,

does the most specific policy win ?

imo multiple DownstreamTrafficPolicy on the same Gateway or the same Gateway listener should not be supported

Great question. I would say that we limit it to one per listener. That way if you want a unique one for each listener then you can do that. If you have one attached to the entire Gateway then you cannot attach another to the Gateway or it's listeners.

@Alice-Lilith Alice-Lilith changed the title proposal: DownstreamTrafficPolicy proposal: InboundTrafficPolicy Sep 14, 2023
@arkodg
Copy link
Contributor

arkodg commented Sep 14, 2023

one more question around overlapping intent (which doesnt need to be tackled now since the problem doesnt exist today) - what >happens when a user defines a DownstreamTrafficPolicy for the entire Gateway as well as another policy for a specific listener >within the Gateway,
does the most specific policy win ?
imo multiple DownstreamTrafficPolicy on the same Gateway or the same Gateway listener should not be supported

Great question. I would say that we limit it to one per listener. That way if you want a unique one for each listener then you can do that. If you have one attached to the entire Gateway then you cannot attach another to the Gateway or it's listeners.

@AliceProxy this has been addressed upstream and is part of the PolicyAttachment GEP thanks to @zhaohuabing !
and here's the relevant snippet

When multiple Policies of the same type target the same object, one with a sectionName specified, and one without, the one with a sectionName is more specific, and so will have all its settings apply. The less-specific Policy will not attach to the target.

@arkodg
Copy link
Contributor

arkodg commented Sep 14, 2023

im hoping the first few features/fields added to this API are

@arkodg
Copy link
Contributor

arkodg commented Sep 14, 2023

thanks for updating the API name @AliceProxy
was chatting with @ZackButcher earlier today who recommended we use InboundTrafficPolicy or ListenerTrafficPolicy, since DownstreamTrafficPolicy might be too technical & confusing for the end user

@arkodg arkodg added this to the 0.6.0-rc1 milestone Sep 21, 2023
@kflynn
Copy link
Contributor

kflynn commented Sep 21, 2023

Proxy protocol is an interesting one -- @arkodg and I talked about this in the Envoy Gateway API meeting today, and there are interesting interactions between proxy here and in the Gateway's Listener. I'll try to talk to some of the Gateway API folks about this.

@arkodg arkodg changed the title proposal: InboundTrafficPolicy proposal: ClientTrafficPolicy Sep 27, 2023
arkodg added a commit to arkodg/gateway that referenced this issue Sep 27, 2023
Relates to envoyproxy#1846

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg self-assigned this Sep 27, 2023
arkodg added a commit to arkodg/gateway that referenced this issue Sep 27, 2023
* adds base API + CRD
* adding k8s provider reconciliation
* add skeleton in gateway-api translation layer

Relates to envoyproxy#1846

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
arkodg added a commit to arkodg/gateway that referenced this issue Sep 29, 2023
* adds base API + CRD
* adding k8s provider reconciliation
* add skeleton in gateway-api translation layer

Relates to envoyproxy#1846

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
zirain pushed a commit that referenced this issue Oct 3, 2023
* design: ClientTrafficPolicy

Relates to #1846

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* lint

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* address open question

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* lint

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* fix field indentation

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

---------

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
arkodg added a commit that referenced this issue Oct 3, 2023
* feat: ClientTrafficPolicy

* adds base API + CRD
* adding k8s provider reconciliation
* add skeleton in gateway-api translation layer

Relates to #1846

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* validate policy

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* conditions and tests

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
arkodg added a commit to arkodg/gateway that referenced this issue Oct 10, 2023
Relates to envoyproxy#1846

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api API-related issues area/policy kind/enhancement New feature or request kind/question Further information is requested no stalebot
Projects
None yet
Development

No branches or pull requests

4 participants