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

Simplify byteorder transformation & remove magic number in Authz #407

Merged
merged 1 commit into from
Jun 22, 2024

Conversation

tacslon
Copy link
Contributor

@tacslon tacslon commented May 31, 2024

What type of PR is this?
/kind enhancement

What this PR does / why we need it:

  1. Clarify byteorder transformation in Authz
  2. Remove magic number in Authz
    Which issue(s) this PR fixes:
    Fixes Enhance authz #323

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@kmesh-bot kmesh-bot added the kind/enhancement New feature or request label May 31, 2024
@codecov-commenter
Copy link

codecov-commenter commented May 31, 2024

Codecov Report

Attention: Patch coverage is 0% with 44 lines in your changes missing coverage. Please review.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Flag Coverage Δ
unittests 37.16% <0.00%> (+5.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
pkg/auth/xdp_auth_handler.go 0.00% <0.00%> (ø)
pkg/auth/rbac.go 57.42% <0.00%> (+0.66%) ⬆️

... and 6 files with indirect coverage changes

@tacslon tacslon force-pushed the main branch 4 times, most recently from 069856b to 39a6dce Compare May 31, 2024 10:18
@kmesh-bot kmesh-bot added size/L and removed size/M labels May 31, 2024
pkg/auth/rbac.go Outdated Show resolved Hide resolved
pkg/auth/rbac.go Outdated Show resolved Hide resolved
@tacslon
Copy link
Contributor Author

tacslon commented Jun 3, 2024

/hold

@tacslon
Copy link
Contributor Author

tacslon commented Jun 3, 2024

/unhold

pkg/auth/rbac.go Outdated Show resolved Hide resolved
@tacslon tacslon force-pushed the main branch 3 times, most recently from 577fb0a to cee357d Compare June 4, 2024 11:40
Copy link
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. but should supplement test cases

@tacslon tacslon changed the title Clarify byteorder transformation & remove magic number in Authz Simplify byteorder transformation & remove magic number in Authz Jun 17, 2024
@hzxuzhonghu
Copy link
Member

Please add some unit tests

return conn, err
}
// srcIp and dstIp are big endian, and dstPort is little endian, which is consistent with authorization policy flushed to Kmesh
conn.srcIp = binary.BigEndian.AppendUint32(conn.srcIp, tupleV4.SrcAddr)
Copy link
Member

Choose a reason for hiding this comment

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

we can use netip.AddrFromSlice if we make srcIp as type netip.Addr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will be redesigned in the future if necessary

pkg/auth/xdp_auth_handler.go Outdated Show resolved Hide resolved
@tacslon tacslon force-pushed the main branch 13 times, most recently from f9f810c to fd1bd5e Compare June 20, 2024 12:41
Copy link
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.

And please add ipv6 case

IPV4_TUPLE_LENGTH = 12
IPV4_TUPLE_LENGTH = int(unsafe.Sizeof(bpfSockTupleV4{}))
// TUPLE_LEN is the fixed length of 4-tuple(source/dest IP/port) in a record from map of tuple
TUPLE_LEN = int(unsafe.Sizeof(bpfSockTupleV6{}))
Copy link
Member

Choose a reason for hiding this comment

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

:+1 For making it meaningful

pkg/auth/rbac_test.go Show resolved Hide resolved
Copy link
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

Some nits can be fixed in a following pr

// Judge results
found := val == 1
if found != tt.wantFound {
t.Errorf("want %v, but got %v", tt.wantFound, found)
Copy link
Member

Choose a reason for hiding this comment

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

nit: we can use assert.Equal()


// Close maps
mapOfTuple.Close()
mapOfAuth.Close()
Copy link
Member

Choose a reason for hiding this comment

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

nit: move just after map initiation, and use defer

@kmesh-bot
Copy link
Collaborator

[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

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 13fbc98 into kmesh-net:main Jun 22, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance authz
4 participants