You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "kfaraz (via GitHub)" <gi...@apache.org> on 2024/01/14 03:50:55 UTC
[PR] [Do Not Merge] Fix basic auth integration test (druid)
kfaraz opened a new pull request, #15679:
URL: https://github.com/apache/druid/pull/15679
[Do Not Merge]
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fix basic auth integration test (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #15679:
URL: https://github.com/apache/druid/pull/15679#issuecomment-1891010907
Thanks a lot for the review, @abhishekrb19 !
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fix basic auth integration test (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 merged PR #15679:
URL: https://github.com/apache/druid/pull/15679
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fix basic auth integration test (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #15679:
URL: https://github.com/apache/druid/pull/15679#discussion_r1451772492
##########
integration-tests/src/test/java/org/apache/druid/tests/security/ITBasicAuthConfigurationTest.java:
##########
@@ -77,6 +77,20 @@ public void test_druid99User_hasNodeAccess()
checkNodeAccess(druid99);
}
+ @Override
+ protected void setupHttpClientsAndUsers() throws Exception
+ {
+ super.setupHttpClientsAndUsers();
+
+ // Add a delay to allow propagation of credentials to all services
+ try {
+ Thread.sleep(5000);
Review Comment:
Yes, we recently introduced caching for basic auth in PR https://github.com/apache/druid/pull/15648 . That has sped up the authentication process triggering the flakiness in this test.
Only the coordinator exposes such APIs which can be used to fetch auth status, and indeed that API would return the expected response right away. The issue is the propagation of the creds from coordinator to other services (broker in this case) which may be delayed. Other services don't expose any API that would indicate the current set or version of auth creds that they have cached.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fix basic auth integration test (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #15679:
URL: https://github.com/apache/druid/pull/15679#discussion_r1451768471
##########
integration-tests/src/test/java/org/apache/druid/tests/security/ITBasicAuthConfigurationTest.java:
##########
@@ -77,6 +77,20 @@ public void test_druid99User_hasNodeAccess()
checkNodeAccess(druid99);
}
+ @Override
+ protected void setupHttpClientsAndUsers() throws Exception
+ {
+ super.setupHttpClientsAndUsers();
+
+ // Add a delay to allow propagation of credentials to all services
+ try {
+ Thread.sleep(5000);
Review Comment:
Do we know what changed here recently in auth land that this sleep is required? At any rate, I wonder if we could poll `/status` and/or issue a similar API call to verify things are setup instead of a static sleep.
##########
integration-tests/src/test/java/org/apache/druid/tests/security/ITBasicAuthConfigurationTest.java:
##########
@@ -77,6 +77,20 @@ public void test_druid99User_hasNodeAccess()
checkNodeAccess(druid99);
}
+ @Override
+ protected void setupHttpClientsAndUsers() throws Exception
+ {
+ super.setupHttpClientsAndUsers();
+
+ // Add a delay to allow propagation of credentials to all services
+ try {
+ Thread.sleep(5000);
+ }
+ catch (Throwable t) {
Review Comment:
```suggestion
catch (InterruptedException e) {
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org