Skip to content

Conversation

@Lokowandtg
Copy link
Contributor

Not tested on a system without ratelimiting...

Copy link
Contributor

@Kehrlann Kehrlann left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

[issue] the current implementation doesn't work when UAA_API_REQUEST_LIMIT is not set. See the suggestions in the IntegrationTestConfiguration class.

<logger name="cloudfoundry-client.delay" level="${CLIENT_LOGGING_LEVEL:-INFO}"/>
<logger name="cloudfoundry-client.operations" level="${CLIENT_LOGGING_LEVEL:-INFO}"/>
<logger name="cloudfoundry-client.request" level="${CLIENT_LOGGING_LEVEL:-INFO}"/>
<logger name="cloudfoundry-client.request" level="DEBUG"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

import org.immutables.value.Value;
import reactor.core.publisher.Mono;

public class ThrottlingUaaClient implements UaaClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public class ThrottlingUaaClient implements UaaClient {
/**
* An {@link UaaClient} implementation that throttles calls to the UAA
* {@code /Groups} and {@code /Users} endpoints. It uses a single "bucket"
* for throttling requests to both endpoints.
*
* @see <a href="https://resilience4j.readme.io/docs/ratelimiter">resilience4j docs</a>
*/
public class ThrottlingUaaClient implements UaaClient {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks.

private final ThrottledUsers users;
private Groups groups;

public ThrottlingUaaClient(ReactorUaaClient delegate, int uaaLimit) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public ThrottlingUaaClient(ReactorUaaClient delegate, int uaaLimit) {
public ThrottlingUaaClient(ReactorUaaClient delegate, int maxRequestsPerSecond) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

public ThrottlingUaaClient(ReactorUaaClient delegate, int uaaLimit) {
// uaaLimit is calls per second. We need the milliseconds for one call because
// resilience4j uses sliced timeslots, while the uaa server uses a sliding window.
int timeBasePerRequest = 1000 / uaaLimit;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
int timeBasePerRequest = 1000 / uaaLimit;
int clockSkewMillis = 20; // 20ms clock skew
int rateLimitRefreshPeriodMillis = (1000 / maxRequestsPerSecond) + clockSkewMillis;

[nitpick] The clock skew only makes sense if you have 1 to ~10 rps. If you set 100rps, then the clock skew is bigger than the un-skewed window, and you're going to be doing roughly 33rps instead of 100, which might be surprising.
I think it's fine to leave it as-is, maybe add a comment explaining that this value was chosen for a maxRequestsPerSecond value between 1 and 10?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, done.

.limitForPeriod(1)
.limitRefreshPeriod(
Duration.ofMillis(
timeBasePerRequest + 20)) // 20 ms to handle clock skew.
Copy link
Contributor

Choose a reason for hiding this comment

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

See above: put the clock skew in the variable directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

<dependency>
<groupId>io.github.resilience4j</groupId>
<artifactId>resilience4j-ratelimiter</artifactId>
<version>1.7.0</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put the version in a property in the root pom.xml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, done.

Comment on lines 335 to 376
@Bean
Boolean uaaRatelimit(
ConnectionContext connectionContext, @Qualifier("admin") UaaClient uaaClient) {
return uaaClient
.rateLimit()
.getRatelimit(RatelimitRequest.builder().build())
.map(response -> getServerRatelimit(response))
.timeout(Duration.ofSeconds(5))
.onErrorResume(
ex -> {
logger.error(
"Warning: could not fetch UAA rate limit, using default"
+ " "
+ 0
+ ". Cause: "
+ ex);
return Mono.just(false);
})
.block();
}

private Boolean getServerRatelimit(RatelimitResponse response) {
Current curr = response.getCurrentData();
if (!"ACTIVE".equals(curr.getStatus())) {
logger.debug(
"UaaRatelimitInitializer server ratelimit is not 'ACTIVE', but "
+ curr.getStatus()
+ ". Ignoring server value for ratelimit.");
return false;
}
Integer result = curr.getLimiterMappings();
logger.info(
"Server uses uaa rate limiting. There are "
+ result
+ " mappings declared in file "
+ response.getFromSource());
logger.info(
"If you encounter 429 return codes, configure uaa rate limiting or set variable"
+ " 'UAA_API_REQUEST_LIMIT' to a save value.");
return true;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

[suggestion] Drop this entirely, this is just a helper message. Only maintainers will ever care about this, and I don't think we need this level of "discoverability". Your change in the README is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it to a dedicated integration test, that fits better and also can give the warning.

.build())
.build();
@Value("${test.admin.clientSecret}") String clientSecret,
int uaaLimiterMapping) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[suggestion] Drop the uaaLimiterMapping bean, and instead inject the value here. This allows you to create an unthrottled UAA client, if necessary. For example:

@Bean
@Qualifier("admin")
UaaClient adminUaaClient(
        ConnectionContext connectionContext,
        @Value("${test.admin.clientId}") String clientId,
        @Value("${test.admin.clientSecret}") String clientSecret,
        @Value("${uaa.api.request.limit:#{null}}") Integer environmentRequestLimit) {
    ReactorUaaClient unthrottledClient = ReactorUaaClient.builder()
            .connectionContext(connectionContext)
            .tokenProvider(
                    ClientCredentialsGrantTokenProvider.builder()
                            .clientId(clientId)
                            .clientSecret(clientSecret)
                            .build())
            .build();
    if (environmentRequestLimit == null) {
        return unthrottledClient;
    } else {
        return new ThrottlingUaaClient(
                unthrottledClient,
                environmentRequestLimit);
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats better, done.

.connectionContext(connectionContext)
.tokenProvider(tokenProvider)
.build();
UaaClient uaaClient(
Copy link
Contributor

Choose a reason for hiding this comment

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

Apply the same suggestion as the adminUaaClient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants