Skip to content

Increase CNAME lookup limit from 7 to 10 #7153

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

Jump to bottom
Merged
merged 1 commit into from
Mar 24, 2025
Merged

Increase CNAME lookup limit from 7 to 10 #7153

merged 1 commit into from
Mar 24, 2025

Conversation

uuoonim
Copy link
Contributor

@uuoonim uuoonim commented Feb 21, 2025

1. Why is this pull request needed and what does it do?

Found a use case where we need to do more than 7 CNAME lookups. For example, hitting an OpenAI endpoint from azure. One example of the endpoint they have on their website is docs-test-001.openai.azure.com, which exceeds 7 lookups, cannot get to the A record.

2. Which issues (if any) are related?

#7123
#7125

3. Which documentation changes (if any) need to be made?

This isn't documented anywhere already. Not sure if it needs to be

4. Does this introduce a backward incompatible change or deprecation?

No

@uuoonim uuoonim force-pushed the increase-cname-limit branch 2 times, most recently from b6dfb7f to edfa8f2 Compare February 21, 2025 21:12
@SuperQ
Copy link
Collaborator

SuperQ commented Feb 26, 2025

So digging around, I found a thread NLnetLabs/unbound#438 where a similar limit was increased to 11.

There is https://lists.isc.org/pipermail/bind-users/2010-July/080543.html that claims BIND will do 16.

Perhaps it's worth making this a configuration option at the same time we bump it?

@codecov Codecov
Copy link

codecov bot commented Feb 26, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 57.84%. Comparing base (93c57b6) to head (d96036e).
Report is 1383 commits behind head on master.

Files with missing lines Patch % Lines
plugin/backend_lookup.go 0.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7153      +/-   ##
==========================================
+ Coverage   55.70%   57.84%   +2.14%     
==========================================
  Files         224      270      +46     
  Lines       10016    17383    +7367     
==========================================
+ Hits         5579    10056    +4477     
- Misses       3978     6716    +2738     
- Partials      459      611     +152

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@uuoonim uuoonim changed the title Incrase CNAME lookup limit from 7 to 10 Increase CNAME lookup limit from 7 to 10 Feb 26, 2025
@uuoonim
Copy link
Contributor Author

uuoonim commented Feb 26, 2025 •
edited
Loading

Perhaps it's worth making this a configuration option at the same time we bump it?

Thought about making it a config option too but wasn't sure where to begin. Ill give it a shot though

Question: would this be a proper use case to create a plugin? or can this be done on existing code?

@uuoonim
Copy link
Contributor Author

uuoonim commented Mar 4, 2025

Can we get this merged for now? I can work on getting it a configurable value after, not sure how long it'll take for me.

@uuoonim uuoonim force-pushed the increase-cname-limit branch from edfa8f2 to 5a5c3d6 Compare March 4, 2025 16:18
@SuperQ
Copy link
Collaborator

SuperQ commented Mar 4, 2025

Yes, I'm OK with this for now.

Maybe at a minimum, change the value to a package level const so it's consistent. We can refactor it to a config var later.

@uuoonim uuoonim force-pushed the increase-cname-limit branch 2 times, most recently from e3e3542 to 28462f7 Compare March 13, 2025 19:43
@uuoonim
Copy link
Contributor Author

uuoonim commented Mar 13, 2025

Yes, I'm OK with this for now.

Maybe at a minimum, change the value to a package level const so it's consistent. We can refactor it to a config var later.

Sorry for the delay. done.

@johnbelamaric
Copy link
Member

@uuoonim you need to fix the CI failures before we can merge this

@uuoonim uuoonim force-pushed the increase-cname-limit branch from 28462f7 to 5a5c3d6 Compare March 14, 2025 19:02
@uuoonim @uuoonim-corp
Increase CNAME lookup limit from 7 to 10
d96036e 
Signed-off-by: Min Woo Kim <59036289+minportant@users.noreply.github.com>
Signed-off-by: Min Woo Kim <minportant@gmail.com>
@uuoonim uuoonim force-pushed the increase-cname-limit branch from 5a5c3d6 to d96036e Compare March 14, 2025 19:03
@chrisohaver chrisohaver merged commit 33d0d05 into coredns:master Mar 24, 2025
13 checks passed
@BrewTestBot BrewTestBot mentioned this pull request Mar 31, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperQ SuperQ SuperQ approved these changes

@miekg miekg Awaiting requested review from miekg miekg is a code owner

@chrisohaver chrisohaver Awaiting requested review from chrisohaver chrisohaver is a code owner

@johnbelamaric johnbelamaric Awaiting requested review from johnbelamaric johnbelamaric is a code owner

@yongtang yongtang Awaiting requested review from yongtang yongtang is a code owner

@stp-ip stp-ip Awaiting requested review from stp-ip stp-ip is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@uuoonim @SuperQ @johnbelamaric @chrisohaver