Skip to content

Conversation

@soreana
Copy link
Member

@soreana soreana commented Mar 9, 2021

Description

As a cloud provider, we don't want our customers to see other templates. This pr limits template access to the domain.

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

How Has This Been Tested?

To test this feature, I created two domains named Test0, Test1 and Test2, each with their respective domain admins (test0, test1 and test2).
I used cloudmonkey command to list different combination of templateFilters ( all, featured, self, selfexecutable, sharedexecutable, executable and community ) and accounts ( admin, test0, test1 and test2 ).

Pre configuration:

  1. Create three domain with their respective domain admin accounts.
  2. Register new template for each account like the following table.
  3. Follow the test cases.
Owner Templates
Admin U20, SystemVM
Test0 U0
Test1 U1
Test2 U2

Test case one.

  1. Set share.public.templates to false for every domain
  2. List templates in every account. You can't see any templates except the owneres templates.

Test case two

  1. Set share.public.templates like the following table.
  2. List templates in test0 account. You should see combination of the U20, U0, U2 with different templatefilter but not U1
Owner share.public.templates
Admin true
Test0 Doesn't matter
Test1 false
Test2 true

Test case three

  1. Set share.public.templates like the test case two
  2. List templates in test0 with template id. You should be ablet to see the U20, U0, U2 but empty result if you use U1 id.

I wrote this script to test this pr, you can find it in the following link. You need the cmk command and you should put admin, test1, and test2 users info in the cmk configuration file. How to run this?

  • Put account names in accounts array defined at top of the script
  • ./listTemplates.sh will list all filter for all accounts
  • ./listTemplates.sh list all templates using all possible filters for
  • ./listTemplates.sh list all templates for accounts in ('admin' 'test' 'test2') using
  • ./listTemplates.sh list all templates for using

@harikrishna-patnala
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

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

@blueorangutan
Copy link

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2890

@harikrishna-patnala
Copy link
Contributor

harikrishna-patnala commented Mar 9, 2021

@soreana, does this have any impact on upgrades, existing templates in the environment might have already shared and used across domains. Does a domain lose viewing or accessing the templates which are already used when the configuration parameter is set to true.

@blueorangutan
Copy link

Packaging result: ✖️ centos7 ✖️ centos8 ✔️ debian. SL-JID 59

@blueorangutan
Copy link

Packaging result: ✔️ centos7 ✖️ centos8 ✔️ debian. SL-JID 63

@soreana
Copy link
Member Author

soreana commented Mar 9, 2021

@harikrishna-patnala I didn't noticed any issue. Here is what I tested today.

  1. Set the global setting to false (templates shared between domains).
  2. Created the instance (named INSTANCE) in test2 domain using Debian template (which belongs to test1 domain)
  3. Set the global setting to false (templates shared between domains).
  4. Debian Template was removed from the template list.
  5. I tested everything related to INSTANCE including: VNC console, Migration, Stop, Start, Reboot, Snapshot
  6. I found nothing.

Let me know if you have other concerns.

Btw, while I tried to reinstall the INSTANCE I lost access to Debian template which is legitimate behaviour. See attached.

Without restriction (global setting sets to false)

Screenshot 2021-03-09 at 19 33 44

With restriction (global setting sets to true)

Screenshot 2021-03-09 at 19 33 16

@harikrishna-patnala
Copy link
Contributor

@soreana I have raised the question exactly like Reinstall VM you mentioned above, loosing access to the template which are already in use. May be keeping the configuration scope to Domain level will give flexibility in allowing access in these cases.

Let's hear from others too. Thanks.

@DaanHoogland DaanHoogland added this to the 4.16.0.0 milestone Mar 11, 2021
@DaanHoogland
Copy link
Contributor

Not that I want to intrude on your discussion @soreana @harikrishna-patnala , but would it make sense to add a scope to the filter:

all, featured, self, selfexecutable, sharedexecutable, executable, community, domain

@soreana
Copy link
Member Author

soreana commented Mar 11, 2021

@DaanHoogland It is nice feature. But it will not address access right issue fixed in this pr.

@soreana
Copy link
Member Author

soreana commented Mar 11, 2021

@harikrishna-patnala In regards to your Reinstall VM comment. If test1 user (who owned the template) makes it private, test2 user (who used the template) will lose access to the template and he/she can't see that template in the Reinstall VM list.

As a result, with or without these changes, the test2 user might lose access to the template. It is an inherent risk in using community templates. :D

Reinstall VM before making Debian template private

Screenshot 2021-03-11 at 10 47 24

Reinstall VM after making Debian template private

Screenshot 2021-03-11 at 10 43 05

@harikrishna-patnala
Copy link
Contributor

Thanks @soreana for testing further. If you can add scope to domain that will be great.

@soreana
Copy link
Member Author

soreana commented Mar 16, 2021

@harikrishna-patnala I updated the PR. I migrated the setting to the domain level. It is a limited code change required to do so :D. I tested that with subdomains as well. More details:

Root (Ubuntu)
  |-- domain0 (Debian)
  |       |-- sub0 (Fedora)
  |       |-- sub1
  |-- domain1

If you set restrict.public.template.access.to.domain to true for domain1, domain1 access to Debian and Fedora will be limited.

If you set restrict.public.template.access.to.domain to true for sub1, sub1 access to Fedora will be limited.

Let me know if you have other concerns.

P.S: I'm updating the test cases.

@nvazquez
Copy link
Contributor

Hi @soreana looks like the PR has a conflict, can you please resolve it?

@harikrishna-patnala
Copy link
Contributor

domain scope change looks good @soreana, please resolve the conflicts on the PR.

@soreana
Copy link
Member Author

soreana commented Jul 9, 2021

I fixed the conflict.
@nvazquez @harikrishna-patnala Sorry for the late response. As I was away for couple of weeks, I completely forgot about this.

@nvazquez
Copy link
Contributor

No problem, thanks @soreana
@blueorangutan package

@blueorangutan
Copy link

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

@blueorangutan
Copy link

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

@nvazquez
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

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

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

code looks generally good, needs testing and some remarks on the integration test

@blueorangutan
Copy link

Trillian test result (tid-1246)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 46884 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4774-t1246-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 84 look OK, 4 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
ContextSuite context=TestKubernetesCluster>:teardown Error 72.00 test_kubernetes_clusters.py
test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true Failure 396.74 test_routers_network_ops.py
test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false Failure 341.05 test_routers_network_ops.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL Failure 539.81 test_vpc_redundant.py
test_02_redundant_VPC_default_routes Failure 373.90 test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers Failure 549.32 test_vpc_redundant.py
test_05_rvpc_multi_tiers Failure 553.14 test_vpc_redundant.py
test_05_rvpc_multi_tiers Error 553.16 test_vpc_redundant.py
test_01_vpc_site2site_vpn_multiple_options Failure 305.87 test_vpc_vpn.py

@nvazquez
Copy link
Contributor

@soreana can you address the open comments on the marvin tests?

@blueorangutan
Copy link

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

@rohityadavcloud
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@rohityadavcloud a Jenkins job has been kicked to build packages. It will be bundled with

SystemVM template(s). I'll keep you posted as I make progress.

@blueorangutan
Copy link

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

@nvazquez
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@nvazquez 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

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

@nvazquez
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

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

@sureshanaparti
Copy link
Contributor

@sureshanaparti please advise if this is ready after your testing, thanks

@nvazquez @soreana test LGTM

Manually covered with the following tests, and verified the public templates access restricted to other domains when the global or the respective domain's config "share.public.templates.with.other.domains" is set to false.

  • Created domains: Domain1 and Domain2.
  • Created accounts/users: Domain1Admin and Domain2Admin.
  • Registered public templates from macchinina-admin from Admin account.
  • Registered public templates from macchinina-domain1, centos65-domain1 from Domain1Admin account.
  • Registered public templates from macchinina-domain2, centos65-domain2 from Domain2Admin account.

