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

feat: allow proxy protocol configuration for APISIX #528

Merged
merged 15 commits into from
Jun 27, 2023
Merged

feat: allow proxy protocol configuration for APISIX #528

merged 15 commits into from
Jun 27, 2023

Conversation

heresie
Copy link
Contributor

@heresie heresie commented Apr 3, 2023

Hello everybody 👋

I'm currently trying to install APISIX on Kubernetes clusters that need to use Proxy Protocol in order to be usable. Unfortunately, the current Helm Chart does not allow to configure APISIX with proxy protocol by using values.yaml configuration.

The previous PR #353 about this subject is getting old, and @hgranillo has moved from APISIX, so here is a new one, updated and ready to merge.

I tested this Chart update on our K8S clusters, without any problem.

As stated in the original PR :

This PR adds the option to enable Proxy Protocol in the APISIX configuration file and allows to add the proxy protocol listeners to the gateway Kubernetes service.

Made all ports configurable in a fashion similar to apisix-gateway and apisix-gateway-tls.

I left enable_tcp_pp_to_upstream out because at this moment I have no way to test it. I can add the toggle if needed to merge this PR.

I tested these changes with the following configuration in one of my EKS Clusters.
I'm using the aws-load-balancer-controller to provision a NBL with SSL/TLS Offloading and Proxy Protocol enabled (see loadbalancer annotations below)

This allows me to send HTTP(80) -> ProxyProtocol HTTP(9181) and HTTPS 443 -> ProxyProtocol HTTP (9181)

Here is the values.yaml I used for tests :

global:
  enableIPv6: false
  storageClass: <my-storage-class>

apisix:
  kind: DaemonSet
  proxyProtocol:
    enabled: true
    listenHttpPort: 9181
    listenHttpsPort: 9182

gateway:
  type: NodePort
  http:
    enabled: true
  tls:
    enabled: true
  proxyProtocol:
    http:
      enabled: true
      nodePort: 30080
      containerPort: 9181
    https:
      enabled: true
      nodePort: 30443
      containerPort: 9182

This PR also bumps Chart Version to the next minor : 1.4.0.

All comments are welcome.

@heresie
Copy link
Contributor Author

heresie commented Apr 24, 2023

Hello!

Some checks have failed due to a missing space in chart comments.
It has been fixed.

@heresie
Copy link
Contributor Author

heresie commented Apr 25, 2023

Hello!

Thanks for the CI trigger.

I've updated the Apisix Helm Chart Readme accordingly with helm-docs, as required by the GitHub workflow. (Nice tool I didn't know by the way!)

@heresie
Copy link
Contributor Author

heresie commented May 4, 2023

Hello!

Is there any maintainer for the PR review? Thanks in advance.

Copy link
Member

@Gallardot Gallardot left a comment

Choose a reason for hiding this comment

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

I didn't find any more details on this proxy protocol feature. Not sure if the current modification is appropriate. PTAL @tao12345666333

charts/apisix/templates/configmap.yaml Outdated Show resolved Hide resolved
@tao12345666333
Copy link
Member

I will add this to my list and review it ASAP. Thanks

@tao12345666333 tao12345666333 self-assigned this May 5, 2023
@tao12345666333 tao12345666333 added the enhancement New feature or request label May 8, 2023
charts/apisix/Chart.yaml Outdated Show resolved Hide resolved
charts/apisix/templates/configmap.yaml Outdated Show resolved Hide resolved
@heresie
Copy link
Contributor Author

heresie commented May 22, 2023

Hello @tao12345666333, have you seen latest changes for this topic? Thank you in advance for your help.

@tao12345666333
Copy link
Member

Other parts LGTM

@tao12345666333
Copy link
Member

And please resolve the conflicts.

@heresie
Copy link
Contributor Author

heresie commented Jun 26, 2023

@tao12345666333 said :

And please resolve the conflicts.

The conflicts are resolved.

@franek
Copy link

franek commented Jun 27, 2023

image
🎉

Copy link
Member

@Gallardot Gallardot left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for your contribution!

@tao12345666333 tao12345666333 merged commit 9efc9f3 into apache:master Jun 27, 2023
@heresie heresie deleted the feat/allow-enable-proxy-protocol branch June 27, 2023 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants