dnsproxy: bind dns proxy to localhost only #25309
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
mhofstetter
added
the
release-note/minor
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
mhofstetter
force-pushed
the
pr/mhofstetter/dns-proxy-explicit-listeners
branch
from
aba89a7
to
f0a8213
Compare
|
/test |
mhofstetter
force-pushed
the
pr/mhofstetter/dns-proxy-explicit-listeners
branch
from
f0a8213
to
839791d
Compare
|
/test |
mhofstetter
force-pushed
the
pr/mhofstetter/dns-proxy-explicit-listeners
branch
from
839791d
to
f48014f
Compare
|
/test Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed: Click to show.Test NameFailure OutputJenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2166/ If it is a flake and a GitHub issue doesn't already exist to track it, comment Then please upload the Jenkins artifacts to that issue. Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed: Click to show.Test NameFailure OutputJenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2175/ If it is a flake and a GitHub issue doesn't already exist to track it, comment Then please upload the Jenkins artifacts to that issue. |
mhofstetter
force-pushed
the
pr/mhofstetter/dns-proxy-explicit-listeners
branch
from
f48014f
to
efac03a
Compare
|
(after rebasing to /test |
pippolo84
requested changes
May 10, 2023
Currently the dns proxy is bound to all interfaces. With this commit, the dns proxy only gets bound to the localhost interfaces. Therefore, up to 4 DNS servers are created (udpv4, tcpv4, udpv6, tcpv6 - depending on the configuration) which all are using the same DNS handler. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit introduces proper error propagation when errors occur during sessionFactory.SetSocketOptions. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Currently, switching the localOnly property of a proxy rule doesn't get reflected in the iptables rules. The checks for adding iptable rules don't include the IP of a rule. Therefore, when upgrading, the old rule remains in the table. The same applies for the check whether an outdated rule should be deleted from the table. This commit fixes this, and adds support for changing the localOnly property of a proxy rule. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit introduces the ipfamily type with its current implementations v4 & v6. This way duplication can be avoided. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Currently, dns server goroutine would exit with log.fatal in case of returning without an error from ActivateAndServe (in case of a properly initiated shutdown). This commit changes this behaviour to only fail fatally in case of a returned error. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
mhofstetter
force-pushed
the
pr/mhofstetter/dns-proxy-explicit-listeners
branch
from
efac03a
to
9215f5e
Compare
|
/test |
2 tasks
christarazi
added a commit
to christarazi/cilium
that referenced
this pull request
Sep 5, 2023
In a previous change [1], the bind address for the proxy changed from 0.0.0.0 to localhost. This broke restoring the old proxy port and caused Cilium to always allocate a new proxy port. Fix it by changing the regex string to include the new bind address as well as the previously used "0.0.0.0" and "::", for backwards-compatibility reasons on upgrade. Found by code inspection. [1]: cilium#25309 Fixes: 5304088 ("dnsproxy: bind dns proxy to localhost only") Fixes: cilium#25309 Signed-off-by: Chris Tarazi <chris@isovalent.com>
youngnick
pushed a commit
that referenced
this pull request
Sep 10, 2023
In a previous change [1], the bind address for the proxy changed from 0.0.0.0 to localhost. This broke restoring the old proxy port and caused Cilium to always allocate a new proxy port. Fix it by changing the regex string to include the new bind address as well as the previously used "0.0.0.0" and "::", for backwards-compatibility reasons on upgrade. Found by code inspection. [1]: #25309 Fixes: 5304088 ("dnsproxy: bind dns proxy to localhost only") Fixes: #25309 Signed-off-by: Chris Tarazi <chris@isovalent.com>
christarazi
added a commit
that referenced
this pull request
Sep 12, 2023
[ upstream commit 1f8e015 ] In a previous change [1], the bind address for the proxy changed from 0.0.0.0 to localhost. This broke restoring the old proxy port and caused Cilium to always allocate a new proxy port. Fix it by changing the regex string to include the new bind address as well as the previously used "0.0.0.0" and "::", for backwards-compatibility reasons on upgrade. Found by code inspection. [1]: #25309 Fixes: 5304088 ("dnsproxy: bind dns proxy to localhost only") Fixes: #25309 Signed-off-by: Chris Tarazi <chris@isovalent.com>
christarazi
added a commit
that referenced
this pull request
Sep 13, 2023
[ upstream commit 1f8e015 ] In a previous change [1], the bind address for the proxy changed from 0.0.0.0 to localhost. This broke restoring the old proxy port and caused Cilium to always allocate a new proxy port. Fix it by changing the regex string to include the new bind address as well as the previously used "0.0.0.0" and "::", for backwards-compatibility reasons on upgrade. Found by code inspection. [1]: #25309 Fixes: 5304088 ("dnsproxy: bind dns proxy to localhost only") Fixes: #25309 Signed-off-by: Chris Tarazi <chris@isovalent.com>
mhofstetter
added a commit
to mhofstetter/cilium
that referenced
this pull request
Sep 15, 2023
PR cilium#25309 introduced a data race by sharing the sessionUDPFactory between the DNS server instances for the different IP families (IPv4 & IPv6). This has been detected by cilium#27979. This commit fixes the data race by using dedicated instances of the sessionUDPFactory. Fixes: cilium#28156 Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
aanm
pushed a commit
that referenced
this pull request
Sep 18, 2023
PR #25309 introduced a data race by sharing the sessionUDPFactory between the DNS server instances for the different IP families (IPv4 & IPv6). This has been detected by #27979. This commit fixes the data race by using dedicated instances of the sessionUDPFactory. Fixes: #28156 Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
mhofstetter
added a commit
to mhofstetter/cilium
that referenced
this pull request
Sep 19, 2023
PR cilium#25309 introduced a data race by sharing the sessionUDPFactory between the DNS server instances for the different IP families (IPv4 & IPv6). This has been detected by cilium#27979. This commit fixes the issue for the TCP servers too. It not set explicitly, these are initialized with the default udp session factory to prevent nil pointer exceptions. Therefore, an explicit noop udp session factory is set. Fixes: cilium#28156 Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
aanm
pushed a commit
that referenced
this pull request
Sep 25, 2023
PR #25309 introduced a data race by sharing the sessionUDPFactory between the DNS server instances for the different IP families (IPv4 & IPv6). This has been detected by #27979. This commit fixes the issue for the TCP servers too. It not set explicitly, these are initialized with the default udp session factory to prevent nil pointer exceptions. Therefore, an explicit noop udp session factory is set. Fixes: #28156 Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
giorio94
pushed a commit
that referenced
this pull request
Sep 26, 2023
[ upstream commit fb6bd90 ] PR #25309 introduced a data race by sharing the sessionUDPFactory between the DNS server instances for the different IP families (IPv4 & IPv6). This has been detected by #27979. This commit fixes the data race by using dedicated instances of the sessionUDPFactory. Fixes: #28156 Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com> Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
giorio94
pushed a commit
that referenced
this pull request
Sep 26, 2023
[ upstream commit f73e1c5 ] [ backporter's notes: switched cilium/dns import to miekg/dns, as v1.14 was relying on a replace directive instead of pointing to the fork. ] PR #25309 introduced a data race by sharing the sessionUDPFactory between the DNS server instances for the different IP families (IPv4 & IPv6). This has been detected by #27979. This commit fixes the issue for the TCP servers too. It not set explicitly, these are initialized with the default udp session factory to prevent nil pointer exceptions. Therefore, an explicit noop udp session factory is set. Fixes: #28156 Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com> Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
aanm
pushed a commit
that referenced
this pull request
Sep 29, 2023
[ upstream commit fb6bd90 ] PR #25309 introduced a data race by sharing the sessionUDPFactory between the DNS server instances for the different IP families (IPv4 & IPv6). This has been detected by #27979. This commit fixes the data race by using dedicated instances of the sessionUDPFactory. Fixes: #28156 Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com> Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
aanm
pushed a commit
that referenced
this pull request
Sep 29, 2023
[ upstream commit f73e1c5 ] [ backporter's notes: switched cilium/dns import to miekg/dns, as v1.14 was relying on a replace directive instead of pointing to the fork. ] PR #25309 introduced a data race by sharing the sessionUDPFactory between the DNS server instances for the different IP families (IPv4 & IPv6). This has been detected by #27979. This commit fixes the issue for the TCP servers too. It not set explicitly, these are initialized with the default udp session factory to prevent nil pointer exceptions. Therefore, an explicit noop udp session factory is set. Fixes: #28156 Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com> Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
gandro
pushed a commit
to gandro/cilium
that referenced
this pull request
Oct 10, 2023
[ upstream commit fb6bd90 ] PR cilium#25309 introduced a data race by sharing the sessionUDPFactory between the DNS server instances for the different IP families (IPv4 & IPv6). This has been detected by cilium#27979. This commit fixes the data race by using dedicated instances of the sessionUDPFactory. Fixes: cilium#28156 Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com> Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
gandro
pushed a commit
to gandro/cilium
that referenced
this pull request
Oct 10, 2023
[ upstream commit f73e1c5 ] [ backporter's notes: switched cilium/dns import to miekg/dns, as v1.14 was relying on a replace directive instead of pointing to the fork. ] PR cilium#25309 introduced a data race by sharing the sessionUDPFactory between the DNS server instances for the different IP families (IPv4 & IPv6). This has been detected by cilium#27979. This commit fixes the issue for the TCP servers too. It not set explicitly, these are initialized with the default udp session factory to prevent nil pointer exceptions. Therefore, an explicit noop udp session factory is set. Fixes: cilium#28156 Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com> Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently the dns proxy binds to all interfaces.
With this commit, the dns proxy only gets bound to the localhost interfaces. Therefore, up to 4 DNS servers are created (udpv4, tcpv4, udpv6, tcpv6 - depending on the configuration) which all are using the same DNS handler.
Fixes parts of #23353