-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Convert GET to POST requests #140
Conversation
@Pearl1594
|
@Pearl1594 you probably don't want to do this as a default, we need to check if mgmt server logs API requests when things are POST-ed. You can probably add conditionals for new user-data related APIs. @hrak did you face issues when using the deploy VM API or other user-data related APIs? |
@weizhouapache @rohityadavcloud I was just testing to see if there is any issue with using POST for all commands. Hence raised this draft PR. |
cloudstack-go also uses POST by default for both DeployVirtualMachine and UpdateVirtualMachine, but i wouldn't use POST for everything as this PR seems to do. |
Yes, i can't submit userdata > 4K using Cloudmonkey. |
@Pearl1594 isn't deploy VM using POST in cmk? |
@rohityadavcloud No, deployVM is a GET request. I'll identify the commands that use POST and update the PR, that said, all operations seem to work fine using post |
@Pearl1594 I suppose a simple fix could be something like if the request size is say > 2048 or > 4096 then use POST, otherwise use GET. One thing we must check if when we use POST APIs, does cloudstack log them (for debugging/support purposes). |
or use POST if |
4133f08
to
9f81008
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, haven't tested it though
@hrak |
Can we get a config item for the user to choose POST or not e.g. |
FYI - I tested this out and am getting auth errors via keys if I try to call
I did not run in debug mode so I didn't capture that output, but retrying without the userdata worked. Then trying to |
@mlsorensen can you share a test userdata string/content for us to test/reproduce it? Could you also check mgmt server logs (share the same)? It could be a limitation/issue in cmk or backend mgmt server. |
@DaanHoogland @Pearl1594 is there a way you could test against large userdata input string to see how it behaves wrt post vs get request? |
after a short discuss off(this)line with @Pearl1594 , the performance is not very relevant as with only 2kb of url length userdata is going to be too long easily. If we wish we can implement a flag to try get first or a flag to use get (or post) for deploymachine. I think this is out of scope for this PR, though. |
liberally stealing from my chat with @Pearl1594;
after:
|
Fixes #139