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

Transaction-safe ObjectRole creation #281

Merged
merged 3 commits into from
Apr 8, 2024

Conversation

AlanCoding
Copy link
Member

I got a report from the UI that doing multiple assignments concurrently would give server errors inconsistently. It actually made complete sense that this would be the case, and I developed a reproducer that identifies the obvious suspect.

We can't put this reproducer in a test, because the test works inside of a transaction. Nested transactions (in postgres/django) do not work the same as independent transactions (auto-commit outside the context). Because of that, these are ran in 2 different terminal tabs.

from django.contrib.contenttypes.models import ContentType

# for process 1
org = Organization.objects.create(name='repro')
teams = [Team.objects.create(name=f'repro-{i}', organization=org) for i in range(5)]
inv = Inventory.objects.create(organization=org, name='for-repro')
rd = RoleDefinition.objects.create_from_permissions(name='inv-view', permissions=['view_inventory'], content_type=ContentType.objects.get_for_model(Inventory))

# for other process
org = Organization.objects.get(name='repro')
teams = [Team.objects.get(name=f'repro-{i}', organization=org) for i in range(5)]
inv = Inventory.objects.get(organization=org, name='for-repro')
rd = RoleDefinition.objects.get(name='inv-view')

Reproducer, these lines all have to happen sequentially. You're flipping between 2 tabs. shell_plus works best.

# process 1
from django.db import transaction
transaction.atomic().__enter__()
assignment = rd.give_permission(teams[0], inv)

# process 2
from django.db import transaction
with transaction.atomic():
  assignment = rd.give_permission(teams[1], inv)

# back to process 2
# stop here
transaction.atomic().__exit__(None, None, None)

Once you run the final command, "process 2" hits this error:

Cell In[14], line 2
      1 with transaction.atomic():
----> 2   assignment = rd.give_permission(teams[1], inv)

File ~/repos/awx/testing/django-ansible-base/ansible_base/rbac/models.py:185, in RoleDefinition.give_permission(self, actor, content_object)
    184 def give_permission(self, actor, content_object):
--> 185     return self.give_or_remove_permission(actor, content_object, giving=True)

File ~/repos/awx/testing/django-ansible-base/ansible_base/rbac/models.py:203, in RoleDefinition.give_or_remove_permission(self, actor, content_object, giving, sync_action)
    201     if not giving:
    202         return  # nothing to do
--> 203     object_role = ObjectRole.objects.create(**kwargs)
    204     created = True
    206 from ansible_base.rbac.triggers import needed_updates_on_assignment, update_after_assignment

<snip>

File ~/venvs/awx/lib64/python3.11/site-packages/psycopg/cursor.py:732, in Cursor.execute(self, query, params, prepare, binary)
    728         self._conn.wait(
    729             self._execute_gen(query, params, prepare=prepare, binary=binary)
    730         )
    731 except e._NO_TRACEBACK as ex:
--> 732     raise ex.with_traceback(None)
    733 return self

IntegrityError: duplicate key value violates unique constraint "one_object_role_per_object_and_role"
DETAIL:  Key (object_id, content_type_id, role_definition_id)=(4, 34, 6) already exists.

I developed a test that uses mock to approximate this, although I would not trust such a test to fully verify the fix. Like, it's non-obvious that the .get after the IntegrityError would actually be able to fetch the object. I re-ran the above reproducer and every worked and I'm fairly satisfied with that.

Internal AAP-22518

@AlanCoding AlanCoding marked this pull request as ready for review April 8, 2024 03:05
@AlanCoding AlanCoding added the Ready for review This PR is ready for review either initially or comments have been address label Apr 8, 2024
@AlanCoding AlanCoding enabled auto-merge (squash) April 8, 2024 21:45
@AlanCoding AlanCoding merged commit 338657e into ansible:devel Apr 8, 2024
6 checks passed
Copy link

sonarcloud bot commented Apr 8, 2024

AlanCoding added a commit that referenced this pull request May 24, 2024
We hit another version of
#281 from out in the
wild.

The report of this is ansible/awx#15185, and
although it is short on specific of what went on, with some speculation
I was able to mostly logically brute-force out the probable cause. The
imagined reproducer:

- In the "old" AWX UI, go to give a team permission to 2 different roles
to the same thing, like inventory update and adhoc roles.
 - It is assumed that this team has members already.
 - Submit the requests to assign those permissions concurrently.

This fixes that issue by ignoring conflicts on bulk creation of role evaluations.
thedoubl3j pushed a commit to thedoubl3j/django-ansible-base that referenced this pull request Jun 18, 2024
…e#401)

We hit another version of
ansible#281 from out in the
wild.

The report of this is ansible/awx#15185, and
although it is short on specific of what went on, with some speculation
I was able to mostly logically brute-force out the probable cause. The
imagined reproducer:

- In the "old" AWX UI, go to give a team permission to 2 different roles
to the same thing, like inventory update and adhoc roles.
 - It is assumed that this team has members already.
 - Submit the requests to assign those permissions concurrently.

This fixes that issue by ignoring conflicts on bulk creation of role evaluations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for review This PR is ready for review either initially or comments have been address
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants