-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support sha256 digest pinning #165
Comments
That's an interesting perspective. One would think that pinning a digest implies you want to fix the image at that digest, and refuse any others. But as you explained, that's not the only way to read this scrawl - you might just want the digest included for accounting purposes, even just so you can force a reload when it has changed upstream. This sounds like a potential novel application of ImagePolicy that wouldn't have been possible without overcomplicating in Flux v1. I think we will go on recommending immutable tags, but some folks are not in control of the repos upstream, like I assume you don't in your example, haproxy. We usually say that image tags must be immutable, but like git tags they are not immutable unless policy is configured preventing them from being overwritten. In the vast majority of image repos in the wild (and git repos, for that matter) this isn't the default behavior and it probably hasn't been configured that way, either. Thanks for opening a new thread. |
Oh it just occurs to me to add that I don't want this to ignore the ImagePolicy. I want this to enhance it. That is, if I have an ImagePolicy that says
I still agree with you 100% that I would love tags to immutable and I do not want you to recommend it any other way. This is just the situation that I find myself in. |
@plockaby how would Flux know which digest to chose? Most images are multi-arch and for the same tag there are many digests available. |
Ha, in the few hours since I started this thread 2.2.14 was indeed replaced with a new digest. For multiarch images there is always a digest that represents the "parent". So for haproxy:2.2.14, these are the digests of the individual architectures: linux/s390x: sha256:88d4465a398b20357befa2125b348a2d4d4fd38846a5b0bc40ff403ce3411bb1 However, when I ask the registry for the hash then I get this:
And when I restart haproxy and force it to pull a new one on my cluster:
This is in fact the same thing that Dependabot does. If you put an architecture specific digest into your Dockerfile dependabot will replace it with the architecture general digest. |
To accommodate the digest patching we could introduce the following setters:
|
With respect to flux v1, there's a bunch of cruft around dealing with images and their names, and it was hard to see how to fit digests into it. Hence, resistance. But I absolutely see the need for using digests. For Flux v2 there's a better opportunity to cover that ground. #90 suggests including setters for digests (a suggestion repeated above). fluxcd/image-reflector-controller#87 talks about how to include the digest in the |
FYI: Here is a detailed writeup about the problem: https://blog.atomist.com/keeping-up-with-docker-official-images/ |
I think we agreed this would be a nice feature, and now it is just waiting for someone to implement it. (Back up, it will make sense for there to be a more formal proposal first, perhaps this can be posted to Discussions?) I do not think it makes sense to start work on this before the refactoring is over, but it can still get design cycles and be discussed as a proposal. |
I need this as well, and I think it makes sense and would also close #90 by creating a digest setter. @kingdonb can you elaborate or link to the refactoring going one? As far as design, what I would propose is that there be another option for |
@mbrancato This is the meta-issue to connect all of the meta-issues that are supposed to represent the big refactor: Digest pinning opens up a lot of new possibilities that wouldn't have made sense with straight tags alone. When an image tag is reused non-immutably (in a way that wouldn't be possible with GitOps without digests) there can be a digest update mentioned in the gitrepo source, such that you can change the image to a later version without changing the tag itself. This suggests to me the possibility of a "permissive" and "immutable" mode for digest automation policy. You can pin a tag at a particular digest, or you can pin it to the latest observed digest. This would be useful for containers like the memcached tags (that were used in the Flux v1 helm chart), where the software package itself is less frequently released than the base image from upstream, and tags are not considered as immutable (see: docker.io/library/memcached:1.6.10-alpine3.14. This tag gets pushed over by memcached CI infra each time a new alpine base image from upstream is published. It has the behavior of a Flux should have an automation mode which is fully expressive and compatible with this, it is a common mode of operating. |
@stefanprodan any update on this matter? is it possible to use this marker {"$imagepolicy": "::tag:digest"} |
@grglzrv This is not available for now The I don't know what might block the digest tag policy type markers from being implemented, as described in #165 (comment) I think that it gets a lot easier now that we're dropping libgit2 support everywhere across the board. This is one of the major refactors that we've been waiting for, before there can be more structural changes or expansion of features scope. There is a compelling use case to write image digests directly into YAML manifests, especially considering how you could use them with cosign to make ImageUpdateAutomations actually secure and verified with a CI process that checks for things like policy violations or linter/syntax errors ahead of sending the artifact to the cluster to be applied, I'm excited about the possibilities in this issue and I think this is something we should prioritize. (I'm flagging this one to be included in the discussion at Bug Scrub today as well, so it gets some more attention!) |
@kingdonb, Can I try and take this issue? or do you think it is more suitable for a flux member? |
@ehudyonasi I'm happy to assign this to you, if you want to work on it! Please bear in mind that without an RFC, we haven't communicated clearly and exhaustively what this feature should look like, in as few words as possible what I'm trying to say is that it may be easier to submit a PR than to get it merged. But I'm strongly in support of this feature, and I know we haven't made room for it on any roadmap yet, if you want to work on it, then I'm happy to provide feedback! |
@ehudyonasi I think since you volunteered to have this issue assigned to you, I've fielded this question no less than 4 times, or reached for this capability myself at least as many times. I'm really interested to hear if you've made progress! |
To allow binary authorization to work (GCP context), one can still tag an image, but the digest needs to be there as well. This means this form:
So having a setter like @stefanprodan suggested would work for us.
It seems there is currently no-one working on this however? I'll ask around in our org if we have someone capable of working on this. Is there perhaps somebody able to put us on the right path to start looking at this? |
@kingdonb, I hope to send a PR until next week, and then from there we can discuss the issues if there will be until we can get it merged :) |
Using the new `:digest` suffix in policy markers one can now instruct IAC to store the digest of an image instead of the tag in the given manifest. This makes use of the the `.status.imageDigest` field introduced in fluxcd/image-reflector-controller#368. closes #165 Signed-off-by: Max Jonas Werner <[email protected]>
Using the new `:digest` suffix in policy markers one can now instruct IAC to store the digest of an image instead of the tag in the given manifest. This makes use of the the `.status.imageDigest` field introduced in fluxcd/image-reflector-controller#368. closes #165 Signed-off-by: Max Jonas Werner <[email protected]>
Using the new `:digest` suffix in policy markers one can now instruct IAC to store the digest of an image instead of the tag in the given manifest. This makes use of the the `.status.imageDigest` field introduced in fluxcd/image-reflector-controller#368. closes #165 Signed-off-by: Max Jonas Werner <[email protected]>
Using the new `:digest` suffix in policy markers one can now instruct IAC to store the digest of an image instead of the tag in the given manifest. This makes use of the the `.status.imageDigest` field introduced in fluxcd/image-reflector-controller#368. closes #165 Signed-off-by: Max Jonas Werner <[email protected]>
Using the new `:digest` suffix in policy markers one can now instruct IAC to store the digest of an image instead of the tag in the given manifest. This makes use of the the `.status.imageDigest` field introduced in fluxcd/image-reflector-controller#368. closes #165 Signed-off-by: Max Jonas Werner <[email protected]>
I'm so happy to see @makkes PR #508 that makes this available, in particular so I can automate updates of upstream images that don't use any sensible tagging (e.g. just @makkes PR is untouched for months, despite the fact that he appears to be a flux core maintainer. This is just a long winded way of saying "bump" to try and get some activity I guess? |
@ThisGuyCodes thanks for pushing this up. Flux team was mostly occupied with the GA release and we are slowly digging up the issues that had to be shelved in favor of GA. I took action moving this work further. Expect an update this week. |
Hi folks, this is a feature which interests me too. Happy to help however I can |
It's on my list of things to pick up in Q1 2024. |
Greetings, I am curious about this as well, since I use a few images where the publishing practices are abysmal, e.g exclusively tagging :latest and thats it. Is there a current roadblock on this or currently just on hold due to time reasons? Best Regards |
It still has not been patched(
does not apply anything on this line |
@glad2os As this issue is still open please consider the feature to not be implemented, yet. |
I figured it'd be more polite to open a new issue about this rather than spawn two conversations from #164 so I'm quoting from that.
I've been thinking about this a bit more. As a user of flux v1 I very much understand the rate limiting issues that were arising. However, tags are mutable and (from my understanding) it is considered a good security practice to pin to digests. Indeed, if I have two nodes that run haproxy it is possible that I could end up deploying two different versions of haproxy:2.2.14 if I do not use digest pinning. (I'm going to keep using haproxy as an example. Sorry.) While I don't want to discourage anyone from just putting into their yaml files a tag and being happy, I would like request support for digest pinning.
Here's how I figure it can work:
Image: haproxy:2.2.14@sha256:792ac7e9a42dcb028039bd6ec02cfdde75e94980652121e1fe557e7cf8b847bb
in my configuration.Getting the correct digest fortunately only requires one call to the registry for the one tag and not repeated calls for all tags and also does not need to be made if the flux user isn't using digest pinning. It's as simple as:
I don't feel like this is outlandish since this is basically the same support that dependabot has:
However, I do know that this has been requested a few times on the v1 repository and rejected. It's ok with me if you don't want to adopt this for v2 but if I don't ask then I can't receive. :)
The text was updated successfully, but these errors were encountered: