Skip to content

fix(monitors,synthetics_tests): drop stale restricted_roles/principals under new flag (3/4)#631

Merged
michael-richey merged 2 commits into
drop-unresolvable-principals-restriction-policiesfrom
drop-unresolvable-principals-monitors-synthetics
Jul 15, 2026
Merged

fix(monitors,synthetics_tests): drop stale restricted_roles/principals under new flag (3/4)#631
michael-richey merged 2 commits into
drop-unresolvable-principals-restriction-policiesfrom
drop-unresolvable-principals-monitors-synthetics

Conversation

@michael-richey

@michael-richey michael-richey commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Context

Stacked PR 3 of 4. Base: drop-unresolvable-principals-restriction-policies (PR #630).

This PR

Applies the same drop-aware pattern (PR #629 helpers) to the two resource types carrying both a flat restricted_roles list and a restriction_policy.bindings.principals composite:

  • monitors: restricted_roles + restriction_policy.bindings.principals
  • synthetics_tests: options.restricted_roles + restriction_policy.bindings.principals

Each connect_resources override keeps non-access-control connections on the generic path and filters access-control references through the shared helpers. An emptied list is a normal resource-connection failure: it skips by default, while --skip-failed-resource-connections=true intentionally continues with the explicit unrestricted-destination ERROR, risk metric, and end-of-run ERROR summary introduced in PR #629. extract_source_ids remains unaffected. Behavior is unchanged when the new drop flag is off.

Testing

The monitor and synthetics suites cover both composite and flat-list shapes, including drop, default empty-risk failure, suppressed empty-risk propagation, middle-element regression, and flag-off behavior. Focused tests green; ruff clean.

@michael-richey
michael-richey force-pushed the drop-unresolvable-principals-restriction-policies branch from c0fbb7e to 76bcbff Compare July 14, 2026 21:32
@michael-richey
michael-richey force-pushed the drop-unresolvable-principals-monitors-synthetics branch from 518113c to 27fd78f Compare July 14, 2026 21:32
@michael-richey
michael-richey force-pushed the drop-unresolvable-principals-restriction-policies branch from 76bcbff to d71030b Compare July 15, 2026 12:40
@michael-richey
michael-richey force-pushed the drop-unresolvable-principals-monitors-synthetics branch from 27fd78f to de6e34d Compare July 15, 2026 12:40
@michael-richey
michael-richey force-pushed the drop-unresolvable-principals-restriction-policies branch from d71030b to 8a645fa Compare July 15, 2026 13:37
@michael-richey
michael-richey force-pushed the drop-unresolvable-principals-monitors-synthetics branch from de6e34d to bd82dbe Compare July 15, 2026 13:37
@michael-richey
michael-richey marked this pull request as ready for review July 15, 2026 13:40
@michael-richey
michael-richey requested a review from a team as a code owner July 15, 2026 13:40
@michael-richey
michael-richey requested a review from Copilot July 15, 2026 13:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds drop-aware connect_resources overrides for monitors and synthetics tests so access-control references (flat restricted_roles and composite restriction_policy.bindings.principals) can optionally drop principals/roles that are absent from both source and destination under --drop-unresolvable-principals, while preserving existing failure behavior and explicitly flagging “empty restriction” access-elevation risk.

Changes:

  • Implemented drop-aware connect_resources overrides for monitors and synthetics_tests, routing non-access-control connections through the existing generic find_attr/connect_id path.
  • Applied shared drop-aware filtering helpers for both composite principals and flat restricted role lists, threading through empty-binding risk handling.
  • Added/extended unit tests for both resources to cover drop, flag-off hard-fail behavior, empty-binding risk behavior, suppression behavior, and middle-element regression.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
datadog_sync/model/monitors.py Adds drop-aware connect_resources override and keeps extract_source_ids semantics unchanged for minimize-reads correctness.
datadog_sync/model/synthetics_tests.py Adds drop-aware connect_resources override and documents unchanged extract_source_ids behavior for minimize-reads.
tests/unit/test_monitors.py Adds unit tests covering drop/hard-fail/empty-risk/suppressed-risk cases for monitor access-control shapes.
tests/unit/test_synthetics_tests.py Adds unit tests covering drop/hard-fail/empty-risk/suppressed-risk cases for synthetics test access-control shapes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/unit/test_synthetics_tests.py Outdated
@michael-richey
michael-richey force-pushed the drop-unresolvable-principals-monitors-synthetics branch from bd82dbe to 3699f48 Compare July 15, 2026 14:09
@michael-richey
michael-richey force-pushed the drop-unresolvable-principals-restriction-policies branch from 8a645fa to 6213034 Compare July 15, 2026 14:09
…s under new flag

Applies the same drop-aware pattern (PR1 helpers) to the two resource types that carry
both a flat `restricted_roles` list and a `restriction_policy.bindings.principals` composite:

- monitors: `restricted_roles` + `restriction_policy.bindings.principals`
- synthetics_tests: `options.restricted_roles` + `restriction_policy.bindings.principals`

Each `connect_resources` override keeps all non-access-control connections on the generic
path, filters access-control references through the shared helpers, and preserves the
empty-binding/empty-list access-elevation hard-fail. `extract_source_ids` left unaffected
(documented). Behavior is unchanged when `--drop-unresolvable-principals` is off.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
@michael-richey
michael-richey force-pushed the drop-unresolvable-principals-monitors-synthetics branch from 3699f48 to 63c0e2a Compare July 15, 2026 14:11
@michael-richey
michael-richey marked this pull request as draft July 15, 2026 14:28
@michael-richey
michael-richey marked this pull request as ready for review July 15, 2026 14:29
…oles under new flag (#632)

Completes the rollout by applying the flat-list drop-aware filter to the two remaining
resource types with a `restricted_roles` list:

- dashboards: `restricted_roles`
- synthetics_private_locations: `metadata.restricted_roles`

Both previously had trivial pass-through `connect_id` overrides; they now get a
`connect_resources` override that filters stale roles under `--drop-unresolvable-principals`
while keeping widget/other connections on the generic path and preserving the empty-list
access-elevation hard-fail. Adds a new unit test module for synthetics_private_locations.

Co-authored-by: Claude Sonnet 5 <[email protected]>
@michael-richey
michael-richey merged commit 0f5e61a into drop-unresolvable-principals-restriction-policies Jul 15, 2026
1 check passed
@michael-richey
michael-richey deleted the drop-unresolvable-principals-monitors-synthetics branch July 15, 2026 17:44
michael-richey added a commit that referenced this pull request Jul 15, 2026
…the whole policy (2/4) (#630)

* fix(restriction_policies): drop stale principals instead of skipping the whole policy

Overrides `connect_resources` so that, under `--drop-unresolvable-principals`, a principal
absent from both destination and source state (permanently gone — e.g. a role deleted
before the org's first-ever import) is dropped from its binding and the policy still syncs,
instead of the entire restriction policy being skipped on a single dead reference.

- Principals present in source but not destination keep today's hard-fail/retry behavior.
- If dropping empties a binding whose source list was non-empty, the resource is still
  skipped and flagged as an access-elevation risk (ERROR + metric), governed by
  `--skip-failed-resource-connections` like any other connection failure.
- The "id" (dashboard/slo/notebook) connections keep the generic path unchanged.
- `extract_source_ids` is intentionally left unaffected (documented) so minimize-reads
  lazy-loading can still resolve which principals to check.

Co-Authored-By: Claude Sonnet 5 <[email protected]>

* fix(monitors,synthetics_tests): drop stale restricted_roles/principals under new flag (3/4) (#631)

* fix(monitors,synthetics_tests): drop stale restricted_roles/principals under new flag

Applies the same drop-aware pattern (PR1 helpers) to the two resource types that carry
both a flat `restricted_roles` list and a `restriction_policy.bindings.principals` composite:

- monitors: `restricted_roles` + `restriction_policy.bindings.principals`
- synthetics_tests: `options.restricted_roles` + `restriction_policy.bindings.principals`

Each `connect_resources` override keeps all non-access-control connections on the generic
path, filters access-control references through the shared helpers, and preserves the
empty-binding/empty-list access-elevation hard-fail. `extract_source_ids` left unaffected
(documented). Behavior is unchanged when `--drop-unresolvable-principals` is off.

Co-Authored-By: Claude Sonnet 5 <[email protected]>

* fix(dashboards,synthetics_private_locations): drop stale restricted_roles under new flag (#632)

Completes the rollout by applying the flat-list drop-aware filter to the two remaining
resource types with a `restricted_roles` list:

- dashboards: `restricted_roles`
- synthetics_private_locations: `metadata.restricted_roles`

Both previously had trivial pass-through `connect_id` overrides; they now get a
`connect_resources` override that filters stale roles under `--drop-unresolvable-principals`
while keeping widget/other connections on the generic path and preserving the empty-list
access-elevation hard-fail. Adds a new unit test module for synthetics_private_locations.

Co-authored-by: Claude Sonnet 5 <[email protected]>

---------

Co-authored-by: Claude Sonnet 5 <[email protected]>

---------

Co-authored-by: Claude Sonnet 5 <[email protected]>
michael-richey added a commit that referenced this pull request Jul 15, 2026
…ffolding (1/4) (#629)

* feat: add --drop-unresolvable-principals flag and drop-aware connection scaffolding

Adds the opt-in `--drop-unresolvable-principals` flag plus the shared, inert-when-off
machinery that later PRs wire into individual resource models. No behavior changes when
the flag is absent (the default).

- New `--drop-unresolvable-principals` CLI option (in `_diffs_options`, so it applies to
  sync/diffs/migrate) threaded through the `Configuration` dataclass.
- `BaseResource._resolve_or_drop`: three-way resolver distinguishing destination-present,
  source-present ("not yet synced", hard-fail as today), and permanently-gone
  (absent from both — droppable only under the flag).
- Shared `_filter_stale_binding_principals` / `_filter_stale_flat_roles` /
  `_raise_connection_error_if_any` helpers (rebuild lists rather than mutating in place,
  avoiding the enumerate/index-shift bug).
- `ResourceConnectionError.empty_binding_risk` flag for the access-elevation case.
- New `Counter` buckets (stale drops, empty-binding risks) + reset, surfaced by
  `_emit_apply_summary`; `config.counter` back-reference so models can record drops.
- `_apply_resource_cb` adds a `risk:empty_restriction_policy` metric tag when a binding
  emptied out.

Co-Authored-By: Claude Sonnet 5 <[email protected]>

* fix: report suppressed empty-binding connection risks

* fix: recheck destination after lazy state load

* refactor: name resource connection results

* fix(restriction_policies): drop stale principals instead of skipping the whole policy (2/4) (#630)

* fix(restriction_policies): drop stale principals instead of skipping the whole policy

Overrides `connect_resources` so that, under `--drop-unresolvable-principals`, a principal
absent from both destination and source state (permanently gone — e.g. a role deleted
before the org's first-ever import) is dropped from its binding and the policy still syncs,
instead of the entire restriction policy being skipped on a single dead reference.

- Principals present in source but not destination keep today's hard-fail/retry behavior.
- If dropping empties a binding whose source list was non-empty, the resource is still
  skipped and flagged as an access-elevation risk (ERROR + metric), governed by
  `--skip-failed-resource-connections` like any other connection failure.
- The "id" (dashboard/slo/notebook) connections keep the generic path unchanged.
- `extract_source_ids` is intentionally left unaffected (documented) so minimize-reads
  lazy-loading can still resolve which principals to check.

Co-Authored-By: Claude Sonnet 5 <[email protected]>

* fix(monitors,synthetics_tests): drop stale restricted_roles/principals under new flag (3/4) (#631)

* fix(monitors,synthetics_tests): drop stale restricted_roles/principals under new flag

Applies the same drop-aware pattern (PR1 helpers) to the two resource types that carry
both a flat `restricted_roles` list and a `restriction_policy.bindings.principals` composite:

- monitors: `restricted_roles` + `restriction_policy.bindings.principals`
- synthetics_tests: `options.restricted_roles` + `restriction_policy.bindings.principals`

Each `connect_resources` override keeps all non-access-control connections on the generic
path, filters access-control references through the shared helpers, and preserves the
empty-binding/empty-list access-elevation hard-fail. `extract_source_ids` left unaffected
(documented). Behavior is unchanged when `--drop-unresolvable-principals` is off.

Co-Authored-By: Claude Sonnet 5 <[email protected]>

* fix(dashboards,synthetics_private_locations): drop stale restricted_roles under new flag (#632)

Completes the rollout by applying the flat-list drop-aware filter to the two remaining
resource types with a `restricted_roles` list:

- dashboards: `restricted_roles`
- synthetics_private_locations: `metadata.restricted_roles`

Both previously had trivial pass-through `connect_id` overrides; they now get a
`connect_resources` override that filters stale roles under `--drop-unresolvable-principals`
while keeping widget/other connections on the generic path and preserving the empty-list
access-elevation hard-fail. Adds a new unit test module for synthetics_private_locations.

Co-authored-by: Claude Sonnet 5 <[email protected]>

---------

Co-authored-by: Claude Sonnet 5 <[email protected]>

---------

Co-authored-by: Claude Sonnet 5 <[email protected]>

---------

Co-authored-by: Claude Sonnet 5 <[email protected]>
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.

3 participants