RFE-9274: add disable-external-dns-management annotation#8708
RFE-9274: add disable-external-dns-management annotation#8708chdeshpa-hue wants to merge 1 commit into
Conversation
When loadBalancer.hostname is configured, the CPO unconditionally injects external-dns.alpha.kubernetes.io/hostname on LoadBalancer services. In environments without an external-dns controller this causes a permanent false-negative ExternalDNSReachable condition. Add hypershift.openshift.io/disable-external-dns-management annotation that suppresses the external-dns hostname annotation injection on KAS, Konnectivity, and OAuth LoadBalancer services. On day-2 opt-out the annotation is actively removed from pre-existing services. The ExternalDNSReachable condition reports True with reason ExternalDNSManagementDisabled when opted out. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@chdeshpa-hue: This pull request references RFE-9274 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 feature request 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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
do-not-merge/needs-area
area/api
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chdeshpa-hue The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThis PR introduces the ability to disable external DNS management on LoadBalancer services via a new HostedCluster annotation. A constant for 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (10 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8708 +/- ##
==========================================
+ Coverage 41.50% 41.51% +0.01%
==========================================
Files 758 758
Lines 93689 93711 +22
==========================================
+ Hits 38882 38906 +24
+ Misses 52070 52069 -1
+ Partials 2737 2736 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@chdeshpa-hue: all tests passed! 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. |
![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 `@control-plane-operator/controllers/hostedcontrolplane/kas/service.go`:
- Around line 80-84: The current logic only deletes
hyperv1.ExternalDNSHostnameAnnotation when the load balancer hostname is
non-empty, causing stale annotations to persist; update the block that
references hcp.Annotations[hyperv1.DisableExternalDNSManagementAnnotation], svc
and strategy.LoadBalancer.Hostname so that if the disable flag
(hyperv1.DisableExternalDNSManagementAnnotation) is "true" you unconditionally
delete hyperv1.ExternalDNSHostnameAnnotation from svc, otherwise set the
annotation when strategy.LoadBalancer.Hostname is non-empty and delete it when
hostname is empty — locate this logic around the KAS/Konnectivity and OAuth
reconcilers where svc and hcp are referenced and adjust the if/else to ensure
unconditional deletion on opt-out (and deletion when hostname is absent).
🪄 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: ad0047bd-7c08-40b4-80bd-e4f12bb09d45
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (8)
api/hypershift/v1beta1/hostedcluster_types.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/infra/infra.gocontrol-plane-operator/controllers/hostedcontrolplane/kas/service.gocontrol-plane-operator/controllers/hostedcontrolplane/kas/service_test.gocontrol-plane-operator/controllers/hostedcontrolplane/oauth/service.gocontrol-plane-operator/controllers/hostedcontrolplane/oauth/service_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
| if hcp.Annotations[hyperv1.DisableExternalDNSManagementAnnotation] != "true" { | ||
| svc.Annotations[hyperv1.ExternalDNSHostnameAnnotation] = strategy.LoadBalancer.Hostname | ||
| } else { | ||
| delete(svc.Annotations, hyperv1.ExternalDNSHostnameAnnotation) | ||
| } |
There was a problem hiding this comment.
Root cause: ExternalDNS annotation cleanup is incorrectly tied to hostname presence across reconcilers.
In both KAS/Konnectivity and OAuth reconcilers, stale hyperv1.ExternalDNSHostnameAnnotation is only deleted when LB hostname is non-empty. Opt-out should delete unconditionally when disabled (and ideally when hostname is absent) to guarantee day-2 cleanup.
🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/kas/service.go` around
lines 80 - 84, The current logic only deletes
hyperv1.ExternalDNSHostnameAnnotation when the load balancer hostname is
non-empty, causing stale annotations to persist; update the block that
references hcp.Annotations[hyperv1.DisableExternalDNSManagementAnnotation], svc
and strategy.LoadBalancer.Hostname so that if the disable flag
(hyperv1.DisableExternalDNSManagementAnnotation) is "true" you unconditionally
delete hyperv1.ExternalDNSHostnameAnnotation from svc, otherwise set the
annotation when strategy.LoadBalancer.Hostname is non-empty and delete it when
hostname is empty — locate this logic around the KAS/Konnectivity and OAuth
reconcilers where svc and hcp are referenced and adjust the if/else to ensure
unconditional deletion on opt-out (and deletion when hostname is absent).
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe gitlint check failed because the commit message title Root CauseThe The commit message title This is purely a commit message formatting issue — there is no code or test problem. RecommendationsAmend the commit message to use a Conventional Commits prefix. Since this PR adds a new annotation/feature,
To fix, run: git commit --amend -m "feat(RFE-9274): add disable-external-dns-management annotation" -m "<existing body>"
git push --force-with-leaseEvidence
|
2 participants
RFE-9274: Allow users to opt out of external-dns annotation injection on LoadBalancer services
Repository:
openshift/hypershiftFixes: RFE-9274
Files modified: 9
Objective
Add a
hypershift.openshift.io/disable-external-dns-management: "true"annotation on HostedCluster that prevents the Control Plane Operator from injecting theexternal-dns.alpha.kubernetes.io/hostnameannotation on LoadBalancer services. This enables users who manage their own DNS (BYODNS) to use HyperShift's LoadBalancer publishing strategy without a false-negativeExternalDNSReachablecondition or coupling to an external-dns controller they don't run.Problem Statement
When
spec.services[].servicePublishingStrategy.loadBalancer.hostnameis configured, the CPO unconditionally injectsexternal-dns.alpha.kubernetes.io/hostnameon the KAS, Konnectivity, and (on Azure) OAuth LoadBalancer services.In environments without an external-dns controller — Agent-based HCP on bare metal, KubeVirt with MetalLB, air-gapped clusters — this produces a permanent false-negative:
No persistent workaround exists. Manually removing the annotation from the service is overwritten by CPO reconciliation within ~30 seconds.
Scope
This PR does:
external-dns.alpha.kubernetes.io/hostnameinjection when the annotation is setExternalDNSReachable: True (ExternalDNSManagementDisabled)when opted outmirroredAnnotationsThis PR does NOT:
apifromapi-intDNS resolutionPlatform applicability:
LoadBalancerstrategy + public endpointLoadBalancerstrategyLoadBalancerstrategyDesign Decision
manageDNS *boolon strategydnsHostnamevsserviceHostnamedisable-external-dns-managementdisable-*patterns, promotable to field lateroc explainClosest code pattern:
DisablePKIReconciliationAnnotation— identical guard structure:Implementation
api/hypershift/v1beta1/hostedcluster_types.goDisableExternalDNSManagementAnnotationconstanthypershift-operator/controllers/hostedcluster/hostedcluster_controller.gomirroredAnnotations(HC -> HCP propagation)control-plane-operator/controllers/hostedcontrolplane/kas/service.gocontrol-plane-operator/controllers/hostedcontrolplane/oauth/service.godisableExternalDNSparameter, guard injectioncontrol-plane-operator/controllers/hostedcontrolplane/infra/infra.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.goreconcileExternalDNSStatusConditionkas/service_test.gooauth/service_test.govendor/.../hostedcluster_types.goCore logic (KAS service):
Condition controller:
Testing Results
Unit Tests
TestReconcileServiceDisableExternalDNS(KAS)TestReconcileKonnectivityServiceDisableExternalDNSTestOauthServiceDisableExternalDNSIntegration Tests (Live Cluster)
Environment: OCP 4.22.0, AWS eu-north-1, MCE v2.16.1, MetalLB L2, custom CPO image with fix.
Test 1: AWS/None — Day-2 Opt-Out
Existing cluster with annotation already injected by stock CPO. Applied fix to verify active removal.
Before:
external-dns.alpha.kubernetes.io/hostnamepresent, conditionFalse.After: Annotation deleted by CPO on reconcile, condition
True.Test 2: KubeVirt — Greenfield
Fresh cluster with annotation set from creation. Proves annotation is never injected.
Workers joined via user-managed DNS (private Route53 zone -> MetalLB IP). Full cluster lifecycle validated.
Test 3: Agent — Greenfield
Agent-based workers (EC2 instances booting RHCOS discovery ISO) with user-managed DNS via internal NLB.
Workers joined, CSRs approved, guest cluster fully operational.
Limitations and Known Gaps
mainbranchVersion Matrix
mirroredAnnotations)Backward Compatibility
Usage
Pre-requisite: Users must create DNS records (
api.<name>.<domain>) pointing to the LoadBalancer IP/hostname before worker nodes can join.Related
mirroredAnnotationsin HO controller already includes the propagation logic inmainapi-intendpoint for internal-only resolution (design discussion pending)Summary by CodeRabbit
Release Notes