bigbang 3rd party package chart not properly passing some toplevel chart values to subpackages

[p1-repo-sync-bot[bot]]

Bug

Description

While working on Jira #67 I discovered that some of the builtin networkPolicies do not function because the controlPlaneCidr is not properly inherited from the parent chart values.

Consider:

egress-kube-api.yaml

apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
  name: egress-kube-api
  namespace: {{ .Release.Namespace }}
spec:
  podSelector: {}
  policyTypes:
  - Egress
  egress:
  - to:
    - ipBlock:
        cidr: {{ .Values.networkPolicies.controlPlaneCidr }}

... the assumption is that the bigbang 3rd party package chart will appropriately pass through the networkPolicies.controlPlaneCidr, which was set at the top level parent (bigbang/chart/values.yaml). However this isn't the behavior of the 3rd party package chart. The global values are not merged into (or beneath) the values for the 3rd party package.

bigbang/chart/templates/package/values.yaml

{{- $pkg = include "resourceName" $pkg -}}
{{- $defaults := $.Files.Get (printf "defaults/%s.yaml" $pkg) -}}
{{- if $defaults -}}
{{- $vals := mergeOverwrite $vals ($defaults | fromYaml).package -}}
{{- end -}}
apiVersion: v1
kind: Secret
metadata:
  name: {{ $pkg }}-values
  namespace: {{ dig "namespace" "name" $pkg $vals }}
  labels:
    {{- include "commonLabels" $ | nindent 4 }}
type: Opaque
stringData:
  {{ if and (dig "enabled" true $vals) (not $vals.kustomize) -}}
  values.yaml: |
  {{- tpl (toYaml $vals.values) $ | nindent 4 }}
  {{ else }}
  {{- tpl (toYaml $vals.values) $ | nindent 2 }}
  {{ end }}

Deploying these charts in this state results in helm installation failures:

1s          Warning   InstallFailed      HelmRelease/jira   Helm install failed for release jira/jira with chart jira@1.18.1-bb.0+3ab13cc1dbc2: 1 error occurred:
            * NetworkPolicy.extensions "egress-kube-api" is invalid: spec.egress[0].to[0].ipBlock.cidr: Required value

Maybe this is working as designed, but it is confusing to me to have the global spaces inherited/extended by all the other bigbang charts but not these packages.

BigBang Version

bigbang-2.22.0

[p1-repo-sync-bot[bot]]

michaelmartin commented:

---

I did a quick look just to see how this works. I believe right now, it is known that top-level bigbang values are not passed down/merged. I found an earlier quote from some work being done with the 3rd-party packages some time ago:

Mattermost discussion quote:

One major piece still left to implement for a complete solution. The BB values are still hardcoded in the package. Need to implement BB values passthrough to the package via a secret created in the package namespace.

Currently, when the wrapper is disabled, the defined package values are passed through in a secret, but bigbang's values are not merged into this secret. So, for now, to get this to work, one has to:

packages:
  jira:
    enabled: true
    wrapper:
      enabled: false
    values:
      networkPolicies:
        enabled: true
        controlPlaneCidr: x.x.x.x/x

not ideal, but there is a work-around until this feature is implemented.

[p1-repo-sync-bot[bot]]

akesterson commented:

---

I haven't yet reached the bottom of this rabbit hole, but here's a useful discovery.

TL;DR - Always quote your value references in your yaml.

Consider this test data ran against the current master branch and provided with -f values_file.yaml:

packages:
  jira:
    enabled: true
    wrapper:
      enabled: false
    values:
      networkPolicies:
        enabled: true
        controlPlaneCidr: {{ .Values.networkPolicies.controlPlaneCIDR }}

... this does not work. However, if we do:

packages:
  jira:
    enabled: true
    wrapper:
      enabled: false
    values:
      networkPolicies:
        enabled: true
        controlPlaneCidr: "{{ .Values.networkPolicies.controlPlaneCidr}}"

... it succeeds and we get the value as expected. The reason the first test case succeeds, and the second test case fails, is that the first case is being interpreted by the golang YAML parser, rather than being passed through to the helm template parser. Quoting the value as a string passes it through to the helm template parser and we get the result we expect.

This indicates that the parent bigbang value space is, actually, already available for use in helm templates in a package's namespace. No additional work seems necessary.

Troubleshooting this was quite confusing due to the fact that we have several examples of templates using the exact same notation without quotes, and succeeding. Here are a few examples:

./chart/templates/thanos/values.yaml:33:  controlPlaneCidr: {{ .Values.networkPolicies.controlPlaneCidr }}
./chart/templates/mattermost/values.yaml:67:  controlPlaneCidr: {{ .Values.networkPolicies.controlPlaneCidr }}
./chart/templates/monitoring/values.yaml:24:  controlPlaneCidr: {{ .Values.networkPolicies.controlPlaneCidr }}
./chart/templates/keycloak/values.yaml:38:  controlPlaneCidr: {{ .Values.networkPolicies.controlPlaneCidr }}
./chart/templates/vault/values.yaml:61:  controlPlaneCidr: {{ .Values.networkPolicies.controlPlaneCidr }}

... but upon investigation, these are only working because they are not actually being loaded by the yaml parser the same way our values file would be, by passing them to helm:

-f /path/to/values/file.yaml

... Rather, they are being parsed directly by helm/golang's templating engine before being parsed as yaml. So things like this:

chart/templates/vault/values.yaml

# ... snip ...
{{- define "bigbang.defaults.vault" -}}
networkPolicies:
  enabled: {{ .Values.networkPolicies.enabled }}
  controlPlaneCidr: {{ .Values.networkPolicies.controlPlaneCidr }}
# ... snip ...
{{- end }}

... Don't even get seen by the yaml parser, because they get converted to helm template macros. And then when they get used, they still don't get parsed by the yaml parser in this state:

{{- include "values-secret" (dict "root" $ "package" .Values.addons.vault "name" "vault" "defaults" (include "bigbang.defaults.vault" .)) }}

... which causes the entire body of the defaults macro to be parsed as a helm template, resulting in the {{ .Values.networkPolicies.controlPlaneCidr }} being replaced with a string before the YAML parser ever sees it. So if you look at this and say "Code X is working fine, why not mine?", it's confusing.

[p1-repo-sync-bot[bot]]

akesterson commented:

---

I was having trouble reproducing the original issue, not sure why, reproduction today was rather straightforward:

$ bbctl k3d destroy
$ bbctl k3d create
$ bbctl deploy flux
$ bbctl deploy bigbang --k3d -- -f ~/source/repo1.dso.mil/big-bang/overrides/team-storagecollab.yaml -f ~/source/repo1.dso.mil/big-bang/overrides/jira.yaml

# .. snip ..

$ kubectl describe helmrelease jira -n jira
Events:                     
  Type     Reason            Age    From             Message                    
  ----     ------            ----   ----             -------                                                                                                    
  Normal   HelmChartCreated  9m13s  helm-controller  Created HelmChart/jira/jira-jira with SourceRef 'GitRepository/jira/jira'                                  
  Warning  InstallFailed     8m11s  helm-controller  Helm install failed for release jira/jira with chart jira@1.19.0-bb.1+5183fc8b96e7: 1 error occurred:      
           * NetworkPolicy.networking.k8s.io "egress-kube-api" is invalid: spec.egress[0].to[0].ipBlock.cidr: Required value

[p1-repo-sync-bot[bot]]

akesterson commented:

---

I have a simple minimal fix in MR 4257, here:

https://repo1.dso.mil/big-bang/bigbang/-/merge_requests/4257

... But I don't think this is the way we want to solve this. After looking at the wrapper, I think we would probably prefer to have a bigbang map at the top level of the values space, and reproduce pretty much everything there for packages, the same way we do for the wrapper. It's more work but I think that's more consistent. The logic in the wrapper will probably need to be pulled out into a helper function. I'll work on that version now and send a separate MR so we can A/B the solutions.

[p1-repo-sync-bot[bot]]

akesterson commented:

---

The second solution is indeed superior. I've opened https://repo1.dso.mil/big-bang/bigbang/-/merge_requests/4263 and closed https://repo1.dso.mil/big-bang/bigbang/-/merge_requests/4257

[p1-repo-sync-bot[bot]]

Issue 'bigbang 3rd party package chart not properly passing some toplevel chart values to subpackages' closed from GitLab side