Baoyuantop commented on PR #922: URL: https://github.com/apache/apisix-helm-chart/pull/922#issuecomment-4599987220
Exposing APISIX's proxy_protocol HTTP/HTTPS listeners in the chart is a reasonable direction. The main unresolved issue is the Service exposure model from the previous review thread: with apisix.proxyProtocol.enabled=true, the current diff changes the existing apisix-gateway / apisix-gateway-tls targetPort from the normal HTTP/TLS ports to the proxy protocol ports, instead of adding dedicated service ports. That changes the protocol semantics of the existing Service and does not allow users to expose normal HTTP/HTTPS and PROXY protocol HTTP/HTTPS at the same time. Please first sync with master to resolve the merge conflict and fix the current failed/cancelled CI. Then please adjust the chart to add dedicated proxy protocol service ports/values, or clearly document and test why replacing the existing targetPort is the intended behavior. Please also add Helm rendering coverage for enabled=false, enabled=true, and custom listen_http_port/listen_https_port values. The unrelated formatting changes in values.yaml should be removed to keep the PR focused. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
