ztunnel chart doesn't respect `global.tag/hub`

[bleggett]

Please provide a description of this PR:

Original issue: Acceptance tests were using global to override tag/hub for all charts, but that wasn't actually working for the ztunnel chart, leading to weird test failures if you set HUB/TAG or otherwise tried to run tests with nondefault hub/tag values.

Root cause: Behavior here is generally different between helm and istioctl, and between our charts, which are not consistent with each other - some respect globals overrides for hub/tag/variant, some do not. This makes composition a little weird and is generally pretty confusing. Most of this is due to legacy concerns.

This fixes

• that
• some test cleanup + dumping misses.
• removes globals-flattening in istioctl - we don't just ignore globals for ztunnel, we actively de-scope and flatten them (only for ztunnel, only in istioctl) - this is confusing, and breaks Helm globals semantics generally. Regardless of whether we use globals or not in our charts as a matter of convention, we shouldn't do things that actively break Helm semantics or scoping behaviors.
• Fixes a few leftover spots in tests where we were still setting variant (debug or distroless) differently per-chart, when we should just handle it exactly the same way for all charts (knocks out a few of the tasks in https://github.com/istio/istio/issues/24857)
• Stamps out image properties (hub/tag/variant logic) into a shared template (lives in our profile override logic), which all charts use exactly the same way, so we get exactly the same results.
• Un-special-scopes cni.hub/tag/variant/pullPolicy and pilot.hub/tag/variant/pullPolicy into more normal chart-scoped values, so all charts are consistent here too now (it's always --set hub/tag/variant no matter which chart)

tl;dr

• chart behavior is consistent for all charts for commonly-overridden image values.
• legacy value paths are respected (pilot.tag/cni.tag), if supplied by user at templating time.
• Whether a chart previously used globals for other fields or not, for image-related values of (hub, tag, variant, and imagePullPolicy) may be overridden with globals or chart-local value paths, in the same way, for every chart. This lets us use globals for just-images without requiring globals overrides for every one of those values in each chart.

[bleggett]

isn't this intended? there is no .global at all under ztunnel

No, I don't think so, and I don't know why it would be intended (just for this chart - we already respect it in other spots).

Acceptance tests set global.tag/hub in their overrides: https://github.com/istio/istio/blob/master/tests/integration/helm/util.go#L81 - that's respected everywhere but in this chart.

[howardjohn]

Meta helm question. Respecting global seems to be a layer violation. The child chart needs to be aware that it might be composed by a higher level chart.

Is that just how it works? Everyone is supposed to respect global on every single value, and there are no layers?

[bleggett]

Meta helm question. Respecting global seems to be a layer violation. The child chart needs to be aware that it might be composed by a higher level chart.

Is that just how it works? Everyone is supposed to respect global on every single value, and there are no layers?

values.global.foo is unscoped. global is the only unscoped value prefix. It is values.global.foo no matter what chart you access it from or how those charts are composed - parent value wins.

It's something every chart may choose to respect, if present, and is useful for things that are the same across charts. Like tag/hub.

If you compose charts, you can define global default vals in one spot - the top level. We don't compose charts (well, we do with the ambient chart now), so a lot of the intended benefits are lost - we just use it as a nice alias to avoid having to do --set values.cni.tag --set values.istiod.tag --set values.ztunnel.tag.....

Everyone is supposed to respect global on every single value

You're supposed to decide what you want globally scoped. In all the other spots, we are already deciding to make tag/hub/variant locally overridable, but globally defaultable. That seems reasonable to me (and is relatively common).

[howardjohn]

That is dumb. It should merge for you. Lame.

How do we decide what to bloat with globalsupport vs not? Is it just tag and hub

[bleggett]

How do we decide what to bloat with globalsupport vs not? Is it just tag and hub

Good rule of thumb is "thing you set to the same value for every chart you install, when you install charts together".

Tag/hub/variant fit that nicely, we might throw image pull secret in there too, but don't need more IMO.

the biggest annoyance is the duplication in values files but that's one of the prices we pay for a flat/non-nested chart topology.

Also very easy to throw the "if local otherwise global" checks that should be common into a shared template (but sharing requires either duplicating the template file in each chart, or nesting/using wrapper charts IIRC) - the entire "build image" logic can be a template and it would be the same for every single chart.

[costinm]

I vote for 'intentional' - composing charts, in particular ztunnel is looking for trouble.

We have global in istio charts because long, long ago we tried to compose charts as a way to keep the spaghetti of citadel/pilot/gateway/etc - which was split in parts because they were 'independent microservices' can be installed all at once since they didn't actually upgrade independently.

It also makes helm charts even more scripty and harder to understand - for no real benefit. Setting 'global.tag' instead of 'tag' is not more intuitive if most of the installs are not 'global'.

[costinm]

And the rule of thumb: you should not install things together if you're not going to operate them together (i.e. upgrade at the exact same time, etc). And if you have 2 charts you own that must be operated together - it's a sign you should combine them in one chart.

We made this mistake already - I think the rule was 'new mistakes only' :-)

[bleggett]

We have global in istio charts because long, long ago we tried to compose charts as a way to keep the spaghetti of citadel/pilot/gateway/etc - which was split in parts because they were 'independent microservices' can be installed all at once since they didn't actually upgrade independently.

It also makes helm charts even more scripty and harder to understand - for no real benefit. Setting 'global.tag' instead of 'tag' is not more intuitive if most of the installs are not 'global'.

Depends on the context - you either set the tags once globally, or you must set them N times for N charts to the same value, which is cumbersome in spots - see the integration tests, or our docs.

And the rule of thumb: you should not install things together if you're not going to operate them together (i.e. upgrade at the exact same time, etc). And if you have 2 charts you own that must be operated together - it's a sign you should combine them in one chart.

We made this mistake already - I think the rule was 'new mistakes only' :-) I vote for 'intentional' - composing charts, in particular ztunnel is looking for trouble.

I'm going to support both global and per-chart here, so it should compose like it did before.

• chart-level continues to be preferred over any global setting, so if you don't want to use global, just don't set it and you get the same experience as you would if it didn't exist.

• if chart level not set but global is set, global will be used.

• however you compose charts, you are free to use either to your taste

doing it like this is pretty normative Helm stuff and doesn't put any constraints on composition (or not) of charts, so it seems reasonable to me - I want consistency mostly, and for consistency, we should just be sharing templates for bespoke logic (which we already have for all charts for all image tags).

[bleggett]

That is dumb. It should merge for you. Lame.

How do we decide what to bloat with globalsupport vs not? Is it just tag and hub

I broadened the scope a bit to make the behavior consistent among (istiod/cni/ztunnel) and elide all of this in a template, which is a little better. LMKWYT.

[bleggett]

Hit a side effect of https://github.com/istio/istio/issues/50387, so removed the workarounds mentioned there, we have cut 1.22 and already default to distroless, so we can remove some of the exclusions we needed before that was the case.

[bleggett]

/test integ-helm

[bleggett]

integ-helm is passing locally for me so something is still up where we're special-casing ztunnel images for no reason, looking.

(also as @dhawton pointed out none of the helm tests generate state/log artifacts)

[costinm]

On Tue, Jun 11, 2024 at 2:06 PM Ben Leggett @.***> wrote:

We have global in istio charts because long, long ago we tried to compose charts as a way to keep the spaghetti of citadel/pilot/gateway/etc - which was split in parts because they were 'independent microservices' can be installed all at once since they didn't actually upgrade independently.

It also makes helm charts even more scripty and harder to understand - for no real benefit. Setting 'global.tag' instead of 'tag' is not more intuitive if most of the installs are not 'global'.

Depends on the context - you either set the tags once globally, or you must set them N times for N charts to the same value, which is cumbersome in spots - see the integration tests, or our docs.

I don't think we need to optimize the user experience based on integration tests.

And the docs seem fine - you install each chart independently and you set hub and tag as expected.

I'm not aware of any 'global' when you install a single chart.

And the rule of thumb: you should not install things together if you're not

going to operate them together (i.e. upgrade at the exact same time, etc). And if you have 2 charts you own that must be operated together - it's a sign you should combine them in one chart.

We made this mistake already - I think the rule was 'new mistakes only' :-) I vote for 'intentional' - composing charts, in particular ztunnel is looking for trouble.

I'm going to support both global and per-chart here, so it should compose like it did before.

chart-level continues to be preferred over any global setting, so if you don't want to use global, just don't set it and you get the same experience as you would if it didn't exist.

if chart level not set but global is set, global will be used.

however you compose charts, you are free to use either to your taste

doing it like this is pretty normative Helm stuff and doesn't put any constraints on composition (or not) of charts.

Can you point to this 'norm' ? If it is something recommended by Helm - and you have a link - I'm all for it. I think it's important to be consistent with the rest of the helm charts - I thought John started the ztunnel chart by generating the default/recommended configuration.

But please don't use helm recomendations for 'multiple charts installed/operated at once' - as we had before Istio 1.0 - because that's the worst way to operate something like Ztunnel and Istiod.

Message ID: @.***>

[bleggett]

I don't think we need to optimize the user experience based on integration tests. And the docs seem fine - you install each chart independently and you set hub and tag as expected. I'm not aware of any 'global' when you install a single chart.

We don't, I agree. We do need to optimize for having fewer top-level helm charts however, and we do need to ensure what charts we do publish are composable.

Not using globals makes composition verbose/redundant/hard, and makes config duplicative.

The goal is to have charts that compose nicely, and have few opinions about their composition. A Helm smell is when you take two arbitrary charts, and try to compose them, and you cannot do so easily, because one of them makes assumptions - this remains a big problem in the Helm ecosystem, and if you've ever tried to build your own umbrella chart for 3-4 OSS project charts, you will hit this.

Goal is not to force people to use umbrella charts, goal is to make sure people can compose our charts, without getting angry choices we've made that make that hard.

Can you point to this 'norm' ? If it is something recommended by Helm - and you have a link - I'm all for it. I think it's important to be consistent with the rest of the helm charts - I thought John started the ztunnel chart by generating the default/recommended configuration.

Oh he did, but the sample is a subset of reality.

Using globals is the recommended way to share values among charts, for values that would always be static across a single composition of charts (e.g. hub/tag).

Example: https://github.com/istio/istio/blob/master/manifests/charts/ambient/values.yaml

1. I want to compose Istio charts in $some-configuration like this (doesn't have to be this composition, could be some custom Argo setup, point is, I don't have an opinion)

2. I want to override hub/tag at runtime

Without globals:

// Overrides for the istiod-remote dep istiod: profile: ambient hub: foo.com/hub tag: 1.2.3 variant: debug

// Overrides for the ztunnel dep ztunnel: profile: ambient hub: foo.com/hub tag: 1.2.3 variant: debug

// Overrides for the cni dep cni: profile: ambient hub: foo.com/hub tag: 1.2.3 variant: debug

With globals:

globals: hub: foo.com/hub tag: 1.2.3 variant: debug

// Overrides for the istiod-remote dep istiod: profile: ambient

// Overrides for the ztunnel dep ztunnel: profile: ambient

// Overrides for the cni dep cni: profile: ambient

that's all.

One thing we might want to do is use globals.istio.<hub/tag/variant> tho.

But please don't use helm recomendations for 'multiple charts installed/operated at once' - as we had before Istio 1.0 - because that's the worst way to operate something like Ztunnel and Istiod.

I am going by "how I would expect these Helm charts to work in the least surprising fashion if this were not Istio, but was just any old boring repo that published more than one Helm chart"

[costinm]

Maybe I'm missing something - how do you use this 'globals' ?

Can you add an example CLI invocation ?

You can create a values.yaml.with the hub and use it on each invocation - but I never seen globals ( unless it's all installed at the same time - but we have istioctl for that and it's the least safe option).

On Wed, Jun 12, 2024, 09:19 Ben Leggett @.***> wrote:

I don't think we need to optimize the user experience based on integration tests. And the docs seem fine - you install each chart independently and you set hub and tag as expected. I'm not aware of any 'global' when you install a single chart.

We don't, I agree. We do need to optimize for having fewer top-level helm charts however, and we do need to ensure what charts we do publish are composable.

Not using globals makes composition verbose/redundant/hard, and makes config duplicative.

The goal is to have charts that compose nicely, and have few opinions about their composition. A Helm smell is when you take two arbitrary charts, and try to compose them, and you cannot do so easily, because one of them makes assumptions - this remains a big problem in the Helm ecosystem, and if you've ever tried to build your own umbrella chart for 3-4 OSS project charts, you will hit this.

Goal is not to force people to use umbrella charts, goal is to make sure people can compose our charts, without getting angry at how we do it.

Can you point to this 'norm' ? If it is something recommended by Helm - and you have a link - I'm all for it. I think it's important to be consistent with the rest of the helm charts - I thought John started the ztunnel chart by generating the default/recommended configuration.

Oh he did, but the sample is a subset of reality.

Using globals is the recommended https://helm.sh/docs/chart_template_guide/subcharts_and_globals/#global-chart-values way to share values among charts, for values that would always be static across a single composition of charts (e.g. hub/tag).

Example: https://github.com/istio/istio/blob/master/manifests/charts/ambient/values.yaml

I want to compose Istio charts in $some-configuration like this (doesn't have to be this composition, could be some custom Argo setup, point is, I don't have an opinion) 2.

I want to override hub/tag at runtime

Without globals:

Overrides for the istiod-remote dep

istiod: profile: ambient hub: foo.com/hub tag: 1.2.3 variant: debug Overrides for the ztunnel dep

ztunnel: profile: ambient hub: foo.com/hub tag: 1.2.3 variant: debug Overrides for the cni dep

cni: profile: ambient hub: foo.com/hub tag: 1.2.3 variant: debug

With globals:

globals: hub: foo.com/hub tag: 1.2.3 variant: debug Overrides for the istiod-remote dep

istiod: profile: ambient Overrides for the ztunnel dep

ztunnel: profile: ambient Overrides for the cni dep

cni: profile: ambient

that's all.

— Reply to this email directly, view it on GitHub https://github.com/istio/istio/pull/51517#issuecomment-2163442755, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUR2WPD47PHQR7ZOYHCVDZHBYIDAVCNFSM6AAAAABJE6VQDSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRTGQ2DENZVGU . You are receiving this because your review was requested.Message ID: @.***>

[bleggett]

Maybe I'm missing something - how do you use this 'globals' ? Can you add an example CLI invocation ? You can create a values.yaml.with the hub and use it on each invocation - but I never seen globals ( unless it's all installed at the same time - but we have istioctl for that and it's the least safe option).

We use it in istiod/istio-cni for sure.

Normally, with a standalone chart with no nesting, you would do something like:

• helm install bottom-chart --set values.tag=mytag

Now, if you wrap bottom-chart with top-chart as a helm dependency, and you still set bottom-chart's tag value, Helm would manage the nested value scopes for you, and you would do:

• helm install top-chart --set values.bottom-chart.tag=mytag

This presents a problem if top-chart also needs that tag value, in that case you have to do:

• helm install top-chart --set values.tag=mytag --set values.bottom-chart.tag=mytag

With globals, you can set the value once, and regardless of scoping values.global.xx always has the scoping of values.global.xx, no matter the chart nesting:

• helm install top-chart --set values.global.tag=mytag # top-chart and bottom-chart both reference 'values.global.tag'

There's some other istio-specific wrinkles that make this harder, because we "scope" our chart values by hand and do some weird munging in istioctl, but that's the gist.

