KVM: Improvements on upload direct download certificates by nvazquez · Pull Request #2995 · apache/cloudstack

nvazquez · 2018-11-01T18:51:53Z

Description

There are some issues reported on the direct download feature for KVM:

The API method 'uploadTemplateDirectDownloadCertificate' always returns 'success:true' despite the ceritificate has been properly uploaded or not
The certificate provided as parameter on 'uploadTemplateDirectDownloadCertificate' is not verified before being sent to hosts. This causes that hosts could not set up the certificates and fail the operation.

To tackle these issues, several improvements for the KVM direct download feature have been introduced:

Perform sanity check before sending certificate to hosts:
- Verify the provided certificate is valid (right format) and not expired
Report cases of failure when uploading certificates to hosts.
- Return 'success:false' if the certificate cannot be uploaded to the hosts and log the issue properly
Not issue-related improvements
- Move the KVM direct download logic to KVM code
- Add marvin tests

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)

Screenshots (if appropriate):

How Has This Been Tested?

2 KVM hosts

Upload valid certificate, verify certificate is imported on both hosts -> OK
Upload invalid certificate -> ERROR
Upload existing alias certificate -> ERROR
Remove certificates
Try direct download

yadvr · 2018-11-02T04:32:07Z

@nvazquez please check build failure

nvazquez · 2018-11-05T00:40:25Z

Done thanks @rhtyd
@blueorangutan package

blueorangutan · 2018-11-05T00:40:44Z

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

blueorangutan · 2018-11-05T01:13:02Z

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

nvazquez · 2018-11-05T01:31:50Z

@blueorangutan test

blueorangutan · 2018-11-05T01:32:45Z

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

DaanHoogland

code looks ok

borisstoyanov · 2018-11-08T13:07:58Z

@blueorangutan test

borisstoyanov

LGTM

blueorangutan · 2018-11-08T13:09:00Z

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

blueorangutan · 2018-11-08T20:39:15Z

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

Test	Result	Time (s)	Test File

nvazquez · 2018-12-02T22:02:24Z

Thanks @rhtyd, I moved the other improvement to separate PR #3075

nvazquez · 2018-12-05T12:25:51Z

@rhtyd @borisstoyanov are we ok to merge this one?

borisstoyanov

Yes LGTM @nvazquez

yadvr · 2018-12-06T08:36:58Z

Let me get back to you on this @nvazquez by tomorrow.

yadvr

I think there is some technical debt introduced with the feature, we probably need a central place i.e. the control plane (mgmt server+db) to store certificates so the authorized clients (ssvm+kvm agent) can use them to download it. Likely provisionCertificate API can do this not just for the ca issued cert but other CA certificates added to the system to all ssvm/cpvm/kvm agents.

nvazquez · 2018-12-07T19:51:11Z

@rhtyd I have done it but then spread it into separate PR as requested. Please check #3075. It would need to be placed on top of this one

rafaelweingartner · 2019-06-02T19:22:21Z

+ throw new CloudRuntimeException("Certificate is invalid. Please provide a valid certificate. Error: " + e.getMessage());
+ }
+ if (x509Cert.getSubjectDN() != null) {
+ s_logger.info("Valid certificate for domain name: " + x509Cert.getSubjectDN().getName());


This looks more like a debug message.
BTW, what does this message mean?
If the domain name is not null, then you call it valid? That sounds a bit odd. Could you explain that a bit further?

Actually it is valid if it succeeds the checkValidity() method above. Message is just informative, at this point in both cases (domain name null or not) the certificate is valid. Anyways I'll set it to debug level, or if its confusing then it can be removed as well

nvazquez · 2019-06-03T00:05:03Z

Rebased branch 4.11 and adressed last review comments, thanks @rafaelweingartner!

nvazquez · 2019-06-03T02:46:40Z

@blueorangutan package

blueorangutan · 2019-06-03T02:47:44Z

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

blueorangutan · 2019-06-03T03:08:21Z

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

nvazquez · 2019-06-03T03:09:54Z

@blueorangutan test

blueorangutan · 2019-06-03T03:10:44Z

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

blueorangutan · 2019-06-03T12:44:31Z

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

Test	Result	Time (s)	Test File

nvazquez · 2019-06-03T14:18:04Z

@rafaelweingartner are you ok with this one after comments addressed?

nvazquez · 2019-06-03T21:09:46Z

@blueorangutan package

blueorangutan · 2019-06-03T21:10:47Z

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

blueorangutan · 2019-06-03T21:31:27Z

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

nvazquez · 2019-06-03T21:34:35Z

@blueorangutan test

blueorangutan · 2019-06-03T21:34:47Z

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

blueorangutan · 2019-06-04T05:54:25Z

Trillian test result (tid-3640)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 28211 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2995-t3640-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Smoke tests completed. 69 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

nvazquez requested review from DaanHoogland and yadvr November 1, 2018 18:54

nvazquez force-pushed the certificatesenhacements branch from 906278b to 0df65a8Compare November 1, 2018 19:06

nvazquez force-pushed the certificatesenhacements branch from 0df65a8 to 54fe518Compare November 5, 2018 00:36

DaanHoogland approved these changes Nov 5, 2018
View reviewed changes

Comment threadagent/src/com/cloud/agent/Agent.java Outdated
Comment threadagent/src/com/cloud/agent/Agent.java Outdated
Comment threadagent/src/com/cloud/agent/Agent.java Outdated

nvazquez requested review from GabrielBrascher and rafaelweingartner November 7, 2018 12:27

borisstoyanov approved these changes Nov 8, 2018
View reviewed changes

yadvr modified the milestones: 4.11.2.0, 4.11.3.0 Nov 12, 2018

nvazquez added the status:work-in-progress label Nov 13, 2018

yadvr reviewed Nov 28, 2018
View reviewed changes

Comment threadengine/schema/resources/META-INF/db/schema-41110to41120.sql Outdated

nvazquez force-pushed the certificatesenhacements branch from 5b34b8f to f5eecc7Compare December 2, 2018 22:01

nvazquez removed the status:work-in-progress label Dec 5, 2018

borisstoyanov approved these changes Dec 5, 2018
View reviewed changes

yadvr reviewed Dec 7, 2018
View reviewed changes

rafaelweingartner reviewed Jun 2, 2019
View reviewed changes

nvazquez added 5 commits June 2, 2019 20:34

Improvements on upload direct download certificates
6f0d898

Move upload direct download certificate logic to KVM plugin
547b008

Extend unit test certificate expiration days
fc9e153

Add marvin tests and command to revoke certificates
efe35d3

Review comments
f953268

nvazquez force-pushed the certificatesenhacements branch from 86abc1c to f953268Compare June 3, 2019 00:04

yadvr approved these changes Jun 3, 2019
View reviewed changes

nvazquez mentioned this pull request Jun 3, 2019
KVM: Prevent regenerating keystore on provisionCertificate API #3075
Merged
5 tasks

rafaelweingartner approved these changes Jun 3, 2019
View reviewed changes

Do not include revoke certificates API
49b24ee

nvazquez force-pushed the certificatesenhacements branch from 683dcc3 to 49b24eeCompare June 3, 2019 21:09

anuragaw approved these changes Jun 4, 2019
View reviewed changes

nvazquez merged commit 12c850e into apache:4.11Jun 4, 2019

nvazquez mentioned this pull request Jun 5, 2019
KVM: Enhancements for direct download feature #3374
Merged
5 tasks

nvazquez deleted the certificatesenhacements branch April 6, 2020 14:45

Uh oh!

Conversation

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

Uh oh!

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

yadvr commented Nov 2, 2018

Uh oh!

nvazquez commented Nov 5, 2018

Uh oh!

blueorangutan commented Nov 5, 2018

Uh oh!

blueorangutan commented Nov 5, 2018

Uh oh!

nvazquez commented Nov 5, 2018

Uh oh!

blueorangutan commented Nov 5, 2018

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

borisstoyanov commented Nov 8, 2018

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Nov 8, 2018

Uh oh!

blueorangutan commented Nov 8, 2018

Uh oh!

Uh oh!

nvazquez commented Dec 2, 2018

Uh oh!

nvazquez commented Dec 5, 2018

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Dec 6, 2018

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

nvazquez commented Dec 7, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rafaelweingartnerJun 2, 2019

Choose a reason for hiding this comment

Uh oh!

nvazquezJun 2, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nvazquez commented Jun 3, 2019

Uh oh!

nvazquez commented Jun 3, 2019

Uh oh!

blueorangutan commented Jun 3, 2019

Uh oh!

blueorangutan commented Jun 3, 2019

Uh oh!

nvazquez commented Jun 3, 2019

Uh oh!

blueorangutan commented Jun 3, 2019

Uh oh!

blueorangutan commented Jun 3, 2019

Uh oh!

nvazquez commented Jun 3, 2019

Uh oh!

nvazquez commented Nov 1, 2018•
edited
Loading