From 5e6f94dd47c4d130f0553f77443f913ce4c88b25 Mon Sep 17 00:00:00 2001 From: Roberto Jimenez Sanchez Date: Tue, 17 Jan 2017 15:44:38 +0000 Subject: [PATCH] Delete does not fail when image is not found [finishes #137370769] Signed-off-by: Claudia Beresford --- commands/delete.go | 7 +++-- commands/idfinder/idfinder.go | 40 +++++++++++++++++++----- commands/idfinder/idfinder_test.go | 49 +++++++++++++++++++++++++----- commands/stats.go | 3 +- integration/groot/delete_test.go | 23 +++++++++++--- integration/groot/stats_test.go | 4 +-- integration/root/delete_test.go | 7 +++-- 7 files changed, 105 insertions(+), 28 deletions(-) diff --git a/commands/delete.go b/commands/delete.go index 1a032738c..f110b1353 100644 --- a/commands/delete.go +++ b/commands/delete.go @@ -3,6 +3,7 @@ package commands // import "code.cloudfoundry.org/grootfs/commands" import ( "errors" "fmt" + "os" "path/filepath" "code.cloudfoundry.org/grootfs/commands/config" @@ -42,10 +43,12 @@ var DeleteCommand = cli.Command{ storePath := cfg.BaseStorePath idOrPath := ctx.Args().First() - id, err := idfinder.FindID(storePath, idOrPath) + userID := os.Getuid() + id, err := idfinder.FindID(storePath, userID, idOrPath) if err != nil { logger.Error("find-id-failed", err, lager.Data{"id": idOrPath, "storePath": storePath}) - return cli.NewExitError(err.Error(), 1) + fmt.Println(err) + return nil } if id == idOrPath { diff --git a/commands/idfinder/idfinder.go b/commands/idfinder/idfinder.go index 83423241d..ec0d580fa 100644 --- a/commands/idfinder/idfinder.go +++ b/commands/idfinder/idfinder.go @@ -2,6 +2,7 @@ package idfinder import ( "fmt" + "os" "path/filepath" "regexp" "strings" @@ -9,19 +10,33 @@ import ( "code.cloudfoundry.org/grootfs/store" ) -func FindID(storePath, pathOrID string) (string, error) { +func FindID(storePath string, userID int, pathOrID string) (string, error) { + var ( + imageID string + imagePath string + usrID string + ) + + usrID = fmt.Sprintf("%d", userID) if !strings.HasPrefix(pathOrID, "/") { - return pathOrID, nil + imageID = pathOrID + imagePath = filepath.Join(storePath, usrID, store.IMAGES_DIR_NAME, imageID) + } else { + imagePathRegex := filepath.Join(storePath, usrID, store.IMAGES_DIR_NAME, "(.*)") + pathRegexp := regexp.MustCompile(imagePathRegex) + matches := pathRegexp.FindStringSubmatch(pathOrID) + if len(matches) != 2 { + return "", fmt.Errorf("path `%s` is outside store path", pathOrID) + } + imageID = matches[1] + imagePath = pathOrID } - pathRegexp := regexp.MustCompile(filepath.Join(storePath, "[0-9]*", store.IMAGES_DIR_NAME, "(.*)")) - matches := pathRegexp.FindStringSubmatch(pathOrID) - - if len(matches) != 2 { - return "", fmt.Errorf("path `%s` is outside store path", pathOrID) + if !exists(imagePath) { + return "", fmt.Errorf("image `%s` was not found", imageID) } - return matches[1], nil + return imageID, nil } func FindSubStorePath(storePath, path string) (string, error) { @@ -34,3 +49,12 @@ func FindSubStorePath(storePath, path string) (string, error) { return filepath.Join(storePath, matches[1]), nil } + +func exists(imagePath string) bool { + if _, err := os.Stat(imagePath); err != nil { + if os.IsNotExist(err) { + return false + } + } + return true +} diff --git a/commands/idfinder/idfinder_test.go b/commands/idfinder/idfinder_test.go index 138216ced..dd601098e 100644 --- a/commands/idfinder/idfinder_test.go +++ b/commands/idfinder/idfinder_test.go @@ -1,48 +1,81 @@ package idfinder_test import ( + "io/ioutil" + "os" + "path" + "path/filepath" + "code.cloudfoundry.org/grootfs/commands/idfinder" + "code.cloudfoundry.org/grootfs/store" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) var _ = Describe("Idfinder", func() { + var ( + storePath string + imageDir string + imageId string + err error + ) + + BeforeEach(func() { + imageId = "1234-my-id" + storePath, err = ioutil.TempDir("", "") + imageDir = filepath.Join(storePath, "0", store.IMAGES_DIR_NAME) + Expect(os.MkdirAll(imageDir, 0777)).To(Succeed()) + Expect(err).NotTo(HaveOccurred()) + + Expect(ioutil.WriteFile(path.Join(imageDir, imageId), []byte("hello-world"), 0644)).To(Succeed()) + }) + + AfterEach(func() { + Expect(os.RemoveAll(storePath)).To(Succeed()) + }) + Context("FindID", func() { Context("when a ID is provided", func() { It("returns the ID", func() { - id, err := idfinder.FindID("/hello/store/path", "1234-my-id") + id, err := idfinder.FindID(storePath, 0, imageId) Expect(err).NotTo(HaveOccurred()) - Expect(id).To(Equal("1234-my-id")) + Expect(id).To(Equal(imageId)) }) }) Context("when a path is provided", func() { It("returns the ID", func() { - id, err := idfinder.FindID("/hello/store/path", "/hello/store/path/1200/images/1234-my-id") + id, err := idfinder.FindID(storePath, 0, filepath.Join(imageDir, imageId)) Expect(err).NotTo(HaveOccurred()) - Expect(id).To(Equal("1234-my-id")) + Expect(id).To(Equal(imageId)) }) Context("when the path is not within the store path", func() { It("returns an error", func() { - _, err := idfinder.FindID("/hello/store/path", "/hello/not-store/path/images/1234-my-id") + _, err := idfinder.FindID("/hello/not-store/path", 0, filepath.Join("/hello/not-store/path/images", imageId)) Expect(err).To(MatchError("path `/hello/not-store/path/images/1234-my-id` is outside store path")) }) }) }) + + It("returns an error when the image does not exist", func() { + _, err := idfinder.FindID(storePath, 0, filepath.Join(storePath, "0", store.IMAGES_DIR_NAME, "not-here")) + Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(ContainSubstring("image `not-here` was not found"))) + }) }) Context("SubStorePath", func() { It("returns the correct sub store path", func() { - storePath, err := idfinder.FindSubStorePath("/hello/store/path", "/hello/store/path/1200/images/1234-my-id") + subStorePath, err := idfinder.FindSubStorePath(storePath, filepath.Join(imageDir, imageId)) Expect(err).NotTo(HaveOccurred()) - Expect(storePath).To(Equal("/hello/store/path/1200")) + Expect(subStorePath).To(Equal(filepath.Join(storePath, "0"))) }) Context("when the path is not valid", func() { It("returns an error", func() { - _, err := idfinder.FindSubStorePath("/hello/store/path", "/hello/store/path/images/1234-my-id") + _, err := idfinder.FindSubStorePath(storePath, "/hello/store/path/images/1234-my-id") Expect(err).To(MatchError(ContainSubstring("unable to match substore in path `/hello/store/path/images/1234-my-id`"))) }) }) diff --git a/commands/stats.go b/commands/stats.go index f0a8352f5..9dc2ff89f 100644 --- a/commands/stats.go +++ b/commands/stats.go @@ -41,7 +41,8 @@ var StatsCommand = cli.Command{ storePath := cfg.BaseStorePath idOrPath := ctx.Args().First() - id, err := idfinder.FindID(storePath, idOrPath) + userID := os.Getuid() + id, err := idfinder.FindID(storePath, userID, idOrPath) if err != nil { logger.Error("find-id-failed", err, lager.Data{"id": idOrPath, "storePath": storePath}) return cli.NewExitError(err.Error(), 1) diff --git a/integration/groot/delete_test.go b/integration/groot/delete_test.go index 71b500d32..2bbae17e6 100644 --- a/integration/groot/delete_test.go +++ b/integration/groot/delete_test.go @@ -71,7 +71,7 @@ var _ = Describe("Delete", func() { logBuffer := gbytes.NewBuffer() err := Runner.WithStore("/invalid-store").WithStderr(logBuffer). Delete("/path/to/random-id") - Expect(err).To(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) Expect(logBuffer).To(gbytes.Say(`"id":"/path/to/random-id"`)) }) }) @@ -82,24 +82,37 @@ var _ = Describe("Delete", func() { Expect(image.Path).NotTo(BeAnExistingFile()) }) + Context("when a path to an image does not exist", func() { + It("succeeds but logs a warning", func() { + fakePath := path.Join(StorePath, CurrentUserID, "images/non_existing") + Expect(fakePath).NotTo(BeAnExistingFile()) + + cmd := exec.Command(GrootFSBin, "--store", StorePath, "delete", fakePath) + sess, err := gexec.Start(cmd, GinkgoWriter, GinkgoWriter) + Expect(err).ToNot(HaveOccurred()) + Eventually(sess).Should(gexec.Exit(0)) + Eventually(sess.Out).Should(gbytes.Say("image `non_existing` was not found")) + }) + }) + Context("when the path provided doesn't belong to the `--store` provided", func() { It("returns an error", func() { cmd := exec.Command(GrootFSBin, "--store", StorePath, "delete", "/Iamnot/in/the/storage/images/1234/rootfs") sess, err := gexec.Start(cmd, GinkgoWriter, GinkgoWriter) Expect(err).ToNot(HaveOccurred()) - Eventually(sess).Should(gexec.Exit(1)) + Eventually(sess).Should(gexec.Exit(0)) Eventually(sess.Out).Should(gbytes.Say("path `/Iamnot/in/the/storage/images/1234/rootfs` is outside store path")) }) }) }) Context("when the image ID doesn't exist", func() { - It("returns an error", func() { + It("succeeds but logs a warning", func() { cmd := exec.Command(GrootFSBin, "--store", StorePath, "delete", "non-existing-id") sess, err := gexec.Start(cmd, GinkgoWriter, GinkgoWriter) Expect(err).ToNot(HaveOccurred()) - Eventually(sess).Should(gexec.Exit(1)) - Eventually(sess.Out).Should(gbytes.Say("image not found")) + Eventually(sess).Should(gexec.Exit(0)) + Eventually(sess.Out).Should(gbytes.Say("image `non-existing-id` was not found")) }) }) diff --git a/integration/groot/stats_test.go b/integration/groot/stats_test.go index 8678b429f..4f81335e0 100644 --- a/integration/groot/stats_test.go +++ b/integration/groot/stats_test.go @@ -97,7 +97,7 @@ var _ = Describe("Stats", func() { Context("when the last parameter is a image id", func() { It("returns an error", func() { _, err := Runner.Stats("invalid-id") - Expect(err).To(MatchError(ContainSubstring("image not found: invalid-id"))) + Expect(err).To(MatchError(ContainSubstring("image `invalid-id` was not found"))) }) }) @@ -105,7 +105,7 @@ var _ = Describe("Stats", func() { It("returns an error", func() { invalidImagePath := filepath.Join(StorePath, CurrentUserID, store.IMAGES_DIR_NAME, "not-here") _, err := Runner.Stats(invalidImagePath) - Expect(err).To(MatchError(ContainSubstring("image not found: not-here"))) + Expect(err).To(MatchError(ContainSubstring("image `not-here` was not found"))) }) Context("when the path provided doesn't belong to the `--store` provided", func() { diff --git a/integration/root/delete_test.go b/integration/root/delete_test.go index a74142549..5acbb3785 100644 --- a/integration/root/delete_test.go +++ b/integration/root/delete_test.go @@ -1,6 +1,7 @@ package root_test import ( + "fmt" "io/ioutil" "os/exec" "path" @@ -11,6 +12,7 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + "github.com/onsi/gomega/gbytes" "github.com/onsi/gomega/gexec" ) @@ -26,7 +28,7 @@ var _ = Describe("Delete", func() { }) Context("when trying to delete a image from a different user", func() { - It("returns an error", func() { + It("doesn't return an error but logs a warning", func() { deleteCmd := exec.Command( GrootFSBin, "--log-level", "debug", @@ -44,7 +46,8 @@ var _ = Describe("Delete", func() { session, err := gexec.Start(deleteCmd, GinkgoWriter, GinkgoWriter) Expect(err).NotTo(HaveOccurred()) - Eventually(session).Should(gexec.Exit(1)) + Eventually(session).Should(gexec.Exit(0)) + Eventually(session.Out).Should(gbytes.Say(fmt.Sprintf("path `%s` is outside store path", image.Path))) Expect(image.Path).To(BeADirectory()) }) })