With the defaulting we already do (that I'm trying to make consistent), either form will work:

• helm install top-chart --set values.tag=mytag --set values.bottom-chart.tag=mytag

• helm install top-chart --set values.global.tag=mytag # top-chart and bottom-chart both reference 'values.global.tag'

and if you happened to goof up and specify both at the same time:

• helm install top-chart --set values.global.tag=mytag --set values.bottom-chart.tag=mytag

the more specific bottom-tag would win.

[bleggett]

release-builder will need: https://github.com/istio/release-builder/pull/1871

[howardjohn]

That is dumb. It should merge for you. Lame.

Can we just do the merge our selves? We already do effectively with profiles and defaults etc?

[bleggett]

That is dumb. It should merge for you. Lame.

Can we just do the merge our selves? We already do effectively with profiles and defaults etc?

That's effectively what the template does yea: https://github.com/istio/istio/pull/51517/files#diff-a56810d64b0dbae2e956dc4955c3395f1b1d23307d9c63226500ae6db7994ae6R5

That's a merge in the sense of "behave logically no matter which you set, or even if you set both" - not a merge in the sense of "we munge Helm values in unique ways" - former is generally better behaved and less surprising anyway.

[howardjohn]

I mean so no part of the chart has to change at all, we don't have to special case a few flags, and we don't need global to show up in values.yaml at all

[bleggett]

I mean so no part of the chart has to change at all, we don't have to special case a few flags, and we don't need global to show up in values.yaml at all

Our options are

• to remove all usage of globals from all our charts, because we don't like them, which is a local, non-Helm convention Istio can enforce that would complicate chart composition a bit, and which we'd have to teach people about/sustain.
• do what I've done here
• break Helm semantics with nonstandard flattening (what we had been doing)

I'm personally ok with either of the first two, but the first option is a larger discussion around deprecating existing usage of globals for istiod/istio-cni (we could deprecate and patch for a couple of releases with this template)

[howardjohn]

Other very very popular charts like https://artifacthub.io/packages/helm/prometheus-community/prometheus do not have global anywhere in them, so I am not super convinced this is our only option.. (they do have a global but its nested under something and not related, just confusing naming).

Why do ALL charts need to explicitly to have globals?

Just to clarify, what I am wonder is if we can do something like:

set $ "Values" (mustMergeOverwrite $.Values (pick $.Values.Global "hub" "tag"))

The pick is option if you want to merge everything instead of just a subset

[bleggett]

Other very very popular charts like https://artifacthub.io/packages/helm/prometheus-community/prometheus do not have global anywhere in them, so I am not super convinced this is our only option.. (they do have a global but its nested under something and not related, just confusing naming).

Yeah - I am not saying we have to use globals - just that we already do, and if we continue to use them we have to use them normatively - and that they solve some issues around chart composition.

Why do ALL charts need to explicitly to have globals?

Just to clarify, what I am wonder is if we can do something like:

set $ "Values" (mustMergeOverwrite $.Values (pick $.Values.Global "hub" "tag"))

The pick is option if you want to merge everything instead of just a subset

This might work if we don't want to surface globals in values.yaml - that runs into the problem of it being rather opaque as to which chart respects which globals.

I'd rather use them very obviously, with no munging of them outside of Helm, or not at all.

[howardjohn]

btw was looking around and found https://mikemybytes.com/2020/11/25/isolation-issues-with-helm-umbrella-charts/. SEems like we do have some duplicated template definitions - at least core, I think that is the only one

[bleggett]

btw was looking around and found https://mikemybytes.com/2020/11/25/isolation-issues-with-helm-umbrella-charts/. SEems like we do have some duplicated template definitions - at least core, I think that is the only one

Yep, lemme do a pass for this. We should do it anyway, we can't control how people compose our charts and really shouldn't try.

Part of why I want us to compose our own charts in a few spots is it's a good hygiene check and helps proactively dig up stuff like this.

Some of the templates should/could be global and live exclusively in parent charts, but can't be because our charts aren't fully consistent with each other WRT values scoping (not related to global, just generally - e.g. the values.cni/values.pilot stuff makes this way messier)

[costinm]

On Wed, Jun 12, 2024 at 2:48 PM Ben Leggett @.***> wrote:

The helm tests were not actually using

Maybe I'm missing something - how do you use this 'globals' ? Can you add an example CLI invocation ? You can create a values.yaml.with the hub and use it on each invocation - but I never seen globals ( unless it's all installed at the same time - but we have istioctl for that and it's the least safe option).

We use it in istiod/istio-cni for sure.

Normally, with a standalone chart with no nesting, you would do something like:

helm install bottom-chart --set values.tag=mytag

Now, if you wrap bottom-chart with top-chart as a helm dependency https://helm.sh/docs/helm/helm_dependency/, and you still set bottom-chart's tag value, Helm would manage the nested value scopes for you, and you would do:

helm install top-chart --set values.bottom-chart.tag=mytag

This presents a problem if top-chart also needs that tag value, in that case you have to do:

helm install top-chart --set values.tag=mytag --set values.bottom-chart.tag=mytag

With globals, you can set the value once, and regardless of scoping values.global.xx always has the scoping of values.global.xx, no matter the chart nesting:

helm install top-chart --set values.global.tag=mytag # top-chart and bottom-chart both reference 'values.global.tag'

There's some other istio-specific wrinkles that make this ahrder, because we "scope" our chart values by hand and do some weird munging in istioctl, but that's the gist.

I was afraid of this - basically what we did in early Istio, and we spent a year moving away from.

I am 100%, strongly against any such 'helm dependency' usage - if 2 charts must be installed at the same time, they should be merged, like we did with citadel, pilot, etc. Otherwise - they must be able to be operated independently. That's why we separate the components in different charts in the fist place, so they don't have to be operated in sync.

We do have istioctl for people who want 'easy install everything, I don't care about second day', so there is no gap. And if people want to do crazy things - they can fork the charts.

That's the main reason I was against 'global' - I was worried that someone will go back to the old way to do 'easy install'.

Right now the only 2 components that may need to be installed together are CNI and Ztunnel - and only because people claim they must be managed independently and it's worth the complexity. Using helm dependency is the same - just with a lot of pain and complexity.

Message ID: @.***>

[costinm]

I think the decision to move away from helm dependencies was made and approved many years ago by the TOC - if you believe we should go back, please write a doc - with all the benefits and issues - and have the TOC approve it, probably debating in this thread is not going to be productive.

On Wed, Jun 12, 2024 at 5:59 PM Costin Manolache @.***> wrote:

On Wed, Jun 12, 2024 at 2:48 PM Ben Leggett @.***> wrote:

The helm tests were not actually using

Maybe I'm missing something - how do you use this 'globals' ? Can you add an example CLI invocation ? You can create a values.yaml.with the hub and use it on each invocation - but I never seen globals ( unless it's all installed at the same time - but we have istioctl for that and it's the least safe option).

We use it in istiod/istio-cni for sure.

Normally, with a standalone chart with no nesting, you would do something like:

helm install bottom-chart --set values.tag=mytag

Now, if you wrap bottom-chart with top-chart as a helm dependency https://helm.sh/docs/helm/helm_dependency/, and you still set bottom-chart's tag value, Helm would manage the nested value scopes for you, and you would do:

helm install top-chart --set values.bottom-chart.tag=mytag

This presents a problem if top-chart also needs that tag value, in that case you have to do:

helm install top-chart --set values.tag=mytag --set values.bottom-chart.tag=mytag

With globals, you can set the value once, and regardless of scoping values.global.xx always has the scoping of values.global.xx, no matter the chart nesting:

helm install top-chart --set values.global.tag=mytag # top-chart and bottom-chart both reference 'values.global.tag'

There's some other istio-specific wrinkles that make this ahrder, because we "scope" our chart values by hand and do some weird munging in istioctl, but that's the gist.

I was afraid of this - basically what we did in early Istio, and we spent a year moving away from.

I am 100%, strongly against any such 'helm dependency' usage - if 2 charts must be installed at the same time, they should be merged, like we did with citadel, pilot, etc. Otherwise - they must be able to be operated independently. That's why we separate the components in different charts in the fist place, so they don't have to be operated in sync.

We do have istioctl for people who want 'easy install everything, I don't care about second day', so there is no gap. And if people want to do crazy things - they can fork the charts.

That's the main reason I was against 'global' - I was worried that someone will go back to the old way to do 'easy install'.

Right now the only 2 components that may need to be installed together are CNI and Ztunnel - and only because people claim they must be managed independently and it's worth the complexity. Using helm dependency is the same - just with a lot of pain and complexity.

Message ID: @.***>

[bleggett]

I think the decision to move away from helm dependencies was made and approved many years ago by the TOC - if you believe we should go back, please write a doc - with all the benefits and issues - and have the TOC approve it, probably debating in this thread is not going to be productive.

On Wed, Jun 12, 2024 at 5:59 PM Costin Manolache @.***> wrote:

On Wed, Jun 12, 2024 at 2:48 PM Ben Leggett @.***> wrote:

The helm tests were not actually using

Maybe I'm missing something - how do you use this 'globals' ? Can you add an example CLI invocation ? You can create a values.yaml.with the hub and use it on each invocation - but I never seen globals ( unless it's all installed at the same time - but we have istioctl for that and it's the least safe option).

We use it in istiod/istio-cni for sure.

Normally, with a standalone chart with no nesting, you would do something like:

helm install bottom-chart --set values.tag=mytag

Now, if you wrap bottom-chart with top-chart as a helm dependency https://helm.sh/docs/helm/helm_dependency/, and you still set bottom-chart's tag value, Helm would manage the nested value scopes for you, and you would do:

helm install top-chart --set values.bottom-chart.tag=mytag

This presents a problem if top-chart also needs that tag value, in that case you have to do:

helm install top-chart --set values.tag=mytag --set values.bottom-chart.tag=mytag

With globals, you can set the value once, and regardless of scoping values.global.xx always has the scoping of values.global.xx, no matter the chart nesting:

helm install top-chart --set values.global.tag=mytag # top-chart and bottom-chart both reference 'values.global.tag'

There's some other istio-specific wrinkles that make this ahrder, because we "scope" our chart values by hand and do some weird munging in istioctl, but that's the gist.

I was afraid of this - basically what we did in early Istio, and we spent a year moving away from.

I am 100%, strongly against any such 'helm dependency' usage - if 2 charts must be installed at the same time, they should be merged, like we did with citadel, pilot, etc. Otherwise - they must be able to be operated independently. That's why we separate the components in different charts in the fist place, so they don't have to be operated in sync.

We do have istioctl for people who want 'easy install everything, I don't care about second day', so there is no gap. And if people want to do crazy things - they can fork the charts.

That's the main reason I was against 'global' - I was worried that someone will go back to the old way to do 'easy install'.

Right now the only 2 components that may need to be installed together are CNI and Ztunnel - and only because people claim they must be managed independently and it's worth the complexity. Using helm dependency is the same - just with a lot of pain and complexity.

Message ID: @.***>

I agree and I don't want to move charts to be dependencies - I want the charts to support being used as dependencies, if people want to take our published charts and use them like that.

I am doing a very simple wrapper chart for ambient starter docs, based on direct user feedback - this won't be recommended for general use, and adding the wrapper chart doesn't remove any ability to continue using the individual charts as-is.

I don't want to remove any functionality, I simply want our charts to be hygienic enough to be effectively composed, without Istio-specific gotchas.

[howardjohn]

you can use the chart as a child chart today - you don't need to use global. And it really sucks that helm doesn't provide a way to actually properly have dependencies where a child chart needs to consider anything a parent might possibly need (which will never be enough).

If the target is for simple demo why do we need a hub override in the first place?

On Wed, Jun 12, 2024, 6:15 PM Ben Leggett @.***> wrote:

I think the decision to move away from helm dependencies was made and approved many years ago by the TOC - if you believe we should go back, please write a doc - with all the benefits and issues - and have the TOC approve it, probably debating in this thread is not going to be productive.

On Wed, Jun 12, 2024 at 5:59 PM Costin Manolache @.***> wrote:

On Wed, Jun 12, 2024 at 2:48 PM Ben Leggett @.***> wrote:

The helm tests were not actually using

Maybe I'm missing something - how do you use this 'globals' ? Can you add an example CLI invocation ? You can create a values.yaml.with the hub and use it on each invocation - but I never seen globals ( unless it's all installed at the same time - but we have istioctl for that and it's the least safe option).

We use it in istiod/istio-cni for sure.

Normally, with a standalone chart with no nesting, you would do something like:

helm install bottom-chart --set values.tag=mytag

Now, if you wrap bottom-chart with top-chart as a helm dependency https://helm.sh/docs/helm/helm_dependency/, and you still set bottom-chart's tag value, Helm would manage the nested value scopes for you, and you would do:

helm install top-chart --set values.bottom-chart.tag=mytag

This presents a problem if top-chart also needs that tag value, in that case you have to do:

helm install top-chart --set values.tag=mytag --set values.bottom-chart.tag=mytag

With globals, you can set the value once, and regardless of scoping values.global.xx always has the scoping of values.global.xx, no matter the chart nesting:

helm install top-chart --set values.global.tag=mytag # top-chart and bottom-chart both reference 'values.global.tag'

There's some other istio-specific wrinkles that make this ahrder, because we "scope" our chart values by hand and do some weird munging in istioctl, but that's the gist.

I was afraid of this - basically what we did in early Istio, and we spent a year moving away from.

I am 100%, strongly against any such 'helm dependency' usage - if 2 charts must be installed at the same time, they should be merged, like we did with citadel, pilot, etc. Otherwise - they must be able to be operated independently. That's why we separate the components in different charts in the fist place, so they don't have to be operated in sync.

We do have istioctl for people who want 'easy install everything, I don't care about second day', so there is no gap. And if people want to do crazy things - they can fork the charts.

That's the main reason I was against 'global' - I was worried that someone will go back to the old way to do 'easy install'.

Right now the only 2 components that may need to be installed together are CNI and Ztunnel - and only because people claim they must be managed independently and it's worth the complexity. Using helm dependency is the same - just with a lot of pain and complexity.

Message ID: @.***>

I agree and I don't want to move charts to be dependencies - I want the charts to support being used as dependencies, if people want to take our published charts and use them like that.

I am doing a very simple wrapper chart for ambient starter docs, based on direct user feedback - this won't be recommended for general use, and adding the wrapper chart doesn't remove any ability to continue using the individual charts as-is.

— Reply to this email directly, view it on GitHub https://github.com/istio/istio/pull/51517#issuecomment-2164171837, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEYGXODXBHG5PLDNKXVEGTZHDXEJAVCNFSM6AAAAABJE6VQDSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRUGE3TCOBTG4 . You are receiving this because your review was requested.Message ID: @.***>

[bleggett]

you can use the chart as a child chart today - you don't need to use global. And it really sucks that helm doesn't provide a way to actually properly have dependencies where a child chart needs to consider anything a parent might possibly need (which will never be enough).

If the target is for simple demo why do we need a hub override in the first place?

On Wed, Jun 12, 2024, 6:15 PM Ben Leggett @.***> wrote:

I think the decision to move away from helm dependencies was made and approved many years ago by the TOC - if you believe we should go back, please write a doc - with all the benefits and issues - and have the TOC approve it, probably debating in this thread is not going to be productive.

On Wed, Jun 12, 2024 at 5:59 PM Costin Manolache @.***> wrote:

On Wed, Jun 12, 2024 at 2:48 PM Ben Leggett @.***> wrote:

The helm tests were not actually using

Maybe I'm missing something - how do you use this 'globals' ? Can you add an example CLI invocation ? You can create a values.yaml.with the hub and use it on each invocation - but I never seen globals ( unless it's all installed at the same time - but we have istioctl for that and it's the least safe option).

We use it in istiod/istio-cni for sure.

Normally, with a standalone chart with no nesting, you would do something like:

helm install bottom-chart --set values.tag=mytag

Now, if you wrap bottom-chart with top-chart as a helm dependency https://helm.sh/docs/helm/helm_dependency/, and you still set bottom-chart's tag value, Helm would manage the nested value scopes for you, and you would do:

helm install top-chart --set values.bottom-chart.tag=mytag

This presents a problem if top-chart also needs that tag value, in that case you have to do:

helm install top-chart --set values.tag=mytag --set values.bottom-chart.tag=mytag

With globals, you can set the value once, and regardless of scoping values.global.xx always has the scoping of values.global.xx, no matter the chart nesting:

helm install top-chart --set values.global.tag=mytag # top-chart and bottom-chart both reference 'values.global.tag'

There's some other istio-specific wrinkles that make this ahrder, because we "scope" our chart values by hand and do some weird munging in istioctl, but that's the gist.

I was afraid of this - basically what we did in early Istio, and we spent a year moving away from.

I am 100%, strongly against any such 'helm dependency' usage - if 2 charts must be installed at the same time, they should be merged, like we did with citadel, pilot, etc. Otherwise - they must be able to be operated independently. That's why we separate the components in different charts in the fist place, so they don't have to be operated in sync.

We do have istioctl for people who want 'easy install everything, I don't care about second day', so there is no gap. And if people want to do crazy things - they can fork the charts.

That's the main reason I was against 'global' - I was worried that someone will go back to the old way to do 'easy install'.

Right now the only 2 components that may need to be installed together are CNI and Ztunnel - and only because people claim they must be managed independently and it's worth the complexity. Using helm dependency is the same - just with a lot of pain and complexity.

Message ID: @.***>

I agree and I don't want to move charts to be dependencies - I want the charts to support being used as dependencies, if people want to take our published charts and use them like that.

I am doing a very simple wrapper chart for ambient starter docs, based on direct user feedback - this won't be recommended for general use, and adding the wrapper chart doesn't remove any ability to continue using the individual charts as-is.

— Reply to this email directly, view it on GitHub https://github.com/istio/istio/pull/51517#issuecomment-2164171837, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEYGXODXBHG5PLDNKXVEGTZHDXEJAVCNFSM6AAAAABJE6VQDSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRUGE3TCOBTG4 . You are receiving this because your review was requested.Message ID: @.***>

We don't need to use global, but we already do - albeit inconsistently, which was the source of the bugs I'm fixing here.

I'd argue it makes several things easier, and has no legitimate cost - it's the same as manually propagating a value between charts with set, just more concise and formalized.

There's no way you can break your charts with global that wouldn't also break them without it.

If we would like to remove global from all our charts, that would be a TOC decision, and I'd certainly be open to doing that - but that's a larger and more invasive fix than this PR, which is just about getting hub and tag overrides to apply consistently across our currently-shipped ambient charts, without behavior-breaking overrides in istioctl