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.

Sign up for GitHub

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

Jump to bottom

acl: Avoid allocations when checking implied ops #12114

Merged
merged 1 commit into from
Jul 17, 2023
Merged

acl: Avoid allocations when checking implied ops #12114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 0 additions & 23 deletions src/v/security/acl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,29 +97,6 @@ enum class acl_operation : int8_t {
idempotent_write = 10,
};

/*
* Compute the implied operations based on the specified operation.
*/
inline std::vector<acl_operation> acl_implied_ops(acl_operation operation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i suppose you could return const-ref iterator pointers to static constexpr arrays as a std::pair<> but this is a good design. lgtm.

switch (operation) {
case acl_operation::describe:
return {
acl_operation::describe,
acl_operation::read,
acl_operation::write,
acl_operation::remove,
acl_operation::alter,
};
case acl_operation::describe_configs:
return {
acl_operation::describe_configs,
acl_operation::alter_configs,
};
default:
return {operation};
}
}

inline std::ostream& operator<<(std::ostream& os, acl_operation op) {
switch (op) {
case acl_operation::all:
Expand Down
45 changes: 37 additions & 8 deletions src/v/security/authorizer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,17 +111,46 @@ class authorizer final {
}

// check for allow
auto ops = acl_implied_ops(operation);
return std::any_of(
ops.cbegin(),
ops.cend(),
[&acls, &principal, &host](acl_operation operation) {
return acls.contains(
operation, principal, host, acl_permission::allow);
});
return acl_any_implied_ops_allowed(acls, principal, host, operation);
}

private:
/*
* Compute whether the specified operation is allowed based on the implied
* operations.
*/
bool acl_any_implied_ops_allowed(
const acl_matches& acls,
const acl_principal& principal,
const acl_host& host,
const acl_operation operation) const {
auto check_op = [&acls, &principal, &host](acl_operation operation) {
return acls.contains(
operation, principal, host, acl_permission::allow);
};

switch (operation) {
case acl_operation::describe: {
static constexpr std::array ops = {
acl_operation::describe,
acl_operation::read,
acl_operation::write,
acl_operation::remove,
acl_operation::alter,
};
return std::any_of(ops.begin(), ops.end(), check_op);
}
case acl_operation::describe_configs: {
static constexpr std::array ops = {
acl_operation::describe_configs,
acl_operation::alter_configs,
};
return std::any_of(ops.begin(), ops.end(), check_op);
}
default:
return check_op(operation);
}
}
acl_store _store;

// The list of superusers is stored twice: once as a vector in the
Expand Down