Open mflendrich opened 8 months ago
I would like to do a GA release of the chart in #976 sooner. The scope there (create a chart that deploys the controller and Kong separately, with upgrade paths from both kong/kong 2.x and kong/ingress 0.x) is already known, we have code ready to do it, and it being out has no bearing on the other changes proposed here.
Is there some reason we believe a GA release would prevent us from taking either approach mentioned here later?
Keeping it unreleased requires continuing to deal with the problems associated with umbrella charts requires redoing any change we make to kong/kong 2.x to handle the revised templates and values.yaml structure in the consolidated chart. Enough of the design and implementation of the requests here in #977 are unknown that we'd probably be stuck with that extra burden for a while.
That said, some preliminary reviews of the options:
This seeks to allow installing kong/kong
, kong/foo
, kong/etc
for
different standard configurations using a single chart packaged with a
different base values.yaml. This is, IMO, a bit of overkill for something that
could be handled with a -f https://raw.githubusercontent.com/Kong/charts/main/charts/kong/example-values/minimal-kong-standalone.yaml
,
but it's probably doable with CD hacks.
chart-releaser's package step appears to just be a wrapper around the helm package
command before uploading its output to GH pages. The loader and saver
helm package
uses
don't appear to have much any intelligence around chart structure specifically,
and just serve to package the whole of a chart dir's contents into a tar.gz.
Actually modifying these tools to provide some sort of values.yaml injection would probably require maintaining forks of helm (for a custom package command), chart-releaser (it doesn't let you bring your own helm binary AFAIK), and chart-releaser-action (ditto re BYO chart-releaser binary), which isn't particularly advisable.
The simplicity of the packager does allow us to just copy a chart directory repeatedly and sub out values.yaml before point chart-releaser at that location instead of the default. We also need to update the name in Chart.yaml, though we wouldn't avoid that with modified Helm tooling (Chart.yaml is the apparent source of truth for the name--there's no obvious separately-determined name in the code).
Injecting a complete values.yaml rather than overlaying overrides requires some
method of maintaining separate complete values.yaml. Maintaining these
independently would unfortunaely be a massive pain, as we'd need to update any
key that doesn't need to change between configurations N times for each
variant. Although merging these is simple in Go,
we have no way of invoking Helm's merge tool alone (it only runs as part of
install
or template
) without building our own binary. Using yq
(https://mikefarah.gitbook.io/yq/v/v2.x/merge#overwrite-values) is probably a
viable alternative.
We'd ideally have some clearer expectation around what we cannot do now (or with default values.yaml hacks) before going this route.
Our topologies generally all involve installing a Kong Deployment, with the specifics of its role determined by environment variables. That's an outgrowth of how Kong itself is designed: there is a single Kong image that fulfills different roles based on kong.conf. The surrounding Kubernetes resources generally remain the same regardless of role. If we want to separate them per role, we should know what needs to differ and is difficult or cumbersome to handle via template logic.
The operator is an exception because its chart would presumably not deploy
anything the existing charts deploy, since those charts deploy roughly the same
set of resources the operator is supposed to manage. However, if we plan to
continue supporting installing Kong directly via Helm, an operator chart would
presumably be entirely separate. We could share some common functionality
similar to what's in Bitnami common
, but it's hard to say what the overlap
would be without actually trying to write the operator chart.
The one example I have for using a library dependency (Bitnami's chart library) generally uses it for generic utility functionality shared across charts with significant differences elsewhere (no matter how you deploy Kong, it's going to be pretty different than how you deploy Postgres). For example, they use the library functions to determine a full release name and shared labels for all resources within a chart-specific deployment template.
Dependency charts load the same way regardless of whether they're rendered in
an unbrella chart or simply being used as storage for common templates. For
example. the inclusion of the common chart
in most Bitnami charts comes along withe same Chart.lock pinning
that we currently have in kong/ingress
. We'd still have the same multi-step
releases we do now.
Symlinking a library template file may be a way to circumvent the need for a separate dependency release. We don't have an obvious need to use a separate repo, which would require using Helm's dependency system.
I think that the first option @rainest describes (Single chart, separate values.yamls
) doesn’t look promising. It would mean we’d have non-standard tooling that would be doing the job of releasing the very same chart but with different names and default values. Maintaining forks of helm
or any other dependency looks like a no-go for me. 😟
Picking the second option has its issues but I think it is cleaner and closer to the standard approach (e.g. to how bitnami does that). I’d see that with the following steps to be executed:
kong/kong
to kong/ingress
in https://github.com/Kong/charts/pull/976 and release it as 3.0 (after ensuring all breaking changes we’d like to incorporate into the 3.0 release are already in, e.g. changing the default router type, etc.). I remember @mheap saying that renaming would satisfy the basic requirements we have for now.kong/konnect-dp
, kong/operator
), taking into account that if we find we have to unnecessarily duplicate templates that are already in kong/ingress
, we extract such templates to kong/common
chart. We use the extracted templates from kong/common
in both kong/ingress
and kong/*
new charts.We won’t be able to predict up-front all templates that are really to be extracted to a common chart so I think that trying to create new charts would be the best method to find out what we can extract from kong/ingress
. I think we should make sure we have a clear dependency tree where kong/common
is the only common dependency. No user-facing chart should depend on another user-facing chart (e.g. kong/konnect-dp
should not rely on kong/ingress
). kong/common
should not render any resources by default but it should only export templates to be used (i.e. if I define kong/common
as a dependency in Chart.yaml
it doesn’t make any new resources to be rendered unless I use a common.someTemplateName
in my chart’s templates).
Regarding the issue with multi-step releases in case of modifying something in kong/common
- I believe we could mitigate that by using file://
prefix in the dependencies[].repository
in Chart.yaml
of our user-facing charts. That would allow us to modify kong/common
and release every dependency in one step together with kong/common
. We could write a helper script that would bump the dependency on kong/common
and bump their minor/patch (it could be the script's parameter) in user-facing charts so we don’t have to go one-by-one and manually bump the version in their respective Chart.yaml
. That would mean just a make update-deps
or similar before pushing changes in kong/common
chart.
Problem Statement
We've decided that the 3.0 version of Helm charts will come with a complete revamp of the user experience: instead of starting from the "implementation" (1-1 relationship with the deployment that is either Kong or KIC or both) we will start from the "use case" (KIC, no KIC, Konnect DP, Operator, etc.)
Proposed Solution
First step: merge #976 (3.0.0-alpha.1) Second step: reach agreement on the desired UX. We have two options on the table
kong/ingress
,kong/konnect-dp
,kong/operator
- perhaps more) where default values have a reasonable "batteries included" capabilityvalues.yaml
examples for different use casesWe currently believe that Option 1 is more promising but more research needs to go into it.
Third step: 3.0.0-alpha.2 is implemented in the chosen option out of the above
Fourth step: 3.0.0-alpha.N promoted to 3.0.0
Acceptance Criteria