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: re-enable proxy-protocol configuration nodes #756

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

heresie
Copy link
Contributor

@heresie heresie commented May 16, 2024

Hello everybody 👋🏻

I contributed in june 2023 to this project by adding configuration nodes in helm chart allowing clean proxy-protocol configuration (#528).

Recently we wanted to upgrade our current APISIX release because of CVE-2024-32638 but during upgrade we noticed that all proxy-protocol configuration nodes have disapeared since this PR : #738

Currently, we are unable to upgrade APISIX without manually editing ConfigMaps to re-enable proxy-protocol, and that's a big problem because we cannot automatize our deployments. I think we are not alone in this case (for example @adussarps in #748).

Can you consider my PR that re-introduce needed changes ?

Here are example values for testing purpose :

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

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

service:
  type: NodePort
  http:
    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 : 2.9.1.

All comments are welcome.

@yunerou
Copy link

yunerou commented Jul 4, 2024

I using EKS and this feature is very important. Can anyone help me review and merge this?

@kayx23 kayx23 requested review from AlinsRan and Revolyssup July 4, 2024 16:27
@Revolyssup
Copy link
Contributor

@heresie Can you fix the merge conflicts?

@yunerou
Copy link

yunerou commented Jul 4, 2024

@Revolyssup  I've checked the conflict, and it's just a chart version since it was updated to 2.8.1 before.


Currently, in the value.yaml file, I have to use fullCustomConfig and repeat it one more time, specifying details like ssl port or tcp port, because the Resource Kind Services template cannot understand the config defined in fullCustomConfig.

@heresie heresie force-pushed the re-enable-proxy-protocol-for-apisix-helm-chart branch from 75443e8 to 40cba16 Compare July 8, 2024 07:33
@heresie
Copy link
Contributor Author

heresie commented Jul 8, 2024

Hello @Revolyssup,

This PR was rebased. Future Chart version set to 2.8.2

{{- if or .Values.service.proxyProtocol.http.enabled }}
- name: apisix-gateway-pp-http
port: {{ .Values.service.proxyProtocol.http.servicePort }}
targetPort: {{ .Values.service.proxyProtocol.http.containerPort }}
Copy link

@yunerou yunerou Jul 9, 2024

Choose a reason for hiding this comment

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

@heresie

I realize you didn't define container port in templates/deployment.yaml. The request sent to the proxy_protocol port can't be forwarded to Apisix. Please explain to me if I am wrong.

Copy link
Contributor Author

@heresie heresie Jul 10, 2024

Choose a reason for hiding this comment

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

Hello @yunerou,
Declaring containerPorts on the Deployment is not mandatory to allow communication. However you are right, it's better to have them so it helps understanding.

@kennycottrell
Copy link

This change would be useful for me as well; bumping for visibility :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants