Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Handle image names with an image digest instead of a tag #885

Closed
mithrandi opened this issue Dec 22, 2017 · 10 comments
Closed

Handle image names with an image digest instead of a tag #885

mithrandi opened this issue Dec 22, 2017 · 10 comments

Comments

@mithrandi
Copy link
Contributor

mithrandi commented Dec 22, 2017

An image name like this: gcr.io/decisive-cinema-167507/kube-cert-manager@sha256:4e79e70added5a2e95fb10bf9a63e4234395a3e047c9bf6a6c1131c383c9f04a

Will cause an error like this:

ts=2017-12-22T15:43:30.33587349Z caller=warming.go:154 component=warmer err="requesting tags: mux: variable \"decisive-cinema-167507/kube-cert-manager@sha256\" doesn't match, expected \"^(?:(?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])(?:(?:\\\\.(?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9]))+)?(?::[0-9]+)?/)?[a-z0-9]+(?:(?:(?:[._]|__|[-]*)[a-z0-9]+)+)?(?:(?:/[a-z0-9]+(?:(?:(?:[._]|__|[-]*)[a-z0-9]+)+)?)+)?$\""

This can probably be fixed by just adding more craziness to the regex, although it'll take me a little while to comprehend what is already there.

@squaremo
Copy link
Member

Yeah we should bite the bullet and support image refs with digests. I don't know what we'll do about automation -- maybe ignore them? -- but we could certainly understand them at least.

@squaremo squaremo changed the title Handle image names with an image ID instead of a tag Handle image names with an image digest instead of a tag May 29, 2018
@Nogbit
Copy link

Nogbit commented Dec 19, 2018

Any update on this? It still clutters the logs every 2-4 times per minute.

@munnerz
Copy link

munnerz commented Mar 6, 2019

I'd be happy to take a look at this, as I much prefer pinning my images with sha256 tags.

Is there anything in particular that needs doing here aside from relaxing the regex? I see this issue has "blocked-design" and wonder what extra complexities in flux I may not be aware of that this could affect 😄

@squaremo
Copy link
Member

squaremo commented Mar 6, 2019

Is there anything in particular that needs doing here aside from relaxing the regex? I see this issue has "blocked-design"

Lots of the code, especially automated updates, assumes it's dealing with image tags. That will have to be adapted, and how it's adapted needs some thinking through (-> "blocked-design").

@2opremio
Copy link
Contributor

2opremio commented Mar 6, 2019

@munnerz we need to think about what to do with digest references with respect to updates.

  1. Regarding automatic updates:

    I don't think we should ignore them, I would say we do business as usual (i.e. if the workload is annotated as flux.weave.works/automated: "true" we just update it as we used to, regardless of its tag being a sha256 reference).

    However (at least for now) I don't think we should do any smart stuff such us:

    • using the tag of the image the sha256 refers to for semver comparison
    • using a sha256 of the promoted image if an update happens (i.e. if an update happens, automatic or not, we use the tag).
  2. Regarding manual updates (e.g. fluxctl release) we should understand them an update workload containers to the image provided as usual.

To summarize, if @squaremo agrees, I think this would entail:

  1. Making sure image digest references are understood by flux when:
    • read from a workload manifest in Git and a workload in K8s
    • provided as an argument to fluxctl and, more generally, the HTTP & net/rpc APIs
  2. Making sure that we can distinguish the digest references from the usual tag references , updating the image.Ref if necessary and making sure backwards compatibility isn't broken in the API.

@squaremo
Copy link
Member

squaremo commented Mar 6, 2019

@2opremio Yes I'd go along with that. It's not the absolute minimum that could be done (which is something like, parse images refs with digests but skip them when updating things), but perhaps the minimal satisfactory step to take.

@carlosjgp
Copy link

@2opremio , @squaremo
I think the point of using sha256 hash as an image tag is avoiding upgrades and stick to that particular version always. This is particularly useful for images that only publish latest tags and you can't choose between receiving patches, minor or major upgrades

eg: https://hub.docker.com/r/solsson/kafka-prometheus-jmx-exporter/tags

So in the case of a workload annotated to receive upgrades you might want to upgrade only 1 of the containers of the pod.

Would you accept a PR to remove the log error about not understanding the image tag? It's filling our logs with a lot of noise :(

@carlosjgp
Copy link

It might not be clear on my previous comment but on my comment "Remove the log error"... I meant by extending the regex, adding a special condition or any other approach without affecting the current Flux functionality

@2opremio
Copy link
Contributor

I would much rather get a contribution implementing digest support (you have tips for how to this in my comment above)

carlosjgp added a commit to carlosjgp/flux that referenced this issue Jun 15, 2019
In some occasions the same tag can be assign to different images.
Like for example the official Python images tagged with `3.7` or other
projects which may not have a labeling strategy which only use label
like `latest` or `master`. Using digests instead of labels ensures that
you don't recieve unexpected updagrades

More info about tags and digests can be found
[here](https://success.docker.com/article/images-tagging-vs-digests)
Fixes fluxcd#885
carlosjgp added a commit to carlosjgp/flux that referenced this issue Jun 15, 2019
In some occasions the same tag can be assigned to different images.
Like for example the official Python images tagged with `3.7` or other
projects which may not have a labeling strategy which only uses a label
like `latest` or `master`. Using digests instead of labels ensures that
you don't receive unexpected upgrades

More info about tags and digests can be found
[here](https://success.docker.com/article/images-tagging-vs-digests)

Fixes fluxcd#885
@kingdonb
Copy link
Member

I think this is supported in Flux v1.22.0 and above.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants