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.

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

🌱 (catalogd) add more unit tests for localdir storage.Instance #1713

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion catalogd/cmd/catalogd/main.go
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
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
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
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
Loading