-
Notifications
You must be signed in to change notification settings - Fork 320
ReactorClient is not refreshing the token, when response is 401 #1130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Can you provide some additional information?
Thanks |
@Configuration
@EnableConfigurationProperties({UaaClientProperties.class, AppUriProperties.class})
class ServiceInstanceBindingConfiguration {
@Bean
UaaBackingAppCredentialsClient uaaBackingAppUserClient(ReactorUaaClient uaaClient) {
return new UaaBackingAppCredentialsClient(uaaClient);
}
@Bean
ClientCredentialsGrantTokenProvider bindingTokenProvider(UaaClientProperties uaaProperties) {
return ClientCredentialsGrantTokenProvider.builder()
.clientId(uaaProperties.getClientId())
.clientSecret(uaaProperties.getClientSecret())
.build();
}
@Bean
ReactorUaaClient bindingUaaClient(ConnectionContext connectionContext,
ClientCredentialsGrantTokenProvider tokenProvider) {
return ReactorUaaClient.builder()
.connectionContext(connectionContext)
.tokenProvider(tokenProvider)
.build();
}
@Bean
ConnectionContext uaaConnectionContext(UaaClientProperties uaaProperties) {
return DefaultConnectionContext.builder()
.rootProvider(new RootProvider() {
@Override
public Mono<String> getRoot(ConnectionContext connectionContext) {
return Mono.just(uaaProperties.getUaaUri());
}
@Override
public Mono<String> getRoot(String key, ConnectionContext connectionContext) {
return Mono.just(uaaProperties.getUaaUri());
}
})
.apiHost(uaaProperties.getHost())
.secure(uaaProperties.getScheme().startsWith("https"))
.port(Integer.parseInt(uaaProperties.getPort()))
.build();
}
}
I noticed that there is only one log message about token renewal: |
Hi @dmikusa-pivotal Please let us know if there is any update on this issue. This is related to JIRA https://pivotal-io.atlassian.net/browse/SCS-49 which we opened few months ago. Currently customer is still waiting for a fix so far. Thanks. |
@kvmw I've merged a PR which fixes a comparison check between a 401 status and the response received - we are hoping this is the cause. Since you can reproduce the issue, could you try testing with the latest builds of the operations and reactor libs? |
@pivotal-david-osullivan, Thanks for the update. |
@pivotal-david-osullivan, The only way i could reproduce the issue in my local env. was by providing different reason message for 401 status code from the mock UAA server. So if the UAA server returns anything other than The code you've merged fixes this issue. But I don't have any way to verify if UAA has ever returned a 401 code with a custom reason message in our production env. |
Thanks for confirming @kvmw! We were wondering if something in the user's environment could be interfering with the response to create the scenario that is only being seen in this case? |
@dmikusa-pivotal , Apparently this is the default behaviour of uaa. By changing the UAA access-token lifetime to 60 seconds, i can easily reproduce the issue now with following code: GetClientRequest request = GetClientRequest.builder().clientId("app").build();
System.out.println(client.clients().get(request).block().getName()); // Successful call, first call with new access token.
Thread.sleep(30 * 1000);
System.out.println(client.clients().get(request).block().getName()); // Successful call, access token still is valid.
Thread.sleep(31 * 1000);
System.out.println(client.clients().get(request).block().getName()); // Fails with 401. because access token has been expired after 61 seconds. There is not attempt to refresh the token. Now the more important question is: why only one of our customers complaining about this issue. Given above behaviour all of them should have seen such issue. |
Found this in spring-boot repo: spring-projects/spring-boot#6548 Apparently tomcat 8.5 has stoped sending the reason phrase : https://bz.apache.org/bugzilla/show_bug.cgi?id=60362 Again, this does not answer why more clients are not complaining about this issue. |
I might have one possible reason: Most consumers of the library often have only a short runtime, i.e. below the |
@pivotal-david-osullivan Is the PR that you are referring to in #1130 (comment) the PR at #1136? If yes, then it is already released with Thanks for the info! |
@eaglerainbow - Sorry, we should have followed up and closed out this issue. #1136 should be the fix for this issue, and yes, that is in 5.7.0. The environment where this was reported initially only seldom hit the issue. Due to that, it's a little difficult to confirm 100% that the issue is resolved because it would probably take months to definitively confirm, but confidence in the solution is high. The change uses Hope that helps! I'm going to close this out, but feel free to reopen if you use 5.7.0 and are still seeing issues. |
Thanks for the verbose response! |
Oh, ok. Good to know. I thought users could do that. Thanks |
Looking at
Operator.java
in cf-java-client source code, I see that in case of 401 responses, the reactor-client retries the requests with new token. So there should be noInvalidAccessToken
(401) thrown out of the library. But as you can see in the below stack trace, we are catching such exceptions, frequently, in our code.cf-java-client v4.16.0
stack trace:
The text was updated successfully, but these errors were encountered: