Fix several Helm YAML issues with extraModules and extraInitContainers

[ajacques]

What this PR does / why we need it:

Prior to this fix, users are unable to use OpenTelemetry with DaemonSets due to several issues and not able to specify both values for both extraInitContainers and extraModules. The resulting template will be rejected because it's semantically invalid due to a YAML indention issue. In addition, there's a casing issue. The Deployment refers to image and name, but DaemonSet uses Image and Name.

Given the following values.yaml:

controller:
  extraInitContainers:
    - command:
        - /bin/chown
        - '101'
        - /mycache
      image: busybox:latest
      name: chmod
      volumeMounts:
        - mountPath: /mycache
          name: cache
  extraVolumeMounts:
    - mountPath: /var/cache/nginx
      name: cache
  extraVolumes:
    - hostPath:
        path: /var/cache/nginx-k8s
        type: ''
      name: cache
  kind: DaemonSet
  extraModules:
    - name: opentelemetry
      image: registry.k8s.io/ingress-nginx/opentelemetry:v20230107-helm-chart-4.4.2-2-g96b3d2165@sha256:331b9bebd6acfcd2d3048abbdd86555f5be76b7e3d0b5af4300b04235c6056c9

Prior to this change, the DaemonSet gets generated like this:

      initContainers:
        
        - command:
          - /bin/chown
          - "101"
          - /mycache
          image: busybox:latest
          name: chmod
          volumeMounts:
          - mountPath: /mycache
            name: cache
          - name:   # <--- Notice that this is aligned under the volumeMounts, not under initContainers
            image:   # <-- Also note the empty image and names
            command: ['sh', '-c', '/usr/local/bin/init_module.sh']
            volumeMounts:
              - name: modules
                mountPath: /modules_mount

After this change, it generates a correct YAML:

      initContainers:
        
        - command:
          - /bin/chown
          - "101"
          - /mycache
          image: busybox:latest
          name: chmod
          volumeMounts:
          - mountPath: /mycache
            name: cache
        - name: opentelemetry
          image: registry.k8s.io/ingress-nginx/opentelemetry:v20230107-helm-chart-4.4.2-2-g96b3d2165@sha256:331b9bebd6acfcd2d3048abbdd86555f5be76b7e3d0b5af4300b04235c6056c9
          command: ['sh', '-c', '/usr/local/bin/init_module.sh']
          volumeMounts:
            - name: modules
              mountPath: /modules_mount

Honestly, I'm not sure if extraModules even works in the Deployment case. I wasn't able to do Helm template if I generated a Deployment or DaemonSet. Trying to generate a Deployment gave me:

error: template: ingress-nginx/templates/controller-deployment.yaml:188:134: executing "ingress-nginx/templates/controller-deployment.yaml" at <8>: wrong type for value; expected string; got map[string]interface {}
helm.go:84: [debug] template: ingress-nginx/templates/controller-deployment.yaml:188:134: executing "ingress-nginx/templates/controller-deployment.yaml" at <8>: wrong type for value; expected string; got map[string]interface {}

I only discovered this issue because the DaemonSet didn't support specifying controller.opentelemetry.enabled=true

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• CVE Report (Scanner found CVE and adding report)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation only

Which issue/s this PR fixes

How Has This Been Tested?

I manually reviewed the generated template (see above) and deployed this commit to my cluster and verified the DaemonSet looks correct.

Are there any Helm tests that actually assert anything? I see the files in charts/ingress-nginx/ci, but I didn't see any expected values to make sure that's correct.

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I've read the CONTRIBUTION guide
• I have added unit and/or e2e tests to cover my changes.
• All new and existing tests passed.
• Added Release Notes.

Does my pull request need a release note?

Any user-visible or operator-visible change qualifies for a release note. This could be a:

• fix of a previous Known Issue

No release notes are required for changes to the following:

• Tests
• Build infrastructure
• Fixes for unreleased bugs

For more tips on writing good release notes, check out the Release Notes Handbook

Fixed Helm template issue when specifying extraModules and allow OpenTelemetry to be enabled on DaemonSets

[k8s-ci-robot]

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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/test-infra repository.

[k8s-ci-robot]

Welcome @ajacques!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @ajacques. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

[longwuyuan]

@ajacques thanks for the contribution

@esigo Is this PR only for OTEL or is the code change going to enable any/every possible extraModule that comes to any user's mind. And if its only for OTEL, then how does this play with your PR

[longwuyuan]

I am still checking but allowing any/every possible container as initContainer or sideCar etc is a security concern.

[ajacques]

For context: I don't have any custom modules, just trying to use the OTEL one. This came up because I tried to enable OTEL on a DaemonSet and found a few different bugs that prevented me from doing that and I just fixed them all.

allowing any/every possible container as initContainer or sideCar etc is a security concern.

Can you clarify why this is something to protect the user against? This is the current state since there's no restrictions on extraInitContainers or extraContainers.

[longwuyuan]

@ajacques sorry for confusion

• Please wait for comments from @esigo on OTEL
• The current state leaves much to be desired and work is in progress to change current state
• Much hardening & securing is work-in-progress so I asked if there is a hint of allowing any/all initContainer/sideCar because that changes security

[longwuyuan]

@ajacques the controller as a daemonset with OTEL would also require tests

[longwuyuan]

@ajacques also, there is a much broader impact when it comes the process of integration with extraModules. We have discussed it and there are changes needed in the core of the controller image. Hence I pinged @esigo .

[k8s-triage-robot]

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

[esigo]

@longwuyuan, Otel doesn't rely on extra modules anymore. daemonset changes for Otel looks fine, I haven't tested daemonset with Otel module though.
If there is no other use case for extra modules maybe it can be removed?

[k8s-triage-robot]

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: ajacques / name: Adam Jacques (d3140cd, 02ae6b8, 9bf4473, 297d779)

[tao12345666333]

/check-cla

[ajacques]

Are these changes valuable? Should I remove extraModules or change anything? At the very minimum, I'd suggest we fix the extraModules indention issue, but I'm not really blocked on this change getting merged.

[strongjz]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ajacques, strongjz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• charts/ingress-nginx/OWNERS [strongjz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment