Skip to content
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

dns: Set --tofqdns-min-ttl to zero by default #21439

Merged
merged 1 commit into from Mar 10, 2023
Merged

dns: Set --tofqdns-min-ttl to zero by default #21439

merged 1 commit into from Mar 10, 2023

Conversation

michi-covalent
Copy link
Contributor

@michi-covalent michi-covalent commented Sep 26, 2022

This commit changes the default value of --tofqdns-min-ttl from 3600
seconds to zero. This means Cilium honors the TTLs returned from the
upstream DNS server by default. Explicitly configure --tofqdns-min-ttl
if you need to preserve the previous behavior that lets applications
create new connections within the pre-defined --tofqdns-min-ttl time
window after the DNS TTL is expired.

--tofqdns-min-ttl setting is no longer needed since the poll-based DNS
implementation has been replaced by the proxy-based implementation.
Having the minimum TTL set to 1 hour by default adds unnecessary CPU /
memory overhead, as Cilium ends up keeping track of expired DNS info.
This is especially problematic when the upstream DNS server returns
responses with short TTLs and many unique IP addresses.

Co-authored-by: Joe Stringer joe@cilium.io
Signed-off-by: Michi Mutsuzaki michi@isovalent.com

dns: Set --tofqdns-min-ttl to zero by default

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 26, 2022
@joestringer joestringer added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Oct 3, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 3, 2022
@github-actions
Copy link

github-actions bot commented Nov 3, 2022

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Nov 3, 2022
@github-actions
Copy link

This pull request has not seen any activity since it was marked stale.
Closing.

@github-actions github-actions bot closed this Nov 18, 2022
@christarazi christarazi reopened this Nov 18, 2022
@github-actions github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Nov 19, 2022
@michi-covalent michi-covalent force-pushed the pr/michi/fqdn branch 2 times, most recently from 3a1b6ef to db8afc6 Compare November 30, 2022 00:41
@michi-covalent michi-covalent force-pushed the pr/michi/fqdn branch 2 times, most recently from c5d75db to 64e0f6d Compare December 12, 2022 06:45
@michi-covalent michi-covalent changed the title Set --tofqdns-min-ttl to zero by default dns: Set --tofqdns-min-ttl to zero by default Dec 12, 2022
@michi-covalent
Copy link
Contributor Author

/test

@michi-covalent
Copy link
Contributor Author

/test

@github-actions
Copy link

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jan 12, 2023
@christarazi christarazi added kind/enhancement This would improve or streamline existing functionality. area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. and removed stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. labels Jan 12, 2023
@github-actions
Copy link

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Feb 12, 2023
@github-actions
Copy link

This pull request has not seen any activity since it was marked stale.
Closing.

@github-actions github-actions bot closed this Feb 26, 2023
@christarazi christarazi reopened this Mar 6, 2023
@christarazi christarazi added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Mar 6, 2023
@christarazi
Copy link
Member

@michi-covalent We still want this, right? Any reason for why it's blocked?

@michi-covalent
Copy link
Contributor Author

sorry for the delay chris, i just forgot about this. marked ready for review ✅

@michi-covalent
Copy link
Contributor Author

/test

@christarazi christarazi removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Mar 8, 2023
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, all good Michi 🚀

This commit changes the default value of --tofqdns-min-ttl from 3600
seconds to zero. This means Cilium honors the TTLs returned from the
upstream DNS server by default. Explicitly configure --tofqdns-min-ttl
if you need to preserve the previous behavior that lets applications
create new connections within the pre-defined --tofqdns-min-ttl time
window after the DNS TTL is expired.

--tofqdns-min-ttl setting is no longer needed since the poll-based DNS
implementation has been replaced by the proxy-based implementation.
Having the minimum TTL set to 1 hour by default adds unnecessary CPU /
memory overhead, as Cilium ends up keeping track of expired DNS info.
This is especially problematic when the upstream DNS server returns
responses with short TTLs and many unique IP addresses.

Co-authored-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@michi-covalent
Copy link
Contributor Author

/test

Copy link
Contributor

@thorn3r thorn3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amazing

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 10, 2023
@qmonnet qmonnet merged commit 959b40a into master Mar 10, 2023
@qmonnet qmonnet deleted the pr/michi/fqdn branch March 10, 2023 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants