Skip to content

Don't allow service offering change if encryption value would change#6776

Merged
yadvr merged 3 commits intoapache:mainfrom
mlsorensen:main-match-serviceoffering-encryption
Oct 7, 2022
Merged

Don't allow service offering change if encryption value would change#6776
yadvr merged 3 commits intoapache:mainfrom
mlsorensen:main-match-serviceoffering-encryption

Conversation

@mlsorensen
Copy link
Copy Markdown
Contributor

Signed-off-by: Marcus Sorensen mls@apple.com

Description

This PR blocks change of service offering if the offering root volume encryption values don't match. We don't support dynamically removing or adding encryption to a VM.

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Tested prior to this change - could move from non-encrypted to encrypted offerings.

Tested after this change - clean error indicating the encryption type must not change if selected offering does not match current offering encryption setting. Tested both compute-only disk offering and service offering referencing existing disk offerings.

Tested:

  • Service offering with compute-only disk offerings
  • Service offering with compute referencing disk offerings
  • Trying to go from encrypted to non-encrypted
  • Trying to go from non-encrypted to encrypted
  • VM referencing deleted disk offering still correctly blocks move to offering with different encryption setting

Screen Shot 2022-09-27 at 2 19 45 PM

Signed-off-by: Marcus Sorensen <mls@apple.com>
@mlsorensen mlsorensen changed the title Don't service offering if encryption value would change Don't allow service offering change if encryption value would change Sep 27, 2022
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 27, 2022

Codecov Report

Merging #6776 (4363ef2) into main (713a236) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #6776      +/-   ##
============================================
- Coverage     10.60%   10.60%   -0.01%     
- Complexity     6849     6852       +3     
============================================
  Files          2466     2466              
  Lines        244549   244553       +4     
  Branches      38262    38263       +1     
============================================
  Hits          25936    25936              
- Misses       215331   215335       +4     
  Partials       3282     3282              
Impacted Files Coverage Δ
.../src/main/java/com/cloud/vm/UserVmManagerImpl.java 6.71% <100.00%> (+0.10%) ⬆️
...rg/apache/cloudstack/quota/QuotaStatementImpl.java 36.28% <0.00%> (-3.99%) ⬇️
...om/cloud/hypervisor/kvm/resource/LibvirtVMDef.java 64.15% <0.00%> (ø)
...apache/cloudstack/syslog/AlertsSyslogAppender.java 58.75% <0.00%> (+2.25%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Sep 28, 2022

@mlsorensen can you add a unit or marvin test for the same if feasible?
@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

@rohityadavcloud a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link
Copy Markdown

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 4301

@mlsorensen
Copy link
Copy Markdown
Contributor Author

@rohityadavcloud I'm really not sure how to do that. I'm aware of the smoke tests but as far as understanding what is actually going to be run on PR, and how to actually get a local environment to develop against (Marvin seems to be pretty heavy handed about setting up zones and networking in a specific way during setup for a test, for example I can't just pick a VM test and run it against any existing zone using existing networks and offerings... or can I?). Honestly we would add dozens of tests if we understood how the target environment works and it were easy to iterate on.

@mlsorensen
Copy link
Copy Markdown
Contributor Author

I can probably add a unit test for this since the method is isolated... working.

Signed-off-by: Marcus Sorensen <mls@apple.com>
@sonarqubecloud
Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

73.3% 73.3% Coverage
0.0% 0.0% Duplication

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Sep 29, 2022

@mlsorensen you can for example write a simple marvin test to (1) create an offering with encryption enabled, (2) deploy VM using that, (3) stop the VM if necessary and try changing its offering to one of the available default offerings that don't have an encryption enabled. Since this test is checking only the business logic, it may even run as a simulator test (and therefore also on Travis), and very easy to run smoketests against simulator (https://github.com/shapeblue/hackerbook/blob/main/2-dev.md#simulator-based-development).

To run a local (kvm/vmware/xen) env, you can deploy them as VMs on your workstations (or remote server with your workstation joined over VPN), or use something like https://github.com/shapeblue/mbx to tests using built pkgs.

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Sep 29, 2022

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

@rohityadavcloud a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link
Copy Markdown

Packaging result: ✖️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 4310

@blueorangutan
Copy link
Copy Markdown

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 4316

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Sep 29, 2022

@blueorangutan test

@blueorangutan
Copy link
Copy Markdown

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

@blueorangutan
Copy link
Copy Markdown

Trillian Build Failed (tid-5050)

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Sep 29, 2022

@blueorangutan test

@blueorangutan
Copy link
Copy Markdown

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

@mlsorensen
Copy link
Copy Markdown
Contributor Author

@mlsorensen you can for example write a simple marvin test to (1) create an offering with encryption enabled, (2) deploy VM using that, (3) stop the VM if necessary and try changing its offering to one of the available default offerings that don't have an encryption enabled. Since this test is checking only the business logic, it may even run as a simulator test (and therefore also on Travis), and very easy to run smoketests against simulator (https://github.com/shapeblue/hackerbook/blob/main/2-dev.md#simulator-based-development).

To run a local (kvm/vmware/xen) env, you can deploy them as VMs on your workstations (or remote server with your workstation joined over VPN), or use something like https://github.com/shapeblue/mbx to tests using built pkgs.

Cool, thanks @rohityadavcloud. I'm aware of these (we use simulator quite a bit w/containers) but the gaps are elsewhere, maybe we can meet and discuss.

One is really around environment overload. I already have several environments I maintain for development that represent real configurations. It's a bit cumbersome to then set up yet another VM instance as a blank slate for each dev cycle of smoke testing (or automate it), I'm not aware of how to run Marvin tests against an existing dev environment since many of the tests make assumptions about the environment. Likewise I don't really understand the environment that is under test by Trillian and how to reproduce it in a way that I can develop tests which will work there.

The other is understanding of which tests actually will be run in the community on every PR vs tests that aren't and which tests are run when. It looked like maybe there are some decorators that manage this.

I'd love to just be able to run mvn test -Dfunctests or similar and everything that would normally run during PR gets tested, even if that includes starting up a java instance and some containers.

@blueorangutan
Copy link
Copy Markdown

Trillian test result (tid-5054)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40643 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6776-t5054-kvm-centos7.zip
Smoke tests completed. 103 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Oct 5, 2022

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

Copy link
Copy Markdown
Member

@yadvr yadvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit test LGTM

@yadvr yadvr requested a review from shwstppr October 7, 2022 05:33
@yadvr yadvr added this to the 4.18.0.0 milestone Oct 7, 2022
@yadvr yadvr merged commit 93f0926 into apache:main Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants