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

Small issues that I ran into running tests #198

Open
vsoch opened this issue Oct 10, 2020 · 3 comments
Open

Small issues that I ran into running tests #198

vsoch opened this issue Oct 10, 2020 · 3 comments

Comments

@vsoch
Copy link
Contributor

vsoch commented Oct 10, 2020

I am currently working through conformance testing for my plugin, and I want to open an issue to put notes / things to discuss. Think of this as in progress - I'll likely need a little bit to get through all the tests. I will clearly update the issue (at the end) to indicate completion when it's done. Feel free to disregard until then :)

Conformance testing notes

I ran into some small discrepancies, or more like points that weren't clearly stated in the spec that might be worth making more transparent.

  • push request: I assumed that I would require a content-type to be application/octet-stream for the push request (to return the location) but text-plain is fine. Since the POST is done under the same function, this came down to moving the check to be within a check for the digest (indicating we need the content-type) but I suspect other implementers might hit this issue too.
  • this isn't an issue with the spec here, but a lot of frameworks come with a set of accepted content types off the bat, and might return a 406 error when seeing a manifest / layer content type. What I had to do, for example, is define a custom parser for an incoming request and add it to my views.
  • for pushing a blob in chunks it says that a content range is required, however the conformance test leaves it undefined see here. For now I removed the requirement of the range and have it be 0 through the start given that it's not defined, but this seems likely to run into a bug if a request forgets to specify it, and it's not supposed to be start to end.
  • Pushing a blob monolithically should respond with 201 or 202 but the test only allows 201.
  • A small detail, but it should be explicitly stated that a first chunk not starting with 0 should also return 416 error code - the two are separate so for my implementation I had raised a different kind of error.
  • There isn't any standard / statement on ending slashes for urls, which can lead to trouble for some implementations (just ran into a bug myself, the traditional redirect was taking a DELETE request to the same view's GET).
  • For deleting blobs, what happens to manifests (images) that used to reference them? Is the onus on the requester to clean things up as they should be? There maybe should be some section in the FAQ about deleting best practices (in terms of what the registry should or should not do).
  • this last one is going to sound incredibly silly, but one "best practice" I generally do is to cache views, and of course this means API calls. So I was seeing okay information on my server but getting a clearly wrong response - took me way too long for the piano to fall out of the sky onto my head and realize that they were being cached :)
@vsoch
Copy link
Contributor Author

vsoch commented Oct 12, 2020

okay, done with testing! https://vsoch.github.io/django-oci/conformance/ This issue is ready to be looked at. There are still some tweaks and additional tests I want to do with django-oci, and then after come up with some nice documentation and more storage backends, but I think the hard bit (the specification adherence) is almost there! 🎉

@jdolitsky
Copy link
Member

@vsoch - very cool to see a registry implemented from spec!

Here are some answers:

push request: I assumed that I would require a content-type to be application/octet-stream for the push request (to return the location) but text-plain is fine. Since the POST is done under the same function, this came down to moving the check to be within a check for the digest (indicating we need the content-type) but I suspect other implementers might hit this issue too.

I'm confused what you mean. What do you mean text-plain is fine? As far as conformance, your registry should at least allow octet-stream, and up to you for how strict you want to be.


this isn't an issue with the spec here, but a lot of frameworks come with a set of accepted content types off the bat, and might return a 406 error when seeing a manifest / layer content type. What I had to do, for example, is define a custom parser for an incoming request and add it to my views.

I hadn't come across this issue specifically, but perhaps its worth stating somewhere in FAQ the list of content types that must be allowed


for pushing a blob in chunks it says that a content range is required, however the conformance test leaves it undefined see here. For now I removed the requirement of the range and have it be 0 through the start given that it's not defined, but this seems likely to run into a bug if a request forgets to specify it, and it's not supposed to be start to end.

What you are referring to here is actually a type of monolithic upload. In the spec:

There are two ways to push a blob monolithically:

1. A single POST request
2. A POST request followed by a PUT request

This is number 2. Perhaps is can be better stated in the test suite?


Pushing a blob monolithically should respond with 201 or 202 but the test only allows 201.

Yes, this is under review here: #171


A small detail, but it should be explicitly stated that a first chunk not starting with 0 should also return 416 error code - the two are separate so for my implementation I had raised a different kind of error.

Would this not be considered an out-of-order upload? We could add a test for this too.


There isn't any standard / statement on ending slashes for urls, which can lead to trouble for some implementations (just ran into a bug myself, the traditional redirect was taking a DELETE request to the same view's GET).

Do you have an example? I cant think of an instance of a URL that would/should have a trailing slash, besides /uploads/ session URL.


For deleting blobs, what happens to manifests (images) that used to reference them? Is the onus on the requester to clean things up as they should be? There maybe should be some section in the FAQ about deleting best practices (in terms of what the registry should or should not do).

Would you consider this a separate issue from this discussion? #180


this last one is going to sound incredibly silly, but one "best practice" I generally do is to cache views, and of course this means API calls. So I was seeing okay information on my server but getting a clearly wrong response - took me way too long for the piano to fall out of the sky onto my head and realize that they were being cached :)

I hope you have recovered from this piano incident :) But I do think it's evident that the requests to this API should be as dynamic as possible to reflect whats in the registry. I think some of your issues are a function of using a framework (Django) designed for browser apps vs. APIs

@vsoch
Copy link
Contributor Author

vsoch commented Oct 14, 2020

I'm confused what you mean. What do you mean text-plain is fine? As far as conformance, your registry should at least allow octet-stream, and up to you for how strict you want to be.

I mean to say that I was expecting a content type of application/octet-stream (e.g., for my registry I was validating it and returning an error to get a different content type) but for the conformance test it was sending along text/plain so "text plain is fine." In practice I would have expected the content-type header to have been added here.

What you are referring to here is actually a type of monolithic upload. In the spec:

I think we are talking about different things, because I'm not referencing a POST but rather a PATCH. I think I read that the PATCH should have the content range. I think the two requests for uploads with POST start on lines 63 and 85.

Would this not be considered an out-of-order upload? We could add a test for this too.

Definitely, and maybe a test would help because it feels like an edge case.

Do you have an example? I cant think of an instance of a URL that would/should have a trailing slash, besides /uploads/ session URL. An example is Django's APPEND_SLASH. And then it's relevant in the regular expressions created for each view.

I think it would be enough to state it explicitly - a lot of frameworks will automatically add the slash, so it's more for the developer to know to disable/enable that.

Would you consider this a separate issue from this discussion? #180

Yep I can watch this issue, thanks!

I hope you have recovered from this piano incident :) But I do think it's evident that the requests to this API should be as dynamic as possible to reflect whats in the registry. I think some of your issues are a function of using a framework (Django) designed for browser apps vs. APIs

The Piano Doctor fixed me up :_) You're correct that Django is a general MVC framework, but it makes it relatively easy to design an API and then of course models in the database and views to match. It's really nice for creating a plugin (the models, urls, and views) for someone else to easily plugin into their application, which is why I chose it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants