Flux oci support 12: fix for Unable to use Flux OCI with Harbor robot accounts #5219 #5305
Conversation
✅ Deploy Preview for kubeapps-dev canceled.Built without sensitive environment variables
|
| orasregistryauthv2 "oras.land/oras-go/v2/registry/remote/auth" | ||
| ) | ||
|
|
||
| // This flavor of OCI repsitory ister works with respect to to VMware Harbor |
There was a problem hiding this comment.
| // This flavor of OCI repsitory ister works with respect to to VMware Harbor | |
| // This flavor of OCI repository lister works with respect to VMware Harbor |
Also, I don't think it is just for the VMware harbor instance, no? Ah, perhaps you meant the VMware Harbor product? If so, I think Harbor is not -just-a VMware project, but a Harbor is a Cloud Native Computing Foundation one, as it appears as a Graduated project on their website.
Anyway, just being nitpicky, feel free to ignore the comment :P
There was a problem hiding this comment.
will fix comment
| ) | ||
|
|
||
| // This flavor of OCI repsitory ister works with respect to to VMware Harbor | ||
| // container registry. The Swagger for the API can be found on an running server |
There was a problem hiding this comment.
Another nitpick: the Swagger name is discouraged in favor of "OpenAPI"
| // container registry. The Swagger for the API can be found on an running server | |
| // container registry. The OpenAPI description of the API can be found on a running server |
There was a problem hiding this comment.
link to source please?
Harbor certainly did not get the message
There was a problem hiding this comment.
Sorry, here it is: https://www.openapis.org/faq/style-guide.
Besides, my former company was a member of the OpenAPI Initiative and I was somehow also involved in.
But... the reality is that there are plenty of people still using the "Swagger" word. In fact, it can be a synonym, but only if the spec version is 2.0.
In fact, the according to the style guide, the proper term would be OpenAPI Specification document or just OAS document
There was a problem hiding this comment.
thanks. I will re-word comment
| case http.StatusNotFound: | ||
| return errdef.ErrNotFound | ||
|
|
||
| case http.StatusForbidden: | ||
| return errors.New("forbidden") |
There was a problem hiding this comment.
I don't know where both errors are being handled, but it seems we are mixing up the error builder from oras (errdef) and the builtin errors packages. Should we use the same for both cases? Or perhaps it doesn't really matter due to the error hanlding being performed on the call site?
There was a problem hiding this comment.
I will get rid of lines 157-158. They are not needed.
| if plainHTTP { | ||
| scheme = "http" | ||
| } | ||
| return fmt.Sprintf("%s://%s/api/v2.0", scheme, ref.Host()) |
There was a problem hiding this comment.
Just thinking: would be a good idea to extract all the templates into const being defined at the bottom? This way we would have all the paths in the same place (in case the change their API.
Anyway, not really important, as I don't foresee that breaking changes for the major (v2) this lister is intended to.
There was a problem hiding this comment.
yeah, I don't see them changing v2 API in a backwards incompatible way anywhere soon. Will keep your comment in mind for the next PR(s)
| var body struct { | ||
| Errors requestErrors `json:"errors"` | ||
| } | ||
| lr := io.LimitReader(resp.Body, maxErrorBytes) |
There was a problem hiding this comment.
Just curious as it seems a good idea for preventing some buffer overflow attacks (and we aren't doing it in the rest of the codebase, AFAIK).
Do you think we should limit the read operations in other places?
There was a problem hiding this comment.
I think in general it is a good idea, provided you can make this assumption. Normally, you can't really assume anything about the size of the payload you're dealing with. So whatever limit/boundary X you pick, I will probably find a valid use case where receiving X+1 bytes is valid
I just realized by "other places" you may have meant other places where we handle receiving error response. If so, I definitely think its a good idea
| if err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
I'm seeing this pattern repeated a lot through our code base, have you considered using stretchr/testify library, which is already included as a project dependency, and would make tests (IMHO) easier to write, read and maintain? In this case, we'd replace the three lines with just require.NotNil(err).
There was a problem hiding this comment.
I haven't considered it due to I did not know it existed. I don't have any objection to using it
implemented fix as described in the issue
#5219 (comment)
fixed 1 and implemented 2b
verified it works with respect to robot account of VMware corporate harbor instance