Tests with global level setting:
(i) The global config "share.public.templates.with.other.domains" is set to true by default, and all the public templates of domain1/domain2 are listed for admin, domain1 admin, and domain2 admin.
(ii) Set the global config "restrict.public.template.access.to.domain" to false. Verified the public templates of domain1 are not listed for domain2 admin, and the public templates of domain2 are not listed for domain1 admin. All the public templates are listed for root admin.

Tests with domain level setting:
(i) Set Domain1 config "share.public.templates.with.other.domains": true and Domain2 config "share.public.templates.with.other.domains": false
=> All the public templates are listed for root admin.

=> The public templates of domain1 are listed for domain2 admin.

=> The public templates of domain2 are not listed for domain1 admin.
(ii) Set Domain1 config "share.public.templates.with.other.domains": false and Domain2 config "share.public.templates.with.other.domains": true

=> All the public templates are listed for root admin.

=> The public templates of domain1 are not listed for domain2 admin.

=> The public templates of domain2 are listed for domain1 admin.
(iii) Set Domain1 config "share.public.templates.with.other.domains": true and Domain2 config "share.public.templates.with.other.domains": true

=> All the public templates are listed for root admin.

=> The public templates of domain1 are listed for domain2 admin

=> The public templates of domain2 are listed for domain1 admin
(iv) Set Domain1 config "share.public.templates.with.other.domains": false and Domain2 config "share.public.templates.with.other.domains": false

=> All the public templates are listed for root admin.

=> The public templates of domain1 are not listed for domain2 admin

=> The public templates of domain2 are not listed for domain1 admin

@nvazquez
Copy link
Contributor

Hi @soreana all looking good except travis that is still failing in all these cases:

==== Marvin Init Successful ====
=== TestName: test_01_check_cross_domain_template_access | Status : EXCEPTION ===
=== TestName: test_02_create_template | Status : EXCEPTION ===
=== TestName: test_03_check_subdomain_template_access | Status : EXCEPTION ===
=== TestName: test_04_check_non_public_template_access | Status : EXCEPTION ===
=== TestName: test_05_check_non_public_template_subdomain_access | Status : EXCEPTION ===
=== TestName: test_06_check_sub_public_template_sub_domain_access | Status : EXCEPTION ===
=== TestName: test_07_check_default_public_template_sub_domain_access | Status : EXCEPTION ===
=== TestName: test_08_check_non_public_template_sub_domain_access | Status : EXCEPTION ===

