feat(chart): chart-owned PVCs, HTTPRoute filters and extraManifests

[aaronspruit]

Four additions. Every one is default-off or defaults to today's behaviour, so rendered output is unchanged unless a value is set.

httpRoute gains the ability to set matches, filters, timeouts, additionalRules and name to make it more customizable instead of disabling the httpRoute in the chart and applying it beside the release.

pms.configPersistence renders the config volume as a chart-owned PersistentVolumeClaim instead of a StatefulSet volumeClaimTemplate. A volumeClaimTemplate claim is named pms-config-- and is owned by the StatefulSet, so it cannot be given a stable name and cannot carry helm.sh/resource-policy: keep. That left configExistingClaim as the only way to attach a deliberately-named claim, so the claim had to be created and kept in sync outside the chart. configExistingClaim still takes precedence.

extraPersistentVolumeClaims creates additional claims the release owns, for volumes other than the config one - a backups share, a media library, etc - that otherwise have to be applied out of band and then referenced with extraVolumes.

extraManifests renders arbitrary objects with the release. The chart templates a Service, a StatefulSet, an Ingress and an HTTPRoute; anything else belonging to the deployment (Gateway API policies, NetworkPolicy, ServiceMonitor, ExternalSecret, etc) has to be applied beside the chart today, so helm uninstall leaves it behind.

Both extra* values and additionalRules accept a list or a map, and are passed through tpl so they can reference values and the chart's named templates.

README.md is regenerated with helm-docs 1.14.2.

[cilindrox]

Thanks @aaronspruit - left some comments. Would you consider opening separate PRs for the features so we can keep discussion focused?

[cilindrox]

this seems redundant, given the extraManifests feature - I think having the option to use the configExistingClaim and adding these there should cover most use cases. Maybe we can include the PVC example under the extraManifests stanza in values.yaml ?

[aaronspruit]

yeah, after more thinking, I agree with you and put in the example.

[cilindrox]

nit: this example should follow the extraContainers stanza above

[aaronspruit]

done!

[cilindrox]

nit: wording

Suggested change

	ExternalSecret, ...) can be owned and pruned by this release instead of being
	applied by hand alongside it.
	ExternalSecret, ...) can be owned by this release.

[cilindrox]

I think this description is better suited for the values.yaml description of the entry

[cilindrox]

is this actually required or wouldn't toYaml be greedy enough that we don't care and we can rely solely on the else part of this conditional?

[cilindrox]

instead of name here, we could do a k:v map and a for_each, so k = name. Thoughts?

[aaronspruit]

Thanks @aaronspruit - left some comments. Would you consider opening separate PRs for the features so we can keep discussion focused?

Based on the comments, basically you're asking to re-look at how PVCs are handled in general right? I know my PR was just a quick hack to get my use case working, but want to be sure we are on the same page.

[cilindrox]

The pvcs can be added via extraManifests - I think the use case for the new feature covers these.

httpRoutes can be a dict, so we don't have to do any additional name checks - but I think that one can be a follow-up PR.

[aaronspruit]

Sorry, totally misread these initially. I agree on the PVCs. Both templates are removed.

extraManifests and configExistingClaim together cover the case that I wrote them for. When configExistingClaim is set, the StatefulSet already omits volumeClaimTemplates, so no new template is necessary. values.yaml now documents the config claim as an extraManifests example, as you suggested.

I rebased on master and reduced this PR to extraManifests only. The chart is 1.8.0 and the change is four files. The default render is byte-identical to master, except for the chart version label. HTTPRoute follows in its own PR. The title and body above still describe all four features. Do you want me to rewrite them for extraManifests only, or would you prefer a fresh PR?

I applied the other three comments. I took your suggestion exactly. I moved the list-or-map and tpl description from the template to the values.yaml entry. The stanza now follows the whole extraContainers block, and it no longer splits that block.

I disagree on one point: the kindIs "string" branch. toYaml does not cover a string entry. It renders a string as a YAML scalar, not as a document. The entry then renders as a quoted string instead of an object.

The branch also matters for the PVC example that you asked for. Only a string entry can carry a template of more than one line. values.yaml parses as YAML before tpl runs. As an object, this does not parse:

extraManifests:
  - apiVersion: v1
    kind: PersistentVolumeClaim
    metadata:
      labels:
        {{- include "pms-chart.labels" . | nindent 8 }}

Error: failed to parse values.yaml: error converting YAML to JSON:
yaml: line 6: did not find expected node content

As a string, the same text renders correctly. This is the form that the documented PVC example uses, so dropping the branch would take the example with it. Happy to drop string support entirely if you'd rather have the simpler template, but then the example has to inline the labels by hand.

On httpRoute as a dict with the name as the key: I agree. It is better than a name field, and it also supports more than one route. It is a breaking change to the values shape. I will do it in its own PR, together with the matches, filters, and timeouts work.