Skip to content

Commit

Permalink
(catalogd) add more unit tests for localdir storage.Instance
Browse files Browse the repository at this point in the history 
Also rename `query` to `metas` in places that were missed by #1703

Signed-off-by: Anik Bhattacharjee <[email protected]>
  • Loading branch information
@anik120
anik120 committed Feb 6, 2025
1 parent dcf50b8 commit 45cdb37
Show file tree
Hide file tree
Showing 4 changed files with 95 additions and 34 deletions.
2 changes: 1 addition & 1 deletion catalogd/cmd/catalogd/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ func main() {
localStorage = &storage.LocalDirV1{
RootDir: storeDir,
RootURL: baseStorageURL,
EnableQueryHandler: features.CatalogdFeatureGate.Enabled(features.APIV1QueryHandler),
EnableMetasHandler: features.CatalogdFeatureGate.Enabled(features.APIV1MetasHandler),
}

// Config for the catalogd web server
Expand Down
4 changes: 2 additions & 2 deletions catalogd/internal/features/features.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ import (
)

const (
APIV1QueryHandler = featuregate.Feature("APIV1QueryHandler")
APIV1MetasHandler = featuregate.Feature("APIV1MetasHandler")
)

var catalogdFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{
APIV1QueryHandler: {Default: false, PreRelease: featuregate.Alpha},
APIV1MetasHandler: {Default: false, PreRelease: featuregate.Alpha},
}

var CatalogdFeatureGate featuregate.MutableFeatureGate = featuregate.NewFeatureGate()
Expand Down
28 changes: 21 additions & 7 deletions catalogd/internal/storage/localdir.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,12 @@ import (
type LocalDirV1 struct {
RootDir string
RootURL *url.URL
EnableQueryHandler bool
EnableMetasHandler bool

m sync.RWMutex
// this singleflight Group is used in `getIndex()`` to handle concurrent HTTP requests
// optimally. With the use of this slightflight group, the index is loaded from disk
// once per concurrent group of HTTP requests being handled by the query handler.
// once per concurrent group of HTTP requests being handled by the metas handler.
// The single flight instance gives us a way to load the index from disk exactly once
// per concurrent group of callers, and then let every concurrent caller have access to
// the loaded index. This avoids lots of unnecessary open/decode/close cycles when concurrent
Expand All @@ -60,7 +60,7 @@ func (s *LocalDirV1) Store(ctx context.Context, catalog string, fsys fs.FS) erro
defer os.RemoveAll(tmpCatalogDir)

storeMetaFuncs := []storeMetasFunc{storeCatalogData}
if s.EnableQueryHandler {
if s.EnableMetasHandler {
storeMetaFuncs = append(storeMetaFuncs, storeIndexData)
}

Expand Down Expand Up @@ -126,7 +126,7 @@ func (s *LocalDirV1) ContentExists(catalog string) bool {
return false
}

if s.EnableQueryHandler {
if s.EnableMetasHandler {
indexFileStat, err := os.Stat(catalogIndexFilePath(s.catalogDir(catalog)))
if err != nil {
return false
Expand Down Expand Up @@ -189,8 +189,8 @@ func (s *LocalDirV1) StorageServerHandler() http.Handler {
mux := http.NewServeMux()

mux.HandleFunc(s.RootURL.JoinPath("{catalog}", "api", "v1", "all").Path, s.handleV1All)
if s.EnableQueryHandler {
mux.HandleFunc(s.RootURL.JoinPath("{catalog}", "api", "v1", "metas").Path, s.handleV1Query)
if s.EnableMetasHandler {
mux.HandleFunc(s.RootURL.JoinPath("{catalog}", "api", "v1", "metas").Path, s.handleV1Metas)
}
allowedMethodsHandler := func(next http.Handler, allowedMethods ...string) http.Handler {
allowedMethodSet := sets.New[string](allowedMethods...)
Expand Down Expand Up @@ -219,10 +219,24 @@ func (s *LocalDirV1) handleV1All(w http.ResponseWriter, r *http.Request) {
http.ServeContent(w, r, "", catalogStat.ModTime(), catalogFile)
}

func (s *LocalDirV1) handleV1Query(w http.ResponseWriter, r *http.Request) {
func (s *LocalDirV1) handleV1Metas(w http.ResponseWriter, r *http.Request) {
s.m.RLock()
defer s.m.RUnlock()

// Check for unexpected query parameters
expectedParams := map[string]bool{
"schema": true,
"package": true,
"name": true,
}

for param := range r.URL.Query() {
if !expectedParams[param] {
httpError(w, errInvalidParams)
return
}
}

catalog := r.PathValue("catalog")
catalogFile, catalogStat, err := s.catalogData(catalog)
if err != nil {
Expand Down
95 changes: 71 additions & 24 deletions catalogd/internal/storage/localdir_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,11 @@ func TestLocalDirStoraget(t *testing.T) {
},
},
{
name: "storing with query handler enabled should create indexes",
name: "storing with metas handler enabled should create indices",
setup: func(t *testing.T) (*LocalDirV1, fs.FS) {
s := &LocalDirV1{
RootDir: t.TempDir(),
EnableQueryHandler: true,
EnableMetasHandler: true,
}
return s, createTestFS(t)
},
Expand Down Expand Up @@ -106,7 +106,7 @@ func TestLocalDirStoraget(t *testing.T) {
for i := 0; i < 10; i++ {
wg.Add(1)
go func() {
defer wg.Add(-1)
defer wg.Done()
for j := 0; j < 100; j++ {
s.ContentExists(catalog)
}
Expand Down Expand Up @@ -266,13 +266,13 @@ func TestLocalDirServerHandler(t *testing.T) {
}
}

// Tests to verify the behavior of the query endpoint, as described in
// https://docs.google.com/document/d/1s6_9IFEKGQLNh3ueH7SF4Yrx4PW9NSiNFqFIJx0pU-8/edit?usp=sharing
func TestQueryEndpoint(t *testing.T) {
// Tests to verify the behavior of the metas endpoint, as described in
// https://docs.google.com/document/d/1s6_9IFEKGQLNh3ueH7SF4Yrx4PW9NSiNFqFIJx0pU-8/
func TestMetasEndpoint(t *testing.T) {
store := &LocalDirV1{
RootDir: t.TempDir(),
RootURL: &url.URL{Path: urlPrefix},
EnableQueryHandler: true,
EnableMetasHandler: true,
}
if store.Store(context.Background(), "test-catalog", createTestFS(t)) != nil {
t.Fatal("failed to store test catalog")
Expand All @@ -281,7 +281,7 @@ func TestQueryEndpoint(t *testing.T) {

testCases := []struct {
name string
setupStore func() (*httptest.Server, error)
initRequest func(req *http.Request) error
queryParams string
expectedStatusCode int
expectedContent string
Expand Down Expand Up @@ -329,27 +329,50 @@ func TestQueryEndpoint(t *testing.T) {
expectedContent: "",
},
{
name: "cached response with If-Modified-Since",
queryParams: "?schema=olm.package",
name: "valid query with packageName that returns multiple blobs",
queryParams: "?package=webhook_operator_test",
expectedStatusCode: http.StatusOK,
expectedContent: `{"image":"quaydock.io/namespace/bundle:0.0.3","name":"bundle.v0.0.1","package":"webhook_operator_test","properties":[{"type":"olm.bundle.object","value":{"data":"dW5pbXBvcnRhbnQK"}},{"type":"some.other","value":{"data":"arbitrary-info"}}],"relatedImages":[{"image":"testimage:latest","name":"test"}],"schema":"olm.bundle"}
{"entries":[{"name":"bundle.v0.0.1"}],"name":"preview_test","package":"webhook_operator_test","schema":"olm.channel"}`,
},
{
name: "cached response with If-Modified-Since",
queryParams: "?schema=olm.package",
initRequest: func(req *http.Request) error {
resp, err := http.DefaultClient.Do(req)
if err != nil {
return err
}
resp.Body.Close()
req.Header.Set("If-Modified-Since", resp.Header.Get("Last-Modified"))
return nil
},
expectedStatusCode: http.StatusNotModified,
expectedContent: "",
},
{
name: "request with unknown parameters",
queryParams: "?non-existent=foo",
expectedStatusCode: http.StatusBadRequest,
expectedContent: "400 Bad Request",
},
{
name: "request with duplicate parameters",
queryParams: "?schema=olm.bundle&&schema=olm.bundle",
expectedStatusCode: http.StatusOK,
expectedContent: `{"image":"quaydock.io/namespace/bundle:0.0.3","name":"bundle.v0.0.1","package":"webhook_operator_test","properties":[{"type":"olm.bundle.object","value":{"data":"dW5pbXBvcnRhbnQK"}},{"type":"some.other","value":{"data":"arbitrary-info"}}],"relatedImages":[{"image":"testimage:latest","name":"test"}],"schema":"olm.bundle"}`,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("%s/catalogs/test-catalog/api/v1/metas%s", testServer.URL, tc.queryParams), nil)
reqGet, err := http.NewRequest(http.MethodGet, fmt.Sprintf("%s/catalogs/test-catalog/api/v1/metas%s", testServer.URL, tc.queryParams), nil)
require.NoError(t, err)

if strings.Contains(tc.name, "If-Modified-Since") {
// Do an initial request to get a Last-Modified timestamp
// for the actual request
resp, err := http.DefaultClient.Do(req)
require.NoError(t, err)
resp.Body.Close()
req.Header.Set("If-Modified-Since", resp.Header.Get("Last-Modified"))
if tc.initRequest != nil {
require.NoError(t, tc.initRequest(reqGet))
}
resp, err := http.DefaultClient.Do(req)
resp, err := http.DefaultClient.Do(reqGet)
require.NoError(t, err)
defer resp.Body.Close()

Expand All @@ -358,6 +381,30 @@ func TestQueryEndpoint(t *testing.T) {
actualContent, err := io.ReadAll(resp.Body)
require.NoError(t, err)
require.Equal(t, tc.expectedContent, strings.TrimSpace(string(actualContent)))

// Also do a HEAD request
reqHead, err := http.NewRequest(http.MethodHead, fmt.Sprintf("%s/catalogs/test-catalog/api/v1/metas%s", testServer.URL, tc.queryParams), nil)
require.NoError(t, err)
if tc.initRequest != nil {
require.NoError(t, tc.initRequest(reqHead))
}
resp, err = http.DefaultClient.Do(reqHead)
require.NoError(t, err)
require.Equal(t, tc.expectedStatusCode, resp.StatusCode)
actualContent, err = io.ReadAll(resp.Body)
require.NoError(t, err)
require.Equal(t, "", string(actualContent)) // HEAD should not return a body
resp.Body.Close()

// And make sure any other method is not allowed
for _, method := range []string{http.MethodPost, http.MethodPut, http.MethodDelete} {
reqPost, err := http.NewRequest(method, fmt.Sprintf("%s/catalogs/test-catalog/api/v1/metas%s", testServer.URL, tc.queryParams), nil)
require.NoError(t, err)
resp, err = http.DefaultClient.Do(reqPost)
require.NoError(t, err)
require.Equal(t, http.StatusMethodNotAllowed, resp.StatusCode)
resp.Body.Close()
}
})
}
}
Expand All @@ -366,7 +413,7 @@ func TestServerLoadHandling(t *testing.T) {
store := &LocalDirV1{
RootDir: t.TempDir(),
RootURL: &url.URL{Path: urlPrefix},
EnableQueryHandler: true,
EnableMetasHandler: true,
}

// Create large test data
Expand Down Expand Up @@ -443,20 +490,20 @@ func TestServerLoadHandling(t *testing.T) {
},
},
{
name: "mixed all and query endpoints",
name: "mixed all and metas endpoints",
concurrent: 40,
requests: func(baseURL string) []*http.Request {
var reqs []*http.Request
for i := 0; i < 20; i++ {
allReq, _ := http.NewRequest(http.MethodGet,
fmt.Sprintf("%s/catalogs/test-catalog/api/v1/all", baseURL),
nil)
queryReq, _ := http.NewRequest(http.MethodGet,
metasReq, _ := http.NewRequest(http.MethodGet,
fmt.Sprintf("%s/catalogs/test-catalog/api/v1/metas?schema=olm.bundle", baseURL),
nil)
allReq.Header.Set("Accept", "application/jsonl")
queryReq.Header.Set("Accept", "application/jsonl")
reqs = append(reqs, allReq, queryReq)
metasReq.Header.Set("Accept", "application/jsonl")
reqs = append(reqs, allReq, metasReq)
}
return reqs
},
Expand Down

0 comments on commit 45cdb37

Please sign in to comment.