params: allow signatureversion and expires without logging by pyr · Pull Request #2901 · apache/cloudstack

pyr · 2018-10-15T12:17:26Z

Description

This patch considers the new expires and signatureversion parameters
valid. Without this, all calls log when using the V3 signature scheme.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

This patch considers the new expires and signatureversion parameters valid. Without this, all calls log when using the V3 signature scheme.

rafaelweingartner · 2018-10-15T12:52:56Z

Has your other PR been merged already?

pyr · 2018-10-15T12:53:55Z

@rafaelweingartner which other PR are you referring to?

pyr · 2018-10-15T12:55:31Z

I'm not sure I understand the Jenkins failure but it seems unrelated to this PR

rafaelweingartner · 2018-10-15T12:59:37Z

You are saying that these parameters are always logged as not valid ones. Therefore, I was assuming that they are somehow new parameters that were introduced by you.

It is Monday morning, my brain might not be working yet.

P.S. I restarted Jenkins manually.

pyr · 2018-10-15T13:02:41Z

@rafaelweingartner I did not introduce the new parameters, they were introduced as part of the Signature version 3 scheme in the ApiServer class.

pyr · 2018-10-15T13:31:05Z

@rafaelweingartner provided I find where to push again, how many approvals should I wait for before merging? (it's been a while since I pushed to the project)

rafaelweingartner · 2018-10-15T13:31:52Z

Two positive feedbacks and successful functional tests executed

yadvr · 2018-10-16T05:03:48Z

is this the PR #2893 in question @pyr ? If so, let's review and merge #2893 before this PR.

pyr · 2018-10-16T05:58:41Z

@rhtyd nope, this is unrelated to #2893, signature version 3 parameters are already logged

yadvr · 2018-10-16T06:06:21Z

@blueorangutan package

blueorangutan · 2018-10-16T06:06:59Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-10-16T06:34:28Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2348

pyr · 2018-10-16T07:01:45Z

Thanks for the review!

yadvr · 2018-10-16T10:19:46Z

@pyr we've not tested/merged your PR, we need 2 lgtms just for qualifying, we also need testing. Do you still want the PR -- in which case please re-open the PR.

pyr · 2018-10-17T20:06:28Z

@rhtyd Hi rohit, this wasn't clear to me, I went ahead and committed assuming what @rafaelweingartner mentioned was sufficient, I'm happy to revert if you think that is warranted.

rafaelweingartner · 2018-10-17T20:53:29Z

@pyr somebody has to merge it. Your PR was not merged, and your commit is not in our code base yet.

What do you mean with "went ahead and committed"?

pyr · 2018-10-18T02:21:36Z

@rafaelweingartner I rebased on master and pushed, 58b4e71 is on master now.

yadvr · 2018-10-18T05:36:40Z

@pyr this is not acceptable, you merged a commit on master directly without the PR id and without any testing results on them. Your change may therefore potentially break master for others. Instead of closing or pushing the change, you should have reopened this PR or create a new PR and wait for test results, even though we requested you to re-open your PR. If things were not clear you could discuss and/or ask questions but you committed the change on master. Also now the PR status is closed and not merged this will cause search/audit issues in future.
You may see how other PRs get reviewed, tested and (squash) merged.

yadvr · 2018-10-18T05:37:09Z

@blueorangutan package

blueorangutan · 2018-10-18T05:38:09Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-10-18T06:20:01Z

Packaging result: ✔centos6 ✔centos7 ✖debian. JID-2353

pyr · 2018-10-18T06:43:25Z

@rhtyd Sorry I misinterpreted @rafaelweingartner's instruction. As I mentioned, I'm happy top revert the change. I reopened the PR. My recollection of the workflow is from back when PRs could not be merged against this repo directly.

yadvr · 2018-10-18T07:25:38Z

@blueorangutan test

blueorangutan · 2018-10-18T07:26:09Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

yadvr · 2018-10-18T07:28:30Z

@pyr no need to revert now as it would make git history ugly, I've kicked testing now so let's wait and act accordingly. I'm sure your changes don't break anything, we still need testing as a matter of process to ensure stability. Once testing is OK, you may close this PR as the change is already on master.

rafaelweingartner · 2018-10-18T11:55:31Z

@pyr, @rhtyd is right. If you are a committer, you should have done the merge via Github. Moreover, we need the functional tests. When you pushed your changes, there was no functional testes results.

pyr · 2018-10-18T12:06:35Z

@rafaelweingartner I don't have the merge option on Github, hence I assumed the pre-github workflow prevailed. Again, apologies and all.

rafaelweingartner · 2018-10-18T12:08:13Z

@pyr then you need to follow the "signup" process for the Gitbox experiment.

blueorangutan · 2018-10-18T18:19:48Z

Trillian test result (tid-3083)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37173 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2901-t3083-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_multipleips_per_nic.py
Intermittent failure detected: /marvin/tests/smoke/test_public_ip_range.py
Intermittent failure detected: /marvin/tests/smoke/test_ssvm.py
Intermittent failure detected: /marvin/tests/smoke/test_templates.py
Intermittent failure detected: /marvin/tests/smoke/test_usage.py
Intermittent failure detected: /marvin/tests/smoke/test_volumes.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 62 look OK, 7 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_04_rvpc_internallb_haproxy_stats_on_all_interfaces	`Error`	171.28	test_internal_lb.py
test_nic_secondaryip_add_remove	`Error`	27.75	test_multipleips_per_nic.py
test_09_destroy_ssvm	`Failure`	919.20	test_ssvm.py
test_04_extract_template	`Failure`	128.31	test_templates.py
ContextSuite context=TestISOUsage>:setup	`Error`	0.00	test_usage.py
test_06_download_detached_volume	`Failure`	137.66	test_volumes.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	441.47	test_vpc_redundant.py

yadvr · 2018-10-18T22:29:33Z

@blueorangutan test

blueorangutan · 2018-10-18T22:30:11Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

yadvr · 2018-10-20T16:38:36Z

Deployment/setup failed on testing, retrying again
@blueorangutan test

blueorangutan · 2018-10-20T16:39:19Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

yadvr · 2018-10-24T20:57:11Z

Checked master, there are some intermittent failures but no new/major failure. I'll close the PR as the commit already is on master 58b4e71.

params: allow signatureversion and expires without logging

0c6fab6

This patch considers the new expires and signatureversion parameters valid. Without this, all calls log when using the V3 signature scheme.

rafaelweingartner approved these changes Oct 15, 2018

View reviewed changes

yadvr added this to the 4.12.0.0 milestone Oct 16, 2018

yadvr added the component:api label Oct 16, 2018

yadvr approved these changes Oct 16, 2018

View reviewed changes

pyr closed this Oct 16, 2018

pyr deleted the fix/param-validation branch October 16, 2018 07:01

pyr restored the fix/param-validation branch October 18, 2018 06:46

pyr reopened this Oct 18, 2018

yadvr closed this Oct 24, 2018

Conversation

pyr commented Oct 15, 2018

Description

Types of changes

Uh oh!

rafaelweingartner commented Oct 15, 2018

Uh oh!

pyr commented Oct 15, 2018

Uh oh!

pyr commented Oct 15, 2018

Uh oh!

rafaelweingartner commented Oct 15, 2018

Uh oh!

pyr commented Oct 15, 2018

Uh oh!

pyr commented Oct 15, 2018

Uh oh!

rafaelweingartner commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yadvr commented Oct 16, 2018

Uh oh!

pyr commented Oct 16, 2018

Uh oh!

yadvr commented Oct 16, 2018

Uh oh!

blueorangutan commented Oct 16, 2018

Uh oh!

blueorangutan commented Oct 16, 2018

Uh oh!

pyr commented Oct 16, 2018

Uh oh!

yadvr commented Oct 16, 2018

Uh oh!

pyr commented Oct 17, 2018

Uh oh!

rafaelweingartner commented Oct 17, 2018

Uh oh!

pyr commented Oct 18, 2018

Uh oh!

yadvr commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yadvr commented Oct 18, 2018

Uh oh!

blueorangutan commented Oct 18, 2018

Uh oh!

blueorangutan commented Oct 18, 2018

Uh oh!

pyr commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yadvr commented Oct 18, 2018

Uh oh!

blueorangutan commented Oct 18, 2018

Uh oh!

yadvr commented Oct 18, 2018

Uh oh!

rafaelweingartner commented Oct 18, 2018

Uh oh!

pyr commented Oct 18, 2018

Uh oh!

rafaelweingartner commented Oct 18, 2018

Uh oh!

blueorangutan commented Oct 18, 2018

Uh oh!

yadvr commented Oct 18, 2018

Uh oh!

blueorangutan commented Oct 18, 2018

Uh oh!

yadvr commented Oct 20, 2018

Uh oh!

blueorangutan commented Oct 20, 2018

Uh oh!

yadvr commented Oct 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

rafaelweingartner commented Oct 15, 2018 •

edited

Loading

yadvr commented Oct 18, 2018 •

edited

Loading

pyr commented Oct 18, 2018 •

edited

Loading