UNCLASSIFIED - NO CUI

Skip to content
Snippets Groups Projects

#1993 : Pass big bang values through to package charts

Merged #1993 : Pass big bang values through to package charts
1993_package_bbvars_passthrough_opt2 into master
Merged Andrew Kesterson requested to merge 1993_package_bbvars_passthrough_opt2 into master

General MR

Summary

This MR duplicates the logic currently present in the wrapper chart for presenting bigbang variables in the wrapped package's value space, and provides the same functionality to user defined or community supported packages that do not use the wrapper.

Before this patch, this (intentionally simple and contrived) test case would technically succeed when not using the wrapper:

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

... The chart will successfully render, but deployment will fail because the value is empty:

NetworkPolicy.networking.k8s.io "egress-kube-api" is invalid: spec.egress[0].to[0].ipBlock.cidr: Required value

This is because (as pointed out in #1993 (closed)) the wrapper chart has logic to merge bigbang values into the package values under a toplevel bigbang: key, while the bare package chart does not. By updating the package {{ $pkg }}-values and {{ $pkg }}-wrapper-values secret generators, and abstracting the wrapper value generation logic into a helm template that we can re-use in the package generator, we can get the same behavior in both locations, so that now we can:

networkPolicies:
  enabled: true
  controlPlaneCidr: 192.168.254.0/24
packages:
  jira:
    wrapper: false
    enabled: true
    values:
      networkPolicies:
        controlPlaneCidr: "{{ .Values.bigbang.networkPolicies.controlPlaneCidr }}"

The discussion of "why use a toplevel bigbang key, why not just merge the roots together?" has already been discussed when this functionality was built into the wrapper chart and so isn't debated here. TLDR - because there are things in the toplevel of the package value space and the bigbang value space (like openshift) that are incompatible in type and usage pattern. Separation is necessary.

Relevant logs/screenshots

This is proven effective with a chart generation using my jira override from my dotfiles and comparing the results. Here is a summary of the test results:

branch wrapper? result file result md5sum
master no master-nowrapper-values.yaml e9bf50493dd13d50ac7b2f37ac048e54
1993_package_bbvars_passthrough_opt2 no 1993-nowrapper-values.yaml 72f5fc5de7f4e5d4a4b69bb1e82d827e
master yes master-wrapper-values.yaml 5da70c0350bd5686dfde934c54b978da
1993_package_bbvars_passthrough_opt2 yes 1993-wrapper-values.yaml 5da70c0350bd5686dfde934c54b978da

This tells us that the new code produces identical values for the wrapper as the master branch. This proves that the new values-bigbang helper method produces identical output to the previous version.

However we need to prove that the output of the new toplevel bigbang: map entry is identical in the wrapper and non-wrapper versions. We can use yq to extract the bigbang data, sort it, and md5sum it from both versions:

$ cat 1993-nowrapper-values.yaml | yq '.stringData.*' | yq -P '.bigbang | sort_keys(..)' | md5sum
02bde2050f9a6933f4b2b980de9fb777  -
$ cat 1993-wrapper-values.yaml | yq '.stringData.*' | yq -P '.bigbang | sort_keys(..)' | md5sum
02bde2050f9a6933f4b2b980de9fb777  -

Linked Issue

Closes #1993 (closed)

Upgrade Notices

N/A

Edited by Andrew Kesterson

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Michael Martin
  • Andrew Kesterson added 11 commits

    added 11 commits

    Compare with previous version

  • Andrew Kesterson added 1 commit

    added 1 commit

    Compare with previous version

  • Andrew Kesterson added 1 commit

    added 1 commit

    Compare with previous version

  • Andrew Kesterson added 19 commits

    added 19 commits

    Compare with previous version

  • Andrew Kesterson added 3 commits

    added 3 commits

    Compare with previous version

  • Andrew Kesterson added debug label

    added debug label

  • Andrew Kesterson added 15 commits

    added 15 commits

    Compare with previous version

  • Andrew Kesterson added 13 commits

    added 13 commits

    Compare with previous version

  • bigbang bot changed milestone to %2.28.0

    changed milestone to %2.28.0

  • Andrew Kesterson added 17 commits

    added 17 commits

    Compare with previous version

  • Andrew Kesterson added 9 commits

    added 9 commits

    Compare with previous version

  • Andrew Kesterson added 43 commits

    added 43 commits

    Compare with previous version

  • bigbang bot
    bigbang bot @bb8-bot ·
    Developer

    @andrewshoell : You have been tagged in this merge request for the purpose of conducting secondary review.

  • Michael Martin resolved all threads

    resolved all threads

    • Michael Martin
      Michael Martin @michaelmartin ·
      Owner
      Resolved by Michael Martin

      I don't think this is working. Also, the code examples commented above are not entirely syntax-correct in regards to disabling the wrapper.

      Using a default controlPlaneCidr: of 10.19.71.0/24, if I run this on master branch:

      packages:
  jira:
    enabled: true
    wrapper: 
      enabled: false
    values:
      networkPolicies:
        enabled: true
    git:
      repo: https://repo1.dso.mil/big-bang/product/community/jira.git
      tag: "1.19.0-bb.3"
      path: "./chart"

      I get this generated in jira-values secret:

      values.yaml: |
  networkPolicies:
    enabled: true

      This produces the error, since I turned on networkPolicies, but controlPlaneCidr is not passed through -- the crux of the issue.

      On this MR branch, I still set the original error, and the generated jira-values secret is this which doesn't appear to merge stuff in correctly, since it's (1) passing more key/values pairs than necessary and (2) everything is nested under the bigbang key which the package won't recognize.

      values.yaml: |
  bigbang:
    domain: dev.bigbang.mil
    openshift: false
    addons:
      anchore:
        enabled: false
      argocd:
        enabled: false
      authservice:
        enabled: false
      eckOperator:
        enabled: false
      fortify:
        enabled: false
      gitlab:
        enabled: false
      gitlabRunner:
        enabled: false
      harbor:
        enabled: false
      holocron:
        enabled: false
      keycloak:
        enabled: false
      mattermost:
        enabled: false
      mattermostOperator:
        enabled: false
      metricsServer:
        enabled: auto
      minio:
        enabled: false
      minioOperator:
        enabled: true
      nexusRepositoryManager:
        enabled: false
      sonarqube:
        enabled: false
      thanos:
        enabled: false
      vault:
        enabled: false
      velero:
        enabled: false
    clusterAuditor:
      enabled: false
    eckOperator:
      enabled: true
    elasticsearchKibana:
      enabled: true
    fluentbit:
      enabled: true
    gatekeeper:
      enabled: false
    grafana:
      enabled: true
    istio:
      enabled: true
    istioOperator:
      enabled: true
    jaeger:
      enabled: true
    kiali:
      enabled: true
    kyverno:
      enabled: true
    kyvernoPolicies:
      enabled: false
    kyvernoReporter:
      enabled: true
    loki:
      enabled: false
    monitoring:
      enabled: true
    networkPolicies:
      controlPlaneCidr: 10.19.71.0/24
      enabled: true
      nodeCidr: ""
      vpcCidr: 0.0.0.0/0
    neuvector:
      enabled: false
    promtail:
      enabled: false
    tempo:
      enabled: true
    twistlock:
      enabled: false
  networkPolicies:
    enabled: true

      Reach out on MM, and I can help out. I think we want to make sure this is clean in passing values through--we won't want to pass through unneeded values if possible.

      I removed all-packages, since we really need to manually test this outside of CI to make sure the packages: addons values are merged correctly.

      Edited by Michael Martin
  • Michael Martin added statusdoing label and removed statusreview label

    added statusdoing label and removed statusreview label

  • Michael Martin removed all-packages label

    removed all-packages label

  • Andrew Kesterson added 27 commits

    added 27 commits

    Compare with previous version

  • Andrew Kesterson requested review from @michaelmartin

    requested review from @michaelmartin

  • Andrew Kesterson removed statusdoing label

    removed statusdoing label

  • Andrew Kesterson added statusreview label

    added statusreview label

  • Michael Martin resolved all threads

    resolved all threads

  • Michael Martin added 36 commits

    added 36 commits

    • 7a571ff5...604fe512 - 34 commits from branch master
    • f647494c - Merge remote-tracking branch 'origin/master' into 1993_package_bbvars_passthrough_opt2
    • 815314cc - Merge remote-tracking branch 'origin/master' into 1993_package_bbvars_passthrough_opt2

    Compare with previous version

  • Michael Martin approved this merge request

    approved this merge request

  • Michael Martin enabled an automatic merge when the pipeline for 815314cc succeeds

    enabled an automatic merge when the pipeline for 815314cc succeeds

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
    • Please register or sign in to reply

    UNCLASSIFIED - NO CUI