Skip to content
This repository was archived by the owner on Aug 28, 2023. It is now read-only.

[WIP] Add payload download action. #33

Closed

Conversation

martinpovolny
Copy link
Contributor

@martinpovolny martinpovolny commented Jan 20, 2020

This is not going to work as simple. Because:

  • with an Ajax request I cannot force the browser to open the download window
  • w/o an Ajax request I cannot add the API authentization headers

What could work:

  • a) I could make an API endpoint that would return the data. Then an create a data: link with base64 data and click it. This will blow the browser if the file is large. And it's ugly.

  • b) Or I could create an API endpoint that is NOT authenticated using the headers. It could work like this:

  1. One (normal, authenticated) API call would return a "download token"
  2. server would store the token somewhere for a while
  3. another API call would be authenticated by the download token (not by the header) and would return the data.
  • c) Or else the download functionality would not be in the API, but in the UI worker (assuming that the API and UI workers are on the same appliance).

I am against a).

b) is not very nice, but if it can be guaranteed that the workers are on the same appliance in the V2V scenario, then this will work.

c) is the IMO the best and for example Github does that. But the tokens need to be stored somewhere and it would be wise to implement this in a generic way on the API level.

Trying the C) variant

@mturley : in the second commit I am adding an UI side implementation of the action.

You can call it from the UI (javascript) like this:

window.DoNav('/migration_analytics/payload?task_id=100')

Please, take a look.

The first commit can be ignored.

@miq-bot
Copy link

miq-bot commented Jan 20, 2020

Checked commits martinpovolny/cfme-migration_analytics@d6d5a52~...67dbee5 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 2 offenses detected

**

  • 💣 💥 🔥 🚒 - Linter/Rubocop - missing config files
  • 💣 💥 🔥 🚒 - Linter/Yaml - missing config files

@mturley
Copy link
Collaborator

mturley commented Jan 20, 2020

Thanks @martinpovolny, I'll test it out.

@bthurber, @fdupont-redhat, is it acceptable for us to require that the API and UI workers are on the same appliance in order to download a payload? I assume this will be true in the majority of cases, but I wonder if it's ok to make it a hard requirement.

@martinpovolny maybe if we do have that requirement, the UI can somehow detect if the workers are on the same appliance, and if not it can fall back to the current SCP command display instead of a download button.

@ghost
Copy link

ghost commented Jan 20, 2020

I'll defer to @pemcg for the best practices. I would say that it's a valid requirement, but he's the master :)

@mturley
Copy link
Collaborator

mturley commented Jan 20, 2020

@martinpovolny I just tested this and the download works on my end. Once it's grabbing the actual payload file do you think we can preserve the filename (e.g. cfme_inventory-20200120-38317-hdpx95.tar.gz)?

@mturley
Copy link
Collaborator

mturley commented Jan 20, 2020

Also do you know of any way I can make an API call to check whether the API and UI workers are on the same appliance (i.e. check if the download will work)?

@pemcg
Copy link
Contributor

pemcg commented Jan 21, 2020

I'd say that in a multi-appliance setup you cannot guarantee that the API and UI workers that you're talking to are necessarily on the same appliance, without somehow being able to examine the URI path (i.e. IP address) to each endpoint. The workers are grouped as region resources, and workers are often allocated using the following region methods:

https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_region.rb#L214
https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_region.rb#L237

If you have 2 appliances you might have both configured with active UI and API workers, but depending how workers (i.e. region resources) have been allocated to your inbound request, you may well be served by different appliances.

For example this is the reason we have to store the temporary API access token in the database rather than appliance memcache when using embedded Ansible, if the Ansible playbook needs to make an API call back to Automate using the token. We can't guarantee that Ansible and the Automation engine will be on the same appliance. Could we use the same mechanism?

Perhaps an ignorant question, but how is the authentication handled for other parts of the CloudForms UI that allow for downloading, such as exporting policies or automation domains?

@martinpovolny
Copy link
Contributor Author

This is not about general CF. This is about V2V. Therefor I was asking about the guarantee of running on a single appliance. In the context of V2V.

@pemcg :

Perhaps an ignorant question, but how is the authentication handled for other parts of the CloudForms UI that allow for downloading, such as exporting policies or automation domains?

In other places we are serving data from the database. Which is shared by the workers across the region. Therefor we do not have this problem.

In this particular situation we are downloading a payload that is generated by a worker on the filesystem.

AFAIK there's a flag on that task that says that it should be picked by a worker running on the same appliance as the API. (Correct me if I am wrong, I have limited experience with V2V.) But it's not guarantee that also the UI worker would be running there.

@martinpovolny
Copy link
Contributor Author

I can probably also do a simpler implementation of C) w/o introducing new entity to store the tokens in the database by encrypting information about the task_id into a token and then creating an API endpoint that is not authenticated by the API auth token but instead only decrypts this new token, and takes the task_id from there w/o using the database.

@martinpovolny
Copy link
Contributor Author

@mturley : here's a rebased PR with just he one needed commit: #35

@mturley
Copy link
Collaborator

mturley commented Jan 21, 2020

@martinpovolny thanks for #35, we might just be able to go with that if the single-appliance requirement is acceptable. It would be nice to get it working in a multi-appliance setup, but I'm not sure it's worth all the effort of a custom-auth solution.

I think I'm still missing something about why we can't just use a normal API endpoint and have it return the payload directly in a GET. Could we do option (a) (make an API endpoint that would return the data) but then instead of calling it with AJAX, just put an <a> tag in the HTML with the API URL as its href? Is it a matter of the API response needing to be wrapped in some metadata, preventing us from just returning the payload as the root of the response JSON? If it's possible to get the data in order to create a data: link with base64 data I figure it should be possible to make the data available as a plain URL-navigation-based GET via the API.

@mturley
Copy link
Collaborator

mturley commented Jan 21, 2020

If that's not possible, I think we could have the download button appear only if you're in a single-appliance setup, and otherwise fall back to the SCP command we display today. That way it won't be broken on a multi-appliance setup, and we'll still get the download button in the majority use case (a single appliance has been installed at a new customer specifically for running MA).

@martinpovolny
Copy link
Contributor Author

Could we do option (a) (make an API endpoint that would return the data) but then instead of calling it with AJAX, just put an tag in the HTML with the API URL as its href?

The API is authenticated by a custom header.

How do you envision adding a custom header to a request that is fired from an <a> tag in HTML?

@mturley
Copy link
Collaborator

mturley commented Jan 21, 2020

Ahh.. I forgot it was a custom header and not a cookie or some other means of implicit auth... I figured it would work since I can drop an API URL in my browser address bar (which I now realize uses a saved basic auth password). Fair enough, thanks for clarifying. Looking into other options.. maybe there is a less ugly way to do it with AJAX (stream to the localStorage API or something).

@martinpovolny
Copy link
Contributor Author

the '<a ...>' would work if the data was served from the UI worker which would need the assumption that both run on the same appliance.

Another variant is to have 2 API actions, one that generates a one-time token, and second one (w/o auth) that based on the on-time token returns the data. But for that I would need to store the token somewhere.

Storing it in the database would require a migration and therefor would be a problem with Ivanchuk. Maybe I could just keep it in the memory.

Or I could encrypt the information about the task_id in the token and some timestamp so that the validity of the token is limited....

@mturley
Copy link
Collaborator

mturley commented Jan 21, 2020

I'm happy with whatever solution works for you, but I think I figured out a way to handle this via a regular AJAX API call if you want to keep the backend simple.

I just realized we are already using https://github.com/eligrey/FileSaver.js in v2v to download migration logs via the API. Under the hood it does what you were suggesting (a data: link using the createObjectURL API) but it doesn't insert that into the DOM, so it's a bit more efficient.. it does still require temporarily storing the payload in the browser's RAM, but from what I can find, people have had no problems doing that on the order of hundreds of MB or more (https://www.quora.com/What-is-the-maximum-size-of-a-JavaScript-object-in-browser-memory, https://stackoverflow.com/a/12163588), so maybe that's fine.

Alternatively, there is https://github.com/jimmywarting/StreamSaver.js which has polyfills and doesn't have that RAM restriction. I could make it work via AJAX if we want to.

@martinpovolny
Copy link
Contributor Author

martinpovolny commented Jan 22, 2020

I just realized we are already using https://github.com/eligrey/FileSaver.js in v2v to download migration logs via the API.

Thank you for this information. I did not know that.

If it has been already decided to use this approach in the project I am not going to oppose that. I'll do it the same.

Can you, please, point me to the place? I am not familiar with the codebase and grep FileSaver did not give me any hit.

@mturley
Copy link
Collaborator

mturley commented Jan 22, 2020

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants