For the sake of discussion, I used stream=true|false as if they were query parameters, but in reality the endpoint receives a JSON object (example [0]) with ‘stream’ as one of the fields. I don’t think it changes the substance of the discussion, but wanted to mention it.
Thanks for the suggestion, seems like a nice way to work around the API compatibility. In the case of stream=false, I had considered returning a link with the JSON object as query params or encoded to base64. Something like: /api/1.1/isos?form=oxIiwiaXA2R2F0ZXdheSI6ImJlZWY6OjIiLCJpcEFkZHJ... The handler could then decode it and stream whenever requested. The issue I saw is that the form contains the root password for the image, meaning the URL would too. Would this be an issue? If so, maybe it’d be OK to (en)crypt the password first, before encoding it in the URL? The endpoint doesn’t actually need the plaintext password to generate the ISO, just a crypted digest of it. [0] https://traffic-control-cdn.readthedocs.io/en/latest/api/isos.html#id5 From: ocket 8888 <[email protected]> Reply-To: "[email protected]" <[email protected]> Date: Tuesday, December 10, 2019 at 18:02 To: "[email protected]" <[email protected]> Subject: [EXTERNAL] Re: Proposal: Only allow streaming downloads of ISOs I'd just like to suggest something: what if, when rewriting the endpoint, you just made stream=false always return the ISO generation URL itself but with stream=true. That way you can make it do only streaming downloads without any breaking change to the API, allowing for a proper depreciation of that parameter (or maybe even the whole endpoint?). On Tue, Dec 10, 2019, 08:05 Jeremy Mitchell <[email protected]<mailto:[email protected]>> wrote: There are definitely lots of downsides to the default implementation (providing a link to the iso) and that's why i believe the ?stream=true option was added. Too bad it wasn't always the default/only behavior. I only see a few options here: 1. rewrite POST /api/1.1/isos?stream=true|false with the current behavior and then with api 2.x consider a new route that doesn't support the query param but always streams. 2. rewrite the streaming part of POST /api/1.1/isos and then figure out a way in routes.go to route POST /api/1.1/isos?stream=true to the Go implementation and allow POST /api/1.1/isos?stream=false (or no stream query param) to fallback to the Perl implementation and then with api 2.x consider a new route that doesn't support the query param but always streams. 3. don't rewrite POST /api/1.1/isos?stream=true|false at all and let it continue to be served by Perl and eventually deprecated. Instead take the work you have done and write POST /api/2.0/isos that has no query param support and simply streams the iso. Changing the response of stream=false in 1.x to an error message would be a breaking api change and should probably be avoided and also forcing users to use the blacklist to honor the 1.x contract of that endpoint might be a bit much. Unfortunately, it feels like we've backed ourselves into a corner with this one. Jeremy On Mon, Dec 9, 2019 at 2:51 PM Williams, Adam <[email protected]<mailto:[email protected]>> wrote: > This concerns the `/isos` API endpoint and the re-write from Perl to Go. > The endpoint provides users a way to generate and download an ISO [0]. > Currently it has two modes: > > - stream=true: The response is the ISO file that the user receives as a > download. Nothing is permanently saved on TO servers in this case, instead > the data is streamed directly to the client. > - stream=false: The response contains a link to download the ISO file. > TrafficOps saves the ISO file on the server’s filesystem for a later > download by the client. > > The stream=false mode has a few shortcomings: > - The generated files will either eventually fill up the server’s disk or > need to be periodically deleted (breaking the download links). > - Clients must be routed directly to the TrafficOps server that contains > the ISO file, which can be complicated in a setup where multiple TrafficOps > are fronted by a load balancer. It also requires exposing the TrafficOps > server when otherwise not necessary. > > I propose returning a user-friendly error when stream=false, essentially > restricting users to stream=true. Operators wishing to allow stream=false > can blacklist the route and force Perl to handle it. Barring objections, > I’ll include this as part of the re-write from Perl to Go. > > [0] > https://urldefense.com/v3/__https://traffic-control-cdn.readthedocs.io/en/latest/api/isos.html*isos__;Iw!!CQl3mcHX2A!Qs9fFEXtedepL-GhIxdINyM13ZhiFK7K9l3HIk460pHD6hWCfoSNsKneIaDYPEjtR0kN$<https://urldefense.com/v3/__https:/traffic-control-cdn.readthedocs.io/en/latest/api/isos.html*isos__;Iw!!CQl3mcHX2A!Qs9fFEXtedepL-GhIxdINyM13ZhiFK7K9l3HIk460pHD6hWCfoSNsKneIaDYPEjtR0kN$> >
