Skip to content

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

Merged
michael-richey merged 2 commits into
drop-unresolvable-principals-scaffoldingfrom
drop-unresolvable-principals-restriction-policies
Jul 15, 2026
Merged

fix(restriction_policies): drop stale principals instead of skipping the whole policy (2/4)#630
michael-richey merged 2 commits into
drop-unresolvable-principals-scaffoldingfrom
drop-unresolvable-principals-restriction-policies

Conversation

@michael-richey

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

Copy link
Copy Markdown
Collaborator

Context

Stacked PR 2 of 4. Base: drop-unresolvable-principals-scaffolding (PR #629).

This PR

Overrides connect_resources on RestrictionPolicies so that, under --drop-unresolvable-principals, a principal absent from both destination and source is dropped from its binding and the policy still syncs, instead of the whole 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, it is treated as a normal resource-connection failure. Without --skip-failed-resource-connections, the resource is skipped. With that flag enabled, the failure is intentionally suppressed and sync continues; an immediate ERROR explicitly says DESTINATION RESOURCE MAY BE UNRESTRICTED, and the successful action receives the dedicated risk metric and end-of-run ERROR summary.
  • The "id" (dashboard/slo/notebook) connections keep the generic path unchanged.
  • extract_source_ids intentionally remains unaffected so --minimize-reads lazy-loading can still resolve which principals to check.

Testing

TestRestrictionPoliciesConnectResources covers flag off, drop-and-continue, source-present hard-fail, both empty-binding connection modes, metric/log propagation, multi-binding partial-empty, middle-element index-shift regression, extract_source_ids, org/non-composite pass-through, and dangling IDs. Focused tests green; ruff clean.

@michael-richey
michael-richey force-pushed the drop-unresolvable-principals-scaffolding branch from 7db6366 to a7514ad Compare July 14, 2026 21:32
@michael-richey
michael-richey force-pushed the drop-unresolvable-principals-restriction-policies branch 3 times, most recently from d71030b to 8a645fa Compare July 15, 2026 13:37
@michael-richey
michael-richey marked this pull request as ready for review July 15, 2026 13:39
@michael-richey
michael-richey requested a review from a team as a code owner July 15, 2026 13:39
@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 resource-connection behavior for restriction policies so that, when --drop-unresolvable-principals is enabled, principals missing from both source and destination are removed from bindings (instead of skipping the entire restriction policy), while still preserving the existing hard-fail behavior for “pending” (source-present, destination-missing) principals and for non-principal ID connections.

Changes:

  • Override RestrictionPolicies.connect_resources() to special-case attributes.bindings.principals and delegate principal filtering to the shared drop-aware binding helper.
  • Preserve the existing id-based connection behavior (dashboards/SLOs/notebooks) via the generic find_attr/connect_id path.
  • Add a comprehensive unit test suite covering flag-off behavior, drop-and-continue behavior, hard-fail conditions, empty-binding risk/escalation behavior, and regression cases.

Reviewed changes

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

File Description
datadog_sync/model/restriction_policies.py Overrides connect_resources() to drop stale principals per-binding while keeping generic ID connection logic unchanged.
tests/unit/test_restriction_policies.py Adds unit tests validating drop/keep/hard-fail semantics, empty-binding escalation behavior, and key regressions.

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

Comment thread datadog_sync/model/restriction_policies.py Outdated
…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]>
@michael-richey
michael-richey force-pushed the drop-unresolvable-principals-restriction-policies branch from 8a645fa to 6213034 Compare July 15, 2026 14:09
@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
…s 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]>
@michael-richey
michael-richey merged commit c425427 into drop-unresolvable-principals-scaffolding Jul 15, 2026
1 check passed
@michael-richey
michael-richey deleted the drop-unresolvable-principals-restriction-policies branch July 15, 2026 17:45
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