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

Support downstream and upstream Proxy Protocol #1328

Closed
JuniorJPDJ opened this issue Apr 18, 2023 · 26 comments
Closed

Support downstream and upstream Proxy Protocol #1328

JuniorJPDJ opened this issue Apr 18, 2023 · 26 comments
Assignees
Labels
area/api API-related issues area/policy area/translator Issues related to Gateway's translation service, e.g. translating Gateway APIs into the IR. kind/enhancement New feature or request
Milestone

Comments

@JuniorJPDJ
Copy link
Contributor

JuniorJPDJ commented Apr 18, 2023

Envoy Gateway should support Proxy Protocol on traffic coming from the world to support for example cases of Network Load Balancers from some cloud providers (AWS example from Emissary Ingress docs) and other cases when we have some sort of L4 LB in front of the K8S cluster.

It should also support Proxy Protocol on egress traffic to support cases where eg. pointing to ExternalService outside of the cluster or we just need raw L4 socket (TCP or UDP) and source IP preservation.

I actually need the second case, but I know there are lots of people needing first one due to cloud deployments with L4 LBs in front of the k8s.

Istio for example has 2 issues for upstream proxy proto (istio/istio#42257, istio/istio#44342) and some forum posts from people implementing it with EnvoyFilter mechanism and it also has example of configuration in docs for downstream proxy proto due to cloud usage.

Upstream proxy protocol would for example allow placing nginx ingress controler behind Envoy Gateway without losing the source IPs when someone needs legacy ingress resource support: https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/configmap/#use-proxy-protocol

@arkodg
Copy link
Contributor

arkodg commented Apr 19, 2023

thanks for raising this issue @JuniorJPDJ, ive raised a issue in the upstream API repo

@JuniorJPDJ
Copy link
Contributor Author

Can you please link this issue here?

@arkodg
Copy link
Contributor

arkodg commented Apr 19, 2023

@JuniorJPDJ
Copy link
Contributor Author

Sure.
But IMO it should be extension, not part of main spec.
Lets see what upstream will say :D

@arkodg
Copy link
Contributor

arkodg commented Apr 19, 2023

@JuniorJPDJ yes an extension (policyAttachment or Filter) might be the way to go if the feature is not supported by the native upstream API

@haq204
Copy link
Contributor

haq204 commented Apr 19, 2023

Not sure if a policyAttachment or Filter would be necessary since you could also introduce it as extended protocol types in Gateway.listeners[].protocol (e.g. HTTPPROXY, HTTPSPROXY, etc.).

This is allowed under the existing spec:

Implementations can define their own protocols if a core ProtocolType does not exist. Such definitions must use prefixed name, such as mycompany.com/my-custom-protocol. Un-prefixed names are reserved for core protocols. Any protocol defined by implementations will fall under Implementation-specific conformance.

@arkodg
Copy link
Contributor

arkodg commented Apr 19, 2023

hey @haq204 thats a really good idea
+1 to it
now the protocol name is being overloaded with 3 nouns - HTTP + S + PROXYPROTOCOL :)

@JuniorJPDJ
Copy link
Contributor Author

But this is a solution for just a downstream proxy proto.

@haq204
Copy link
Contributor

haq204 commented Apr 19, 2023

I think for upstream proxy it would probably would need to be a Filter/PolicyAttachment but not really keen on that idea. There is an effort upstream to expand backend properties kubernetes-sigs/gateway-api#1282 which would include protocol selection kubernetes-sigs/gateway-api#1911 so upstream proxy would seem to be a more natural fit here.

@haq204
Copy link
Contributor

haq204 commented Apr 19, 2023

hey @haq204 thats a really good idea +1 to it now the protocol name is being overloaded with 3 nouns - HTTP + S + PROXYPROTOCOL :)

Maybe there should be a protocolStack option as well :)

protocolStack: []Protocol

or a useProxyProtocol flag

@arkodg
Copy link
Contributor

arkodg commented Apr 19, 2023

for upstream, appProtocol within a service may be used https://kubernetes.io/docs/concepts/services-networking/service/#application-protocol to infer upstream protocol type, but this makes things a little magical(mightt surprise the user) and are tied to Service kind, we are also now in the realm of custom protocol naming

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

@github-actions github-actions bot added the stale label May 20, 2023
@JuniorJPDJ
Copy link
Contributor Author

not stale

@github-actions github-actions bot removed the stale label May 21, 2023
@github-actions
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

@github-actions github-actions bot added the stale label Jun 20, 2023
@JuniorJPDJ
Copy link
Contributor Author

not stale

@github-actions
Copy link

github-actions bot commented Sep 3, 2023

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

@github-actions github-actions bot added the stale label Sep 3, 2023
@hzxuzhonghu
Copy link

not stale

@github-actions
Copy link

github-actions bot commented Oct 4, 2023

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

@github-actions github-actions bot added the stale label Oct 4, 2023
@arkodg
Copy link
Contributor

arkodg commented Oct 4, 2023

downstream proxy protocol can be added to the ClientTrafficPolicy API which is now available https://gateway.envoyproxy.io/latest/api/extension_types.html#clienttrafficpolicy and upstream proxy protocol can be added to the BackendTrafficPolicy API which should be in soon

