api: don't throttle api discovery for listApis command by csquire · Pull Request #2894 · apache/cloudstack

csquire · 2018-10-10T19:40:09Z

Description

Users reported that they weren't getting all apis listed in cloudmonkey when running a sync. After some debugging, I found that the problem is that the ApiDiscoveryService is calling ApiRateLimitServiceImpl.checkAccess(), so the results of the listApis command are being truncated because Cloudstack believes the user has exceeded their API throttling rate.

I enabled throttling with a 25 request per second limit. I then created a test role with only list* permissions and assigned it to a test user. When this user calls listApis, they will typically receive anywhere from 15-18 results. Checking the logs, you see The given user has reached his/her account api limit, please retry after 218 ms..

I raised the limit to 200 requests per second, restarted the management server and tried again. This time I got 143 results and no log messages about the user being throttled.

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

Fixes: #2842

Screenshots (if appropriate):

How Has This Been Tested?

Updated the appropriate unit tests to verify the ApiRateLimitService is not longer called when discovering APIs.

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.

yadvr · 2018-10-11T09:56:47Z

plugins/api/discovery/src/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java

                for (APIChecker apiChecker : _apiAccessCheckers) {
                    try {
-                        apiChecker.checkAccess(user, apiName);
+                        if (!"ApiRateLimitServiceImpl".equals(apiChecker.getName())) {


you can use something like this.class.getName() or the .getName()... instead of the hard coded string comparison.

Using the 'ApiRateLimitServiceImpl' class instead of a hard coded string introduces the rate limit plugin module as a dependency to the api discovery module. I didn't think that was a good idea.

yadvr · 2018-10-11T09:56:57Z

plugins/api/discovery/src/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java

            for (APIChecker apiChecker : _apiAccessCheckers) {
                try {
-                    apiChecker.checkAccess(user, name);
+                    if (!"ApiRateLimitServiceImpl".equals(apiChecker.getName())) {


you can use something like this.class.getName() or the .getName()... instead of the hard coded string comparison.

Using the 'ApiRateLimitServiceImpl' class instead of a hard coded string introduces the rate limit plugin module as a dependency to the api discovery module. I didn't think that was a good idea.

What if you used the command class instead? I mean, you are using the "service class", but at this point here you have the command that was used to list the APIs, right? You can use it, and then you do not need to use this hardcoded String.

@rafaelweingartner Maybe I'm missing something with your suggestion? ListApisCmd is available to the discovery service on the classpath (it's not currently passed into the listApis() method), but I don't see how that helps choose which APIChecker to ignore unless the cmd class gets passed into APIChecker.checkAccess(), which would then introduce an api discovery module dependency on the rate limit plugin module.

It looks like there used to be a separate APILimitChecker interface used for this, but it appears to have been abandoned years ago. I also don't like hard coding names, but the only alternative I saw was a much bigger change that would add risk.

Again, let me know if I misunderstood your suggestion.

Ok, I am now seeing the big picture. I looking at the code here. I will provide some feedback in another comment

rafaelweingartner · 2018-10-11T16:53:17Z

@csquire, from your PR description I understood that we should not need/use the API rate limit service. Therefore, we only need to check if the user has access to the API method. Is that it?

If that is the case, why don’t we do something different?

At org.apache.cloudstack.discovery.ApiDiscoveryServiceImpl.listApis(User, String) method, instead of injecting all "APIChecker” implementations, we can only inject the StaticRoleBasedAPIAccessChecker and DynamicRoleBasedAPIAccessChecker. Then, you solve all of your problems with little or no coding at all.
To change the beans that are injected you can change the XML file spring-server-core-misc-context.xml. There is a bean called apiDiscoveryServiceImpl; then, you need to check the property being configured apiAccessCheckers. I guess you can change the property injection from:
<property name="apiAccessCheckers" value="#{apiCheckersRegistry.registered}" />

To something like:

        <property name="apiAccessCheckers">
            <util:list list-class="org.apache.cloudstack.acl.APIChecker">
                <ref bean="DynamicRoleBasedAPIAccessChecker" />
                <ref bean="StaticRoleBasedAPIAccessChecker" />
            </util:list>
        </property>

Of course, you need to import the “util” namespace in spring-server-core-misc-context.xml, but that is quite easy to do.

csquire · 2018-10-11T17:16:40Z

yeah, great idea, that looks like a good solution. I'll update the PR.

csquire · 2018-10-11T21:03:13Z

I took a look at your suggestion @rafaelweingartner but those beans cannot be referenced directly in this context.

rafaelweingartner · 2018-10-11T21:49:31Z

ow really?
So, what if you change the "set" method of that property to remove the "apiRateService" from the list of API checkers being configured?

Or, you can implement the InitializingBean interface. Then, when the bean already has all of the properties configured, you can remove the API rate checker.

csquire · 2018-10-12T15:04:04Z

With the idea of not injecting the rate limit checker, I tried a different approach by creating a new marker interface and new registry for ACL checkers. Thoughts?

rafaelweingartner · 2018-10-12T15:11:21Z

I confess that I really dislike that registry solution that people implemented in ACS. Instead of using Spring life-cycle properly, people decided to create something else.

Your solution now works, and it is better than changing the code like it was done in the first attempt. However, I would still prefer something more integrated with Spring life-cycle.

Right now I am not able to think in something to enable us to work around this problem. Therefore, I am inclined to say that this last proposal is fine.

rafaelweingartner · 2018-10-15T18:13:19Z

api/src/org/apache/cloudstack/acl/APIAclChecker.java

@@ -0,0 +1,7 @@
+package org.apache.cloudstack.acl;


You missed the license header here

yadvr · 2018-12-04T08:31:42Z

@blueorangutan package

blueorangutan · 2018-12-04T08:31:57Z

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

blueorangutan · 2018-12-04T08:51:18Z

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

yadvr · 2018-12-04T11:11:16Z

@blueorangutan test

blueorangutan · 2018-12-04T11:11:58Z

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

blueorangutan · 2018-12-04T17:10:56Z

Trillian test result (tid-3246)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 19967 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2894-t3246-kvm-centos7.zip
Smoke tests completed. 68 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

DaanHoogland · 2018-12-11T11:21:50Z

@rhtyd is this ready?

borisstoyanov

LGTM

yadvr · 2018-12-11T16:48:10Z

Let me test this tomorrow @DaanHoogland

yadvr reviewed Oct 11, 2018

View reviewed changes

yadvr added this to the 4.11.3.0 milestone Oct 11, 2018

csquire force-pushed the 2842_listapi_command_throttling branch 2 times, most recently from 94cd6c7 to 0a9d45e Compare October 12, 2018 15:02

rafaelweingartner added the type:bug label Oct 15, 2018

rafaelweingartner assigned csquire Oct 15, 2018

rafaelweingartner approved these changes Oct 15, 2018

View reviewed changes

api: don't throttle api discovery for listApis command

dcb9d23

csquire force-pushed the 2842_listapi_command_throttling branch from 0a9d45e to dcb9d23 Compare October 15, 2018 18:16

borisstoyanov approved these changes Dec 11, 2018

View reviewed changes

yadvr merged commit 8d53557 into apache:4.11 Dec 12, 2018

yadvr mentioned this pull request May 27, 2019

listApis command returns inconsistent results #2842

Closed

Conversation

csquire commented Oct 10, 2018

Description

Types of changes

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csquire Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner commented Oct 11, 2018

Uh oh!

csquire commented Oct 11, 2018

Uh oh!

csquire commented Oct 11, 2018

Uh oh!

rafaelweingartner commented Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csquire commented Oct 12, 2018

Uh oh!

rafaelweingartner commented Oct 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr commented Dec 4, 2018

Uh oh!

blueorangutan commented Dec 4, 2018

Uh oh!

blueorangutan commented Dec 4, 2018

Uh oh!

yadvr commented Dec 4, 2018

Uh oh!

blueorangutan commented Dec 4, 2018

Uh oh!

blueorangutan commented Dec 4, 2018

Uh oh!

DaanHoogland commented Dec 11, 2018

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Dec 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

csquire Oct 11, 2018 •

edited

Loading

rafaelweingartner commented Oct 11, 2018 •

edited

Loading