USHIFT-6796: C2CC: DNS forwarding between clusters#6638
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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (16)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughThis PR adds C2CC cross-cluster DNS cache configuration. It introduces configurable TTLs for CoreDNS positive/negative caching, computes upstream DNS IPs from remote cluster service networks, generates CoreDNS server blocks with cache directives, and validates the complete flow with unit tests and end-to-end Robot Framework tests. ChangesCluster-to-Cluster DNS Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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.2)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/[email protected]test/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 <[email protected]>
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
|
/retest |
|
/verified by @pmtk |
openshift-ci-robot
added
the
verified
|
@pmtk: This PR has been marked as verified by 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. |
| Deploy DNS Test Workloads | ||
| [Documentation] Create namespace and deploy hello-microshift + curl-pod on both clusters. | ||
| VAR ${assets}= ${EXECDIR}/assets/c2cc | ||
| FOR ${alias} IN cluster-a cluster-b | ||
| ${ns}= Create Unique Namespace On Cluster ${alias} | ||
| Set To Dictionary ${NAMESPACES} ${alias} ${ns} | ||
| Oc On Cluster ${alias} oc apply -n ${ns} -f ${assets}/hello-microshift.yaml | ||
| Oc On Cluster ${alias} oc apply -n ${ns} -f ${assets}/curl-pod.yaml | ||
| END | ||
| Wait For DNS Test Pods | ||
|
|
||
| Wait For DNS Test Pods | ||
| [Documentation] Wait for all test pods to be Ready on both clusters. | ||
| FOR ${alias} IN cluster-a cluster-b | ||
| Oc On Cluster | ||
| ... ${alias} | ||
| ... oc wait pod/hello-microshift pod/curl-pod -n ${NAMESPACES}[${alias}] --for=condition=Ready --timeout=120s | ||
| END | ||
|
|
||
| Cleanup DNS Test Workloads | ||
| [Documentation] Delete test namespace on both clusters. Ignores errors. | ||
| FOR ${alias} IN cluster-a cluster-b | ||
| Run Keyword And Ignore Error | ||
| ... Oc On Cluster ${alias} oc delete namespace ${NAMESPACES}[${alias}] --timeout=60s | ||
| END |
There was a problem hiding this comment.
These 3 Keywords are almost the same as the ones in connectivity.robot. I'd suggest to move all of them into c2cc.resource
There was a problem hiding this comment.
the order is not really important, because it's mainly for human. The tests does not depend on each other, so it's ok.
openshift-ci-robot
removed
the
verified
|
@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. |
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 `@cmd/generate-config/config/config-openapi-spec.json`:
- Around line 192-199: The schema fields cacheNegativeTTL and cacheTTL declare
they must be >= 0 but lack enforcement; update each property's JSON Schema to
include "minimum": 0 (and keep "type": "integer") so the OpenAPI/JSON schema
validates non-negative TTLs (refer to the cacheNegativeTTL and cacheTTL
properties in the diff and add minimum: 0 to each).
🪄 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: 6712f991-d1f5-4d85-8c7b-4769a8f70065
📒 Files selected for processing (7)
cmd/generate-config/config/config-openapi-spec.jsondocs/user/howto_config.mdpackaging/microshift/config.yamlpkg/components/controllers.gopkg/config/c2cc.gopkg/config/c2cc_test.gopkg/config/config.go
|
@pmtk: The following test 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. |
|
/retest |
3 participants
Summary by CodeRabbit
New Features
Documentation