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.

Sign up for GitHub

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

Jump to bottom

feat: add support for "pinned" images #920

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

feat: add support for "pinned" images #920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
efcc951
include pinned image information
inFocus7 Sep 3, 2023
bdcd2e2
Add extra comments on areas we can manage Pinned images
inFocus7 Nov 29, 2023
c30da36
Merge branch 'main' into feat/add-cri-pinned-image-filter
inFocus7 Nov 29, 2023
7f95f58
Move Pinned check + removal to the collector and remover pods
inFocus7 Dec 2, 2023
d2d8b54
remove Pinned config from scanner
inFocus7 Dec 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/unversioned/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ const (
oneDay = unversioned.Duration(time.Hour * 24)
)

// TODO - add defaults for gathering/scanning/removing Pinned images
func Default() *unversioned.EraserConfig {
return &unversioned.EraserConfig{
Manager: unversioned.ManagerConfig{
Expand Down
1 change: 1 addition & 0 deletions api/unversioned/imagejob_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

}

// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN!
Expand Down
5 changes: 0 additions & 5 deletions api/v1/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 0 additions & 5 deletions api/v1alpha1/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions controllers/imagecollector/imagecollector_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,10 +308,12 @@ func (r *Reconciler) createImageJob(ctx context.Context) (ctrl.Result, error) {
fmt.Sprintf("--pprof-port=%d", profileConfig.Port),
}

collArgs := []string{"--scan-disabled=" + strconv.FormatBool(scanDisabled)}
// todo implement the config for this
collArgs := []string{"--scan-disabled=" + strconv.FormatBool(scanDisabled), "--scan-pinned=" + strconv.FormatBool(scanCfg.ScanPinned)}
collArgs = append(collArgs, profileArgs...)

removerArgs := []string{"--log-level=" + logger.GetLevel()}
// todo implement the config for this
removerArgs := []string{"--log-level=" + logger.GetLevel(), "--remove-pinned=" + strconv.FormatBool(eraserCfg.RemovePinned)}
removerArgs = append(removerArgs, profileArgs...)

pullSecrets := []corev1.LocalObjectReference{}
Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

k8s.io/kube-aggregator => k8s.io/kube-aggregator v0.26.11
k8s.io/kube-controller-manager => k8s.io/kube-controller-manager v0.26.11
k8s.io/kube-proxy => k8s.io/kube-proxy v0.26.11
Expand Down
6 changes: 6 additions & 0 deletions pkg/collector/collector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


// Timeout of connecting to server (default: 5m).
timeout = 5 * time.Minute
Expand Down Expand Up @@ -80,6 +81,11 @@ func main() {
}
log.Info("images collected", "finalImages:", finalImages)

if !(*scanPinned) {
log.Info("skipping scanning pinned images")
finalImages = util.RemovePinnedImages(finalImages)
}

data, err := json.Marshal(finalImages)
if err != nil {
log.Error(err, "failed to encode finalImages")
Expand Down
2 changes: 2 additions & 0 deletions pkg/collector/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ func getImages(c cri.Collector) ([]unversioned.Image, error) {
newImg := unversioned.Image{
ImageID: img.Id,
Names: repoTags,
Pinned: img.Pinned,
}

digests, errs := util.ProcessRepoDigests(img.RepoDigests)
Expand Down Expand Up @@ -71,6 +72,7 @@ func getImages(c cri.Collector) ([]unversioned.Image, error) {
ImageID: imageID,
Names: img.Names,
Digests: img.Digests,
Pinned: img.Pinned,
}

if !util.IsExcluded(excluded, currImage.ImageID, idToImageMap) {
Expand Down
14 changes: 13 additions & 1 deletion pkg/remover/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

removed := 0

backgroundContext, cancel := context.WithTimeout(context.Background(), timeout)
Expand All @@ -30,6 +30,7 @@ func removeImages(c cri.Remover, targetImages []string) (int, error) {
newImg := unversioned.Image{
ImageID: img.Id,
Names: repoTags,
Pinned: img.Pinned,
}

digests, errs := util.ProcessRepoDigests(img.RepoDigests)
Expand Down Expand Up @@ -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.

log.Info("image is kept due to being pinned", "given", imgDigestOrTag, "imageID", imageID, "name", idToImageMap[imageID])
continue
}

err = c.DeleteImage(backgroundContext, imageID)
if err != nil {
log.Error(err, "error removing image", "given", imgDigestOrTag, "imageID", imageID, "name", idToImageMap[imageID])
Expand Down Expand Up @@ -108,6 +115,11 @@ func removeImages(c cri.Remover, targetImages []string) (int, error) {
continue
}

if !removePinned && util.IsPinned(imageID, idToImageMap) {
log.Info("image is kept due to being pinned", "imageID", imageID, "name", idToImageMap[imageID])
continue
}

if err := c.DeleteImage(backgroundContext, imageID); err != nil {
success = false
log.Error(err, "error removing image", "imageID", imageID, "name", idToImageMap[imageID])
Expand Down
4 changes: 3 additions & 1 deletion pkg/remover/remover.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ var (
imageListPtr = flag.String("imagelist", "", "name of ImageList")
enableProfile = flag.Bool("enable-pprof", false, "enable pprof profiling")
profilePort = flag.Int("pprof-port", 6060, "port for pprof profiling. defaulted to 6060 if unspecified")
removePinned = flag.Bool("remove-pinned", false, "skip over pinned images when removing")

// Timeout of connecting to server (default: 5m).
timeout = 5 * time.Minute
Expand Down Expand Up @@ -130,7 +131,8 @@ func main() {
log.Info("no images to exclude")
}

removed, err := removeImages(client, imagelist)
// we pass in the removePinned flag to removeImages, because as of now we just have a list of imageIDs, and we don't know if they are pinned or not
removed, err := removeImages(client, *removePinned, imagelist)
if err != nil {
log.Error(err, "failed to remove images")
os.Exit(generalErr)
Expand Down
1 change: 1 addition & 0 deletions pkg/scanners/trivy/trivy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ func main() {
log.Info("Failed", "Images", failedImages)
}

// send to eraser?
err = provider.SendImages(vulnerableImages, failedImages)
if err != nil {
log.Error(err, "unable to write images")
Expand Down
14 changes: 14 additions & 0 deletions pkg/utils/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,10 @@ func GetNonRunningImages(runningImages map[string]string, allImages []unversione
return nonRunningImages
}

func IsPinned(img string, idToImageMap map[string]unversioned.Image) bool {
return idToImageMap[img].Pinned
}

func IsExcluded(excluded map[string]struct{}, img string, idToImageMap map[string]unversioned.Image) bool {
if len(excluded) == 0 {
return false
Expand Down Expand Up @@ -417,3 +421,13 @@ func ProcessRepoDigests(repoDigests []string) ([]string, []error) {

return digests, errs
}

func RemovePinnedImages(images []unversioned.Image) []unversioned.Image {
filteredImages := []unversioned.Image{}
for _, image := range images {
if !image.Pinned {
filteredImages = append(filteredImages, image)
}
}
return filteredImages
}
Loading