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

Explicitly use the alloc op context for dpd nat updates #4654

Merged
merged 6 commits into from
Dec 11, 2023

Conversation

zephraph
Copy link
Contributor

@zephraph zephraph commented Dec 8, 2023

There was an issue in main where deleting an instance failed w/ a 403. Digging in further it looked like the 403 was coming from the instance delete saga, specifically the InstanceDeleteNat step that was added in #4610. It turns out that called instance_delete_dpd_config which called boundary_switches with the normal opcontext. Boundary switches calls list_switch_ports_with_uplinks and that requires fleet level permissions.

The fix here was to pass the opcontext_alloc which gives higher level permissions down the boundary_switches call chain.

I believe that we should ultimately try to figure out some way to encode our authz state into the type system such that you can't just pass down a generic opcontext. I'm not exactly sure how we would do that, but it seems worth pursuing.

@zephraph
Copy link
Contributor Author

zephraph commented Dec 8, 2023

I think there's a test failure here. If anyone feels like getting this over the line, please, be my guest.

@zephraph
Copy link
Contributor Author

@jmpesp I appreciate you!

Copy link
Collaborator

@smklein smklein left a comment

Choose a reason for hiding this comment

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

LGTM, only caveat being "maybe we don't pass the opctx as an argument". But the test makes sense!

@zephraph
Copy link
Contributor Author

I updated to used the recommended approach (which I just realized was also done in other places).

@zephraph zephraph enabled auto-merge (squash) December 11, 2023 17:48
@zephraph zephraph merged commit baf7347 into main Dec 11, 2023
19 of 20 checks passed
@zephraph zephraph deleted the fix-instance-delete branch December 11, 2023 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants