Skip to content

Check discovery config azure and access graph resources during integration DeleteAssociatedResources - #69608

Open
charlestp wants to merge 2 commits into
masterfrom
charles/integration-cascade-delete
Open

Check discovery config azure and access graph resources during integration DeleteAssociatedResources#69608
charlestp wants to merge 2 commits into
masterfrom
charles/integration-cascade-delete

Conversation

@charlestp

Copy link
Copy Markdown
Contributor

This was a fix I came across while implementing the new DiscoveryConfigChangeEvent. deleteAWSOIDCAssociatedResources wasn't checking Azure or AccessGraph dependencies, so it could delete discovery configs with other uses, or leave orphaned AccessGraph syncs. The permission check also drifted from how the Github integration check works. Since we're logging the discovery configs that block an integration delete in the error message, this should also require the list permission.

Manual Test Plan

Test Environment

Cloud staging tenant running local build

Test Cases

  • - Integration cascade delete
    • Config with UUID and only the integration gets deleted
    • Config with another integration blocks deletion
    • config with non UUID name is blocked
  • - Permission checks for cascade delete
    • Role with only integration delete fails. Needs list+delete on discovery configs, list+delete on app servers.
    • Role with discovery config list + delete fails. Still needs list + delete on app servers.
    • Role with integration delete, discovery config list + delete, app server list + delete succeeds.

@charlestp charlestp added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v18 labels Aug 12, 2026
@github-actions
github-actions Bot requested review from greedy52 and tele-lion August 12, 2026 18:28
len(a.Spec.Kube) == 0 &&
(a.Spec.AccessGraph == nil || len(a.Spec.AccessGraph.AWS) == 0)
(a.Spec.AccessGraph == nil ||
(len(a.Spec.AccessGraph.AWS) == 0 && len(a.Spec.AccessGraph.Azure) == 0))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

since it's only caller is removed should we deprecate IsMatchersEmpty ?

// HasOtherMatchers returns true if any matcher or Access Graph sync does not use
// the named integration. GCP and Kubernetes matchers cannot name an integration,
// so each one counts as another.
func (a *DiscoveryConfig) HasOtherMatchers(integration string) bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: not a blocker, but HasOtherMatchers seems ambiguous as it also checks Access Graph.

ExclusiveToIntegration reads a little clearer, but it's the inverse 🤔

@charlestp
charlestp force-pushed the charles/integration-cascade-delete branch from 6561b14 to b8d2089 Compare August 13, 2026 03:49
@charlestp
charlestp force-pushed the charles/integration-cascade-delete branch from b8d2089 to 5c3668b Compare August 13, 2026 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/branch/v18 no-changelog Indicates that a PR does not require a changelog entry size/md

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants