Skip to content
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

Marking .opensearch-knn-models as a system index causes failures in k-NN plugin #2478

Closed
jmazanec15 opened this issue Feb 23, 2023 · 8 comments
Assignees
Labels
bug Something isn't working triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. v2.7.0

Comments

@jmazanec15
Copy link
Member

Hey team, we recently onboarded k-NN plugin onto security (#2265). For 2.6 release, we saw some unauthorized failure exceptions from integ tests with security enabled: https://build.ci.opensearch.org/blue/rest/organizations/jenkins/pipelines/integ-test/runs/3826/nodes/134/steps/1083/log/?start=0.

The exceptions look something like:

  2> REPRODUCE WITH: ./gradlew ':integTest' --tests "org.opensearch.knn.index.FaissIT.testEndToEnd_fromModel" -Dtests.seed=60BB328192F52504 -Dtests.security.manager=false -Dtests.locale=bg-BG -Dtests.timezone=America/Santarem -Druntime.java=14
  2> 

org.opensearch.client.ResponseException: method [POST], host [https://localhost:9200], URI [/_plugins/_knn/models/test-model/_train], status line [HTTP/1.1 403 Forbidden]
    {"error":{"root_cause":[{"type":"security_exception","reason":"no permissions for [] and User [name=admin, backend_roles=[admin], requestedTenant=null]"}],"type":"security_exception","reason":"no permissions for [] and User [name=admin, backend_roles=[admin], requestedTenant=null]"},"status":403}
        at app//org.opensearch.client.RestClient.convertResponse(RestClient.java:375)
        at app//org.opensearch.client.RestClient.performRequest(RestClient.java:345)
        at app//org.opensearch.client.RestClient.performRequest(RestClient.java:320)
        at app//org.opensearch.knn.KNNRestTestCase.trainModel(KNNRestTestCase.java:1089)
        at app//org.opensearch.knn.index.FaissIT.testEndToEnd_fromModel(FaissIT.java:240)

org.opensearch.client.ResponseException: method [DELETE], host [https://localhost:9200], URI [/.opensearch-knn-models], status line [HTTP/1.1 403 Forbidden]
    {"error":{"root_cause":[{"type":"security_exception","reason":"no permissions for [] and User [name=admin, backend_roles=[admin], requestedTenant=null]"}],"type":"security_exception","reason":"no permissions for [] and User [name=admin, backend_roles=[admin], requestedTenant=null]"},"status":403}

I thought that by default the admin backend role should have these permissions. The second failure is related to an attempt to delete the system index as part of the ODFERestTestCase.

I am wondering if I missed a step in plugin onboarding outside of #2265 ?

@github-actions github-actions bot added the untriaged Require the attention of the repository maintainers and may need to be prioritized label Feb 23, 2023
peternied added a commit that referenced this issue Feb 24, 2023
)"

This reverts commit 6ded82d.

Resolves #2478

Signed-off-by: Peter Nied <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
@peternied
Copy link
Member

Have you been able to run the tests with the revert of this change applied. How do we know this change is responsible for the issues in the integration tests - could you provide those details?

@peternied Sure, the problem is that in this change, the ".opensearch-knn-models" index is marked as a system index in security demo. With this, there are protections that come into play. We were seeing the following error:

[2023-02-23T22:29:33,608][ERROR][o.o.k.t.TrainingJobRunner] [] Unable to initialize model serialization: no permissions for [] and User [name=admin, backend_roles=[admin], requestedTenant=null]

This was happening because our training api internally will make a request to index a document into our system index: https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/indices/ModelDao.java#L341. However, this is failing because only super admin users are able to add documents into system indices - here we are just using the normal client, not admin. I am not sure how to fix this yet, as it is not possible to index through the admin client. Im discussing with the AD team, but that might take some time.

Im confident this change won't cause any other problems by removing system index status. Without this protection, this class wont get invoked: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java#L124, avoiding the issue.

We were also getting an error deleting system indices, but that can be resolved by setting up the required certs similar to AD: https://github.com/opensearch-project/anomaly-detection/tree/main/src/test/resources/security.

From #2480

The revert has been pushed to the 2.6 branch 766cf3f

Thanks, jmazanec15 and VijayanB - please work with the release team to get the updated build and confirm the integration tests are fixed.

For adhoc testing, we've published the updated plugin, download

@peternied peternied added bug Something isn't working v2.6.0 'Issues and PRs related to version v2.6.0' and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Feb 24, 2023
@peternied peternied changed the title Question on admin backend role for demo k-NN Marking .opensearch-knn-models as a system index causes failures in k-NN plugin Feb 24, 2023
@cwperks
Copy link
Member

cwperks commented Feb 24, 2023

@peternied Reverting the whole change includes the new roles definitions for k-NN. Should the roles be retained for the release?

@peternied
Copy link
Member

@jmazanec15 @VijayanB what do you think about those role definitions being removed?

@jmazanec15
Copy link
Member Author

Sorry, I think that is okay @peternied. We will add it for next release. This wasnt a particular user ask - just wanted to get k-NN onboarded to security. I rather do it completely than partially.

@stephen-crawford
Copy link
Contributor

[Triage] This issue will be relevant as part of the 2.7 release. I am going to assign @jmazanec15 since it seems like they (you) are driving this issue. Thank you.

@stephen-crawford stephen-crawford added v2.7.0 triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. and removed v2.6.0 'Issues and PRs related to version v2.6.0' labels Feb 27, 2023
wuychn pushed a commit to ochprince/security that referenced this issue Mar 16, 2023
@davidlago
Copy link

@jmazanec15 gentle reminder that 2.7 code freeze is coming up next week. Where are we with this issue? Have the roles been added via a separate issue and are we good to close this one?

@jmazanec15
Copy link
Member Author

Right, this should be fixed with opensearch-project/k-NN#849 but will let @martin-gaievski confirm everything is working as expected.

@davidlago
Copy link

Alright, thanks @jmazanec15. Please re-open if you find there are still issues that need fixing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. v2.7.0
Projects
None yet
Development

No branches or pull requests

5 participants