bitnami / charts

Bitnami Helm Charts
https://bitnami.com
Other
9.03k stars 9.22k forks source link

Customize common named templates by parent chart #20570

Open jeremysprofile opened 1 year ago

jeremysprofile commented 1 year ago

Name and Version

bitnami/common 2.x.x

What is the problem this feature will solve?

There are multiple issues recently where users creating an umbrella chart with multiple Bitnami subcharts will get errors when trying to deploy due to identically named templates from those subcharts' dependency on distinct versions of the Bitnami common chart.

Having multiple different, identically named templates does not work in Helm, and there is a workaround suggested:

One popular naming convention is to prefix each defined template with the name of the chart: {{ define "mychart.labels" }}.

Bitnami's current approach seems to require that chart users update all their Bitnami subcharts in lockstep to avoid depending on incompatible versions of the Bitnami common chart and hoping for the best (emphasis mine):

We make sure that all the charts in the repository, individually, use a compatible version of dependencies. The problem arises in custom umbrella charts developed by the community. Yes, bumping to latest versions at the same time would definitely help. Even doing so, you can still hit this issue. So my recommendation would be to check the subchart versions like bitnami/common to see if all of them are in sync.

This adds significant burden to umbrella chart creators, as not only do all dependent charts have to be updated (sometimes with significant values refactoring required, such as bitnami/kafka 24.0.0), but they must also all be validated afterward, and require the umbrella chart creator to more-or-less manually find the latest version of each chart they can update to in order to keep a standardized version of bitnami/common between all Bitnami subcharts. In our case, we use 11 distinct Bitnami charts rolled into one.

The alternative approach is forking these subcharts or downloading the chart tarball and editing them individually, replacing any common.* named template with $SPECIFIC_SUBCHART.common.*. This seems easier to manage, but could be even easier if it were automatically applied by Bitnami.

What is the feature you are proposing to solve the problem?

I see that Bitnami already uses GitHub actions. It seems possible to use this, or some other automation, to replace common.* named templates with chart-specific versions, like kafka.common.* or zookeeper.common.*, both in the common chart template definitions and in their usage by the Bitnami chart.

Alternatively, do a one-time, manual update of all Bitnami charts to have use $CHART_NAME.common.* named templates and then use GH actions to create chart-specific versions of common to have as dependencies.

Admittedly, this is a pretty big ask for what seems like a minority of your users.

What alternatives have you considered?

As mentioned above, the easiest alternative is for umbrella chart creators to download the tarball, store it locally, and edit all common.* named templates into $SPECIFIC_SUBCHART.common.*. This is manageable, but has the downsides of requiring extra effort every time you want to upgrade a Bitnami subchart version.

migruiz4 commented 12 months ago

Hi @jeremysprofile,

Thank you very much for your detailed comment, nevertheless, I don't think the solution for this is to make common chart individual for each chart, because it would then not make sense.

By definition, ALL charts that require bitnami/common:2.x.x should work with both common:2.0.0 or common:2.13.3. If that is not the case, I would appreciate it if you could share the exact charts and versions where this does not meet this condition, because that would mean that we introduced non-backward compatible changes.

A different topic would be having charts that depend on both common:1.x.x and common:2.x.x which is currently a known issue in Helm (https://github.com/helm/helm/issues/11561) for which we are considering several solutions, such as renaming helpers appname.v3.helperName.

migruiz4 commented 12 months ago

Hi there,

After some further investigation, I found out that the issue may be related with version 2.9.0 of the bitnami/common chart, also known as 'Support for customizing standard labels'.

This version introduced new features in a backward-compatible way, and we updated the entire catalog to use it, but apparently, due to a helm issue handling dependencies, if two versions of bitnami/common conflict it does not necessarily take the newest...

My recommendation would be that, if you are experiencing issues when several bitnami/charts are used as sub-charts, please update all of them to a version that uses bitnami/common later than 2.9.0 at the moment, and try to update them regularly.

In case it helps, I think this discussion may be related to this: https://github.com/bitnami/charts/issues/20504#issuecomment-1816609192

github-actions[bot] commented 11 months ago

This Issue has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thanks for the feedback.

jeremysprofile commented 11 months ago

Thanks @migruiz4 :

due to a helm issue handling dependencies, if two versions of bitnami/common conflict it does not necessarily take the newest ... In case it helps, I think this discussion may be related to this: https://github.com/bitnami/charts/issues/20504#issuecomment-1816609192

Yes, this is the issue we are encountering.

try to update them regularly.

Right, this is similar to the advice given on another Bitnami charts issue:

We make sure that all the charts in the repository, individually, use a compatible version of dependencies. The problem arises in custom umbrella charts developed by the community. Yes, bumping to latest versions at the same time would definitely help. Even doing so, you can still hit this issue. So my recommendation would be to check the subchart versions like bitnami/common to see if all of them are in sync.

But the problem is that even if we did update regularly, Bitnami charts are not all released at the same time, so we sometimes would not be able to update to the latest chart version, because another chart has not been updated to include that latest version of common. As I said before:

This adds significant burden to umbrella chart creators, as not only do all dependent charts have to be updated (sometimes with significant values refactoring required, such as bitnami/kafka 24.0.0), but they must also all be validated afterward, and require the umbrella chart creator to more-or-less manually find the latest version of each chart they can update to in order to keep a standardized version of bitnami/common between all Bitnami subcharts. In our case, we use 11 distinct Bitnami charts rolled into one.

This is a significant amount of work every time we want to upgrade.

A different topic would be having charts that depend on both common:1.x.x and common:2.x.x which is currently a known issue in Helm (https://github.com/helm/helm/issues/11561) for which we are considering several solutions, such as renaming helpers appname.v3.helperName.

Are you referring to named templates? Yes, this is one of the strategies I advocated for above:

It seems possible to use this, or some other automation, to replace common. named templates with chart-specific versions, like kafka.common. or zookeeper.common.*, both in the common chart template definitions and in their usage by the Bitnami chart.

Can you please point me to the issue where you are discussing possible solutions so I can voice my support?

migruiz4 commented 11 months ago

While I understand the inconveniences that the bitnami/common approach causes when multiple bitnami charts are used as subcharts. I only see two solutions for this problematic:

I don't really see option 1 happening in the future, so my suggestion would be to report this issue in Helm's repository requesting to improve the subcharts loading process.

As previously mentioned, the workaround for this issue would be updating charts to at least use the same version of bitnami.common when an error occurs.

Please notice this does not necessarily need to happen too frequently, only when changes such as 2.9.0 are introduced and widely used functions are changed for all the charts, and in those cases, we do update all the charts at the same time: https://github.com/search?q=repo%3Abitnami%2Fcharts+%22Support+for+customizing+standard+labels%22&type=pullrequests

github-actions[bot] commented 10 months ago

This Issue has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thanks for the feedback.

yardenshoham commented 10 months ago

Another solution is to version each function in the common library. See https://github.com/bitnami/charts/issues/15753#issuecomment-1647357669

github-actions[bot] commented 10 months ago

This Issue has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thanks for the feedback.

yardenshoham commented 10 months ago

It's not stale

migruiz4 commented 10 months ago

Hi @yardenshoham,

I'm sorry for the late response. I understand the point at https://github.com/bitnami/charts/issues/15753#issuecomment-1647357669 but it would not work for this case.

Versioning helpers would only solve the case when two different majors of the same chart have to coexist.

Let me use these examples using invented versions to display why there are two different issues and why using versioned helper names would only fix one.

main-chart version 2.x.x
 |- Subchart bitnami/postgresql 70.
 |    |- subchart bitnami/common with condition 5.x.x using v5.1.2
 |- Subchart bitnami/redis 44.x.x
      |- subchart bitnami/common with condition 4.x.x using v4.10.1

In this scenario, having versioned helpers would be the only solution because of non-backward-compatible changes. In this case, both helpers from common-4.x.x and common-5.x.x MUST be loaded for both charts to be deployed.

While having two charts using different majors may be more frequent for other charts such as bitnami/postgresql(scenario where your suggestion and contribution comes handy https://github.com/bitnami/charts/pull/17847), that scenario is less probable for bitnami/common and won't be a priority for us until bitnami/common 3.x.x is released.

Now, the scenario of this discussion and why it is an issue with helm/helm and not our charts would be the following:

main-chart version 2.x.x
 |- Subchart bitnami/redis 18.x.x
 |    |- subchart bitnami/common with condition 2.x.x using v2.14.1
 |- Subchart bitnami/postgresql 11.8.2
      |- subchart bitnami/common with condition 2.x.x using v2.0.1.

In this case, we have two subcharts that require common 2.x.x.

Expected behavior would be to use the latest version of them, which would be 2.14.1 because it won't include non-backward-compatible changes, so features in 2.0.1 would be present in 2.14.1, but not the opposite.

Actual behavior is that Helm will randomly load them and the last to be loaded prevails, meaning that there could be the case where common-2.0.1 is loaded after 2.14.1 overriding it.

In that case, the chart bitnami/postgresql 11.8.2 will be deployed successfully, but bitnami/redis 18.x.x won't because it will be missing features in the bitnami/common helpers.

TL;DR - Renaming helpers will only work when common:2.x.x and common:1.x.x (or future 3.x.x) coexist, but won't fix the most frequent scenario, where Helm's loading order may cause two charts depending on the same major (2.x.x) to use the earlier version instead of the older one.

yardenshoham commented 10 months ago

Thanks for the writeup. I understand. You are correct. So the only solution I see is publishing common-X for each version whenever a new version of common is available then using common-X in each dependency.

We should include the version so for example common-5.6.7 would be a new chart published for the source common of version 5.6.7. By namespacing like this, no 2 charts are fighting for loading order. We still have to version each function.

For example Chart.yaml:

dependencies:
- name: common-5.6.7
  repository: oci://registry-1.docker.io/bitnamicharts
  tags:
  - bitnami-common
  version: 5.6.7
migruiz4 commented 9 months ago

@yardenshoham pinpointing each version of bitnami/common would not be feasible. First, there would be no point in using semver if one patch release is not backward-compatible with the previous one because it renames all the helpers. Second, the amount of work it would generate outweighs the benefits of having bitnami/common to share libraries between all bitnami charts.

That would be equivalent to having each chart its own helpers and removing dependencies on bitnami/common.

I'm sorry but I stand on my position that there is no workaround on our side for this issue, and it is Helm that should improve its dependencies loading process.

If you are a user affected by this issue, I would recommend to:

github-actions[bot] commented 9 months ago

This Issue has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thanks for the feedback.

yardenshoham commented 9 months ago

not stale, still a problem

github-actions[bot] commented 8 months ago

This Issue has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thanks for the feedback.

yardenshoham commented 8 months ago

not stale, still a problem