Skip to content

OCPBUGS-59176: skip specific tests if DNSManaged is false #29985

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

Closed
lihongan wants to merge 1 commit into from
Closed

OCPBUGS-59176: skip specific tests if DNSManaged is false #29985

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
27 changes: 27 additions & 0 deletions test/extended/router/gatewayapicontroller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"net"
"net/http"
"reflect"
"strings"
"time"

Expand Down Expand Up @@ -218,6 +219,14 @@ var _ = g.Describe("[sig-network-edge][OCPFeatureGate:GatewayAPIController][Feat
})

g.It("Ensure HTTPRoute object is created", func() {
isDNSManaged, err := isDNSManaged(oc, time.Minute)
if err != nil {
e2e.Failf("Failed to get default ingresscontroller DNSManaged status: %v", err)
}
if !isDNSManaged {
g.Skip("Skipping on this cluster since DNSManaged is false")
}

g.By("Ensure default GatewayClass is accepted")
errCheck := checkGatewayClass(oc, gatewayClassName)
o.Expect(errCheck).NotTo(o.HaveOccurred(), "GatewayClass %q was not installed and accepted", gatewayClassName)
Expand Down Expand Up @@ -425,14 +434,32 @@ func assertDNSRecordStatus(oc *exutil.CLI, gatewayName string) {
for _, record := range gatewayDNSRecords.Items {
if record.Labels["gateway.networking.k8s.io/gateway-name"] == gatewayName {
gatewayDNSRecord = &record
e2e.Logf("Found the desired dnsrecord and spec is: %v", gatewayDNSRecord.Spec)
break
}
}

// skip status check if privateZone/publicZone in dns.config is nil or empty
emptyDNSZone := &configv1.DNSZone{}
dns, err := oc.AdminConfigClient().ConfigV1().DNSes().Get(context, "cluster", metav1.GetOptions{})
Copy link
Contributor

@candita candita Aug 15, 2025

Choose a reason for hiding this comment

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

Can this be checked outside the 10 minute loop, or do we need to check it every time?

if err != nil {
e2e.Logf("Get dnses.config/cluster failed: %v, retrying...", err)
return false, nil
}
if dns.Spec.PublicZone == nil && dns.Spec.PrivateZone == nil {
e2e.Logf("All zones in dns.config are nil, needn't to check status")
return true, nil
}
if reflect.DeepEqual(dns.Spec.PublicZone, emptyDNSZone) || reflect.DeepEqual(dns.Spec.PrivateZone, emptyDNSZone) {
e2e.Logf("Zone in dns.config is empty, needn't to check status")
return true, nil
}

// checking the gateway DNS record status
for _, zone := range gatewayDNSRecord.Status.Zones {
for _, condition := range zone.Conditions {
if condition.Type == "Published" && condition.Status == "True" {
e2e.Logf("The published status is true for zone %v", zone.DNSZone)
return true, nil
}
}
Expand Down
14 changes: 11 additions & 3 deletions test/extended/router/grpc-interop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,16 @@ var _ = g.Describe("[sig-network-edge][Conformance][Area:Networking][Feature:Rou
g.Skip("Skip on platforms where the default router is not exposed by a load balancer service.")
}

defaultDomain, err := getDefaultIngressClusterDomainName(oc, time.Minute)
o.Expect(err).NotTo(o.HaveOccurred(), "failed to find default domain name")
isDNSManaged, err := isDNSManaged(oc, time.Minute)
if err != nil {
e2e.Failf("Failed to get default ingresscontroller DNSManaged status: %v", err)
}
if !isDNSManaged {
g.Skip("Skipping on this cluster since DNSManaged is false")
}
Comment on lines +56 to +62
Copy link
Contributor

@alebedev87 alebedev87 Jul 18, 2025

Choose a reason for hiding this comment

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

I'm wondering why only GRPC interoperability test should be skipped. We have a lot more tests in extended/router suite, do they all work when there are no DNS zones for the cluster?

Copy link
Contributor Author

@lihongan lihongan Jul 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Firstly, there are three conditions for DNSManaged status and if any of them are unsatisfied the status will be set as False, DNS zones in dns.config is just one of condition. see https://github.com/openshift/api/blob/fed56f2794b13fb3691cee40d0b429c2111d0e7f/operator/v1/types_ingress.go#L2060-L2065

Secondly, in the job "e2e-gcp-user-provisioned-dns" we just see the 4 failing tests, and the cluster set LoadBalancer.DNSManagementPolicy = UnmanagedLoadBalancerDNS but not rely on DNS zones. see https://github.com/openshift/cluster-ingress-operator/blob/cbc0b217b655f1f0ce0becc9145c2a6042beabea/pkg/operator/controller/ingress/controller.go#L478-L492

Thirdly, I believe most tests in extended/router suite are just tested with default ingresscontroller and the request go through default router pod, but GRPC/http2 tests create another shard ingresscontroller to test the feature and it rely on cloud DNS to resolve the shard ingresscontroller domain.

Copy link
Contributor

@sadasu sadasu Aug 6, 2025

Choose a reason for hiding this comment

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

@alebedev87 , @lihongan just checking to see what the resolution of this is. We need this fix so that the "e2e-gcp-user-provisioned-dns" periodic jobs are not in the Red due to these tests and force this feature to be reverted.

Copy link
Contributor

@candita candita Aug 15, 2025

Choose a reason for hiding this comment

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

Are we planning to make this a permanent skip? I don't agree with that.

Copy link
Contributor Author

@lihongan lihongan Aug 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As discussed with Andrey, I plan to amend the logic of sending of HTTP requests to target the Load Balancer's IP address in case the DNS is not managed, with this trick then we don't need to skip the tests.


baseDomain, err := getClusterBaseDomainName(oc, time.Minute)
o.Expect(err).NotTo(o.HaveOccurred(), "failed to find base domain name")

g.By("Locating the canary image reference")
image, err := getCanaryImage(oc)
Expand Down Expand Up @@ -196,7 +204,7 @@ var _ = g.Describe("[sig-network-edge][Conformance][Area:Networking][Feature:Rou
pemCrt2, err := certgen.MarshalCertToPEMString(tlsCrt2Data)
o.Expect(err).NotTo(o.HaveOccurred())

shardFQDN := oc.Namespace() + "." + defaultDomain
shardFQDN := oc.Namespace() + "." + baseDomain

g.By("Creating routes to test for gRPC interoperability")
routeType := oc.Namespace()
Expand Down
8 changes: 4 additions & 4 deletions test/extended/router/h2spec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,9 @@ var _ = g.Describe("[sig-network-edge][Conformance][Area:Networking][Feature:Rou
g.Skip("Skip on platforms where the default router is not exposed by a load balancer service.")
}

g.By("Getting the default domain")
defaultDomain, err := getDefaultIngressClusterDomainName(oc, time.Minute)
o.Expect(err).NotTo(o.HaveOccurred(), "failed to find default domain name")
g.By("Getting the base domain")
baseDomain, err := getClusterBaseDomainName(oc, time.Minute)
o.Expect(err).NotTo(o.HaveOccurred(), "failed to find base domain name")

g.By("Locating the router image reference")
routerImage, err := exutil.FindRouterImage(oc)
Expand Down Expand Up @@ -393,7 +393,7 @@ BFNBRELPe53ZdLKWpf2Sr96vRPRNw
e2e.ExpectNoError(e2epod.WaitForPodNameRunningInNamespace(context.TODO(), oc.KubeClient(), "h2spec-haproxy", oc.KubeFramework().Namespace.Name))
e2e.ExpectNoError(e2epod.WaitForPodNameRunningInNamespace(context.TODO(), oc.KubeClient(), "h2spec", oc.KubeFramework().Namespace.Name))

shardFQDN := oc.Namespace() + "." + defaultDomain
shardFQDN := oc.Namespace() + "." + baseDomain

// The new router shard is using a namespace
// selector so label this test namespace to
Expand Down
54 changes: 51 additions & 3 deletions test/extended/router/http2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
utilpointer "k8s.io/utils/pointer"

configv1 "github.com/openshift/api/config/v1"
operatorv1 "github.com/openshift/api/operator/v1"
routev1 "github.com/openshift/api/route/v1"
routeclientset "github.com/openshift/client-go/route/clientset/versioned"

Expand Down Expand Up @@ -109,8 +110,16 @@ var _ = g.Describe("[sig-network-edge][Conformance][Area:Networking][Feature:Rou
g.Skip("Skip on platforms where the default router is not exposed by a load balancer service.")
}

defaultDomain, err := getDefaultIngressClusterDomainName(oc, time.Minute)
o.Expect(err).NotTo(o.HaveOccurred(), "failed to find default domain name")
isDNSManaged, err := isDNSManaged(oc, time.Minute)
if err != nil {
e2e.Failf("Failed to get default ingresscontroller DNSManaged status: %v", err)
}
if !isDNSManaged {
g.Skip("Skipping on this cluster since DNSManaged is false")
}

baseDomain, err := getClusterBaseDomainName(oc, time.Minute)
o.Expect(err).NotTo(o.HaveOccurred(), "failed to find base domain name")

g.By("Locating the canary image reference")
image, err := getCanaryImage(oc)
Expand Down Expand Up @@ -254,7 +263,7 @@ var _ = g.Describe("[sig-network-edge][Conformance][Area:Networking][Feature:Rou
pemCrt2, err := certgen.MarshalCertToPEMString(tlsCrt2Data)
o.Expect(err).NotTo(o.HaveOccurred())

shardFQDN := oc.Namespace() + "." + defaultDomain
shardFQDN := oc.Namespace() + "." + baseDomain

// The new router shard is using a namespace
// selector so label this test namespace to
Expand Down Expand Up @@ -520,6 +529,23 @@ var _ = g.Describe("[sig-network-edge][Conformance][Area:Networking][Feature:Rou
})
})

func getClusterBaseDomainName(oc *exutil.CLI, timeout time.Duration) (string, error) {
var domain string

if err := wait.PollImmediate(time.Second, timeout, func() (bool, error) {
dns, err := oc.AdminConfigClient().ConfigV1().DNSes().Get(context.TODO(), "cluster", metav1.GetOptions{})
if err != nil {
e2e.Logf("Get dnses.config/cluster failed: %v, retrying...", err)
return false, nil
}
domain = dns.Spec.BaseDomain
return true, nil
}); err != nil {
return "", err
}
return domain, nil
}

func getDefaultIngressClusterDomainName(oc *exutil.CLI, timeout time.Duration) (string, error) {
var domain string

Expand Down Expand Up @@ -598,3 +624,25 @@ func platformHasHTTP2LoadBalancerService(platformType configv1.PlatformType) boo
return false
}
}

func isDNSManaged(oc *exutil.CLI, timeout time.Duration) (bool, error) {
var status operatorv1.ConditionStatus

if err := wait.PollImmediate(time.Second, timeout, func() (bool, error) {
ic, err := oc.AdminOperatorClient().OperatorV1().IngressControllers("openshift-ingress-operator").Get(context.Background(), "default", metav1.GetOptions{})
if err != nil {
e2e.Logf("Failed to get default ingresscontroller: %v, retrying...", err)
return false, nil
}
for _, condition := range ic.Status.Conditions {
if condition.Type == string(operatorv1.DNSManagedIngressConditionType) {
status = condition.Status
return true, nil
}
}
return false, nil
}); err != nil {
return false, err
}
return status != operatorv1.ConditionFalse, nil
}