[GitHub] [kafka] acsaki commented on pull request #12179: [KAFKA-13848] Clients remain connected after SASL re-authentication f…
acsaki commented on PR #12179: URL: https://github.com/apache/kafka/pull/12179#issuecomment-1152416005 Thank you @showuon, @tombentley and @SamBarker for the review, guidance and help! I'm really happy to see my first contribution merged, thank you! -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] acsaki commented on pull request #12179: [KAFKA-13848] Clients remain connected after SASL re-authentication f…
acsaki commented on PR #12179: URL: https://github.com/apache/kafka/pull/12179#issuecomment-1150937597 Thanks @showuon , I really like the idea that we should set `sessionExpirationTimeNanos` regardless whether max reauth is set or not, otherwise clients probably won't get disconnected after their tokens expire. Maybe the original intention was to still allow the clients to re-authenticate but nothing else? Feels safer this way. Regarding whether max reauth is in play, I realized that the null check might not be not enough, we should probably treat it being the default 0L the same. (I think max reauth = 0 doesn't really make sense, does it?) So I've updated the PR and now it checks `connectionsMaxReauthMs != null && connectionsMaxReauthMs > 0` where there was only a null check. Tons of failing tests turned green instantly. Does this make sense? -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] acsaki commented on pull request #12179: [KAFKA-13848] Clients remain connected after SASL re-authentication f…
acsaki commented on PR #12179: URL: https://github.com/apache/kafka/pull/12179#issuecomment-1145812750 > Nice tests! Thank you. Left some comments. Also, I found there are many tests failed with the error: > > ``` > org.opentest4j.AssertionFailedError: Topic [__consumer_offsets] metadata not propagated after 6 ms > ``` > > ref: https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-12179/4/#showFailuresLink > > I guess maybe it's because we use mock time, instead of system time now? Please help check them. Thank you. Thank you, haven't thought of MockTime. Indeed, lot of tests are failing yet, I've found out that in some test `connections.max.reauth.ms` gets set to 0 instead of being left as null, so my current code sets the session expiration eventually and clients seem to get disconnected. -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] acsaki commented on pull request #12179: [KAFKA-13848] Clients remain connected after SASL re-authentication f…
acsaki commented on PR #12179: URL: https://github.com/apache/kafka/pull/12179#issuecomment-1142294491 > @acsaki , we haven't get your response for some days, do you need help on it? We can co-author with you to address the comments and fix the tests. Please let me know. Thank you. Hi @showuon , Thank you! I wasn't working for a few days.. Thanks for all the suggestions (@SamBarker and @tombentley too), I'm writing some more tests to capture the intended behavior, which is as I understood: - 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 - when reauth is enabled and the token expires after max reauth time, set sessionExpirationTimeNanos to when the reauth is due and also return sessionLifetimeMs in the SaslAuthenticateResponse accordingly - when reauth is enabled and the token expires before max reauth time, set sessionExpirationTimeNanos to when the token expires and also return sessionLifetimeMs in the SaslAuthenticateResponse accordingly. SaslServerAuthenticatorTest does not lend itself easily to testing all these, oh well. :) -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
