Conversation
Extract more basic http-related functionality out of lfsapi and into a new package, lfshttp. Everything is currently functional aside from authorization.
Refactor the client redirect code to allow lfsapi to re-authenticate redirected requests
|
This is the first PR to handle #3194. The actual changes to the access mode management will land in a separate PR after this is merged. |
bk2204
left a comment
There was a problem hiding this comment.
Overall, this looks good. I made one suggestion, but I'd be happy with it where it stands.
lfsapi/auth_test.go
Outdated
| return errors.New("Not implemented") | ||
| } | ||
|
|
||
| // func TestClientRedirectReauthenticate(t *testing.T) { |
There was a problem hiding this comment.
It looks like you added this code commented out in the first commit which was uncommented in this commit. I wonder if it might be tidier if we deferred adding it until the second commit.
|
Some further information on reviewing: d101bdb is exclusively functionality extraction, moving To review this commit, I'd take a look at e6c4c63 moves some of the |
ttaylorr
left a comment
There was a problem hiding this comment.
@PastelMobileSuit Huge ❇️'s from me on this -- I'm unreasonably happy with how everything shook out between lfsapi and lfshttp.
It seems like we have a great path forward to write code in package tq that can't assume it's talking to a Git LFS server, and instead can use lfshttp directly (or choose between the two).
Thanks for your comment in #3244 (comment), it certainly helped me during the review process. In the future, do you think it would be possible to include some of that information in your commits, too? I like that we've started to include this type of historical record in the commits themselves, since it makes it easier when trying to recall why a change was made in the future to run git blame and read the last commit that touched a line (instead of opening the commit in the browser, finding the affected PR, and reading through its comments).
I think that in general the division between the commits is good. I think in general I find it difficult to review two commits that have nearly 2,000 SLOC worth of diff, but I think for changes like these there's no great atomic way to separate things out.
Anyhow, my TL;DR is: this change is great, so is the writeup you posted, and in the future I would love that same information in the commit.
👏
At present, when the GIT_CURL_VERBOSE environment variable is set to a value such as "1" or "true", Git LFS client outputs some of the details of each HTTP request and response it makes to its standard error stream. We gather the HTTP request headers to be logged in the traceRequest() method of the Client structure from our "lfshttp" package. To gather the headers, the traceRequest() method uses the DumpRequest() function from the from the "net/http/httputil" package in the Go standard library. Our use of the DumpRequest() function for this purpose dates back to commit 6fafade of PR git-lfs#366 in 2015, when it was first introduced into what was then the traceHttpRequest() function in our "lfs" package. This function gradually evolved into the current traceRequest() method through a series of PRs. First, portions of the "lfs" package were migrated into a new "httputil" package in PR git-lfs#1226. That package was later removed in PR git-lfs#1846 after a new "lfsapi" package was added in PR git-lfs#1794 with a version of the traceRequest() method very similar to the current one. Finally, in PR git-lfs#3244 we moved the method into the current "lfshttp" package. Throughout these changes, however, our use of the DumpRequest() function has remained the same. However, since the Git LFS client only acts as an HTTP client and not an HTTP server, we should instead be using the DumpRequestOut() function from the standard "httputil" package to gather the headers of an HTTP request for logging purposes. Replacing our use of the DumpRequest() function with the DumpRequestOut() function would allow us to output a number of request headers that we currently fail to log, as was noted with respect to the Accept-Encoding header in issue git-lfs#6193. Since Go version 1.6, the "httputil" package's documentation has stated that the DumpRequest() function "should only be used by servers to debug client requests", and that the DumpRequestOut() function "is like DumpRequest but for outgoing client requests": https://pkg.go.dev/net/http/httputil@go1.6#DumpRequest https://pkg.go.dev/net/http/httputil@go1.6#DumpRequestOut However, at the time we first adopted the use of the DumpRequest() function, the Go documentation did not include these directives, and merely noted that "DumpRequest returns the as-received wire representation of req, optionally including the request body, for debugging" and that "DumpRequestOut is like DumpRequest but includes headers that the standard http.Transport adds, such as User-Agent": https://pkg.go.dev/net/http/httputil@go1.4#DumpRequest https://pkg.go.dev/net/http/httputil@go1.4#DumpRequestOut It is reasonable to assume, therefore, that when PR git-lfs#366 was written its authors simply assumed that the DumpRequest() function was sufficient, given the state of the Go documentation at that time. As we would prefer to log a more complete set of client HTTP request headers, though, particular the Accept-Encoding header which the Go "net/http" package automatically adds to outgoing requests, we now update our traceRequest() method to use the DumpRequestOut() function. We then also update our Go language TestVerbose*() test functions to confirm that the Accept-Encoding request header is now captured and reported when verbose logging is enabled. Note that without the change in this PR to the use of the DumpRequestOut() function, these revised tests would fail, since the Accept-Encoding header would not appear in headers returned by the DumpRequest() function.
At present, when the GIT_CURL_VERBOSE environment variable is set to a value such as "1" or "true", the Git LFS client outputs some of the details of each HTTP request and response it makes to its standard error stream. We gather the HTTP request headers to be logged in the traceRequest() method of the Client structure from our "lfshttp" package. To gather the headers, the traceRequest() method uses the DumpRequest() function from the from the "net/http/httputil" package in the Go standard library. Our use of the DumpRequest() function for this purpose dates back to commit 6fafade of PR git-lfs#366 in 2015, when it was first introduced into what was then the traceHttpRequest() function in our "lfs" package. This function gradually evolved into the current traceRequest() method through a series of PRs. First, portions of the "lfs" package were migrated into a new "httputil" package in PR git-lfs#1226. That package was later removed in PR git-lfs#1846 after a new "lfsapi" package was added in PR git-lfs#1794 with a version of the traceRequest() method very similar to the current one. Finally, in PR git-lfs#3244 we moved the method into the current "lfshttp" package. Throughout these changes, however, our use of the DumpRequest() function has remained the same. However, since the Git LFS client only acts as an HTTP client and not an HTTP server, we should instead be using the DumpRequestOut() function from the standard "httputil" package to gather the headers of an HTTP request for logging purposes. Replacing our use of the DumpRequest() function with the DumpRequestOut() function would allow us to output a number of request headers that we currently fail to log, as was noted with respect to the Accept-Encoding header in issue git-lfs#6193. Since Go version 1.6, the "httputil" package's documentation has stated that the DumpRequest() function "should only be used by servers to debug client requests", and that the DumpRequestOut() function "is like DumpRequest but for outgoing client requests": https://pkg.go.dev/net/http/httputil@go1.6#DumpRequest https://pkg.go.dev/net/http/httputil@go1.6#DumpRequestOut However, at the time we first adopted the use of the DumpRequest() function, the Go documentation did not include these directives, and merely noted that "DumpRequest returns the as-received wire representation of req, optionally including the request body, for debugging" and that "DumpRequestOut is like DumpRequest but includes headers that the standard http.Transport adds, such as User-Agent": https://pkg.go.dev/net/http/httputil@go1.4#DumpRequest https://pkg.go.dev/net/http/httputil@go1.4#DumpRequestOut It is reasonable to assume, therefore, that when PR git-lfs#366 was written its authors simply assumed that the DumpRequest() function was sufficient, given the state of the Go documentation at that time. As we would prefer to log a more complete set of client HTTP request headers, though, particularly the Accept-Encoding header which the Go "net/http" package automatically adds to outgoing requests, we now update our traceRequest() method to use the DumpRequestOut() function. We then also update some of our Go language TestVerbose*() test functions to confirm that the Accept-Encoding request header is now captured and reported when verbose logging is enabled. Note that without the change in this PR to the use of the DumpRequestOut() function, these revised tests would fail, since the Accept-Encoding header would not appear in headers returned by the DumpRequest() function.
This PR extracts much of the lower level, http request-handling functionality out of the
lfsapipackage and into a newlfshttppackage.The first commit (d101bdb) largely just extracts files into the new package with minimal changes, and updates
lfsapiand other packages to rely on it. The second commit (e6c4c63) refactors the way redirects are handled so thatlfsapican re-authenticate on a redirect when the original request was authenticated.