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

WIP: E2E test case for Kmesh L4 authorization #641

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

WIP: E2E test case for Kmesh L4 authorization #641

wants to merge 12 commits into from
+212 −7

Conversation

YaoZengzeng
Copy link
Member

What type of PR is this?

/kind enhancement

What this PR does / why we need it:

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

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

@kmesh-bot kmesh-bot added size/L and removed size/M labels Jul 27, 2024
@codecov Codecov
Copy link

codecov bot commented Jul 27, 2024 •
edited
Loading

Codecov Report

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

Project coverage is 45.23%. Comparing base (2eb3f92) to head (335cb2a).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
pkg/status/status_server.go 0.00% 17 Missing ⚠️

❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Files with missing lines Coverage Δ
pkg/status/status_server.go 34.93% <0.00%> (-0.96%) ⬇️

... and 2 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 724789d...335cb2a. Read the comment docs.

@YaoZengzeng
Copy link
Member Author

/test

@kmesh-bot
Copy link
Collaborator

@YaoZengzeng: No presubmit jobs available for kmesh-net/kmesh@main

In response to this:

/test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@YaoZengzeng
Copy link
Member Author

/retest

hzxuzhonghu
hzxuzhonghu reviewed Jul 30, 2024
View reviewed changes
test/e2e/baseline_test.go
if len(apps.ServiceWithWaypointAtServiceGranularity) == 0 {
t.Fatal(fmt.Errorf("need at least 1 instance of apps.ServiceWithWaypointAtServiceGranularity"))
}
src := apps.ServiceWithWaypointAtServiceGranularity[0]
Copy link
Member

Choose a reason for hiding this comment

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

why do you choose this special workload as a src client

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no specific choice, any workload managed by Kmesh is OK.

hzxuzhonghu
hzxuzhonghu reviewed Jul 30, 2024
View reviewed changes
test/e2e/baseline_test.go

addresses := clients.Addresses()
if len(addresses) < 2 {
t.Fatal(fmt.Errorf("need at least 2 clients"))
Copy link
Member

Choose a reason for hiding this comment

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

? why require at least 2 addresses

Copy link
Member Author

Choose a reason for hiding this comment

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

One for allow and one for deny.

hzxuzhonghu
hzxuzhonghu reviewed Jul 30, 2024
View reviewed changes
test/e2e/baseline_test.go Outdated
check.ErrorContains("EOF"), // TCP envoy
check.ErrorContains("read: connection reset by peer"), // TCP Kmesh
check.NoErrorAndStatus(http.StatusForbidden), // HTTP
check.NoErrorAndStatus(http.StatusServiceUnavailable), // HTTP client, TCP server
Copy link
Member

Choose a reason for hiding this comment

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

This is too wide, for tcp authz, can we only check reset?

hzxuzhonghu
hzxuzhonghu reviewed Jul 30, 2024
View reviewed changes
test/e2e/baseline_test.go
selector:
matchLabels:
app: "{{.Destination}}"
`+tc.spec+`
Copy link
Member

Choose a reason for hiding this comment

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

A hacky way, would prefer using fmt.Sprintf or golang template

@hzxuzhonghu
Copy link
Member

authz offloading is disabled by default, the only way to enable it is kmeshctl authx enable

@YaoZengzeng YaoZengzeng mentioned this pull request Jan 21, 2025
Open
@kmesh-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign bitcoffeeiux for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 added size/XXL and removed size/L labels Jan 26, 2025
@weli-l
Copy link
Contributor

weli-l commented Feb 17, 2025

/retest

@hzxuzhonghu
Copy link
Member

/test all

@kmesh-bot
Copy link
Collaborator

@hzxuzhonghu: No presubmit jobs available for kmesh-net/kmesh@main

In response to this:

/test all

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@YaoZengzeng
add deny scenario
43a7fde 
Signed-off-by: YaoZengzeng <[email protected]>
@YaoZengzeng
fix comments
d927b90 
Signed-off-by: YaoZengzeng <[email protected]>
@YaoZengzeng
adjust call timeout
f45941c 
Signed-off-by: YaoZengzeng <[email protected]>
@YaoZengzeng
kmeshctl control authz
820b570 
Signed-off-by: YaoZengzeng <[email protected]>
@YaoZengzeng
fix codespell
fc95d15 
Signed-off-by: YaoZengzeng <[email protected]>
@YaoZengzeng
more fix
2cab055 
Signed-off-by: YaoZengzeng <[email protected]>
@YaoZengzeng
optimize kmeshctl authz
c10f1f2 
Signed-off-by: YaoZengzeng <[email protected]>
@YaoZengzeng
fix flaky
a2e5ba7 
Signed-off-by: YaoZengzeng <[email protected]>
@YaoZengzeng
fix authz
b4775df 
Signed-off-by: YaoZengzeng <[email protected]>
@kmesh-bot kmesh-bot added size/L and removed size/XXL labels Feb 17, 2025
@YaoZengzeng
Copy link
Member Author

/test all

@kmesh-bot
Copy link
Collaborator

@YaoZengzeng: No presubmit jobs available for kmesh-net/kmesh@main

In response to this:

/test all

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@YaoZengzeng
delte go backup file
335cb2a 
Signed-off-by: YaoZengzeng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hzxuzhonghu hzxuzhonghu hzxuzhonghu left review comments

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

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
do-not-merge/work-in-progress kind/enhancement New feature or request size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add authz e2e test coverage
4 participants
@YaoZengzeng @kmesh-bot @hzxuzhonghu @weli-l