⚠ (feat) Introduce new feature-gated metas endpoint #1643

anik120 · 2025-01-21T20:31:20Z

Description

Implementation of Metas Endpoint API RFC

Adds a new feature-gated endpoint, that can be enabled by passing the flag --feature-gates=APIV1MetasHandler=true to the binary
Once the feature gate is enabled, separate indexes are created while storing unpacked data from a catalog image, besides the existing all data.
Query endpoint can be queried at <base.url>/api/vi/metas? for each catalog
Server uses the indexes created previously to serve the metas endpoint to send a filtered list of FBC meta blobs.

Note: Some renaming from query -> metas done in #1713

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2025-01-21T20:31:38Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`5b46ad1`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/67a13659ef6b9e0008fef162
😎 Deploy Preview	https://deploy-preview-1643--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2025-01-21T20:51:52Z

Codecov Report

Attention: Patch coverage is 62.44131% with 160 lines in your changes missing coverage. Please review.

Project coverage is 67.48%. Comparing base (f77ebfa) to head (5b46ad1).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...talogd/internal/storage/http_precoditions_check.go	41.91%	76 Missing and 3 partials ⚠️
catalogd/internal/storage/localdir.go	70.35%	42 Missing and 17 partials ⚠️
catalogd/internal/storage/index.go	80.51%	14 Missing and 1 partial ⚠️
catalogd/cmd/catalogd/main.go	0.00%	6 Missing ⚠️
catalogd/internal/serverutil/serverutil.go	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1643      +/-   ##
==========================================
- Coverage   67.74%   67.48%   -0.27%     
==========================================
  Files          57       59       +2     
  Lines        4620     4998     +378     
==========================================
+ Hits         3130     3373     +243     
- Misses       1265     1378     +113     
- Partials      225      247      +22

Flag	Coverage Δ
e2e	`53.35% <ø> (-0.09%)`	⬇️
unit	`55.24% <62.44%> (+0.73%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

catalogd/internal/storage/multireadseeker.go

catalogd/internal/serverutil/serverutil.go

catalogd/internal/serverutil/serverutil_tests.go

azych · 2025-01-22T08:47:28Z

What's the relation of this PR to #1642?
This one seems to include two additional commits, with the first 4 being the same.

Which PR is a duplicate and which should be reviewed?
If this is the one to review (and also not a Draft), can you add a description of the main changes this introduces?

anik120 · 2025-01-22T21:31:51Z

What's the relation of this PR to #1642? This one seems to include two additional commits, with the first 4 being the same.

Which PR is a duplicate and which should be reviewed? If this is the one to review (and also not a Draft), can you add a description of the main changes this introduces?

@azych apologies for the confusion. You're just seeing the result of rapid experimentation. This is the one to review, but it's still a WIP. I've converted this PR to a draft.

catalogd/internal/storage/localdir_test.go

joelanford · 2025-01-28T20:57:55Z

catalogd/internal/storage/localdir.go

+		httpError(w, fs.ErrNotExist)
+		return


This should change to write a 200 status and write no content to the response body.

I noticed that index.Get() was only ever returning true (so this code path was never actually being hit). Something else solved the test case failure though, but I went ahead and cleaned up Get()'s signature 53692a9

catalogd/internal/storage/localdir.go

joelanford · 2025-01-28T21:00:59Z

catalogd/internal/storage/localdir.go

+		storeMetaFuncs = append(storeMetaFuncs, storeIndexData)
+	}
+
+	var (


Throughout these lines where we setup and run concurrent channel writers and readers, we should add some comments to explain what is happening.

catalogd/internal/storage/localdir.go

catalogd/internal/serverutil/serverutil_test.go

catalogd/internal/storage/index.go

joelanford · 2025-01-28T21:56:36Z

catalogd/internal/storage/index.go

+	return newMultiReadSeeker(srs...), true
+}
+
+func (i *index) getSectionSet(schema, packageName, name string) sets.Set[section] {


One thing that occurs to me is that the indexed sections are always guaranteed to be sorted by the offset. We could likely take advantage of that fact and implement a more performant algorithm that can step directly through each slice rather than building a set for each slice, performing a set intersection, the converting back into a slice and resorting.

But this is something we can capture as a follow-up.

grokspawn

All follow ups are being tracked in other issues to try not to distract this effort any further.

joelanford · 2025-02-03T21:51:48Z

catalogd/internal/serverutil/serverutil.go

@@ -97,3 +93,12 @@ func logrLoggingHandler(l logr.Logger, handler http.Handler) http.Handler {
 		l.Info("handled request", "host", host, "username", username, "method", params.Request.Method, "uri", uri, "protocol", params.Request.Proto, "status", params.StatusCode, "size", params.Size)
 	})
 }
+
+func storageServerHandlerWrapped(l logr.Logger, cfg CatalogServerConfig) http.Handler {


Nit (follow-up): We could probably change CatalogServerConfig to have an http.Handler field instead of the entire LocalStorage field?

catalogd/internal/storage/http_precoditions_check.go

joelanford · 2025-02-03T21:59:32Z

catalogd/internal/storage/localdir.go

+	defer s.m.RUnlock()
+
+	catalog := r.PathValue("catalog")
+	catalogFile, catalogStat, err := s.catalogData(catalog)


Need to defer catalogFile.Close() here.

Even better, now that we've got separate handling for all and query, maybe we go all the way to simply http.ServeFile?

joelanford · 2025-02-03T22:02:25Z

catalogd/internal/storage/localdir.go

+	case errors.Is(err, errInvalidParams):
+		code = http.StatusBadRequest


I don't think I see any where that we call httpError with errInvalidParams. Seems we should add a check in the query handler for unexpected parameters?

catalogd/internal/storage/localdir.go

joelanford · 2025-02-03T22:07:08Z

catalogd/internal/storage/localdir_test.go

+				for i := 0; i < 10; i++ {
+					wg.Add(1)
+					go func() {
+						defer wg.Add(-1)


Suggested change

defer wg.Add(-1)

defer wg.Done()

joelanford · 2025-02-03T22:10:49Z

catalogd/internal/storage/localdir_test.go

+	testCases := []struct {
+		name               string
+		setupStore         func() (*httptest.Server, error)
+		queryParams        string


Nit: might be nicer to use url.Values for more structure setup of the query params.

joelanford · 2025-02-03T22:13:54Z

catalogd/internal/storage/localdir_test.go

+		{
+			name:               "valid query with package schema",
+			queryParams:        "?schema=olm.package",
+			expectedStatusCode: http.StatusOK,
+			expectedContent:    `{"defaultChannel":"preview_test","name":"webhook_operator_test","schema":"olm.package"}`,
+		},


I don't see any test that verifies that a query that matches multiple blobs returns all the blobs. We should add variations of that for each of schema, package, and name.

joelanford · 2025-02-03T22:17:28Z

catalogd/internal/storage/localdir_test.go

+			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"))
+			}


We shouldn't base test logic on the content of a free-form test name. But maybe it makes sense to do this for every test case, where each test case is first sent without the header, and then resent with the header?

Not a fan of what I did here either, but for the lack of a better way of doing it I just went with string matching.

Now that I am thinking about it again with a clearer head, I think I can do better: have a "setup" func for each test, and only do a "pre-call" for this test in the setup function. I'll add this in the follow up.

joelanford · 2025-02-03T22:19:07Z

catalogd/internal/storage/localdir_test.go

+
+	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/query%s", testServer.URL, tc.queryParams), nil)


We should parameters the http method and test HEAD (which should work, but with no response body), as well as POST and DELETE, and PUT which should all fail with the unsupported method status code.

joelanford · 2025-02-03T22:21:20Z

catalogd/internal/storage/localdir_test.go

+			queryParams:        "?schema=olm.package",
+			expectedStatusCode: http.StatusNotModified,
+			expectedContent:    "",
+		},


Add another test cases with an unknown parameter, expected to return BadRequest

And another when we get duplicate parameters? (i.e. 'query?package=foo&package=foo')

* refer: 1. [Metas Endpoint RFC] (https://docs.google.com/document/d/1s6_9IFEKGQLNh3ueH7SF4Yrx4PW9NSiNFqFIJx0pU-8/edit?usp=sharing) 2. [operator-framework/operator-controller-1643](operator-framework/operator-controller#1643) 3. [operator-framework/operator-controller-1713](operator-framework/operator-controller@45cdb37#diff-03c0636b035013a21712c5f4f04a30ae71ef533c9aa4a6d0724dc5575bf0dffdR9) * Required a vendor update to pull in [openshift/api-2202](https://github.com/openshift/api/pull/2202/files#diff-a8b6135d50534471326ea7bcd20e0f5eae25353f7788338060f718128a6a0b34R525) $ go get github.com/openshift/api@744790f2cff777b1bb29bdccce10e4e84cff0a69 $ go mod tidy $ go mod vendor Also needed $ go get openshift/client-go@f7ec47e to resolve vendoring inconsistency.

anik120 requested a review from a team as a code owner January 21, 2025 20:31

anik120 force-pushed the query-enpoint branch from 1a96032 to a27fada Compare January 21, 2025 20:34

joelanford reviewed Jan 21, 2025

View reviewed changes

catalogd/internal/storage/multireadseeker.go Outdated Show resolved Hide resolved

anik120 force-pushed the query-enpoint branch 2 times, most recently from 17e0f9a to 729645c Compare January 21, 2025 21:57

joelanford reviewed Jan 21, 2025

View reviewed changes

catalogd/internal/serverutil/serverutil.go Outdated Show resolved Hide resolved

joelanford reviewed Jan 21, 2025

View reviewed changes

catalogd/internal/serverutil/serverutil_tests.go Outdated Show resolved Hide resolved

joelanford reviewed Jan 21, 2025

View reviewed changes

catalogd/internal/serverutil/serverutil_tests.go Outdated Show resolved Hide resolved

anik120 force-pushed the query-enpoint branch from 729645c to 478c152 Compare January 22, 2025 21:24

anik120 marked this pull request as draft January 22, 2025 21:30

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 22, 2025

anik120 mentioned this pull request Jan 22, 2025

Query api #1642

Closed

4 tasks

joelanford reviewed Jan 22, 2025

View reviewed changes

catalogd/internal/storage/localdir_test.go Outdated Show resolved Hide resolved

grokspawn changed the title ~~Query endpoint~~ ✨ Query endpoint Jan 23, 2025

anik120 force-pushed the query-enpoint branch 4 times, most recently from d8dcf61 to f94eb2e Compare January 24, 2025 20:16

joelanford reviewed Jan 28, 2025

View reviewed changes

catalogd/internal/storage/localdir_test.go Outdated Show resolved Hide resolved

joelanford reviewed Jan 28, 2025

View reviewed changes

catalogd/internal/storage/localdir.go Show resolved Hide resolved

joelanford reviewed Jan 28, 2025

View reviewed changes

catalogd/internal/storage/localdir.go Outdated Show resolved Hide resolved

joelanford reviewed Jan 28, 2025

View reviewed changes

catalogd/internal/serverutil/serverutil_test.go Outdated Show resolved Hide resolved

joelanford reviewed Jan 28, 2025

View reviewed changes

catalogd/internal/storage/index.go Show resolved Hide resolved

joelanford reviewed Jan 28, 2025

View reviewed changes

anik120 added 5 commits February 3, 2025 15:12

cleanup index.Get's signature

ee38cbe

only allow GET/HEAD methods

f8b7a64

refractor serverJSONLines

55333ba

replace static test variable

6ae66cd

fix unit test

870f38f

anik120 force-pushed the query-enpoint branch from d76a738 to 870f38f Compare February 3, 2025 20:35

refractor getIndex() (and other lint issues)

5b46ad1

grokspawn enabled auto-merge February 3, 2025 21:38

grokspawn approved these changes Feb 3, 2025

View reviewed changes

grokspawn added this pull request to the merge queue Feb 3, 2025

joelanford reviewed Feb 3, 2025

View reviewed changes

catalogd/internal/storage/http_precoditions_check.go Show resolved Hide resolved

joelanford reviewed Feb 3, 2025

View reviewed changes

Merged via the queue into operator-framework:main with commit 068fd48 Feb 3, 2025
21 of 23 checks passed

joelanford reviewed Feb 3, 2025

View reviewed changes

catalogd/internal/storage/localdir.go Show resolved Hide resolved

joelanford reviewed Feb 3, 2025

View reviewed changes

This was referenced Feb 4, 2025

implementing feature-gated, caching catalogd web query API #1603

Closed

serve catalog content based on supplied parameters #1606

Closed

anik120 changed the title ~~⚠ (feat) Introduce new feature-gated query endpoint~~ ⚠ (feat) Introduce new feature-gated metas endpoint Feb 20, 2025

anik120 mentioned this pull request Feb 20, 2025

OPRUN-3690: Watch for new upstream feature gate APIV1MetasHandler openshift/cluster-olm-operator#104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠ (feat) Introduce new feature-gated metas endpoint #1643

⚠ (feat) Introduce new feature-gated metas endpoint #1643

anik120 commented Jan 21, 2025 •

edited

Loading

netlify bot commented Jan 21, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

azych commented Jan 22, 2025

anik120 commented Jan 22, 2025

joelanford Jan 28, 2025

anik120 Jan 31, 2025

joelanford Jan 28, 2025

joelanford Jan 28, 2025

grokspawn left a comment

joelanford Feb 3, 2025

joelanford Feb 3, 2025

joelanford Feb 3, 2025

joelanford Feb 3, 2025

joelanford Feb 3, 2025

joelanford Feb 3, 2025 •

edited

Loading

joelanford Feb 3, 2025

anik120 Feb 3, 2025

joelanford Feb 3, 2025

joelanford Feb 3, 2025

grokspawn Feb 3, 2025

		case errors.Is(err, errInvalidParams):
		code = http.StatusBadRequest

⚠ (feat) Introduce new feature-gated metas endpoint #1643

⚠ (feat) Introduce new feature-gated metas endpoint #1643

Conversation

anik120 commented Jan 21, 2025 • edited Loading

Description

Reviewer Checklist

netlify bot commented Jan 21, 2025 • edited Loading

✅ Deploy Preview for olmv1 ready!

codecov bot commented Jan 21, 2025 • edited Loading

Codecov Report

azych commented Jan 22, 2025

anik120 commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grokspawn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anik120 commented Jan 21, 2025 •

edited

Loading

netlify bot commented Jan 21, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

joelanford Feb 3, 2025 •

edited

Loading