Skip to content

RFE-9274: add disable-external-dns-management annotation #8708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chdeshpa-hue wants to merge 1 commit into
base: main
Choose a base branch
from
Open

RFE-9274: add disable-external-dns-management annotation #8708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions api/hypershift/v1beta1/hostedcluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,11 @@ const (
// ExternalDNSHostnameAnnotation is the annotation external-dns uses to register DNS name for different HCP services.
ExternalDNSHostnameAnnotation = "external-dns.alpha.kubernetes.io/hostname"

// DisableExternalDNSManagementAnnotation when set to "true" on a HostedCluster prevents the
// control-plane-operator from adding the external-dns hostname annotation to LoadBalancer services.
// This allows users to manage their own DNS records for the API and Konnectivity endpoints.
DisableExternalDNSManagementAnnotation = "hypershift.openshift.io/disable-external-dns-management"

// ForceUpgradeToAnnotation is the annotation that forces HostedCluster upgrade even if the underlying ClusterVersion
// is reporting it is not Upgradeable. The annotation value must be set to the release image being forced.
ForceUpgradeToAnnotation = "hypershift.openshift.io/force-upgrade-to"
Expand Down
12 changes: 12 additions & 0 deletions control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -773,6 +773,18 @@ func (r *HostedControlPlaneReconciler) reconcileExternalDNSStatusCondition(ctx c
Reason: hyperv1.StatusUnknownReason,
}

if hostedControlPlane.Annotations[hyperv1.DisableExternalDNSManagementAnnotation] == "true" {
newCondition = metav1.Condition{
Type: string(hyperv1.ExternalDNSReachable),
Status: metav1.ConditionTrue,
Reason: "ExternalDNSManagementDisabled",
Message: "External DNS management is disabled by annotation",
}
newCondition.ObservedGeneration = hostedControlPlane.Generation
meta.SetStatusCondition(&hostedControlPlane.Status.Conditions, newCondition)
return
}

kasExternalHostname := netutil.ServiceExternalDNSHostname(hostedControlPlane, hyperv1.APIServer)
if kasExternalHostname != "" {
if err := netutil.ResolveDNSHostname(ctx, kasExternalHostname); err != nil {
Expand Down
2 changes: 1 addition & 1 deletion control-plane-operator/controllers/hostedcontrolplane/infra/infra.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ func (r *Reconciler) reconcileOAuthServerService(ctx context.Context, hcp *hyper
p := oauth.NewOAuthServiceParams(hcp)
oauthServerService := manifests.OauthServerService(hcp.Namespace)
if _, err := createOrUpdate(ctx, r.Client, oauthServerService, func() error {
return oauth.ReconcileService(oauthServerService, p.OwnerRef, serviceStrategy, hcp.Spec.Platform.Type, netutil.IsPrivateHCP(hcp))
return oauth.ReconcileService(oauthServerService, p.OwnerRef, serviceStrategy, hcp.Spec.Platform.Type, netutil.IsPrivateHCP(hcp), hcp.Annotations[hyperv1.DisableExternalDNSManagementAnnotation] == "true")
}); err != nil {
return fmt.Errorf("failed to reconcile OAuth service: %w", err)
}
Expand Down
12 changes: 10 additions & 2 deletions control-plane-operator/controllers/hostedcontrolplane/kas/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,11 @@ func ReconcileService(svc *corev1.Service, strategy *hyperv1.ServicePublishingSt
svc.Annotations[AWSNLBAnnotation] = "nlb"
}
if strategy.LoadBalancer != nil && strategy.LoadBalancer.Hostname != "" {
svc.Annotations[hyperv1.ExternalDNSHostnameAnnotation] = strategy.LoadBalancer.Hostname
if hcp.Annotations[hyperv1.DisableExternalDNSManagementAnnotation] != "true" {
svc.Annotations[hyperv1.ExternalDNSHostnameAnnotation] = strategy.LoadBalancer.Hostname
} else {
delete(svc.Annotations, hyperv1.ExternalDNSHostnameAnnotation)
}
Comment on lines +80 to +84

@coderabbitai coderabbitai Bot Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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).

}
if !azureutil.IsAroHCP() {
svc.Spec.LoadBalancerSourceRanges = apiAllowedCIDRBlocks
Expand Down Expand Up @@ -309,7 +313,11 @@ func ReconcileKonnectivityServerService(svc *corev1.Service, ownerRef config.Own
if svc.Annotations == nil {
svc.Annotations = map[string]string{}
}
svc.Annotations[hyperv1.ExternalDNSHostnameAnnotation] = strategy.LoadBalancer.Hostname
if hcp.Annotations[hyperv1.DisableExternalDNSManagementAnnotation] != "true" {
svc.Annotations[hyperv1.ExternalDNSHostnameAnnotation] = strategy.LoadBalancer.Hostname
} else {
delete(svc.Annotations, hyperv1.ExternalDNSHostnameAnnotation)
}
}
case hyperv1.NodePort:
svc.Spec.Type = corev1.ServiceTypeNodePort
Expand Down
125 changes: 125 additions & 0 deletions control-plane-operator/controllers/hostedcontrolplane/kas/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,3 +438,128 @@ func TestKonnectivityServiceReconcile(t *testing.T) {
})
}
}

