8000 allow init containers, e.g. to deploy a crl file to nginx by g10f · Pull Request #2100 · nginx/kubernetes-ingress · GitHub
[go: up one dir, main page]
Skip to content

allow init containers, e.g. to deploy a crl file to nginx #2100

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
Merged
merged 4 commits into from
Nov 4, 2021
Merged

allow init containers, e.g. to deploy a crl file to nginx #2100

merged 4 commits into from
Nov 4, 2021

Conversation

g10f
Copy link
Contributor
@g10f g10f commented Oct 14, 2021 •
edited
Loading

Proposed changes

For deploying crl files with size of several MBs to nginx we need an InitContainer.
Our crl file is about 8 MB which can not be stored in a config map because of the size. Therfore we use an init container to provide the crl.
E. g. here a values file snippet with an init container that copys a crl file to data/pem/crl.pem

  initContainers:
  - name: init-nginx
    image: xyz.dkr.ecr.eu-central-1.amazonaws.com/init-nginx:1.1.12 
    volumeMounts:
    - name: extra-volume
      mountPath: /data/pem
  volumeMounts:
  - name: extra-volume
    mountPath: /etc/nginx/crl.pem
    subPath: crl.pem
  volumes:
  - name: extra-volume
    emptyDir: {}

The corresponding ingress yaml snippet is

metadata:
  annotations:
    nginx.org/server-snippets: |
      ssl_verify_client optional;
      ssl_verify_depth 2;
      ssl_crl crl.pem;
      ...

Additionally we use a cronjob which does a kubectl rollout restart of the deployment so that new crl files are used by nginx. Would be nice, if this is supported by the helm chart.

@pleshakov
Copy link
Contributor

Hi @g10f Thanks for the PR!

Could you possibly clarify the use case(s) for init containers in the Ingress Controller? You mentioned crl files. However, could you explain how it works?

@pleshakov pleshakov added the proposal An issue that proposes a feature request label Oct 15, 2021
@g10f
Copy link
Contributor Author
g10f commented Oct 17, 2021

Hi @g10f Thanks for the PR!

Could you possibly clarify the use case(s) for init containers in the Ingress Controller? You mentioned crl files. However, could you explain how it works?

I added some information above. The main thing is, that we copy the crl file with the init container to a shared mount.

@tomasohaodha
Copy link
Contributor

Hi Gunnar,

Many thanks for the PR, we are eager to merge it. Some further feedback:

  1. Could you update deployments/helm-chart/templates/controller-daemonset.yaml to include the init containers
  2. In deployments/helm-chart/values.yaml, the entry for init container needs to have a doc that matches the entry in the helm readme for consistency:
## InitContainers for the Ingress controller pods.  
initContainers: []
  1. Can you update https://github.com/nginxinc/kubernetes-ingress/blob/master/docs/content/installation/installation-with-helm.md#configuration similarly to the helm readme
  2. Optional but useful: In the values file, also include an example of an init container in the comment, similarly to the volumeMounts: []. For example:
  initContainers:
  # - name: init-container
  #   image: busybox:1.34
  #   command: ['sh', '-c', 'echo "init container will run for 120s" && sleep 120']

@g10f
Copy link
Contributor Author
g10f commented Oct 29, 2021

Hi Gunnar,

Many thanks for the PR, we are eager to merge it. Some further feedback:

  1. Could you update deployments/helm-chart/templates/controller-daemonset.yaml to include the init containers
  2. In deployments/helm-chart/values.yaml, the entry for init container needs to have a doc that matches the entry in the helm readme for consistency:
## InitContainers for the Ingress controller pods.  
initContainers: []
  1. Can you update https://github.com/nginxinc/kubernetes-ingress/blob/master/docs/content/installation/installation-with-helm.md#configuration similarly to the helm readme
  2. Optional but useful: In the values file, also include an example of an init container in the comment, similarly to the volumeMounts: []. For example:
  initContainers:
  # - name: init-container
  #   image: busybox:1.34
  #   command: ['sh', '-c', 'echo "init container will run for 120s" && sleep 120']

Hi @tomasohaodha, thank you for the feedback. I updated the pull request.

@tomasohaodha
Copy link
Contributor

Hi Gunnar,
Many thanks for the PR, we are eager to merge it. Some further feedback:

  1. Could you update deployments/helm-chart/templates/controller-daemonset.yaml to include the init containers
  2. In deployments/helm-chart/values.yaml, the entry for init container needs to have a doc that matches the entry in the helm readme for consistency:
## InitContainers for the Ingress controller pods.  
initContainers: []
  1. Can you update https://github.com/nginxinc/kubernetes-ingress/blob/master/docs/content/installation/installation-with-helm.md#configuration similarly to the helm readme
  2. Optional but useful: In the values file, also include an example of an init container in the comment, similarly to the volumeMounts: []. For example:
  initContainers:
  # - name: init-container
  #   image: busybox:1.34
  #   command: ['sh', '-c', 'echo "init container will run for 120s" && sleep 120']

Hi @tomasohaodha, thank you for the feedback. I updated the pull request.

Thanks for that Gunnar, we'll review asap

@vepatel
Copy link
Contributor
vepatel commented Nov 3, 2021 •
edited
Loading

Verified:

  Normal  Pulled     3m20s  kubelet            Container image "busybox:1.34" already present on machine
  Normal  Created    3m20s  kubelet            Created container init-container
  Normal  Started    3m20s  kubelet            Started container init-container

@g10f approved 👍🏼 , just a few minor comments:

  • ReadMe: can we remove crl.pem specific line Useful for fetching crl files from a URL and copy the data to a mounted volume to crl.pem for using with nginx or maybe add Init containers can contain utilities or setup scripts not present in an app image instead?
  • In values.yaml, can give a more generic example like: 
    initContainers: 
 - name: init-container
    image: busybox:1.34
    command: ['sh', '-c', 'echo this is initial setup!']

@tomasohaodha tomasohaodha added the enhancement Pull requests for new features/feature enhancements label Nov 4, 2021
@g10f
Copy link
Contributor Author
g10f commented Nov 4, 2021

Verified:

  Normal  Pulled     3m20s  kubelet            Container image "busybox:1.34" already present on machine
  Normal  Created    3m20s  kubelet            Created container init-container
  Normal  Started    3m20s  kubelet            Started container init-container

@g10f approved 👍🏼 , just a few minor comments:

  • ReadMe: can we remove crl.pem specific line Useful for fetching crl files from a URL and copy the data to a mounted volume to crl.pem for using with nginx or maybe add Init containers can contain utilities or setup scripts not present in an app image instead?
  • In values.yaml, can give a more generic example like: 
    initContainers: 
 - name: init-container
    image: busybox:1.34
    command: ['sh', '-c', 'echo this is initial setup!']

Verified:

  Normal  Pulled     3m20s  kubelet            Container image "busybox:1.34" already present on machine
  Normal  Created    3m20s  kubelet            Created container init-container
  Normal  Started    3m20s  kubelet            Started container init-container

@g10f approved 👍🏼 , just a few minor comments:

  • ReadMe: can we remove crl.pem specific line Useful for fetching crl files from a URL and copy the data to a mounted volume to crl.pem for using with nginx or maybe add Init containers can contain utilities or setup scripts not present in an app image instead?
  • In values.yaml, can give a more generic example like: 
    initContainers: 
 - name: init-container
    image: busybox:1.34
    command: ['sh', '-c', 'echo this is initial setup!']

Thanks for reviewing. I have implemented your suggestions.

@tomasohaodha tomasohaodha enabled auto-merge (squash) November 4, 2021 10:08
@tomasohaodha tomasohaodha merged commit 3770c0a into nginx:master Nov 4, 2021
@tomasohaodha
Copy link
Contributor

Merged into master, many thanks @g10f Gunnar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vepatel vepatel vepatel approved these changes

+1 more reviewer

@pleshakov pleshakov pleshakov approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
enhancement Pull requests for new features/feature enhancements proposal An issue that proposes a feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@g10f @pleshakov @tomasohaodha @vepatel
0