Director advertise origin instead of writebackhost #622
Conversation
Changes made with this PR: - Client now gets the endpoint for a put request through the director, this endpoint is the origin_url. PUTs now require a running director to work. - When running origin serve and as a configurable option, you can specify if the origin is writeable (default is true). - Made a test within handle_http_test.go end-to-end with xrootd, using the federation in a box since now PUTs require a director to be running. - To remove an import cycle, made the pelican version global through config
| } | ||
|
|
||
| payload := []byte("forPUT") | ||
| req, err := http.NewRequest("GET", directorUrl.String(), bytes.NewBuffer(payload)) |
There was a problem hiding this comment.
Why is this calling GET in order to do a PUT? Shouldn't it just invoke a PUT and rely on the director to redirect?
What's the implications on backward compatibility here? Doesn't this break OSDF integration because it doesn't use the topology information when that's the preferred mechanism?
To avoid backward compat breaks, we should do something similar to what is done with downloads:
0. Based on configuration, perhaps take the topology code branches.
- If using the director, invoke a
PUTto the director when doing the namespace resolution. - Instead of following the redirect, look at the headers / redirect location header to determine the "real" location of the destination.
- Based on (2), fill in the namespace structure accordingly.
- Then, the upload code proceeds without knowing where the namespace information comes from.
For examples from the analogous GET case, see https://github.com/PelicanPlatform/pelican/blob/v7.3.3/client/director.go#L66
There was a problem hiding this comment.
Is it a put? It's not doing an update to the director, is it? It's getting information.
There was a problem hiding this comment.
@bbockelm Just want to make sure I am approaching this correctly:
So should I move this logic to around where we query the director/CreateNsFromDirectorResp within client/main.go and populate writebackhost with the correct value in cases we do not use topology. Then change uploadfile to get the destination from writebackhost in the namespace struct?
There was a problem hiding this comment.
One tweak:
Then change uploadfile to get the destination from writebackhost in the namespace struct?
I'd say change uploadfile back to this behavior. Basically, revert the changes in that method back to where it was before.
There was a problem hiding this comment.
Oh, another clarification:
move this logic to around where we query the director
The logic should also change slightly too. Instead of a GET with body contents, it should be a PUT.
| // Actually perform the Put request to the server | ||
| func doPut(request *http.Request, responseChan chan<- *http.Response, errorChan chan<- error) { | ||
| var UploadClient = &http.Client{Transport: config.GetTransport()} |
There was a problem hiding this comment.
What's the impact of this? Shouldn't clients be shared?
There was a problem hiding this comment.
I moved this within the function because when it was a global variable, the transport was not being set correctly. For example, it did not recognize TLSSkipVerify being set to true and it was always false. Somehow it was getting the transport before any init functions in config were called.
These fixes are for PR PelicanPlatform#622 which helps reinstate backwards compatibility. Changes include: - Returning `uploadFile` to its original state - Running a PUT on the director to get PUT endpoint instead of a GET - Moved this logic to `client/main.go` when we gather ns info - Moved the gathering of ns info to a seperate function
Yes, Emma fixed this in one of her PRs |
It depends on what you mean by "updated". |
These fixes are for PR PelicanPlatform#622 which helps reinstate backwards compatibility. Changes include: - Returning `uploadFile` to its original state - Running a PUT on the director to get PUT endpoint instead of a GET - Moved this logic to `client/main.go` when we gather ns info - Moved the gathering of ns info to a seperate function
Changes made with this PR:
Might have missed some other patch work to make everything work properly as well but this is the gist of it