Skip to content
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

feat: add support for "pinned" images #920

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

inFocus7
Copy link
Contributor

@inFocus7 inFocus7 commented Nov 29, 2023

TODO (very likely) - move logic from the scanner to the collector & remover

What this PR does / why we need it:

Add support for CRI pinned images. Pinned is a field images which should not be garbage collected have. When scanning images for removal we should .

We also make this configurable to allow users to decide whether or not they care about pinned images.

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

Special notes for your reviewer:
The configurability can be removed if we think that images being pinned is enough for us to assume they should never be deleted.


Notes for me (Fabian) so I ensure these behaviors occur...

deletePinnedImages: true

isPinned notPinned
isVulnerable delete delete
notVulnerable keep keep

deletePinnedImages: false (default)

isPinned notPinned
isVulnerable keep delete
notVulnerable keep keep
  • Do we care about scanning pinned images if deletePinnedImages: false? Since we'll be keeping them either way...
    • As of now, I don't believe we have any reporting of CVEs, so scanning images we know we won't delete won't help with anything, but could keep us open to add to reports if we do so in the future (surface vulnerable images #356). I'll look into how/if we do special case handling for deleteEOL and other similar flags.

Signed-off-by: Fabian Gonzalez <fabiangonz98@gmail.com>
Signed-off-by: Fabian Gonzalez <fabiangonz98@gmail.com>
@inFocus7 inFocus7 marked this pull request as draft November 29, 2023 23:42
Signed-off-by: Fabian Gonzalez <fabiangonz98@gmail.com>
Signed-off-by: Fabian Gonzalez <fabiangonz98@gmail.com>
Comment on lines -141 to -145
// Convert_unversioned_Image_To_v1_Image is an autogenerated conversion function.
func Convert_unversioned_Image_To_v1_Image(in *unversioned.Image, out *Image, s conversion.Scope) error {
return autoConvert_unversioned_Image_To_v1_Image(in, out, s)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will look into why this got removed, i did hit some issues when I last generated code due to not being able to covert unversioned...Pinned to <version>...Pinned as the Pinned field did/does not exist in versioned generations.

@@ -151,6 +151,7 @@ replace (
k8s.io/component-helpers => k8s.io/component-helpers v0.26.11
k8s.io/controller-manager => k8s.io/controller-manager v0.26.11
k8s.io/csi-translation-lib => k8s.io/csi-translation-lib v0.26.11
k8s.io/dynamic-resource-allocation => k8s.io/dynamic-resource-allocation v0.26.11
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hit issues locally due to the infamous

unknown revision <kubernetes-library> v0.0.0

requiring me to pin this.

@@ -75,6 +76,12 @@ func removeImages(c cri.Remover, targetImages []string) (int, error) {
continue
}

// TODO - figure out why is imgDigestOrTag used instead of imageID when it's called "idToImageMap" (copied usage from isExcluded).
if !removePinned && util.IsPinned(imageID, idToImageMap) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The util.IsExcluded function does a idToImageMap[imgDigestOrTag], although I would expect it to use the imageId as the key..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is a frustrating one: Docker's ImageID is different from Containerd's Digest. Newer versions of docker can use containerd to manage storage & metadata of images.

But without that feature, docker reports the sha256 digest of the Image Config as the ImageID. On the other hand, containerd's Image Digest is the sha256 digest of the image manifest. Of the two, the image digest is more stable, because the name of the image is included in the Image Config. Thus, a change to the name of an image (without any changes to the image itself) will result in a change to the ImageID.

Thus we use the digest as the hash key for the Set of images we build. Each distinct digest is a distinct image, full stop; the same is not true for ImageIDs: tag an existing image with a new name and you have one distinct image with two distinct ImageIDs.

The CRI is kind of in an in-between state. It was developed to provide an interface and had to work with older clusters using dockershim and newer clusters using containerd. As such, it takes the ImageID into account more than it should.

Using trivy to scan by ImageID doesn't work. Trivy scans the containerd image store by creating a containerd client (from the containerd library) and querying it directly. Since containerd doesn't manage images by docker's ImageID, it can't provide any image information to trivy for the scan if it's looking for it by ImageID.

We want to scan and remove by content as much as possible, not by name. We use the image name + tag as a backup in the event that the call to the CRI's ListImages returns an image with no digest information. This happens surprisingly frequently because of the ImageID cruft in the CRI implementation.

Copy link
Contributor

@pmengelbert pmengelbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for the draft PR, and apologies for taking so long to review. I've suggested some changes to the general approach, and I will be happy to clarify if anything isn't clear.

@@ -25,6 +25,7 @@ type Image struct {
ImageID string `json:"image_id"`
Names []string `json:"names,omitempty"`
Digests []string `json:"digests,omitempty"`
Pinned bool `json:"pinned,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will want to make a simultaneous identical change to v1 and unversioned. We keep unversioned synchronized with the latest api version for each type. Currently:

  • imagejob_types.go -> v1 == unversioned
  • imagelist_types -> v1 == unversioned
  • eraserconfig_type -> v1alpha3 == unversioned

@@ -23,6 +23,7 @@ var (
enableProfile = flag.Bool("enable-pprof", false, "enable pprof profiling")
profilePort = flag.Int("pprof-port", 6060, "port for pprof profiling. defaulted to 6060 if unspecified")
scanDisabled = flag.Bool("scan-disabled", false, "boolean for if scanner container is disabled")
scanPinned = flag.Bool("scan-pinned", false, "boolean for if scanner container should scan pinned images")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are making an effort to stop using cli args to control the container applications. We are using the configmap instead. In the code that spawns this pod (imagecollector_controller.go or imagejob_controller.go), we can set an environment variable based on the configmap value.

For more information as to why we are moving away from cli args:
#446

@@ -75,6 +76,12 @@ func removeImages(c cri.Remover, targetImages []string) (int, error) {
continue
}

// TODO - figure out why is imgDigestOrTag used instead of imageID when it's called "idToImageMap" (copied usage from isExcluded).
if !removePinned && util.IsPinned(imageID, idToImageMap) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is a frustrating one: Docker's ImageID is different from Containerd's Digest. Newer versions of docker can use containerd to manage storage & metadata of images.

But without that feature, docker reports the sha256 digest of the Image Config as the ImageID. On the other hand, containerd's Image Digest is the sha256 digest of the image manifest. Of the two, the image digest is more stable, because the name of the image is included in the Image Config. Thus, a change to the name of an image (without any changes to the image itself) will result in a change to the ImageID.

Thus we use the digest as the hash key for the Set of images we build. Each distinct digest is a distinct image, full stop; the same is not true for ImageIDs: tag an existing image with a new name and you have one distinct image with two distinct ImageIDs.

The CRI is kind of in an in-between state. It was developed to provide an interface and had to work with older clusters using dockershim and newer clusters using containerd. As such, it takes the ImageID into account more than it should.

Using trivy to scan by ImageID doesn't work. Trivy scans the containerd image store by creating a containerd client (from the containerd library) and querying it directly. Since containerd doesn't manage images by docker's ImageID, it can't provide any image information to trivy for the scan if it's looking for it by ImageID.

We want to scan and remove by content as much as possible, not by name. We use the image name + tag as a backup in the event that the call to the CRI's ListImages returns an image with no digest information. This happens surprisingly frequently because of the ImageID cruft in the CRI implementation.

@@ -8,7 +8,7 @@ import (
util "github.com/eraser-dev/eraser/pkg/utils"
)

func removeImages(c cri.Remover, targetImages []string) (int, error) {
func removeImages(c cri.Remover, removePinned bool, targetImages []string) (int, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of passing a boolean flag, just require that the caller remove any pinned images from targetimages before invoking this function. It won't require any changes here but will require them elsewhere.

If the collector is turned on, filter them out during the collector stage. If the collector (and therefore scanner) is turned off, filter them out just prior to removal.

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

Successfully merging this pull request may close these issues.

Add support for CRI "pinned" images
2 participants