-
Notifications
You must be signed in to change notification settings - Fork 27
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
Store(local dir) and Serve(http server) FBC #135
Conversation
Codecov Report
@@ Coverage Diff @@
## main #135 +/- ##
==========================================
- Coverage 78.01% 76.94% -1.07%
==========================================
Files 2 2
Lines 282 295 +13
==========================================
+ Hits 220 227 +7
- Misses 39 42 +3
- Partials 23 26 +3
|
afffbe6
to
1c21eb3
Compare
@ncdc it's a WIP PR 😄 (I didn't think anyone had notifications on for new PRs for this repo yet, hence the skipping of the WIP tag 😮💨 ) |
Sorry, saw the notification come through so I started looking at it. |
@ncdc fyi this is just copy paste of https://github.com/operator-framework/rukpak/blob/main/pkg/storage/localdir.go (I was using this for a private conversation), but it looks like we should look into auditing this in rukpak based off of your suggestions |
046fd14
to
a9c9482
Compare
7610d7c
to
ad474b3
Compare
ad474b3
to
baeb9be
Compare
3496d8a
to
4c0f6b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between the server and the storage? Why do we need two separate concepts as opposed to one?
c512ed8
to
bddb98c
Compare
@@ -50,5 +50,5 @@ spec: | |||
- "--health-probe-bind-address=:8081" | |||
- "--metrics-bind-address=127.0.0.1:8080" | |||
- "--leader-elect" | |||
- "--feature-gates=PackagesBundleMetadataAPIs=true,CatalogMetadataAPI=true" | |||
- "--feature-gates=PackagesBundleMetadataAPIs=true,CatalogMetadataAPI=true,HTTPServer=false" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want the http server disabled by default? I figure we probably want all of them enabled so we can start the deprecation/removal process after a new release with these changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still thinking this should probably be set to true by default so that all of the serving methods are available by default in the next release and we can remove the Package
, BundleMetadata
, and CatalogMetadata
in the next+1 release. That being said that can be done in a follow up if we want and shouldn't block this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we even have to wait for the next +1 release. We should be fine just removing those, and switching this to true. But yea let's leave that for a follow up
if features.CatalogdFeatureGate.Enabled(features.HTTPServer) && existingCatsrc.DeletionTimestamp.IsZero() && !controllerutil.ContainsFinalizer(&existingCatsrc, fbcDeletionFinalizer) { | ||
controllerutil.AddFinalizer(&existingCatsrc, fbcDeletionFinalizer) | ||
if err := r.Update(ctx, &existingCatsrc); err != nil { | ||
return ctrl.Result{}, err | ||
} | ||
} | ||
if features.CatalogdFeatureGate.Enabled(features.HTTPServer) && !existingCatsrc.DeletionTimestamp.IsZero() && controllerutil.ContainsFinalizer(&existingCatsrc, fbcDeletionFinalizer) { | ||
if err := r.Storage.Delete(existingCatsrc.Name); err != nil { | ||
return ctrl.Result{}, err | ||
} | ||
controllerutil.RemoveFinalizer(&existingCatsrc, fbcDeletionFinalizer) | ||
err := r.Update(ctx, &existingCatsrc) | ||
return ctrl.Result{}, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Based on our current code structure, would this logic be better suited in the reconcile()
function below? IIUC any changes to the finalizer will be reflected when we issue the update requests at the end of this function. For consistency I would prefer we do our finalizer update logic where we also do the status updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to actually add the finalizer to the object before storing anything into the storage directory. Here's the problematic scenario:
- Lookup and get a copy of the object we're reconciling
- Add a finalizer to the copy (without actually updating it in etcd)
- Store the catalog data
- Send the object update, but get a failure on the update
Now, we've stored the data, but a finalizer isn't present to make sure it gets deleted when the object is deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But +1 on moving the finalizer logic to reconcile()
.
What we could do there is add the finalizer to the copy, but then just return immediately when we've added the finalizer, and let the existing diff-ing update logic here Reconcile
handle the actual update call.
|
||
if features.CatalogdFeatureGate.Enabled(features.HTTPServer) && existingCatsrc.DeletionTimestamp.IsZero() && !controllerutil.ContainsFinalizer(&existingCatsrc, fbcDeletionFinalizer) { | ||
controllerutil.AddFinalizer(&existingCatsrc, fbcDeletionFinalizer) | ||
if err := r.Update(ctx, &existingCatsrc); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's almost always best to use Patch instead of Update. When using Update:
- If the running code has an older version of the go struct definition, and
- The CRD in the cluster contains new fields, and
- There are CRs that have the new fields populated, then
- When you issue an Update using the older client, the result is the new fields will be nulled out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably orthogonal to this PR, given we already have some generic diff-then-update logic in the Reconcile()
method. I propose we make a separate issue to change the existing Update
calls to Patch
.
} | ||
if features.CatalogdFeatureGate.Enabled(features.HTTPServer) && !existingCatsrc.DeletionTimestamp.IsZero() && controllerutil.ContainsFinalizer(&existingCatsrc, fbcDeletionFinalizer) { | ||
if err := r.Storage.Delete(existingCatsrc.Name); err != nil { | ||
return ctrl.Result{}, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should set a condition on the Catalog here
return ctrl.Result{}, err | ||
} | ||
controllerutil.RemoveFinalizer(&existingCatsrc, fbcDeletionFinalizer) | ||
err := r.Update(ctx, &existingCatsrc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto re patch
|
||
if features.CatalogdFeatureGate.Enabled(features.HTTPServer) { | ||
if err := r.Storage.Store(catalog.Name, fbc); err != nil { | ||
return ctrl.Result{}, updateStatusUnpackFailing(&catalog.Status, fmt.Errorf("error storing fbc: %v", err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should set a condition on the Catalog here
pkg/storage/storage.go
Outdated
} | ||
} | ||
|
||
func (s *Storage) Store(owner string, fbc *declcfg.DeclarativeConfig) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other thing we glossed over - when a catalog is updated, we will call Store()
again with the same name as before. I think the most seamless behavior here is:
- write contents to a new file
- move new file to the location of the old one
- don't touch the HTTP handlers
This will ensure zero-downtime for the callers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've captured this as a follow up item. We need an entire story for handling updates to the content in the remote registry (ie what we currently call the polling strategy in v0), and for handling changes to the spec.image field of the CR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the controller not handle update events for CatalogSource
objects? Is there no place where this can slot in already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. It's unimplemented. So far what we have is "creation and deletion once". We haven't gotten to updates at all.
obsoletes operator-framework#113 Signed-off-by: Anik <anikbhattacharya93@gmail.com>
Signed-off-by: Anik <anikbhattacharya93@gmail.com>
Signed-off-by: Anik <anikbhattacharya93@gmail.com>
Signed-off-by: Anik <anikbhattacharya93@gmail.com>
Signed-off-by: Anik <anikbhattacharya93@gmail.com>
Signed-off-by: Anik <anikbhattacharya93@gmail.com>
6e71f82
to
4e304f9
Compare
Signed-off-by: Anik <anikbhattacharya93@gmail.com>
Signed-off-by: Anik <anikbhattacharya93@gmail.com>
Expect(res).To(Equal(ctrl.Result{})) | ||
Expect(err).ToNot(HaveOccurred()) | ||
}) | ||
It("the catalog should become available at addr/catalogs", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be done in a follow up, but should we also validate that the returned JSON stream from the catalog being stored matches what we expect? IIRC, the tests for the other methods of storing/serving validate that the actual content == expected content
Signed-off-by: Anik <anikbhattacharya93@gmail.com>
eee8ebd
to
2d64104
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good to me. Have one nit and a couple comments that I think would be nice to have addressed either as part of this PR or a follow-up.
fs := http.FileServer(http.FS(os.DirFS(dir))) | ||
return fs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Can reduce this to a one liner:
fs := http.FileServer(http.FS(os.DirFS(dir))) | |
return fs | |
return http.FileServer(http.FS(os.DirFS(dir))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also expect http.StripPath
to enter into the equation somewhere.
The URL paths will come in with /catalogs/<catalogName>/all.json
, but our http.FileSystem
will only see <catalogName>/all.json
, so we need to strip /catalogs
from the path before the http.FileSystem
sees the request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the benefit of making the path /catalogs/catalogName/all.json? Why not just keep it simple to /catalogName.json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original intention behind /catalogs/catalogName/all.json
was to leave room for expanding the endpoints (files being served) if desired without impacting clients wanting everything. For example, if we wanted to expand via file serving for getting only all the bundles for the catalog we could add /catalogs/{catalogName}/bundles.json
without changing the API for clients that always want to fetch everything. It becomes purely additive (and is IMO intuitive) to add new file based endpoints with that structure
@@ -50,5 +50,5 @@ spec: | |||
- "--health-probe-bind-address=:8081" | |||
- "--metrics-bind-address=127.0.0.1:8080" | |||
- "--leader-elect" | |||
- "--feature-gates=PackagesBundleMetadataAPIs=true,CatalogMetadataAPI=true" | |||
- "--feature-gates=PackagesBundleMetadataAPIs=true,CatalogMetadataAPI=true,HTTPServer=false" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still thinking this should probably be set to true by default so that all of the serving methods are available by default in the next release and we can remove the Package
, BundleMetadata
, and CatalogMetadata
in the next+1 release. That being said that can be done in a follow up if we want and shouldn't block this PR
It("the catalog should become available at server endpoint", func() { | ||
resp, err := httpclient.Do(httpRequest) | ||
Expect(err).To(Not(HaveOccurred())) | ||
defer resp.Body.Close() | ||
|
||
catalogs, err := io.ReadAll(resp.Body) | ||
Expect(err).To(Not(HaveOccurred())) | ||
Expect(string(catalogs)).To(Equal(fmt.Sprintf(httpResponse, fmt.Sprintf("\n<a href=\"%s\">%s</a>", catalogKey.Name+".json", catalogKey.Name+".json")))) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see this validate the response matches what we expect when we request the catalog contents rather than checking that there is a link to the file when visiting the index endpoint, but I'm okay with that being a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we should test that this returns the content we want, if not, what are we confident that we've written here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
// if the ShutdownTimeout is zero, wait forever to shutdown | ||
// otherwise force shut down when timeout expires | ||
sc := context.Background() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: sc
and scc
are opaque names that I'm not sure everyone will unpack (I struggled!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How bout shutdownCtx
and shutdownCancel
?
req, err := http.NewRequest("GET", testServer.URL, nil) | ||
Expect(err).To(Not(HaveOccurred())) | ||
req.Header.Set("Accept", "text/html") | ||
httpRequest = req |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a side-effect mutation to bring this up a scope? Why not create the request in the test closure that uses it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
creating that request in multiple closures is just repeating the same code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's wrong with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little bit less tongue-in-cheek: tests must be as independent as possible. The more that tests are intertwined, it's harder to break just one and grok all of it at once while reviewing changes. Repeating two or three lines of code is really not something we should be optimizing for.
It("the catalog should become available at server endpoint", func() { | ||
resp, err := httpclient.Do(httpRequest) | ||
Expect(err).To(Not(HaveOccurred())) | ||
defer resp.Body.Close() | ||
|
||
catalogs, err := io.ReadAll(resp.Body) | ||
Expect(err).To(Not(HaveOccurred())) | ||
Expect(string(catalogs)).To(Equal(fmt.Sprintf(httpResponse, fmt.Sprintf("\n<a href=\"%s\">%s</a>", catalogKey.Name+".json", catalogKey.Name+".json")))) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we should test that this returns the content we want, if not, what are we confident that we've written here?
pkg/storage/storage.go
Outdated
} | ||
} | ||
|
||
func (s *Storage) Store(owner string, fbc *declcfg.DeclarativeConfig) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is important.
Expect(err).To(Not(HaveOccurred())) | ||
//omitting trailing new line char from response | ||
Expect(string(catalogs[:len(catalogs)-1])).To(Equal(catalogKey.Name)) | ||
Expect(string(catalogs)).To(Equal(fmt.Sprintf(httpResponse, fmt.Sprintf("\n<a href=\"%s\">%s</a>", catalogKey.Name+".json", catalogKey.Name+".json")))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RFC does not mention anything about serving index.html
pages for directory listings. I would expect this request to be a 404 Not Found
response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up for clarity. I would expect tests for (pseudocode):
http.Get("<host>/catalogs/<catalogName>/all.json
=> response code 200 and valid FBChttp.Get("<host>/catalogs/<catalogName>/non-exist.json
=> response code 404http.Get("<host>/catalogs/<catalogName>/
=> response code 404http.Get("<host>/catalogs/
=> response code 404
I am not sure this file is the right place for these tests. This file contains tests for what Reconcile
does. In the case of catalog contents, Reconcile
only stores the contents into the storage directory. So I would expect tests here to just assert that the expected files exist in the storage directory after Reconcile
is called.
I think we should have unit tests in pkg/catalogserver
that use httptest
package.
And I think we should have an e2e test that actually queries the pod endpoint after a Catalog
says it is unpacked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Steve pointed out to me that we can probably get by without the Reconcile()
-specific test that would assert on filesystem contents if we have an e2e. That would make it easier to make internal changes and still be assured they don't break users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joelanford the catalogserver just starts a server. If the tests are moved to pkg/catalogserver
then you'd have to simulate a Storage
, at which point you're just duplicating work being done in the reconciler tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Steve pointed out to me that we can probably get by without the
Reconcile()
-specific test that would assert on filesystem contents if we have an e2e. That would make it easier to make internal changes and still be assured they don't break users.
I don't understand the difference between the e2e test being suggested here vs the Reconcile test. In fact the whole idea of this test set up was to avoid adding e2e tests unless we absolutely need them wasn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is different than what http.FileServer does though. FileServer just serves up the entire directory, and there's not much configurability when it comes to access points.
I think that this is partially true. The http.FileServer
is less flexible than rolling our own with http.ServeFile
, but I think we can achieve what @joelanford suggested by doing something similar to rukpak's implementation that creates a filesystem wrapper that forces 404 not found on requests that would return a directory list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, we're onto something now.....
Again, rukpak is exposing an arbitrary list of arbitrary artifacts stored in the directory. Using that implementation to expose just FBC in our case is definitely overkill. The argument put forward for using http.FileServer
was also something along the lines of "less configuration/less code required for standing up the server", but using that implementation to expose all.json is looking more like a long winded way of shoehorning everything in for http.FileServer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know that I personally feel strongly about @joelanford requirements, but in any case the FilesOnlyFilesystrem
is twenty lines of code you can import and re-use. You do not need to manage everything else that http.FileServer
does for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The requirements are from the RFC and I do agree with those requirements since they adhere to design best practices. We all +1-ed the requirements in the RFC too.
- FileServer is a wrapper around filehandler, and ServerHTTP ultimately calls
serveFile
.
ServeFile just takes the file name we pass as a parameter, splits the name into "dir name" and "file name", and uses those variables to call the exact same serveFile.
What is this "everything else" you're alluding to?
- I'm confused by rukpak's code. Could someone point me to where ServeHTTP is actually used? How are the files really exposed? I searched in the project and the only caller of this function is a test function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither here nor there on the requirements. Importing the 20LoC or copy-pasting it is low cost.
As for what "everything else" is - I would recommend going a bit deeper there. From the management of which files are and are not served, to how how paths are cleaned and block traversal attacks, etc.
Signed-off-by: Anik <anikbhattacharya93@gmail.com>
@stevekuznetsov @operator-framework/catalogd-maintainers closing this PR in favor of splitting it up into smaller PRs so that it's easier to review and get things moving along. Look out for a series of PRs that'll be based off of this PR, but with some additional stuff to address remaining open comments for the work being done in this PR. First one starts with #144 to store the FBC in a local Dir. |
closes #113