From 926262fb999c1880e49be3d4e9ed9af2a1cec4c0 Mon Sep 17 00:00:00 2001 From: Jordan Keister Date: Fri, 11 Oct 2024 17:08:40 -0500 Subject: [PATCH] first stab at api re-abstraction Signed-off-by: Jordan Keister --- cmd/manager/main.go | 7 ++-- internal/storage/localdir.go | 46 +++++++++++++++------ internal/storage/localdir_test.go | 69 ++++++++++++++++--------------- 3 files changed, 73 insertions(+), 49 deletions(-) diff --git a/cmd/manager/main.go b/cmd/manager/main.go index b4cfee79..0676cfdf 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -252,13 +252,14 @@ func main() { os.Exit(1) } - baseStorageURL, err := url.Parse(fmt.Sprintf("%s/catalogs/", externalAddr)) + externalURL, err := url.Parse(externalAddr) if err != nil { - setupLog.Error(err, "unable to create base storage URL") + setupLog.Error(err, fmt.Sprintf("unable to create base URL: %q", externalAddr)) os.Exit(1) } + v1RawStorageURL := externalURL.JoinPath("api", "v1") - localStorage = storage.LocalDir{RootDir: storeDir, RootURL: baseStorageURL} + localStorage = storage.LocalDir{RootDir: storeDir, RootURL: v1RawStorageURL} catalogServerConfig := serverutil.CatalogServerConfig{ ExternalAddr: externalAddr, diff --git a/internal/storage/localdir.go b/internal/storage/localdir.go index b406670c..dd959541 100644 --- a/internal/storage/localdir.go +++ b/internal/storage/localdir.go @@ -24,8 +24,10 @@ type LocalDir struct { RootURL *url.URL } +const rawFileName = "all.json" + func (s LocalDir) Store(ctx context.Context, catalog string, fsys fs.FS) error { - fbcDir := filepath.Join(s.RootDir, catalog, "api", "v1") + fbcDir := filepath.Join(s.RootDir, catalog) if err := os.MkdirAll(fbcDir, 0700); err != nil { return err } @@ -40,10 +42,10 @@ func (s LocalDir) Store(ctx context.Context, catalog string, fsys fs.FS) error { } _, err = tempFile.Write(meta.Blob) return err - }); err != nil { + }, declcfg.WithConcurrency(1)); err != nil { return fmt.Errorf("error walking FBC root: %w", err) } - fbcFile := filepath.Join(fbcDir, "all") + fbcFile := filepath.Join(fbcDir, rawFileName) return os.Rename(tempFile.Name(), fbcFile) } @@ -52,7 +54,7 @@ func (s LocalDir) Delete(catalog string) error { } func (s LocalDir) ContentURL(catalog string) string { - return fmt.Sprintf("%s%s/api", s.RootURL, catalog) + return fmt.Sprintf("%s%s", s.RootURL, catalog) } func (s LocalDir) StorageServerHandler() http.Handler { @@ -66,11 +68,12 @@ func (s LocalDir) StorageServerHandler() http.Handler { gzHandler.ServeHTTP(w, r) }) mux.Handle(s.RootURL.Path, typeHandler) + return mux } func (s LocalDir) ContentExists(catalog string) bool { - file, err := os.Stat(filepath.Join(s.RootDir, catalog, "api", "v1", "all")) + file, err := os.Stat(filepath.Join(s.RootDir, catalog, rawFileName)) if err != nil { return false } @@ -88,9 +91,11 @@ type filesOnlyFilesystem struct { FS fs.FS } -// Open opens a named file from the underlying filesystem. If the file -// is not a regular file, it return os.ErrNotExists. Callers are resposible -// for closing the file returned. +// Open accesses a catalog from the underlying filesystem to fulfill the v1 'raw' API IFF: +// 1. the request refers to a directory (catalog name), AND +// 2. the directory contains a file matching `rawFileName` variable +// otherwise the function will return the encountered error, or os.ErrNotExist +// Callers are resposible for closing the file returned. func (f *filesOnlyFilesystem) Open(name string) (fs.File, error) { file, err := f.FS.Open(name) if err != nil { @@ -101,9 +106,26 @@ func (f *filesOnlyFilesystem) Open(name string) (fs.File, error) { _ = file.Close() return nil, err } - if !stat.Mode().IsRegular() { - _ = file.Close() - return nil, os.ErrNotExist + // if the name is a directory, it should be the name of the catalog we want to access. + // test for the existence of the raw FBC file in that dir, and return it if found. + // otherwise, return os.ErrNotExist. + if stat.IsDir() { + defer file.Close() + rawFile, err := f.FS.Open(filepath.Join(name, rawFileName)) + if err != nil { + _ = file.Close() + return nil, os.ErrNotExist + } + filestat, err := rawFile.Stat() + if err != nil { + _ = file.Close() + return nil, err + } + if !filestat.Mode().IsRegular() { + _ = file.Close() + return nil, os.ErrNotExist + } + return rawFile, nil } - return file, nil + return nil, os.ErrNotExist } diff --git a/internal/storage/localdir_test.go b/internal/storage/localdir_test.go index ee8cc05e..80a5b28b 100644 --- a/internal/storage/localdir_test.go +++ b/internal/storage/localdir_test.go @@ -26,7 +26,8 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" ) -const urlPrefix = "/catalogs/" +const fsPrefix = "/catalogs/" +const rawURLPrefix = "/api/v1/" var ctx = context.Background() @@ -53,9 +54,9 @@ var _ = Describe("LocalDir Storage Test", func() { BeforeEach(func() { d, err := os.MkdirTemp(GinkgoT().TempDir(), "cache") Expect(err).ToNot(HaveOccurred()) - rootDir = d + rootDir = fmt.Sprintf("%s%s", d, fsPrefix) - baseURL = &url.URL{Scheme: "http", Host: "test-addr", Path: urlPrefix} + baseURL = &url.URL{Scheme: "http", Host: "test-addr", Path: rawURLPrefix} store = LocalDir{RootDir: rootDir, RootURL: baseURL} unpackResultFS = &fstest.MapFS{ "bundle.yaml": &fstest.MapFile{Data: []byte(testBundle), Mode: os.ModePerm}, @@ -67,22 +68,23 @@ var _ = Describe("LocalDir Storage Test", func() { BeforeEach(func() { err := store.Store(context.Background(), catalog, unpackResultFS) Expect(err).To(Not(HaveOccurred())) + exists := store.ContentExists(catalog) + Expect(exists).To(BeTrue()) }) It("should store the content in the RootDir correctly", func() { - fbcDir := filepath.Join(rootDir, catalog, "api", "v1") - fbcFile := filepath.Join(fbcDir, "all") + fbcDir := filepath.Join(rootDir, catalog) + fbcFile := filepath.Join(fbcDir, rawFileName) _, err := os.Stat(fbcFile) Expect(err).To(Not(HaveOccurred())) - gotConfig, err := declcfg.LoadFS(ctx, unpackResultFS) Expect(err).To(Not(HaveOccurred())) - storedConfig, err := declcfg.LoadFile(os.DirFS(fbcDir), "all") + storedConfig, err := declcfg.LoadFile(os.DirFS(fbcDir), rawFileName) Expect(err).To(Not(HaveOccurred())) diff := cmp.Diff(gotConfig, storedConfig) Expect(diff).To(Equal("")) }) It("should form the content URL correctly", func() { - Expect(store.ContentURL(catalog)).To(Equal(fmt.Sprintf("%s%s/api", baseURL, catalog))) + Expect(store.ContentURL(catalog)).To(Equal(fmt.Sprintf("%s%s", baseURL, catalog))) }) It("should report content exists", func() { Expect(store.ContentExists(catalog)).To(BeTrue()) @@ -113,10 +115,9 @@ var _ = Describe("LocalDir Server Handler tests", func() { BeforeEach(func() { d, err := os.MkdirTemp(GinkgoT().TempDir(), "cache") Expect(err).ToNot(HaveOccurred()) - Expect(os.MkdirAll(filepath.Join(d, "test-catalog", "api", "v1"), 0700)).To(Succeed()) - store = LocalDir{RootDir: d, RootURL: &url.URL{Path: urlPrefix}} + Expect(os.MkdirAll(filepath.Join(d, "test-catalog"), 0700)).To(Succeed()) + store = LocalDir{RootDir: d, RootURL: &url.URL{Path: rawURLPrefix}} testServer = httptest.NewServer(store.StorageServerHandler()) - }) It("gets 404 for the path /", func() { expectNotFound(testServer.URL) @@ -127,33 +128,33 @@ var _ = Describe("LocalDir Server Handler tests", func() { It("gets 404 for the path /catalogs/test-catalog/", func() { expectNotFound(fmt.Sprintf("%s/%s", testServer.URL, "/catalogs/test-catalog/")) }) - It("gets 404 for the path /test-catalog/foo.txt", func() { - // This ensures that even if the file exists, the URL must contain the /catalogs/ prefix + It("gets 404 for the path /api/v1/test-catalog when the expected raw FBC file does not exist", func() { + // This ensures that even if there is content, if it does not follow the expected format it will fail the access Expect(os.WriteFile(filepath.Join(store.RootDir, "test-catalog", "foo.txt"), []byte("bar"), 0600)).To(Succeed()) - expectNotFound(fmt.Sprintf("%s/%s", testServer.URL, "/test-catalog/foo.txt")) - }) - It("gets 404 for the path /catalogs/test-catalog/non-existent.txt", func() { - expectNotFound(fmt.Sprintf("%s/%s", testServer.URL, "/catalogs/test-catalog/non-existent.txt")) + expectNotFound(fmt.Sprintf("%s/%s", testServer.URL, "/api/v1/test-catalog/foo.txt")) }) - It("gets 200 for the path /catalogs/foo.txt", func() { - expectedContent := []byte("bar") - Expect(os.WriteFile(filepath.Join(store.RootDir, "foo.txt"), expectedContent, 0600)).To(Succeed()) - expectFound(fmt.Sprintf("%s/%s", testServer.URL, "/catalogs/foo.txt"), expectedContent) - }) - It("gets 200 for the path /catalogs/test-catalog/foo.txt", func() { - expectedContent := []byte("bar") - Expect(os.WriteFile(filepath.Join(store.RootDir, "test-catalog", "foo.txt"), expectedContent, 0600)).To(Succeed()) - expectFound(fmt.Sprintf("%s/%s", testServer.URL, "/catalogs/test-catalog/foo.txt"), expectedContent) + It("gets 404 for an unknown catalog", func() { + expectNotFound(fmt.Sprintf("%s/%s", testServer.URL, "/api/vi/unknown-catalog")) }) - It("ignores accept-encoding for the path /catalogs/test-catalog/api/v1/all with size < 1400 bytes", func() { + // It("gets 200 for the path /catalogs/foo.txt", func() { + // expectedContent := []byte("bar") + // Expect(os.WriteFile(filepath.Join(store.RootDir, "foo.txt"), expectedContent, 0600)).To(Succeed()) + // expectFound(fmt.Sprintf("%s/%s", testServer.URL, "/catalogs/foo.txt"), expectedContent) + // }) + // It("gets 200 for the path /catalogs/test-catalog/foo.txt", func() { + // expectedContent := []byte("bar") + // Expect(os.WriteFile(filepath.Join(store.RootDir, "test-catalog", "foo.txt"), expectedContent, 0600)).To(Succeed()) + // expectFound(fmt.Sprintf("%s/%s", testServer.URL, "/catalogs/test-catalog/foo.txt"), expectedContent) + // }) + It("ignores accept-encoding for raw FBC access to test-catalog with size < 1400 bytes", func() { expectedContent := []byte("bar") - Expect(os.WriteFile(filepath.Join(store.RootDir, "test-catalog", "api", "v1", "all"), expectedContent, 0600)).To(Succeed()) - expectFound(fmt.Sprintf("%s/%s", testServer.URL, "/catalogs/test-catalog/api/v1/all"), expectedContent) + Expect(os.WriteFile(filepath.Join(store.RootDir, "test-catalog", rawFileName), expectedContent, 0600)).To(Succeed()) + expectFound(fmt.Sprintf("%s/%s", testServer.URL, "/api/v1/test-catalog"), expectedContent) }) - It("provides gzipped content for the path /catalogs/test-catalog/api/v1/all with size > 1400 bytes", func() { + It("provides gzipped content for raw FBC access to test-catalog with size > 1400 bytes", func() { expectedContent := []byte(testCompressableJSON) - Expect(os.WriteFile(filepath.Join(store.RootDir, "test-catalog", "api", "v1", "all"), expectedContent, 0600)).To(Succeed()) - expectFound(fmt.Sprintf("%s/%s", testServer.URL, "/catalogs/test-catalog/api/v1/all"), expectedContent) + Expect(os.WriteFile(filepath.Join(store.RootDir, "test-catalog", rawFileName), expectedContent, 0600)).To(Succeed()) + expectFound(fmt.Sprintf("%s/%s", testServer.URL, "/api/v1/test-catalog"), expectedContent) }) It("provides json-lines format for the served JSON catalog", func() { catalog := "test-catalog" @@ -165,7 +166,7 @@ var _ = Describe("LocalDir Server Handler tests", func() { expectedContent, err := generateJSONLines([]byte(testCompressableJSON)) Expect(err).To(Not(HaveOccurred())) - expectFound(fmt.Sprintf("%s/%s", testServer.URL, "/catalogs/test-catalog/api/v1/all"), []byte(expectedContent)) + expectFound(fmt.Sprintf("%s/%s", testServer.URL, "/api/v1/test-catalog"), []byte(expectedContent)) }) It("provides json-lines format for the served YAML catalog", func() { catalog := "test-catalog" @@ -179,7 +180,7 @@ var _ = Describe("LocalDir Server Handler tests", func() { expectedContent, err := generateJSONLines(yamlData) Expect(err).To(Not(HaveOccurred())) - expectFound(fmt.Sprintf("%s/%s", testServer.URL, "/catalogs/test-catalog/api/v1/all"), []byte(expectedContent)) + expectFound(fmt.Sprintf("%s/%s", testServer.URL, "/api/v1/test-catalog"), []byte(expectedContent)) }) AfterEach(func() { testServer.Close()