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.

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

Jump to bottom

helm: support initContainers #3838

Merged
merged 4 commits into from
Aug 10, 2023
Merged

helm: support initContainers #3838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
72b6c8c
support initContainers
calvinbui Aug 2, 2023
1baba27
update changelog
calvinbui Aug 2, 2023
2e9864a
add initContainers to chart README
calvinbui Aug 2, 2023
1488cc1
Merge branch 'kubernetes-sigs:master' into master
calvinbui Aug 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions charts/external-dns/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- Disallowed privilege escalation in container security context and set the seccomp profile type to `RuntimeDefault`. ([#3689](https://github.com/kubernetes-sigs/external-dns/pull/3689)) [@nrvnrvn](https://github.com/nrvnrvn)
- Added RBAC for Traefik to ClusterRole. ([#3325](https://github.com/kubernetes-sigs/external-dns/pull/3325)) [@ThomasK33](https://github.com/thomask33)
- Support initContainers. ([#3325](https://github.com/kubernetes-sigs/external-dns/pull/3838)) [@calvinbui](https://github.com/calvinbui)

## [v1.13.0] - 2023-03-30

Expand Down
1 change: 1 addition & 0 deletions charts/external-dns/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ The following table lists the configurable parameters of the _ExternalDNS_ chart
| `serviceAccount.name` | Service account to be used. If not set and `serviceAccount.create` is `true`, a name is generated using the full name template. | `""` |
| `rbac.create` | If `true`, create the RBAC resources. | `true` |
| `rbac.additionalPermissions` | Additional permissions to be added to the cluster role. | `{}` |
| `initContainers` | Add init containers to the pod. | `[]` |
| `deploymentAnnotations` | Annotations to add to the Deployment. | `{}` |
| `podLabels` | Labels to add to the pod. | `{}` |
| `podAnnotations` | Annotations to add to the pod. | `{}` |
Expand Down
4 changes: 4 additions & 0 deletions charts/external-dns/templates/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ spec:
{{- with .Values.dnsPolicy }}
dnsPolicy: {{ . }}
{{- end }}
{{- with .Values.initContainers }}
initContainers:
{{- toYaml . | nindent 8 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{- toYaml . | nindent 8 }}
{{- tpl (toYaml .) . | nindent 8 }}

By using tpl, it will allow to use {{ include "external-dns.image" . }} in the values

Copy link
Contributor Author

@calvinbui calvinbui Aug 2, 2023 •
edited

Choose a reason for hiding this comment

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

i've never seen any templating possible through values?

it doesn't work for me:

initContainers:
  - name: init-jitter
    image: {{ include "external-dns.image" . }}
    command:
      - /bin/sh
      - -c
      - 'FOR=$((RANDOM % 10))s;echo "Sleeping for $FOR";sleep $FOR'

Error: cannot load values.yaml: error converting YAML to JSON: yaml: invalid map key: map[interface {}]interface {}{"include \"external-dns.image\" .":interface {}(nil)}

Copy link
Contributor

Choose a reason for hiding this comment

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

@mloiseleur that would be incorrect, tpl shouldn't be used like this. If you want tpl support you'd need to support init containers as either a string or an array and only template when it's been provided as a string. I'd prefer this to stay as it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

@calvinbui It has been done on Traefik Helm Chart, see this PR for instance.
@stevehipwell I understand. It's ok for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mloiseleur this is a well known anti-pattern that looks like it should work, works in very simple scenarios but fails hard for anything more complex. To prove this attempt to use the selector labels template for traefik affinity match labels and you will see what I mean.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you're right.

{{- end }}
containers:
- name: external-dns
{{- with .Values.securityContext }}
Expand Down
2 changes: 2 additions & 0 deletions charts/external-dns/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ rbac:
create: true
additionalPermissions: []

initContainers: []

# Annotations to add to the Deployment
deploymentAnnotations: {}

Expand Down