SamBarker commented on code in PR #12179:
URL: https://github.com/apache/kafka/pull/12179#discussion_r886228291
##########
clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticator.java:
##########
@@ -679,10 +679,11 @@ private long
calcCompletionTimesAndReturnSessionLifetimeMs() {
else if (connectionsMaxReauthMs == null)
retvalSessionLifetimeMs =
zeroIfNegative(credentialExpirationMs - authenticationEndMs);
else
- retvalSessionLifetimeMs = zeroIfNegative(
- Math.min(credentialExpirationMs -
authenticationEndMs, connectionsMaxReauthMs));
+ retvalSessionLifetimeMs =
zeroIfNegative(Math.min(credentialExpirationMs - authenticationEndMs,
connectionsMaxReauthMs));
- sessionExpirationTimeNanos = authenticationEndNanos + 1000 *
1000 * retvalSessionLifetimeMs;
+ if (connectionsMaxReauthMs != null) {
Review Comment:
>when reauth is disabled (when max reauth ms is NOT set), leave
ReauthInfo#sessionExpirationTimeNanos as null but return millis until the token
expires in SaslAuthenticateResponse's sessionLifetimeMs
I think I have a different understanding. My mental model was that if either
of `credentialExpirationMs != null || connectionsMaxReauthMs != null` was
`true` we have re-auth enabled. The re-auth disabled case I was thinking of was
when both were `false`. Now that I think about that I'm not sure that is a
valid scenario.
I think your original change to make setting `sessionExpirationTimeNanos`
un-conditional was the right one, my `>=0` test is effectively the same as
making it un-conditional. Therefore my suggestion of delayed initialisation
becomes moot as we don't need to worry about excluding the default value from
the test.
I was wondering if the property change was what was actually causing the
test failures that you found rather than making it un-conditional.
##########
clients/src/test/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticatorTest.java:
##########
@@ -126,40 +135,94 @@ public void testUnexpectedRequestType() throws
IOException {
@Test
public void testOldestApiVersionsRequest() throws IOException {
testApiVersionsRequest(ApiKeys.API_VERSIONS.oldestVersion(),
- ClientInformation.UNKNOWN_NAME_OR_VERSION,
ClientInformation.UNKNOWN_NAME_OR_VERSION);
+ ClientInformation.UNKNOWN_NAME_OR_VERSION,
ClientInformation.UNKNOWN_NAME_OR_VERSION);
}
@Test
public void testLatestApiVersionsRequest() throws IOException {
testApiVersionsRequest(ApiKeys.API_VERSIONS.latestVersion(),
- "apache-kafka-java", AppInfoParser.getVersion());
+ "apache-kafka-java", AppInfoParser.getVersion());
}
@Test
- public void testExpiredCredentialLifetime() throws IOException {
+ public void
testSessionExpirationLeftAsNullButLifetimeReturnedToTheClientWhenReauthDisabled()
throws IOException {
String mechanism = OAuthBearerLoginModule.OAUTHBEARER_MECHANISM;
+ Duration tokenExpirationDuration = Duration.ofSeconds(100);
SaslServer saslServer = mock(SaslServer.class);
- when(saslServer.getMechanismName()).thenReturn(mechanism);
- when(saslServer.evaluateResponse(any())).thenReturn(new byte[]{});
-
when(saslServer.getNegotiatedProperty(eq(SaslInternalConfigs.CREDENTIAL_LIFETIME_MS_SASL_NEGOTIATED_PROPERTY_KEY))).thenReturn(1L);
- KafkaPrincipalBuilder kafkaPrincipalBuilder =
mock(KafkaPrincipalBuilder.class);
- when(kafkaPrincipalBuilder.build(any())).thenReturn(new
KafkaPrincipal("[principal-type]", "[principal-name"));
+
+ MockTime time = new MockTime();
Review Comment:
I'm wondering if its worth extracting a new test class
`SaslServerAuthenticatorSessionExpiryTest` or similar and that would allow
some of the common setup code to actually be extracted to a `@BeforeEach` and
thus make it easier to reason about what's happening in the tests.
##########
clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticator.java:
##########
@@ -679,10 +679,11 @@ private long
calcCompletionTimesAndReturnSessionLifetimeMs() {
else if (connectionsMaxReauthMs == null)
retvalSessionLifetimeMs =
zeroIfNegative(credentialExpirationMs - authenticationEndMs);
else
- retvalSessionLifetimeMs = zeroIfNegative(
- Math.min(credentialExpirationMs -
authenticationEndMs, connectionsMaxReauthMs));
+ retvalSessionLifetimeMs =
zeroIfNegative(Math.min(credentialExpirationMs - authenticationEndMs,
connectionsMaxReauthMs));
- sessionExpirationTimeNanos = authenticationEndNanos + 1000 *
1000 * retvalSessionLifetimeMs;
+ if (connectionsMaxReauthMs != null) {
Review Comment:
@showuon what is your thinking on this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
