NE-2116: Update haproxy-config.template to use HTTPS/TCP log formats #667
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rohara: This pull request references NE-2116 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@rohara: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@rohara I was having some chat with @alebedev87 and @Miciah about testing templates, are you considering adding some cases on https://github.com/openshift/router/blob/master/pkg/router/router_test.go using t.SetEnv to assert that the template rendering is working? I don't have comments about the feature/directives, will leave this for someone with more context! Thanks! |
|
/assign |
|
/assign @rikatz |
There was a problem hiding this comment.
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.
| {{- if ne (env "ROUTER_SYSLOG_ADDRESS") "" }} | ||
| {{- if ne (env "ROUTER_HTTP_LOG_FORMAT") "" }} | ||
| log-format {{ env "ROUTER_HTTP_LOG_FORMAT" }} | ||
| {{- else }} | ||
| option httplog | ||
| {{- end }} |
There was a problem hiding this comment.
This does not look syntactically correct: You add two ifs and only one end.
| {{- if ne (env "ROUTER_TCP_LOG_FORMAT") "" }} | ||
| log-format {{ env "ROUTER_TCP_LOG_FORMAT" }} | ||
| {{- else }} |
There was a problem hiding this comment.
Here, you add if/else without a matching end.
| {{- if ne (env "ROUTER_SYSLOG_ADDRESS") "" }} | ||
| {{- if ne (env "ROUTER_HTTPS_LOG_FORMAT") "" }} | ||
| log-format {{ env "ROUTER_HTTPS_LOG_FORMAT" }} | ||
| {{- else }} | ||
| option httpslog | ||
| {{- end }} |
| {{- if ne (env "ROUTER_SYSLOG_ADDRESS") "" }} | ||
| {{- if ne (env "ROUTER_HTTPS_LOG_FORMAT") "" }} | ||
| log-format {{ env "ROUTER_HTTPS_LOG_FORMAT" }} | ||
| {{- else }} | ||
| option httsplog | ||
| {{- end }} |
| @@ -227,6 +227,12 @@ listen stats | |||
|
|
|||
| {{ if .BindPorts -}} | |||
| frontend public | |||
| {{- if ne (env "ROUTER_SYSLOG_ADDRESS") "" }} | |||
There was a problem hiding this comment.
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
ifconditions.
There was a problem hiding this comment.
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.
|
The PR description has some useful detail that is not in the commit message. Please do make sure that the commit message has a clear description. |
The haproxy-config.template must be updated to allow configuration of both HTTP and TCP log formats. Previously, only HTTP log format was configurable, and if not set, the default 'option httplog' was used. This logic is now pushed down into each frontend. The basic idea is that if access logging is enabled, we then look at the respective log format environment variable, then use either the custom log format or the default log format.