Page MenuHomePhabricator

Fix rendering issue in modules.app.job when cronjobs are enabled and private values are defined
Open, HighPublic

Description

When investigating a CI failure seemingly unrelated with the submitted patch, I encountered a bug in the modules/app/job_1.0.0.tpl template.

After investigating, it appeared that this bug is only triggered when cronjobs are enabled and private values are defined.

Here are steps to reproduce the issue:

~/wmf/deployment-charts master *17 ?4 ❯ ./create_new_service.sh
What will be the name of your chart? test
INFO:sextant:Loading available components
INFO:sextant:Creating the chart from /Users/brouberol/wmf/deployment-charts/_scaffold/service
===> Available components:

* generic-application
  Basic component for generic (non-LAMP) applications

* LAMP
  Basic LAMP application component.

* ingress
  Kubernetes ingress setup with istio.

* service-mesh
  Sets up the service mesh and the TLS termination (if needed)

* prometheus-statsd
  sets up prometheus-statsd-exporter

* mcrouter
  sets up mcrouter
==> Please select a component by name (q to quit)
> generic-application
INFO:wmflib.interactive:User input is: "generic-application"
===> Available components:

* ingress
  Kubernetes ingress setup with istio.

* service-mesh
  Sets up the service mesh and the TLS termination (if needed)

* prometheus-statsd
  sets up prometheus-statsd-exporter

* mcrouter
  sets up mcrouter
Already selected: generic-application
==> Please select a component by name (q to quit)
> ingress
INFO:wmflib.interactive:User input is: "ingress"
===> Available components:

* service-mesh
  Sets up the service mesh and the TLS termination (if needed)

* prometheus-statsd
  sets up prometheus-statsd-exporter

* mcrouter
  sets up mcrouter
Already selected: generic-application,ingress
==> Please select a component by name (q to quit)
> service-mesh
INFO:wmflib.interactive:User input is: "service-mesh"
===> Available components:

* prometheus-statsd
  sets up prometheus-statsd-exporter

* mcrouter
  sets up mcrouter
Already selected: generic-application,ingress,service-mesh
==> Please select a component by name (q to quit)
> q
INFO:wmflib.interactive:User input is: "q"
Selected components: generic-application,ingress,service-mesh


===> Please answer the following questions:

* Please input the complete name of the image (without the registry)

> whatever
* Please input the port that this service will live on. If this service
  is not behind an ingress (i.e. you didn't select the "ingress"
  component), please reserve a port at
  https://wikitech.wikimedia.org/wiki/Kubernetes/Service_ports
> 8000
Chart test created, now vendoring dependencies.
INFO:sextant:Copied modules/app/generic_1.0.1.tpl => charts/test/templates/vendor/app/generic_1.0.1.tpl
INFO:sextant:Copied modules/base/helper_1.1.1.tpl => charts/test/templates/vendor/base/helper_1.1.1.tpl
INFO:sextant:Copied modules/base/meta_2.0.0.tpl => charts/test/templates/vendor/base/meta_2.0.0.tpl
INFO:sextant:Copied modules/base/name_1.0.0.tpl => charts/test/templates/vendor/base/name_1.0.0.tpl
INFO:sextant:Copied modules/app/job_1.0.1.tpl => charts/test/templates/vendor/app/job_1.0.1.tpl
INFO:sextant:Copied modules/base/external-services-networkpolicy_1.0.1.tpl => charts/test/templates/vendor/base/external-services-networkpolicy_1.0.1.tpl
INFO:sextant:Copied modules/base/networkpolicy_1.2.0.tpl => charts/test/templates/vendor/base/networkpolicy_1.2.0.tpl
INFO:sextant:Copied modules/ingress/istio_1.0.3.tpl => charts/test/templates/vendor/ingress/istio_1.0.3.tpl
INFO:sextant:Copied modules/mesh/configuration_1.7.0.tpl => charts/test/templates/vendor/mesh/configuration_1.7.0.tpl
INFO:sextant:Copied modules/mesh/certificate_1.1.0.tpl => charts/test/templates/vendor/mesh/certificate_1.1.0.tpl
INFO:sextant:Copied modules/mesh/name_1.1.0.tpl => charts/test/templates/vendor/mesh/name_1.1.0.tpl
INFO:sextant:Copied modules/mesh/deployment_1.3.0.tpl => charts/test/templates/vendor/mesh/deployment_1.3.0.tpl
INFO:sextant:Copied modules/mesh/networkpolicy_1.2.0.tpl => charts/test/templates/vendor/mesh/networkpolicy_1.2.0.tpl
INFO:sextant:Copied modules/mesh/service_1.1.0.tpl => charts/test/templates/vendor/mesh/service_1.1.0.tpl

I then define the following data in values.yaml:

config:
  public: {} # Add here all the keys that can be publicly available as a ConfigMap
  private:
    password: secretpassword
~/wmf/deployment-charts master *17 ?5 ❯ helm template myrelease ./charts/test -f ./charts/test/.fixtures/cronjob_enabled.yaml
Error: template: test/templates/cronjob.yaml:24:14: executing "test/templates/cronjob.yaml" at <include "app.job.container" (dict "Root" $root "Job" $cronjob)>: error calling include: template: test/templates/vendor/base/name_1.0.0.tpl:35:27: executing "base.name.release" at <.Chart.Name>: nil pointer evaluating interface {}.Name

Use --debug flag to render out invalid YAML

The issue comes from a series of indentation errors and a wrong variable passing in modules/app/job_1.0.0.tpl, specifically in:

- name: {{ $k }}
    valueFrom:
    secretKeyRef:
        name: {{ template "base.name.release" $ }}-secret-config
        key: {{ $k }}

This block should actually read

- name: {{ $k }}
   valueFrom:
      secretKeyRef:
        name: {{ template "base.name.release" $.Root }}-secret-config
        key: {{ $k }}

With this, rendering works:

~/wmf/deployment-charts master *17 ?5 ❯ helm template myrelease ./charts/test -f ./charts/test/.fixtures/cronjob_enabled.yaml | grep -C 2 
# Source: test/templates/service.yaml
--
                secretKeyRef:
                  name: test-myrelease-secret-config
                  key: password

          resources:
--
            imagePullPolicy: IfNotPresent
            env:
            - name: password
              valueFrom:
                secretKeyRef:
                  name: test-myrelease-secret-config
                  key: password
            command:
              - /bin/cowsay

Event Timeline

Change #1021843 had a related patch set uploaded (by Brouberol; author: Brouberol):

[operations/deployment-charts@master] modules: release a new version of app.job

https://gerrit.wikimedia.org/r/1021843

@jijiki I recall you had dug up some things around jobs/cronjobs as well, maybe you can take a look?

@brouberol thank you for finding this. While I had spotted this in the past, I began working on updating the module T356885, as its current design is somewhat limited. If you are currently blocked, my suggestion is for you to go ahead with this minor update, for now. When I make some time to properly finish T356885, I will update any charts with active jobs anyway. @JMeybohm any objections?

Gehel subscribed.

Removing DPE SRE from this. Decision on what to do needs to be taken outside of our team.