[Feature/multi_tenancy] Delay context restoring for initializing master key and deleting agent

[dbwiddis]

Description

Fixes too-eager restoring of context in cases where it needed to still be stashed

Check List

• New functionality includes testing. (Separately in [Feature/multi_tenancy] Tenant-aware integration tests for Connector, Model, Agent, Model Groups #2818)
• Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[dbwiddis]

Windows failing on

REPRODUCE WITH: gradlew ':opensearch-ml-plugin:test' --tests "org.opensearch.ml.action.model_group.SearchModelGroupITTests.test_term_search" -Dtests.seed=57E7F417B46750BC -Dtests.security.manager=false -Dtests.locale=lv -Dtests.timezone=Kwajalein -Druntime.java=17

Linux failing on

REPRODUCE WITH: ./gradlew ':opensearch-ml-plugin:integTest' --tests "org.opensearch.ml.rest.RestMLGetTaskActionIT.testGetTaskAPI_Success" -Dtests.seed=34FFF2F5CDF198BA -Dtests.security.manager=false -Dtests.locale=ar-DZ -Dtests.timezone=America/Curacao -Druntime.java=21

Neither is associated with this PR and pass on the alternate OS

[dhrubo-os]

Or should we stash the context again before the deletion?

[dbwiddis]

This particular branch doesn't do the deletion, it's returning the 403 message because you are not superadmin and the model is hidden. The else branch keeps the context stashed and does the deletion.

[dhrubo-os]

Yeah, I know but I wasn't able to comment there as that wasn't a modified code section. In summary, I was just wondering if we should add context restore in multiple places? Or should we just stash again before performing any DB call. That will keep the stash context scope smaller and more secured IMO.

[dbwiddis]

The previous code used runBefore() on the action listener which basically just delayed restoring it until the action lister was completed (with success or failure). I was somewhat trying to restore earlier with the initial code but it was too early. This delays it for when it's needed.

I can probably put it back closer to the runBefore() implementation which is the opposite direction of your wondering... or since it's used in try-with-resources, it actually will be automatically restored as part of that auto-closure, so manually doing anything to it isn't actually required here.

I'm happy to go either way, which do you prefer?

[dhrubo-os]

I agree with you. Let's put it back closer to the runBefore() and then we can make the changes in main to keep the context scope smaller.