USHIFT-6796: C2CC: DNS forwarding between clusters#6638
USHIFT-6796: C2CC: DNS forwarding between clusters#6638pmtk wants to merge 4 commits intoopenshift:mainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@pmtk: This pull request references USHIFT-6796 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this: 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pmtk The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughAdds cross-cluster DNS rendering: compute a DNS IP per resolved remote cluster when a domain is set, render CoreDNS server blocks, expose them to the DNS controller template, and update tests and Robot Framework suites to validate DNS blocks, resolution, and connectivity. ChangesCross-Cluster DNS Rendering
Sequence DiagramsequenceDiagram
participant Config as C2CC Config
participant Ctrl as DNS Controller
participant CM as CoreDNS ConfigMap
participant DNS as CoreDNS Server
participant Client as Test Pod (curl)
Config->>Config: validateRemoteCluster computes DNSIP from<br/>service network CIDR when Domain is set
Config->>Ctrl: ResolvedRemoteCluster with Domain & DNSIP
Ctrl->>Ctrl: RenderC2CCDNSBlocks(resolved)
Ctrl->>CM: Inject C2CCDNSBlocks into Corefile template
CM->>DNS: CoreDNS reloads updated Corefile with server blocks
Client->>DNS: getent hosts remote.<domain>
DNS->>Client: returns resolved IP
Client->>Client: curl http://<resolved-ip>:8080/cgi-bin/hello
Client->>Client: receives "Hello from" with source IP
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2." ... [truncated 31032 characters] ... elet: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/metrics: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/mount-utils: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/pod-security-admission: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-apiserver: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-cli-plugin: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-controller: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/suites/c2cc/dns.robot`:
- Around line 71-77: Make namespace creation idempotent: change the "Oc On
Cluster ${alias} oc create namespace ${NAMESPACE}" call in the "Deploy DNS
Test Workloads" block (and the similar calls at lines 88-93) so it doesn't fail
if the namespace already exists or is terminating — e.g., check for existence
before creating (using the same "Oc On Cluster" helper to run an "oc get
namespace ${NAMESPACE}" and only run create if absent) or replace create with an
idempotent operation; also ensure teardown ignores or handles delete errors
consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 273f8e99-0d08-4cb7-bb27-08129a56d98f
📒 Files selected for processing (10)
assets/components/openshift-dns/dns/configmap.yamlpkg/components/controllers.gopkg/config/c2cc.gopkg/config/c2cc_test.gopkg/controllers/c2cc/helpers_test.gotest/assets/c2cc/hello-microshift.yamltest/resources/c2cc.resourcetest/scenarios-bootc/el9/presubmits/el98-src@c2cc.shtest/suites/c2cc/connectivity.robottest/suites/c2cc/dns.robot
|
/test verify |
Avoid namespace collisions on reruns by generating a random namespace per cluster instead of using a hardcoded name. Also flatten nested validation logic in c2cc.go to satisfy the nestif linter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/config/c2cc.go (1)
360-367: ⚡ Quick winSkip domain blocks when
DNSIPis empty.If a
ResolvedRemoteClusterhasDomainbut an emptyDNSIP, the generatedforwarddirective is invalid and can break Corefile rendering. Add a defensive guard here.Suggested patch
func RenderC2CCDNSBlocks(resolved []ResolvedRemoteCluster) string { var blocks []string for _, rc := range resolved { - if rc.Domain == "" { + if rc.Domain == "" || rc.DNSIP == "" { continue } blocks = append(blocks, formatDNSBlock(rc.Domain, rc.DNSIP)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/c2cc.go` around lines 360 - 367, RenderC2CCDNSBlocks currently appends DNS blocks for every ResolvedRemoteCluster with a Domain, but if rc.DNSIP is empty the resulting forward directive is invalid; update RenderC2CCDNSBlocks to skip entries where rc.DNSIP == "" (i.e., treat both rc.Domain and rc.DNSIP as required) before calling formatDNSBlock, so only clusters with non-empty DNSIP produce blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/config/c2cc.go`:
- Around line 360-367: RenderC2CCDNSBlocks currently appends DNS blocks for
every ResolvedRemoteCluster with a Domain, but if rc.DNSIP is empty the
resulting forward directive is invalid; update RenderC2CCDNSBlocks to skip
entries where rc.DNSIP == "" (i.e., treat both rc.Domain and rc.DNSIP as
required) before calling formatDNSBlock, so only clusters with non-empty DNSIP
produce blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 36664956-24a7-47cc-ae45-f4a7e5e341dc
📒 Files selected for processing (4)
pkg/config/c2cc.gotest/resources/c2cc.resourcetest/suites/c2cc/connectivity.robottest/suites/c2cc/dns.robot
🚧 Files skipped from review as they are similar to previous changes (2)
- test/resources/c2cc.resource
- test/suites/c2cc/connectivity.robot
|
/test verify |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@pmtk: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary by CodeRabbit
New Features
Tests
Documentation