ApiServer: signature v3 to accept more formats by greut · Pull Request #2893 · apache/cloudstack

greut · 2018-10-10T11:48:31Z

Description

The format expected by signatureVersion=3&expires=.... is quite limited.

It should accepts the following formats that are containing a timezone and/or milliseconds.

2018-10-01T08:12:14Z
2018-10-01T08:12:14+01:00
2018-10-01T08:12:14+0100
2018-10-01T08:12:14.000Z
2018-10-01T08:12:14.000+01:00
2018-10-01T08:12:14.000+0100

afaik only 2018-10-01T08:12:14+0100 is accepted by the current codebase.

This PR echoes another pull requests I made earlier this year. #2392 and #2867

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)

GitHub Issue/PRs

n/a

Screenshots (if appropriate):

n/a

How Has This Been Tested?

the Java unit tests. If anyone want to help with the marvin work... cs does it already.

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Testing
I have added tests to cover my changes.
All relevant new and existing integration tests have passed.
A full integration testsuite with all test that can run on my environment has passed.

It does it by reusing the DateUtil helpers. DateUtil uses java.time.* as that one knows how to deal with timezones correctly. Signed-off-by: Yoan Blanc <yoan.blanc@exoscale.ch>

yadvr · 2018-10-16T05:38:41Z

@blueorangutan package

blueorangutan · 2018-10-16T05:39:00Z

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

blueorangutan · 2018-10-16T06:12:01Z

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

rafaelweingartner · 2018-10-17T23:05:28Z

@blueorangutan package

blueorangutan · 2018-10-17T23:06:07Z

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

blueorangutan · 2018-10-17T23:35:12Z

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

yadvr · 2018-10-18T05:42:21Z

@blueorangutan test

blueorangutan · 2018-10-18T05:43:10Z

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

blueorangutan · 2018-10-18T13:15:26Z

Trillian test result (tid-3082)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 25023 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2893-t3082-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_multipleips_per_nic.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
Smoke tests completed. 67 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_nic_secondaryip_add_remove	`Error`	23.71	test_multipleips_per_nic.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	486.43	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Failure`	328.47	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Error`	356.85	test_vpc_redundant.py

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

@blueorangutan test

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

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

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

@blueorangutan test

blueorangutan · 2018-10-20T16:38:20Z

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

yadvr · 2018-10-25T01:29:07Z

@blueorangutan package

blueorangutan · 2018-10-25T01:29:39Z

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

blueorangutan · 2018-10-25T02:18:47Z

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

DaanHoogland · 2018-10-29T09:36:32Z

@blueorangutan package

yadvr · 2018-10-29T10:11:20Z

@DaanHoogland sorry lab's down, BO queue will be stuck for a while.

yadvr · 2018-10-30T11:44:25Z

@blueorangutan package

blueorangutan · 2018-10-30T11:45:20Z

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

blueorangutan · 2018-10-30T12:23:10Z

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

yadvr · 2018-10-30T12:27:35Z

@blueorangutan package

blueorangutan · 2018-10-30T12:28:20Z

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

blueorangutan · 2018-10-30T12:59:57Z

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

yadvr · 2018-10-30T13:11:03Z

@blueorangutan test

blueorangutan · 2018-10-30T13:16:19Z

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

blueorangutan · 2018-10-30T20:09:08Z

Trillian test result (tid-3135)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 23046 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2893-t3135-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_multipleips_per_nic.py
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
Smoke tests completed. 66 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_nic_secondaryip_add_remove	`Error`	35.14	test_multipleips_per_nic.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	461.59	test_vpc_redundant.py
runTest	`Error`	0.00	test_vm_life_cycle.py

DaanHoogland

code looks good 👍

bwsw · 2018-11-05T02:21:19Z

@DaanHoogland @rhtyd @greut
Unit-tests buggy. Take a look:

@TestpublicvoidzonedTimeFormatIsoNoColonZMs() throwsParseException {
Datetime = newDate();
DateTimeFormatterformatter = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSSSSZ");
Stringstr = OffsetDateTime.now().format(formatter);
DatedtParsed = DateUtil.parseTZDateString(str);
assertEquals(str, time.toString(), dtParsed.toString());
}

This unittest is obviously buggy. Because there are two times when objects are used to get current DT, as a result, sometimes it fails.

https://travis-ci.org/apache/cloudstack/jobs/450628551#L1047

Need a codefix or PR reverted and fixed. PR was merged recently. Now CS fails to build sometimes.

bwsw · 2018-11-05T02:24:33Z

Other tests in this PR also must be revised on similar behaviors.

DaanHoogland · 2018-11-05T10:24:16Z

@bwsw You are right that these tests depend on hwclocktime and are not reliable. Any sugestions on how to fix without losing test-validity?

DaanHoogland · 2018-11-05T14:18:41Z

@bwsw a quick hack of mine:

 @Test
public void zonedTimeFormatIsoNoColonZMs() throws ParseException {
Date time = new Date();
DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSSSSZ");
String str = OffsetDateTime.of(time.getYear(),time.getMonth(),time.getDay(),time.getHours(),time.getMinutes(),time.getSeconds(),0,ZoneOffset.ofHours(time.getTimezoneOffset())).format(formatter);
Date dtParsed = DateUtil.parseTZDateString(str);
assertEquals(str, time.toString(), dtParsed.toString());
}

ugly quick hack but should almost do what is intended without losing test quality or time skew bug as you reported. not sure how the nanoseconds must be handled yet.
thoughts?

bwsw · 2018-11-05T14:21:39Z

@DaanHoogland Looks good to me. May be imperfect, but should work, at least.

DaanHoogland · 2018-11-05T14:27:43Z

well @bwsw the code needs debugging still.

DaanHoogland · 2018-11-05T14:49:46Z

strangely this works:

 @Test
public void zonedTimeFormatIsoNoColonZMs() throws ParseException {
Date time = new Date();
DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSSSSZ");
String str = OffsetDateTime.of(time.getYear()+1900,time.getMonth()+1,time.getDate(),time.getHours(),time.getMinutes(),time.getSeconds(),0,ZoneOffset.ofHours(-1*time.getTimezoneOffset()/60)).format(formatter);
Date dtParsed = DateUtil.parseTZDateString(str);
assertEquals(str, time.toString(), dtParsed.toString());
}

not sure if it is valid in all cases though, can you test @bwsw
@nvazquez could you test from the western hemisphere, please?

nvazquez · 2018-11-05T15:18:34Z

Tested on latest master: first tested running every test and passed successfully. Then replaced the last one for the code @DaanHoogland provided above and also succeeded

bwsw · 2018-11-05T16:24:33Z

@DaanHoogland
@nvazquez
There are five tests like the one above in that unit test module. All of them must be reworked. I think it's either to revert PR or to open a separate issue for fixing each of them.

DaanHoogland · 2018-11-05T16:29:51Z

@bwsw #3003 contains what I think is enough, let's discuss further overthere

ApiServer: signature v3 to accept more formats
c2b4785
It does it by reusing the DateUtil helpers. DateUtil uses java.time.* as that one knows how to deal with timezones correctly. Signed-off-by: Yoan Blanc <yoan.blanc@exoscale.ch>

rafaelweingartner assigned rafaelweingartner and greut and unassigned rafaelweingartner Oct 15, 2018

rafaelweingartner added the type:enhancement label Oct 15, 2018

rafaelweingartner added this to the 4.12.0.0 milestone Oct 15, 2018

rafaelweingartner approved these changes Oct 15, 2018
View reviewed changes

yadvr mentioned this pull request Oct 16, 2018
params: allow signatureversion and expires without logging #2901
Closed
5 tasks

pyr approved these changes Oct 17, 2018
View reviewed changes

DaanHoogland approved these changes Oct 31, 2018
View reviewed changes

yadvr approved these changes Oct 31, 2018
View reviewed changes

yadvr merged commit 17c164d into apache:masterOct 31, 2018

bwsw mentioned this pull request Nov 5, 2018
CLOUDSTACK-2998: Added two more device name patterns to valid bridge slaves (lo* and dummy*) #3000
Merged
5 tasks

greut deleted the accept-iso-datetime-containing-z branch November 5, 2018 10:29

Uh oh!

Conversation

greut commented Oct 10, 2018

Description

Types of changes

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

Uh oh!

yadvr commented Oct 16, 2018

Uh oh!

blueorangutan commented Oct 16, 2018

Uh oh!

blueorangutan commented Oct 16, 2018

Uh oh!

rafaelweingartner commented Oct 17, 2018

Uh oh!

blueorangutan commented Oct 17, 2018

Uh oh!

blueorangutan commented Oct 17, 2018

Uh oh!

yadvr commented Oct 18, 2018

Uh oh!

blueorangutan 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 25, 2018

Uh oh!

blueorangutan commented Oct 25, 2018

Uh oh!

blueorangutan commented Oct 25, 2018

Uh oh!

DaanHoogland commented Oct 29, 2018

Uh oh!

yadvr commented Oct 29, 2018

Uh oh!

yadvr commented Oct 30, 2018

Uh oh!

blueorangutan commented Oct 30, 2018

Uh oh!

blueorangutan commented Oct 30, 2018

Uh oh!

yadvr commented Oct 30, 2018

Uh oh!

blueorangutan commented Oct 30, 2018

Uh oh!

blueorangutan commented Oct 30, 2018

Uh oh!

yadvr commented Oct 30, 2018

Uh oh!

blueorangutan commented Oct 30, 2018

Uh oh!

blueorangutan commented Oct 30, 2018

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

bwsw commented Nov 5, 2018

Uh oh!

bwsw commented Nov 5, 2018

Uh oh!

DaanHoogland commented Nov 5, 2018

Uh oh!

DaanHoogland commented Nov 5, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bwsw commented Nov 5, 2018

Uh oh!

DaanHoogland commented Nov 5, 2018

Uh oh!

DaanHoogland commented Nov 5, 2018•
edited
Loading