OCPBUGS-15100: Create valid DNS names for Gateway API on GCP #949
Conversation
openshift-ci-robot
added
jira/severity-critical
|
@candita: This pull request references Jira Issue OCPBUGS-15100, 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@candita: This pull request references Jira Issue OCPBUGS-15100, 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. |
|
/test e2e-hypershift |
/test e2e-azure-operator |
| @@ -222,6 +223,10 @@ func (r *reconciler) ensureDNSRecordsForGateway(ctx context.Context, gateway *ga | |||
| var errs []error | |||
| for _, domain := range domains { | |||
| name := operatorcontroller.GatewayDNSRecordName(gateway, domain) | |||
| // If domain doesn't have a trailing dot, add it | |||
There was a problem hiding this comment.
| // If domain doesn't have a trailing dot, add it | |
| // If domain doesn't have a trailing dot, add it. |
I would have added the trailing dot in desiredDNSRecord, but maybe this makes more sense.
|
/assign @Miciah |
|
/assign |
Done |
|
/retest-required |
|
OCPBUGS-12479 exists for the azure-ovn test issue:
|
/test e2e-hypershift |
|
/test e2e-azure-ovn |
I still only see one line in the commit message of the most recent commit that you pushed for this PR: Did you mean to add more? The description of this PR is pretty descriptive, so I figured you would use that as the commit message (with minor reformatting, and with "for Gateway API" in the commit message's body as you have it in the PR's title). |
Before this change, in ensureDNSRecordsForGateway we didn't add a trailing dot to the domain before creating the dnsrecord, causing the DNS name to fail to be published on a GCP platform. Add a trailing dot if one doesn't exist, add test to check that only one dot is added, and change expectations of other tests to check for a trailing dot in the domain name.
I never add long commit messages like you do. I can start doing it but may need some reminders. |
|
@candita: The following test failed, say
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. |
Thanks! I might go overboard sometimes, so I haven't been insisting on the level of detail that I usually provide, and what you have now is sufficient. However, commit messages (not PR descriptions) are what go into the Git history, which folks rely on to understand the reason and context for a change, for example when fixing a bug or when trying to determine which feature a change was a part of, and so having clear and reasonably detailed commit messages is critical. For single-commit PRs, GitHub uses the commit message as the initial PR description. I've been assuming that folks have been putting the same information in their commit messages that they put into their PR descriptions, and I often don't look at the individual commits, but I'll start being more careful when reviewing PRs. * grin *. /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 |
openshift-ci
bot
added
the
approved
openshift-merge-robot
merged commit ecb3786
into
openshift:master
|
@candita: Jira Issue OCPBUGS-15100: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-15100 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 |
|
@candita: new pull request created: #953 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. |
| // If domain doesn't have a trailing dot, add it | ||
| if !strings.HasSuffix(domain, ".") { | ||
| domain = domain + "." | ||
| } |
There was a problem hiding this comment.
You need to account for the trailing dot in deleteStaleDNSRecordsForGateway too, or else it immediately deletes the DNSRecord CR that ensureDNSRecordsForGateway created. Better yet, add the trailing dot in getGatewayHostnames:
diff --git a/pkg/operator/controller/gateway-service-dns/controller.go b/pkg/operator/controller/gateway-service-dns/controller.go
index 1084d65c..919b8d32 100644
--- a/pkg/operator/controller/gateway-service-dns/controller.go
+++ b/pkg/operator/controller/gateway-service-dns/controller.go
@@ -199,7 +199,12 @@ func getGatewayHostnames(gateway *gatewayapiv1beta1.Gateway) sets.String {
if listener.Hostname == nil || len(*listener.Hostname) == 0 {
continue
}
- domains.Insert(string(*listener.Hostname))
+ domain := string(*listener.Hostname)
+ // If domain doesn't have a trailing dot, add it.
+ if !strings.HasSuffix(domain, ".") {
+ domain = domain + "."
+ }
+ domains.Insert(domain)
}
return domains
}
@@ -223,10 +228,6 @@ func (r *reconciler) ensureDNSRecordsForGateway(ctx context.Context, gateway *ga
var errs []error
for _, domain := range domains {
name := operatorcontroller.GatewayDNSRecordName(gateway, domain)
- // If domain doesn't have a trailing dot, add it
- if !strings.HasSuffix(domain, ".") {
- domain = domain + "."
- }
_, _, err := dnsrecord.EnsureDNSRecord(r.client, name, labels, ownerRef, domain, service)
errs = append(errs, err)
}This also has the advantage that "foo.com" without a trailing "." and and "foo.com." with a trailing "." will get the same hash.
candita
changed the title
|
@candita: Jira Issue OCPBUGS-15434: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-15434 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. |
candita
changed the title
|
@candita: Jira Issue OCPBUGS-15100 is in an unrecognized state (ON_QA) and will not be 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. |
Before this change, in
ensureDNSRecordsForGatewaywe didn't add a trailing dot to the domain before creating the dnsrecord, causing the DNS name to fail to be published on a GCP platform.Add a trailing dot if one doesn't exist, add test to check that only one dot is added, and change expectations of other tests to check for a trailing dot in the domain name.