func TestReconcileServiceDisableExternalDNS(t *testing.T) {
host := "api.example.com"

testCases := []struct {
name string
hcpAnnotations map[string]string
existingAnnotations map[string]string
expectAnnotation bool
}{
{
name: "ExternalDNS annotation is set when disable annotation is absent",
hcpAnnotations: nil,
expectAnnotation: true,
},
{
name: "ExternalDNS annotation is not set when disable annotation is true",
hcpAnnotations: map[string]string{hyperv1.DisableExternalDNSManagementAnnotation: "true"},
expectAnnotation: false,
},
{
name: "ExternalDNS annotation is set when disable annotation is false",
hcpAnnotations: map[string]string{hyperv1.DisableExternalDNSManagementAnnotation: "false"},
expectAnnotation: true,
},
{
name: "ExternalDNS annotation is deleted on day-2 opt-out",
hcpAnnotations: map[string]string{hyperv1.DisableExternalDNSManagementAnnotation: "true"},
existingAnnotations: map[string]string{
hyperv1.ExternalDNSHostnameAnnotation: host,
},
expectAnnotation: false,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)

hcp := &hyperv1.HostedControlPlane{
ObjectMeta: v1.ObjectMeta{
Annotations: tc.hcpAnnotations,
},
Spec: hyperv1.HostedControlPlaneSpec{
Platform: hyperv1.PlatformSpec{
Type: hyperv1.AWSPlatform,
AWS: &hyperv1.AWSPlatformSpec{
EndpointAccess: hyperv1.Public,
},
},
},
}
svc := &corev1.Service{
ObjectMeta: v1.ObjectMeta{
Annotations: tc.existingAnnotations,
},
}
strategy := &hyperv1.ServicePublishingStrategy{
Type: hyperv1.LoadBalancer,
LoadBalancer: &hyperv1.LoadBalancerPublishingStrategy{
Hostname: host,
},
}

err := ReconcileService(svc, strategy, &v1.OwnerReference{}, 6443, []string{}, hcp)
g.Expect(err).ToNot(HaveOccurred())

if tc.expectAnnotation {
g.Expect(svc.Annotations).To(HaveKeyWithValue(hyperv1.ExternalDNSHostnameAnnotation, host))
} else {
g.Expect(svc.Annotations).ToNot(HaveKey(hyperv1.ExternalDNSHostnameAnnotation))
}
})
}
}

func TestReconcileKonnectivityServiceDisableExternalDNS(t *testing.T) {
host := "konnectivity.example.com"

testCases := []struct {
name string
hcpAnnotations map[string]string
expectAnnotation bool
}{
{
name: "ExternalDNS annotation is set when disable annotation is absent",
hcpAnnotations: nil,
expectAnnotation: true,
},
{
name: "ExternalDNS annotation is not set when disable annotation is true",
hcpAnnotations: map[string]string{hyperv1.DisableExternalDNSManagementAnnotation: "true"},
expectAnnotation: false,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)

hcp := &hyperv1.HostedControlPlane{
ObjectMeta: v1.ObjectMeta{
Annotations: tc.hcpAnnotations,
},
Spec: hyperv1.HostedControlPlaneSpec{
Platform: hyperv1.PlatformSpec{Type: hyperv1.AWSPlatform},
},
}
svc := &corev1.Service{}
strategy := &hyperv1.ServicePublishingStrategy{
Type: hyperv1.LoadBalancer,
LoadBalancer: &hyperv1.LoadBalancerPublishingStrategy{
Hostname: host,
},
}

err := ReconcileKonnectivityServerService(svc, config.OwnerRef{}, strategy, hcp)
g.Expect(err).ToNot(HaveOccurred())

if tc.expectAnnotation {
g.Expect(svc.Annotations).To(HaveKeyWithValue(hyperv1.ExternalDNSHostnameAnnotation, host))
} else {
g.Expect(svc.Annotations).ToNot(HaveKey(hyperv1.ExternalDNSHostnameAnnotation))
}
})
}
}
8 changes: 6 additions & 2 deletions control-plane-operator/controllers/hostedcontrolplane/oauth/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ var (
}
)

