OCPBUGS-31521: Don't allow DNS records for gateway to be created if they clash with any existing ingress controller. #1145
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@rfredette: This pull request references Jira Issue OCPBUGS-31521, 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@rfredette: This pull request references Jira Issue OCPBUGS-31521, 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
removed
the
jira/invalid-bug
|
Hit some 500 internal server errors during initial setup on several tests |
|
/retest |
|
/assign |
|
Lots of disruption failures. |
rfredette
force-pushed
the
OCPBUGS-31521-gateway-dns
branch
from
1266379
to
37d0f99
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…ting ingress controllers.
rfredette
force-pushed
the
OCPBUGS-31521-gateway-dns
branch
from
37d0f99
to
2b5a03e
Compare
|
Test failures include underlying cluster instability |
|
@rfredette: 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-sigs/prow repository. I understand the commands that are listed here. |
| var errs []error | ||
| for _, domain := range domains { | ||
| // Check if the domain matches one from an existing ingress controller. If so, log an error and ignore the invalid domain. | ||
| if err := domainClashes(domain, ics); err != nil { | ||
| log.Error(fmt.Errorf("error creating DNS record for gateway %s: %w", gateway.Name, err), "ignoring invalid gateway domain") |
There was a problem hiding this comment.
Please use %v instead of %w for errors.
| l("stage-https", "*.stage.apps.example.com", 443), | ||
| // apps.example.com looks like it'll clash with the default ic below, but the ic creates a record | ||
| // for *.apps.example.com, which doesn't actually clash with apps.example.com | ||
| l("apps-https", "apps.example.com", 443), |
There was a problem hiding this comment.
This should clash though, because the DNSRecord for the listener is created as a wildcard and thus will be prefixed with *. too. This has to be prevented because if the listener's DNSRecord gets deleted then the IC's DNSRecord is too and the component routes begin to fail.
| name := fmt.Sprintf("%s-%s", gateway.Name, util.Hash(host)) | ||
| if strings.HasPrefix(host, "*.") { | ||
| name = name + "-wildcard" | ||
| } | ||
| return types.NamespacedName{ | ||
| Namespace: gateway.Namespace, | ||
| Name: fmt.Sprintf("%s-%s-wildcard", gateway.Name, util.Hash(host)), | ||
| Name: name, |
There was a problem hiding this comment.
We can't change this, the Listener's DNSRecord will always be a wildcard.
Miciah
added
the
priority/backlog
If a gateway attempts to create a listener for a domain that's already served by an ingress controller, don't create a DNSRecord for that domain, and log an error.
In the future, some more visible method of error reporting should be added, such as an alert or a status condition on a relevant object, but that is intentionally left for a future change.