search

irods / irods_rule_engine_plugin_logical_quotas

BSD 3-Clause "New" or "Revised" License
1 stars 9 forks source link

Bundle of fixes (main) #110

Closed korydraughn closed 3 weeks ago

korydraughn commented 3 weeks ago

All tests passed.

Have to add a test for the commit containing (TEST THIS) in the commit message.

korydraughn commented 3 weeks ago

After some discussion, we've decided to drop the commit containing (TEST THIS).

Original work can be found here: https://github.com/korydraughn/irods_rule_engine_plugin_logical_quotas/tree/return_violation_on_matching_total_max_data_size.m

korydraughn commented 3 weeks ago

The whole nested shared monitored collections thing was added because the plugin uses the RsComm as-is to update quotas. If the user has permission to write to a collection under someone else's collection, updating the quota of the collection will work. However, when the plugin starts walking up the logical path to update quotas on parent-parent collections, that user may NOT have permission to update those which will lead to an error.

This is entirely avoidable by creating a client-side connection as the service account and updating the quotas using the admin flag. Doing that will slow things down due to TCP connection setup/teardown, but it is guaranteed to avoid the issue described. Using a new connection also means the server can exhaust its resources if there's a lot of activity happening on the server.

If use of a new client connection isn't ideal, then it's on the admin to manage the permissions and decide whether nested shared monitored collections should be allowed.

Thoughts?

trel commented 3 weeks ago

Can we walk up the tree as-is, and then, if a permission error is hit, switch to adminFlag? So we get speed for normal cases, and then have a silent, successful fallback for the scenario you describe?

korydraughn commented 3 weeks ago

That is possible. Just need to determine if that's a simple/quick tweak or not.

trel commented 3 weeks ago

It's a complex / not-obvious scenario. So, it can wait if there are other things, I think.

korydraughn commented 3 weeks ago

Not a blocker for sure. It was something that crossed my mind. I'll put it in an issue so it's not lost.

You agree with the additional wording in the README?

trel commented 3 weeks ago

You agree with the additional wording in the README?

Sure. This is good - and helps people think about ramifications of alternate approaches.

korydraughn commented 3 weeks ago

All tests passed.

Squashing.

korydraughn commented 3 weeks ago

Squashed.

korydraughn commented 3 weeks ago

Tweaked a test name. Will pound after the tests come back as passing.

korydraughn commented 3 weeks ago

Pounded.

korydraughn commented 3 weeks ago

Merging.