Serve locally stored fbc content via an http server #148

anik120 · 2023-08-28T21:02:05Z

Closes #113

anik120 · 2023-08-28T21:02:24Z

@rashmigottipati @everettraven fyi

codecov · 2023-08-28T21:05:54Z

Codecov Report

Patch coverage: 78.57% and project coverage change: -0.04% ⚠️

Comparison is base (d30f161) 79.10% compared to head (d687d3a) 79.06%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
- Coverage   79.10%   79.06%   -0.04%     
==========================================
  Files           3        3              
  Lines         201      215      +14     
==========================================
+ Hits          159      170      +11     
- Misses         26       28       +2     
- Partials       16       17       +1

Files Changed	Coverage Δ
pkg/storage/localdir.go	`64.51% <78.57%> (+11.57%)`	⬆️

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

pkg/server/server.go

cmd/manager/main.go

pkg/server/server.go

cmd/manager/main.go

pkg/server/server.go

cmd/manager/main.go

everettraven · 2023-09-06T18:43:36Z

pkg/storage/localdir.go

@@ -45,3 +48,26 @@ func (s LocalDir) Store(catalog string, fsys fs.FS) error {
 func (s LocalDir) Delete(catalog string) error {
 	return os.RemoveAll(filepath.Join(s.RootDir, catalog))
 }
+
+func (s LocalDir) StorageServerHandler() http.Handler {
+	return http.StripPrefix(s.URL.Path, http.FileServer(http.FS(&filesOnlyFilesystem{os.DirFS(s.RootDir)})))


Instead of calling os.DirFS() can we pass a fs.FS as a function parameter?

I assume you're asking this for mocking purposes?

LocalDir needs to write files into the OS filesystem at RootDir and the whole point of moving this handler method into the LocalDir struct is to share that context to make it easier to keep things aligned.

At best, we would need some sort of writable FS interface for RootDir.

Doable, but what value does it bring, if not for unit tests (made a comment about unit tests below)? If we can think of a valuable unit test this'll need to be done anyway.

I assume you're asking this for mocking purposes?

Mocking the filesystem to test the handler implementation to be more precise, but yes.

LocalDir needs to write files into the OS filesystem at RootDir and the whole point of moving this handler method into the LocalDir struct is to share that context to make it easier to keep things aligned

I think this is fair, but I feel we could probably do better than a call to os.DirFS() to make it easier to test. That being said, I do think we could still create a temporary testing directory for unit testing purposes and inject it here so not a huge deal if we don't want to go into the effort of not using a direct os.DirFS() call.

At best, we would need some sort of writable FS interface for RootDir

I do wish the standard fs package had some way of having a writable FS interface, but if we wanted to completely abstract the use of the os package this would be needed.

If we can think of a valuable unit test this'll need to be done anyway.

Unit tests for the http server:

/catalogs returns a 404

/catalogs/<catalogName> returns a 404 (even if the directory exists)

/catalogs/<catalogName>/all.json returns 200 and expected JSON content

could also add unit tests specifically for the filesOnlyFilesystem but I think that would be covered in the above tests

For fs interfaces, please use afero if the stock fs.FS doesn't meet your need.

pkg/controllers/core/catalog_controller_test.go

joelanford · 2023-09-06T19:09:23Z

test/e2e/e2e_suite_test.go

+	Expect(corev1.AddToScheme(scheme)).To(Succeed())
+	Expect(rbacv1.AddToScheme(scheme)).To(Succeed())
+	Expect(batchv1.AddToScheme(scheme)).To(Succeed())


It seems like we should use the default scheme that includes these built-in types, rather than starting with an empty scheme and having to add built-in types one-by-one.

https://pkg.go.dev/k8s.io/[email protected]/kubernetes/scheme#Scheme

We do start with that, looks like it's the same in rukpak?

You do need to add non-core types to the runtime.Scheme().

Yes, I'm suggesting starting with scheme.Scheme instead of runtime.NewScheme(), because the former has all of the built-in types already added. Then all you have to worry about is adding the non-core types.

(still adding to scheme)

joelanford · 2023-09-06T19:13:42Z

test/e2e/unpack_test.go

+				buf := new(bytes.Buffer)
+				_, err = buf.ReadFrom(logReader)
+				Expect(err).ToNot(HaveOccurred())
+				cfg, err := declcfg.LoadReader(buf)


Any reason not to just do a string comparison of the raw FBC json?

If we stick with LoadReader, there's no need for the intermediate buffer. Just pass logReader directly into declcfg.LoadReader

Reason to not to string comparison is how the information is laid out, ie lack of ordering guarantee.

Go's JSON encoder has ordering guarantees for field names. The previous code in operator-registry would sort lists as well before sending them over the wire. Why wouldn't we want to add deterministic output to the set of expectations from the server?

lack of ordering guarantee

Can you explain further? The all.json file is literally a file on disk and the written into the http response body. I don't see any possibility of non-determinism in the response.

test/e2e/unpack_test.go

config/default/manager_auth_proxy_patch.yaml

config/rbac/catalog_reader_client_clusterrole.yaml

cmd/manager/main.go

pkg/storage/localdir.go

test/e2e/unpack_test.go

stevekuznetsov · 2023-09-06T19:26:42Z

test/e2e/unpack_test.go

+			}
+			rawMessage, err := json.Marshal(value{PackageName: "prometheus", Version: "0.47.0"})
+			Expect(err).To(Not(HaveOccurred()))
+			expectedDeclCfg := declcfg.DeclarativeConfig{


Can we use a golden fixture on disk and have an UPDATE=true mode for tests to simply update the fixture?

I have no idea what you mean exactly.

However while trying to decipher your statement I realized that I should probably not make a config by hand here, and instead read the file from the test folder. If that's what you meant by your comment then +1

Yep - sorry for the nomenclature. "golden fixture" just means a file on disk in the repo that has the desired state. We check against it with a simple diff and have a helper that, instead of checking and failing on a diff, updates the file, to facilitate easier development in the future.

test/e2e/unpack_test.go

anik120 · 2023-09-07T17:48:11Z

@joelanford @everettraven @stevekuznetsov Latest update switches to using a separate server than the metrics server.

Currently in progress: refactoring server related code in Storage to make it more unit test-able + unit tests. And the two unresolved comments from above.

Pushing what I have till now to see what we can do as follow up to make it more iterative, since current sprint commitment is at risk:

Work blocked on this PR:

#118
#127

(note again that I am working on the unit tests related refactoring + unit tests in the background)

config/rbac/auth_proxy_service.yaml

cmd/manager/main.go

config/rbac/auth_proxy_service.yaml

everettraven

Approving pending green e2e

test/e2e/unpack_test.go

Closes operator-framework#113 Signed-off-by: Anik <[email protected]>

anik120 requested a review from a team as a code owner August 28, 2023 21:02

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 Aug 28, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2023

everettraven mentioned this pull request Aug 31, 2023

Add metrics to the catalog server #156

Merged

everettraven reviewed Aug 31, 2023

View reviewed changes

pkg/server/server.go Outdated Show resolved Hide resolved

everettraven mentioned this pull request Aug 31, 2023

Serve locally stored fbc content via a server #113

Closed

joelanford reviewed Aug 31, 2023

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

everettraven reviewed Aug 31, 2023

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

joelanford reviewed Aug 31, 2023

View reviewed changes

pkg/server/server.go Outdated Show resolved Hide resolved

ncdc reviewed Sep 6, 2023

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

cmd/manager/main.go Outdated Show resolved Hide resolved

pkg/server/server.go Outdated Show resolved Hide resolved

anik120 force-pushed the server branch from 9fd459f to 09e7c67 Compare September 6, 2023 17:43

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 6, 2023

anik120 changed the title ~~WIP: Server~~ WIP: Serve locally stored fbc content via an http serve Sep 6, 2023

anik120 force-pushed the server branch 3 times, most recently from fe2259c to be7f1dc Compare September 6, 2023 18:18

ncdc changed the title ~~WIP: Serve locally stored fbc content via an http serve~~ WIP: Serve locally stored fbc content via an http server Sep 6, 2023

anik120 changed the title ~~WIP: Serve locally stored fbc content via an http server~~ Serve locally stored fbc content via an http server Sep 6, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 6, 2023

everettraven reviewed Sep 6, 2023

View reviewed changes