Do not list Quota API's to accounts with Quota disabled #7599

JoaoJandre · 2023-06-06T14:50:59Z

Description

Currently, even if an account has the Quota plugin disabled, they may still list and use the Quota APIs. This PR fixes this behavior, making the quota.account.enabled be respected when listing APIs and calling APIs.

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

Screenshots (if appropriate):

How Has This Been Tested?

A new role was created and granted permission to call all Quota APIs, then a new account was created from that role.
With the plugin enabled for this account, all Quota APIs were called and worked as expected. Then the plugin was disabled for the account and all the APIs were called again, all the calls got a permission denied error, except the quotaIsEnable API, which continued to work (as expected).
Finally, still with quota disabled for the account, a sync was done using CloudMonkey to verify that the quota APIs would not be discovered, and only quotaIsEnable was discovered for that account (as expected).

framework/config/src/main/java/org/apache/cloudstack/framework/config/PluginAccessConfigs.java

DaanHoogland

clgtm, but can you extract the new blocks in ApiDiscoveryServiceImpl , please?
also there is a license header missing

...ins/api/discovery/src/main/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java

codecov · 2023-06-07T13:25:03Z

Codecov Report

Merging #7599 (741abeb) into main (b2e9993) will increase coverage by 16.07%.
Report is 485 commits behind head on main.
The diff coverage is 42.13%.

@@              Coverage Diff              @@
##               main    #7599       +/-   ##
=============================================
+ Coverage     12.97%   29.05%   +16.07%     
- Complexity     8978    30906    +21928     
=============================================
  Files          2716     5189     +2473     
  Lines        256309   365814   +109505     
  Branches      39968    53493    +13525     
=============================================
+ Hits          33268   106293    +73025     
- Misses       218880   244954    +26074     
- Partials       4161    14567    +10406

Flag	Coverage Δ
simulator-marvin-tests	`24.98% <41.01%> (?)`
uitests	`4.50% <ø> (?)`
unit-tests	`14.79% <7.92%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...com/cloud/agent/dhcp/DhcpProtocolParserServer.java	`0.00% <ø> (ø)`
api/src/main/java/com/cloud/agent/api/Answer.java	`42.30% <ø> (ø)`
...c/main/java/com/cloud/agent/api/to/DatadiskTO.java	`0.00% <ø> (ø)`
...i/src/main/java/com/cloud/agent/api/to/DpdkTO.java	`0.00% <ø> (ø)`
...ud/agent/api/to/deployasis/OVFConfigurationTO.java	`0.00% <ø> (ø)`
...om/cloud/agent/api/to/deployasis/OVFNetworkTO.java	`0.00% <ø> (ø)`
...nt/api/to/deployasis/OVFVirtualHardwareItemTO.java	`0.00% <ø> (ø)`
...api/to/deployasis/OVFVirtualHardwareSectionTO.java	`0.00% <ø> (ø)`
...main/java/com/cloud/offering/DiskOfferingInfo.java	`76.47% <ø> (ø)`
.../com/cloud/resource/RollingMaintenanceManager.java	`7.31% <ø> (ø)`
... and 178 more

... and 3654 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

JoaoJandre · 2023-08-09T17:32:52Z

@blueorangutan package

blueorangutan · 2023-08-09T17:34:08Z

@JoaoJandre a [SF] 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 · 2023-08-09T18:43:52Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6726

DaanHoogland · 2023-08-11T06:57:31Z

@blueorangutan test

blueorangutan · 2023-08-11T06:58:03Z

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

blueorangutan · 2023-08-11T20:33:52Z

[SF] Trillian test result (tid-7375)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 47511 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7599-t7375-kvm-centos7.zip
Smoke tests completed. 112 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_migrate_VM_and_root_volume	`Error`	78.82	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	`Error`	52.43	test_vm_life_cycle.py

DaanHoogland

clgtm

shwstppr · 2023-10-09T08:13:58Z

@blueorangutan package

blueorangutan · 2023-10-09T08:16:05Z

@shwstppr a [SF] 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 · 2023-10-09T09:25:42Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7271

DaanHoogland · 2023-10-09T10:20:17Z

@blueorangutan test

blueorangutan · 2023-10-09T10:22:04Z

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

...ins/api/discovery/src/main/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java

blueorangutan · 2023-10-09T22:18:42Z

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

Test	Result	Time (s)	Test File

DaanHoogland · 2023-10-31T15:03:57Z

@JoaoJandre , can you look at @GutoVeronezi 's comments/suggestions?

JoaoJandre · 2023-11-13T11:22:19Z

@blueorangutan package

blueorangutan · 2023-11-13T11:24:04Z

@JoaoJandre a [SL] 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 · 2023-11-13T12:23:33Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7729

DaanHoogland · 2023-11-14T07:49:44Z

@blueorangutan test

blueorangutan · 2023-11-14T07:58:05Z

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

blueorangutan · 2023-11-14T23:21:13Z

[SF] Trillian test result (tid-8314)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 53832 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7599-t8314-kvm-centos7.zip
Smoke tests completed. 115 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_invalid_upgrade_kubernetes_cluster	`Failure`	3606.69	test_kubernetes_clusters.py
test_02_upgrade_kubernetes_cluster	`Failure`	3616.32	test_kubernetes_clusters.py
test_03_deploy_and_scale_kubernetes_cluster	`Failure`	0.05	test_kubernetes_clusters.py
test_04_autoscale_kubernetes_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_05_basic_lifecycle_kubernetes_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_06_delete_kubernetes_cluster	`Failure`	0.06	test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster	`Failure`	0.05	test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster	`Failure`	0.05	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_10_vpc_tier_kubernetes_cluster	`Failure`	42.79	test_kubernetes_clusters.py
test_11_test_unmanaged_cluster_lifecycle	`Error`	1.29	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	53.69	test_kubernetes_clusters.py
test_08_migrate_vm	`Error`	0.06	test_vm_life_cycle.py

DaanHoogland · 2023-11-15T08:53:07Z

@kishankavala @GutoVeronezi can you approve?

GutoVeronezi · 2023-11-17T19:23:33Z

@kishankavala @GutoVeronezi can you approve?

@DaanHoogland
I was thinking about this one, perhaps it does not make sense to not list the APIs if the Quota for the account is disabled. For instance, we could have an account that would visualize the Quota (like a manager) but would not be accounted for; therefore, the Quota would be disabled for the account, but the account would still be able to request the Quota APIs. For denying access to the APIs, we could create different roles.

cc: @JoaoJandre

DaanHoogland · 2023-11-18T11:16:23Z

@kishankavala @GutoVeronezi can you approve?

@DaanHoogland I was thinking about this one, perhaps it does not make sense to not list the APIs if the Quota for the account is disabled. For instance, we could have an account that would visualize the Quota (like a manager) but would not be accounted for; therefore, the Quota would be disabled for the account, but the account would still be able to request the Quota APIs. For denying access to the APIs, we could create different roles.

cc: @JoaoJandre

make functional sense @GutoVeronezi , let me know what/when... moving this out of 4.19 for now

JoaoJandre · 2023-11-20T19:19:35Z

@kishankavala @GutoVeronezi can you approve?

@DaanHoogland I was thinking about this one, perhaps it does not make sense to not list the APIs if the Quota for the account is disabled. For instance, we could have an account that would visualize the Quota (like a manager) but would not be accounted for; therefore, the Quota would be disabled for the account, but the account would still be able to request the Quota APIs. For denying access to the APIs, we could create different roles.

cc: @JoaoJandre

@DaanHoogland @GutoVeronezi Looking at it this way, I believe that this PR is not really relevant anymore. As this functionality can be achieved using different roles to block the wanted APIs. I'll be closing the PR.

João Jandre added 2 commits June 6, 2023 10:27

Do not list Quota API's to accounts with Quota disabled

27aa906

few fixes

cfc9062

boring-cyborg bot added component:api component:quota labels Jun 6, 2023

DaanHoogland reviewed Jun 7, 2023

View reviewed changes

framework/config/src/main/java/org/apache/cloudstack/framework/config/PluginAccessConfigs.java Show resolved Hide resolved

DaanHoogland reviewed Jun 7, 2023

View reviewed changes

kishankavala reviewed Jun 7, 2023

View reviewed changes

...ins/api/discovery/src/main/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java Outdated Show resolved Hide resolved

Address daans reviews

438c54a

DaanHoogland added this to the 4.19.0.0 milestone Jun 22, 2023

DaanHoogland approved these changes Aug 14, 2023

View reviewed changes

DaanHoogland closed this Aug 14, 2023

DaanHoogland reopened this Aug 14, 2023

JoaoJandre requested a review from kishankavala September 28, 2023 18:15

GutoVeronezi reviewed Oct 9, 2023

View reviewed changes

Address Daniel's reviews

741abeb

DaanHoogland assigned kishankavala and GutoVeronezi Nov 15, 2023

DaanHoogland modified the milestones: 4.19.0.0, unplanned Nov 18, 2023

JoaoJandre closed this Nov 20, 2023

Do not list Quota API's to accounts with Quota disabled #7599

Do not list Quota API's to accounts with Quota disabled #7599

Uh oh!

Conversation

JoaoJandre commented Jun 6, 2023

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JoaoJandre commented Aug 9, 2023

Uh oh!

blueorangutan commented Aug 9, 2023

Uh oh!

blueorangutan commented Aug 9, 2023

Uh oh!

DaanHoogland commented Aug 11, 2023

Uh oh!

blueorangutan commented Aug 11, 2023

Uh oh!

blueorangutan commented Aug 11, 2023

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

shwstppr commented Oct 9, 2023

Uh oh!

blueorangutan commented Oct 9, 2023

Uh oh!

blueorangutan commented Oct 9, 2023

Uh oh!

DaanHoogland commented Oct 9, 2023

Uh oh!

blueorangutan commented Oct 9, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blueorangutan commented Oct 9, 2023

Uh oh!

DaanHoogland commented Oct 31, 2023

Uh oh!

JoaoJandre commented Nov 13, 2023

Uh oh!

blueorangutan commented Nov 13, 2023

Uh oh!

blueorangutan commented Nov 13, 2023

Uh oh!

DaanHoogland commented Nov 14, 2023

Uh oh!

blueorangutan commented Nov 14, 2023

Uh oh!

blueorangutan commented Nov 14, 2023

Uh oh!

DaanHoogland commented Nov 15, 2023

Uh oh!

GutoVeronezi commented Nov 17, 2023

Uh oh!

DaanHoogland commented Nov 18, 2023

Uh oh!

JoaoJandre commented Nov 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

codecov bot commented Jun 7, 2023 •

edited

Loading