fix(helm): Respect global registry in sidecar image (#18246)

[fbuetler]

What this PR does / why we need it:
The image in the sidecar does not respect the global registry, if set. This PR ensures this is respected.

Which issue(s) this PR fixes:
Fixes #18246

Special notes for your reviewer:
The PR is constructed in a way to be backwards compatible for the case the image is configured with a private registry i.e. image: private-registry.com/kiwigrid/k8s-sidecar
To detect this configuration, in the helm template, the assumption is made that a registry has a dot (.) in its fully qualified domain name (FQDN).

Also, slightly related to the recent change in e0c07c4

Possible configuration combinations and its result:

# default registry
➜ helm template loki . | rg "kiwigrid/k8s-sidecar"
          image: "docker.io/kiwigrid/k8s-sidecar:1.30.10"

# global registry
➜ helm template loki . --set global.registry='private.registry' | rg "kiwigrid/k8s-sidecar"
          image: "private.registry/kiwigrid/k8s-sidecar:1.30.10"

#  custom registry
➜ helm template loki . --set sidecar.image.registry='private.registry' | rg "kiwigrid/k8s-sidecar"
          image: "private.registry/kiwigrid/k8s-sidecar:1.30.10"
          
# backwards compatibility: private registry in repository
➜  helm template loki . --set sidecar.image.repository='private.registry/kiwigrid/k8s-sidecar' | rg "kiwigrid/k8s-sidecar"
          image: "private.registry/kiwigrid/k8s-sidecar:1.30.10"

(for this to make it work on my branch I had to also use --set loki.useTestSchema=true --set minio.enabled=true but this is also the case on the main branch so I guess something else is broken there)

Checklist

• Reviewed the CONTRIBUTING.md guide (required)
• Title matches the required conventional commits format, see here
• Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md

[CLAassistant]

All committers have signed the CLA.

[jkroepke]

Thanks for you PR, see comments.

[jkroepke]

@JStickler could you please take a look here? I guess the final version of the helm chart must be adjusted.

[jkroepke]

@fbuetler you can confirm this is not a breaking change, because this PR contains a backward compatible logic?

[fbuetler]

Yes, the idea is to be backwards-compatible (I tried to illustrate that with the code snippet above), and I picked the next patch version for the upgrade documentation. This might have to be adjusted according to the main branch.

[jkroepke]

And please add a note to the charts CHANGELOG.md

[jkroepke]

Potential superseded by #19246

[fbuetler]

I adjusted the PR according to your comments, please have another look.

I rerun the commands and they yield the same results:

➜ helm template loki . | rg "kiwigrid/k8s-sidecar"
          image: "docker.io/kiwigrid/k8s-sidecar:1.30.10"

➜ helm template loki . --set global.registry='private.registry' | rg "kiwigrid/k8s-sidecar"
          image: "private.registry/kiwigrid/k8s-sidecar:1.30.10"

➜ helm template loki . --set sidecar.image.registry='private.registry' | rg "kiwigrid/k8s-sidecar"
          image: "private.registry/kiwigrid/k8s-sidecar:1.30.10"

➜ helm template loki . --set sidecar.image.repository='private.registry/kiwigrid/k8s-sidecar' | rg "kiwigrid/k8s-sidecar"
          image: "private.registry/kiwigrid/k8s-sidecar:1.30.10"

And with the sha:

➜ helm template loki . --set sidecar.image.sha='abcdef' | rg "kiwigrid/k8s-sidecar"
          image: "docker.io/kiwigrid/k8s-sidecar:1.30.10@sha256:abcdef"

➜ helm template loki . --set global.registry='private.registry' --set sidecar.image.sha='abcdef' | rg "kiwigrid/k8s-sidecar"
          image: "private.registry/kiwigrid/k8s-sidecar:1.30.10@sha256:abcdef"

➜ helm template loki . --set sidecar.image.registry='private.registry' --set sidecar.image.sha='abcdef' | rg "kiwigrid/k8s-sidecar"
          image: "private.registry/kiwigrid/k8s-sidecar:1.30.10@sha256:abcdef"

➜ helm template loki . --set sidecar.image.repository='private.registry/kiwigrid/k8s-sidecar' --set sidecar.image.sha='abcdef' | rg "kiwigrid/k8s-sidecar"
          image: "private.registry/kiwigrid/k8s-sidecar:1.30.10@sha256:abcdef"

[jkroepke]

LGTM

[jkroepke]

I guess #19246 replaces this as well. Not sure.

[fbuetler]

I guess #19246 replaces this as well. Not sure.

@jkroepke I think the main difference for this PR to #19246, is that this PR is backwards-compatible and #19246 is not.
#19246 solves the issue by introducing a new global.imageRegistry and deprecates image.registry instead.
While this may also resolve the issue, it requires manual intervention by users that have already configured a private registry for the sidecar image.

[jkroepke]

Deprecating old stuff is the right way. Maintain multiple options for the same is a pain and errorful.

[jkroepke]

@fbuetler do you want to update this?

[fbuetler]

@fbuetler do you want to update this?

@jkroepke are you proposing to introduce a new global.imageRegistry and deprecate image.registry?

[jkroepke]

@fbuetler could you check, if you could re-use the loki.baseImage helper, introduced in #19246

[fbuetler]

I think it is not possible to reuse loki.baseImage with backwards-compatibility in mind for two purposes:

• In sidecard.image the keyword sha is used whereas the loki.baseImage only respects digest and
• in the sidecard.image.repository the registry might already be included, hence we need to handle that case

I think it is the right approach to align the different options with each other, i.e. introducing registry in sidecard.image and also digest (and deprecating sha) in sidecar.image.

My proposal is to introduce loki.sidecarImage with the same interface (dict with service and global) as loki.baseImage that is backwards compatible.
On the next major release, this can then be simply swapped out loki.sidecarImage with loki.baseImage.

In the same process, I also introduced .digest in the sidecar image and deprecated .tag. This further aligns the loki.sidecarImage with loki.baseImage.

The obligatory check:

# current
helm template loki . --set loki.useTestSchema=true | rg "kiwigrid/k8s-sidecar"
          image: docker.io/kiwigrid/k8s-sidecar:1.30.10
# supported ways for a custom registry
helm template loki . --set loki.useTestSchema=true --set global.registry='private.registry' | rg "kiwigrid/k8s-sidecar"
          image: private.registry/kiwigrid/k8s-sidecar:1.30.10
helm template loki . --set loki.useTestSchema=true --set global.imageRegistry='private.registry' | rg "kiwigrid/k8s-sidecar"
          image: private.registry/kiwigrid/k8s-sidecar:1.30.10
helm template loki . --set loki.useTestSchema=true --set sidecar.image.registry='private.registry' | rg "kiwigrid/k8s-sidecar"
          image: private.registry/kiwigrid/k8s-sidecar:1.30.10
# backwards compatible
helm template loki . --set loki.useTestSchema=true --set sidecar.image.repository='private.registry/kiwigrid/k8s-sidecar' | rg "kiwigrid/k8s-sidecar"
          image: private.registry/kiwigrid/k8s-sidecar:1.30.10
# sha
helm template loki . --set loki.useTestSchema=true --set sidecar.image.sha='abcdef' | rg "kiwigrid/k8s-sidecar"
          image: docker.io/kiwigrid/k8s-sidecar@sha256:abcdef
# digest
helm template loki . --set loki.useTestSchema=true --set sidecar.image.digest='sha256:fedcba' | rg "kiwigrid/k8s-sidecar"
          image: docker.io/kiwigrid/k8s-sidecar@sha256:fedcba
# digest should be preferred over sha
helm template loki . --set loki.useTestSchema=true --set sidecar.image.sha='abcdef' --set sidecar.image.digest='sha256:fedcba' | rg "kiwigrid/k8s-sidecar"
          image: docker.io/kiwigrid/k8s-sidecar@sha256:fedcba

[jkroepke]

What did you think about replace the current loki.baseImage with your proposal? It would fix
Fixes #19662 as well then.

[fbuetler]

Yes, that should fix #19662.

Note: for this to work, I had to fix a error in the dict for the baseImage by removing .image from .Values.global.image as the new global.imageRegsitry introduced in #19246 would not have been passed into baseImage.
The fix goes in the same direction as #19715, but #19715 only fixes it for the loki image.

[fbuetler]

Thank you for pointing this out, I was not aware of this required step.

Should I also rebase the branch on main and remove your merge commit @Jayclifford345 ?

[fbuetler]

IMO the remaining failing test case is not a result of this PR. It passed in the last run of the checks, therefore I call it a flaky test.

[Jayclifford345]

All good @fbuetler unfortunately, we have flakey as the team are building Loki 4.0 we just need to rerun and normall they clear up

[fbuetler]

@Jayclifford345 @jkroepke Can we merge this?

[JStickler]

Updated the CHANGELOG, let's see if this passes tests...

[fbuetler]

LGTM

[Jayclifford345]

Sorry team, completlty missed this just fixed the merge issues and running tests again so we can get this merged

[hansingt]

I'm not sure, but I think, this is a typo, is it? Current version of Helm Chart is 6.49.0, so next version would be 6.49.1, not 6.46.1, right?

[JStickler]

Good catch, this PR was open for a long time and the chart version in this file wasn't updated. I'll fix this in the docs. Thanks for flagging it!

[JStickler]

PR with the update