@arkodg arkodg added this to the Backlog milestone Oct 4, 2023
arkodg added a commit to arkodg/gateway that referenced this issue Nov 14, 2023
Relates to envoyproxy#1328

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg removed the help wanted Extra attention is needed label Nov 14, 2023
@arkodg arkodg self-assigned this Nov 14, 2023
arkodg added a commit to arkodg/gateway that referenced this issue Nov 14, 2023
Relates to envoyproxy#1328

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Xunzhuo pushed a commit that referenced this issue Nov 15, 2023
Relates to #1328

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
arkodg added a commit to arkodg/gateway that referenced this issue Nov 15, 2023
relates to envoyproxy#1328

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
zirain pushed a commit that referenced this issue Nov 17, 2023
Relates to #1328

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
arkodg added a commit to arkodg/gateway that referenced this issue Nov 17, 2023
relates to envoyproxy#1328

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
arkodg added a commit to arkodg/gateway that referenced this issue Nov 17, 2023
relates to envoyproxy#1328

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
arkodg added a commit to arkodg/gateway that referenced this issue Nov 17, 2023
relates to envoyproxy#1328

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
arkodg added a commit to arkodg/gateway that referenced this issue Nov 20, 2023
relates to envoyproxy#1328

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
zirain added a commit that referenced this issue Nov 21, 2023
* feat: proxy protocol in ClientTrafficPolicy

relates to #1328

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

* address review comments

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

---------

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Co-authored-by: zirain <zirain2009@gmail.com>
arkodg added a commit to arkodg/gateway that referenced this issue Nov 22, 2023
relates to envoyproxy#1328

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
arkodg added a commit to arkodg/gateway that referenced this issue Nov 23, 2023
relates to envoyproxy#1328

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
arkodg added a commit that referenced this issue Nov 23, 2023
* feat: proxyProtocol in BackendTrafficPolicy

relates to #1328

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

* xds

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

---------

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg closed this as completed Nov 23, 2023
@JuniorJPDJ
Copy link
Contributor Author

JuniorJPDJ commented Mar 15, 2024

@arkodg thank you very much for implementing it.
I have problem now, that it doesn't work.. ;o
Which is even more weird as... well...
Generated config seems good...

apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
  generation: 1
  labels:
    argocd.argoproj.io/instance: cluster-config
  name: internal
  namespace: envoy-gateway-system
spec:
  gatewayClassName: envoy-gateway
  listeners:
    - allowedRoutes:
        namespaces:
          from: All
      name: http-0
      port: 80
      protocol: HTTP
    - allowedRoutes:
        namespaces:
          from: All
      name: tls-pass-0
      port: 443
      protocol: TLS
      tls:
        mode: Passthrough
    - allowedRoutes:
        namespaces:
          from: All
      hostname: '*.p.r00t.party'
      name: https-0
      port: 443
      protocol: HTTPS
      tls:
        certificateRefs:
          - group: ''
            kind: Secret
            name: r00t-party-cert
        mode: Terminate
---

apiVersion: gateway.networking.k8s.io/v1alpha2
kind: TLSRoute
metadata:
  generation: 2
  labels:
    argocd.argoproj.io/instance: cluster-config
  name: ingress-nginx-tls
  namespace: ingress-nginx
spec:
  hostnames:
    - <REDACTED>
  parentRefs:
    - group: gateway.networking.k8s.io
      kind: Gateway
      name: default
      namespace: envoy-gateway-system
      sectionName: tls-pass-0
  rules:
    - backendRefs:
        - group: ''
          kind: Service
          name: ingress-nginx-controller
          port: 443
          weight: 1
---

apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy
metadata:
  generation: 3
  labels:
    argocd.argoproj.io/instance: cluster-config
  name: proxy-protocol-route-ingress-nginx-tls
  namespace: ingress-nginx
spec:
  proxyProtocol:
    version: V2
  targetRef:
    group: gateway.networking.k8s.io
    kind: TLSRoute
    name: ingress-nginx-tls
    namespace: ingress-nginx

Traffic is being routed, BackendTrafficPolicy is accepted, checking ./egctl config envoy-proxy cluster shows transportSockets for proxyProtocol... but well... traffic is not being encapsulated in proxy protocol (and ingress is refusing to accept it due to that).
It worked on patched istio before.
The same with external haproxy. When I enable expect-proxy in config I get PR_END_OF_FILE_ERROR in browser. When it's disabled it works, but client IP are not preserved.

output of

egctl config envoy-proxy listener
egctl config envoy-proxy route
egctl config envoy-proxy cluster

https://gist.github.com/JuniorJPDJ/19d0cd1bfd2ac0e00230adce374ac358

Oh, and I forgot - I'm on v1.0.0

@arkodg
Copy link
Contributor

arkodg commented Mar 15, 2024

the BTP policy is attaching to a TLSRoute which isn't supported today, but should be fixed with #2880

@arkodg
Copy link
Contributor

arkodg commented Mar 15, 2024

@arkodg
Copy link
Contributor

arkodg commented Mar 15, 2024

also @JuniorJPDJ you mentioned that you can see the xds cluster config with proxy protocol enabled, wonder how thats possible ..

@JuniorJPDJ
Copy link
Contributor Author

JuniorJPDJ commented Mar 15, 2024

Yup, look at those lines:
https://gist.github.com/JuniorJPDJ/19d0cd1bfd2ac0e00230adce374ac358#file-cluster-json-L67-L92

EDIT: that's http, bad catch, wait a sec

@JuniorJPDJ
Copy link
Contributor Author

You were right.. I just greped for proxyProtocol and wasn't reading too much..
Proxy protocol is enabled only on HTTP routes/clusters, not at TLS routes.

@JuniorJPDJ
Copy link
Contributor Author

Aaaand it works now on the latest commit due to #3004

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 area/translator Issues related to Gateway's translation service, e.g. translating Gateway APIs into the IR. kind/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants