-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix(provider): IDNA awareness in the zone finder #5705
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
Conversation
|
Welcome @hanapedia! |
Hi @hanapedia. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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'm not sure if this required. Could you share a real case end evidences. I did explicitly a validation. Which image you are using btw. |
Share arguments as well, not helm please. |
/ok-to-test |
@ivankatliarchuk I haven’t run against the current master branch in production yet—the logs I referred to were observed in v0.17.0 and v0.18.0 when creating or deleting a DNSEndpoint with The exact warning logs (domain masked) were:
Arguments used (running on bare metal Kubernetes with CloudDNS provider): args:
- --source=crd
- --source=service
- --provider=google
- --registry=txt
- --log-format=json
- --publish-internal-services I also verified that this behavior still exists in current master by adding new test cases with "*" in the domain name to zonefinder_test.go—the warnings still appear there. hook := testutils.LogsUnderTestWithLogLevel(log.WarnLevel, t)
zoneID, zoneName = z.FindZone("*.example.com")
assert.Equal(t, "*.example.com", zoneName)
assert.Equal(t, "123412", zoneID)
testutils.TestHelperLogContains("Failed to convert label \"*\" of hostname \"*.example.com\" to its Unicode form: idna: disallowed rune U+002A", hook, t) |
Signed-off-by: Hiroki Hanada <[email protected]>
Will double check when have time just in case. Fixes #5706 |
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.
/lgtm
cc: @mloiseleur
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mloiseleur 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 |
Signed-off-by: Hiroki Hanada <[email protected]>
What does it do ?
This PR fixes an overlooked usage of the old IDNA logic in
FindZone()
that still emits warning logs when parsing hostnames containing the*
character. Although PR #5685 addressed this issue in some parts of the code by using a new IDNA profile that allows characters like*
, one remaining path still uses the old profile, resulting in warning logs like:Failed to convert label “*” of hostname “*.example.com” to its Unicode form: idna: disallowed rune U+002A
This PR updates the logic to consistently use the intended IDNA profile and avoid logging spurious warnings for wildcard hostnames.
Motivation
This PR follows up on the changes in #5685 , which partially addressed unnecessary warning logs for hostnames with characters like
*
. However, theFindZone()
function still uses the outdated IDNA conversion logic, causing the warning to appear even after #5685 was merged.Suppressing these unnecessary warnings improves log clarity and avoids confusion for users working with wildcard DNS entries.
More