HelmRelease OpenAPI spec doesn't specify merge keys for valuesFrom

[ryphon]

Wildly specific title, I know. Very closely related to an issue in Kustomize.

In my use case, I'm following similarly to the multi-tenant example. I have a base HelmRelease, and an overlay HelmRelease with a kustomize.yaml that's doing a strategicMerge on the HelmRelease objects.

Base HelmRelease has a valuesFrom:

kind: HelmRelease
metadata:
  name: mimir
spec:
  releaseName: mimir
  interval: 5m
  chart:
    spec:
      chart: mimir-distributed
      version: 5.0.0
      sourceRef:
        kind: HelmRepository
        namespace: flux-system
        name: mimir
  valuesFrom:
    - kind: ConfigMap
      name: mimir-values

Overlay HelmRelease has a values: and a different valuesFrom:

kind: HelmRelease
metadata:
  name: mimir
  namespace: mimir
spec:
  chart:
    spec:
      version: "5.0.0"
  valuesFrom:
    - kind: Secret
      name: mimir-slack-webhook
      valuesKey: values.yaml
  values:
    alertmanager:
      inhibitRules: []

Due to the Kustomize issue linked above, the end HelmRelease and kustomization.yaml to create it looks like:

namespace: mimir

resources:
  - ../../bases/grafana/mimir

patchesStrategicMerge:
  - flux-helm.yaml

kind: HelmRelease
metadata:
  labels:
    kustomize.toolkit.fluxcd.io/name: mimir
    kustomize.toolkit.fluxcd.io/namespace: flux-system
  name: mimir
  namespace: mimir
spec:
  chart:
    spec:
      chart: mimir-distributed
      sourceRef:
        kind: HelmRepository
        name: mimir
        namespace: flux-system
      version: 5.0.0
  interval: 5m
  releaseName: mimir
  values:
    alertmanager:
      inhibitRules: []
  valuesFrom:
  - kind: Secret
    name: mimir-slack-webhook
    valuesKey: values.yaml

And according to the Kustomize issues from above, the issue relies on having openAPI schemas to properly understand how to merge these values rather than doing a replacement.

Specifically, I believe the HelmRelease object needs some keys similar to:
"x-kubernetes-patch-merge-key": "name",, and "x-kubernetes-patch-strategy": "merge", but I haven't dug enough into the OpenAPI structures to understand what's required to actually merge these lists together.

[stefanprodan]

You can use a json patch to add an item to the list e.g.:

patches:
  - patch: |
      - op: add
        path: '/spec/valuesFrom/-'
        value:
            kind: Secret
            name: mimir-slack-webhook
            valuesKey: values.yaml
    target:
      kind: HelmRelease
      name: mimir

[ryphon]

I was trying to avoid multiple patches, I think having both the strategic merge and a json patch makes the whole thing a bit harder to read.

I think doing a strategic merge does introduce some undefined behavior, in where which list item becomes first vs second vs n.

Alright, I think I'll just add the patch and move on.

[stefanprodan]

Strategic merge is problematic especially with HelmReleases because the order of the valuesFrom items matters https://fluxcd.io/flux/components/helm/helmreleases/#values-overrides.

I'm not dismissing the issue with our OpenAPI spec, but adding the merge strategy now would mean a major breaking change. We could cause major problems and even downtime to users that rely on the current replace behaviour. Depending on how the charts are written, merging the array could result in StatefulSets being deleted and who know what other things.

[ryphon]

Yeah, and strategic merge doesn't really have a capacity to understand list ordering. Nor does the OpenAPI spec handle the idea of appending or prepending for list merges.

I see your point, and I'm not sure the spec should merge the lists. Ordering is important and I think the strategic merge isn't the path forward with this kind of situation.

I guess then I'll keep the issue open, maybe just to specify on the openAPI to do replace for a strategic merge so it's defined rather than relying on a default kustomize behavior.