-
Notifications
You must be signed in to change notification settings - Fork 132
add authz and ipsec E2E test #1489
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @xiaojiangao123, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces new end-to-end (E2E) test cases to validate the authorization functionality within Kmesh. Specifically, these tests cover policies based on IP addresses, network ports, and namespaces. The primary impact is to enhance the robustness and reliability of Kmesh's security authorization features, particularly when these policies are offloaded to XDP (eXpress Data Path) eBPF programs.
Highlights
- New E2E Tests: Added comprehensive end-to-end (E2E) tests for Kmesh authorization policies.
- IP Authorization Tests: Introduced
TestIPAuthorizationto verify authorization based on source IP addresses, covering both allow and deny scenarios. - Port Authorization Tests: Implemented
TestPortAuthorizationto validate authorization rules applied to specific destination ports, including allow and deny cases. - Namespace Authorization Tests: Developed
TestNamespaceAuthorizationto ensure correct authorization behavior based on the source namespace. - XDP Program Verification: Included a utility function
waitForXDPOnDstWorkloadsto confirm the loading of XDP eBPF programs on destination workloads, emphasizing the XDP offload aspect of these tests.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Code flows, rules apply,Security holds fast.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds end-to-end tests for L4 authorization features (IP, port, and namespace). The tests are well-structured and cover both ALLOW and DENY policies. I've found one high-severity issue related to a resource leak in the waitForXDPOnDstWorkloads helper function, which I've detailed in a specific comment with a suggested fix. Otherwise, the test logic looks correct.
| func waitForXDPOnDstWorkloads(t framework.TestContext, dst echo.Instances) { | ||
| count := 0 | ||
| workloads := dst.WorkloadsOrFail(t) | ||
| namespace := apps.Namespace.Name() | ||
| for _, client := range workloads { | ||
| if count == len(workloads) { | ||
| break | ||
| } | ||
| podName := client.PodName() | ||
| timeout := time.After(5 * time.Second) | ||
| ticker := time.NewTicker(500 * time.Millisecond) | ||
| defer ticker.Stop() | ||
| InnerLoop: | ||
| for { | ||
| select { | ||
| case <-timeout: | ||
| t.Fatalf("Timeout: XDP eBPF program not found on pod %s", podName) | ||
| case <-ticker.C: | ||
| cmd := exec.Command("kubectl", "exec", "-n", namespace, podName, "--", "sh", "-c", "ip a | grep xdp") | ||
| output, err := cmd.CombinedOutput() | ||
| if err == nil && len(output) > 0 { | ||
| t.Logf("XDP program is loaded on pod %s", podName) | ||
| count++ | ||
| break InnerLoop | ||
| } | ||
| t.Logf("Waiting for XDP program to load on pod %s: %v", podName, err) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation of waitForXDPOnDstWorkloads has a resource leak. The defer ticker.Stop() is inside a for loop, which means that tickers are not stopped until the function returns. For a destination with N workloads, N-1 tickers will be leaked until the function exits. This can lead to resource exhaustion.
Additionally, the count variable and the check if count == len(workloads) are redundant and can be removed to simplify the code.
I suggest refactoring the function to explicitly stop the ticker in each iteration and remove the unnecessary code.
func waitForXDPOnDstWorkloads(t framework.TestContext, dst echo.Instances) {
workloads := dst.WorkloadsOrFail(t)
namespace := apps.Namespace.Name()
for _, client := range workloads {
podName := client.PodName()
ticker := time.NewTicker(500 * time.Millisecond)
timeout := time.After(5 * time.Second)
waitLoop:
for {
select {
case <-timeout:
ticker.Stop()
t.Fatalf("Timeout: XDP eBPF program not found on pod %s", podName)
case <-ticker.C:
cmd := exec.Command("kubectl", "exec", "-n", namespace, podName, "--", "sh", "-c", "ip a | grep xdp")
output, err := cmd.CombinedOutput()
if err == nil && len(output) > 0 {
t.Logf("XDP program is loaded on pod %s", podName)
ticker.Stop()
break waitLoop
}
t.Logf("Waiting for XDP program to load on pod %s: %v", podName, err)
}
}
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds comprehensive E2E test cases for authorization features in Kmesh, specifically testing IP-based, port-based, and namespace-based authorization policies. The tests validate that XDP eBPF programs correctly enforce authorization rules at the network layer.
- Adds IP authorization tests that verify ALLOW/DENY policies based on source IP addresses
- Implements port authorization tests to validate policy enforcement on specific service/pod ports
- Introduces namespace authorization tests to ensure proper isolation between namespaces
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| for _, client := range workloads { | ||
| if count == len(workloads) { | ||
| break | ||
| } |
Copilot
AI
Aug 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition count == len(workloads) will never be true at this point since count starts at 0 and is only incremented inside the inner loop after this check. This break statement is unreachable and the loop will always process all workloads unnecessarily.
| } |
| // Enable authorization offload to xdp. | ||
|
|
||
| if len(apps.ServiceWithWaypointAtServiceGranularity) == 0 { | ||
| t.Fatal(fmt.Errorf("need at least 1 instance of apps.ServiceWithWaypointAtServiceGranularity")) |
Copilot
AI
Aug 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using t.Fatal(fmt.Errorf(...)) is redundant. Use t.Fatalf(\"need at least 1 instance of apps.ServiceWithWaypointAtServiceGranularity\") instead for cleaner error reporting.
| t.Fatal(fmt.Errorf("need at least 1 instance of apps.ServiceWithWaypointAtServiceGranularity")) | |
| t.Fatalf("need at least 1 instance of apps.ServiceWithWaypointAtServiceGranularity") |
|
|
||
| addresses := clients.Addresses() | ||
| if len(addresses) < 2 { | ||
| t.Fatal(fmt.Errorf("need at least 2 clients")) |
Copilot
AI
Aug 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using t.Fatal(fmt.Errorf(...)) is redundant. Use t.Fatalf(\"need at least 2 clients\") instead for cleaner error reporting.
| t.Fatal(fmt.Errorf("need at least 2 clients")) | |
| t.Fatalf("need at least 2 clients") |
| // Enable authorization offload to XDP. | ||
|
|
||
| if len(apps.ServiceWithWaypointAtServiceGranularity) == 0 { | ||
| t.Fatal(fmt.Errorf("need at least 1 instance of apps.ServiceWithWaypointAtServiceGranularity")) |
Copilot
AI
Aug 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using t.Fatal(fmt.Errorf(...)) is redundant. Use t.Fatalf(\"need at least 1 instance of apps.ServiceWithWaypointAtServiceGranularity\") instead for cleaner error reporting.
| t.Fatal(fmt.Errorf("need at least 1 instance of apps.ServiceWithWaypointAtServiceGranularity")) | |
| t.Fatalf("need at least 1 instance of apps.ServiceWithWaypointAtServiceGranularity") |
| // Enable authorization offload to XDP. | ||
|
|
||
| if len(apps.ServiceWithWaypointAtServiceGranularity) == 0 { | ||
| t.Fatal(fmt.Errorf("need at least 1 instance of apps.ServiceWithWaypointAtServiceGranularity")) |
Copilot
AI
Aug 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using t.Fatal(fmt.Errorf(...)) is redundant. Use t.Fatalf(\"need at least 1 instance of apps.ServiceWithWaypointAtServiceGranularity\") instead for cleaner error reporting.
| t.Fatal(fmt.Errorf("need at least 1 instance of apps.ServiceWithWaypointAtServiceGranularity")) | |
| t.Fatalf("need at least 1 instance of apps.ServiceWithWaypointAtServiceGranularity") |
| t.Fatal("invalid action") | ||
| } | ||
|
|
||
| return check.OK() |
Copilot
AI
Aug 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return check.OK() statement after the switch block is unreachable because all switch cases either return a value or call t.Fatal() which terminates execution.
| return check.OK() |
| t.Fatal("invalid action") | ||
| } | ||
|
|
||
| return check.OK() |
Copilot
AI
Aug 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return check.OK() statement after the switch block is unreachable because all switch cases either return a value or call t.Fatal() which terminates execution.
| return check.OK() |
| t.Fatal("invalid action") | ||
| } | ||
|
|
||
| return check.OK() |
Copilot
AI
Aug 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return check.OK() statement after the switch block is unreachable because all switch cases either return a value or call t.Fatal() which terminates execution.
| return check.OK() |
Codecov Report✅ All modified and coverable lines are covered by tests. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
@xiaojiangao123 please rebase this PR and make all the CI passed first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
test/e2e/ipsec_test.go
Outdated
| time.Sleep(3 * time.Second) | ||
| continue | ||
| } | ||
| // fields[2] 是 pod 状态 |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment is in Chinese. Should be translated to English for consistency: '// fields[2] is the pod status'
| // fields[2] 是 pod 状态 | |
| // fields[2] is the pod status |
| Addresses is used to store the internal ip address informatioon on the | ||
| host. The IP address information is used to generate the IPsec state | ||
| informatioon. IPsec uses this information to determine which network |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in 'informatioon' - should be 'information'
| Addresses is used to store the internal ip address informatioon on the | |
| host. The IP address information is used to generate the IPsec state | |
| informatioon. IPsec uses this information to determine which network | |
| Addresses is used to store the internal ip address information on the | |
| host. The IP address information is used to generate the IPsec state | |
| information. IPsec uses this information to determine which network |
| Addresses is used to store the internal ip address informatioon on the | ||
| host. The IP address information is used to generate the IPsec state | ||
| informatioon. IPsec uses this information to determine which network |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in 'informatioon' - should be 'information'
| Addresses is used to store the internal ip address informatioon on the | |
| host. The IP address information is used to generate the IPsec state | |
| informatioon. IPsec uses this information to determine which network | |
| Addresses is used to store the internal ip address information on the | |
| host. The IP address information is used to generate the IPsec state | |
| information. IPsec uses this information to determine which network |
9afc8c9 to
ffee924
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
test/e2e/ipsec_test.go:1
- Typo in comment: 'informatioon' should be 'information'.
//go:build integ
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| func checkIPSecRules(t framework.TestContext, containerName string) error { | ||
| stateCmd := exec.Command("docker", "exec", containerName, "bash", "-c", "ip xfrm state") | ||
| stateOut, err := stateCmd.CombinedOutput() | ||
| if err != nil { | ||
| t.Fatalf("failed to run ip xfrm state: %v\n%s", err, string(stateOut)) | ||
| return err | ||
| } |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unreachable code: the return statement after t.Fatalf() will never execute since t.Fatalf() terminates the test. Either remove the return or replace t.Fatalf() with t.Errorf() if you want to continue execution.
| policyCmd := exec.Command("docker", "exec", containerName, "bash", "-c", "ip xfrm policy") | ||
| policyOut, err := policyCmd.CombinedOutput() | ||
| if err != nil { | ||
| t.Fatalf("failed to run ip xfrm policy: %v\n%s", err, string(policyOut)) | ||
| return err | ||
| } |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unreachable code: the return statement after t.Fatalf() will never execute since t.Fatalf() terminates the test. Either remove the return or replace t.Fatalf() with t.Errorf() if you want to continue execution.
| if !strings.Contains(stateStr, "proto esp spi") || !strings.Contains(stateStr, "mode tunnel") { | ||
| t.Fatalf("ip xfrm state output does not contain expected ESP tunnel rules") | ||
| return fmt.Errorf("ip xfrm state output does not contain expected ESP tunnel rules") | ||
| } |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unreachable code: the return statement after t.Fatalf() will never execute since t.Fatalf() terminates the test. Either remove the return or replace t.Fatalf() with t.Errorf() if you want to continue execution.
| if !strings.Contains(stateStr, "aead rfc4106(gcm(aes))") { | ||
| t.Fatalf("ip xfrm state output does not contain expected AEAD algorithm") | ||
| return fmt.Errorf("ip xfrm state output does not contain expected AEAD algorithm") | ||
| } |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unreachable code: the return statement after t.Fatalf() will never execute since t.Fatalf() terminates the test. Either remove the return or replace t.Fatalf() with t.Errorf() if you want to continue execution.
| if !strings.Contains(policyStr, "tmpl src") || !strings.Contains(policyStr, "proto esp") || !strings.Contains(policyStr, "mode tunnel") { | ||
| t.Fatalf("ip xfrm policy output does not contain expected tunnel template") | ||
| return fmt.Errorf("ip xfrm policy output does not contain expected tunnel template") | ||
| } |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unreachable code: the return statement after t.Fatalf() will never execute since t.Fatalf() terminates the test. Either remove the return or replace t.Fatalf() with t.Errorf() if you want to continue execution.
| if !strings.Contains(policyStr, "dir out") || !strings.Contains(policyStr, "dir in") || !strings.Contains(policyStr, "dir fwd") { | ||
| t.Fatalf("ip xfrm policy output does not contain expected directions (out/in/fwd)") | ||
| return fmt.Errorf("ip xfrm policy output does not contain expected directions (out/in/fwd)") | ||
| } |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unreachable code: the return statement after t.Fatalf() will never execute since t.Fatalf() terminates the test. Either remove the return or replace t.Fatalf() with t.Errorf() if you want to continue execution.
| if !strings.Contains(policyStr, "mark") { | ||
| t.Fatalf("ip xfrm policy output does not contain expected mark field") | ||
| return fmt.Errorf("ip xfrm policy output does not contain expected mark field") | ||
| } |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unreachable code: the return statement after t.Fatalf() will never execute since t.Fatalf() terminates the test. Either remove the return or replace t.Fatalf() with t.Errorf() if you want to continue execution.
9457306 to
650db84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| func genRandomKeyHex() string { | ||
| b := make([]byte, 36) | ||
| _, err := rand.Read(b) | ||
| if err != nil { | ||
| panic("failed to generate random key") | ||
| } | ||
| return hex.EncodeToString(b) | ||
| } |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The magic number 36 should be defined as a named constant to improve code readability and maintainability.
| selectedPodPort := 19090 | ||
| selectedServicePort := 9090 | ||
| notSelectedPodPort := 16060 | ||
| notSelectedServicePort := 9091 | ||
|
|
||
| // Echo Pod healthy port | ||
| readyPort := 8080 | ||
| livenessPort := 3333 |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These port numbers appear to be magic numbers. Consider defining them as constants at the package level or documenting why these specific values are chosen.
| go func() { | ||
| out, _ := runTcpdumpESP(t, "kmesh-testing-control-plane", 10) | ||
| tcpdumpCh <- out | ||
| }() |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error from runTcpdumpESP is discarded with _. This could mask important failures. The error should be handled or at least logged.
| t.Logf("Starting tcpdump in kmesh-testing-control-plane container after key rotation...") | ||
| tcpdumpCh = make(chan string) | ||
| go func() { | ||
| out, _ := runTcpdumpESP(t, "kmesh-testing-control-plane", 10) |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error from runTcpdumpESP is discarded with _. This could mask important failures. The error should be handled or at least logged.
| out, _ := runTcpdumpESP(t, "kmesh-testing-control-plane", 10) | |
| out, err := runTcpdumpESP(t, "kmesh-testing-control-plane", 10) | |
| if err != nil { | |
| t.Logf("runTcpdumpESP error: %v", err) | |
| } |
|
|
||
| For the Offload Authorization feature, E2E test cases are designed for IP, Port, Header, namespace, and hosts authorization. | ||
|
|
||
| The test environment requires at least a 2-node Kubernetes cluster using httpbin and sleep or fortio as test applications. |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The word 'informatioon' on line 43 contains a spelling error - it should be 'information'.
Signed-off-by: gaoxiaojian123 <[email protected]>
Signed-off-by: gaoxiaojian123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
Signed-off-by: xiaojiangao123 <[email protected]>
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: YaoZengzeng 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 |
What type of PR is this?
/kind security
What this PR does / why we need it:
add E2E test cases for ip, port, namespace authz and ipsec feature
What this PR fix:
Fixes #1408