system-tests: update egress-service.go to support IPv6 single stack#1342
system-tests: update egress-service.go to support IPv6 single stack#1342laurayang842 wants to merge 4 commits intorh-ecosystem-edge:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughApply dynamic Service IP family policy based on configured remote IPv4/IPv6 targets and refactor egress-service connectivity checks into a shared helper that conditionally performs IPv4 and/or IPv6 curl+source-IP verifications, with assertions that at least one remote target IP is set. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/system-tests/rdscore/internal/rdscorecommon/egress-service.go (2)
385-413: IPFamilyPolicy selection logic is correct.The guard plus switch is sound: the
Expect(hasIPv4 || hasIPv6)precondition makes thedefaultbranch unambiguously the "IPv4-only" case, so the three branches fully and correctly cover the tri-state (v4-only / v6-only / dual). Combined withSingleStack, downstream loops at lines 516‑541 that still branch onmyIP.Is4()will only see Ingress entries of the configured family, so the emptyremoteTargetIP/remoteTargetIPv6cannot accidentally leak into the curl command.Note: this same switch block is duplicated verbatim at lines 701‑729 in
VerifyEgressServiceWithLocalETPWrapper. Consider extracting a small helper, e.g.:♻️ Suggested helper to dedupe the ipFamilyPolicy setup (applies to both wrappers)
// applyIPFamilyPolicy configures svcBuilder's IP families based on which remote targets are set. // Callers must ensure at least one of v4 or v6 is non-empty. func applyIPFamilyPolicy(svcBuilder *service.Builder, remoteTargetIP, remoteTargetIPv6 string) *service.Builder { hasIPv4 := remoteTargetIP != "" hasIPv6 := remoteTargetIPv6 != "" switch { case hasIPv4 && hasIPv6: By("Setting ipFamilyPolicy to 'RequireDualStack'") return svcBuilder.WithIPFamily( []corev1.IPFamily{corev1.IPv4Protocol, corev1.IPv6Protocol}, corev1.IPFamilyPolicyRequireDualStack) case hasIPv6: By("Setting ipFamilyPolicy to 'SingleStack' (IPv6)") return svcBuilder.WithIPFamily( []corev1.IPFamily{corev1.IPv6Protocol}, corev1.IPFamilyPolicySingleStack) default: By("Setting ipFamilyPolicy to 'SingleStack' (IPv4)") return svcBuilder.WithIPFamily( []corev1.IPFamily{corev1.IPv4Protocol}, corev1.IPFamilyPolicySingleStack) } }Then at both call sites (lines 385‑413 and 701‑729):
- hasIPv4 := remoteTargetIP != "" - hasIPv6 := remoteTargetIPv6 != "" - - Expect(hasIPv4 || hasIPv6).To(BeTrue(), + Expect(remoteTargetIP != "" || remoteTargetIPv6 != "").To(BeTrue(), "At least one remote target IP (IPv4 or IPv6) must be configured") - - switch { - case hasIPv4 && hasIPv6: - ... - } + svcBuilder = applyIPFamilyPolicy(svcBuilder, remoteTargetIP, remoteTargetIPv6)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/system-tests/rdscore/internal/rdscorecommon/egress-service.go` around lines 385 - 413, The ipFamilyPolicy switch using hasIPv4/hasIPv6 is correct but duplicated; extract the logic into a helper (e.g., applyIPFamilyPolicy) that takes svcBuilder, remoteTargetIP and remoteTargetIPv6 and returns the updated svcBuilder, reusing the same branches (dual → WithIPFamily([...IPv4, IPv6...], IPFamilyPolicyRequireDualStack; IPv6-only → WithIPFamily([IPv6], IPFamilyPolicySingleStack); default/IPv4-only → WithIPFamily([IPv4], IPFamilyPolicySingleStack)). Replace the two verbatim blocks (the current block using svcBuilder.WithIPFamily at lines around the first occurrence and the duplicate in VerifyEgressServiceWithLocalETPWrapper) with calls to this helper to remove duplication and keep behavior identical.
1001-1115: Deduplicate the fourVerifyEgressServiceConnectivity*functions.All four functions (
VerifyEgressServiceConnectivityETPCluster,…ETPClusterSourceIPByNetwork,…ETPLocal,…ETPLocalSourceIPByNetwork) now share an identical shape: precondition assertion → conditional IPv4 curl +verifySourceIP(..., false, …)→ conditional IPv6 curl +verifySourceIP(..., true, …). The only variation is the service name, labels, and theBy()message prefix. This is a good opportunity to collapse the boilerplate into a single helper to reduce future drift (the IPv6-only support was added in four places and any future tweak — e.g., timeout, URL shape, additional assertion — will need to be mirrored four times).♻️ Suggested helper to remove the 4-way duplication
// verifyEgressServiceConnectivity runs source-IP verification for the IPv4 and/or IPv6 remote // targets configured in RDSCoreConfig, using the provided service identifiers and a By() label prefix. func verifyEgressServiceConnectivity(svcName, svcNS, svcLabels, byPrefix string) { Expect(RDSCoreConfig.EgressServiceRemoteIP != "" || RDSCoreConfig.EgressServiceRemoteIPv6 != ""). To(BeTrue(), "Neither EgressServiceRemoteIP nor EgressServiceRemoteIPv6 is configured") if RDSCoreConfig.EgressServiceRemoteIP != "" { By(fmt.Sprintf("Verifying %s connectivity (IPv4)", byPrefix)) cmdToRun := []string{"/bin/bash", "-c", fmt.Sprintf("curl --connect-timeout 3 -Ls http://%s:%s/clientip", RDSCoreConfig.EgressServiceRemoteIP, RDSCoreConfig.EgressServiceRemotePort)} verifySourceIP(svcName, svcNS, svcLabels, cmdToRun, false, RDSCoreConfig.EgressServiceNetworkExpectedIPs) } if RDSCoreConfig.EgressServiceRemoteIPv6 != "" { By(fmt.Sprintf("Verifying %s connectivity (IPv6)", byPrefix)) cmdToRun := []string{"/bin/bash", "-c", fmt.Sprintf("curl --connect-timeout 3 -Ls http://[%s]:%s/clientip", RDSCoreConfig.EgressServiceRemoteIPv6, RDSCoreConfig.EgressServiceRemotePort)} verifySourceIP(svcName, svcNS, svcLabels, cmdToRun, true, RDSCoreConfig.EgressServiceNetworkExpectedIPs) } }Call sites collapse to a single line each:
func VerifyEgressServiceConnectivityETPCluster() { - Expect(...).To(BeTrue(), "...") - if RDSCoreConfig.EgressServiceRemoteIP != "" { ... } - if RDSCoreConfig.EgressServiceRemoteIPv6 != "" { ... } + verifyEgressServiceConnectivity(egressSVC1Name, RDSCoreConfig.EgressServiceNS, egressSVC1Labels, + "EgressService ETP=Cluster") } // ... similar for the other three, switching service name/labels and prefix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/system-tests/rdscore/internal/rdscorecommon/egress-service.go` around lines 1001 - 1115, The four functions VerifyEgressServiceConnectivityETPCluster, VerifyEgressServiceConnectivityETPClusterSourceIPByNetwork, VerifyEgressServiceConnectivityETPLocal, and VerifyEgressServiceConnectivityETPLocalSourceIPByNetwork are duplicated; extract their common logic into a single helper (e.g., verifyEgressServiceConnectivity) that accepts svcName, svcNS, svcLabels and a byPrefix string, moves the shared Expect(...) check and the IPv4/IPv6 curl + verifySourceIP(...) blocks into it, and update each original function to call that helper with the appropriate egressSVC*Name, RDSCoreConfig.EgressServiceNS, egressSVC*Labels and message prefix (e.g., "EgressService ETP=Cluster") to remove boilerplate and keep behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/system-tests/rdscore/internal/rdscorecommon/egress-service.go`:
- Around line 385-413: The ipFamilyPolicy switch using hasIPv4/hasIPv6 is
correct but duplicated; extract the logic into a helper (e.g.,
applyIPFamilyPolicy) that takes svcBuilder, remoteTargetIP and remoteTargetIPv6
and returns the updated svcBuilder, reusing the same branches (dual →
WithIPFamily([...IPv4, IPv6...], IPFamilyPolicyRequireDualStack; IPv6-only →
WithIPFamily([IPv6], IPFamilyPolicySingleStack); default/IPv4-only →
WithIPFamily([IPv4], IPFamilyPolicySingleStack)). Replace the two verbatim
blocks (the current block using svcBuilder.WithIPFamily at lines around the
first occurrence and the duplicate in VerifyEgressServiceWithLocalETPWrapper)
with calls to this helper to remove duplication and keep behavior identical.
- Around line 1001-1115: The four functions
VerifyEgressServiceConnectivityETPCluster,
VerifyEgressServiceConnectivityETPClusterSourceIPByNetwork,
VerifyEgressServiceConnectivityETPLocal, and
VerifyEgressServiceConnectivityETPLocalSourceIPByNetwork are duplicated; extract
their common logic into a single helper (e.g., verifyEgressServiceConnectivity)
that accepts svcName, svcNS, svcLabels and a byPrefix string, moves the shared
Expect(...) check and the IPv4/IPv6 curl + verifySourceIP(...) blocks into it,
and update each original function to call that helper with the appropriate
egressSVC*Name, RDSCoreConfig.EgressServiceNS, egressSVC*Labels and message
prefix (e.g., "EgressService ETP=Cluster") to remove boilerplate and keep
behavior identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 469296a6-7fb7-407f-91d8-6d884c4043c2
📒 Files selected for processing (1)
tests/system-tests/rdscore/internal/rdscorecommon/egress-service.go
laurayang842
force-pushed
the
egresservice
branch
from
b0bbee7 to
1f6f645
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/system-tests/rdscore/internal/rdscorecommon/egress-service.go (1)
521-546: Optional: consider consolidating the inline IPv4/IPv6 curl branching with the same pattern used by the new helper.With single-stack IPv6 only,
remoteTargetIPwill be""and theIs4()branch is effectively unreachable (LB ingress will only return IPv6 VIPs). The current logic is safe, but the per-family curl construction here (and again at lines 820-836 inVerifyEgressServiceWithLocalETPWrapper) is near-duplicate of what you centralized inverifyEgressServiceConnectivity. Extracting a smallbuildClientIPCurl(family, ip, port)helper would keep the IPv6 bracket-quoting and URL format in one place.Not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/system-tests/rdscore/internal/rdscorecommon/egress-service.go` around lines 521 - 546, The two-place IPv4/IPv6 curl construction in the loop that handles svcBuilder.Object.Status.LoadBalancer.Ingress (and the similar block in VerifyEgressServiceWithLocalETPWrapper) duplicates logic centralized in verifyEgressServiceConnectivity; extract a small helper function buildClientIPCurl(family string, ip string, port string) that returns the properly bracketed URL or full curl command (handling IPv6 [] brackets and using remoteTargetIP vs remoteTargetIPv6), then replace the inline Is4() branches in the loop and in VerifyEgressServiceWithLocalETPWrapper to call buildClientIPCurl and use its result for cmdToRun so URL formatting is single-sourced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/system-tests/rdscore/internal/rdscorecommon/egress-service.go`:
- Around line 521-546: The two-place IPv4/IPv6 curl construction in the loop
that handles svcBuilder.Object.Status.LoadBalancer.Ingress (and the similar
block in VerifyEgressServiceWithLocalETPWrapper) duplicates logic centralized in
verifyEgressServiceConnectivity; extract a small helper function
buildClientIPCurl(family string, ip string, port string) that returns the
properly bracketed URL or full curl command (handling IPv6 [] brackets and using
remoteTargetIP vs remoteTargetIPv6), then replace the inline Is4() branches in
the loop and in VerifyEgressServiceWithLocalETPWrapper to call buildClientIPCurl
and use its result for cmdToRun so URL formatting is single-sourced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a16fffc1-30da-452e-8b3b-bb27f68a2e95
📒 Files selected for processing (1)
tests/system-tests/rdscore/internal/rdscorecommon/egress-service.go
2 participants
Summary by CodeRabbit