porch: consider Helm chart support as a package format

[liamfallon]

Original issue URL: https://github.com/kptdev/kpt/issues/3847 Original issue user: https://github.com/johnbelamaric Original issue created at: 2023-02-23T21:46:32Z Original issue last updated at: 2023-02-25T01:18:41Z Original issue body: Helm is the most widely used solution today for K8s workload packaging. Of course, kpt and config-as-data is the approach this community wants to promote and believes is the better long-term alternative. However, we still need to integrate with the existing ecosystem.

The question is, what is the best way to interface to Helm? In kpt CLI, we do have imperative support for rendering helm charts using the render-helm-chart function. This can't really be used in Porch, at least not directly through the API. A few ideas for approaches:

1. We could add Helm chart as a possible type of package under Porch's management. It would simply present as a special type of PackageRevision. We would require a "render" phase in order for the kpt pipeline to kick in. If we did that we would need a way to supply the inputs which could be messy.
2. We could add support in the PackageVariant for a Helm chart as an upstream package, and use that controller as an upstream (see #3827).
3. We could add a new type of resource that specifically renders a Helm chart into a Kpt package.
4. We could add the Helm chart in a special directory in a kpt package, which is ignored during the function pipeline (effectively, the same way we treat application config, except some of these files will probably have yaml extensions). Then, we could add support for calling the render-helm-chart, because we wouldn't need network or volume support, everything would be right there in the package. (Or, we may want to make a native way to render helm charts that is simpler than that function?)

The nice thing about the last one is that you can use the kpt pipeline / KRM functions both before and after the render. So, you can use it to prepare and create the complete values file (or an input ConfigMap we convert to a values file), and also do make modifications to the new resources created by the render operation. This would allowing us to leverage, for example, all the injection functionality we are building in PackageVariant to help construct the render inputs, and the package conditions functionality (#3455) for coordinated post-render customization.

An approach like that can provide a transition path - things in the Helm chart that make more sense in the kpt package could migrate out of the chart bit-by-bit, reducing the surface area of what Helm renders.

Thoughts / criticisms / ideas?

cc @mortent @droot @natasha41575 @aravind254 @tliron @henderiw @s3wong

Original issue comments: Comment user: https://github.com/johnbelamaric Comment created at: 2023-02-23T21:48:42Z Comment last updated at: 2023-02-23T21:48:42Z Comment body: Question - could approach 4) be accomplished today with .krmignore? Maybe that's a way to prototype it.

Comment user: https://github.com/tliron Comment created at: 2023-02-23T23:16:05Z Comment last updated at: 2023-02-23T23:19:00Z Comment body: My first thought was indeed option 4. The whole Helm chart is in the kpt package including the values.yaml if one is needed. The first kpt function in the pipeline would be "render-helm-chart", which would run helm template or an equivalent on the chart and pass on other CLI arguments. So, after this first phase of the pipeline we would have regular old KRM.

To support chart repositories we can add yet another kpt function called "pull-helm-chart", which would pull it from a repository. So the most minimalistic version would be a single Kptfile that pulls and then renders.

There are subtle potential issues. helm template is not always good enough, because some charts do refer to runtime values and do not deterministically produce the same results for every run. But this could be a documented limitation. And in any case if you are going to the final cluster via Config Sync, the chart is not rendered there anyway. The cluster might not even exist yet. We could potentially forbid any runtime values. (It's a problematic feature, anyway.)

Also, many Helm charts in the wild come with pretty big Helm plugins that can do ... literally anything. If Porch is running helm commands, then it might need to do so in a sandboxed environment, possibly an extra Pod called "helm-runner" or similar.

Comment user: https://github.com/mortent Comment created at: 2023-02-23T23:56:27Z Comment last updated at: 2023-02-23T23:56:27Z Comment body: There is some prior art with using something similar to option 4 in kpt: https://github.com/GoogleContainerTools/kpt/issues/517. I haven't looked at it in a while, but it seems similar.

Another idea might be to be able to support generator functions as upstream in kpt packages. So rather than specifying a package as the upstream, it would be a function and a set of parameters. It needs some more thinking on whether this is feasible, but it might be a possible path.

Comment user: https://github.com/johnbelamaric Comment created at: 2023-02-24T00:05:53Z Comment last updated at: 2023-02-24T00:05:53Z Comment body: > The first kpt function in the pipeline would be "render-helm-chart",

That was my first thought too, but in fact if we ignore chart contents in the function ResourceList (or treat it as opaque application config - something @yuwenma has a solution for already), then the result of render function can be to simply add the Helm generated resources to the ResourceList. This makes: 1) all elements of the original ResourceList available to manipulate and feed into the Helm render; and 2) the output of the Helm render as just more resources that the rest of the pipeline can manipulate. Just like any other function, the eventual output is stored in the package.

There would be some limitations - for example, the rendering of the Helm chart should be idempotent. But I suspect that's usually the case for existing Helm charts.

There is some prior art with using something similar to option 4 in kpt: https://github.com/GoogleContainerTools/kpt/issues/517. I haven't looked at it in a while, but it seems similar.

Yes, that does look like someone doing something quite similar, at least in the initial request.

Comment user: https://github.com/yuwenma Comment created at: 2023-02-24T02:32:24Z Comment last updated at: 2023-02-24T02:32:24Z Comment body: I'd vote for option 3 which further isolates the helm rendering workflow from other kpt/porch initiative approaches and make the least affection to current kpt/porch logic IIUC. This is also seems to be similar to what github.com/GoogleContainerTools/skaffold eventually chose which optimize their helm user experiences whom are not ready to use other rendering solutions.

Comment user: https://github.com/natasha41575 Comment created at: 2023-02-24T18:23:38Z Comment last updated at: 2023-02-24T18:38:37Z Comment body: I think I agree with @yuwenma that option 3 has the desirable quality that it would be isolated and separate logic. Option 4 seems similarly noninvasive. Options 1 and 2 seem to require first-class support of Helm charts in core kpt/porch functionality, and I don't know if we want to head in that direction.

Option 4 is interesting, and I can see the utility in being able to modify the resources either before or after the render.

Question - could approach 4) be accomplished today with .krmignore? Maybe that's a way to prototype it.

The render-helm-chart function would still have access to the files in the directories specified by the .krmignore file AFAIK, while the kpt functions would ignore them. So I think this approach or something similar could work and is worth trying out.

Comment user: https://github.com/tliron Comment created at: 2023-02-24T19:02:36Z Comment last updated at: 2023-02-24T19:02:59Z Comment body: Good discussion. I'll add a reason for my preference for option 4 -- from a Nephio perspective we really don't want to treat Helm charts as simply opaque generators of KRM. We want Helm's KRM "output" to be an intrinsic part of the process, so not really an "output" at all. In other words, treat Helm as a frontend rather than a generator. It could be that option 3 can achieve this as well.

Comment user: https://github.com/johnbelamaric Comment created at: 2023-02-24T19:07:42Z Comment last updated at: 2023-02-24T19:07:42Z Comment body: The problem with option 3 is that it is more like an "import" step, and therefore is effectively a prerequisite operation rather than part of the overall hydration flow. In other words, I can't automate it as easily - it is not easily composable with PackageVariant. It's not totally disconnected - PackageVariant could consume the output - but it's harder to build that into a workflow where we have people constructing packages that in turn reference other packages.

Anyway, I'll see if I do option 4 with existing functionality, which would be really nice. Then we can see how well it works and whether we want to add more purpose-built support for it or if the existing functionality is sufficient.

Comment user: https://github.com/yuwenma Comment created at: 2023-02-25T01:18:41Z Comment last updated at: 2023-02-25T01:18:41Z Comment body: I don't know much about PackageVariant, so maybe some of the issues we encountered in kpt and other places (like kustomize) may not be a problem here.

Just want to share one more thought: I think as long as the approach define a clear scope for the 3 config operation phases (hydration, package publishing/consuming and actuation), it can be treated as a good approach. Mixing the tools in the hydration phase (e.g. helm template + kpt fn + kustomize build) can be really painful, but it is not a problem if mixing helm oriented resources to the kpt hydration workflow (the idea of "helm as a generator" original comes from this).

The problem of 3 is its constraints to cooperate helm with other part of kpt. In some cases this is desirable because it avoids the conflicts of hydration tools by nature. But if we do need a more interactive hydration approach, I agree option 4 will then be a better choice.