Conversation
2. fix mac conflict case Signed-off-by: clyi <clyi@alauda.io>
dosubot
bot
added
the
size:L
Summary of ChangesHello @changluyi, 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 expands the testing suite for AdminNetworkPolicy (ANP) by adding comprehensive end-to-end tests for the Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces two new e2e tests for AdminNetworkPolicy (ANP) domain name rules and fixes a bug in MAC address parsing in a subnet test. The new ANP tests are well-structured and cover important scenarios, including the interaction of domain name and CIDR rules, as well as wildcard domain matching. I've provided a suggestion to reorder some checks in one of the tests to prevent potential flakiness due to DNS resolution behavior. The fix for MAC address parsing is a good improvement, making it more robust. I've also added a minor suggestion for a performance and maintainability improvement there. Overall, this is a valuable contribution that enhances test coverage and reliability.
test/e2e/anp-domain/e2e_test.go
Outdated
| // Test connectivity after applying ANP with wildcard rules | ||
| // All baidu.com subdomains should be blocked (wildcard deny rule) | ||
| testNetworkConnectivity("https://www.baidu.com", false, "Testing connectivity to www.baidu.com after applying ANP (should be blocked by wildcard)") | ||
| testNetworkConnectivity("https://api.baidu.com", false, "Testing connectivity to api.baidu.com after applying ANP (should be blocked by wildcard)") | ||
| testNetworkConnectivity("https://blog.baidu.com", false, "Testing connectivity to blog.baidu.com after applying ANP (should be blocked by wildcard)") | ||
|
|
||
| // www.google.com should be allowed (specific allow rule overrides wildcard deny) | ||
| testNetworkConnectivity("https://www.google.com", true, "Testing connectivity to www.google.com after applying ANP (should be allowed by specific rule)") | ||
|
|
||
| // Other google.com subdomains should be blocked (wildcard deny rule) | ||
| testNetworkConnectivity("https://mail.google.com", false, "Testing connectivity to mail.google.com after applying ANP (should be blocked by wildcard)") | ||
|
|
||
| ginkgo.By("Testing edge cases with wildcard rules") | ||
| // Test that exact domain matches work | ||
| testNetworkConnectivity("https://baidu.com", true, "Testing connectivity to baidu.com (exact domain, should be allowed)") | ||
|
|
||
| // Test that subdomain matching works correctly | ||
| testNetworkConnectivity("https://subdomain.baidu.com", false, "Testing connectivity to subdomain.baidu.com (should be blocked by wildcard)") |
There was a problem hiding this comment.
The order of these connectivity tests could lead to flaky results. The test at line 496, testNetworkConnectivity("https://baidu.com", true, ...) correctly expects traffic to baidu.com to be allowed, as the deny rule *.baidu.com. does not match the apex domain.
However, previous tests (lines 484-486) attempt to connect to subdomains like www.baidu.com, which are denied. This causes their resolved IP addresses to be associated with a 'deny' action by OVN. If baidu.com later resolves to one of these same "tainted" IPs, traffic to it will also be blocked, causing this test to fail intermittently.
To make the test more robust and avoid this potential DNS-related flakiness, I suggest reordering the tests. The check for the apex domain (baidu.com) should be performed before checks for subdomains (www.baidu.com, etc.).
| // Test connectivity after applying ANP with wildcard rules | |
| // All baidu.com subdomains should be blocked (wildcard deny rule) | |
| testNetworkConnectivity("https://www.baidu.com", false, "Testing connectivity to www.baidu.com after applying ANP (should be blocked by wildcard)") | |
| testNetworkConnectivity("https://api.baidu.com", false, "Testing connectivity to api.baidu.com after applying ANP (should be blocked by wildcard)") | |
| testNetworkConnectivity("https://blog.baidu.com", false, "Testing connectivity to blog.baidu.com after applying ANP (should be blocked by wildcard)") | |
| // www.google.com should be allowed (specific allow rule overrides wildcard deny) | |
| testNetworkConnectivity("https://www.google.com", true, "Testing connectivity to www.google.com after applying ANP (should be allowed by specific rule)") | |
| // Other google.com subdomains should be blocked (wildcard deny rule) | |
| testNetworkConnectivity("https://mail.google.com", false, "Testing connectivity to mail.google.com after applying ANP (should be blocked by wildcard)") | |
| ginkgo.By("Testing edge cases with wildcard rules") | |
| // Test that exact domain matches work | |
| testNetworkConnectivity("https://baidu.com", true, "Testing connectivity to baidu.com (exact domain, should be allowed)") | |
| // Test that subdomain matching works correctly | |
| testNetworkConnectivity("https://subdomain.baidu.com", false, "Testing connectivity to subdomain.baidu.com (should be blocked by wildcard)") | |
| // Test connectivity after applying ANP with wildcard rules | |
| ginkgo.By("Testing edge cases with wildcard rules") | |
| // Test that exact domain matches work | |
| testNetworkConnectivity("https://baidu.com", true, "Testing connectivity to baidu.com (exact domain, should be allowed)") | |
| // All baidu.com subdomains should be blocked (wildcard deny rule) | |
| testNetworkConnectivity("https://www.baidu.com", false, "Testing connectivity to www.baidu.com after applying ANP (should be blocked by wildcard)") | |
| testNetworkConnectivity("https://api.baidu.com", false, "Testing connectivity to api.baidu.com after applying ANP (should be blocked by wildcard)") | |
| testNetworkConnectivity("https://blog.baidu.com", false, "Testing connectivity to blog.baidu.com after applying ANP (should be blocked by wildcard)") | |
| // www.google.com should be allowed (specific allow rule overrides wildcard deny) | |
| testNetworkConnectivity("https://www.google.com", true, "Testing connectivity to www.google.com after applying ANP (should be allowed by specific rule)") | |
| // Other google.com subdomains should be blocked (wildcard deny rule) | |
| testNetworkConnectivity("https://mail.google.com", false, "Testing connectivity to mail.google.com after applying ANP (should be blocked by wildcard)") | |
| // Test that subdomain matching works correctly | |
| testNetworkConnectivity("https://subdomain.baidu.com", false, "Testing connectivity to subdomain.baidu.com (should be blocked by wildcard)") |
|
|
||
| gatewayMAC := strings.TrimSpace(string(output)) | ||
| // Extract MAC address from the output string | ||
| macRegex := regexp.MustCompile(`([0-9A-Fa-f]{2}[:-]){5}[0-9A-Fa-f]{2}`) |
There was a problem hiding this comment.
For performance and maintainability, it's better to compile this regular expression once at the package level instead of inside the test function. This avoids recompiling the regex on every test execution. You can define macRegex as a package-level variable.
| ginkgo.By("Cleaning up test resources") | ||
| ctx := context.Background() | ||
|
|
||
| // Clean up ANP if it exists |
3 participants
Pull Request
What type of this PR
Examples of user facing changes:
Which issue(s) this PR fixes
Fixes #(issue-number)