Skip to content

NE-2116: Update haproxy-config.template to use HTTPS/TCP log formats #667

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.

Sign up for GitHub

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

Open
rohara wants to merge 1 commit into
base: master
Choose a base branch
from
+21 −0
Open

NE-2116: Update haproxy-config.template to use HTTPS/TCP log formats #667

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions images/router/haproxy/conf/haproxy-config.template
View file
Open in desktop
Copy link
Contributor

@Miciah Miciah Aug 28, 2025

Choose a reason for hiding this comment

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

It isn't obvious why you keep log-format / option httplog in defaults. Is that intentional, to preserve ROUTER_SYSLOG_FORMAT as the default for frontends that (implicitly) use mode tcp when ROUTER_TCP_LOG_FORMAT or ROUTER_HTTPS_LOG_FORMAT is not specified? It would be helpful to explain this in the commit message.

Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,12 @@ listen stats

{{ if .BindPorts -}}
frontend public
{{- if ne (env "ROUTER_SYSLOG_ADDRESS") "" }}
Copy link
Contributor

@Miciah Miciah Aug 28, 2025

Choose a reason for hiding this comment

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

Consider defining {{- $syslogAddress := env "ROUTER_SYSLOG_ADDRESS" }}, {{- $httpLogFormat := env "ROUTER_HTTP_LOG_FORMAT" }}, and so on for the following reasons:

  • Less performance hit from repeatedly calling into Go helpers.
  • Less risk of typos in the environment variable's name.
  • Slightly simpler and more readable if conditions.

Copy link
Contributor

@Miciah Miciah Aug 28, 2025

Choose a reason for hiding this comment

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

By the way, the env helper can take additional arguments; it uses the first nonempty value, so you could do the following:

{{- $httpLogFormat := env "ROUTER_HTTP_LOG_FORMAT" (env "ROUTER_SYSLOG_FORMAT") }}
{{- $httpsLogFormat := env "ROUTER_HTTPS_LOG_FORMAT" $httpLogFormat }}
{{- $tcpLogFormat := env "ROUTER_TCP_LOG_FORMAT" $httpLogFormat }}

This also makes the defaulting behavior more explicit and clear, in my opinion.

{{- if ne (env "ROUTER_HTTP_LOG_FORMAT") "" }}
log-format {{ env "ROUTER_HTTP_LOG_FORMAT" }}
{{- else }}
option httplog
{{- end }}
Comment on lines +230 to +235
Copy link
Contributor

@Miciah Miciah Aug 28, 2025

Choose a reason for hiding this comment

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

This does not look syntactically correct: You add two ifs and only one end.

{{ if eq "v4v6" $router_ip_v4_v6_mode }}
bind :{{ env "ROUTER_SERVICE_HTTP_PORT" "80" }}{{ if isTrue (env "ROUTER_USE_PROXY_PROTOCOL") }} accept-proxy{{ end }}
bind :::{{ env "ROUTER_SERVICE_HTTP_PORT" "80" }} v6only{{ if isTrue (env "ROUTER_USE_PROXY_PROTOCOL") }} accept-proxy{{ end }}
Expand Down Expand Up @@ -302,6 +308,9 @@ frontend public
# that terminates encryption in this router (edge)
frontend public_ssl
{{- if ne (env "ROUTER_SYSLOG_ADDRESS") "" }}
{{- if ne (env "ROUTER_TCP_LOG_FORMAT") "" }}
log-format {{ env "ROUTER_TCP_LOG_FORMAT" }}
{{- else }}
Comment on lines +311 to +313
Copy link
Contributor

@Miciah Miciah Aug 28, 2025

Choose a reason for hiding this comment

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

Here, you add if/else without a matching end.

option tcplog
{{- end }}
{{ if eq "v4v6" $router_ip_v4_v6_mode }}
Expand Down Expand Up @@ -343,6 +352,12 @@ backend be_sni
server fe_sni unix@/var/lib/haproxy/run/haproxy-sni.sock weight 1 send-proxy

frontend fe_sni
{{- if ne (env "ROUTER_SYSLOG_ADDRESS") "" }}
{{- if ne (env "ROUTER_HTTPS_LOG_FORMAT") "" }}
log-format {{ env "ROUTER_HTTPS_LOG_FORMAT" }}
{{- else }}
option httpslog
{{- end }}
Comment on lines +355 to +360
Copy link
Contributor

@Miciah Miciah Aug 28, 2025

Choose a reason for hiding this comment

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

Missing end.

# terminate ssl on edge
bind unix@/var/lib/haproxy/run/haproxy-sni.sock ssl
{{- if isTrue (env "ROUTER_STRICT_SNI") }} strict-sni {{ end }}
Expand Down Expand Up @@ -461,6 +476,12 @@ backend be_no_sni
server fe_no_sni unix@/var/lib/haproxy/run/haproxy-no-sni.sock weight 1 send-proxy

frontend fe_no_sni
{{- if ne (env "ROUTER_SYSLOG_ADDRESS") "" }}
{{- if ne (env "ROUTER_HTTPS_LOG_FORMAT") "" }}
log-format {{ env "ROUTER_HTTPS_LOG_FORMAT" }}
{{- else }}
option httsplog
{{- end }}
Comment on lines +479 to +484
Copy link
Contributor

@Miciah Miciah Aug 28, 2025

Choose a reason for hiding this comment

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

Missing end.

Copy link
Member

@ShudiLi ShudiLi Sep 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

httsplog should be httpslog? Thanks.

# terminate ssl on edge
bind unix@/var/lib/haproxy/run/haproxy-no-sni.sock ssl crt {{ firstMatch ".+" .DefaultCertificate "/var/lib/haproxy/conf/default_pub_keys.pem" }} accept-proxy
{{- with (env "ROUTER_MUTUAL_TLS_AUTH") }}
Expand Down