func ReconcileService(svc *corev1.Service, ownerRef config.OwnerRef, strategy *hyperv1.ServicePublishingStrategy, platformType hyperv1.PlatformType, isPrivate bool) error {
func ReconcileService(svc *corev1.Service, ownerRef config.OwnerRef, strategy *hyperv1.ServicePublishingStrategy, platformType hyperv1.PlatformType, isPrivate bool, disableExternalDNS bool) error {
ownerRef.ApplyTo(svc)
if svc.Spec.Selector == nil {
svc.Spec.Selector = oauthServerLabels
Expand Down Expand Up @@ -69,7 +69,11 @@ func ReconcileService(svc *corev1.Service, ownerRef config.OwnerRef, strategy *h
svc.Annotations = map[string]string{}
}
if strategy.LoadBalancer != nil && strategy.LoadBalancer.Hostname != "" {
svc.Annotations[hyperv1.ExternalDNSHostnameAnnotation] = strategy.LoadBalancer.Hostname
if !disableExternalDNS {
svc.Annotations[hyperv1.ExternalDNSHostnameAnnotation] = strategy.LoadBalancer.Hostname
} else {
delete(svc.Annotations, hyperv1.ExternalDNSHostnameAnnotation)
}
}
if isPrivate {
svc.Annotations[azureutil.InternalLoadBalancerAnnotation] = azureutil.InternalLoadBalancerValue
Expand Down
45 changes: 44 additions & 1 deletion control-plane-operator/controllers/hostedcontrolplane/oauth/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ func TestOauthServiceReconcile(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)
err := ReconcileService(&tc.svc_in, config.OwnerRef{}, &tc.strategy, tc.platform, tc.isPrivate)
err := ReconcileService(&tc.svc_in, config.OwnerRef{}, &tc.strategy, tc.platform, tc.isPrivate, false)

if tc.err == nil {
g.Expect(err).ToNot(HaveOccurred())
Expand Down Expand Up @@ -351,3 +351,46 @@ func TestReconcileServiceStatus(t *testing.T) {
})
}
}

func TestOauthServiceDisableExternalDNS(t *testing.T) {
hostname := "oauth.example.com"

testCases := []struct {
name string
disableExtDNS bool
expectAnnotation bool
}{
{
name: "ExternalDNS annotation is set when not disabled",
disableExtDNS: false,
expectAnnotation: true,
},
{
name: "ExternalDNS annotation is not set when disabled",
disableExtDNS: true,
expectAnnotation: false,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)

svc := &corev1.Service{}
strategy := &v1beta1.ServicePublishingStrategy{
Type: v1beta1.LoadBalancer,
LoadBalancer: &v1beta1.LoadBalancerPublishingStrategy{
Hostname: hostname,
},
}

err := ReconcileService(svc, config.OwnerRef{}, strategy, v1beta1.AzurePlatform, false, tc.disableExtDNS)
g.Expect(err).ToNot(HaveOccurred())

if tc.expectAnnotation {
g.Expect(svc.Annotations).To(HaveKeyWithValue(v1beta1.ExternalDNSHostnameAnnotation, hostname))
} else {
g.Expect(svc.Annotations).ToNot(HaveKey(v1beta1.ExternalDNSHostnameAnnotation))
}
})
}
}
1 change: 1 addition & 0 deletions hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2372,6 +2372,7 @@ func reconcileHostedControlPlaneAnnotations(hcp *hyperv1.HostedControlPlane, hcl
"hypershift.openshift.io/aws-termination-handler-queue-url",
hyperv1.SwiftPodNetworkInstanceAnnotation,
hyperv1.EnableMetricsForwarding,
hyperv1.DisableExternalDNSManagementAnnotation,
}
for _, key := range mirroredAnnotations {
val, hasVal := hcluster.Annotations[key]
Expand Down
5 changes: 5 additions & 0 deletions vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading