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

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

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

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

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 nginxinc: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
Labels
enhancement Pull requests for new features/feature enhancements proposal An issue that proposes a feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants