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

[affinity] Allow listing affinity/anti-affinity groups per-instance #7857

Merged
merged 229 commits into from
Mar 25, 2025

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Mar 21, 2025

Part of #7614

@david-crespo
Copy link
Contributor

This will make for a great Affinity tab on the instance page in the console.

@smklein smklein marked this pull request as ready for review March 21, 2025 22:51
@smklein smklein requested a review from david-crespo March 21, 2025 22:52
create_default_ip_pool(&client).await;
api.create_project(PROJECT_NAME).await;

let project_api = api.use_project::<T>(PROJECT_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

WHAT IS THIS BEAUTIFUL API

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

a hackery of query parameter appending for the affinity tests... but feel free to re-use it, if you want!

pagparams: &PaginatedBy<'_>,
) -> ListResultVec<db::model::AntiAffinityGroup> {
let (.., authz_instance) =
instance_lookup.lookup_for(authz::Action::ListChildren).await?;
Copy link
Contributor

@david-crespo david-crespo Mar 24, 2025

Choose a reason for hiding this comment

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

Should this (also or instead) check list children on projects since the groups are children of the project? I think that check is implied by this check because anyone with viewer+ on the project has list children on the project and the instance, but that's sort of incidental. It might be better if this check was more direct.

resource Project {
permissions = [
"list_children",
"modify",
"read",
"create_child",
];
roles = [ "admin", "collaborator", "viewer" ];
# Roles implied by other roles on this resource
"viewer" if "collaborator";
"collaborator" if "admin";
# Permissions granted directly by roles on this resource
"list_children" if "viewer";
"read" if "viewer";
"create_child" if "collaborator";
"modify" if "admin";

// If this resource is directly inside a Project, we only need to define
// permissions that are contingent on having roles on that Project.
(PolarSnippet::InProject, "Project") => format!(
r#"
resource {} {{
permissions = [
"list_children",
"modify",
"read",
"create_child",
];
relations = {{ containing_project: Project }};
"list_children" if "viewer" on "containing_project";
"read" if "viewer" on "containing_project";
"modify" if "collaborator" on "containing_project";
"create_child" if "collaborator" on "containing_project";
}}
has_relation(parent: Project, "containing_project", child: {})
if child.project = parent;
"#,
resource_name, resource_name,
),

Copy link
Contributor

Choose a reason for hiding this comment

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

I see we do the same for listing instance disks, which are directly analogous from an authz POV. Seems fine?

/// Lists disks attached to the instance.
pub(crate) async fn instance_list_disks(
&self,
opctx: &OpContext,
instance_lookup: &lookup::Instance<'_>,
pagparams: &PaginatedBy<'_>,
) -> ListResultVec<db::model::Disk> {
let (.., authz_instance) =
instance_lookup.lookup_for(authz::Action::ListChildren).await?;
self.db_datastore
.instance_list_disks(opctx, &authz_instance, pagparams)
.await
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gonna leave this as-is -- we can revisit both together, but keeping them uniform seems fine for now?

group_dsl::name,
&pagparams.map_name(|n| Name::ref_cast(n)),
),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I typed this up

Maybe this is just necessary to make the compiler happy, but what does it mean to paginate by either name or ID here? Is it possible to select one or the other?

but I answered my own question. Putting it here for posterity: the use of PaginatedByNameOrId in the query params on the endpoint means this takes a sort_by=NameOrIdSortMode param that lets you specify sort by ID, name asc, or name desc.

Copy link
Contributor

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

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

Look great, could possibly use one small authz tweak.

Base automatically changed from affinity-query-commentary to main March 25, 2025 18:06
@smklein smklein enabled auto-merge (squash) March 25, 2025 21:31
@smklein smklein merged commit f675788 into main Mar 25, 2025
17 checks passed
@smklein smklein deleted the groups-for-instance branch March 25, 2025 23:10
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.

2 participants