Fix #332: Use lru cache to expire entrypoint items #350
Conversation
knative-prow-robot
added
size/M
There was a problem hiding this comment.
Thanks for this change! It's nice that it's so easy to use, and should help us avoid a DoS avenue.
Can you add some tests for this new behavior in this change? It should be fairly easy to simulate the calls to have something expire from the cache, particularly if you make the size a package level var and set it to something low in your test.
| cache: make(map[string][]string), | ||
| lru, err := lru.New(defaultEntryPointCacheSize) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
We shouldn't panic here, we should return the error from NewCache and update callers to handle it.
There was a problem hiding this comment.
THX for review. Has fixed code as you requested. @imjasonh
There was a problem hiding this comment.
Thanks for implementing this @mathspanda ! 🎉
I have a bit of feedback, mostly about mutating the value of entrypointCacheSize
Also before you merge this in, could you amend the commit and explain a bit more about why this change is being made in the commit message (you can probably reuse a lot of what @imjasonh wrote in #332 if you want), and add the issue number to the commit itself? See our commit message guidelines
Thanks!!!
| // due to the possibility of a panic if two workers were to attempt to read and | ||
| // write to the internal map at the same time. | ||
| // getting the Entrypoint of a container image from a remote registry. The | ||
| // internal lru cache is thread-safe. |
mathspanda
changed the title
| @@ -1,4 +1,4 @@ | |||
| package entrypoint_test | |||
| package entrypoint | |||
There was a problem hiding this comment.
Any reason to use entrypoint here (instead of entrypoint_test) ? I'm guessing for cacheSize 🙃
(I tend to really like when the test package is different from the current package as it forces to write test that tests the behavior more than the implementation)
There was a problem hiding this comment.
I actually usually prefer to have tests in the same package, so that package-level vars/consts/funcs don't get exported just so they can be called from tests.
In some cases it's possible to only call funcs that are exported anyway, but sometimes those funcs get long and need to be split up into smaller focused funcs, and it's usually a good idea to avoid exporting those.
There was a problem hiding this comment.
I actually usually prefer to have tests in the same package, so that package-level vars/consts/funcs don't get exported just so they can be called from tests.
Yeah, we should definitely not export var/const/func just for the sake of tests 😉
|
/ok-to-test |
knative-prow-robot
removed
the
needs-ok-to-test
Use a lru cache that will only keep the most recently-used entrypoints cached and drop entrypoints haven't been used recently, in order to present a risk of DoS. 1. replace internal map in original Cache with lru.Cache 2. fix incorrect entrypoint_test package name Signed-off-by: mathspanda <[email protected]>
|
The following is the coverage report on pkg/.
|
There was a problem hiding this comment.
Looking great @mathspanda ! I think we just need one more review from @imjasonh at this point
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ImJasonH, mathspanda The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
Missing dep, probably after lru cache was implemented in tektoncd#350
Missing dep, probably after lru cache was implemented in #350
Fix issue #332
Use a lru cache that will only keep the most recently-used entrypoints cached and drop entrypoints haven't been used recently, in order to present a risk of DoS.
Signed-off-by: mathspanda [email protected]