-
Notifications
You must be signed in to change notification settings - Fork 190
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.
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
OCPBUGS-13810: Update TestAWSELBConnectionIdleTimeout to not use wildcard DNS record #944
OCPBUGS-13810: Update TestAWSELBConnectionIdleTimeout to not use wildcard DNS record #944
Conversation
@gcs278: This pull request references Jira Issue OCPBUGS-13810, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In 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 kubernetes/test-infra repository. |
@gcs278: This pull request references Jira Issue OCPBUGS-13810, which is invalid:
Comment In 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 kubernetes/test-infra repository. |
…card DNS record TestAWSELBConnectionIdleTimeout has been very flakey due to the fact that the CI test runner cluster is failing to resolve the newly created wildcard DNS Record in a reasonable time. To work around this, we switch to using ELB's hostname, which is consistently resolving and adding the "Host" header to the HTTP request `test/e2e/operator_test.go`: Modify TestAWSELBConnectionIdleTimeout to use ELB hostname and Host header with route hostname
@gcs278: This pull request references Jira Issue OCPBUGS-13810, which is invalid:
Comment In 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 kubernetes/test-infra repository. |
@gcs278: This pull request references Jira Issue OCPBUGS-13810, which is invalid:
Comment In 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 kubernetes/test-infra repository. |
68133e7
to
960a8d6
Compare
/jira refresh |
@gcs278: This pull request references Jira Issue OCPBUGS-13810, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In 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 kubernetes/test-infra repository. |
Round 1: Passed (must gather failures) unrelated e2e-hypershift failures |
Round 2: Passed (must gather failures again) |
distruption issues with the ovn tests: |
round 3 passed e2e-aws-operator: |
Round 4 Passed e2e-aws-operator. I feel like that's enough to prove it works. |
/assign |
if err := kclient.Get(context.TODO(), wildcardRecordName, wildcardRecord); err != nil { | ||
t.Fatalf("failed to get wildcard dnsrecord %s: %v", wildcardRecordName, err) | ||
} | ||
elbHostname := wildcardRecord.Spec.Targets[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a wildcard record, could there be more than one target? How do you know which target matches the ELB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DNSRecord CRD allows there to be more than one target, since a real-life DNS record can support multiple targets; however, in practice the Ingress Operator will never make a DNS record with more than 1 target based on the code here:
cluster-ingress-operator/pkg/resources/dnsrecord/dns.go
Lines 144 to 170 in 71c424d
var target string | |
var recordType iov1.DNSRecordType | |
if len(ingress.Hostname) > 0 { | |
recordType = iov1.CNAMERecordType | |
target = ingress.Hostname | |
} else { | |
recordType = iov1.ARecordType | |
target = ingress.IP | |
} | |
return true, &iov1.DNSRecord{ | |
ObjectMeta: metav1.ObjectMeta{ | |
Namespace: name.Namespace, | |
Name: name.Name, | |
Labels: dnsRecordLabels, | |
OwnerReferences: []metav1.OwnerReference{ownerRef}, | |
Finalizers: []string{manifests.DNSRecordFinalizer}, | |
}, | |
Spec: iov1.DNSRecordSpec{ | |
DNSName: domain, | |
DNSManagementPolicy: dnsPolicy, | |
Targets: []string{target}, | |
RecordType: recordType, | |
RecordTTL: defaultRecordTTL, | |
}, | |
} |
And this assumption of 1 target is made in all of our DNS Provides, e.g.:
cluster-ingress-operator/pkg/dns/aws/dns.go
Line 503 in e8f3f45
domain, target := record.Spec.DNSName, record.Spec.Targets[0] |
cluster-ingress-operator/pkg/dns/azure/dns.go
Line 112 in 475c9b6
Address: record.Spec.Targets[0], |
err = service.Add(zoneInfo.ID, rr, string(record.Spec.RecordType), record.Spec.Targets[0], record.Spec.RecordTTL) |
if err := wait.PollImmediate(5*time.Second, 5*time.Minute, func() (bool, error) { | ||
_, err := net.LookupIP(route.Spec.Host) | ||
_, err := net.LookupIP(elbHostname) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually set the route name and create it in https://github.com/openshift/cluster-ingress-operator/pull/944/files#diff-cf4b4e5424070a666a364d1d1b04011478d888d6d3d65c943ca7783333b7a6e4R2616.
I know this issue is seen on platforms going back to 4.11, but did you investigate whether or not we need to wait for the route to be ready after we create it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking to the lookup prior to this fix, _, err := net.LookupIP(route.Spec.Host)
, the readiness of the route (or even the route itself) should have no impact on the wildcard DNS record that is created by the IngressController.
When we create an ingress controller, in this case called test-idle-timeout
, it creates a wildcard DNS record for *.test-idle-timeout.ci-op-rh28w0d0-43abb.origin-ci-int-aws.dev.rhcloud.com
for subsequent admitted routes to use.
Before this route is even created, any domain that satisfies this wildcard, e.g. foo.test-idle-timeout.ci-op-rh28w0d0-43abb.origin-ci-int-aws.dev.rhcloud.com
, should resolve, regardless if there is a route or not. So we actually don't even need the route for this part of the test, we are waiting on the DNS Wildcard for the Ingress Controller. The route in our test has a hostname of idle-timeout-httpd-openshift-ingress.test-idle-timeout.ci-op-rh28w0d0-43abb.origin-ci-int-aws.dev.rhcloud.com
, but in this case the first part of the domain idle-timeout-httpd-openshift-ingress
provides the same record as foo
.
Furthermore, Miciah suggested to test another random, no-associated-route, domain name inside the same wildcard, so I tested with https://github.com/openshift/cluster-ingress-operator/pull/940/files#diff-cf4b4e5424070a666a364d1d1b04011478d888d6d3d65c943ca7783333b7a6e4R3053-R3059. The results show it doesn't resolve any quicker than the route and had the same problems. Long story short, we don't even need a route created to demonstrate this DNS propagation issue, just the Ingress Controller and the DNS Record.
// Add the "Host" header to direct request to ELB to the route we are testing which bypasses the need | ||
// for the wildcard DNS record to propagate to the CI test runner cluster's DNS servers which has gotten very slow. | ||
// See https://issues.redhat.com/browse/OCPBUGS-13810 | ||
request.Host = route.Spec.Host |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we do this trick anywhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - in verifyExternalIngressController
req.Host = hostname |
unmanaged_dns_test.go
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting that those all call it with a hostname of "apps."+ic.Spec.Domain
, and here we are calling it with a created route hostname.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not obvious the way it's written, but "apps.".ic.Spec.Domain
is a created route hostname. They created it here as a part of echoRoute
:
echoRoute := buildRouteWithHost(echoPod.Name, echoPod.Namespace, echoService.Name, hostname) |
So this verifyExternalIngressController
doing the same style as this PR update.
Overall, looks fine. Even knowing the background, I just had some questions. /lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Miciah The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
e2e-hypershift failed, but this PR only changes test code that the e2e-hypershift job doesn't run. |
e2e-hypershift failed again with similar test failures. |
e2e-hypershift failed because of timeouts pulling from the build05 image registry:
/test e2e-hypershift |
e2e-hypershift failed again, this time on
The container logs have the following:
@enxebre, do you know how to diagnose this further? |
e2e-hypershift failed again with a similar failure:
The container logs have the same panic:
/test e2e-hypershift |
@gcs278: all tests passed! Full PR test history. Your PR dashboard. 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 kubernetes/test-infra repository. I understand the commands that are listed here. |
@gcs278: Jira Issue OCPBUGS-13810: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-13810 has been moved to the MODIFIED state. In 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 kubernetes/test-infra repository. |
/cherry-pick release-4.13 |
@gcs278: new pull request created: #955 In 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 kubernetes/test-infra repository. |
TestAWSELBConnectionIdleTimeout has been very flakey due to the fact that the CI test runner cluster is failing to resolve the newly created wildcard DNS Record in a reasonable time. To work around this, we switch to using ELB's hostname, which is consistently resolving and adding the "Host" header to the HTTP request.
test/e2e/operator_test.go
: Modify TestAWSELBConnectionIdleTimeout to use ELB hostname and Host header with route hostnameWIP PR with debugging output (which helped me arrive at this solution): #940
Why it's failing:
Resolution:
unmanaged_dns_test.go,
just using the same patternAlternative Solutions: