-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Working goproxy implementation #55
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
base: main
Are you sure you want to change the base?
Conversation
| max-ttl = "8h" | ||
| } | ||
|
|
||
| gomod { |
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.
Sick!
| MutableTTL time.Duration `hcl:"mutable-ttl,optional" help:"TTL for mutable Go module proxy endpoints (list, latest). Defaults to 5m." default:"5m"` | ||
| ImmutableTTL time.Duration `hcl:"immutable-ttl,optional" help:"TTL for immutable Go module proxy endpoints (versioned info, mod, zip). Defaults to 168h (7 days)." default:"168h"` |
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.
Are these relevant anymore?
| // as a signal to fetch from upstream. | ||
| func (g *goproxyCacher) Get(ctx context.Context, name string) (io.ReadCloser, error) { | ||
| // Hash the name to create a cache key that matches cachew's format | ||
| key := cache.Key(sha256.Sum256([]byte(name))) |
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.
| key := cache.Key(sha256.Sum256([]byte(name))) | |
| key := cache.NewKey(name) |
| // Hash the name to create a cache key that matches cachew's format | ||
| key := cache.Key(sha256.Sum256([]byte(name))) | ||
|
|
||
| // Try to open the cached content |
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.
Can we get rid of all these completely superfluous comments? I find they make it quite a bit harder to comprehend what is actually going on.
I'm a bit surprised they're showing up TBH, the AGENTS.md file has explicit instructions about it 🤔
| // it represents mutable or immutable content. | ||
| func (g *goproxyCacher) Put(ctx context.Context, name string, content io.ReadSeeker) error { | ||
| // Hash the name to create a cache key | ||
| key := cache.Key(sha256.Sum256([]byte(name))) |
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.
| key := cache.Key(sha256.Sum256([]byte(name))) | |
| key := cache.NewKey(name) |
| // The TTL is determined by inspecting the cache name to identify whether | ||
| // it represents mutable or immutable content. |
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.
Super weird, is this actually true? What is "name" in the context of the interface? Does it actually include things like /v@/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.
I checked and you are 100% right...that's super weird, as the default implementation doesn't seem to do any sort of timing out of /list entries etc...
Ah, I bet I know what's going on - I bet it uses an ETag to conditionally update it.
I think that means we can actually get rid of the two different TTL types, and just rely on the ETag to manage the cache.
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 not at all convinced I'm correct about that actually, however it does seem to overwrite the list on every go get, as well as hit upstream list on every request...so there doesn't seem to be much point in having a TTL on list at all. If anything, it might be better to completely ignore /list...
DBG Request received client=127.0.0.1:56140 request="GET /gomod/github.com/alecthomas/chroma/@v/list"
DBG Request received client=127.0.0.1:56142 request="GET /gomod/github.com/alecthomas/@v/list"
DBG Request received client=127.0.0.1:56141 request="GET /gomod/github.com/@v/list"
DBG Go module proxy outbound request method=GET url=https://proxy.golang.org/github.com/alecthomas/@v/list
DBG Go module proxy outbound request method=GET url=https://proxy.golang.org/github.com/@v/list
DBG Go module proxy outbound request method=GET url=https://proxy.golang.org/github.com/alecthomas/chroma/@v/list
DBG goproxy.Get client=127.0.0.1:56141 request="GET /gomod/github.com/@v/list" name=github.com/@v/list
DBG goproxy.Get client=127.0.0.1:56142 request="GET /gomod/github.com/alecthomas/@v/list" name=github.com/alecthomas/@v/list
DBG goproxy.Put client=127.0.0.1:56140 request="GET /gomod/github.com/alecthomas/chroma/@v/list" name=github.com/alecthomas/chroma/@v/list
DBG Request received client=127.0.0.1:56142 request="GET /gomod/github.com/dlclark/regexp2/@v/list"
DBG Go module proxy outbound request method=GET url=https://proxy.golang.org/github.com/dlclark/regexp2/@v/list
DBG goproxy.Put client=127.0.0.1:56142 request="GET /gomod/github.com/dlclark/regexp2/@v/list" name=github.com/dlclark/regexp2/@v/list
Super weird.
Also interestingly, on cold start it seems to hit the upstream no matter what. Subsequent runs are quite fast, so it appears it might have some in-memory caching too.
I'm not sure what to think about it all.
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 checked the source and it does indeed just call the upstream. I also checked and the upstream has cache control headers with a 60 second TTL.
| ttl := g.calculateTTL(name) | ||
|
|
||
| // Determine Content-Type from the file extension | ||
| contentType := g.getContentType(name) |
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 this make sense?
|
I pushed up a minor fix to configure it to use our logger, so it didn't spam to stdout, however it does spam errors for every package it pulls, for some reason: Running: Gives: I think we're going to need some kind of log interceptor to filter these out 😕 |
|
Okay fixed the logging too! |
What?
This replaces the custom gomod handling and embeds https://github.com/goproxy/goproxy instead.
Why?
More robust and battle tested than our current implementation. It also should give us a path to supporting private modules as well via a custom fetcher in the future.
Tests