From 96fb47213efc62fd6b01a11375a388763f767161 Mon Sep 17 00:00:00 2001 From: Antonio Murdaca Date: Wed, 14 Feb 2018 13:16:55 +0100 Subject: [PATCH 1/3] container_create: correctly set user We had a bug in ImageStatus where we weren't returning the default image user if set, thus running all containers as root despite a user being set in the image config. We weren't populating the Username field of ImageStatus. This patch fixes that along with the handling of multiple images based on the registry patch for multiple images. It also fixes ListImages to return Username as well. Signed-off-by: Antonio Murdaca --- pkg/storage/image.go | 34 +++++++++++++++++++ server/container_create.go | 2 +- server/image_list.go | 26 +++++++------- server/image_status.go | 69 ++++++++++++++++++++++++++++++-------- 4 files changed, 103 insertions(+), 28 deletions(-) diff --git a/pkg/storage/image.go b/pkg/storage/image.go index 5994d952..a94f95ad 100644 --- a/pkg/storage/image.go +++ b/pkg/storage/image.go @@ -39,6 +39,7 @@ type ImageResult struct { Size *uint64 Digest digest.Digest ConfigDigest digest.Digest + User string } type indexInfo struct { @@ -181,6 +182,16 @@ func (svc *imageService) ListImages(systemContext *types.SystemContext, filter s if err != nil { return nil, err } + imageFull, err := ref.NewImage(systemContext) + if err != nil { + return nil, err + } + defer imageFull.Close() + + imageConfig, err := imageFull.OCIConfig() + if err != nil { + return nil, err + } name, tags, digests := sortNamesByType(image.Names) imageDigest, repoDigests := svc.makeRepoDigests(digests, tags, image.ID) results = append(results, ImageResult{ @@ -191,6 +202,7 @@ func (svc *imageService) ListImages(systemContext *types.SystemContext, filter s Size: size, Digest: imageDigest, ConfigDigest: configDigest, + User: imageConfig.Config.User, }) } } else { @@ -213,6 +225,16 @@ func (svc *imageService) ListImages(systemContext *types.SystemContext, filter s if err != nil { return nil, err } + imageFull, err := ref.NewImage(systemContext) + if err != nil { + return nil, err + } + defer imageFull.Close() + + imageConfig, err := imageFull.OCIConfig() + if err != nil { + return nil, err + } name, tags, digests := sortNamesByType(image.Names) imageDigest, repoDigests := svc.makeRepoDigests(digests, tags, image.ID) results = append(results, ImageResult{ @@ -223,6 +245,7 @@ func (svc *imageService) ListImages(systemContext *types.SystemContext, filter s Size: size, Digest: imageDigest, ConfigDigest: configDigest, + User: imageConfig.Config.User, }) } } @@ -246,6 +269,16 @@ func (svc *imageService) ImageStatus(systemContext *types.SystemContext, nameOrI if err != nil { return nil, err } + imageFull, err := ref.NewImage(systemContext) + if err != nil { + return nil, err + } + defer imageFull.Close() + + imageConfig, err := imageFull.OCIConfig() + if err != nil { + return nil, err + } img, err := ref.NewImageSource(systemContext) if err != nil { @@ -268,6 +301,7 @@ func (svc *imageService) ImageStatus(systemContext *types.SystemContext, nameOrI Size: size, Digest: imageDigest, ConfigDigest: configDigest, + User: imageConfig.Config.User, } return &result, nil diff --git a/server/container_create.go b/server/container_create.go index 2de24f67..07b6ee67 100644 --- a/server/container_create.go +++ b/server/container_create.go @@ -449,7 +449,7 @@ func setupContainerUser(specgen *generate.Generator, rootfs string, sc *pb.Linux containerUser := "" // Case 1: run as user is set by kubelet if sc.GetRunAsUser() != nil { - containerUser = strconv.FormatInt(sc.GetRunAsUser().Value, 10) + containerUser = strconv.FormatInt(sc.GetRunAsUser().GetValue(), 10) } else { // Case 2: run as username is set by kubelet userName := sc.GetRunAsUsername() diff --git a/server/image_list.go b/server/image_list.go index bcdc1036..ac75ff9a 100644 --- a/server/image_list.go +++ b/server/image_list.go @@ -31,20 +31,20 @@ func (s *Server) ListImages(ctx context.Context, req *pb.ListImagesRequest) (res } resp = &pb.ListImagesResponse{} for _, result := range results { - if result.Size != nil { - resp.Images = append(resp.Images, &pb.Image{ - Id: result.ID, - RepoTags: result.RepoTags, - RepoDigests: result.RepoDigests, - Size_: *result.Size, - }) - } else { - resp.Images = append(resp.Images, &pb.Image{ - Id: result.ID, - RepoTags: result.RepoTags, - RepoDigests: result.RepoDigests, - }) + resImg := &pb.Image{ + Id: result.ID, + RepoTags: result.RepoTags, + RepoDigests: result.RepoDigests, } + uid, username := getUserFromImage(result.User) + if uid != nil { + resImg.Uid = &pb.Int64Value{Value: *uid} + } + resImg.Username = username + if result.Size != nil { + resImg.Size_ = *result.Size + } + resp.Images = append(resp.Images, resImg) } logrus.Debugf("ListImagesResponse: %+v", resp) return resp, nil diff --git a/server/image_status.go b/server/image_status.go index 4e2e6a0e..842f612d 100644 --- a/server/image_status.go +++ b/server/image_status.go @@ -2,6 +2,8 @@ package server import ( "fmt" + "strconv" + "strings" "time" "github.com/containers/storage" @@ -37,23 +39,62 @@ func (s *Server) ImageStatus(ctx context.Context, req *pb.ImageStatusRequest) (r return nil, err } } - // match just the first registry as that's what kube meant - image = images[0] - status, err := s.StorageImageServer().ImageStatus(s.ImageContext(), image) - if err != nil { - if errors.Cause(err) == storage.ErrImageUnknown { - return &pb.ImageStatusResponse{}, nil + var ( + notfound bool + lastErr error + ) + for _, image := range images { + status, err := s.StorageImageServer().ImageStatus(s.ImageContext(), image) + if err != nil { + if errors.Cause(err) == storage.ErrImageUnknown { + logrus.Warnf("imageStatus: can't find %s", image) + notfound = true + continue + } + logrus.Warnf("imageStatus: error getting status from %s: %v", image, err) + lastErr = err + continue } - return nil, err + resp = &pb.ImageStatusResponse{ + Image: &pb.Image{ + Id: status.ID, + RepoTags: status.RepoTags, + RepoDigests: status.RepoDigests, + Size_: *status.Size, + }, + } + uid, username := getUserFromImage(status.User) + if uid != nil { + resp.Image.Uid = &pb.Int64Value{Value: *uid} + } + resp.Image.Username = username + break } - resp = &pb.ImageStatusResponse{ - Image: &pb.Image{ - Id: status.ID, - RepoTags: status.RepoTags, - RepoDigests: status.RepoDigests, - Size_: *status.Size, - }, + if lastErr != nil && resp == nil { + return nil, lastErr + } + if notfound && resp == nil { + return &pb.ImageStatusResponse{}, nil } logrus.Debugf("ImageStatusResponse: %+v", resp) return resp, nil } + +// getUserFromImage gets uid or user name of the image user. +// If user is numeric, it will be treated as uid; or else, it is treated as user name. +func getUserFromImage(user string) (*int64, string) { + // return both empty if user is not specified in the image. + if user == "" { + return nil, "" + } + // split instances where the id may contain user:group + user = strings.Split(user, ":")[0] + // user could be either uid or user name. Try to interpret as numeric uid. + uid, err := strconv.ParseInt(user, 10, 64) + if err != nil { + // If user is non numeric, assume it's user name. + return nil, user + } + // If user is a numeric uid. + return &uid, "" +} From c53211eacd63718d1eb1219990dfd6f5bb39f118 Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Wed, 14 Feb 2018 13:57:18 -0500 Subject: [PATCH 2/3] imageService: cache information about images Cache information about images that isn't trivially read from them, so that ImageStatus and particularly ListImages don't have to do potentially-expensive things for every image that they report. The cache is an in-memory map, and we prune it after ListImages has assembled its result set. Signed-off-by: Nalin Dahyabhai --- pkg/storage/image.go | 145 ++++++++++++++++++++++++++++++------------- 1 file changed, 102 insertions(+), 43 deletions(-) diff --git a/pkg/storage/image.go b/pkg/storage/image.go index a94f95ad..2c2c3f62 100644 --- a/pkg/storage/image.go +++ b/pkg/storage/image.go @@ -47,12 +47,22 @@ type indexInfo struct { secure bool } +// A set of information that we prefer to cache about images, so that we can +// avoid having to reread them every time we need to return information about +// images. +type imageCacheItem struct { + user string + size *uint64 + configDigest digest.Digest +} + type imageService struct { store storage.Store defaultTransport string insecureRegistryCIDRs []*net.IPNet indexConfigs map[string]*indexInfo registries []string + imageCache map[string]imageCacheItem } // sizer knows its size. @@ -172,25 +182,38 @@ func (svc *imageService) ListImages(systemContext *types.SystemContext, filter s return nil, err } if image, err := istorage.Transport.GetStoreImage(svc.store, ref); err == nil { - img, err := ref.NewImageSource(systemContext) - if err != nil { - return nil, err - } - size := imageSize(img) - configDigest, err := imageConfigDigest(img, nil) - img.Close() - if err != nil { - return nil, err - } - imageFull, err := ref.NewImage(systemContext) - if err != nil { - return nil, err - } - defer imageFull.Close() - - imageConfig, err := imageFull.OCIConfig() - if err != nil { - return nil, err + var user string + var size *uint64 + var configDigest digest.Digest + if cacheItem, ok := svc.imageCache[image.ID]; ok { + user, size, configDigest = cacheItem.user, cacheItem.size, cacheItem.configDigest + } else { + img, err := ref.NewImageSource(systemContext) + if err != nil { + return nil, err + } + size = imageSize(img) + configDigest, err = imageConfigDigest(img, nil) + img.Close() + if err != nil { + return nil, err + } + imageFull, err := ref.NewImage(systemContext) + if err != nil { + return nil, err + } + defer imageFull.Close() + imageConfig, err := imageFull.OCIConfig() + if err != nil { + return nil, err + } + user = imageConfig.Config.User + cacheItem := imageCacheItem{ + user: user, + size: size, + configDigest: configDigest, + } + svc.imageCache[image.ID] = cacheItem } name, tags, digests := sortNamesByType(image.Names) imageDigest, repoDigests := svc.makeRepoDigests(digests, tags, image.ID) @@ -202,7 +225,7 @@ func (svc *imageService) ListImages(systemContext *types.SystemContext, filter s Size: size, Digest: imageDigest, ConfigDigest: configDigest, - User: imageConfig.Config.User, + User: user, }) } } else { @@ -210,30 +233,65 @@ func (svc *imageService) ListImages(systemContext *types.SystemContext, filter s if err != nil { return nil, err } + visited := make(map[string]struct{}) + defer func() { + // We built a map using IDs of images that we looked + // at, so remove any items from the cache that don't + // correspond to any of those IDs. + removedIDs := make([]string, 0, len(svc.imageCache)) + for imageID := range svc.imageCache { + if _, keep := visited[imageID]; !keep { + // We have cached data for an image + // with this ID, but it's not in the + // list of images now, so the image has + // been removed. + removedIDs = append(removedIDs, imageID) + } + } + // Handle the removals. + for _, removedID := range removedIDs { + delete(svc.imageCache, removedID) + } + }() for _, image := range images { - ref, err := istorage.Transport.ParseStoreReference(svc.store, "@"+image.ID) - if err != nil { - return nil, err - } - img, err := ref.NewImageSource(systemContext) - if err != nil { - return nil, err - } - size := imageSize(img) - configDigest, err := imageConfigDigest(img, nil) - img.Close() - if err != nil { - return nil, err - } - imageFull, err := ref.NewImage(systemContext) - if err != nil { - return nil, err - } - defer imageFull.Close() + visited[image.ID] = struct{}{} + var user string + var size *uint64 + var configDigest digest.Digest + if cacheItem, ok := svc.imageCache[image.ID]; ok { + user, size, configDigest = cacheItem.user, cacheItem.size, cacheItem.configDigest + } else { + ref, err := istorage.Transport.ParseStoreReference(svc.store, "@"+image.ID) + if err != nil { + return nil, err + } + img, err := ref.NewImageSource(systemContext) + if err != nil { + return nil, err + } + size = imageSize(img) + configDigest, err = imageConfigDigest(img, nil) + img.Close() + if err != nil { + return nil, err + } + imageFull, err := ref.NewImage(systemContext) + if err != nil { + return nil, err + } + defer imageFull.Close() - imageConfig, err := imageFull.OCIConfig() - if err != nil { - return nil, err + imageConfig, err := imageFull.OCIConfig() + if err != nil { + return nil, err + } + user = imageConfig.Config.User + cacheItem := imageCacheItem{ + user: user, + size: size, + configDigest: configDigest, + } + svc.imageCache[image.ID] = cacheItem } name, tags, digests := sortNamesByType(image.Names) imageDigest, repoDigests := svc.makeRepoDigests(digests, tags, image.ID) @@ -245,7 +303,7 @@ func (svc *imageService) ListImages(systemContext *types.SystemContext, filter s Size: size, Digest: imageDigest, ConfigDigest: configDigest, - User: imageConfig.Config.User, + User: user, }) } } @@ -619,6 +677,7 @@ func GetImageService(store storage.Store, defaultTransport string, insecureRegis indexConfigs: make(map[string]*indexInfo, 0), insecureRegistryCIDRs: make([]*net.IPNet, 0), registries: cleanRegistries, + imageCache: make(map[string]imageCacheItem), } insecureRegistries = append(insecureRegistries, "127.0.0.0/8") From 125ec8a7bd7d14a4986846c082c4654e62e1fb20 Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Wed, 14 Feb 2018 17:55:31 -0800 Subject: [PATCH 3/3] image: Add lock around image cache access Signed-off-by: Mrunal Patel --- pkg/storage/image.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/pkg/storage/image.go b/pkg/storage/image.go index 2c2c3f62..9df7ffbc 100644 --- a/pkg/storage/image.go +++ b/pkg/storage/image.go @@ -5,6 +5,7 @@ import ( "net" "path" "strings" + "sync" "github.com/containers/image/copy" "github.com/containers/image/docker/reference" @@ -63,6 +64,7 @@ type imageService struct { indexConfigs map[string]*indexInfo registries []string imageCache map[string]imageCacheItem + imageCacheLock sync.Mutex } // sizer knows its size. @@ -249,16 +251,21 @@ func (svc *imageService) ListImages(systemContext *types.SystemContext, filter s } } // Handle the removals. + svc.imageCacheLock.Lock() for _, removedID := range removedIDs { delete(svc.imageCache, removedID) } + svc.imageCacheLock.Unlock() }() for _, image := range images { visited[image.ID] = struct{}{} var user string var size *uint64 var configDigest digest.Digest - if cacheItem, ok := svc.imageCache[image.ID]; ok { + svc.imageCacheLock.Lock() + cacheItem, ok := svc.imageCache[image.ID] + svc.imageCacheLock.Unlock() + if ok { user, size, configDigest = cacheItem.user, cacheItem.size, cacheItem.configDigest } else { ref, err := istorage.Transport.ParseStoreReference(svc.store, "@"+image.ID) @@ -291,7 +298,9 @@ func (svc *imageService) ListImages(systemContext *types.SystemContext, filter s size: size, configDigest: configDigest, } + svc.imageCacheLock.Lock() svc.imageCache[image.ID] = cacheItem + svc.imageCacheLock.Unlock() } name, tags, digests := sortNamesByType(image.Names) imageDigest, repoDigests := svc.makeRepoDigests(digests, tags, image.ID)