Skip to content

optimizie xdp auth#1256

Merged
kmesh-bot merged 2 commits intokmesh-net:mainfrom
weli-l:dev/auth_ip_optimize
Mar 6, 2025
Merged

optimizie xdp auth#1256
kmesh-bot merged 2 commits intokmesh-net:mainfrom
weli-l:dev/auth_ip_optimize

Conversation

@weli-l
Copy link
Copy Markdown
Contributor

@weli-l weli-l commented Feb 27, 2025 •
edited
Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #1207

Previously, since the xdp program only supports ip and port, policies configured with ns and principal types need to be
switched to user mode. However, the logic for determining whether to switch to user mode was wrong. The previous logic
was that when policy_index is greater than `MAX_MEMBER_NUM_PER_POLICY`, it will determine whether to switch to
user mode. However, `MAX_MEMBER_NUM_PER_POLICY` is a fixed value, and the number of deployed policies may be
less than this number.
The correct logic should be to traverse all policies in xdp. In each policy, if the policy does not match, the result of authz
will be stored in `match_ctx->auth_result`. If all policies are traversed and ns or principal rules are deployed, switch to
user mode. If it is not necessary to switch to user mode, return directly according to `match_ctx->auth_result`

###authz explanation and cases

B → C Traffic Authorization Policy Result

Case Policy Scope Policy Rules Result Logic Description
1 Single B-targeted allow B PASS Direct match
2 Single B-targeted deny B DROP Direct match
3 Single non-B target allow A DROP Non-target defaults to deny
4 Single non-B target deny A PASS Non-target defaults to allow
5 Mixed (contains B) allow B + deny A PASS B-target priority
6 Mixed (contains B) deny B + allow A DROP B-target priority
7 Multi-B (ordered) allow B → deny B PASS First-match execution
8 Multi-B (ordered) deny B → allow B DROP First-match execution

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

@weli-l
optimizie xdp auth
bfa4e00 
Signed-off-by: weli-l <1289113577@qq.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 27, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.69%. Comparing base (d950911) to head (eeeb399).
Report is 20 commits behind head on main.

see 6 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55a35b9...eeeb399. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hzxuzhonghu hzxuzhonghu requested a review from Copilot February 27, 2025 06:33
Copilot AI reviewed Feb 27, 2025
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

@supercharge-xsy
Copy link
Copy Markdown
Contributor

supercharge-xsy commented Feb 27, 2025

/lgtm
and we need to sort out the current authentication specifications, especially in the case of multiple policies.

@kmesh-bot kmesh-bot added the lgtm label Feb 27, 2025
@hzxuzhonghu
Copy link
Copy Markdown
Member

hzxuzhonghu commented Feb 27, 2025

can you add some description about what this pr is doning

hzxuzhonghu
hzxuzhonghu reviewed Feb 27, 2025
View reviewed changes
bpf/kmesh/workload/xdp.c
match_ctx.policies = policies;
match_ctx.need_tailcall_to_userspace = false;
match_ctx.policy_index = 0;
match_ctx.auth_result = XDP_PASS;
Copy link
Copy Markdown
Member

@hzxuzhonghu hzxuzhonghu Feb 27, 2025

Choose a reason for hiding this comment

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

Wy need pass this? I kind of feel this is the default value. Not needed to pass

hzxuzhonghu
hzxuzhonghu reviewed Feb 27, 2025
View reviewed changes
bpf/kmesh/workload/include/authz.h
if (!policy) {
return XDP_PASS;
if (match_ctx->need_tailcall_to_userspace) {
bpf_tail_call(ctx, &map_of_xdp_tailcall, TAIL_CALL_AUTH_IN_USER_SPACE);
Copy link
Copy Markdown
Member

@hzxuzhonghu hzxuzhonghu Feb 27, 2025

Choose a reason for hiding this comment

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

can you add a comment why tail call to user space if no policy found

hzxuzhonghu
hzxuzhonghu reviewed Feb 27, 2025
View reviewed changes
bpf/kmesh/workload/include/authz.h
return XDP_PASS;
}
return XDP_PASS;
if (match_ctx->auth_result == XDP_PASS) {
Copy link
Copy Markdown
Member

@hzxuzhonghu hzxuzhonghu Feb 27, 2025

Choose a reason for hiding this comment

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

Do you ever set it to XDP_DROP? Can you point it

Copy link
Copy Markdown
Contributor Author

@weli-l weli-l Mar 5, 2025

Choose a reason for hiding this comment

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

in Line 681, if unmatched, authz result is stored temporarily
match_ctx->auth_result = match_ctx->action == ISTIO__SECURITY__ACTION__DENY ? XDP_PASS : XDP_DROP;

@weli-l
add comment for tailcall to userspace
eeeb399 
Signed-off-by: weli-l <1289113577@qq.com>
@kmesh-bot kmesh-bot removed the lgtm label Mar 5, 2025
@weli-l
Copy link
Copy Markdown
Contributor Author

weli-l commented Mar 6, 2025

/retest

hzxuzhonghu
hzxuzhonghu approved these changes Mar 6, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

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

/lgtm

@kmesh-bot kmesh-bot added the lgtm label Mar 6, 2025
@kmesh-bot
Copy link
Copy Markdown
Collaborator

kmesh-bot commented Mar 6, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kmesh-bot kmesh-bot merged commit 5d61b19 into kmesh-net:main Mar 6, 2025
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@hzxuzhonghu hzxuzhonghu hzxuzhonghu approved these changes

@bitcoffeeiux bitcoffeeiux Awaiting requested review from bitcoffeeiux

@kevin-wangzefeng kevin-wangzefeng Awaiting requested review from kevin-wangzefeng

Assignees

@hzxuzhonghu hzxuzhonghu

@supercharge-xsy supercharge-xsy

Labels

approved lgtm size/S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

kmeshctl authz can't trigger offload authorization enable

5 participants

@weli-l @supercharge-xsy @hzxuzhonghu @kmesh-bot