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

improve custom template handling #48

Merged
merged 5 commits into from
Mar 23, 2022
Merged

improve custom template handling #48

merged 5 commits into from
Mar 23, 2022

Conversation

ironashram
Copy link
Contributor

@ironashram ironashram commented Mar 15, 2022

Add a new templateFile option that references a file instead of copy/pasting the template into the values chart.

@jcmoraisjr
Copy link
Member

Hi, I could find any problem in the current implementation. How are you customizing the template (what you configure and where) and what were you expecting as a result?

@ironashram
Copy link
Contributor Author

ironashram commented Mar 20, 2022

Hi, for me neither of the following
{{ .Values.controller.template | indent 4 }}
{{ .Files.Get "haproxy.tmpl" | indent 4 }}
results in helm3 rendering the chart successfully, i tried both adding the modified template to the values or loading the modified haproxy.tmpl but it looks like helm tries to interpret the template this way hence the reason why i propose this change

As for the actual changes to the haproxy template i do the following

  1. Add dummy backends for sticky tables, which i use for custom rate limiting requests/ddos protection, i add this to the global section of the template, so that those gets added only to the main haproxy.cfg and not for every sharded files that would cause an error
  2. Change the position where the custom frontend snippets are being added, i do this because the custom snippet i need contains a few tcp-request/connection directives which with the default template end up being placed after some other http related directives, thus haproxy keeps throwing warnings about the wrong order on every reload

@jcmoraisjr
Copy link
Member

Hi, regarding the template I can successfully change it with these two changes:

  • remove the two spaces indentation from the first line - yaml parser doesn't like it because it'd instead interpret that the whole file would have those extra spaces, which it isn't true;
  • remove the - from the last line, using {{ end }} instead of {{- end }} - haproxy parser doesn't like this last line without a line break and the template is removing the trailing line breaks because of the minus signal in the haproxy.tmpl: |- helm template.

I think that these two changes would be benefit for the functionality:

  • remove the spaces in the first line of the template (controller repo)
  • change from |- to simply | in the helm template

Other than that, regarding the issues you're trying to fix:

  • 1. can be configured with config-sections, doc here
  • 2. please provide a reproducer in the controller repo so this won't be forgotten

@ironashram
Copy link
Contributor Author

Hi, thank you for all the details, i'm going to do a few tests with helm and the insight you gave me

Regarding the suggestions

  1. I know there is this possibility from v0.13, unfortunately i can't use that yet on production due to having an old k8s version on some clusters
  2. Opened --> Warning on reload if config-frontend snippet contains a tcp-request jcmoraisjr/haproxy-ingress#909

@ironashram
Copy link
Contributor Author

@jcmoraisjr i made some tests and i agree with your proposed solution

  1. remove the spaces in the first line of the template (controller repo)
  2. change from |- to simply | in the helm template

However i'd still change Values.template to boolean and remove {{ .Values.controller.template | indent 4 }} or not make it the default behavior since it's very messy to use

@jcmoraisjr
Copy link
Member

This cannot be changed due to backward compatibility, but there isn't any problem to create another key with a distinct behavior, maybe a templateFile which receives a path? How does it sound?

@ironashram
Copy link
Contributor Author

This cannot be changed due to backward compatibility, but there isn't any problem to create another key with a distinct behavior, maybe a templateFile which receives a path? How does it sound?

pushed a commit that includes this new key and keeps backwards compatibility, let me know if that works for you

Copy link
Member

@jcmoraisjr jcmoraisjr left a comment

Choose a reason for hiding this comment

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

thanks! here are a few update suggestions.

@@ -109,6 +109,7 @@ Parameter | Description | Default
`controller.extraContainers` | extra containers that to the haproxy-ingress-controller | `[]`
`controller.initContainers` | extra containers that can initialize the haproxy-ingress-controller | `[]`
`controller.template` | custom template for haproxy-ingress-controller | `{}`
`controller.templateFile` | custom haproxy template path for haproxy-ingress-controller | `{}`
Copy link
Member

Choose a reason for hiding this comment

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

"" instead in the last column

@@ -1,4 +1,4 @@
{{- if .Values.controller.template -}}
{{- if or .Values.controller.template .Values.controller.templateFile -}}
Copy link
Member

Choose a reason for hiding this comment

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

missing this same logic in the _podtemplate.yaml, look for controller.template

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, added

{{ .Values.controller.template | indent 4 }}
{{/* .Files.Get "haproxy.tmpl" | indent 4 */}}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

these trimleft configs broke the yaml parser, here is a config suggestion that worked for me:

  haproxy.tmpl: |
{{- if .Values.controller.templateFile }}
    {{- .Files.Get .Values.controller.templateFile | nindent 4 }}
{{- else }}
    {{- .Values.controller.template | nindent 4 }}
{{- end }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@jcmoraisjr
Copy link
Member

Lgtm thanks! Merging now.

@jcmoraisjr jcmoraisjr merged commit fc1271d into haproxy-ingress:master Mar 23, 2022
jcmoraisjr pushed a commit that referenced this pull request Mar 26, 2022
Add a new `templateFile` option that references a file instead of copy/pasting the template into the values chart.
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.

2 participants