GitHub proxy: download GitHub server keys #50891
Conversation
greedy52
added
the
no-changelog
lib/srv/git/github.go
Outdated
| } | ||
|
|
||
| func (c *githubMetadataHTTPClient) fetchFingerprints() ([]string, string, error) { | ||
| resp, err := http.Get(c.api) |
There was a problem hiding this comment.
Why not use https://pkg.go.dev/github.com/google/go-github/v68/github#MetaService.Get instead? This is something that we likely only need to refresh at most every 24 hours or so.
There was a problem hiding this comment.
it's a pretty simple get call so i would prefer not to import another package. i also like the idea doing If-None-Match.
There was a problem hiding this comment.
I think I'd rather use pre-existing package to lower the maintenance costs, but it is true an extra dependency of this size is not trivial.
|
|
||
| // Remove newline from ssh.MarshalAuthorizedKey. | ||
| key := strings.TrimSpace(string(ssh.MarshalAuthorizedKey(targetKey))) | ||
| if slices.Contains(m.keys, key) { |
There was a problem hiding this comment.
To avoid confusing errors, I'd check that m.keys is non-empty as a special case.
| return &githubServerKeyManager{ | ||
| clock: clockwork.NewRealClock(), | ||
| apiEndpoint: "https://api.github.com/meta", | ||
| client: &http.Client{ | ||
| Timeout: defaults.HTTPRequestTimeout, | ||
| }, | ||
| } |
There was a problem hiding this comment.
We start with an empty list of keys that will only be refreshed on first request. This increases the latency and has unhappy consequences on error. Given how often GitHub experiences hiccups, I'd prefer to run refreshLocked in a separate goroutine, with variable schedule based on current status.
For example:
- Empty list of known keys? Retry every minute until we get the list.
- Non-empty list of known keys, but last attempt at refresh failed? Retry after 15 minutes.
- Non-empty list of known keys and last attempt worked? Repeat after 12h.
With this we decouple update schedule from checking, and check() method becomes very simple.
There was a problem hiding this comment.
It is definitely not great experience on unhappy scenarios. the api is also rate-limited.
So doing it in some schedular is preferable. However, I don't want to pull things from the api if they won't be used. so to do it properly, we need a new "service" registered to the proxy process and watch for Git servers. then only call the API when seeing github proxy is present. So i don't think it's worth it at the moment.
With the current implementation, we only need to successfully fetch once to unblock the flow. One thing i can improve with the current implementation, is to stop refreshing attempt if a failure has happened in the last 15 minutes. WDYT?
greedy52
changed the title
Related: