consul-k8s icon indicating copy to clipboard operation
consul-k8s copied to clipboard

Allow image tags/name/repository to be configured separately

Open cucxabong opened this issue 3 years ago • 9 comments

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request. Searching for pre-existing feature requests helps us consolidate datapoints for identical requirements into a single place, thank you!
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment.

We have an in-house tool that will fetch latest tags for an image in a (local docker registry) & then fill values (using --set) file on deployment, so this tool can not work when current chart requires input image as a single string (with registry/name/tag in a single line)

Contributions

I'm happy to work on this

cucxabong avatar Jun 28 '21 15:06 cucxabong

Hey @cucxabong !! Thanks for filing an issue. I do understand the appeal of having the tags, name and repository being separate fields that are individually configurable, it would make it a little more complicated to parse the helm config for most of our users. Would it be possible for the in-house tool's output to be configured in the repository/name:tag format so that it can be provided to the --set flag? Because unfortunately, if we don't see more community support for this (with 👍 ), we might not want to make this update as I fear it will make the values file more complicated to understand as most users are familiar with images in the repo/name:tag format and might find them being in separate field non-intuitive.

thisisnotashwin avatar Jul 01 '21 18:07 thisisnotashwin

I second this... We have "air-gapped" environments, where we cannot reach "docker" proper, if we could just set something like global.registry to internal-mirror.domain.com, and not mess with the image name or tag, that would make things easier. Now, whenever we upgrade the helm chart, we have to go back in and verify that we have the correct version of consul-k8s and consul ...

Additionally, I think it would be nice if there was a way of just setting global.enterprise: true instead of overriding the image name manually... or even just automatically select the enterprise image if they provide a license?

TJM avatar Jul 01 '21 18:07 TJM

@TJM thanks for the feedback. This definitely makes sense in an airgapped env where all one wants to update is the repository but retain the name of the image and tag. Given docker works without a repo makes sense. A tentative solution would be to have a global flag for repo which defaults to index.docker.io which would work with docker but could be swapped for internal-mirror.domain.com or any other repo that the images have been migrated to.

re license: we have separate binaries for consul and consul-enterprise and hence separate images as well. hence the differing image name. i do agree that ideally, if the image was the same, the presence of a license should just lead to enterprise features being toggled on but that is not the case here unfortunately.

thisisnotashwin avatar Jul 01 '21 19:07 thisisnotashwin

I don't think we can support this without breaking backwards compatibility. Say we do as @thisisnotashwin suggests and have:

# values.yaml
global:
  imageRepo: docker.io
  imageK8S: hashicorp/consul-k8s:0.36.0

And then in our templates we use:

image: {{ .Values.global.imageRepo }}/{{ .Values.imageK8S}}

This would break anyone that was previously setting the full image, e.g.:

global:
  imageK8S: my.internal.repo/hashicorp/consul-k8s:0.36.0

Since it would now render as:

image: docker.io/my.internal.repo/hashicorp/consul-k8s:0.36.0

Given that this is a nice-to-have and that there should be a way to work around it in tooling and that I don't see a way to implement this without breaking backwards compatibility I'm not sure it makes sense to implement this.

lkysow avatar Nov 04 '21 21:11 lkysow

Without breaking backwards compatibility... If the parameter to set the "full image name" (global.imageK8S) is set, use it, otherwise construct an image name using the component parts. Just don't use the same parameter for the full image name as the component parts. The "image" that is passed to the kubernetes resource can all be magic'd out in a helpers template.

NOTE: you can even deprecate the old "full image name" parameter, such that some time in the future, you could remove it in a major release. It really isn't too difficult to use though. Take a look at the "fullNameOverride" in _helpers.tpl which defines the name of various parts of a typical helm template (helm create example)

TJM avatar Nov 04 '21 22:11 TJM

This was discussed recently whether it would be viable to make this change. Currently would introduce a lot of changes for the integration testing side of Consul-k8s so we've delayed working on this as it is a breaking change.

david-yu avatar Jul 05 '23 18:07 david-yu

hey @david-yu I wanted to bump this one. Looking at the chart I can see how this would be a challenging refactor, but I do want to highlight how worthwhile my organization would find it.

In our case, we're angling to get full off of Dockerhub and onto the Public ECR Gallery for images so we don't leave the AWS network to pull images.

In the case of the vault chart, it was fairly simple to make this migration. We replaced the repository on each component and we were good to go.

https://github.com/hashicorp/vault-helm/blob/main/values.yaml#L68-L71

  image:
    repository: "hashicorp/vault-k8s"
    tag: "1.2.1"

With consul however, the part that somewhat holds me back from taking the leap is that I would then end up having to take up tracing both the versions of the chart and the images. While that is doable and if I want off of Dockerhub bad enough, I'll probably do it. But it puts me in a position of not just worrying about my chart version and what the associated default version of Consul is for that chart, but matching those up and keeping them up-to-date.

From my end, it would definitely be preferable if I was able to update the repository without having to take up the additional task of keeping an eye on the version of the image as a side-effect.

My thought on a possible contract that might work would be to do something like Vault does, but test for if image is set to a string or a map in a template helper and if it's a string, take that value, otherwise compose it from the map. That might make it less breaking, though not less work from an integration testing side.

Thanks for the consideration!

ebachle avatar Oct 02 '23 15:10 ebachle