Skip to content

fix(roles): guard built-in Datadog roles from create and update#562

Merged
michael-richey merged 3 commits into
mainfrom
michael.richey/guard-builtin-roles-all-ops
May 13, 2026
Merged

fix(roles): guard built-in Datadog roles from create and update#562
michael-richey merged 3 commits into
mainfrom
michael.richey/guard-builtin-roles-all-ops

Conversation

@michael-richey
Copy link
Copy Markdown
Collaborator

@michael-richey michael-richey commented May 13, 2026

Summary

The 3 built-in Datadog roles (Datadog Admin Role, Datadog Read Only Role, Datadog Standard Role) cannot be created, updated, or deleted via the API. Previously only delete_resource raised SkipResource for these names; create_resource and update_resource would still attempt the request and fail (the existing managed attribute check only short-circuited when the destination role happened to carry it).

This PR adds name-based BUILTIN_ROLE_NAMES guards across all three CRUD paths in datadog_sync/model/roles.py:

  • delete_resource — unchanged; existing guard still raises SkipResource.
  • update_resource — raises SkipResource at the top. Safe because the handler only invokes update when state mapping already exists (see resources_handler.py:247), so skipping preserves the existing mapping.
  • create_resource — does not unconditionally skip. Built-in roles always exist at the destination, and dependent resources (users, restriction policies, authn mappings, etc.) resolve role IDs via state.destination["roles"][source_id]. Skipping outright would break those dependents. Instead:
    • Raise SkipResource only if the built-in is somehow not present at the destination (defensive).
    • Otherwise fall through to the existing "role already exists at destination" branch and short-circuit before the diff/update logic, returning the destination role data so the wrapper records the mapping into state without making any API call.

Test plan

  • tox -e ruff,black clean
  • tox -e py313 -- tests/unit/test_roles.py — 14/14 pass
  • Full unit suite — 569/569 pass (no regressions)
  • Integration cassettes for test_roles contain zero POST/PATCH/DELETE; the two update-related integration tests are already @pytest.mark.skip (RBAC disabled in test org), so no integration impact
  • New unit test test_create_maps_existing_builtin_role_without_api_call exercises the load-bearing case: state mapping is recorded via _create_resource, post/patch never awaited
  • Green/green coverage: non-built-in roles still create/update/delete normally

🤖 Generated with Claude Code

michael-richey and others added 3 commits May 13, 2026 14:14
The 3 built-in Datadog roles (Admin, Read Only, Standard) cannot be
created, updated, or deleted via the API. Only delete was being guarded
with a SkipResource check; create and update would still attempt the
request and fail. Add the same name-based guard to create_resource and
update_resource so all three operations short-circuit cleanly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cover all 3 built-in role names across create, update, and delete with
parametrized SkipResource assertions, plus green/green cases confirming
non-built-in role names proceed through to the destination client.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@michael-richey michael-richey marked this pull request as ready for review May 13, 2026 18:41
@michael-richey michael-richey requested a review from a team as a code owner May 13, 2026 18:41
@michael-richey michael-richey merged commit 70da379 into main May 13, 2026
20 of 21 checks passed
@michael-richey michael-richey deleted the michael.richey/guard-builtin-roles-all-ops branch May 13, 2026 19:16
@michael-richey michael-richey mentioned this pull request May 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants