Fix double revision in istio configmap name

[nrfox]

Uses: conf.ExternalServices.Istio.ConfigMapName without adding the rev tag when it is set and not the default istio.

Fixes #6669

[nrfox]

@jmazzitelli please take a look at this when you get a chance

[jmazzitelli]

This code change makes sense. We need to make the call to this new IstioConfigMap name in all the places where we need the name.

These places: https://github.com/search?q=repo%3Akiali%2Fkiali+%2FGetConfigMap.*ExternalServices.Istio.ConfigMapName%2F&type=code

Here's the files that probably need to be changed in this PR:

$ grep -R 'GetConfig.*ExternalServices.Istio.ConfigMapName'
business/istio_certs.go:	istioConfigMap, err := ics.k8s.GetConfigMap(cfg.IstioNamespace, cfg.ExternalServices.Istio.ConfigMapName)
business/istio_certs.go:	istioConfigMap, err := ics.k8s.GetConfigMap(cfg.IstioNamespace, cfg.ExternalServices.Istio.ConfigMapName)
business/namespaces.go:	if icm, err := in.kialiCache.GetConfigMap(in.conf.IstioNamespace, in.conf.ExternalServices.Istio.ConfigMapName); err == nil {
business/tls.go:	istioConfig, err := kubeCache.GetConfigMap(cfg.IstioNamespace, cfg.ExternalServices.Istio.ConfigMapName)
business/istio_validations.go:		istioConfig, err = kialiCache.GetConfigMap(cfg.IstioNamespace, cfg.ExternalServices.Istio.ConfigMapName)
business/istio_validations.go:		istioConfig, err = userClient.GetConfigMap(cfg.IstioNamespace, cfg.ExternalServices.Istio.ConfigMapName)
business/mesh.go:	istioConfig, err := in.kialiCache.GetConfigMap(in.conf.IstioNamespace, in.conf.ExternalServices.Istio.ConfigMapName)

Note that this PR directly addresses this issue too - so we can close this issue when this PR is merged.

[yohanb]

@nrfox thanks for this PR! We just ran into this issue and traced it back to the same faulty code.
When can we expect this to be complete? Only missing a few refactors ;)

[nrfox]

@nrfox thanks for this PR! We just ran into this issue and traced it back to the same faulty code.
When can we expect this to be complete? Only missing a few refactors ;)

@yohanb hoping to have this complete by the end of the week so it can be included with the next release.

[yohanb]

@nrfox thanks for this PR! We just ran into this issue and traced it back to the same faulty code.
When can we expect this to be complete? Only missing a few refactors ;)

@yohanb hoping to have this complete by the end of the week so it can be included with the next release.

Awesome, thanks so much 👍

[nrfox]

@jmazzitelli it seemed like the e2e tests were failing because of the tracing flakes. I rebased with main to pickup the latest fix. I also added some additional test coverage to the area that was causing this error to be logged which was in gateway validations.

[jmazzitelli]

the thing I don't see explained anywhere here is why does this pass in the Revision but everywhere else it passes in empty string for revision. How does this work when sometimes you need the revision but other times that is not required to get the configmap?

[nrfox]

Because basically this is the only place that is handling there being multiple controlplanes. Everywhere else just assumes there's one. When set config.external_services.istio.config_map_name, IstioConfigMapName will always return what you've set there regardless of what revision you pass in. It's a bit of a hack to get around the fact that, right now we expect users to set this manually when using control plane revisions.

[jmazzitelli]

this does the refactor so everywhere in the code gets the configmap name using the common function. I have a feeling our support for canary upgrades still isn't seamless but we can worry about that later.

[nrfox]

I have a feeling our support for canary upgrades still isn't seamless but we can worry about that later.

Yes this change doesn't make the support for canary upgrades any better but it at least gets rid of the error log when you've set config.external_services.istio.config_map_name.

[amazingandyyy]

good to see this get merged, what's the ETA for new helm chart release? thanks for the hard work!

[yohanb]

probably removing the logic to append the revision all together would be a more cohesive solution. I think we can simply expect users to provide the correct values through external_services.istio

[jmazzitelli]

probably removing the logic to append the revision all together would be a more cohesive solution. I think we can simply expect users to provide the correct values through external_services.istio

I'm OK doing either suggestion. if Kiali users like @yohanb think it would be simpler and expected of users to set the correct value in the Kiali config, then let's do that.

cc @nrfox

[nrfox]

I'm OK doing either suggestion. if Kiali users like @yohanb think it would be simpler and expected of users to set the correct value in the Kiali config, then let's do that.

That sounds good to me. The only place that is currently using the offending code from GetMesh does not use the config map anyways and I'd rather be sure to not break existing functionality or cause noise for users. I appreciate the feedback here and it's very useful knowing that this is config that users rely on.

[jmazzitelli]

we need to backport this to the 1.73 branch.