Function to update the global setting "restrict.public.access.to.templates" for domain
"""
update_configuration_cmd = updateConfiguration.updateConfigurationCmd()
update_configuration_cmd.name = "restrict.public.template.access.to.domain"
Copy link
Member

Choose a reason for hiding this comment

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

@soreana
since you have changed the name of global configuration to share.public.templates.with.other.domains, can you change the name in component test ? (maybe the value as well)

The test needs to be updated to use the new configuration name
@acs-robot
Copy link

Found UI changes, kicking a new UI QA build
@blueorangutan ui

@blueorangutan
Copy link

Trillian test result (tid-3958)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37433 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4774-t3958-kvm-centos7.zip
Smoke tests completed. 95 look OK, 1 have errors
Only failed tests results shown below:

Test Result Time (s) Test File
test_01_nic Error 141.55 test_nic.py

@acs-robot
Copy link

PR Coverage Report

CLASS INSTRUCTION MISSED INSTRUCTION COVERED BRANCH MISSED BRANCH COVERED LINE MISSED LINE COVERED
Network 554 0 42 0 107 0
Volume 109 0 2 0 44 0
NetworkOrchestrationService 0 101 0 0 0 10
StorageManager 0 211 0 0 0 18
AgentAttache 1042 0 124 0 219 0
AgentManagerImpl 3463 0 380 0 714 0
ClusteredAgentManagerImpl 2361 0 242 0 536 0
NetworkOrchestrator 9722 0 1198 0 1914 0
VolumeOrchestrator 5170 0 560 0 977 0
DataCenterVnetVO 58 0 0 0 24 0
VlanVO 186 0 2 0 72 0
AccountGuestVlanMapVO 46 0 0 0 19 0
NetworkDaoImpl 3307 0 116 0 467 0
NetworkOfferingVO 414 31 0 0 126 12
SnapshotVO 172 58 12 0 48 20
VolumeVO 529 133 4 0 179 39
SnapshotDaoImpl 768 0 8 0 128 0
Upgrade41610to41700 82 7 4 0 23 2
DomainRouterDaoImpl 1604 0 22 0 228 0
DirectDownloadCertificateHostMapDaoImpl 121 0 0 0 18 0
DirectDownloadCertificateHostMapVO 54 0 4 0 19 0
AncientDataMotionStrategy 1458 52 170 4 338 10
DefaultSnapshotStrategy 758 220 100 10 164 37
SnapshotDataFactoryImpl 152 58 15 5 37 13
DefaultVMSnapshotStrategy 486 677 52 30 100 140
ScaleIOVMSnapshotStrategy 1063 4 80 0 224 1
StorageStrategyFactoryImpl 37 77 1 5 8 18
SnapshotDataStoreDaoImpl 1683 60 44 2 292 13
DefaultHostListener 537 0 40 0 101 0
VolumeObject 773 665 77 39 178 117
LibvirtComputingResource 8927 1961 1096 140 1992 444
LibvirtVMDef 37 114 6 4 11 36
LibvirtMigrateCommandWrapper 977 383 158 36 228 98
LibvirtReadyCommandWrapper 8 37 2 2 1 10
LibvirtRevertSnapshotCommandWrapper 387 130 20 0 69 16
LibvirtUtilitiesHelper 115 58 4 2 27 8
IscsiAdmStorageAdaptor 981 0 52 0 178 0
IscsiAdmStoragePool 129 0 0 0 32 0
KVMStoragePoolManager 926 4 76 0 192 1
KVMStorageProcessor 5992 498 448 22 1239 71
LibvirtStorageAdaptor 3376 19 257 0 797 2
LibvirtStoragePool 239 89 15 9 64 29
LinstorStorageAdaptor 1076 0 50 0 246 0
ManagedNfsStorageAdaptor 446 0 20 0 125 0
ScaleIOStorageAdaptor 684 121 75 13 152 31
KVMHostInfo 150 146 10 4 30 38
QemuImg 674 0 64 0 160 0
MockVmManagerImpl 1495 0 90 0 338 0
VmwareServerDiscoverer 1586 0 182 0 378 0
VmwareManagerImpl 2613 528 296 38 615 117
VmwareResource 20060 0 2242 0 4330 0
VmwareStorageProcessor 9892 9 940 0 2122 2
XcpServerDiscoverer 1554 101 176 6 345 21
CitrixResourceBase 14566 557 1452 34 3160 122
CitrixReadyCommandWrapper 58 37 3 1 15 11
KubernetesClusterManagerImpl 4724 0 480 0 760 0
KubernetesClusterVO 298 0 6 0 109 0
KubernetesClusterActionWorker 1540 0 114 0 281 0
KubernetesClusterResourceModifierActionWorker 1843 0 140 0 325 0
KubernetesClusterStartWorker 2678 0 168 0 426 0
ListVMsMetricsCmd 39 0 0 0 10 0
MetricsServiceImpl 1673 0 98 0 337 0
NetScalerControlCenterResource 1943 0 144 0 468 0
NetscalerResource 6882 0 806 0 1623 0
ElastistorHostListener 150 0 14 0 30 0
DateraPrimaryDataStoreDriver 3195 0 283 0 748 0
DateraHostListener 635 0 74 0 136 0
CloudStackPrimaryDataStoreDriverImpl 903 0 114 0 229 0
LinstorPrimaryDataStoreDriverImpl 1442 0 91 0 348 0
ScaleIOPrimaryDataStoreDriver 2537 0 246 0 537 0
ScaleIOHostListener 196 0 14 0 43 0
SolidFirePrimaryDataStoreDriver 3347 0 284 0 697 0
SolidFireHostListener 545 0 60 0 112 0
SolidFireSharedHostListener 407 0 30 0 82 0
SAMLUtils 202 465 41 11 53 108
DomainChecker 1206 0 300 0 238 0
ApiDBUtils 2367 0 210 0 590 0
ApiResponseHelper 12144 0 1274 0 2779 0
ParamProcessWorker 1050 0 155 0 241 0
QueryManagerImpl 14233 0 1248 0 2403 0
ViewResponseHelper 1662 0 150 0 305 0
UserVmJoinDaoImpl 1531 0 184 0 328 0
VolumeJoinDaoImpl 770 0 94 0 171 0
VolumeJoinVO 267 0 0 0 93 0
Config 152 5162 30 6 42 342
ConfigurationManagerImpl 18104 0 3032 0 3570 0
LibvirtServerDiscoverer 988 0 116 0 218 0
IpAddressManagerImpl 4045 0 461 0 806 0
NetworkModelImpl 6182 0 838 0 1300 0
NetworkServiceImpl 13372 0 1862 0 2548 0
AutoScaleManagerImpl 3297 0 320 0 691 0
ConfigDriveNetworkElement 1466 0 173 0 306 0
FirewallManagerImpl 2522 0 391 0 455 0
GuestNetworkGuru 622 298 98 34 124 64
PrivateNetworkGuru 394 0 46 0 88 0
LoadBalancingRulesManagerImpl 6024 0 666 0 1254 0
NetworkHelperImpl 2019 0 264 0 428 0
RulesManagerImpl 4074 0 492 0 790 0
SecurityGroupManagerImpl 2330 0 260 0 497 0
NetworkACLServiceImpl 2698 0 302 0 520 0
VpcManagerImpl 6870 0 758 0 1311 0
ResourceManagerImpl 8512 0 982 0 1658 0
ConfigurationServerImpl 2075 0 178 0 499 0
ManagementServerImpl 11819 0 1052 0 2361 0
StatsCollector 1875 0 104 0 308 0
StorageManagerImpl 8547 0 974 0 1704 0
VolumeApiServiceImpl 10851 0 1500 0 2035 0
StoragePoolMonitor 427 0 72 0 107 0
SnapshotManager 107 0 0 0 9 0
SnapshotManagerImpl 4186 0 410 0 751 0
TaggedResourceManagerImpl 473 0 58 0 96 0
TemplateManagerImpl 4996 0 696 0 1042 0
AccountManagerImpl 6446 0 906 0 1351 0
UserVmManagerImpl 20798 0 2566 0 3869 0
VMSnapshotManagerImpl 3135 0 292 0 623 0
BackupManagerImpl 2776 0 224 0 488 0
DirectDownloadManagerImpl 1679 0 186 0 361 0
UnmanagedVMsManagerImpl 4881 0 542 0 800 0
MockNetworkManagerImpl 485 0 22 0 83 0
PremiumSecondaryStorageManagerImpl 775 0 64 0 116 0
SecondaryStorageManagerImpl 3494 149 343 11 623 32
VirtualMachineMO 8953 135 1063 17 1982 31

@weizhouapache weizhouapache reopened this Apr 21, 2022
@nvazquez nvazquez closed this Apr 21, 2022
@nvazquez nvazquez reopened this Apr 21, 2022
@nvazquez nvazquez merged commit debfb45 into apache:main Apr 22, 2022
nlgordon added a commit to ippathways/cloudstack that referenced this pull request Jun 29, 2022
…PR: apache#4774 in commit e94c1e2. Had to make a few changes to support 4.11 and cleanup some of the quirks.
Slair1 pushed a commit to ippathways/cloudstack that referenced this pull request Nov 27, 2022
* Pulling in the relevant parts of the global settings version of this PR: apache#4774 in commit e94c1e2. Had to make a few changes to support 4.11 and cleanup some of the quirks.

* Making the 'allow.public.user.templates' global setting allow domain and resource admins to still upload public templates. Previously it was limited to just root admins.

* One additional template filter type that includes public templates

* Fixing permissions bug where there weren't permissions to templates in child domains

* Updating comments based on code review

* Was previously allowing public, but not featured for domain admins. Added featured for domain admins. Also limited public to only when the new setting is enabled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

10 participants