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

Loosen up Owner-Child resource subscription checks #4343

Merged
merged 7 commits into from
Dec 3, 2024

Conversation

super-harsh
Copy link
Collaborator

@super-harsh super-harsh commented Oct 15, 2024

Closes #3754

What this PR does / why we need it:

We introduced this check a while ago with single-operator multitenancy credentials to prevent users from creating resources in a subscription that doesn't match the subscription of the owner resource. However, there could be scenarios where owner and child resources could be provisioned in distinct subscriptions.

This PR is to loosen up check between Owner-Child resource subscription mismatch for ARMID owner references. From a security perspective Azure has access checks and the operation is not allowed, it'll be rejected at ARM level.

Special notes for your reviewer:

Will add documentation after we make a decision on taking the change

If applicable:

  • this PR contains tests

Copy link
Member

@theunrepentantgeek theunrepentantgeek left a comment

Choose a reason for hiding this comment

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

This just removes the restriction - I thought we discussed yesterday that we needed it when the owner was within the cluster, but possibly loosen it when the owner is referenced directly by ARM ID.

@super-harsh
Copy link
Collaborator Author

@theunrepentantgeek I've made the change as per above.

Copy link
Member

@matthchr matthchr left a comment

Choose a reason for hiding this comment

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

LGTM, with one minor comment

v2/internal/controllers/owner_arm_id_negative_test.go Outdated Show resolved Hide resolved
v2/internal/resolver/resource_hierarchy.go Show resolved Hide resolved
@super-harsh super-harsh added this pull request to the merge queue Dec 3, 2024
Merged via the queue into main with commit 79d125c Dec 3, 2024
7 checks passed
@super-harsh super-harsh deleted the improve/subscription-checks branch December 3, 2024 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Recently Completed
Development

Successfully merging this pull request may close these issues.

Bug: remove subscription check between resource/owner for DNS records
3 participants