Migrate revised security IT to embedded tests #18358
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ); | ||
| return Objects.requireNonNull(latestSnapshots).getLatestStatus().get(0); | ||
| } | ||
| catch (Exception e) { |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
| import java.util.function.Function; | ||
|
|
||
| /** | ||
| * Client to call various basic auth APIs on the Coordinator. |
There was a problem hiding this comment.
Dummy suggestion to trigger CI:
| * Client to call various basic auth APIs on the Coordinator. | |
| * Client to call various basic auth APIs on the Coordinator for sure. |
There was a problem hiding this comment.
I don't understand this suggestion.
There was a problem hiding this comment.
Ah, sorry for the confusion.
I was exploring the idea of trying out an alt account to bypass my GHA issue.
This is a dummy suggestion that I had requested @uds5501 to leave so that we could commit it and hopefully trigger CI. But that didn't work as only committers can commit from the GitHub console.
embedded-tests/src/test/java/org/apache/druid/testing/embedded/auth/BasicAuthIndexingTest.java
Outdated
Show resolved
Hide resolved
embedded-tests/src/test/java/org/apache/druid/testing/embedded/auth/BasicAuthMsqTest.java
Outdated
Show resolved
Hide resolved
embedded-tests/src/test/java/org/apache/druid/testing/embedded/auth/BasicAuthMsqTest.java
Outdated
Show resolved
Hide resolved
| // Time in ms to sleep after updating role permissions in each test. This intends to give the | ||
| // underlying test cluster enough time to sync permissions and be ready when test execution starts. | ||
| private static final int SYNC_SLEEP = 10000; | ||
| private static final int SYNC_SLEEP = 500; |
There was a problem hiding this comment.
I see the sleep is shorter now, but could it be made unnecessary? 500ms also seems dangerously short for a sleep that is actually required for some reason. It's short enough that some random slowness on the test runner will cause a test to flake.
There was a problem hiding this comment.
Fair point, let me check if there is any metric that we can watch.
| String task = createTaskString(); | ||
| StatusResponseHolder statusResponseHolder = overlordResourceTestClient.submitTaskAndReturnStatusWithAuth(task, USER_1, USER_1_PASSWORD); | ||
| Assert.assertEquals(HttpResponseStatus.FORBIDDEN, statusResponseHolder.getStatus()); | ||
| private Task createExportTask(String taskId) |
There was a problem hiding this comment.
The direct creation of the controller task is super verbose and a little brittle: this isn't meant to be a reliable Java API and using it creates coupling between this test and the main code.
How about going through SQL instead?
There was a problem hiding this comment.
Yeah, I think the test using this method is unnecessary itself.
There are already tests for verifying the SQL /v2/task endpoint with auth in this class.
And for testing out the taskPost API on OverlordResource, we have BasicAuthIndexingTest.
Should we just remove this altogether?
There was a problem hiding this comment.
Sure, you can remove it then. I don't think submitting a controller task adds meaningful testing if the /druid/v2/sql/task endpoint is already being tested. It will submit a task under the hood. And users aren't meant to be submitting controller tasks manually anyway.
embedded-tests/src/test/java/org/apache/druid/testing/embedded/auth/BasicAuthMsqTest.java
Outdated
Show resolved
Hide resolved
| import java.util.function.Function; | ||
|
|
||
| /** | ||
| * Client to call various basic auth APIs on the Coordinator. |
There was a problem hiding this comment.
I don't understand this suggestion.
|
@gianm, thanks for the review. I have updated the PR based on your feedback. |
5 participants
Main changes
ITSecurityBasicQuerytoBasicAuthMsqTestBasicAuthIndexingTestwhich extendsIndexTaskTestEmbeddedServiceClientto allow creation of a custom client that can talk to various services in an embedded clusterOther changes
SecurityClientto work withEmbeddedServiceClientCompactionResourceTestClientto work withEmbeddedServiceClientServiceClientinstances for Coordinator, Overlord and Broker inServiceClientModuleCoordinatorServiceClientwith@Coordinator ServiceClientMsqExportDirectoryThis PR has: